diff --git a/rust/crates/sift_cli/CHANGELOG.md b/rust/crates/sift_cli/CHANGELOG.md index 77d3f0f027..2c072d9ed9 100644 --- a/rust/crates/sift_cli/CHANGELOG.md +++ b/rust/crates/sift_cli/CHANGELOG.md @@ -15,6 +15,17 @@ This project adheres to [Semantic Versioning](http://semver.org/). `report_template_url` link to the template, and `list_report_templates` adds a `url` to each template. Agents no longer link a new template to a report's page. +- The artifact tools now return the artifact's Sift web link. `list_artifacts` + rows carry `url`, and `list_artifact_versions`, `download_artifact`, + `create_artifact`, `update_artifact`, `archive_artifact`, and + `unarchive_artifact` return `artifact_url`. `download_artifact` links the + requested version when `artifact_version_id` is set. Agents link the artifact + page instead of the Sift home page. +- A name with a stray `[` or `]`, such as `results v2].csv`, no longer breaks + the Markdown link a tool tells the agent to show. +- The Sift skill now says to present a URL a tool returns as a Markdown link + named for the item, the same as the tools say. It used to ask for plain + text, so agents got conflicting instructions. ## [v0.7.3] - October 7, 2026 diff --git a/rust/crates/sift_cli/assets/skills/sift/SKILL.md b/rust/crates/sift_cli/assets/skills/sift/SKILL.md index fc50868944..d409e2797d 100644 --- a/rust/crates/sift_cli/assets/skills/sift/SKILL.md +++ b/rust/crates/sift_cli/assets/skills/sift/SKILL.md @@ -257,9 +257,10 @@ tool. - **Stop on empty list results.** If any `list_*` tool returns no items, tell the user that nothing matched and ask how to proceed. Do not make subsequent tool calls using an empty or placeholder name or id from the missing result. -- **Surface URLs as plain text, in full.** The link from `explore_url` and the - `View in Sift:` line from `sift-cli import` are deliverables. Never invent a - URL that a tool did not return. +- **Present a returned URL as a Markdown link.** Use the item's name as the + text, or what an Explore view shows, never a bare id or the word "link". The + link from `explore_url` and the `View in Sift:` line from `sift-cli import` + are deliverables. Never invent a URL that a tool did not return. - **Confirm every write before you run it.** Show the user the proposed change and its target, then wait for approval. - **Write tools are off by default.** Read-only is the default access mode. diff --git a/rust/crates/sift_cli/assets/skills/sift/references/cli.md b/rust/crates/sift_cli/assets/skills/sift/references/cli.md index b2a88a50c0..9ad92cc42e 100644 --- a/rust/crates/sift_cli/assets/skills/sift/references/cli.md +++ b/rust/crates/sift_cli/assets/skills/sift/references/cli.md @@ -92,8 +92,8 @@ per session. The rest apply to each subcommand invocation. accepts no `--wait`, `--preview`, or `--run`. 7. **Surface the Explore link from import output.** Each profile must set `app_uri`. `sift-cli import` prints a `View in Sift: ` tip when this - value is usable. Surface the URL as plain text, in full. Do not wrap it in a - markdown link. An incomplete old profile does not block the import. The CLI + value is usable. Present the URL as a Markdown link whose text names the + imported data. An incomplete old profile does not block the import. The CLI prints a config note instead. Relay that note. For an unknown domain, ask the user to copy the origin from their Sift web app. Do not assume a top-level domain. Do not invent a URL. diff --git a/rust/crates/sift_cli/assets/skills/sift/references/explore-links.md b/rust/crates/sift_cli/assets/skills/sift/references/explore-links.md index ab571b87d8..c39cc1b960 100644 --- a/rust/crates/sift_cli/assets/skills/sift/references/explore-links.md +++ b/rust/crates/sift_cli/assets/skills/sift/references/explore-links.md @@ -1,7 +1,7 @@ # Sift Explore links -Build the link with `explore_url`, then surface the URL to the user as plain -text, in full. +Build the link with `explore_url`, then present it to the user as a Markdown +link whose text names what the view shows. Send one source type. Pass `run_ids` when the request names a run and `asset_ids` otherwise. Use IDs returned by `list_runs` and `list_assets`; the diff --git a/rust/crates/sift_mcp/src/service/url/mod.rs b/rust/crates/sift_mcp/src/service/url/mod.rs index 51738a7d50..4058702dd7 100644 --- a/rust/crates/sift_mcp/src/service/url/mod.rs +++ b/rust/crates/sift_mcp/src/service/url/mod.rs @@ -179,6 +179,21 @@ impl UrlService { Ok(format!("{host}/run/{}", encode_value(run_id))) } + /// The artifact page follows the latest version unless `artifact_version_id` + /// pins it to one. + pub fn build_artifact_url( + &self, + artifact_id: &str, + artifact_version_id: Option<&str>, + ) -> Result { + let host = self.app_host()?; + let url = format!("{host}/artifacts/{}", encode_value(artifact_id)); + Ok(match artifact_version_id { + Some(version_id) => format!("{url}?artifactVersionId={}", encode_value(version_id)), + None => url, + }) + } + fn app_host(&self) -> Result<&str, ErrorData> { let host = self.app_uri.trim().trim_end_matches('/'); (!host.is_empty()).then_some(host).ok_or_else(|| { diff --git a/rust/crates/sift_mcp/src/service/url/test.rs b/rust/crates/sift_mcp/src/service/url/test.rs index 4d5efb1a39..bcbfd82475 100644 --- a/rust/crates/sift_mcp/src/service/url/test.rs +++ b/rust/crates/sift_mcp/src/service/url/test.rs @@ -214,3 +214,16 @@ fn annotation_asset_run_urls_use_singular_path_segments() { "https://app.siftstack.com/run/run-1" ); } + +#[test] +fn artifact_url_pins_a_version_only_when_given_one() { + let svc = service(); + assert_eq!( + svc.build_artifact_url("art-1", None).unwrap(), + "https://app.siftstack.com/artifacts/art-1" + ); + assert_eq!( + svc.build_artifact_url("art-1", Some("ver 2")).unwrap(), + "https://app.siftstack.com/artifacts/art-1?artifactVersionId=ver%202" + ); +} diff --git a/rust/crates/sift_mcp/src/tool/artifacts/mod.rs b/rust/crates/sift_mcp/src/tool/artifacts/mod.rs index 7c17609563..1eb34cb934 100644 --- a/rust/crates/sift_mcp/src/tool/artifacts/mod.rs +++ b/rust/crates/sift_mcp/src/tool/artifacts/mod.rs @@ -16,7 +16,7 @@ use crate::{ error::{self, from_anyhow}, server::SiftMcpServer, service::artifacts::{ArtifactView, CreateArtifactInput, UpdateArtifactInput, UpdatePath}, - tool::common::{MetadataEntry, list_body, to_values}, + tool::common::{MetadataEntry, list_body, to_values, url_clause}, }; #[cfg(test)] @@ -267,15 +267,47 @@ fn content_note(artifact: &ArtifactView, uploaded: bool) -> &'static str { } } -fn artifact_result(artifact: &ArtifactView, next_step: String) -> error::McpResult { +fn artifact_result( + artifact: &ArtifactView, + artifact_url: Option, + next_step: String, +) -> error::McpResult { let mut result = CallToolResult::structured(serde_json::json!({ "artifact": artifact_view_value(artifact)?, + "artifact_url": artifact_url, "next_step": next_step, })); result.content = vec![ContentBlock::text(next_step)]; Ok(result) } +/// Build the web URL for an artifact, pinned to `artifact_version_id` when set. +/// Returns `None` when `artifact_id` is empty or the host can't be derived, so +/// URL derivation never fails an operation. +fn artifact_url_for( + server: &SiftMcpServer, + artifact_id: &str, + artifact_version_id: Option<&str>, +) -> Option { + if artifact_id.is_empty() { + return None; + } + server + .url_service + .build_artifact_url(artifact_id, artifact_version_id) + .ok() +} + +/// The name the Sift UI shows for an artifact: its title, or its file name when +/// the title is blank. +fn display_name(artifact: &ArtifactView) -> Option { + let version = artifact.version(); + [version.title, version.file_name] + .into_iter() + .flatten() + .find(|name| !name.trim().is_empty()) +} + fn with_include_archived(filter: String, include_archived: bool) -> String { if !include_archived || filter.contains("include_archived") { return filter; @@ -306,7 +338,10 @@ impl SiftMcpServer { - `{ \"artifacts\": [Artifact, ...] }`. Each item includes `artifact_id`, `artifact_version_id`, `current_version_id`, `version`, `title`, `summary`, `storage_class`, `created_via`, `metadata`, `file_name`, `file_mime_type`, `remote_file_id`, `created_date` - (the artifact's), `version_created_date` (this version's), and `archived_date` when set. + (the artifact's), `version_created_date` (this version's), and `archived_date` when set, + plus an added `url` field with the artifact's Sift web link (`/artifacts/`). + `url` is omitted when the host can't be derived. Present each artifact to the user as a + Markdown link with its title as the text and `url` as the target. `payload` is NOT included, even for structured artifacts: read one with `download_artifact`. - `count`: how many items THIS response carries — read it instead of @@ -392,8 +427,20 @@ impl SiftMcpServer { let artifacts = page .items .iter() - .map(artifact_value) - .collect::, _>>()?; + .map(|details| { + let mut value = artifact_value(details)?; + let artifact_id = details + .artifact + .as_ref() + .map_or("", |artifact| artifact.artifact_id.as_str()); + if let (Value::Object(map), Some(url)) = + (&mut value, artifact_url_for(self, artifact_id, None)) + { + map.insert("url".to_string(), Value::String(url)); + } + Ok(value) + }) + .collect::, ErrorData>>()?; Ok(CallToolResult::structured(list_body( "artifacts", @@ -410,11 +457,14 @@ impl SiftMcpServer { version, so this is how you see what changed and when. Output: - - `{ \"artifact_versions\": [ArtifactVersion, ...] }`. Each item carries `artifact_version_id`, - `artifact_id`, `version`, `title`, `summary`, `metadata`, `file_name`, `file_mime_type`, - `remote_file_id`, `authoring_message_id`, `source_tool_use_ids`, and `created_date`. + - `{ \"artifact_versions\": [ArtifactVersion, ...], \"artifact_url\": string|null }`. Each item + carries `artifact_version_id`, `artifact_id`, `version`, `title`, `summary`, `metadata`, + `file_name`, `file_mime_type`, `remote_file_id`, `authoring_message_id`, + `source_tool_use_ids`, and `created_date`. `payload` is NOT included, even for structured artifacts: fetch one version's body with `download_artifact` and its `artifact_version_id`. + - `artifact_url`: the artifact's Sift web link (`/artifacts/`), or null when + the host can't be derived. - `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 versions the artifact has. @@ -445,7 +495,7 @@ impl SiftMcpServer { - The order is fixed newest first; there is no `filter` or `order_by` on this tool. Pass `limit: 1` to read just the current version. - Use `download_artifact` with an `artifact_version_id` from here to read that version's bytes - or JSON payload. + or JSON payload. Its `artifact_url` links that version when the user needs one. ", annotations(title = "artifacts/list_artifact_versions", read_only_hint = true) )] @@ -459,13 +509,16 @@ impl SiftMcpServer { fields, }) = params; - if artifact_id.trim().is_empty() { + // The service trims the id, so the link has to use the trimmed value too. + let artifact_id = artifact_id.trim().to_owned(); + if artifact_id.is_empty() { return Err(ErrorData::invalid_params( "`artifact_id` must not be empty", None, )); } + let artifact_url = artifact_url_for(self, &artifact_id, None); let page = self .artifact_service .list_artifact_versions(artifact_id, limit) @@ -473,13 +526,12 @@ impl SiftMcpServer { .map_err(from_anyhow)?; let versions = to_values(&page.items)?; + let mut body = list_body("artifact_versions", versions, fields, page.has_more); + if let Value::Object(map) = &mut body { + map.insert("artifact_url".to_string(), serde_json::json!(artifact_url)); + } - Ok(CallToolResult::structured(list_body( - "artifact_versions", - versions, - fields, - page.has_more, - ))) + Ok(CallToolResult::structured(body)) } #[tool( @@ -488,10 +540,16 @@ impl SiftMcpServer { Get one artifact by `artifact_id`, resolved to the latest version unless `artifact_version_id` pins one. Output: - - `{ \"artifact\": Artifact }`. Same chrome as `list_artifacts`, plus `download_url` when the - version has uploaded bytes (`remote_file_id` is set). `download_url` is a short-lived signed - URL; fetch it only when the user needs the body. Structured artifacts carry their JSON `payload` - directly. When `remote_file_id` is absent, `download_url` is omitted. + - `{ \"artifact\": Artifact, \"artifact_url\": string|null }`. `artifact` has the same fields as + a `list_artifacts` row except `url` (the link is `artifact_url` here), plus `download_url` when + the version has uploaded bytes (`remote_file_id` is set). `download_url` is a short-lived + signed URL; fetch it only when the user needs the body. + Structured artifacts carry their JSON `payload` directly. When `remote_file_id` is absent, + `download_url` is omitted. + - `artifact_url` is the artifact's Sift web link (`/artifacts/`), pinned to + the version when you passed `artifact_version_id`, or null when the host can't be derived. + Present the artifact to the user as a Markdown link with its title as the text and + `artifact_url` as the target. Never link `download_url` that way: it expires. Parameters: - `artifact_id`: required stable container id. @@ -532,15 +590,20 @@ impl SiftMcpServer { )); } + let pinned = artifact_version_id.is_some(); let artifact = self .artifact_service .download_artifact(artifact_id, artifact_version_id) .await .map_err(from_anyhow)?; - Ok(CallToolResult::structured( - serde_json::json!({ "artifact": artifact_view_value(&artifact)? }), - )) + let version_id = pinned.then(|| artifact.version().artifact_version_id); + let artifact_url = artifact_url_for(self, &artifact.artifact_id(), version_id.as_deref()); + + Ok(CallToolResult::structured(serde_json::json!({ + "artifact": artifact_view_value(&artifact)?, + "artifact_url": artifact_url, + }))) } #[tool( @@ -550,7 +613,9 @@ impl SiftMcpServer { This is a WRITE. Reversible with `unarchive_artifact`. Output: - - `{ \"artifact_id\": string, \"archived\": true, \"next_step\": string }`. + - `{ \"artifact_id\": string, \"archived\": true, \"artifact_url\": string|null, + \"next_step\": string }`. `artifact_url` is the artifact's Sift web link + (`/artifacts/`), or null when the host can't be derived. Parameters: - `artifact_id`: required stable artifact id. @@ -579,24 +644,30 @@ impl SiftMcpServer { ) -> error::McpResult { self.require_destructive()?; let Parameters(ArtifactArchiveParams { artifact_id }) = params; - if artifact_id.trim().is_empty() { + // The service trims the id, so the link has to use the trimmed value too. + let artifact_id = artifact_id.trim().to_owned(); + if artifact_id.is_empty() { return Err(ErrorData::invalid_params( "`artifact_id` must not be empty", None, )); } + let artifact_url = artifact_url_for(self, &artifact_id, None); self.artifact_service .archive_artifact(artifact_id.clone()) .await .map_err(from_anyhow)?; let next_step = format!( - "Archived artifact `{artifact_id}`. Tell the user it is hidden from default artifact listings and that `unarchive_artifact` restores it." + "Archived artifact `{artifact_id}`.{} Tell the user it is hidden from default artifact \ + listings and that `unarchive_artifact` restores it.", + url_clause("artifact", None, artifact_url.as_deref()), ); let mut result = CallToolResult::structured(serde_json::json!({ "artifact_id": artifact_id, "archived": true, + "artifact_url": artifact_url, "next_step": next_step, })); result.content = vec![ContentBlock::text(next_step)]; @@ -609,7 +680,9 @@ impl SiftMcpServer { Restore an archived artifact so it appears in default artifact listings again. This is a WRITE. Output: - - `{ \"artifact_id\": string, \"unarchived\": true, \"next_step\": string }`. + - `{ \"artifact_id\": string, \"unarchived\": true, \"artifact_url\": string|null, + \"next_step\": string }`. `artifact_url` is the artifact's Sift web link + (`/artifacts/`), or null when the host can't be derived. Parameters: - `artifact_id`: required stable artifact id. @@ -638,24 +711,30 @@ impl SiftMcpServer { ) -> error::McpResult { self.require_destructive()?; let Parameters(ArtifactArchiveParams { artifact_id }) = params; - if artifact_id.trim().is_empty() { + // The service trims the id, so the link has to use the trimmed value too. + let artifact_id = artifact_id.trim().to_owned(); + if artifact_id.is_empty() { return Err(ErrorData::invalid_params( "`artifact_id` must not be empty", None, )); } + let artifact_url = artifact_url_for(self, &artifact_id, None); self.artifact_service .unarchive_artifact(artifact_id.clone()) .await .map_err(from_anyhow)?; let next_step = format!( - "Unarchived artifact `{artifact_id}`. Tell the user it is restored to default artifact listings." + "Unarchived artifact `{artifact_id}`.{} Tell the user it is restored to default artifact \ + listings.", + url_clause("artifact", None, artifact_url.as_deref()), ); let mut result = CallToolResult::structured(serde_json::json!({ "artifact_id": artifact_id, "unarchived": true, + "artifact_url": artifact_url, "next_step": next_step, })); result.content = vec![ContentBlock::text(next_step)]; @@ -671,10 +750,11 @@ impl SiftMcpServer { with its `artifact_id`; calling this one again mints a duplicate the user has to reconcile. Output: - - `{ \"artifact\": Artifact, \"next_step\": string }`. The returned artifact is version 1, - 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`. + - `{ \"artifact\": Artifact, \"artifact_url\": string|null, \"next_step\": string }`. The + returned artifact is version 1, 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`. `artifact_url` is the artifact's Sift web link + (`/artifacts/`), or null when the host can't be derived. Parameters: - `title`: optional display title stored on the version. @@ -804,14 +884,20 @@ impl SiftMcpServer { .await .map_err(from_anyhow)?; + let artifact_url = artifact_url_for(self, &artifact.artifact_id(), None); let next_step = format!( - "Created artifact {} version {}.{} Surface the title and destination to the \ + "Created artifact {} version {}.{}{} Surface the title and destination to the \ user and confirm they match their intent before further edits.", artifact.artifact_id(), artifact.version().version, content_note(&artifact, uploaded), + url_clause( + "artifact", + display_name(&artifact).as_deref(), + artifact_url.as_deref() + ), ); - artifact_result(&artifact, next_step) + artifact_result(&artifact, artifact_url, next_step) } #[tool( @@ -824,8 +910,9 @@ impl SiftMcpServer { nothing for a field means `leave it alone`, never `blank it`. Output: - - `{ \"artifact\": Artifact, \"next_step\": string }`. Same shape as `download_artifact`, - resolved to the version this call wrote. + - `{ \"artifact\": Artifact, \"artifact_url\": string|null, \"next_step\": string }`. Same + shape as `download_artifact`, resolved to the version this call wrote. `artifact_url` links + the artifact rather than this version, so it keeps showing the latest. Parameters: - `artifact_id`: required stable container id. Resolve it with `list_artifacts` if you only @@ -936,13 +1023,19 @@ impl SiftMcpServer { .await .map_err(from_anyhow)?; + let artifact_url = artifact_url_for(self, &artifact.artifact_id(), None); let next_step = format!( - "Wrote version {} of artifact {}.{} Surface the new version to the user and confirm it \ + "Wrote version {} of artifact {}.{}{} Surface the new version to the user and confirm it \ matches their intent.", artifact.version().version, artifact.artifact_id(), content_note(&artifact, uploaded), + url_clause( + "artifact", + display_name(&artifact).as_deref(), + artifact_url.as_deref() + ), ); - artifact_result(&artifact, next_step) + artifact_result(&artifact, artifact_url, next_step) } } diff --git a/rust/crates/sift_mcp/src/tool/artifacts/test.rs b/rust/crates/sift_mcp/src/tool/artifacts/test.rs index 82be551202..0f2d7ebc8e 100644 --- a/rust/crates/sift_mcp/src/tool/artifacts/test.rs +++ b/rust/crates/sift_mcp/src/tool/artifacts/test.rs @@ -20,10 +20,11 @@ use tonic::{Response, Status, transport::Server}; use super::{ ArtifactArchiveParams, ArtifactVersionListParams, CreateArtifactParams, DownloadArtifactParams, - UpdateArtifactParams, parse_created_via, parse_storage_class, + UpdateArtifactParams, display_name, parse_created_via, parse_storage_class, }; use crate::{ server::SiftMcpServer, + service::artifacts::ArtifactView, tool::{ artifacts::ArtifactListParams, common::{ @@ -101,6 +102,29 @@ fn parse_entity_types_uses_the_proto_enum() { } } +#[test] +fn display_name_falls_back_to_the_file_name_for_a_blank_title() { + let view = |title: Option<&str>, file_name: Option<&str>| ArtifactView { + details: sample_with( + |_| {}, + |version| { + version.title = title.map(Into::into); + version.file_name = file_name.map(Into::into); + }, + ), + download_url: None, + }; + assert_eq!( + display_name(&view(Some("report"), Some("data.csv"))).as_deref(), + Some("report") + ); + assert_eq!( + display_name(&view(Some(" "), Some("data.csv"))).as_deref(), + Some("data.csv") + ); + assert_eq!(display_name(&view(None, None)), None); +} + async fn server_with_mock( mock: MockArtifactServiceImpl, allow_create: bool, @@ -185,6 +209,10 @@ async fn list_artifacts_returns_rows() { let artifacts = structured_field(resp, "artifacts"); assert_eq!(artifacts.as_array().unwrap().len(), 1); assert_eq!(artifacts[0]["artifactId"], "art-1"); + assert_eq!( + artifacts[0]["url"], + "https://app.test.local/artifacts/art-1" + ); } #[tokio::test] @@ -240,10 +268,31 @@ async fn get_artifact_returns_snake_case_download_url() { })) .await .expect("get"); - let artifact = structured_field(resp, "artifact"); + let body = structured(resp); + let artifact = &body["artifact"]; assert_eq!(artifact["artifactId"], "art-1"); assert_eq!(artifact["download_url"], "https://files.test.local/rf-1"); assert!(artifact.get("downloadUrl").is_none()); + assert_eq!( + body["artifact_url"], + "https://app.test.local/artifacts/art-1" + ); +} + +#[tokio::test] +async fn get_artifact_pins_the_artifact_url_to_a_requested_version() { + let (server, _h) = server_with_mock(get_returns(sample_artifact()), false).await; + let resp = server + .download_artifact(Parameters(DownloadArtifactParams { + artifact_id: "art-1".into(), + artifact_version_id: Some("ver-1".into()), + })) + .await + .expect("get"); + assert_eq!( + structured_field(resp, "artifact_url"), + "https://app.test.local/artifacts/art-1?artifactVersionId=ver-1" + ); } #[tokio::test] @@ -338,11 +387,17 @@ async fn list_artifact_versions_returns_rows_newest_first() { })) .await .expect("list versions"); - let versions = structured_field(resp, "artifact_versions"); - let rows = versions.as_array().expect("rows"); + let body = structured(resp); + let rows = body["artifact_versions"].as_array().expect("rows"); assert_eq!(rows.len(), 2); assert_eq!(rows[0]["artifactVersionId"], "ver-2"); assert_eq!(rows[1]["artifactVersionId"], "ver-1"); + assert!(rows.iter().all(|row| row.get("url").is_none())); + assert_eq!(body["count"], 2); + assert_eq!( + body["artifact_url"], + "https://app.test.local/artifacts/art-1" + ); } #[tokio::test] @@ -359,6 +414,33 @@ async fn list_artifact_versions_rejects_empty_artifact_id() { assert_eq!(err.code, ErrorCode::INVALID_PARAMS); } +#[tokio::test] +async fn list_artifact_versions_trims_the_artifact_id() { + let mut mock = MockArtifactServiceImpl::new(); + mock.expect_list_artifact_versions() + .withf(|request| request.get_ref().artifact_id == "art-1") + .returning(|_| { + Ok(Response::new(ListArtifactVersionsResponse { + versions: Vec::new(), + next_page_token: String::new(), + })) + }); + + let (server, _h) = server_with_mock(mock, true).await; + let resp = server + .list_artifact_versions(Parameters(ArtifactVersionListParams { + artifact_id: " art-1\n".into(), + limit: None, + fields: None, + })) + .await + .expect("list versions"); + assert_eq!( + structured_field(resp, "artifact_url"), + "https://app.test.local/artifacts/art-1" + ); +} + #[tokio::test] async fn archive_artifact_blocked_without_allow_destructive() { let (server, _h) = server_with_mocks( @@ -438,7 +520,15 @@ async fn archive_artifact_returns_structured_result() { let body = structured(response); assert_eq!(body["artifact_id"], "art-1"); assert_eq!(body["archived"], true); - assert!(body["next_step"].is_string()); + assert_eq!( + body["artifact_url"], + "https://app.test.local/artifacts/art-1" + ); + let next_step = body["next_step"].as_str().unwrap(); + assert!( + next_step.contains("https://app.test.local/artifacts/art-1"), + "{next_step}" + ); } #[tokio::test] @@ -458,7 +548,47 @@ async fn unarchive_artifact_returns_structured_result() { let body = structured(response); assert_eq!(body["artifact_id"], "art-1"); assert_eq!(body["unarchived"], true); - assert!(body["next_step"].is_string()); + assert_eq!( + body["artifact_url"], + "https://app.test.local/artifacts/art-1" + ); + let next_step = body["next_step"].as_str().unwrap(); + assert!( + next_step.contains("https://app.test.local/artifacts/art-1"), + "{next_step}" + ); +} + +#[tokio::test] +async fn archive_and_unarchive_trim_the_artifact_id() { + let mut mock = MockArtifactServiceImpl::new(); + mock.expect_archive_artifact() + .withf(|req| req.get_ref().artifact_id == "art-1") + .returning(|_| Ok(Response::new(ArchiveArtifactResponse {}))); + mock.expect_unarchive_artifact() + .withf(|req| req.get_ref().artifact_id == "art-1") + .returning(|_| Ok(Response::new(UnarchiveArtifactResponse {}))); + + let (server, _h) = server_with_mock(mock, true).await; + let archived = server + .archive_artifact(Parameters(ArtifactArchiveParams { + artifact_id: " art-1\n".into(), + })) + .await + .expect("archive"); + let unarchived = server + .unarchive_artifact(Parameters(ArtifactArchiveParams { + artifact_id: " art-1\n".into(), + })) + .await + .expect("unarchive"); + for body in [structured(archived), structured(unarchived)] { + assert_eq!(body["artifact_id"], "art-1"); + assert_eq!( + body["artifact_url"], + "https://app.test.local/artifacts/art-1" + ); + } } #[tokio::test] @@ -538,12 +668,20 @@ async fn update_artifact_masks_only_what_was_passed() { let artifact = structured_field(resp.clone(), "artifact"); assert_eq!(artifact["version"], 2); assert_eq!(artifact["title"], "Renamed"); + assert_eq!( + structured_field(resp.clone(), "artifact_url"), + "https://app.test.local/artifacts/art-1" + ); let next_step = structured_field(resp, "next_step"); let next_step = next_step.as_str().unwrap(); assert!( next_step.starts_with("Wrote version 2 of artifact art-1"), "{next_step}" ); + assert!( + next_step.contains("[Renamed](https://app.test.local/artifacts/art-1)"), + "{next_step}" + ); } #[tokio::test] @@ -884,9 +1022,18 @@ async fn create_artifact_happy_path() { })) .await .expect("create"); - let artifact = structured_field(resp, "artifact"); - assert_eq!(artifact["artifactId"], "art-1"); - assert!(artifact.get("download_url").is_none()); + let body = structured(resp); + assert_eq!(body["artifact"]["artifactId"], "art-1"); + assert!(body["artifact"].get("download_url").is_none()); + assert_eq!( + body["artifact_url"], + "https://app.test.local/artifacts/art-1" + ); + let next_step = body["next_step"].as_str().unwrap(); + assert!( + next_step.contains("[report](https://app.test.local/artifacts/art-1)"), + "{next_step}" + ); } #[tokio::test] diff --git a/rust/crates/sift_mcp/src/tool/common/mod.rs b/rust/crates/sift_mcp/src/tool/common/mod.rs index 4a13d936e8..c59964d1bf 100644 --- a/rust/crates/sift_mcp/src/tool/common/mod.rs +++ b/rust/crates/sift_mcp/src/tool/common/mod.rs @@ -45,7 +45,10 @@ pub(crate) fn url_clause(kind: &str, name: Option<&str>, url: Option<&str>) -> S return String::new(); }; match name { - Some(name) => format!(" Present it to the user as the Markdown link [{name}]({url})."), + Some(name) => format!( + " Present it to the user as the Markdown link [{}]({url}).", + escape_link_text(name) + ), None => format!( " Present the {kind} to the user as a Markdown link to {url} whose text is the {kind}'s \ name, never a bare id or the word \"link\"." @@ -53,6 +56,20 @@ pub(crate) fn url_clause(kind: &str, name: Option<&str>, url: Option<&str>) -> S } } +/// Backslash-escape the characters that end or nest Markdown link text. An +/// unbalanced bracket, as in `results v2].csv`, otherwise stops the text from +/// rendering as a link at all. +fn escape_link_text(text: &str) -> String { + let mut escaped = String::with_capacity(text.len()); + for c in text.chars() { + if matches!(c, '\\' | '[' | ']') { + escaped.push('\\'); + } + escaped.push(c); + } + escaped +} + /// Serialize each item to JSON and inject a `url` field built by `url_of`, so a /// listing surfaces a clickable Sift web link per row. Items whose url can't be /// built (host underivable on self-hosted deployments) are returned unchanged, diff --git a/rust/crates/sift_mcp/src/tool/common/test.rs b/rust/crates/sift_mcp/src/tool/common/test.rs index 41dd06f3e7..f3bdf00200 100644 --- a/rust/crates/sift_mcp/src/tool/common/test.rs +++ b/rust/crates/sift_mcp/src/tool/common/test.rs @@ -34,6 +34,18 @@ fn url_clause_with_name_renders_a_named_markdown_link() { ); } +#[test] +fn url_clause_escapes_brackets_and_backslashes_in_the_name() { + assert_eq!( + url_clause( + "artifact", + Some(r"results v2].csv [draft\"), + Some("https://app.siftstack.com/artifacts/a-1") + ), + r" Present it to the user as the Markdown link [results v2\].csv \[draft\\](https://app.siftstack.com/artifacts/a-1)." + ); +} + #[test] fn url_clause_without_name_asks_the_model_to_name_the_entity() { let clause = url_clause("rule", None, Some("https://app.siftstack.com/rules/r-1"));