diff --git a/Cargo.toml b/Cargo.toml index 4319def0a..da6bd48e5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,6 +53,7 @@ include_dir = "0.7" indicatif = "0.18" indoc = "2.0" mcap = "0.25" +mime_guess = "2" parquet = "58.3.0" percent-encoding = "2" prost = "^0.14" @@ -65,7 +66,7 @@ pyo3-async-runtimes = { version = "0.28", features = ["tokio-runtime"] } pyo3-stub-gen = "0.10" rand = "0.10" reqwest = "0.13" -rmcp = "3.1.2" +rmcp = "3.2.0" semver = "1.0" serde = "^1.0" serde_json = "^1.0" diff --git a/rust/crates/sift_cli/CHANGELOG.md b/rust/crates/sift_cli/CHANGELOG.md index 69260fa8c..8a542e281 100644 --- a/rust/crates/sift_cli/CHANGELOG.md +++ b/rust/crates/sift_cli/CHANGELOG.md @@ -16,6 +16,13 @@ This project adheres to [Semantic Versioning](http://semver.org/). importing metadata records as run metadata (`--metadata-record`), recoverable-error handling (`--parse-error-policy`), and how variable-cardinality fields are imported (`--complex-types-import-mode`). +- `create_artifact` accepts a `file_path` and uploads the file as the new + version's content (streamed to Sift's file store, up to 1 GiB). The response + then carries the file's name, mime type, `remote_file_id`, and a signed + `download_url`. +- Updated `rmcp` to 3.2.0. A client that names protocol version `2026-07-28` + over a legacy `initialize` handshake is now answered with `2025-11-25`, as + that revision has no handshake; stateless 2026 clients are unaffected. - Added artifact MCP tools: `list_artifacts`, `download_artifact`, and `create_artifact`, backed by public `sift.artifacts.v1.ArtifactService`. `create_artifact` writes artifact metadata (and can link a new artifact to a diff --git a/rust/crates/sift_cli/assets/skills/sift/SKILL.md b/rust/crates/sift_cli/assets/skills/sift/SKILL.md index af38f1286..9ba2970b1 100644 --- a/rust/crates/sift_cli/assets/skills/sift/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/sift/SKILL.md @@ -140,10 +140,13 @@ exists. Send a rename on its own. The API applies a `name` change by itself and ignores every other field, so `update_user_defined_function` rejects `name` combined with anything else. -- **Create an artifact.** `create_artifact` with a `title` / `summary`. - Pass `conversation_id` to link it to a chat, and `authoring_kind=agent` when +- **Create an artifact.** `create_artifact` with a `title` / `summary`, and + `file_path` pointing at the local file that is the artifact's content — an + artifact without a file has nothing to preview or download. Pass + `conversation_id` to link it to a chat, and `authoring_kind=agent` when a Sift agent produced it. Append a version by passing the existing - `artifact_id`. Creating is gated by `--allow-create`; appending a version to + `artifact_id`. One artifact per real deliverable; never one per scratch + file. Creating is gated by `--allow-create`; appending a version to an existing artifact also needs `--allow-destructive`. Discover artifacts with `list_artifacts` (oldest first, no `order_by`); fetch a version or its `download_url` with `download_artifact`. diff --git a/rust/crates/sift_cli/src/cmd/mcp.rs b/rust/crates/sift_cli/src/cmd/mcp.rs index cdbc688aa..96206912c 100644 --- a/rust/crates/sift_cli/src/cmd/mcp.rs +++ b/rust/crates/sift_cli/src/cmd/mcp.rs @@ -58,6 +58,7 @@ pub async fn run(ctx: Context, args: McpArgs, app_uri: String) -> Result Result, client_event_config: Option, feature_flags: FeatureFlags, + rest_config: Option, ) -> Result<()> { let client_event_reporter = client_event::ClientEventReporter::from_config(client_event_config, &cli_version); @@ -145,6 +148,7 @@ pub async fn run_with_client_events( update_check, client_event_reporter, feature_flags, + rest_config, }, ) .await @@ -158,6 +162,7 @@ struct RunConfig { update_check: Option, client_event_reporter: client_event::ClientEventReporter, feature_flags: FeatureFlags, + rest_config: Option, } async fn run_server(credentials: Credentials, use_tls: bool, config: RunConfig) -> Result<()> { @@ -176,6 +181,7 @@ async fn run_server(credentials: Credentials, use_tls: bool, config: RunConfig) config.update_check, config.client_event_reporter, config.feature_flags, + config.rest_config, ) .serve(stdio()) .await diff --git a/rust/crates/sift_mcp/src/server/mod.rs b/rust/crates/sift_mcp/src/server/mod.rs index 902629cdc..2b595cf3b 100644 --- a/rust/crates/sift_mcp/src/server/mod.rs +++ b/rust/crates/sift_mcp/src/server/mod.rs @@ -170,9 +170,20 @@ impl SiftMcpServer { Some(update_check), ClientEventReporter::default(), FeatureFlags::default(), + None, ) } + /// Test-only: route artifact file uploads at a mock REST endpoint. + #[cfg(test)] + pub fn with_artifact_uploader( + mut self, + uploader: crate::service::remote_files::RemoteFileUploader, + ) -> Self { + self.artifact_service = self.artifact_service.with_uploader(uploader); + self + } + #[allow(clippy::too_many_arguments)] pub(crate) fn new_with_client_events( channel: SiftChannel, @@ -183,6 +194,7 @@ impl SiftMcpServer { update_check: Option, client_event_reporter: ClientEventReporter, feature_flags: FeatureFlags, + rest_config: Option, ) -> Self { // Add more routers here as new tool groups are introduced, e.g. // tool_router.merge(Self::ingestion_router()) @@ -217,7 +229,12 @@ impl SiftMcpServer { let retry_policy = RetryPolicy::default(); let annotation_service = AnnotationService::new(channel.clone(), retry_policy.clone()); - let artifact_service = ArtifactService::new(channel.clone(), retry_policy.clone()); + let mut artifact_service = ArtifactService::new(channel.clone(), retry_policy.clone()); + if let Some(rest_config) = rest_config { + artifact_service = artifact_service.with_uploader( + crate::service::remote_files::RemoteFileUploader::new(rest_config, &cli_version), + ); + } let asset_service = AssetService::new(channel.clone(), retry_policy.clone()); let calculated_channel_service = CalculatedChannelService::new(channel.clone(), retry_policy.clone()); diff --git a/rust/crates/sift_mcp/src/server/test.rs b/rust/crates/sift_mcp/src/server/test.rs index ca03f41dd..935291ba7 100644 --- a/rust/crates/sift_mcp/src/server/test.rs +++ b/rust/crates/sift_mcp/src/server/test.rs @@ -128,6 +128,7 @@ async fn server_with_feature_flags( update_check, client_event_reporter, feature_flags, + None, ), handle, ) @@ -392,6 +393,15 @@ fn modern_request(id: u64, method: &str) -> Value { }) } +/// A pre-2026 list result: a plain item array with none of the 2026 result +/// envelope fields. +fn assert_legacy_list_result<'a>(response: &'a Value, item_key: &str) -> &'a [Value] { + assert!(response["result"].get("resultType").is_none(), "{response}"); + assert!(response["result"].get("ttlMs").is_none(), "{response}"); + assert!(response["result"].get("cacheScope").is_none(), "{response}"); + response["result"][item_key].as_array().unwrap() +} + fn assert_modern_list_result<'a>(response: &'a Value, item_key: &str) -> &'a [Value] { assert_eq!(response["result"]["resultType"], "complete"); assert_eq!(response["result"]["ttlMs"], 0); @@ -738,8 +748,12 @@ async fn disabled_update_check_is_not_advertised() { finish(reader, writer, server).await; } +/// `2026-07-28` has no `initialize` handshake, so rmcp (3.2.0+) answers a +/// client that names it over `initialize` with the newest legacy version and +/// serves the session in that shape. Stateless 2026 clients carry the version +/// in per-request `_meta` instead; see the `stateless_2026_*` tests. #[tokio::test] -async fn claude_legacy_handshake_gets_complete_2026_list_results() { +async fn claude_legacy_handshake_naming_2026_is_negotiated_down() { let current = UpdateCheck::Current { current_version: "0.4.0".to_string(), latest_version: "0.4.0".to_string(), @@ -749,7 +763,7 @@ async fn claude_legacy_handshake_gets_complete_2026_list_results() { .await; let initialize = read_json(&mut reader).await; - assert_eq!(initialize["result"]["protocolVersion"], "2026-07-28"); + assert_eq!(initialize["result"]["protocolVersion"], "2025-11-25"); writer .write_all(b"{\"jsonrpc\":\"2.0\",\"method\":\"notifications/initialized\"}\n") @@ -760,7 +774,7 @@ async fn claude_legacy_handshake_gets_complete_2026_list_results() { .await .unwrap(); let tools = read_json(&mut reader).await; - let tools = assert_modern_list_result(&tools, "tools"); + let tools = assert_legacy_list_result(&tools, "tools"); assert!(tools.iter().any(|tool| tool["name"] == "list_assets")); assert!(tools.iter().any(|tool| tool["name"] == "check_for_updates")); @@ -769,7 +783,7 @@ async fn claude_legacy_handshake_gets_complete_2026_list_results() { .await .unwrap(); let prompts = read_json(&mut reader).await; - let prompts = assert_modern_list_result(&prompts, "prompts"); + let prompts = assert_legacy_list_result(&prompts, "prompts"); assert!( prompts .iter() diff --git a/rust/crates/sift_mcp/src/service/artifacts/mod.rs b/rust/crates/sift_mcp/src/service/artifacts/mod.rs index 02a6cd225..9c83800a3 100644 --- a/rust/crates/sift_mcp/src/service/artifacts/mod.rs +++ b/rust/crates/sift_mcp/src/service/artifacts/mod.rs @@ -12,8 +12,11 @@ use sift_rs::{ }, }; +use std::path::Path; + use crate::policy::{RetryPolicy, with_retry}; use crate::service::common; +use crate::service::remote_files::RemoteFileUploader; #[cfg(test)] mod test; @@ -30,11 +33,23 @@ pub struct ArtifactView { pub struct ArtifactService { channel: SiftChannel, policy: RetryPolicy, + // Absent only when the server runs without a REST endpoint (some tests); + // creating an artifact with a file requires it. + uploader: Option, } impl ArtifactService { pub fn new(channel: SiftChannel, policy: RetryPolicy) -> Self { - Self { channel, policy } + Self { + channel, + policy, + uploader: None, + } + } + + pub fn with_uploader(mut self, uploader: RemoteFileUploader) -> Self { + self.uploader = Some(uploader); + self } pub async fn list_artifacts( @@ -121,7 +136,17 @@ impl ArtifactService { conversation_id: Option, artifact_id: Option, authoring_kind: ArtifactAuthoringKind, + file_path: Option<&Path>, ) -> Result { + // Refuse before creating any rows, so a misconfigured server does not + // leave a byteless version behind. + let uploader = match file_path { + Some(_) => Some(self.uploader.as_ref().context( + "this server was started without a REST endpoint, so `file_path` is not supported", + )?), + None => None, + }; + let channel = self.channel.clone(); let created = with_retry(&self.policy, move || { let channel = channel.clone(); @@ -148,9 +173,46 @@ impl ArtifactService { .artifact .ok_or_else(|| anyhow!("create artifact response missing artifact"))?; + let (Some(uploader), Some(path)) = (uploader, file_path) else { + return Ok(ArtifactView { + inner: created, + download_url: None, + }); + }; + + // The version row exists from here on: a failed upload must say so, + // or the agent will retry the create and mint a duplicate artifact. + let upload_context = format!( + "artifact {} version {} was created, but uploading `{}` failed; do NOT create the artifact again", + created.artifact_id, + created.version, + path.display() + ); + uploader + .upload_artifact_version_file( + &created.organization_id, + &created.artifact_version_id, + path, + ) + .await + .context(upload_context)?; + + // Refresh so the returned artifact carries the uploaded file's name, + // mime type, and remote_file_id, and mint the download link. + let refreshed = self + .get_artifact( + created.artifact_id.clone(), + Some(created.artifact_version_id.clone()), + ) + .await + .unwrap_or(created); + let download_url = match refreshed.remote_file_id.clone() { + Some(remote_file_id) => self.download_url(remote_file_id).await.ok(), + None => None, + }; Ok(ArtifactView { - inner: created, - download_url: None, + inner: refreshed, + 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 6e89594d6..57f36800f 100644 --- a/rust/crates/sift_mcp/src/service/artifacts/test.rs +++ b/rust/crates/sift_mcp/src/service/artifacts/test.rs @@ -336,6 +336,7 @@ async fn create_artifact_returns_created_row() { Some("conv-1".into()), None, ArtifactAuthoringKind::Agent, + None, ) .await .expect("create"); diff --git a/rust/crates/sift_mcp/src/service/mod.rs b/rust/crates/sift_mcp/src/service/mod.rs index c603531a2..8ccaa277c 100644 --- a/rust/crates/sift_mcp/src/service/mod.rs +++ b/rust/crates/sift_mcp/src/service/mod.rs @@ -7,6 +7,7 @@ pub mod data; pub mod docs; pub mod ingest; pub mod ping; +pub mod remote_files; pub mod report_templates; pub mod reports; pub mod rule_evaluation; diff --git a/rust/crates/sift_mcp/src/service/remote_files/mod.rs b/rust/crates/sift_mcp/src/service/remote_files/mod.rs new file mode 100644 index 000000000..a9a8cd413 --- /dev/null +++ b/rust/crates/sift_mcp/src/service/remote_files/mod.rs @@ -0,0 +1,154 @@ +use std::path::Path; +use std::time::Duration; + +use anyhow::{Context, Result, bail}; +use reqwest::header::USER_AGENT; + +const UPLOAD_PATH: &str = "/api/v0/remote-files/upload"; +const CLIENT_NAME: &str = "sift_mcp"; +/// Client-side cap on one uploaded file. The server allows more, but an +/// artifact version larger than this is almost certainly a mistake (raw data +/// belongs in ingestion, not artifacts). +pub const MAX_UPLOAD_BYTES: u64 = 1 << 30; +/// Matches the server's file-name length cap on remote files. +const MAX_FILE_NAME_BYTES: usize = 255; +/// Sent when the file extension maps to no known mime type. The server stores +/// whatever the part declares, and the UI needs a non-empty type to decide +/// how to present the file. +const FALLBACK_MIME: &str = "application/octet-stream"; +/// Uploads stream from disk and can be large, so the request timeout is far +/// looser than the interactive-call default. +const UPLOAD_TIMEOUT: Duration = Duration::from_secs(10 * 60); + +/// 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. +#[derive(Clone)] +pub struct RestConfig { + pub rest_uri: String, + pub api_key: String, +} + +impl RestConfig { + pub fn new(rest_uri: String, api_key: String) -> Self { + Self { rest_uri, api_key } + } +} + +/// Uploads local files to the remote-files store over the REST multipart +/// endpoint, attaching each to one entity (for artifacts: entity type +/// `artifact_versions`, entity id = the version's id). +#[derive(Clone)] +pub struct RemoteFileUploader { + client: reqwest::Client, + endpoint: String, + api_key: String, + user_agent: String, +} + +impl RemoteFileUploader { + pub fn new(config: RestConfig, cli_version: &str) -> Self { + Self { + client: reqwest::Client::new(), + endpoint: format!("{}{UPLOAD_PATH}", config.rest_uri.trim_end_matches('/')), + api_key: config.api_key, + user_agent: format!("{CLIENT_NAME}/{cli_version}"), + } + } + + /// Streams one local file into remote_files as the bytes of an artifact + /// version. The part declares a mime type derived from the file extension + /// (the server's own extension table is small and misses `.md`, `.py`, + /// `.parquet`, ...), and the server binds the row to the version. + pub async fn upload_artifact_version_file( + &self, + organization_id: &str, + artifact_version_id: &str, + path: &Path, + ) -> Result<()> { + let file_name = validate_upload_path(path).await?; + + let file = tokio::fs::File::open(path) + .await + .with_context(|| format!("failed to open `{}`", path.display()))?; + let size = file + .metadata() + .await + .with_context(|| format!("failed to stat `{}`", path.display()))? + .len(); + + let mime_type = mime_type_for(&file_name); + let part = reqwest::multipart::Part::stream_with_length(reqwest::Body::from(file), size) + .file_name(file_name) + .mime_str(&mime_type) + .with_context(|| format!("invalid mime type `{mime_type}`"))?; + let form = reqwest::multipart::Form::new() + .text("organizationId", organization_id.to_string()) + .text("entityId", artifact_version_id.to_string()) + .text("entityType", "artifact_versions") + .part("file", part); + + let response = self + .client + .post(&self.endpoint) + .timeout(UPLOAD_TIMEOUT) + .bearer_auth(&self.api_key) + .header(USER_AGENT, &self.user_agent) + .multipart(form) + .send() + .await + .context("failed to reach the remote-file upload endpoint")?; + + let status = response.status(); + if !status.is_success() { + let detail = response.text().await.unwrap_or_default(); + let detail = detail.chars().take(512).collect::(); + bail!("remote-file upload returned HTTP {status}: {detail}"); + } + Ok(()) + } +} + +/// Mime type to declare for a file name, from its extension. Unknown or +/// missing extensions get [`FALLBACK_MIME`] rather than an empty type. +pub fn mime_type_for(file_name: &str) -> String { + mime_guess::from_path(file_name) + .first_raw() + .unwrap_or(FALLBACK_MIME) + .to_owned() +} + +/// Checks the path points at a regular, non-empty-named, size-capped file and +/// returns its file name. +async fn validate_upload_path(path: &Path) -> Result { + let metadata = tokio::fs::metadata(path) + .await + .with_context(|| format!("`{}` does not exist or is not readable", path.display()))?; + if !metadata.is_file() { + bail!("`{}` is not a regular file", path.display()); + } + if metadata.len() == 0 { + bail!("`{}` is empty; artifacts need content", path.display()); + } + if metadata.len() > MAX_UPLOAD_BYTES { + bail!( + "`{}` is {} bytes, above the {} byte artifact limit", + path.display(), + metadata.len(), + MAX_UPLOAD_BYTES + ); + } + let file_name = path + .file_name() + .and_then(|name| name.to_str()) + .map(str::to_owned) + .filter(|name| !name.is_empty()) + .with_context(|| format!("`{}` has no usable file name", path.display()))?; + if file_name.len() > MAX_FILE_NAME_BYTES { + bail!("file name `{file_name}` exceeds {MAX_FILE_NAME_BYTES} bytes"); + } + Ok(file_name) +} + +#[cfg(test)] +mod test; diff --git a/rust/crates/sift_mcp/src/service/remote_files/test.rs b/rust/crates/sift_mcp/src/service/remote_files/test.rs new file mode 100644 index 000000000..55022b12b --- /dev/null +++ b/rust/crates/sift_mcp/src/service/remote_files/test.rs @@ -0,0 +1,127 @@ +use std::io::Write; + +use tempdir::TempDir; + +use super::{RemoteFileUploader, RestConfig, mime_type_for}; +use crate::client_event::start_http_server; + +fn write_file(dir: &TempDir, name: &str, contents: &[u8]) -> std::path::PathBuf { + let path = dir.path().join(name); + let mut file = std::fs::File::create(&path).unwrap(); + file.write_all(contents).unwrap(); + path +} + +#[tokio::test] +async fn uploads_the_file_as_one_multipart_request() { + let (rest_uri, server) = start_http_server( + b"HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: 2\r\nconnection: close\r\n\r\n{}" + .to_vec(), + ) + .await; + let dir = TempDir::new("artifact-upload").unwrap(); + let path = write_file(&dir, "report.md", b"# Battery Report\n"); + + let uploader = RemoteFileUploader::new(RestConfig::new(rest_uri, "test-key".into()), "1.2.3"); + uploader + .upload_artifact_version_file("org-1", "ver-1", &path) + .await + .unwrap(); + + let request = String::from_utf8(server.await.unwrap()).unwrap(); + let (headers, body) = request.split_once("\r\n\r\n").unwrap(); + + assert!(headers.starts_with("POST /api/v0/remote-files/upload HTTP/1.1")); + assert!( + headers + .lines() + .any(|line| line.eq_ignore_ascii_case("authorization: Bearer test-key")) + ); + assert!( + headers + .lines() + .any(|line| line.eq_ignore_ascii_case("user-agent: sift_mcp/1.2.3")) + ); + + // Multipart form fields the upload handler parses. + assert!(body.contains("name=\"organizationId\"")); + assert!(body.contains("org-1")); + assert!(body.contains("name=\"entityId\"")); + assert!(body.contains("ver-1")); + assert!(body.contains("name=\"entityType\"")); + assert!(body.contains("artifact_versions")); + assert!(body.contains("name=\"file\"; filename=\"report.md\"")); + // The server's own extension table does not know `.md`, so the part must + // declare the type itself. + assert!( + body.to_ascii_lowercase() + .contains("content-type: text/markdown"), + "{body}" + ); + assert!(body.contains("# Battery Report")); +} + +#[tokio::test] +async fn a_failed_upload_reports_the_status_and_detail() { + let (rest_uri, server) = start_http_server( + b"HTTP/1.1 413 Payload Too Large\r\ncontent-type: application/json\r\ncontent-length: 24\r\nconnection: close\r\n\r\n{\"error\":\"file too big\"}" + .to_vec(), + ) + .await; + let dir = TempDir::new("artifact-upload").unwrap(); + let path = write_file(&dir, "export.csv", b"a,b\n1,2\n"); + + let uploader = RemoteFileUploader::new(RestConfig::new(rest_uri, "test-key".into()), "1.2.3"); + let error = uploader + .upload_artifact_version_file("org-1", "ver-1", &path) + .await + .unwrap_err(); + server.await.unwrap(); + + let message = format!("{error:#}"); + assert!(message.contains("413"), "{message}"); + assert!(message.contains("file too big"), "{message}"); +} + +#[tokio::test] +async fn rejects_missing_empty_and_directory_paths() { + let dir = TempDir::new("artifact-upload").unwrap(); + let uploader = RemoteFileUploader::new( + RestConfig::new("http://unused.test.local".into(), "test-key".into()), + "1.2.3", + ); + + let missing = dir.path().join("nope.md"); + let error = uploader + .upload_artifact_version_file("org-1", "ver-1", &missing) + .await + .unwrap_err(); + assert!(format!("{error:#}").contains("does not exist")); + + let empty = write_file(&dir, "empty.md", b""); + let error = uploader + .upload_artifact_version_file("org-1", "ver-1", &empty) + .await + .unwrap_err(); + assert!(format!("{error:#}").contains("is empty")); + + let error = uploader + .upload_artifact_version_file("org-1", "ver-1", dir.path()) + .await + .unwrap_err(); + assert!(format!("{error:#}").contains("not a regular file")); +} + +#[test] +fn mime_type_comes_from_the_extension_with_a_binary_fallback() { + assert_eq!(mime_type_for("report.md"), "text/markdown"); + assert_eq!(mime_type_for("export.csv"), "text/csv"); + assert_eq!(mime_type_for("plot.png"), "image/png"); + assert_eq!(mime_type_for("analysis.py"), "text/plain"); + assert_eq!( + mime_type_for("data.parquet"), + "application/vnd.apache.parquet" + ); + assert_eq!(mime_type_for("frames.sift"), "application/octet-stream"); + assert_eq!(mime_type_for("no_extension"), "application/octet-stream"); +} diff --git a/rust/crates/sift_mcp/src/startup.rs b/rust/crates/sift_mcp/src/startup.rs index 4ff118fae..1e8d34720 100644 --- a/rust/crates/sift_mcp/src/startup.rs +++ b/rust/crates/sift_mcp/src/startup.rs @@ -96,7 +96,9 @@ mod tests { let mut response = String::new(); reader.read_line(&mut response).await.unwrap(); let init_response: Value = serde_json::from_str(&response).unwrap(); - assert_eq!(init_response["result"]["protocolVersion"], "2026-07-28"); + // `2026-07-28` has no `initialize` handshake, so rmcp negotiates a + // client that names it there down to the newest legacy version. + assert_eq!(init_response["result"]["protocolVersion"], "2025-11-25"); assert_eq!( init_response["result"]["instructions"], "profile needs app_uri" diff --git a/rust/crates/sift_mcp/src/tool/artifacts/mod.rs b/rust/crates/sift_mcp/src/tool/artifacts/mod.rs index 25df75e39..db6bf7f07 100644 --- a/rust/crates/sift_mcp/src/tool/artifacts/mod.rs +++ b/rust/crates/sift_mcp/src/tool/artifacts/mod.rs @@ -38,6 +38,7 @@ pub struct CreateArtifactParams { conversation_id: Option, artifact_id: Option, authoring_kind: Option, + file_path: Option, } /// Also accepts the proto enum names so an agent can echo a value it read from `list_artifacts`. @@ -209,12 +210,14 @@ impl SiftMcpServer { #[tool( name = "create_artifact", description = " - Create a new artifact, or append a version to an existing one. This writes artifact metadata - only; version bytes live in remote_files and are not uploaded by this tool. + Create a new artifact, or append a version to an existing one, optionally uploading a local + file as the version's content. Output: - `{ \"artifact\": Artifact, \"next_step\": string }`. The returned artifact is the created or - appended version, including `artifact_id`, `artifact_version_id`, and `version`. + appended version, including `artifact_id`, `artifact_version_id`, and `version`. When a + file was uploaded it also carries `file_name`, `file_mime_type`, `remote_file_id`, and a + short-lived signed `download_url`. Parameters: - `title`: optional display title stored on the version. @@ -227,6 +230,10 @@ impl SiftMcpServer { proto names that `list_artifacts` / `download_artifact` emit (`ARTIFACT_AUTHORING_KIND_USER`, `ARTIFACT_AUTHORING_KIND_AGENT`) are also accepted. Use `agent` when a Sift agent is producing the artifact during a turn. + - `file_path`: optional absolute or relative path of a local file to upload as this + version's content. The file streams to Sift's file store; its name and extension drive + the mime type and how the UI previews it. Regular, non-empty files up to 1 GiB. + Omit it to record metadata only (content can not be attached later to the same version). Access: - Creating a new artifact needs `--allow-create`. @@ -235,14 +242,18 @@ impl SiftMcpServer { Errors: - `INVALID_PARAMS` if `authoring_kind` is not `user` or `agent`, if `conversation_id` is set - while appending, or if `artifact_id` / `conversation_id` is empty when set. + while appending, or if `artifact_id` / `conversation_id` / `file_path` is empty when set. - `INVALID_REQUEST` if the server was launched without the flag the call needs (see Access). - `RESOURCE_NOT_FOUND` if the conversation or existing artifact is not visible to the caller. - - `INTERNAL_ERROR` for upstream failures. + - `INTERNAL_ERROR` for upstream failures. When the message says the artifact was created but + the upload failed, the version exists without content — report that to the user instead of + calling `create_artifact` again, which would mint a duplicate. Guidance: - This is a write. CONFIRM the title and destination conversation with the user before invoking. - Edits always create a new version; there is no edit-in-place path. + - Prefer passing `file_path`: an artifact without content has nothing to preview or download. + - One artifact per real deliverable. Do not create artifacts for intermediate scratch files. ", annotations( title = "artifacts/create_artifact", @@ -263,6 +274,7 @@ impl SiftMcpServer { conversation_id, artifact_id, authoring_kind, + file_path, }) = params; // Appending rewrites what every linked conversation resolves to, so it takes the stronger gate. @@ -292,25 +304,51 @@ impl SiftMcpServer { None, )); } + if let Some(path) = file_path.as_deref() + && path.trim().is_empty() + { + return Err(ErrorData::invalid_params( + "`file_path` must not be empty when set", + None, + )); + } let authoring_kind = parse_authoring_kind(authoring_kind)?; let appending = artifact_id.is_some(); + let uploaded = file_path.is_some(); let artifact = self .artifact_service - .create_artifact(title, summary, conversation_id, artifact_id, authoring_kind) + .create_artifact( + title, + summary, + conversation_id, + artifact_id, + authoring_kind, + file_path.as_deref().map(std::path::Path::new), + ) .await .map_err(from_anyhow)?; + // The refresh and download-link steps after an upload are best-effort, so + // say only what the returned artifact actually carries. + let content_note = if uploaded && artifact.download_url.is_some() { + " Its file content was uploaded and the user can preview and download it." + } else if uploaded { + " Its file content was uploaded, but the refreshed artifact or its download link \ + could not be fetched; call `download_artifact` for the link." + } else { + " It has no file content; the user has nothing to preview or download." + }; let next_step = if appending { format!( - "Appended version {} to artifact {}. Surface the new version to the user and confirm it \ - matches their intent.", + "Appended version {} to artifact {}.{content_note} Surface the new version to the user \ + and confirm it matches their intent.", artifact.inner.version, artifact.inner.artifact_id ) } else { format!( - "Created artifact {} version {}. Surface the title and destination to the user and confirm \ - they match their intent before further edits.", + "Created artifact {} version {}.{content_note} Surface the title and destination to the \ + user and confirm they match their intent before further edits.", artifact.inner.artifact_id, artifact.inner.version ) }; diff --git a/rust/crates/sift_mcp/src/tool/artifacts/test.rs b/rust/crates/sift_mcp/src/tool/artifacts/test.rs index 5f2d52111..fd818d275 100644 --- a/rust/crates/sift_mcp/src/tool/artifacts/test.rs +++ b/rust/crates/sift_mcp/src/tool/artifacts/test.rs @@ -213,6 +213,7 @@ async fn create_artifact_blocked_without_allow_create() { conversation_id: None, artifact_id: None, authoring_kind: None, + file_path: None, })) .await .expect_err("gated"); @@ -236,6 +237,7 @@ async fn create_artifact_append_blocked_without_allow_destructive() { conversation_id: None, artifact_id: Some("art-1".into()), authoring_kind: None, + file_path: None, })) .await .expect_err("append gated"); @@ -269,6 +271,7 @@ async fn create_artifact_append_reports_appended_version() { conversation_id: None, artifact_id: Some("art-1".into()), authoring_kind: None, + file_path: None, })) .await .expect("append"); @@ -309,6 +312,7 @@ async fn create_artifact_accepts_authoring_kind_in_any_case() { conversation_id: None, artifact_id: None, authoring_kind: Some(input.into()), + file_path: None, })) .await .unwrap_or_else(|err| panic!("{input}: {err:?}")); @@ -325,6 +329,7 @@ async fn create_artifact_rejects_unknown_authoring_kind() { conversation_id: None, artifact_id: None, authoring_kind: Some("robot".into()), + file_path: None, })) .await .expect_err("unknown kind"); @@ -341,6 +346,7 @@ async fn create_artifact_rejects_append_with_conversation() { conversation_id: Some("conv-1".into()), artifact_id: Some("art-1".into()), authoring_kind: None, + file_path: None, })) .await .expect_err("illegal combo"); @@ -364,6 +370,7 @@ async fn create_artifact_happy_path() { conversation_id: Some("conv-1".into()), artifact_id: None, authoring_kind: Some("agent".into()), + file_path: None, })) .await .expect("create"); @@ -371,3 +378,218 @@ async fn create_artifact_happy_path() { assert_eq!(artifact["artifactId"], "art-1"); assert!(artifact.get("download_url").is_none()); } + +#[tokio::test] +async fn create_artifact_with_file_path_uploads_and_returns_the_refreshed_artifact() { + use std::io::Write as _; + + use crate::client_event::start_http_server; + use crate::service::remote_files::{RemoteFileUploader, RestConfig}; + + let dir = tempdir::TempDir::new("artifact-tool-upload").unwrap(); + let path = dir.path().join("report.md"); + std::fs::File::create(&path) + .unwrap() + .write_all(b"# Battery Report\n") + .unwrap(); + + let (rest_uri, rest_server) = start_http_server( + b"HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: 2\r\nconnection: close\r\n\r\n{}" + .to_vec(), + ) + .await; + + let mut mock = MockArtifactServiceImpl::new(); + mock.expect_create_artifact().returning(|_| { + Ok(Response::new(CreateArtifactResponse { + artifact: Some(sample_artifact()), + })) + }); + // The refresh after the upload returns the version with its file fields. + mock.expect_get_artifact().returning(|_| { + let mut uploaded = sample_artifact(); + uploaded.remote_file_id = Some("rf-1".into()); + uploaded.file_name = Some("report.md".into()); + Ok(Response::new(GetArtifactResponse { + artifact: Some(uploaded), + })) + }); + let mut remote_files = MockRemoteFileServiceImpl::new(); + remote_files + .expect_get_remote_file_download_url() + .returning(|_| { + Ok(Response::new(GetRemoteFileDownloadUrlResponse { + download_url: "https://files.test.local/rf-1".into(), + })) + }); + + let (server, _h) = server_with_mocks(mock, remote_files, true, true).await; + let server = server.with_artifact_uploader(RemoteFileUploader::new( + RestConfig::new(rest_uri, "test-key".into()), + "1.2.3", + )); + + let resp = server + .create_artifact(Parameters(CreateArtifactParams { + title: Some("report".into()), + summary: None, + conversation_id: None, + artifact_id: None, + authoring_kind: Some("agent".into()), + file_path: Some(path.to_string_lossy().into_owned()), + })) + .await + .expect("create with file"); + + let request = String::from_utf8(rest_server.await.unwrap()).unwrap(); + assert!(request.contains("name=\"entityId\"")); + assert!(request.contains("ver-1")); + assert!(request.contains("# Battery Report")); + + let artifact = structured_field(resp.clone(), "artifact"); + assert_eq!(artifact["remoteFileId"], "rf-1"); + assert_eq!(artifact["fileName"], "report.md"); + assert_eq!(artifact["download_url"], "https://files.test.local/rf-1"); + let next_step = structured_field(resp, "next_step"); + assert!( + next_step + .as_str() + .unwrap() + .contains("file content was uploaded"), + "{next_step}" + ); +} + +#[tokio::test] +async fn create_artifact_rejects_an_empty_file_path() { + let (server, _h) = server_with_mock(MockArtifactServiceImpl::new(), true).await; + let err = server + .create_artifact(Parameters(CreateArtifactParams { + title: None, + summary: None, + conversation_id: None, + artifact_id: None, + authoring_kind: None, + file_path: Some(" ".into()), + })) + .await + .expect_err("empty file_path"); + assert_eq!(err.code, ErrorCode::INVALID_PARAMS); +} + +#[tokio::test] +async fn create_artifact_names_the_created_artifact_when_the_upload_fails() { + use std::io::Write as _; + + use crate::client_event::start_http_server; + use crate::service::remote_files::{RemoteFileUploader, RestConfig}; + + let dir = tempdir::TempDir::new("artifact-tool-upload-fail").unwrap(); + let path = dir.path().join("report.md"); + std::fs::File::create(&path) + .unwrap() + .write_all(b"# Battery Report\n") + .unwrap(); + + let (rest_uri, rest_server) = start_http_server( + b"HTTP/1.1 500 Internal Server Error\r\ncontent-length: 0\r\nconnection: close\r\n\r\n" + .to_vec(), + ) + .await; + + let mut mock = MockArtifactServiceImpl::new(); + mock.expect_create_artifact().returning(|_| { + Ok(Response::new(CreateArtifactResponse { + artifact: Some(sample_artifact()), + })) + }); + + let (server, _h) = server_with_mocks(mock, MockRemoteFileServiceImpl::new(), true, true).await; + let server = server.with_artifact_uploader(RemoteFileUploader::new( + RestConfig::new(rest_uri, "test-key".into()), + "1.2.3", + )); + + let err = server + .create_artifact(Parameters(CreateArtifactParams { + title: None, + summary: None, + conversation_id: None, + artifact_id: None, + authoring_kind: Some("agent".into()), + file_path: Some(path.to_string_lossy().into_owned()), + })) + .await + .expect_err("upload failed"); + rest_server.await.unwrap(); + + let message = format!("{err:?}"); + assert!(message.contains("art-1"), "{message}"); + assert!( + message.contains("do NOT create the artifact again"), + "{message}" + ); +} + +#[tokio::test] +async fn create_artifact_with_file_path_says_so_when_the_download_link_is_missing() { + use std::io::Write as _; + + use crate::client_event::start_http_server; + use crate::service::remote_files::{RemoteFileUploader, RestConfig}; + + let dir = tempdir::TempDir::new("artifact-tool-upload-nolink").unwrap(); + let path = dir.path().join("report.md"); + std::fs::File::create(&path) + .unwrap() + .write_all(b"# Battery Report\n") + .unwrap(); + + let (rest_uri, rest_server) = start_http_server( + b"HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: 2\r\nconnection: close\r\n\r\n{}" + .to_vec(), + ) + .await; + + let mut mock = MockArtifactServiceImpl::new(); + mock.expect_create_artifact().returning(|_| { + Ok(Response::new(CreateArtifactResponse { + artifact: Some(sample_artifact()), + })) + }); + // The post-upload refresh fails, so the response has no file fields or link. + mock.expect_get_artifact() + .returning(|_| Err(tonic::Status::not_found("gone"))); + + let (server, _h) = server_with_mocks(mock, MockRemoteFileServiceImpl::new(), true, true).await; + let server = server.with_artifact_uploader(RemoteFileUploader::new( + RestConfig::new(rest_uri, "test-key".into()), + "1.2.3", + )); + + let resp = server + .create_artifact(Parameters(CreateArtifactParams { + title: Some("report".into()), + summary: None, + conversation_id: None, + artifact_id: None, + authoring_kind: Some("agent".into()), + file_path: Some(path.to_string_lossy().into_owned()), + })) + .await + .expect("upload succeeded even though the refresh failed"); + rest_server.await.unwrap(); + + let artifact = structured_field(resp.clone(), "artifact"); + assert!(artifact.get("download_url").is_none()); + let next_step = structured_field(resp, "next_step"); + let next_step = next_step.as_str().unwrap(); + assert!( + next_step.contains("call `download_artifact`"), + "{next_step}" + ); + assert!( + !next_step.contains("can preview and download"), + "{next_step}" + ); +}