From c720dc30dad301e265505294d0874f112ae89070 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 1 Jul 2026 11:40:53 -0700 Subject: [PATCH 1/2] add update test results mcps --- .../sift_cli/assets/docs/src/agents/mcp.md | 3 + .../assets/skills/agents-md/AGENTS.md | 4 + .../assets/skills/claude-code/SKILL.md | 4 + .../sift_mcp/src/service/test_reports/mod.rs | 303 ++++++++++- .../sift_mcp/src/service/test_reports/spec.rs | 28 + .../sift_mcp/src/service/test_reports/test.rs | 327 ++++++++++- .../sift_mcp/src/tool/test_reports/mod.rs | 515 +++++++++++++++++- .../sift_mcp/src/tool/test_reports/test.rs | 207 ++++++- 8 files changed, 1380 insertions(+), 11 deletions(-) diff --git a/rust/crates/sift_cli/assets/docs/src/agents/mcp.md b/rust/crates/sift_cli/assets/docs/src/agents/mcp.md index 87131744e..52f366ceb 100644 --- a/rust/crates/sift_cli/assets/docs/src/agents/mcp.md +++ b/rust/crates/sift_cli/assets/docs/src/agents/mcp.md @@ -51,6 +51,9 @@ not run interactively. | `count_test_measurements` | Count test measurements matching a filter, without fetching them. | | `create_test_report` | Create a test report with its steps and measurements from a JSON document. | | `append_test_measurements` | Append measurements to an existing test step. | +| `update_test_report` | Update a test report's fields (status, name, times, run link, archive). | +| `update_test_step` | Update a test step's fields (name, status, times, error info, metadata). | +| `update_test_measurement` | Update a test measurement's value, bounds, verdict, unit, or metadata. | | `get_data` | Download channel data for an asset/run to a Parquet file, with optional decimation. | | `sql` | Run SQL over one or more Parquet files; chain after `get_data` for analysis. | | `upload_dataset` | Stream a Parquet dataset into Sift. | diff --git a/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md b/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md index 5b05d0f55..33b6dac2a 100644 --- a/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md +++ b/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md @@ -25,6 +25,10 @@ to combine them when working with Sift. - `create_test_report`, `append_test_measurements`: create a test report with its step/measurement tree, or add measurements to an existing step (writes — confirm with the user first). + - `update_test_report`, `update_test_step`, `update_test_measurement`: update + fields of an existing report, step, or measurement (writes — only the fields + you set change; metadata/channel_names use replace semantics; confirm current + versus proposed values with the user first). - `get_data`: download channel data for an asset/run to a Parquet file. - `sql`: run SQL over one or more Parquet files (chain after `get_data`). - `upload_dataset`: stream a Parquet dataset into Sift. Returns an diff --git a/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md b/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md index 848221d28..9c27e2eb7 100644 --- a/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md @@ -41,6 +41,10 @@ to combine them when working with Sift. - `create_test_report`, `append_test_measurements`: create a test report with its step/measurement tree, or add measurements to an existing step (writes — confirm with the user first). + - `update_test_report`, `update_test_step`, `update_test_measurement`: update + fields of an existing report, step, or measurement (writes — only the fields + you set change; metadata/channel_names use replace semantics; confirm current + versus proposed values with the user first). - `get_data`: download channel data for an asset/run to a Parquet file. - `sql`: run SQL over one or more Parquet files (chain after `get_data`). - `upload_dataset`: stream a Parquet dataset into Sift. Returns an diff --git a/rust/crates/sift_mcp/src/service/test_reports/mod.rs b/rust/crates/sift_mcp/src/service/test_reports/mod.rs index 492c24e55..ed922aec4 100644 --- a/rust/crates/sift_mcp/src/service/test_reports/mod.rs +++ b/rust/crates/sift_mcp/src/service/test_reports/mod.rs @@ -4,15 +4,19 @@ use std::collections::HashMap; use crate::policy::{RetryPolicy, with_retry}; use crate::service::common; use anyhow::{Context, Result, anyhow}; +use pbjson_types::{FieldMask, Timestamp}; use sift_rs::{ SiftChannel, + metadata::v1::MetadataValue, test_reports::v1::{ CountTestMeasurementsRequest, CountTestStepsRequest, CreateTestMeasurementsRequest, - CreateTestStepRequest, ListTestMeasurementsRequest, ListTestMeasurementsResponse, - ListTestReportsRequest, ListTestReportsResponse, ListTestStepsRequest, - ListTestStepsResponse, TestMeasurement, TestReport, TestStep, - test_report_service_client::TestReportServiceClient, + CreateTestStepRequest, ErrorInfo, ListTestMeasurementsRequest, + ListTestMeasurementsResponse, ListTestReportsRequest, ListTestReportsResponse, + ListTestStepsRequest, ListTestStepsResponse, TestMeasurement, TestReport, TestStep, + UpdateTestMeasurementRequest, UpdateTestReportRequest, UpdateTestStepRequest, + test_measurement, test_report_service_client::TestReportServiceClient, }, + unit::v2::Unit, }; pub mod spec; @@ -484,4 +488,295 @@ impl TestReportService { Ok(resp.measurements_created_count as usize) } + + /// Update a subset of an existing test report's fields. Per + /// `protos/sift/test_reports/v1/test_reports.proto::UpdateTestReportRequest` the updatable + /// fields are `status`, `name`, `test_system_name`, `test_case`, `start_time`, `end_time`, + /// `serial_number`, `part_number`, `system_operator`, `run_id`, and `is_archived`. Only the + /// provided fields are written; the field mask always matches what was sent, so an omitted + /// field is left untouched. + #[allow(clippy::too_many_arguments)] + pub async fn update_test_report( + &self, + test_report_id: String, + status: Option, + name: Option, + test_system_name: Option, + test_case: Option, + start_time: Option, + end_time: Option, + serial_number: Option, + part_number: Option, + system_operator: Option, + run_id: Option, + is_archived: Option, + ) -> Result { + let mut report = TestReport { + test_report_id, + ..Default::default() + }; + let mut paths = Vec::new(); + + if let Some(v) = status { + report.status = v; + paths.push("status".to_string()); + } + if let Some(v) = name { + report.name = v; + paths.push("name".to_string()); + } + if let Some(v) = test_system_name { + report.test_system_name = v; + paths.push("test_system_name".to_string()); + } + if let Some(v) = test_case { + report.test_case = v; + paths.push("test_case".to_string()); + } + if let Some(v) = start_time { + report.start_time = Some(v); + paths.push("start_time".to_string()); + } + if let Some(v) = end_time { + report.end_time = Some(v); + paths.push("end_time".to_string()); + } + if let Some(v) = serial_number { + report.serial_number = v; + paths.push("serial_number".to_string()); + } + if let Some(v) = part_number { + report.part_number = v; + paths.push("part_number".to_string()); + } + if let Some(v) = system_operator { + report.system_operator = v; + paths.push("system_operator".to_string()); + } + if let Some(v) = run_id { + report.run_id = v; + paths.push("run_id".to_string()); + } + if let Some(v) = is_archived { + report.is_archived = v; + paths.push("is_archived".to_string()); + } + + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let report = report.clone(); + let paths = paths.clone(); + async move { + let mut client = TestReportServiceClient::new(channel); + client + .update_test_report(UpdateTestReportRequest { + test_report: Some(report), + update_mask: Some(FieldMask { paths }), + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to update test report")?; + + resp.test_report + .ok_or_else(|| anyhow!("update_test_report response missing report")) + } + + /// Update a subset of an existing test step's fields. Per + /// `protos/sift/test_reports/v1/test_reports.proto::UpdateTestStepRequest` the updatable + /// fields are `name`, `description`, `step_type`, `step_path`, `status`, `start_time`, + /// `end_time`, `error_info`, and `metadata`. `metadata` uses REPLACE semantics: the supplied + /// list replaces all existing entries, and `Some(vec![])` clears them. (The proto mask comment + /// also lists `test_case`, but `TestStep` has no such field, so it is not exposed.) + #[allow(clippy::too_many_arguments)] + pub async fn update_test_step( + &self, + test_step_id: String, + name: Option, + description: Option, + step_type: Option, + step_path: Option, + status: Option, + start_time: Option, + end_time: Option, + error_info: Option, + metadata: Option>, + ) -> Result { + let mut step = TestStep { + test_step_id, + ..Default::default() + }; + let mut paths = Vec::new(); + + if let Some(v) = name { + step.name = v; + paths.push("name".to_string()); + } + if let Some(v) = description { + step.description = v; + paths.push("description".to_string()); + } + if let Some(v) = step_type { + step.step_type = v; + paths.push("step_type".to_string()); + } + if let Some(v) = step_path { + step.step_path = v; + paths.push("step_path".to_string()); + } + if let Some(v) = status { + step.status = v; + paths.push("status".to_string()); + } + if let Some(v) = start_time { + step.start_time = Some(v); + paths.push("start_time".to_string()); + } + if let Some(v) = end_time { + step.end_time = Some(v); + paths.push("end_time".to_string()); + } + if let Some(v) = error_info { + step.error_info = Some(v); + paths.push("error_info".to_string()); + } + if let Some(v) = metadata { + step.metadata = v; + paths.push("metadata".to_string()); + } + + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let step = step.clone(); + let paths = paths.clone(); + async move { + let mut client = TestReportServiceClient::new(channel); + client + .update_test_step(UpdateTestStepRequest { + test_step: Some(step), + update_mask: Some(FieldMask { paths }), + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to update test step")?; + + resp.test_step + .ok_or_else(|| anyhow!("update_test_step response missing step")) + } + + /// Update a subset of an existing test measurement's fields. Per + /// `protos/sift/test_reports/v1/test_reports.proto::UpdateTestMeasurementRequest` the updatable + /// fields are `name`, `measurement_type`, the value (`numeric_value`/`string_value`/ + /// `boolean_value`), `unit`, the bounds (`numeric_bounds`/`string_bounds`), `passed`, + /// `timestamp`, `description`, `channel_names`, and `metadata`. `value` and `bounds` are proto + /// oneofs; the mask names the specific arm that was set. `channel_names` and `metadata` use + /// REPLACE semantics. + #[allow(clippy::too_many_arguments)] + pub async fn update_test_measurement( + &self, + measurement_id: String, + name: Option, + measurement_type: Option, + value: Option, + unit: Option, + bounds: Option, + passed: Option, + timestamp: Option, + description: Option, + channel_names: Option>, + metadata: Option>, + ) -> Result { + let mut measurement = TestMeasurement { + measurement_id, + ..Default::default() + }; + let mut paths = Vec::new(); + + if let Some(v) = name { + measurement.name = v; + paths.push("name".to_string()); + } + if let Some(v) = measurement_type { + measurement.measurement_type = v; + paths.push("measurement_type".to_string()); + } + if let Some(v) = value { + // The mask names the specific oneof arm the caller set. + paths.push( + match v { + test_measurement::Value::NumericValue(_) => "numeric_value", + test_measurement::Value::StringValue(_) => "string_value", + test_measurement::Value::BooleanValue(_) => "boolean_value", + } + .to_string(), + ); + measurement.value = Some(v); + } + if let Some(v) = unit { + measurement.unit = Some(Unit { + abbreviated_name: v, + ..Default::default() + }); + paths.push("unit".to_string()); + } + if let Some(v) = bounds { + paths.push( + match v { + test_measurement::Bounds::NumericBounds(_) => "numeric_bounds", + test_measurement::Bounds::StringBounds(_) => "string_bounds", + } + .to_string(), + ); + measurement.bounds = Some(v); + } + if let Some(v) = passed { + measurement.passed = v; + paths.push("passed".to_string()); + } + if let Some(v) = timestamp { + measurement.timestamp = Some(v); + paths.push("timestamp".to_string()); + } + if let Some(v) = description { + measurement.description = v; + paths.push("description".to_string()); + } + if let Some(v) = channel_names { + measurement.channel_names = v; + paths.push("channel_names".to_string()); + } + if let Some(v) = metadata { + measurement.metadata = v; + paths.push("metadata".to_string()); + } + + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let measurement = measurement.clone(); + let paths = paths.clone(); + async move { + let mut client = TestReportServiceClient::new(channel); + client + .update_test_measurement(UpdateTestMeasurementRequest { + test_measurement: Some(measurement), + update_mask: Some(FieldMask { paths }), + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to update test measurement")?; + + resp.test_measurement + .ok_or_else(|| anyhow!("update_test_measurement response missing measurement")) + } } diff --git a/rust/crates/sift_mcp/src/service/test_reports/spec.rs b/rust/crates/sift_mcp/src/service/test_reports/spec.rs index 142590308..bfa17fad2 100644 --- a/rust/crates/sift_mcp/src/service/test_reports/spec.rs +++ b/rust/crates/sift_mcp/src/service/test_reports/spec.rs @@ -382,6 +382,34 @@ where .ok_or_else(|| anyhow!("unknown `{field}` value `{raw}`")) } +/// Parse an optional `TestStatus` name (`"PASSED"` or `"TEST_STATUS_PASSED"`) to its `i32` tag. +/// Shared with the update tools so create and update accept the same enum spellings. +pub fn parse_status(raw: Option<&str>) -> Result> { + parse_enum(raw, "TEST_STATUS_", "status", |n| { + TestStatus::from_str_name(n).map(|e| e as i32) + }) +} + +/// Parse an optional `TestStepType` name to its `i32` tag. +pub fn parse_step_type(raw: Option<&str>) -> Result> { + parse_enum(raw, "TEST_STEP_TYPE_", "step_type", |n| { + TestStepType::from_str_name(n).map(|e| e as i32) + }) +} + +/// Parse an optional `TestMeasurementType` name to its `i32` tag. +pub fn parse_measurement_type(raw: Option<&str>) -> Result> { + parse_enum(raw, "TEST_MEASUREMENT_TYPE_", "measurement_type", |n| { + TestMeasurementType::from_str_name(n).map(|e| e as i32) + }) +} + +/// Parse an optional RFC3339 timestamp string to a proto `Timestamp`. `field` names the +/// parameter for the error message. +pub fn parse_timestamp(raw: Option<&str>, field: &str) -> Result> { + parse_ts(raw, field) +} + fn parse_ts(raw: Option<&str>, field: &str) -> Result> { let Some(raw) = raw else { return Ok(None); diff --git a/rust/crates/sift_mcp/src/service/test_reports/test.rs b/rust/crates/sift_mcp/src/service/test_reports/test.rs index efb228601..c66b71add 100644 --- a/rust/crates/sift_mcp/src/service/test_reports/test.rs +++ b/rust/crates/sift_mcp/src/service/test_reports/test.rs @@ -1,9 +1,11 @@ +use pbjson_types::Timestamp; use sift_rs::test_reports::v1::{ CountTestMeasurementsResponse, CountTestStepsResponse, CreateTestMeasurementsResponse, - CreateTestReportResponse, CreateTestStepResponse, ListTestMeasurementsResponse, - ListTestReportsResponse, ListTestStepsResponse, TestMeasurement, TestMeasurementType, - TestReport, TestStatus, TestStep, TestStepType, test_measurement, - test_report_service_server::TestReportServiceServer, + CreateTestReportResponse, CreateTestStepResponse, ErrorInfo, ListTestMeasurementsResponse, + ListTestReportsResponse, ListTestStepsResponse, NumericBounds, StringBounds, TestMeasurement, + TestMeasurementType, TestReport, TestStatus, TestStep, TestStepType, + UpdateTestMeasurementResponse, UpdateTestReportResponse, UpdateTestStepResponse, + test_measurement, test_report_service_server::TestReportServiceServer, }; use sift_test_util::{ grpc::memory_sift_channel, mock::test_reports::v1::MockTestReportServiceImpl, @@ -652,3 +654,320 @@ async fn count_test_steps_forwards_short_enum_form() { assert_eq!(count, 7); } + +// --- update_test_report --- + +#[tokio::test] +async fn update_test_report_builds_mask_from_provided_fields() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_report() + .withf(|req| { + let req = req.get_ref(); + let report = req.test_report.as_ref().unwrap(); + let paths = &req.update_mask.as_ref().unwrap().paths; + report.test_report_id == "r1" + && report.name == "renamed" + && report.status == TestStatus::Failed as i32 + && report.is_archived + && report.start_time.as_ref().map(|t| t.seconds) == Some(1) + && paths + == &vec![ + "status".to_string(), + "name".to_string(), + "start_time".to_string(), + "is_archived".to_string(), + ] + }) + .returning(|_| { + Ok(Response::new(UpdateTestReportResponse { + test_report: Some(TestReport { + test_report_id: "r1".into(), + name: "renamed".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let report = service + .update_test_report( + "r1".to_string(), + Some(TestStatus::Failed as i32), + Some("renamed".to_string()), + None, + None, + Some(Timestamp { + seconds: 1, + nanos: 0, + }), + None, + None, + None, + None, + None, + Some(true), + ) + .await + .expect("update_test_report failed"); + + assert_eq!(report.name, "renamed"); +} + +#[tokio::test] +async fn update_test_report_propagates_grpc_error() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_report() + .returning(|_| Err(Status::not_found("no such report"))); + + let (service, _h) = service_with_mock(mock).await; + + let err = service + .update_test_report( + "r1".to_string(), + None, + Some("x".to_string()), + None, + None, + None, + None, + None, + None, + None, + None, + None, + ) + .await + .expect_err("expected error"); + + assert!(err.to_string().contains("failed to update test report")); +} + +// --- update_test_step --- + +#[tokio::test] +async fn update_test_step_builds_mask_with_error_info_and_metadata() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_step() + .withf(|req| { + let req = req.get_ref(); + let step = req.test_step.as_ref().unwrap(); + let paths = &req.update_mask.as_ref().unwrap().paths; + let error_ok = step + .error_info + .as_ref() + .map(|e| e.error_code == 7 && e.error_message == "boom") + .unwrap_or(false); + step.test_step_id == "s1" + && step.status == TestStatus::Failed as i32 + && error_ok + // Empty metadata list still records the path (REPLACE → clear). + && step.metadata.is_empty() + && paths + == &vec![ + "status".to_string(), + "error_info".to_string(), + "metadata".to_string(), + ] + }) + .returning(|_| { + Ok(Response::new(UpdateTestStepResponse { + test_step: Some(TestStep { + test_step_id: "s1".into(), + test_report_id: "r1".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let step = service + .update_test_step( + "s1".to_string(), + None, + None, + None, + None, + Some(TestStatus::Failed as i32), + None, + None, + Some(ErrorInfo { + error_code: 7, + error_message: "boom".into(), + }), + Some(vec![]), + ) + .await + .expect("update_test_step failed"); + + assert_eq!(step.test_report_id, "r1"); +} + +#[tokio::test] +async fn update_test_step_propagates_grpc_error() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_step() + .returning(|_| Err(Status::not_found("no such step"))); + + let (service, _h) = service_with_mock(mock).await; + + let err = service + .update_test_step( + "s1".to_string(), + Some("x".to_string()), + None, + None, + None, + None, + None, + None, + None, + None, + ) + .await + .expect_err("expected error"); + + assert!(err.to_string().contains("failed to update test step")); +} + +// --- update_test_measurement --- + +#[tokio::test] +async fn update_test_measurement_masks_value_and_numeric_bounds() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_measurement() + .withf(|req| { + let req = req.get_ref(); + let m = req.test_measurement.as_ref().unwrap(); + let paths = &req.update_mask.as_ref().unwrap().paths; + let value_ok = matches!( + m.value, + Some(test_measurement::Value::NumericValue(v)) if v == 3.3 + ); + let bounds_ok = matches!( + &m.bounds, + Some(test_measurement::Bounds::NumericBounds(b)) + if b.min == Some(3.0) && b.max == Some(3.6) + ); + m.measurement_id == "m1" + && value_ok + && bounds_ok + && m.passed + && paths + == &vec![ + "numeric_value".to_string(), + "numeric_bounds".to_string(), + "passed".to_string(), + ] + }) + .returning(|_| { + Ok(Response::new(UpdateTestMeasurementResponse { + test_measurement: Some(TestMeasurement { + measurement_id: "m1".into(), + test_report_id: "r1".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let m = service + .update_test_measurement( + "m1".to_string(), + None, + None, + Some(test_measurement::Value::NumericValue(3.3)), + None, + Some(test_measurement::Bounds::NumericBounds(NumericBounds { + min: Some(3.0), + max: Some(3.6), + })), + Some(true), + None, + None, + None, + None, + ) + .await + .expect("update_test_measurement failed"); + + assert_eq!(m.test_report_id, "r1"); +} + +#[tokio::test] +async fn update_test_measurement_masks_string_bounds() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_measurement() + .withf(|req| { + let req = req.get_ref(); + let m = req.test_measurement.as_ref().unwrap(); + let paths = &req.update_mask.as_ref().unwrap().paths; + let bounds_ok = matches!( + &m.bounds, + Some(test_measurement::Bounds::StringBounds(b)) if b.expected_value == "ok" + ); + bounds_ok && paths == &vec!["string_bounds".to_string()] + }) + .returning(|_| { + Ok(Response::new(UpdateTestMeasurementResponse { + test_measurement: Some(TestMeasurement { + measurement_id: "m1".into(), + ..Default::default() + }), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + service + .update_test_measurement( + "m1".to_string(), + None, + None, + None, + None, + Some(test_measurement::Bounds::StringBounds(StringBounds { + expected_value: "ok".into(), + })), + None, + None, + None, + None, + None, + ) + .await + .expect("update_test_measurement failed"); +} + +#[tokio::test] +async fn update_test_measurement_propagates_grpc_error() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_measurement() + .returning(|_| Err(Status::not_found("no such measurement"))); + + let (service, _h) = service_with_mock(mock).await; + + let err = service + .update_test_measurement( + "m1".to_string(), + Some("x".to_string()), + None, + None, + None, + None, + None, + None, + None, + None, + None, + ) + .await + .expect_err("expected error"); + + assert!( + err.to_string() + .contains("failed to update test measurement") + ); +} 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 b0469aeba..5456acf4e 100644 --- a/rust/crates/sift_mcp/src/tool/test_reports/mod.rs +++ b/rust/crates/sift_mcp/src/tool/test_reports/mod.rs @@ -6,12 +6,16 @@ use rmcp::{ tool, tool_router, }; use serde::Deserialize; +use sift_rs::{ + metadata::v1::MetadataValue, + test_reports::v1::{ErrorInfo, NumericBounds, StringBounds, test_measurement}, +}; use crate::{ error::{self, from_anyhow}, server::SiftMcpServer, service::test_reports::spec::{self, ReportSpec}, - tool::common::{ListParams, url_clause}, + tool::common::{ListParams, MetadataEntry, url_clause}, }; #[cfg(test)] @@ -42,6 +46,56 @@ pub struct AppendMeasurementsParams { pub(crate) measurements_json: String, } +#[derive(Debug, Deserialize, JsonSchema)] +pub struct UpdateTestReportParams { + pub(crate) test_report_id: String, + pub(crate) status: Option, + pub(crate) name: Option, + pub(crate) test_system_name: Option, + pub(crate) test_case: Option, + pub(crate) start_time: Option, + pub(crate) end_time: Option, + pub(crate) serial_number: Option, + pub(crate) part_number: Option, + pub(crate) system_operator: Option, + pub(crate) run_id: Option, + pub(crate) is_archived: Option, +} + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct UpdateTestStepParams { + pub(crate) test_step_id: String, + pub(crate) name: Option, + pub(crate) description: Option, + pub(crate) step_type: Option, + pub(crate) step_path: Option, + pub(crate) status: Option, + pub(crate) start_time: Option, + pub(crate) end_time: Option, + pub(crate) error_code: Option, + pub(crate) error_message: Option, + pub(crate) metadata: Option>, +} + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct UpdateTestMeasurementParams { + pub(crate) measurement_id: String, + pub(crate) name: Option, + pub(crate) measurement_type: Option, + pub(crate) numeric_value: Option, + pub(crate) string_value: Option, + pub(crate) boolean_value: Option, + pub(crate) unit: Option, + pub(crate) numeric_bounds_min: Option, + pub(crate) numeric_bounds_max: Option, + pub(crate) string_expected: Option, + pub(crate) passed: Option, + pub(crate) timestamp: Option, + pub(crate) description: Option, + pub(crate) channel_names: Option>, + pub(crate) metadata: Option>, +} + #[tool_router(router = test_reports_router, vis = "pub(crate)")] impl SiftMcpServer { #[tool( @@ -467,4 +521,463 @@ impl SiftMcpServer { result.content = vec![Content::text(next_step)]; Ok(result) } + + #[tool( + name = "update_test_report", + description = " + Update specific fields of an existing test report, identified by `test_report_id`. This is a + WRITE. Only the fields you set are changed; every other field on the report is preserved. + Wraps `test_reports/v1 UpdateTestReport`. + + Output: + - `{ \"test_report\": TestReport, \"report_url\": string|null, \"next_step\": string }`. The + returned `TestReport` is the post-update state from the server; `report_url` is its Sift + web link (`/test-results/`), or null when the host can't be derived. + + Parameters: + - `test_report_id`: required; the id of the report to update. + - `status`: optional; one of `PASSED`, `FAILED`, `ABORTED`, `ERROR`, `IN_PROGRESS`, + `SKIPPED`, `DRAFT` (the `TEST_STATUS_` prefix is optional). + - `name`, `test_system_name`, `test_case`: optional new string values. + - `start_time` / `end_time`: optional RFC3339 timestamps, e.g. \"2026-06-24T10:00:00Z\". + - `serial_number`, `part_number`, `system_operator`: optional new string values. + - `run_id`: optional; links the report to a different Sift run (empty string to unlink). + - `is_archived`: optional; archive (`true`) or unarchive (`false`) the report. + + At least one updatable field besides `test_report_id` must be set; otherwise the tool returns + `INVALID_PARAMS`. + + Errors: + - `INVALID_PARAMS` if `test_report_id` is empty, no updatable field is provided, `status` is + an unknown enum value, or a timestamp is not RFC3339. + - `RESOURCE_NOT_FOUND` if no report matches `test_report_id`. + - `INTERNAL_ERROR` for upstream gRPC failures. + + Guidance: + - CONFIRM the change with the user before invoking: read the current report first (via + `list_test_reports` filtered by `test_report_id == \"...\"`) and show them the current value + versus the proposed value for each field you intend to change. + ", + annotations( + title = "test_reports_router/update_test_report", + read_only_hint = false + ) + )] + pub async fn update_test_report( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(UpdateTestReportParams { + test_report_id, + status, + name, + test_system_name, + test_case, + start_time, + end_time, + serial_number, + part_number, + system_operator, + run_id, + is_archived, + }) = params; + + if test_report_id.is_empty() { + return Err(ErrorData::invalid_params( + "`test_report_id` must not be empty", + None, + )); + } + + let has_update = status.is_some() + || name.is_some() + || test_system_name.is_some() + || test_case.is_some() + || start_time.is_some() + || end_time.is_some() + || serial_number.is_some() + || part_number.is_some() + || system_operator.is_some() + || run_id.is_some() + || is_archived.is_some(); + if !has_update { + return Err(ErrorData::invalid_params( + "at least one updatable field besides `test_report_id` must be provided", + None, + )); + } + + let status = spec::parse_status(status.as_deref()) + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let start_time = spec::parse_timestamp(start_time.as_deref(), "start_time") + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let end_time = spec::parse_timestamp(end_time.as_deref(), "end_time") + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + + let report = self + .test_report_service + .update_test_report( + test_report_id, + status, + name, + test_system_name, + test_case, + start_time, + end_time, + serial_number, + part_number, + system_operator, + run_id, + is_archived, + ) + .await + .map_err(from_anyhow)?; + + let report_url = self + .url_service + .build_test_report_url(&report.test_report_id) + .ok(); + let next_step = format!( + "Updated test report `{}` ({}).{} Surface the new state to the user and confirm the change \ + matches their intent.", + report.name, + report.test_report_id, + url_clause(report_url.as_deref()), + ); + + let mut result = CallToolResult::structured(serde_json::json!({ + "test_report": report, + "report_url": report_url, + "next_step": next_step, + })); + result.content = vec![Content::text(next_step)]; + Ok(result) + } + + #[tool( + name = "update_test_step", + description = " + Update specific fields of an existing test step, identified by `test_step_id`. This is a WRITE. + Only the fields you set are changed; every other field on the step is preserved. Wraps + `test_reports/v1 UpdateTestStep`. + + Output: + - `{ \"test_step\": TestStep, \"report_url\": string|null, \"next_step\": string }`. The + returned `TestStep` is the post-update state; `report_url` links the step's report in the + Sift web app, or null when the host can't be derived. + + Parameters: + - `test_step_id`: required; the id of the step to update. + - `name`, `description`: optional new string values. + - `step_type`: optional; one of `SEQUENCE`, `GROUP`, `ACTION`, `FLOW_CONTROL` (the + `TEST_STEP_TYPE_` prefix is optional). + - `step_path`: optional new hierarchical path (e.g. \"1.2\"). Editing this can desync the step + tree — change it only when deliberately restructuring. + - `status`: optional; see `update_test_report` for the accepted values. + - `start_time` / `end_time`: optional RFC3339 timestamps. + - `error_code` / `error_message`: optional; MUST be provided together — they set the step's + `error_info`. `error_info` is diagnostic only and does not by itself mean the step failed. + - `metadata`: optional; REPLACES the full metadata list. Each entry is + `{ \"name\": \"\", \"value\": }`. Pass `[]` to clear. + + At least one updatable field besides `test_step_id` must be set; otherwise the tool returns + `INVALID_PARAMS`. + + Errors: + - `INVALID_PARAMS` if `test_step_id` is empty, no updatable field is provided, only one of + `error_code`/`error_message` is set, `step_type`/`status` is unknown, or a timestamp is not + RFC3339. + - `RESOURCE_NOT_FOUND` if no step matches `test_step_id`. + - `INTERNAL_ERROR` for upstream gRPC failures. + + Guidance: + - CONFIRM before invoking: read the current step (via `list_test_steps` filtered by + `test_step_id == \"...\"`) and show the user current versus proposed values. `metadata` is a + REPLACE, so read-modify-write when appending. + ", + annotations(title = "test_reports_router/update_test_step", read_only_hint = false) + )] + pub async fn update_test_step( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(UpdateTestStepParams { + test_step_id, + name, + description, + step_type, + step_path, + status, + start_time, + end_time, + error_code, + error_message, + metadata, + }) = params; + + if test_step_id.is_empty() { + return Err(ErrorData::invalid_params( + "`test_step_id` must not be empty", + None, + )); + } + + let error_info = match (error_code, error_message) { + (Some(error_code), Some(error_message)) => Some(ErrorInfo { + error_code, + error_message, + }), + (None, None) => None, + _ => { + return Err(ErrorData::invalid_params( + "`error_code` and `error_message` must be provided together", + None, + )); + } + }; + + let has_update = name.is_some() + || description.is_some() + || step_type.is_some() + || step_path.is_some() + || status.is_some() + || start_time.is_some() + || end_time.is_some() + || error_info.is_some() + || metadata.is_some(); + if !has_update { + return Err(ErrorData::invalid_params( + "at least one updatable field besides `test_step_id` must be provided", + None, + )); + } + + let step_type = spec::parse_step_type(step_type.as_deref()) + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let status = spec::parse_status(status.as_deref()) + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let start_time = spec::parse_timestamp(start_time.as_deref(), "start_time") + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let end_time = spec::parse_timestamp(end_time.as_deref(), "end_time") + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let metadata = metadata.map(|m| m.into_iter().map(MetadataValue::from).collect::>()); + + let step = self + .test_report_service + .update_test_step( + test_step_id, + name, + description, + step_type, + step_path, + status, + start_time, + end_time, + error_info, + metadata, + ) + .await + .map_err(from_anyhow)?; + + let report_url = self + .url_service + .build_test_report_url(&step.test_report_id) + .ok(); + let next_step = format!( + "Updated test step `{}` ({}) in report `{}`.{} Surface the new state to the user and confirm \ + it matches their intent. Remember: metadata is a REPLACE.", + step.name, + step.test_step_id, + step.test_report_id, + url_clause(report_url.as_deref()), + ); + + let mut result = CallToolResult::structured(serde_json::json!({ + "test_step": step, + "report_url": report_url, + "next_step": next_step, + })); + result.content = vec![Content::text(next_step)]; + Ok(result) + } + + #[tool( + name = "update_test_measurement", + description = " + Update specific fields of an existing test measurement, identified by `measurement_id`. This is + a WRITE. Only the fields you set are changed; every other field is preserved. Wraps + `test_reports/v1 UpdateTestMeasurement`. + + Output: + - `{ \"test_measurement\": TestMeasurement, \"report_url\": string|null, \"next_step\": string }`. + The returned `TestMeasurement` is the post-update state; `report_url` links its report in + the Sift web app, or null when the host can't be derived. + + Parameters: + - `measurement_id`: required; the id of the measurement to update. + - `name`, `description`: optional new string values. + - `measurement_type`: optional; one of `DOUBLE`, `STRING`, `BOOLEAN`, `LIMIT` (the + `TEST_MEASUREMENT_TYPE_` prefix is optional). + - `numeric_value` / `string_value` / `boolean_value`: optional new value. Set AT MOST ONE; + setting more than one returns `INVALID_PARAMS`. This does not auto-recompute `passed`. + - `unit`: optional unit abbreviation (e.g. \"V\"). + - `numeric_bounds_min` / `numeric_bounds_max`: optional numeric bounds. Setting either sets + `numeric_bounds` (an unset side is unbounded). + - `string_expected`: optional expected string; sets `string_bounds`. Mutually exclusive with + the numeric bounds fields. + - `passed`: optional pass/fail verdict. + - `timestamp`: optional RFC3339 timestamp. + - `channel_names`: optional; REPLACES the full channel-name list. Pass `[]` to clear. + - `metadata`: optional; REPLACES the full metadata list (`{ \"name\": ..., \"value\": }`). + + At least one updatable field besides `measurement_id` must be set; otherwise the tool returns + `INVALID_PARAMS`. + + Errors: + - `INVALID_PARAMS` if `measurement_id` is empty, no updatable field is provided, more than one + value field is set, both numeric and string bounds are set, an enum is unknown, or a + timestamp is not RFC3339. + - `RESOURCE_NOT_FOUND` if no measurement matches `measurement_id`. + - `INTERNAL_ERROR` for upstream gRPC failures. + + Guidance: + - CONFIRM before invoking: read the current measurement (via `list_test_measurements` filtered + by `measurement_id == \"...\"`) and show the user current versus proposed values. Changing a + value does not recompute `passed` — set `passed` explicitly if the verdict should change. + ", + annotations( + title = "test_reports_router/update_test_measurement", + read_only_hint = false + ) + )] + pub async fn update_test_measurement( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(UpdateTestMeasurementParams { + measurement_id, + name, + measurement_type, + numeric_value, + string_value, + boolean_value, + unit, + numeric_bounds_min, + numeric_bounds_max, + string_expected, + passed, + timestamp, + description, + channel_names, + metadata, + }) = params; + + if measurement_id.is_empty() { + return Err(ErrorData::invalid_params( + "`measurement_id` must not be empty", + None, + )); + } + + // At most one value variant. + let value_count = numeric_value.is_some() as u8 + + string_value.is_some() as u8 + + boolean_value.is_some() as u8; + if value_count > 1 { + return Err(ErrorData::invalid_params( + "set at most one of `numeric_value`, `string_value`, or `boolean_value`", + None, + )); + } + let value = if let Some(v) = numeric_value { + Some(test_measurement::Value::NumericValue(v)) + } else if let Some(v) = string_value { + Some(test_measurement::Value::StringValue(v)) + } else { + boolean_value.map(test_measurement::Value::BooleanValue) + }; + + // At most one bounds kind. + let has_numeric_bounds = numeric_bounds_min.is_some() || numeric_bounds_max.is_some(); + if has_numeric_bounds && string_expected.is_some() { + return Err(ErrorData::invalid_params( + "set either numeric bounds (`numeric_bounds_min`/`numeric_bounds_max`) or \ + `string_expected`, not both", + None, + )); + } + let bounds = if has_numeric_bounds { + Some(test_measurement::Bounds::NumericBounds(NumericBounds { + min: numeric_bounds_min, + max: numeric_bounds_max, + })) + } else { + string_expected.map(|expected_value| { + test_measurement::Bounds::StringBounds(StringBounds { expected_value }) + }) + }; + + let has_update = name.is_some() + || measurement_type.is_some() + || value.is_some() + || unit.is_some() + || bounds.is_some() + || passed.is_some() + || timestamp.is_some() + || description.is_some() + || channel_names.is_some() + || metadata.is_some(); + if !has_update { + return Err(ErrorData::invalid_params( + "at least one updatable field besides `measurement_id` must be provided", + None, + )); + } + + let measurement_type = spec::parse_measurement_type(measurement_type.as_deref()) + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let timestamp = spec::parse_timestamp(timestamp.as_deref(), "timestamp") + .map_err(|e| ErrorData::invalid_params(format!("{e}"), None))?; + let metadata = metadata.map(|m| m.into_iter().map(MetadataValue::from).collect::>()); + + let measurement = self + .test_report_service + .update_test_measurement( + measurement_id, + name, + measurement_type, + value, + unit, + bounds, + passed, + timestamp, + description, + channel_names, + metadata, + ) + .await + .map_err(from_anyhow)?; + + let report_url = self + .url_service + .build_test_report_url(&measurement.test_report_id) + .ok(); + let next_step = format!( + "Updated test measurement `{}` ({}) in report `{}`.{} Surface the new state to the user and \ + confirm it matches their intent. Remember: channel_names and metadata are REPLACE \ + operations, and changing a value does not recompute `passed`.", + measurement.name, + measurement.measurement_id, + measurement.test_report_id, + url_clause(report_url.as_deref()), + ); + + let mut result = CallToolResult::structured(serde_json::json!({ + "test_measurement": measurement, + "report_url": report_url, + "next_step": next_step, + })); + result.content = vec![Content::text(next_step)]; + Ok(result) + } } diff --git a/rust/crates/sift_mcp/src/tool/test_reports/test.rs b/rust/crates/sift_mcp/src/tool/test_reports/test.rs index fc1256cce..5a8f50d03 100644 --- a/rust/crates/sift_mcp/src/tool/test_reports/test.rs +++ b/rust/crates/sift_mcp/src/tool/test_reports/test.rs @@ -1,6 +1,7 @@ use rmcp::handler::server::wrapper::Parameters; use sift_rs::test_reports::v1::{ - CreateTestMeasurementsResponse, CreateTestReportResponse, TestReport, + CreateTestMeasurementsResponse, CreateTestReportResponse, TestMeasurement, TestReport, + TestStep, UpdateTestMeasurementResponse, UpdateTestReportResponse, UpdateTestStepResponse, test_report_service_server::TestReportServiceServer, }; use sift_test_util::{ @@ -9,9 +10,66 @@ use sift_test_util::{ use tokio::task::JoinHandle; use tonic::{Response, transport::Server}; -use super::{AppendMeasurementsParams, CreateTestReportParams}; +use super::{ + AppendMeasurementsParams, CreateTestReportParams, UpdateTestMeasurementParams, + UpdateTestReportParams, UpdateTestStepParams, +}; use crate::{server::SiftMcpServer, tool::common::test_support::structured_field}; +/// A `UpdateTestReportParams` with only the id set, so tests set just the fields they exercise. +fn report_params(test_report_id: &str) -> UpdateTestReportParams { + UpdateTestReportParams { + test_report_id: test_report_id.into(), + status: None, + name: None, + test_system_name: None, + test_case: None, + start_time: None, + end_time: None, + serial_number: None, + part_number: None, + system_operator: None, + run_id: None, + is_archived: None, + } +} + +fn step_params(test_step_id: &str) -> UpdateTestStepParams { + UpdateTestStepParams { + test_step_id: test_step_id.into(), + name: None, + description: None, + step_type: None, + step_path: None, + status: None, + start_time: None, + end_time: None, + error_code: None, + error_message: None, + metadata: None, + } +} + +fn measurement_params(measurement_id: &str) -> UpdateTestMeasurementParams { + UpdateTestMeasurementParams { + measurement_id: measurement_id.into(), + name: None, + measurement_type: None, + numeric_value: None, + string_value: None, + boolean_value: None, + unit: None, + numeric_bounds_min: None, + numeric_bounds_max: None, + string_expected: None, + passed: None, + timestamp: None, + description: None, + channel_names: None, + metadata: None, + } +} + async fn server_with_mock(mock: MockTestReportServiceImpl) -> (SiftMcpServer, JoinHandle<()>) { let (client, server) = tokio::io::duplex(1024); let channel = memory_sift_channel(client).await; @@ -56,6 +114,151 @@ async fn create_test_report_surfaces_url() { assert_eq!(report_url, "https://app.test.local/test-results/tr1"); } +#[tokio::test] +async fn update_test_report_rejects_empty_id() { + let (server, _h) = server_with_mock(MockTestReportServiceImpl::new()).await; + + let mut params = report_params(""); + params.name = Some("x".into()); + let err = server + .update_test_report(Parameters(params)) + .await + .expect_err("expected empty-id rejection"); + assert!(err.message.contains("test_report_id")); +} + +#[tokio::test] +async fn update_test_report_rejects_no_fields() { + let (server, _h) = server_with_mock(MockTestReportServiceImpl::new()).await; + + let err = server + .update_test_report(Parameters(report_params("tr1"))) + .await + .expect_err("expected no-fields rejection"); + assert!(err.message.contains("at least one updatable field")); +} + +#[tokio::test] +async fn update_test_report_surfaces_url() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_report().returning(|_| { + Ok(Response::new(UpdateTestReportResponse { + test_report: Some(TestReport { + test_report_id: "tr1".into(), + name: "renamed".into(), + ..Default::default() + }), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let mut params = report_params("tr1"); + params.name = Some("renamed".into()); + let resp = server + .update_test_report(Parameters(params)) + .await + .expect("update_test_report failed"); + + let report_url = structured_field(resp, "report_url"); + assert_eq!(report_url, "https://app.test.local/test-results/tr1"); +} + +#[tokio::test] +async fn update_test_step_rejects_partial_error_info() { + let (server, _h) = server_with_mock(MockTestReportServiceImpl::new()).await; + + let mut params = step_params("ts1"); + params.error_code = Some(7); // error_message missing + let err = server + .update_test_step(Parameters(params)) + .await + .expect_err("expected partial error_info rejection"); + assert!(err.message.contains("error_code` and `error_message")); +} + +#[tokio::test] +async fn update_test_step_surfaces_url() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_step().returning(|_| { + Ok(Response::new(UpdateTestStepResponse { + test_step: Some(TestStep { + test_step_id: "ts1".into(), + test_report_id: "tr1".into(), + name: "step".into(), + ..Default::default() + }), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let mut params = step_params("ts1"); + params.description = Some("note".into()); + let resp = server + .update_test_step(Parameters(params)) + .await + .expect("update_test_step failed"); + + let report_url = structured_field(resp, "report_url"); + assert_eq!(report_url, "https://app.test.local/test-results/tr1"); +} + +#[tokio::test] +async fn update_test_measurement_rejects_multiple_values() { + let (server, _h) = server_with_mock(MockTestReportServiceImpl::new()).await; + + let mut params = measurement_params("m1"); + params.numeric_value = Some(1.0); + params.boolean_value = Some(true); + let err = server + .update_test_measurement(Parameters(params)) + .await + .expect_err("expected multiple-value rejection"); + assert!(err.message.contains("at most one")); +} + +#[tokio::test] +async fn update_test_measurement_rejects_conflicting_bounds() { + let (server, _h) = server_with_mock(MockTestReportServiceImpl::new()).await; + + let mut params = measurement_params("m1"); + params.numeric_bounds_max = Some(5.0); + params.string_expected = Some("ok".into()); + let err = server + .update_test_measurement(Parameters(params)) + .await + .expect_err("expected conflicting-bounds rejection"); + assert!(err.message.contains("not both")); +} + +#[tokio::test] +async fn update_test_measurement_surfaces_url() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_update_test_measurement().returning(|_| { + Ok(Response::new(UpdateTestMeasurementResponse { + test_measurement: Some(TestMeasurement { + measurement_id: "m1".into(), + test_report_id: "tr1".into(), + name: "meas".into(), + ..Default::default() + }), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let mut params = measurement_params("m1"); + params.passed = Some(false); + let resp = server + .update_test_measurement(Parameters(params)) + .await + .expect("update_test_measurement failed"); + + let report_url = structured_field(resp, "report_url"); + assert_eq!(report_url, "https://app.test.local/test-results/tr1"); +} + #[tokio::test] async fn append_test_measurements_surfaces_url() { let mut mock = MockTestReportServiceImpl::new(); From 0729052f77f08bcc4387bf7659a15152640fb1e9 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Wed, 1 Jul 2026 14:35:30 -0700 Subject: [PATCH 2/2] add test results report auditing --- .../sift_cli/assets/docs/src/agents/mcp.md | 1 + .../assets/docs/src/agents/prompts.md | 34 +- .../assets/skills/agents-md/AGENTS.md | 4 + .../assets/skills/claude-code/SKILL.md | 4 + rust/crates/sift_mcp/src/prompt/mod.rs | 64 ++++ .../src/service/test_reports/export.rs | 332 ++++++++++++++++++ .../sift_mcp/src/service/test_reports/mod.rs | 30 ++ .../sift_mcp/src/service/test_reports/test.rs | 268 ++++++++++++++ .../sift_mcp/src/tool/test_reports/mod.rs | 122 +++++++ .../sift_mcp/src/tool/test_reports/test.rs | 57 ++- 10 files changed, 911 insertions(+), 5 deletions(-) create mode 100644 rust/crates/sift_mcp/src/service/test_reports/export.rs diff --git a/rust/crates/sift_cli/assets/docs/src/agents/mcp.md b/rust/crates/sift_cli/assets/docs/src/agents/mcp.md index 52f366ceb..3621c0e13 100644 --- a/rust/crates/sift_cli/assets/docs/src/agents/mcp.md +++ b/rust/crates/sift_cli/assets/docs/src/agents/mcp.md @@ -51,6 +51,7 @@ not run interactively. | `count_test_measurements` | Count test measurements matching a filter, without fetching them. | | `create_test_report` | Create a test report with its steps and measurements from a JSON document. | | `append_test_measurements` | Append measurements to an existing test step. | +| `export_test_report` | Snapshot a test report's full tree to a JSON file for audit/diff and dry-runs. | | `update_test_report` | Update a test report's fields (status, name, times, run link, archive). | | `update_test_step` | Update a test step's fields (name, status, times, error info, metadata). | | `update_test_measurement` | Update a test measurement's value, bounds, verdict, unit, or metadata. | diff --git a/rust/crates/sift_cli/assets/docs/src/agents/prompts.md b/rust/crates/sift_cli/assets/docs/src/agents/prompts.md index 631e68604..5f929e104 100644 --- a/rust/crates/sift_cli/assets/docs/src/agents/prompts.md +++ b/rust/crates/sift_cli/assets/docs/src/agents/prompts.md @@ -33,6 +33,7 @@ then in Claude Code each prompt is available as a slash command of the form - `/mcp__sift__explore_asset` - `/mcp__sift__analyze_run` - `/mcp__sift__derive_and_upload` +- `/mcp__sift__audit_test_report` The `sift` in the command is your registered server name, so a different name in `.mcp.json` changes the prefix accordingly. Arguments are passed positionally @@ -116,4 +117,35 @@ Specify the destination explicitly: The agent extracts the source data, applies the transform with `sql` (keeping `timestamp_unix_nanos` as the first column, as Sift requires), confirms the target asset, run, and any tags with you, then uploads the result with -`upload_dataset`. \ No newline at end of file +`upload_dataset`. + +## `audit_test_report` + +Snapshots a test report (the test-results feature: a report owns steps, which +own measurements) to a JSON file for audit and diffing. Given a description of +intended edits, it dry-runs them first: it projects the changes, diffs them +against the baseline for you to confirm, and only then applies the writes and +verifies the result. Omit the edits for an audit snapshot alone. + +| Argument | Required | Description | +| ------------- | -------- | ----------------------------------------------------------------- | +| `test_report` | yes | Report to audit, by id or name. | +| `changes` | no | Plain-language description of the batch edits to dry-run. | +| `output_dir` | no | Directory for the snapshot files. Defaults to the working directory. | + +Snapshot a report for the record: + +``` +/mcp__sift__audit_test_report "nightly-regression 2024-05-01" +``` + +Dry-run a batch of edits before applying: + +``` +/mcp__sift__audit_test_report tr_abc123 "mark every step under power-on as FAILED and set its error_info" +``` + +The agent resolves the report, exports a baseline snapshot, projects the edits +to a second file, and diffs the two so you can confirm. Once you approve, it +applies the changes with the `update_test_*` tools, re-exports, and diffs +against the projection to confirm the server recorded what you intended. \ No newline at end of file diff --git a/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md b/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md index 33b6dac2a..c52d021ba 100644 --- a/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md +++ b/rust/crates/sift_cli/assets/skills/agents-md/AGENTS.md @@ -25,6 +25,10 @@ to combine them when working with Sift. - `create_test_report`, `append_test_measurements`: create a test report with its step/measurement tree, or add measurements to an existing step (writes — confirm with the user first). + - `export_test_report`: snapshot a report's full tree to a JSON file (reads + only; omits server-managed fields, deterministically ordered). Use it to + record a baseline for audit/diff, or to dry-run bulk edits — export, apply + changes to a copy, diff, then run the update tools and re-export to confirm. - `update_test_report`, `update_test_step`, `update_test_measurement`: update fields of an existing report, step, or measurement (writes — only the fields you set change; metadata/channel_names use replace semantics; confirm current diff --git a/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md b/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md index 9c27e2eb7..b566d828d 100644 --- a/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/claude-code/SKILL.md @@ -41,6 +41,10 @@ to combine them when working with Sift. - `create_test_report`, `append_test_measurements`: create a test report with its step/measurement tree, or add measurements to an existing step (writes — confirm with the user first). + - `export_test_report`: snapshot a report's full tree to a JSON file (reads + only; omits server-managed fields, deterministically ordered). Use it to + record a baseline for audit/diff, or to dry-run bulk edits — export, apply + changes to a copy, diff, then run the update tools and re-export to confirm. - `update_test_report`, `update_test_step`, `update_test_measurement`: update fields of an existing report, step, or measurement (writes — only the fields you set change; metadata/channel_names use replace semantics; confirm current diff --git a/rust/crates/sift_mcp/src/prompt/mod.rs b/rust/crates/sift_mcp/src/prompt/mod.rs index 255c7fc39..3be0e48e4 100644 --- a/rust/crates/sift_mcp/src/prompt/mod.rs +++ b/rust/crates/sift_mcp/src/prompt/mod.rs @@ -30,6 +30,13 @@ pub struct DeriveAndUploadArgs { target_run: Option, } +#[derive(Debug, Deserialize, JsonSchema)] +pub struct AuditTestReportArgs { + test_report: String, + changes: Option, + output_dir: Option, +} + #[prompt_router(vis = "pub(crate)")] impl SiftMcpServer { #[prompt( @@ -157,4 +164,61 @@ impl SiftMcpServer { vec![PromptMessage::new_text(PromptMessageRole::User, body)] } + + #[prompt( + name = "audit_test_report", + description = "Snapshot a test report for audit, and optionally dry-run a batch of edits: diff the projected changes before applying, then apply and verify. Writes only after you confirm the diff." + )] + pub async fn audit_test_report( + &self, + params: Parameters, + ) -> Vec { + let Parameters(AuditTestReportArgs { + test_report, + changes, + output_dir, + }) = params; + + let dir = output_dir.unwrap_or_else(|| "the current working directory".to_string()); + + let tail = match changes { + Some(changes) => format!( + "The user wants to dry-run this batch of edits before applying them: \"{changes}\".\n\n\ + 4. DRY RUN: apply the intended edits to a COPY of the baseline file (write \ + `/.projected.json`). Do NOT call any write tool yet. Field names in \ + the snapshot mirror the update-tool inputs, so map each edit to the step or measurement \ + by its id. Remember `metadata` and `channel_names` are REPLACE, and changing a \ + measurement value does not recompute `passed` — set it explicitly if the verdict should \ + change.\n\ + 5. Diff baseline vs projected and show the user exactly what would change. Get explicit \ + confirmation before writing.\n\ + 6. On approval, apply the edits with `update_test_report`, `update_test_step`, and \ + `update_test_measurement`, using the ids from the snapshot.\n\ + 7. Re-export to `/.after.json` and diff it against the projected \ + file. Report any discrepancy between what was intended and what the server recorded." + ), + None => { + "This is an audit snapshot only. Report the baseline file path as the record and \ + make no changes." + .to_string() + } + }; + + let body = format!( + "Help the user audit a Sift test report (the test-results feature: a report owns steps, which \ + own measurements). The user referred to the report as: \"{test_report}\".\n\n\ + Write snapshot files under: {dir}.\n\n\ + Steps:\n\ + 1. Resolve the report with `list_test_reports`. If \"{test_report}\" looks like an id, filter \ + `test_report_id == \"{test_report}\"`; otherwise filter `name == \"{test_report}\"`. If \ + several match, ask the user which one before continuing.\n\ + 2. Snapshot the baseline with `export_test_report` to \ + `/.before.json`. This is the audit record; it omits server-managed \ + fields and is deterministically ordered, so diffs show only user-editable changes.\n\ + 3. Confirm with the user what the baseline captured (step and measurement counts).\n\ + {tail}" + ); + + vec![PromptMessage::new_text(PromptMessageRole::User, body)] + } } diff --git a/rust/crates/sift_mcp/src/service/test_reports/export.rs b/rust/crates/sift_mcp/src/service/test_reports/export.rs new file mode 100644 index 000000000..219a7b1bc --- /dev/null +++ b/rust/crates/sift_mcp/src/service/test_reports/export.rs @@ -0,0 +1,332 @@ +//! Canonical, diff-stable snapshot of a test report tree for `export_test_report`. +//! +//! [`build`] lowers the flat proto types (report + steps + measurements) into a nested, +//! deterministically ordered tree and drops server-managed fields that a user cannot edit +//! (e.g. the report's derived `archived_date`), so a diff between two snapshots shows only +//! real, user-driven changes. Field names mirror the create/update tool inputs +//! (`string_expected`, `numeric_bounds`, `unit`) so a value can be copied from a snapshot +//! straight into an update call. + +use std::cmp::Ordering; +use std::collections::{BTreeMap, HashMap}; + +use pbjson_types::Timestamp; +use serde::Serialize; +use sift_rs::{ + metadata::v1::{MetadataValue, metadata_value::Value as MetaInner}, + test_reports::v1::{ + TestMeasurement, TestMeasurementType, TestReport, TestStatus, TestStep, TestStepType, + test_measurement, + }, +}; + +/// A snapshot plus the counts of what it contains, so the tool can report totals without +/// re-walking the tree. +#[derive(Debug)] +pub struct Export { + pub report: ExportedReport, + pub steps_exported: usize, + pub measurements_exported: usize, +} + +/// A metadata scalar, serialized as a bare JSON value (untagged) to match the tool inputs. +#[derive(Debug, Serialize, PartialEq)] +#[serde(untagged)] +pub enum MetaScalar { + String(String), + Number(f64), + Boolean(bool), +} + +#[derive(Debug, Serialize, PartialEq)] +pub struct ExportedReport { + pub test_report_id: String, + pub name: String, + pub status: String, + pub test_system_name: String, + pub test_case: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub start_time: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub end_time: Option, + #[serde(skip_serializing_if = "String::is_empty")] + pub serial_number: String, + #[serde(skip_serializing_if = "String::is_empty")] + pub part_number: String, + #[serde(skip_serializing_if = "String::is_empty")] + pub system_operator: String, + #[serde(skip_serializing_if = "String::is_empty")] + pub run_id: String, + pub is_archived: bool, + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + pub metadata: BTreeMap, + pub steps: Vec, +} + +#[derive(Debug, Serialize, PartialEq)] +pub struct ExportedStep { + pub test_step_id: String, + pub step_path: String, + pub name: String, + pub step_type: String, + pub status: String, + #[serde(skip_serializing_if = "String::is_empty")] + pub description: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub start_time: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub end_time: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub error_info: Option, + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + pub metadata: BTreeMap, + #[serde(skip_serializing_if = "Vec::is_empty")] + pub measurements: Vec, + #[serde(skip_serializing_if = "Vec::is_empty")] + pub steps: Vec, +} + +#[derive(Debug, Serialize, PartialEq)] +pub struct ExportedErrorInfo { + pub error_code: i32, + pub error_message: String, +} + +#[derive(Debug, Serialize, PartialEq)] +pub struct ExportedMeasurement { + pub measurement_id: String, + pub name: String, + pub measurement_type: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub numeric_value: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub string_value: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub boolean_value: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub unit: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub numeric_bounds: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub string_expected: Option, + pub passed: bool, + #[serde(skip_serializing_if = "Option::is_none")] + pub timestamp: Option, + #[serde(skip_serializing_if = "String::is_empty")] + pub description: String, + #[serde(skip_serializing_if = "Vec::is_empty")] + pub channel_names: Vec, + #[serde(skip_serializing_if = "BTreeMap::is_empty")] + pub metadata: BTreeMap, +} + +#[derive(Debug, Serialize, PartialEq)] +pub struct ExportedNumericBounds { + #[serde(skip_serializing_if = "Option::is_none")] + pub min: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub max: Option, +} + +/// Assemble the report, its steps, and its measurements into a canonical tree. Steps are nested +/// by `parent_step_id` and ordered by `step_path` (numeric segment order, so `10` sorts after +/// `2`); measurements are attached to their step and ordered by name then id. `steps_exported` +/// and `measurements_exported` count the fetched rows. +pub fn build( + report: TestReport, + steps: Vec, + measurements: Vec, +) -> Export { + let steps_exported = steps.len(); + let measurements_exported = measurements.len(); + + // Group measurements under their step, ordered deterministically. + let mut by_step: HashMap> = HashMap::new(); + for mut m in measurements { + let step_id = std::mem::take(&mut m.test_step_id); + by_step + .entry(step_id) + .or_default() + .push(exported_measurement(m)); + } + for v in by_step.values_mut() { + v.sort_by(|a, b| { + a.name + .cmp(&b.name) + .then_with(|| a.measurement_id.cmp(&b.measurement_id)) + }); + } + + // Index steps by parent so children can be attached; roots have an empty parent. + let mut children: HashMap> = HashMap::new(); + for mut s in steps { + let parent = std::mem::take(&mut s.parent_step_id); + children.entry(parent).or_default().push(s); + } + + let mut roots = build_children("", &mut children, &mut by_step); + // Defensive: never drop a step whose parent isn't present — surface it at the root rather + // than losing it from an audit snapshot. Consistent server data leaves `children` empty here. + // Ordering is handled by the final root sort below, so no need to order the orphan parents. + let orphan_parents: Vec = children.keys().cloned().collect(); + for parent in orphan_parents { + roots.extend(build_children(&parent, &mut children, &mut by_step)); + } + roots.sort_by(|a, b| cmp_step_path(&a.step_path, &b.step_path)); + + Export { + report: ExportedReport { + test_report_id: report.test_report_id, + name: report.name, + status: status_name(report.status), + test_system_name: report.test_system_name, + test_case: report.test_case, + start_time: ts_to_rfc3339(report.start_time.as_ref()), + end_time: ts_to_rfc3339(report.end_time.as_ref()), + serial_number: report.serial_number, + part_number: report.part_number, + system_operator: report.system_operator, + run_id: report.run_id, + is_archived: report.is_archived, + metadata: metadata_map(report.metadata), + steps: roots, + }, + steps_exported, + measurements_exported, + } +} + +fn build_children( + parent_id: &str, + children: &mut HashMap>, + by_step: &mut HashMap>, +) -> Vec { + let Some(mut nodes) = children.remove(parent_id) else { + return Vec::new(); + }; + nodes.sort_by(|a, b| cmp_step_path(&a.step_path, &b.step_path)); + nodes + .into_iter() + .map(|s| { + let measurements = by_step.remove(&s.test_step_id).unwrap_or_default(); + let child_steps = build_children(&s.test_step_id, children, by_step); + ExportedStep { + measurements, + steps: child_steps, + error_info: s.error_info.map(|e| ExportedErrorInfo { + error_code: e.error_code, + error_message: e.error_message, + }), + start_time: ts_to_rfc3339(s.start_time.as_ref()), + end_time: ts_to_rfc3339(s.end_time.as_ref()), + metadata: metadata_map(s.metadata), + step_type: step_type_name(s.step_type), + status: status_name(s.status), + test_step_id: s.test_step_id, + step_path: s.step_path, + name: s.name, + description: s.description, + } + }) + .collect() +} + +fn exported_measurement(m: TestMeasurement) -> ExportedMeasurement { + let (numeric_value, string_value, boolean_value) = match m.value { + Some(test_measurement::Value::NumericValue(v)) => (Some(v), None, None), + Some(test_measurement::Value::StringValue(v)) => (None, Some(v), None), + Some(test_measurement::Value::BooleanValue(v)) => (None, None, Some(v)), + None => (None, None, None), + }; + let (numeric_bounds, string_expected) = match m.bounds { + Some(test_measurement::Bounds::NumericBounds(b)) => ( + Some(ExportedNumericBounds { + min: b.min, + max: b.max, + }), + None, + ), + Some(test_measurement::Bounds::StringBounds(b)) => (None, Some(b.expected_value)), + None => (None, None), + }; + ExportedMeasurement { + measurement_id: m.measurement_id, + name: m.name, + measurement_type: measurement_type_name(m.measurement_type), + numeric_value, + string_value, + boolean_value, + unit: m.unit.map(|u| u.abbreviated_name).filter(|s| !s.is_empty()), + numeric_bounds, + string_expected, + passed: m.passed, + timestamp: ts_to_rfc3339(m.timestamp.as_ref()), + description: m.description, + channel_names: m.channel_names, + metadata: metadata_map(m.metadata), + } +} + +/// Compare hierarchical step paths (`"1"`, `"1.10"`, `"2"`) segment by segment, numerically when +/// both segments parse as integers so `10` sorts after `2`. +fn cmp_step_path(a: &str, b: &str) -> Ordering { + let mut ai = a.split('.'); + let mut bi = b.split('.'); + loop { + match (ai.next(), bi.next()) { + (Some(x), Some(y)) => { + let ord = match (x.parse::(), y.parse::()) { + (Ok(nx), Ok(ny)) => nx.cmp(&ny), + _ => x.cmp(y), + }; + if ord != Ordering::Equal { + return ord; + } + } + (Some(_), None) => return Ordering::Greater, + (None, Some(_)) => return Ordering::Less, + (None, None) => return Ordering::Equal, + } + } +} + +fn status_name(v: i32) -> String { + TestStatus::try_from(v) + .map(|e| e.as_str_name().to_string()) + .unwrap_or_else(|_| v.to_string()) +} + +fn step_type_name(v: i32) -> String { + TestStepType::try_from(v) + .map(|e| e.as_str_name().to_string()) + .unwrap_or_else(|_| v.to_string()) +} + +fn measurement_type_name(v: i32) -> String { + TestMeasurementType::try_from(v) + .map(|e| e.as_str_name().to_string()) + .unwrap_or_else(|_| v.to_string()) +} + +fn ts_to_rfc3339(ts: Option<&Timestamp>) -> Option { + let ts = ts?; + let dt = chrono::DateTime::from_timestamp(ts.seconds, ts.nanos as u32)?; + Some(dt.to_rfc3339_opts(chrono::SecondsFormat::AutoSi, true)) +} + +/// Lower proto metadata to a sorted map of scalars. Relation values (and any future non-scalar +/// kinds) are outside the user-editable scalar surface, so they are dropped. +fn metadata_map(values: Vec) -> BTreeMap { + let mut map = BTreeMap::new(); + for mv in values { + let Some(key) = mv.key else { continue }; + let scalar = match mv.value { + Some(MetaInner::StringValue(s)) => MetaScalar::String(s), + Some(MetaInner::NumberValue(n)) => MetaScalar::Number(n), + Some(MetaInner::BooleanValue(b)) => MetaScalar::Boolean(b), + _ => continue, + }; + map.insert(key.name, scalar); + } + map +} diff --git a/rust/crates/sift_mcp/src/service/test_reports/mod.rs b/rust/crates/sift_mcp/src/service/test_reports/mod.rs index ed922aec4..eaffd87c7 100644 --- a/rust/crates/sift_mcp/src/service/test_reports/mod.rs +++ b/rust/crates/sift_mcp/src/service/test_reports/mod.rs @@ -19,6 +19,7 @@ use sift_rs::{ unit::v2::Unit, }; +pub mod export; pub mod spec; use spec::{BuiltReport, BuiltStep}; @@ -779,4 +780,33 @@ impl TestReportService { resp.test_measurement .ok_or_else(|| anyhow!("update_test_measurement response missing measurement")) } + + /// Fetch a full test report tree (report, its steps, and optionally its measurements) and + /// assemble it into a canonical, diff-stable snapshot. Reuses the list RPCs, filtering each by + /// `test_report_id`. Returns `Ok(None)` when no report matches, so the tool layer can classify + /// that as `RESOURCE_NOT_FOUND`. + pub async fn export_test_report( + &self, + test_report_id: String, + include_measurements: bool, + ) -> Result> { + let id_filter = format!("test_report_id == \"{test_report_id}\""); + + let mut reports = self + .list_test_reports(id_filter.clone(), None, Some(1)) + .await?; + if reports.is_empty() { + return Ok(None); + } + let report = reports.remove(0); + + let steps = self.list_test_steps(id_filter.clone(), None, None).await?; + let measurements = if include_measurements { + self.list_test_measurements(id_filter, None, None).await? + } else { + Vec::new() + }; + + Ok(Some(export::build(report, steps, measurements))) + } } diff --git a/rust/crates/sift_mcp/src/service/test_reports/test.rs b/rust/crates/sift_mcp/src/service/test_reports/test.rs index c66b71add..0155283e5 100644 --- a/rust/crates/sift_mcp/src/service/test_reports/test.rs +++ b/rust/crates/sift_mcp/src/service/test_reports/test.rs @@ -14,6 +14,7 @@ use tokio::task::JoinHandle; use tonic::{Response, Status, transport::Server}; use super::TestReportService; +use super::export; use super::spec; use crate::policy::RetryPolicy; use crate::service::common::PAGE_SIZE; @@ -971,3 +972,270 @@ async fn update_test_measurement_propagates_grpc_error() { .contains("failed to update test measurement") ); } + +// --- export::build (pure tree assembly) --- + +#[test] +fn export_build_nests_orders_and_attaches_measurements() { + let report = TestReport { + test_report_id: "r1".into(), + name: "nightly".into(), + status: TestStatus::Passed as i32, + test_system_name: "rig".into(), + test_case: "tc".into(), + ..Default::default() + }; + let mk_step = |id: &str, parent: &str, path: &str, name: &str, ty: TestStepType| TestStep { + test_step_id: id.into(), + test_report_id: "r1".into(), + parent_step_id: parent.into(), + step_path: path.into(), + name: name.into(), + step_type: ty as i32, + status: TestStatus::Passed as i32, + ..Default::default() + }; + // Deliberately out of order, with a two-digit path to exercise numeric sorting. + let steps = vec![ + mk_step("s2", "", "2", "second", TestStepType::Action), + mk_step("s10", "", "10", "tenth", TestStepType::Action), + mk_step("s1", "", "1", "first", TestStepType::Sequence), + mk_step("s1c", "s1", "1.1", "child", TestStepType::Action), + ]; + let mk_meas = |id: &str, name: &str| TestMeasurement { + measurement_id: id.into(), + test_step_id: "s1".into(), + test_report_id: "r1".into(), + name: name.into(), + measurement_type: TestMeasurementType::Double as i32, + value: Some(test_measurement::Value::NumericValue(1.0)), + passed: true, + ..Default::default() + }; + let measurements = vec![mk_meas("m2", "bbb"), mk_meas("m1", "aaa")]; + + let export = export::build(report, steps, measurements); + assert_eq!(export.steps_exported, 4); + assert_eq!(export.measurements_exported, 2); + + // Roots ordered numerically: 1, 2, 10 (not lexicographically 1, 10, 2). + let paths: Vec<&str> = export + .report + .steps + .iter() + .map(|s| s.step_path.as_str()) + .collect(); + assert_eq!(paths, vec!["1", "2", "10"]); + + let first = &export.report.steps[0]; + // Child nested under "1". + assert_eq!(first.steps.len(), 1); + assert_eq!(first.steps[0].step_path, "1.1"); + // Measurements attached under "1", ordered by name. + let ms: Vec<&str> = first.measurements.iter().map(|m| m.name.as_str()).collect(); + assert_eq!(ms, vec!["aaa", "bbb"]); + // Enum tags rendered as canonical names, not ints. + assert_eq!(first.status, "TEST_STATUS_PASSED"); + assert_eq!(first.step_type, "TEST_STEP_TYPE_SEQUENCE"); +} + +#[test] +fn export_omits_server_managed_fields_but_keeps_is_archived() { + let report = TestReport { + test_report_id: "r1".into(), + name: "n".into(), + status: TestStatus::Passed as i32, + test_system_name: "rig".into(), + test_case: "tc".into(), + is_archived: true, + archived_date: Some(Timestamp { + seconds: 100, + nanos: 0, + }), + ..Default::default() + }; + let export = export::build(report, vec![], vec![]); + let json = serde_json::to_value(&export.report).expect("serialize"); + let obj = json.as_object().unwrap(); + assert!( + !obj.contains_key("archived_date"), + "server-managed archived_date must be omitted" + ); + assert_eq!(obj.get("is_archived"), Some(&serde_json::Value::Bool(true))); +} + +#[test] +fn export_maps_measurement_value_and_bounds() { + let report = TestReport { + test_report_id: "r1".into(), + name: "n".into(), + status: TestStatus::Passed as i32, + test_system_name: "rig".into(), + test_case: "tc".into(), + ..Default::default() + }; + let steps = vec![TestStep { + test_step_id: "s1".into(), + test_report_id: "r1".into(), + step_path: "1".into(), + name: "s".into(), + step_type: TestStepType::Action as i32, + status: TestStatus::Passed as i32, + ..Default::default() + }]; + let measurements = vec![ + TestMeasurement { + measurement_id: "m1".into(), + test_step_id: "s1".into(), + test_report_id: "r1".into(), + name: "num".into(), + measurement_type: TestMeasurementType::Double as i32, + value: Some(test_measurement::Value::NumericValue(3.3)), + bounds: Some(test_measurement::Bounds::NumericBounds(NumericBounds { + min: Some(3.0), + max: Some(3.6), + })), + passed: true, + ..Default::default() + }, + TestMeasurement { + measurement_id: "m2".into(), + test_step_id: "s1".into(), + test_report_id: "r1".into(), + name: "str".into(), + measurement_type: TestMeasurementType::String as i32, + value: Some(test_measurement::Value::StringValue("ok".into())), + bounds: Some(test_measurement::Bounds::StringBounds(StringBounds { + expected_value: "ok".into(), + })), + passed: true, + ..Default::default() + }, + ]; + + let export = export::build(report, steps, measurements); + let ms = &export.report.steps[0].measurements; + // Ordered by name: num, str. + assert_eq!(ms[0].numeric_value, Some(3.3)); + let nb = ms[0].numeric_bounds.as_ref().unwrap(); + assert_eq!((nb.min, nb.max), (Some(3.0), Some(3.6))); + assert_eq!(ms[1].string_value.as_deref(), Some("ok")); + assert_eq!(ms[1].string_expected.as_deref(), Some("ok")); + assert_eq!(ms[1].measurement_type, "TEST_MEASUREMENT_TYPE_STRING"); +} + +// --- export_test_report (composite against the mock) --- + +#[tokio::test] +async fn export_test_report_assembles_from_lists() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_list_test_reports() + .withf(|req| req.get_ref().filter == "test_report_id == \"r1\"") + .returning(|_| { + Ok(Response::new(ListTestReportsResponse { + test_reports: vec![TestReport { + test_report_id: "r1".into(), + name: "n".into(), + status: TestStatus::Passed as i32, + test_system_name: "rig".into(), + test_case: "tc".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + mock.expect_list_test_steps().returning(|_| { + Ok(Response::new(ListTestStepsResponse { + test_steps: vec![TestStep { + test_step_id: "s1".into(), + test_report_id: "r1".into(), + step_path: "1".into(), + name: "s".into(), + step_type: TestStepType::Action as i32, + status: TestStatus::Passed as i32, + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + mock.expect_list_test_measurements().returning(|_| { + Ok(Response::new(ListTestMeasurementsResponse { + test_measurements: vec![TestMeasurement { + measurement_id: "m1".into(), + test_step_id: "s1".into(), + test_report_id: "r1".into(), + name: "v".into(), + measurement_type: TestMeasurementType::Double as i32, + value: Some(test_measurement::Value::NumericValue(1.0)), + passed: true, + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let export = service + .export_test_report("r1".to_string(), true) + .await + .expect("export_test_report failed") + .expect("report present"); + + assert_eq!(export.report.test_report_id, "r1"); + assert_eq!(export.steps_exported, 1); + assert_eq!(export.measurements_exported, 1); + assert_eq!(export.report.steps[0].measurements[0].name, "v"); +} + +#[tokio::test] +async fn export_test_report_returns_none_when_not_found() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_list_test_reports().returning(|_| { + Ok(Response::new(ListTestReportsResponse { + test_reports: vec![], + next_page_token: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + + let export = service + .export_test_report("missing".to_string(), true) + .await + .expect("export_test_report failed"); + + assert!(export.is_none(), "missing report should return None"); +} + +#[tokio::test] +async fn export_test_report_skips_measurements_when_disabled() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_list_test_reports().returning(|_| { + Ok(Response::new(ListTestReportsResponse { + test_reports: vec![TestReport { + test_report_id: "r1".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + mock.expect_list_test_steps().returning(|_| { + Ok(Response::new(ListTestStepsResponse { + test_steps: vec![], + next_page_token: String::new(), + })) + }); + // Measurements must not be fetched when the caller opts out. + mock.expect_list_test_measurements().never(); + + let (service, _h) = service_with_mock(mock).await; + + let export = service + .export_test_report("r1".to_string(), false) + .await + .expect("export_test_report failed") + .expect("report present"); + + assert_eq!(export.measurements_exported, 0); +} 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 5456acf4e..1e36250a3 100644 --- a/rust/crates/sift_mcp/src/tool/test_reports/mod.rs +++ b/rust/crates/sift_mcp/src/tool/test_reports/mod.rs @@ -1,3 +1,5 @@ +use std::{fs::File, io::Write, path::PathBuf}; + use rmcp::{ ErrorData, handler::server::wrapper::Parameters, @@ -46,6 +48,13 @@ pub struct AppendMeasurementsParams { pub(crate) measurements_json: String, } +#[derive(Debug, Deserialize, JsonSchema)] +pub struct ExportTestReportParams { + pub(crate) test_report_id: String, + pub(crate) output: PathBuf, + pub(crate) include_measurements: Option, +} + #[derive(Debug, Deserialize, JsonSchema)] pub struct UpdateTestReportParams { pub(crate) test_report_id: String, @@ -522,6 +531,119 @@ impl SiftMcpServer { Ok(result) } + #[tool( + name = "export_test_report", + description = " + Snapshot a full test report tree (the report, its steps, and their measurements) to a JSON + file for audit and diffing. Reads only; writes a local file, never mutates Sift. The snapshot + omits server-managed fields (e.g. the report's derived `archived_date`) and orders everything + deterministically, so a diff between two snapshots shows only user-editable changes. + + Use it to record a baseline before a batch of `update_test_*` edits, then re-export afterward + and diff to confirm the writes matched intent. To DRY-RUN a bulk edit without touching Sift: + export the baseline, apply the intended changes to a copy of the file, and diff the two — only + then run the write tools. + + Output: + - Writes a JSON file at `output` (truncate mode). Shape: the report's user-editable fields, + then a nested `steps` array (children nested under parents, ordered by `step_path`), each + step carrying its `measurements`. Field names mirror the create/update tool inputs + (`numeric_bounds`, `string_expected`, `unit`), so a value can be copied straight into an + update call. Empty/defaulted fields are omitted. + - Tool result: `{ \"output\": \"\", \"test_report_id\": \"...\", \"steps_exported\": N, + \"measurements_exported\": M, \"report_url\": string|null, \"next_step\": string }`. + + Parameters: + - `test_report_id`: required; the report to snapshot. + - `output`: required filesystem path for the JSON file. Opened in truncate mode; an existing + file is overwritten. + - `include_measurements`: optional (default `true`). Set `false` for a structure-only snapshot + that skips the measurement fetch. + + Errors: + - `INVALID_PARAMS` if `test_report_id` is empty. + - `RESOURCE_NOT_FOUND` if no report matches `test_report_id`. + - `INTERNAL_ERROR` if an upstream gRPC call fails or the file cannot be written. + + Guidance: + - Snapshot into a version-controlled path (or two dated files) so `git diff` / `diff` gives a + clean before/after audit trail across a batch of edits. + ", + annotations( + title = "test_reports_router/export_test_report", + read_only_hint = true + ) + )] + pub async fn export_test_report( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(ExportTestReportParams { + test_report_id, + output, + include_measurements, + }) = params; + + if test_report_id.is_empty() { + return Err(ErrorData::invalid_params( + "`test_report_id` must not be empty", + None, + )); + } + let include_measurements = include_measurements.unwrap_or(true); + + let export = self + .test_report_service + .export_test_report(test_report_id.clone(), include_measurements) + .await + .map_err(from_anyhow)? + .ok_or_else(|| { + ErrorData::resource_not_found( + format!("no test report matches `{test_report_id}`"), + None, + ) + })?; + + let json = serde_json::to_string_pretty(&export.report).map_err(|e| { + ErrorData::internal_error(format!("failed to serialize export: {e}"), None) + })?; + let mut file = File::options() + .create(true) + .write(true) + .truncate(true) + .open(&output) + .map_err(|e| { + ErrorData::internal_error(format!("failed to open output file: {e}"), None) + })?; + file.write_all(json.as_bytes()).map_err(|e| { + ErrorData::internal_error(format!("failed to write output file: {e}"), None) + })?; + + let output_str = output.to_string_lossy().into_owned(); + let report_url = self.url_service.build_test_report_url(&test_report_id).ok(); + let next_step = format!( + "Wrote a canonical snapshot of test report `{}` ({} step(s), {} measurement(s)) to \ + `{output_str}`. It omits server-managed fields so a diff shows only user-editable \ + changes.{} To dry-run bulk edits: keep this file, apply the intended changes to a copy, \ + diff the two, and only then run the `update_test_*` tools; re-export afterward to confirm.", + test_report_id, + export.steps_exported, + export.measurements_exported, + url_clause(report_url.as_deref()), + ); + + let mut result = CallToolResult::structured(serde_json::json!({ + "output": output_str, + "test_report_id": test_report_id, + "steps_exported": export.steps_exported, + "measurements_exported": export.measurements_exported, + "report_url": report_url, + "next_step": next_step, + })); + result.content = vec![Content::text(next_step)]; + Ok(result) + } + #[tool( name = "update_test_report", description = " diff --git a/rust/crates/sift_mcp/src/tool/test_reports/test.rs b/rust/crates/sift_mcp/src/tool/test_reports/test.rs index 5a8f50d03..94c5e549c 100644 --- a/rust/crates/sift_mcp/src/tool/test_reports/test.rs +++ b/rust/crates/sift_mcp/src/tool/test_reports/test.rs @@ -1,7 +1,8 @@ use rmcp::handler::server::wrapper::Parameters; use sift_rs::test_reports::v1::{ - CreateTestMeasurementsResponse, CreateTestReportResponse, TestMeasurement, TestReport, - TestStep, UpdateTestMeasurementResponse, UpdateTestReportResponse, UpdateTestStepResponse, + CreateTestMeasurementsResponse, CreateTestReportResponse, ListTestMeasurementsResponse, + ListTestReportsResponse, ListTestStepsResponse, TestMeasurement, TestReport, TestStep, + UpdateTestMeasurementResponse, UpdateTestReportResponse, UpdateTestStepResponse, test_report_service_server::TestReportServiceServer, }; use sift_test_util::{ @@ -11,8 +12,8 @@ use tokio::task::JoinHandle; use tonic::{Response, transport::Server}; use super::{ - AppendMeasurementsParams, CreateTestReportParams, UpdateTestMeasurementParams, - UpdateTestReportParams, UpdateTestStepParams, + AppendMeasurementsParams, CreateTestReportParams, ExportTestReportParams, + UpdateTestMeasurementParams, UpdateTestReportParams, UpdateTestStepParams, }; use crate::{server::SiftMcpServer, tool::common::test_support::structured_field}; @@ -284,3 +285,51 @@ async fn append_test_measurements_surfaces_url() { let report_url = structured_field(resp, "report_url"); assert_eq!(report_url, "https://app.test.local/test-results/tr1"); } + +#[tokio::test] +async fn export_test_report_writes_file_and_surfaces_url() { + let mut mock = MockTestReportServiceImpl::new(); + mock.expect_list_test_reports().returning(|_| { + Ok(Response::new(ListTestReportsResponse { + test_reports: vec![TestReport { + test_report_id: "tr1".into(), + name: "nightly".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + mock.expect_list_test_steps().returning(|_| { + Ok(Response::new(ListTestStepsResponse { + test_steps: vec![], + next_page_token: String::new(), + })) + }); + mock.expect_list_test_measurements().returning(|_| { + Ok(Response::new(ListTestMeasurementsResponse { + test_measurements: vec![], + next_page_token: String::new(), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + + let out = std::env::temp_dir().join("sift_mcp_export_test_report_test.json"); + let params = ExportTestReportParams { + test_report_id: "tr1".into(), + output: out.clone(), + include_measurements: Some(true), + }; + let resp = server + .export_test_report(Parameters(params)) + .await + .expect("export_test_report failed"); + + let report_url = structured_field(resp, "report_url"); + assert_eq!(report_url, "https://app.test.local/test-results/tr1"); + + let written = std::fs::read_to_string(&out).expect("snapshot file written"); + assert!(written.contains("\"test_report_id\": \"tr1\"")); + assert!(written.contains("\"name\": \"nightly\"")); + let _ = std::fs::remove_file(&out); +}