From b12af8e1aefd3a40a0c578a28df68d39607719b9 Mon Sep 17 00:00:00 2001 From: Bill Raj-Derouin Date: Wed, 14 Oct 2026 17:39:50 -0700 Subject: [PATCH 1/2] rust(feat): add file attachment tools to the MCP server --- rust/crates/sift_cli/CHANGELOG.md | 6 + .../sift_cli/assets/skills/sift/SKILL.md | 8 +- rust/crates/sift_mcp/src/server/mod.rs | 6 +- .../sift_mcp/src/service/artifacts/mod.rs | 41 +-- .../sift_mcp/src/service/artifacts/test.rs | 10 +- .../sift_mcp/src/service/remote_files/mod.rs | 183 ++++++++++++ .../sift_mcp/src/service/remote_files/test.rs | 256 ++++++++++++++++- .../sift_mcp/src/tool/file_attachments/mod.rs | 222 +++++++++++++++ .../src/tool/file_attachments/test.rs | 262 ++++++++++++++++++ rust/crates/sift_mcp/src/tool/mod.rs | 1 + rust/crates/sift_mcp/src/tool_events.json | 2 + 11 files changed, 963 insertions(+), 34 deletions(-) create mode 100644 rust/crates/sift_mcp/src/tool/file_attachments/mod.rs create mode 100644 rust/crates/sift_mcp/src/tool/file_attachments/test.rs diff --git a/rust/crates/sift_cli/CHANGELOG.md b/rust/crates/sift_cli/CHANGELOG.md index 4f1ed7ac2c..ce9e1375fd 100644 --- a/rust/crates/sift_cli/CHANGELOG.md +++ b/rust/crates/sift_cli/CHANGELOG.md @@ -7,6 +7,12 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### What's New +- New `list_file_attachments` and `download_file_attachment` tools let agents + find the files attached to runs, assets, annotations and their comments, test + reports, and test steps, and fetch each one through a short-lived download + URL. Agents used to + send users to the run page to download these by hand. + ## [v0.7.4] - October 14, 2026 ### What's New diff --git a/rust/crates/sift_cli/assets/skills/sift/SKILL.md b/rust/crates/sift_cli/assets/skills/sift/SKILL.md index d409e2797d..7b01952378 100644 --- a/rust/crates/sift_cli/assets/skills/sift/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/sift/SKILL.md @@ -85,13 +85,19 @@ tool. connectivity check; when it fails, expect every other Sift tool to fail too. - **Discovery:** `list_assets`, `list_runs`, `list_channels`, `list_reports`, `list_report_templates`, `list_rules`, `list_rule_versions`, `list_annotations`, - `list_artifacts`, `list_artifact_versions`. + `list_artifacts`, `list_artifact_versions`, `list_file_attachments`. - **Artifacts:** `download_artifact` (latest version, or pin `artifact_version_id`), `list_artifact_versions` for one artifact's history, newest first. The artifact tools, including `list_artifacts` and the artifact writes below, are enabled per account by the agents feature flag resolved when the MCP server starts, so they may be absent from the tool list. Enabling them requires an account setting and an MCP restart. +- **File attachments:** `list_file_attachments` lists the files attached to + runs, assets, annotations and their comments, test reports, and test steps; + scope it with `entity_ids`. `download_file_attachment` returns a short-lived signed URL + for one file that you can fetch without Sift credentials. Use these when a + user asks for a file attached to a run or asset, instead of sending them to + the run page to download it. - **Derived channels:** `list_calculated_channels`, `list_calculated_channel_versions`, `list_user_defined_functions`, `list_user_defined_function_versions`. diff --git a/rust/crates/sift_mcp/src/server/mod.rs b/rust/crates/sift_mcp/src/server/mod.rs index 0ecf101b41..6c0a47e6ce 100644 --- a/rust/crates/sift_mcp/src/server/mod.rs +++ b/rust/crates/sift_mcp/src/server/mod.rs @@ -48,7 +48,7 @@ pub(crate) const BASE_INSTRUCTIONS: &str = concat!( use crate::service::{ annotations::AnnotationService, artifacts::ArtifactService, assets::AssetService, calculated_channels::CalculatedChannelService, channels::ChannelService, data::DataService, - docs::DocsService, ingest::IngestService, ping::PingService, + docs::DocsService, ingest::IngestService, ping::PingService, remote_files::RemoteFileService, report_templates::ReportTemplateService, reports::ReportService, rule_evaluation::RuleEvaluationService, rules::RuleService, runs::RunService, test_reports::TestReportService, url::UrlService, @@ -69,6 +69,7 @@ pub struct SiftMcpServer { pub url_service: UrlService, pub ingest_service: IngestService, pub ping_service: PingService, + pub remote_file_service: RemoteFileService, pub run_service: RunService, pub report_service: ReportService, pub report_template_service: ReportTemplateService, @@ -230,6 +231,7 @@ impl SiftMcpServer { tool_router.merge(Self::rule_evaluation_router()); tool_router.merge(Self::annotations_router()); tool_router.merge(Self::artifacts_router()); + tool_router.merge(Self::file_attachments_router()); tool_router.merge(Self::test_reports_router()); tool_router.merge(Self::docs_router()); tool_router.merge(Self::user_defined_functions_router()); @@ -289,6 +291,7 @@ impl SiftMcpServer { let url_service = UrlService::new(app_uri); let ingest_service = IngestService::new(channel.clone()); let ping_service = PingService::new(channel.clone(), retry_policy.clone()); + let remote_file_service = RemoteFileService::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 = @@ -312,6 +315,7 @@ impl SiftMcpServer { url_service, ingest_service, ping_service, + remote_file_service, run_service, report_service, report_template_service, diff --git a/rust/crates/sift_mcp/src/service/artifacts/mod.rs b/rust/crates/sift_mcp/src/service/artifacts/mod.rs index 36b8b401f9..6cdbdae3eb 100644 --- a/rust/crates/sift_mcp/src/service/artifacts/mod.rs +++ b/rust/crates/sift_mcp/src/service/artifacts/mod.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result, anyhow, bail}; +use anyhow::{Context, Result, anyhow}; use sift_rs::{ SiftChannel, artifacts::v1::{ @@ -9,16 +9,13 @@ use sift_rs::{ artifact_service_client::ArtifactServiceClient, }, metadata::v1::MetadataValue, - remote_files::v1::{ - GetRemoteFileDownloadUrlRequest, remote_file_service_client::RemoteFileServiceClient, - }, }; use std::path::Path; use crate::policy::{RetryPolicy, once, with_retry}; use crate::service::common; -use crate::service::remote_files::RemoteFileUploader; +use crate::service::remote_files::{RemoteFileService, RemoteFileUploader}; #[cfg(test)] mod test; @@ -103,6 +100,7 @@ impl UpdatePath { pub struct ArtifactService { channel: SiftChannel, policy: RetryPolicy, + remote_files: RemoteFileService, // Absent only when the server runs without a REST endpoint (some tests); // creating an artifact with a file requires it. uploader: Option, @@ -111,6 +109,7 @@ pub struct ArtifactService { impl ArtifactService { pub fn new(channel: SiftChannel, policy: RetryPolicy) -> Self { Self { + remote_files: RemoteFileService::new(channel.clone(), policy.clone()), channel, policy, uploader: None, @@ -259,7 +258,12 @@ impl ArtifactService { .as_ref() .and_then(|version| version.remote_file_id.clone()); let download_url = match remote_file_id { - Some(remote_file_id) => Some(self.download_url(remote_file_id).await?), + Some(remote_file_id) => Some( + self.remote_files + .download_url(remote_file_id) + .await + .context("failed to get artifact download url")?, + ), None => None, }; Ok(ArtifactView { @@ -451,7 +455,7 @@ impl ArtifactService { .as_ref() .and_then(|version| version.remote_file_id.clone()); let download_url = match remote_file_id { - Some(remote_file_id) => self.download_url(remote_file_id).await.ok(), + Some(remote_file_id) => self.remote_files.download_url(remote_file_id).await.ok(), None => None, }; Ok(ArtifactView { @@ -486,27 +490,4 @@ impl ArtifactService { .artifact .ok_or_else(|| anyhow!("get artifact response missing artifact")) } - - async fn download_url(&self, remote_file_id: String) -> Result { - let channel = self.channel.clone(); - let resp = with_retry(&self.policy, move || { - let channel = channel.clone(); - let remote_file_id = remote_file_id.clone(); - async move { - let mut client = RemoteFileServiceClient::new(channel); - client - .get_remote_file_download_url(GetRemoteFileDownloadUrlRequest { - remote_file_id, - }) - .await - .map(|resp| resp.into_inner()) - } - }) - .await - .context("failed to get artifact download url")?; - if resp.download_url.is_empty() { - bail!("download url response was empty"); - } - Ok(resp.download_url) - } } diff --git a/rust/crates/sift_mcp/src/service/artifacts/test.rs b/rust/crates/sift_mcp/src/service/artifacts/test.rs index 6cfd804d4a..61675b063f 100644 --- a/rust/crates/sift_mcp/src/service/artifacts/test.rs +++ b/rust/crates/sift_mcp/src/service/artifacts/test.rs @@ -290,7 +290,15 @@ async fn download_artifact_rejects_empty_download_url() { .download_artifact("art-1".into(), None) .await .expect_err("empty url is an error"); - assert!(err.to_string().contains("download url response was empty")); + let message = format!("{err:#}"); + assert!( + message.contains("failed to get artifact download url"), + "{message}" + ); + assert!( + message.contains("download url response was empty"), + "{message}" + ); } #[tokio::test] diff --git a/rust/crates/sift_mcp/src/service/remote_files/mod.rs b/rust/crates/sift_mcp/src/service/remote_files/mod.rs index 5560176876..071cd9407f 100644 --- a/rust/crates/sift_mcp/src/service/remote_files/mod.rs +++ b/rust/crates/sift_mcp/src/service/remote_files/mod.rs @@ -3,8 +3,17 @@ use std::time::Duration; use anyhow::{Context, Result, bail}; use reqwest::header::USER_AGENT; +use sift_rs::{ + SiftChannel, + remote_files::v1::{ + GetRemoteFileDownloadUrlRequest, ListRemoteFilesRequest, ListRemoteFilesResponse, + RemoteFile, remote_file_service_client::RemoteFileServiceClient, + }, +}; use crate::ClientName; +use crate::policy::{RetryPolicy, with_retry}; +use crate::service::common; const UPLOAD_PATH: &str = "/api/v0/remote-files/upload"; /// Client-side cap on one uploaded file. The server allows more, but an @@ -21,6 +30,180 @@ const FALLBACK_MIME: &str = "application/octet-stream"; /// looser than the interactive-call default. const UPLOAD_TIMEOUT: Duration = Duration::from_secs(10 * 60); +/// The entity types users attach files to. Remote files on other entities +/// (artifact versions, chat uploads, canvas code) are not attachments and stay +/// out of every listing. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum AttachmentEntityType { + Run, + Asset, + Annotation, + /// A comment on an annotation. + AnnotationLog, + TestReport, + TestStep, +} + +impl AttachmentEntityType { + pub const ALL: [Self; 6] = [ + Self::Run, + Self::Asset, + Self::Annotation, + Self::AnnotationLog, + Self::TestReport, + Self::TestStep, + ]; + + /// The singular name callers use. + pub fn name(self) -> &'static str { + match self { + Self::Run => "run", + Self::Asset => "asset", + Self::Annotation => "annotation", + Self::AnnotationLog => "annotation_log", + Self::TestReport => "test_report", + Self::TestStep => "test_step", + } + } + + /// The value the `ListRemoteFiles` filter compares `entity_type` against: + /// the backend's table name, not the `ENTITY_TYPE_*` name listed rows carry. + pub fn filter_value(self) -> &'static str { + match self { + Self::Run => "runs", + Self::Asset => "assets", + Self::Annotation => "annotations", + Self::AnnotationLog => "annotation_logs", + Self::TestReport => "test_reports", + Self::TestStep => "test_steps", + } + } +} + +/// ANDs the scope onto the caller's CEL `filter`. With no `entity_type`, the +/// listing is held to every [`AttachmentEntityType`]. +fn attachment_filter( + entity_ids: &[String], + entity_type: Option, + filter: &str, +) -> String { + let mut clauses = Vec::new(); + if !entity_ids.is_empty() { + let ids = entity_ids + .iter() + .map(|id| format!("\"{}\"", common::cel_escape(id.trim()))) + .collect::>() + .join(", "); + clauses.push(format!("entity_id in [{ids}]")); + } + match entity_type { + Some(entity_type) => { + clauses.push(format!("entity_type == \"{}\"", entity_type.filter_value())) + } + // The backend fails `entity_type in [...]` with an internal error, so + // the type set is spelled as `==` alternatives. + None => clauses.push(format!( + "({})", + AttachmentEntityType::ALL + .iter() + .map(|entity_type| format!("entity_type == \"{}\"", entity_type.filter_value())) + .collect::>() + .join(" || ") + )), + } + let filter = filter.trim(); + if !filter.is_empty() { + clauses.push(format!("({filter})")); + } + clauses.join(" && ") +} + +/// Reads remote files over gRPC: the files attached to runs, assets, and other +/// entities, and the bytes behind artifact versions. +#[derive(Clone)] +pub struct RemoteFileService { + channel: SiftChannel, + policy: RetryPolicy, +} + +impl RemoteFileService { + pub fn new(channel: SiftChannel, policy: RetryPolicy) -> Self { + Self { channel, policy } + } + + /// Lists the attachments matching the scope and the caller's CEL `filter`, + /// held to [`AttachmentEntityType`] even when no type is given. + /// + /// Makes one request only. The backend's page tokens do not advance past + /// the first page, so following one returns the same rows again. A page can + /// also come back short, or empty, with a token when the caller can't view + /// some of its files, so `has_more` comes from the token, not the row count. + pub async fn list_attachments( + &self, + entity_ids: Vec, + entity_type: Option, + filter: String, + limit: Option, + ) -> Result> { + let (page_size, record_limit) = common::paging(limit); + let filter = attachment_filter(&entity_ids, entity_type, &filter); + + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let filter = filter.clone(); + async move { + let mut client = RemoteFileServiceClient::new(channel); + client + .list_remote_files(ListRemoteFilesRequest { + page_size, + filter, + ..Default::default() + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to query file attachments")?; + + let ListRemoteFilesResponse { + mut remote_files, + next_page_token, + } = resp; + let has_more = remote_files.len() > record_limit || !next_page_token.is_empty(); + remote_files.truncate(record_limit); + Ok(common::Page { + items: remote_files, + has_more, + }) + } + + /// Mints a short-lived signed URL for one remote file's bytes. + pub async fn download_url(&self, remote_file_id: String) -> Result { + let channel = self.channel.clone(); + let resp = with_retry(&self.policy, move || { + let channel = channel.clone(); + let remote_file_id = remote_file_id.clone(); + async move { + let mut client = RemoteFileServiceClient::new(channel); + client + .get_remote_file_download_url(GetRemoteFileDownloadUrlRequest { + remote_file_id, + }) + .await + .map(|resp| resp.into_inner()) + } + }) + .await + .context("failed to get remote file download url")?; + if resp.download_url.is_empty() { + bail!("download url response was empty"); + } + Ok(resp.download_url) + } +} + /// REST endpoint and credential for calls the gRPC surface does not offer. /// Today that is one thing: the multipart remote-file upload that gives an /// artifact version its bytes. diff --git a/rust/crates/sift_mcp/src/service/remote_files/test.rs b/rust/crates/sift_mcp/src/service/remote_files/test.rs index 043bad1cc0..eb7ef28f2a 100644 --- a/rust/crates/sift_mcp/src/service/remote_files/test.rs +++ b/rust/crates/sift_mcp/src/service/remote_files/test.rs @@ -1,9 +1,263 @@ use std::io::Write; +use sift_rs::remote_files::v1::{ + GetRemoteFileDownloadUrlResponse, ListRemoteFilesResponse, RemoteFile, + remote_file_service_server::RemoteFileServiceServer, +}; +use sift_test_util::{ + grpc::memory_sift_channel, mock::remote_files::v1::MockRemoteFileServiceImpl, +}; use tempdir::TempDir; +use tokio::task::JoinHandle; +use tonic::{Code, Response, Status, transport::Server}; -use super::{RemoteFileUploader, RestConfig, mime_type_for}; +use super::{ + AttachmentEntityType, RemoteFileService, RemoteFileUploader, RestConfig, attachment_filter, + mime_type_for, +}; use crate::client_event::start_http_server; +use crate::policy::RetryPolicy; + +async fn service_with_mock(mock: MockRemoteFileServiceImpl) -> (RemoteFileService, 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(RemoteFileServiceServer::new(mock)) + .serve_with_incoming(tokio_stream::once(Ok::<_, std::io::Error>(server))) + .await + .unwrap(); + }); + + ( + RemoteFileService::new(channel, RetryPolicy::default()), + handle, + ) +} + +fn remote_file(remote_file_id: &str) -> RemoteFile { + RemoteFile { + remote_file_id: remote_file_id.into(), + entity_id: "run-1".into(), + file_name: format!("{remote_file_id}.csv"), + ..Default::default() + } +} + +fn ids(page: &crate::service::common::Page) -> Vec<&str> { + page.items + .iter() + .map(|file| file.remote_file_id.as_str()) + .collect() +} + +const ATTACHMENT_TYPES: &str = "(entity_type == \"runs\" || entity_type == \"assets\" || \ + entity_type == \"annotations\" || entity_type == \"annotation_logs\" || \ + entity_type == \"test_reports\" || entity_type == \"test_steps\")"; + +#[test] +fn unscoped_filter_holds_the_listing_to_attachment_types() { + assert_eq!(attachment_filter(&[], None, ""), ATTACHMENT_TYPES); + assert_eq!(attachment_filter(&[], None, " "), ATTACHMENT_TYPES); +} + +#[test] +fn filter_ands_ids_type_and_the_callers_expression() { + let ids = vec!["run-1".to_string(), " run-2 ".to_string()]; + assert_eq!( + attachment_filter( + &ids, + Some(AttachmentEntityType::Run), + "file_name.matches(\"(?i)log\") || file_name == \"a.csv\"" + ), + "entity_id in [\"run-1\", \"run-2\"] && entity_type == \"runs\" && \ + (file_name.matches(\"(?i)log\") || file_name == \"a.csv\")" + ); +} + +#[test] +fn filter_escapes_quotes_in_ids() { + let ids = vec!["a\"b".to_string()]; + assert_eq!( + attachment_filter(&ids, Some(AttachmentEntityType::AnnotationLog), ""), + "entity_id in [\"a\\\"b\"] && entity_type == \"annotation_logs\"" + ); +} + +#[test] +fn every_attachment_type_has_a_distinct_name_and_filter_value() { + let names: std::collections::HashSet<_> = AttachmentEntityType::ALL + .map(AttachmentEntityType::name) + .into(); + let values: std::collections::HashSet<_> = AttachmentEntityType::ALL + .map(AttachmentEntityType::filter_value) + .into(); + assert_eq!(names.len(), AttachmentEntityType::ALL.len()); + assert_eq!(values.len(), AttachmentEntityType::ALL.len()); +} + +#[tokio::test] +async fn list_attachments_sends_the_scoped_filter_and_limit() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files() + .withf(|req| { + let req = req.get_ref(); + req.filter == "entity_id in [\"run-1\"] && entity_type == \"runs\"" + && req.page_size == 50 + && req.page_token.is_empty() + }) + .times(1) + .returning(|_| { + Ok(Response::new(ListRemoteFilesResponse { + remote_files: vec![remote_file("rf-1")], + next_page_token: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + let page = service + .list_attachments( + vec!["run-1".into()], + Some(AttachmentEntityType::Run), + String::new(), + None, + ) + .await + .expect("list"); + assert_eq!(ids(&page), ["rf-1"]); + assert!(!page.has_more); +} + +#[tokio::test] +async fn list_attachments_reads_has_more_from_the_token_without_following_it() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files() + .withf(|req| req.get_ref().page_size == 5) + .times(1) + .returning(|_| { + Ok(Response::new(ListRemoteFilesResponse { + remote_files: vec![remote_file("rf-1"), remote_file("rf-2")], + next_page_token: "next".into(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + let page = service + .list_attachments(Vec::new(), None, String::new(), Some(5)) + .await + .expect("list"); + assert_eq!(ids(&page), ["rf-1", "rf-2"]); + assert!(page.has_more); +} + +#[tokio::test] +async fn list_attachments_reports_more_behind_an_empty_page() { + // The backend drops files the caller can't view after paging, so a page + // can be empty and still carry a token. + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files().times(1).returning(|_| { + Ok(Response::new(ListRemoteFilesResponse { + remote_files: Vec::new(), + next_page_token: "next".into(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + let page = service + .list_attachments(Vec::new(), None, String::new(), None) + .await + .expect("list"); + assert!(page.items.is_empty()); + assert!(page.has_more); +} + +#[tokio::test] +async fn list_attachments_truncates_to_limit() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files().returning(|_| { + Ok(Response::new(ListRemoteFilesResponse { + remote_files: vec![ + remote_file("rf-1"), + remote_file("rf-2"), + remote_file("rf-3"), + ], + next_page_token: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + let page = service + .list_attachments(Vec::new(), None, String::new(), Some(2)) + .await + .expect("list"); + assert_eq!(ids(&page), ["rf-1", "rf-2"]); + assert!(page.has_more); +} + +#[tokio::test] +async fn list_attachments_propagates_invalid_argument() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files() + .returning(|_| Err(Status::invalid_argument("bad filter"))); + + let (service, _h) = service_with_mock(mock).await; + let err = service + .list_attachments(Vec::new(), None, "file_name ==".into(), None) + .await + .expect_err("expected error"); + let status = err.downcast_ref::().expect("status"); + assert_eq!(status.code(), Code::InvalidArgument); +} + +#[tokio::test] +async fn download_url_returns_the_signed_url() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_get_remote_file_download_url() + .withf(|req| req.get_ref().remote_file_id == "rf-1") + .times(1) + .returning(|_| { + Ok(Response::new(GetRemoteFileDownloadUrlResponse { + download_url: "https://files.test.local/rf-1?sig=abc".into(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + let url = service.download_url("rf-1".into()).await.expect("url"); + assert_eq!(url, "https://files.test.local/rf-1?sig=abc"); +} + +#[tokio::test] +async fn download_url_rejects_an_empty_url() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_get_remote_file_download_url().returning(|_| { + Ok(Response::new(GetRemoteFileDownloadUrlResponse { + download_url: String::new(), + })) + }); + + let (service, _h) = service_with_mock(mock).await; + let err = service + .download_url("rf-1".into()) + .await + .expect_err("empty url is an error"); + assert!(err.to_string().contains("download url response was empty")); +} + +#[tokio::test] +async fn download_url_propagates_not_found() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_get_remote_file_download_url() + .returning(|_| Err(Status::not_found("remote file not found"))); + + let (service, _h) = service_with_mock(mock).await; + let err = service + .download_url("missing".into()) + .await + .expect_err("expected error"); + let status = err.downcast_ref::().expect("status"); + assert_eq!(status.code(), Code::NotFound); +} fn write_file(dir: &TempDir, name: &str, contents: &[u8]) -> std::path::PathBuf { let path = dir.path().join(name); diff --git a/rust/crates/sift_mcp/src/tool/file_attachments/mod.rs b/rust/crates/sift_mcp/src/tool/file_attachments/mod.rs new file mode 100644 index 0000000000..d6c7e05b5e --- /dev/null +++ b/rust/crates/sift_mcp/src/tool/file_attachments/mod.rs @@ -0,0 +1,222 @@ +use rmcp::{ + ErrorData, + handler::server::wrapper::Parameters, + model::CallToolResult, + schemars::{self, JsonSchema}, + tool, tool_router, +}; +use serde::Deserialize; + +use crate::{ + error::{self, from_anyhow}, + server::SiftMcpServer, + service::remote_files::AttachmentEntityType, + tool::common::{list_body, to_values}, +}; + +#[cfg(test)] +mod test; + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct ListFileAttachmentsParams { + entity_ids: Option>, + entity_type: Option, + filter: String, + limit: Option, + fields: Option>, +} + +#[derive(Debug, Deserialize, JsonSchema)] +pub struct DownloadFileAttachmentParams { + remote_file_id: String, +} + +/// Accepts the singular name, the plural the backend filters on, or the proto +/// name rows carry (`ENTITY_TYPE_RUN`), in any case. +fn parse_entity_type(value: Option) -> Result, ErrorData> { + let Some(value) = value else { + return Ok(None); + }; + let lowered = value.trim().to_ascii_lowercase(); + if lowered.is_empty() { + return Ok(None); + } + let name = lowered.strip_prefix("entity_type_").unwrap_or(&lowered); + AttachmentEntityType::ALL + .into_iter() + .find(|entity_type| name == entity_type.name() || name == entity_type.filter_value()) + .map(Some) + .ok_or_else(|| { + let expected = AttachmentEntityType::ALL + .map(|entity_type| format!("`{}`", entity_type.name())) + .join(", "); + ErrorData::invalid_params( + format!("unknown `entity_type` `{value}`; expected one of {expected}"), + None, + ) + }) +} + +/// An empty array is an error, not "no scope": it usually means an earlier +/// lookup found nothing, and listing every attachment in the organization +/// would answer a different question. +fn parse_entity_ids(entity_ids: Option>) -> Result, ErrorData> { + let Some(entity_ids) = entity_ids else { + return Ok(Vec::new()); + }; + if entity_ids.is_empty() { + return Err(ErrorData::invalid_params( + "`entity_ids` is empty. Resolve at least one run or asset id first, or omit \ + `entity_ids` only when the user wants files across every entity.", + None, + )); + } + if entity_ids.iter().any(|id| id.trim().is_empty()) { + return Err(ErrorData::invalid_params( + "`entity_ids` must not contain an empty id", + None, + )); + } + Ok(entity_ids) +} + +#[tool_router(router = file_attachments_router, vis = "pub(crate)")] +impl SiftMcpServer { + #[tool( + name = "list_file_attachments", + description = " + List the files attached to runs, assets, annotations and their comments, test reports, and test + steps. + + Each row is one file's metadata. Bytes are not returned; call `download_file_attachment` with a + row's `remote_file_id` for a download URL. Artifact files are not attachments and never appear + here; use the artifact tools for those. + + Output: + - `{ \"file_attachments\": [RemoteFile, ...] }`. Each item includes `remote_file_id`, + `entity_id`, `entity_type`, `file_name`, `file_mime_type`, `file_size` (bytes, as a + string), `description`, `metadata_values`, `remote_file_type`, `created_by_user_id`, and + `created_date`. `remote_file_type` is `REMOTE_FILE_TYPE_TELEMETRY` for the source file a + data import read. Rows come back sorted by file name. + - `count`: how many items THIS response carries — read it instead of + counting the array yourself. It is the size of the page you got back, not + how many files match. + - `has_more`: `true` when the service hit `limit` with matches left over, so + this page is not the whole set. Never report `count` as a total while + `has_more` is `true` — raise `limit` or narrow the scope and ask again. `count` can be + below `limit` while `has_more` is `true`, because the page drops files the caller can't view. + + Parameters: + - `entity_ids`: optional ids of the runs, assets, or other entities whose files to list. + Resolve a run or asset by name with `list_runs` or `list_assets` first. Must not be an + empty array; omit it to list across every entity. + - `entity_type`: optional; `run`, `asset`, `annotation`, `annotation_log` (a comment on an + annotation), `test_report`, or `test_step`. Scope by type here, never in `filter`: the + filter takes a different spelling from the `entity_type` rows carry and rejects theirs. + - `filter`: CEL expression ANDed with the scope above. Pass an empty string for none. + Filterable fields are `remote_file_id`, `entity_id`, `file_name`, and `metadata[\"\"]`. + When filtering or searching, use `file_name.matches(\"(?i)flight\")`, not `==`. Use `==` only + for an exact value from a prior result. `contains`/`startsWith`/`endsWith` are case-SENSITIVE: + `contains(\"Flight\")` silently misses `flight_log.csv`. `.` is a regex wildcard, so use + `contains` to match a full literal file name. + - `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. + - `fields`: optional array of field names to keep on each item, e.g. + `[\"file_name\"]`. Omit it for the full object. Names match case-insensitively + and ignore underscores and hyphens, so `remote_file_id`, `remoteFileId` and + `remote-file-id` all work. Any name that matched nothing on any returned item + is listed in `unmatched_fields`; an empty page reports none. + + Errors: + - `INVALID_PARAMS` for an unknown `entity_type`, an empty `entity_ids` array or id, or an + invalid `filter`, including an `entity_type` value copied from a row into it. + - `INTERNAL_ERROR` for upstream failures. + ", + annotations( + title = "file_attachments/list_file_attachments", + read_only_hint = true + ) + )] + pub async fn list_file_attachments( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(ListFileAttachmentsParams { + entity_ids, + entity_type, + filter, + limit, + fields, + }) = params; + + let entity_ids = parse_entity_ids(entity_ids)?; + let entity_type = parse_entity_type(entity_type)?; + + let page = self + .remote_file_service + .list_attachments(entity_ids, entity_type, filter, limit) + .await + .map_err(from_anyhow)?; + + Ok(CallToolResult::structured(list_body( + "file_attachments", + to_values(&page.items)?, + fields, + page.has_more, + ))) + } + + #[tool( + name = "download_file_attachment", + description = " + Get a signed download URL for one file from `list_file_attachments`. + + Output: + - `{ \"remote_file_id\": string, \"download_url\": string }`. `download_url` is a short-lived + signed URL that needs no Sift credentials, so any HTTP client can fetch the file from it. + + Parameters: + - `remote_file_id`: required, from a `list_file_attachments` row. + + Errors: + - `INVALID_PARAMS` if `remote_file_id` is empty. + - `RESOURCE_NOT_FOUND` if the file does not exist. + - `INTERNAL_ERROR` for upstream failures, including a stop signal when the caller cannot view + the file. + + Guidance: + - Fetch `download_url` yourself when you need the file's contents. + - When the user wants the file, give them `download_url` and tell them it expires soon. Call + this tool again for a fresh URL rather than reusing an old one. + - For an artifact's file, call `download_artifact` instead. + ", + annotations( + title = "file_attachments/download_file_attachment", + read_only_hint = true + ) + )] + pub async fn download_file_attachment( + &self, + params: Parameters, + ) -> error::McpResult { + let Parameters(DownloadFileAttachmentParams { remote_file_id }) = params; + let remote_file_id = remote_file_id.trim().to_owned(); + if remote_file_id.is_empty() { + return Err(ErrorData::invalid_params( + "`remote_file_id` must not be empty", + None, + )); + } + + let download_url = self + .remote_file_service + .download_url(remote_file_id.clone()) + .await + .map_err(from_anyhow)?; + + Ok(CallToolResult::structured(serde_json::json!({ + "remote_file_id": remote_file_id, + "download_url": download_url, + }))) + } +} diff --git a/rust/crates/sift_mcp/src/tool/file_attachments/test.rs b/rust/crates/sift_mcp/src/tool/file_attachments/test.rs new file mode 100644 index 0000000000..6a2962a54b --- /dev/null +++ b/rust/crates/sift_mcp/src/tool/file_attachments/test.rs @@ -0,0 +1,262 @@ +use rmcp::{handler::server::wrapper::Parameters, model::ErrorCode}; +use serde_json::json; +use sift_rs::remote_files::v1::{ + EntityType, GetRemoteFileDownloadUrlResponse, ListRemoteFilesResponse, RemoteFile, + remote_file_service_server::RemoteFileServiceServer, +}; +use sift_test_util::{ + grpc::memory_sift_channel, mock::remote_files::v1::MockRemoteFileServiceImpl, +}; +use tokio::task::JoinHandle; +use tonic::{Response, Status, transport::Server}; + +use super::{DownloadFileAttachmentParams, ListFileAttachmentsParams, parse_entity_type}; +use crate::{ + server::SiftMcpServer, service::remote_files::AttachmentEntityType, + tool::common::test_support::structured, +}; + +const ATTACHMENT_TYPES: &str = "(entity_type == \"runs\" || entity_type == \"assets\" || \ + entity_type == \"annotations\" || entity_type == \"annotation_logs\" || \ + entity_type == \"test_reports\" || entity_type == \"test_steps\")"; + +async fn server_with_mock(mock: MockRemoteFileServiceImpl) -> (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(RemoteFileServiceServer::new(mock)) + .serve_with_incoming(tokio_stream::once(Ok::<_, std::io::Error>(server))) + .await + .unwrap(); + }); + + ( + SiftMcpServer::new( + channel, + String::from("https://app.test.local"), + false, + false, + ), + handle, + ) +} + +fn list_params( + entity_ids: Option<&[&str]>, + entity_type: Option<&str>, + filter: &str, +) -> Parameters { + Parameters(ListFileAttachmentsParams { + entity_ids: entity_ids.map(|ids| ids.iter().map(|id| (*id).to_string()).collect()), + entity_type: entity_type.map(str::to_string), + filter: filter.into(), + limit: None, + fields: None, + }) +} + +#[test] +fn entity_type_accepts_singular_plural_and_proto_names() { + for value in ["run", "runs", "RUN", " Run ", "ENTITY_TYPE_RUN"] { + assert_eq!( + parse_entity_type(Some(value.into())).unwrap(), + Some(AttachmentEntityType::Run) + ); + } + for value in [ + "annotation_log", + "annotation_logs", + "ENTITY_TYPE_ANNOTATION_LOG", + ] { + assert_eq!( + parse_entity_type(Some(value.into())).unwrap(), + Some(AttachmentEntityType::AnnotationLog) + ); + } + for entity_type in AttachmentEntityType::ALL { + assert_eq!( + parse_entity_type(Some(entity_type.name().into())).unwrap(), + Some(entity_type) + ); + } + for value in [None, Some(String::new()), Some(" ".into())] { + assert_eq!(parse_entity_type(value).unwrap(), None); + } +} + +#[test] +fn entity_type_rejects_non_attachment_types() { + for value in ["artifact_versions", "chat_conversations", "canvas_version"] { + let err = parse_entity_type(Some(value.into())).unwrap_err(); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); + } +} + +#[tokio::test] +async fn list_file_attachments_sends_the_scoped_filter() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files() + .withf(|req| { + req.get_ref().filter + == "entity_id in [\"run-1\"] && entity_type == \"runs\" && (file_name.matches(\"(?i)log\"))" + }) + .times(1) + .returning(|_| { + Ok(Response::new(ListRemoteFilesResponse { + remote_files: vec![RemoteFile { + remote_file_id: "rf-1".into(), + entity_id: "run-1".into(), + entity_type: EntityType::Run as i32, + file_name: "flight_log.csv".into(), + file_size: 42, + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + let result = server + .list_file_attachments(list_params( + Some(&["run-1"]), + Some("run"), + "file_name.matches(\"(?i)log\")", + )) + .await + .expect("list"); + + let body = structured(result); + assert_eq!(body["count"], json!(1)); + assert_eq!(body["has_more"], json!(false)); + let file = &body["file_attachments"][0]; + assert_eq!(file["remoteFileId"], json!("rf-1")); + assert_eq!(file["entityType"], json!("ENTITY_TYPE_RUN")); + assert_eq!(file["fileName"], json!("flight_log.csv")); +} + +#[tokio::test] +async fn list_file_attachments_projects_fields() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files() + .withf(|req| req.get_ref().filter == ATTACHMENT_TYPES) + .returning(|_| { + Ok(Response::new(ListRemoteFilesResponse { + remote_files: vec![RemoteFile { + remote_file_id: "rf-1".into(), + file_name: "notes.pdf".into(), + ..Default::default() + }], + next_page_token: String::new(), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + let mut params = list_params(None, None, ""); + params.0.fields = Some(vec!["file_name".into()]); + let body = structured(server.list_file_attachments(params).await.expect("list")); + assert_eq!( + body["file_attachments"], + json!([{ "fileName": "notes.pdf" }]) + ); +} + +#[tokio::test] +async fn list_file_attachments_rejects_bad_scope_before_calling_the_service() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files().never(); + + let (server, _h) = server_with_mock(mock).await; + let err = server + .list_file_attachments(list_params(Some(&["run-1", " "]), None, "")) + .await + .expect_err("empty id"); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); + + // An empty array must not widen the listing to the whole organization. + let err = server + .list_file_attachments(list_params(Some(&[]), None, "")) + .await + .expect_err("empty entity_ids"); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); + + let err = server + .list_file_attachments(list_params(None, Some("artifact"), "")) + .await + .expect_err("unknown type"); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn list_file_attachments_maps_an_invalid_filter() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_list_remote_files() + .returning(|_| Err(Status::invalid_argument("bad filter"))); + + let (server, _h) = server_with_mock(mock).await; + let err = server + .list_file_attachments(list_params(None, None, "file_name ==")) + .await + .expect_err("invalid filter"); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn download_file_attachment_returns_the_signed_url() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_get_remote_file_download_url() + .withf(|req| req.get_ref().remote_file_id == "rf-1") + .times(1) + .returning(|_| { + Ok(Response::new(GetRemoteFileDownloadUrlResponse { + download_url: "https://files.test.local/rf-1?sig=abc".into(), + })) + }); + + let (server, _h) = server_with_mock(mock).await; + let result = server + .download_file_attachment(Parameters(DownloadFileAttachmentParams { + remote_file_id: " rf-1 ".into(), + })) + .await + .expect("download"); + assert_eq!( + structured(result), + json!({ + "remote_file_id": "rf-1", + "download_url": "https://files.test.local/rf-1?sig=abc", + }) + ); +} + +#[tokio::test] +async fn download_file_attachment_rejects_an_empty_id() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_get_remote_file_download_url().never(); + + let (server, _h) = server_with_mock(mock).await; + let err = server + .download_file_attachment(Parameters(DownloadFileAttachmentParams { + remote_file_id: " ".into(), + })) + .await + .expect_err("empty id"); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn download_file_attachment_maps_not_found() { + let mut mock = MockRemoteFileServiceImpl::new(); + mock.expect_get_remote_file_download_url() + .returning(|_| Err(Status::not_found("remote file not found"))); + + let (server, _h) = server_with_mock(mock).await; + let err = server + .download_file_attachment(Parameters(DownloadFileAttachmentParams { + remote_file_id: "missing".into(), + })) + .await + .expect_err("not found"); + assert_eq!(err.code, ErrorCode::RESOURCE_NOT_FOUND); +} diff --git a/rust/crates/sift_mcp/src/tool/mod.rs b/rust/crates/sift_mcp/src/tool/mod.rs index 95e2a3296f..585f4f042e 100644 --- a/rust/crates/sift_mcp/src/tool/mod.rs +++ b/rust/crates/sift_mcp/src/tool/mod.rs @@ -7,6 +7,7 @@ pub mod common; pub mod data; pub mod docs; pub mod explore; +pub mod file_attachments; pub mod ping; pub mod report_templates; pub mod reports; diff --git a/rust/crates/sift_mcp/src/tool_events.json b/rust/crates/sift_mcp/src/tool_events.json index 7ceb2c4d90..4650e15e92 100644 --- a/rust/crates/sift_mcp/src/tool_events.json +++ b/rust/crates/sift_mcp/src/tool_events.json @@ -16,6 +16,7 @@ "create_test_report": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_CREATE_TEST_REPORT", "create_user_defined_function": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_CREATE_USER_DEFINED_FUNCTION", "download_artifact": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_DOWNLOAD_ARTIFACT", + "download_file_attachment": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_DOWNLOAD_FILE_ATTACHMENT", "explore_url": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_EXPLORE_URL", "get_data": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_GET_DATA", "list_annotations": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_ANNOTATIONS", @@ -25,6 +26,7 @@ "list_calculated_channel_versions": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_CALCULATED_CHANNEL_VERSIONS", "list_calculated_channels": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_CALCULATED_CHANNELS", "list_channels": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_CHANNELS", + "list_file_attachments": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_FILE_ATTACHMENTS", "list_report_rule_summaries": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_REPORT_RULE_SUMMARIES", "list_report_templates": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_REPORT_TEMPLATES", "list_reports": "CLIENT_EVENT_USER_CALLED_MCP_TOOL_LIST_REPORTS", From 4d3f374a8de066ededb6c51e490185e618981c80 Mon Sep 17 00:00:00 2001 From: Bill Raj-Derouin Date: Wed, 14 Oct 2026 20:12:21 -0700 Subject: [PATCH 2/2] rust(docs): point agents at linked artifacts from the file attachment skill notes --- rust/crates/sift_cli/assets/skills/sift/SKILL.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rust/crates/sift_cli/assets/skills/sift/SKILL.md b/rust/crates/sift_cli/assets/skills/sift/SKILL.md index 7b01952378..ae65ee9691 100644 --- a/rust/crates/sift_cli/assets/skills/sift/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/sift/SKILL.md @@ -97,7 +97,10 @@ tool. scope it with `entity_ids`. `download_file_attachment` returns a short-lived signed URL for one file that you can fetch without Sift credentials. Use these when a user asks for a file attached to a run or asset, instead of sending them to - the run page to download it. + the run page to download it. Artifact files never appear there, so when the + user wants everything on a run or asset, also call `list_artifacts` with + `links.exists(l, l.entity_type == "RUN" && l.entity_id == "")` + (`"ASSET"` for an asset). - **Derived channels:** `list_calculated_channels`, `list_calculated_channel_versions`, `list_user_defined_functions`, `list_user_defined_function_versions`.