From db80370251260471919d4f527c2920ca4445a45e Mon Sep 17 00:00:00 2001 From: escott- Date: Mon, 5 Oct 2026 10:11:18 -0700 Subject: [PATCH] fix(vcs): send the field names the API requires for create_link vcs(action="create_link") posted source_type, source_id, target_type and target_id. The API's create-link request is vcs_object_type, vcs_object_id, cs_object_type and cs_object_id, all required, with both ids as UUIDs. The route has had that shape since the VCS backend was added, so the API refused every call from this tool; an agent reported "missing field vcs_object_type", the first of four it reported only one of. The tool's schema already documents source_* as the VCS object ("repo, pull_request, issue, or commit") and target_* as the ContextStream object ("project, doc, plan, task, todo, decision, or node"), so the handler now sends source_* as vcs_object_* and target_* as cs_object_*. No parameter or schema changes. Both ids are parsed as UUIDs before the request, so a bad id is named in the error and is not left to the API to reject. Tests (new vcs_tests.rs, the first for this tool) drive create_link through VcsTool against a local server that records the request: the exact body and path the API receives, trimming, and an error naming each missing or non-UUID field. They failed on the previous handler with the body {source_id, source_type, target_id, target_type} and pass now. The other vcs actions were not audited. This one had no tests, so a similar mismatch elsewhere would not have been caught either. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: escott- --- CHANGELOG.md | 6 + crates/mcp-tools/src/domains/vcs.rs | 21 ++- crates/mcp-tools/src/domains/vcs_tests.rs | 167 ++++++++++++++++++++++ 3 files changed, 190 insertions(+), 4 deletions(-) create mode 100644 crates/mcp-tools/src/domains/vcs_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4caede9..29c4705 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ default, so a fresh session's `resume` returns the previous session and not itself. The excluded id is the explicit `session_id`, then the id the server was initialized with, then the transport's MCP session id. +- `vcs`: `create_link` works. It sent `source_*` and `target_*` as the request + field names, where the API requires `vcs_object_type`, `vcs_object_id`, + `cs_object_type`, and `cs_object_id`, so the API refused every call with + "missing field `vcs_object_type`". The parameters are unchanged (`source_*` is + the VCS object, `target_*` the ContextStream object). Both ids must be UUIDs and + are checked before the request is sent. ## 1.0.13 diff --git a/crates/mcp-tools/src/domains/vcs.rs b/crates/mcp-tools/src/domains/vcs.rs index f925044..4b06785 100644 --- a/crates/mcp-tools/src/domains/vcs.rs +++ b/crates/mcp-tools/src/domains/vcs.rs @@ -1001,15 +1001,24 @@ impl ToolHandler for VcsTool { .map(|v| v.trim().to_string()) .filter(|v| !v.is_empty()) .ok_or_else(|| Error::Validation("target_id is required".to_string()))?; + // The tool documents `source_*` as the VCS object and `target_*` + // as the ContextStream object. The API names the same two sides + // `vcs_object_*` and `cs_object_*`, requires all four, and takes + // both ids as UUIDs. Sending the tool's own names made the API + // refuse every call ("missing field `vcs_object_type`"). + let source_uuid = Uuid::parse_str(&source_id) + .map_err(|_| Error::Validation("source_id must be a UUID".to_string()))?; + let target_uuid = Uuid::parse_str(&target_id) + .map_err(|_| Error::Validation("target_id must be a UUID".to_string()))?; let path = format!("{}/links", workspace_base); self.client .post( &path, serde_json::json!({ - "source_type": source_type, - "source_id": source_id, - "target_type": target_type, - "target_id": target_id + "vcs_object_type": source_type, + "vcs_object_id": source_uuid, + "cs_object_type": target_type, + "cs_object_id": target_uuid }), ) .await? @@ -1169,3 +1178,7 @@ pub fn register_vcs_tools( ) { registry.register("vcs", Arc::new(VcsTool::new(client))); } + +#[cfg(test)] +#[path = "vcs_tests.rs"] +mod tests; diff --git a/crates/mcp-tools/src/domains/vcs_tests.rs b/crates/mcp-tools/src/domains/vcs_tests.rs new file mode 100644 index 0000000..caf4fe6 --- /dev/null +++ b/crates/mcp-tools/src/domains/vcs_tests.rs @@ -0,0 +1,167 @@ +//! Tests for the VCS domain tool. + +use super::*; +use crate::registry::ToolHandler; +use crate::testing::TestFixtures; +use serde_json::json; + +/// The request line and JSON body the API receives from one `create_link` +/// call. Any other request the call makes is answered 404. +async fn create_link_request(fields: Value) -> (String, Value) { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + tokio::time::timeout(std::time::Duration::from_secs(10), async { + loop { + let (mut socket, _) = listener.accept().await.unwrap(); + let mut request = Vec::new(); + let (head, body) = loop { + let mut chunk = [0u8; 4096]; + let count = socket.read(&mut chunk).await.unwrap(); + assert!(count > 0, "request ended before its body"); + request.extend_from_slice(&chunk[..count]); + if let Some(end) = request.windows(4).position(|w| w == b"\r\n\r\n") { + let head = String::from_utf8_lossy(&request[..end]).to_string(); + let length: usize = head + .lines() + .filter_map(|line| line.split_once(':')) + .find(|(key, _)| key.eq_ignore_ascii_case("content-length")) + .map(|(_, value)| value.trim().parse().unwrap()) + .unwrap_or(0); + if request.len() >= end + 4 + length { + break (head, request[end + 4..end + 4 + length].to_vec()); + } + } + }; + let request_line = head.lines().next().unwrap_or_default().to_string(); + let is_link = request_line.starts_with("POST /api/v1/integrations/workspaces/") + && request_line.contains("/vcs/links"); + let (status, payload) = if is_link { + ("200 OK", json!({"id": Uuid::new_v4()}).to_string()) + } else { + ("404 Not Found", json!({"error": "not found"}).to_string()) + }; + socket + .write_all( + format!( + "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{payload}", + payload.len() + ) + .as_bytes(), + ) + .await + .unwrap(); + if is_link { + return (request_line, serde_json::from_slice::(&body).unwrap()); + } + } + }) + .await + .expect("bounded create_link request") + }); + + let mut config = TestFixtures::test_config(); + config.api_url = format!("http://{address}"); + let tool = VcsTool::new(ContextStreamClient::new(config)); + let mut input = json!({"action": "create_link"}); + for (key, value) in fields.as_object().expect("link fields") { + input[key] = value.clone(); + } + tool.execute(input).await.expect("create_link call"); + server.await.expect("create_link server") +} + +/// A link whose VCS side is `source_*` and whose ContextStream side is +/// `target_*`, as the tool's schema documents them. +fn link_fields() -> Value { + json!({ + "source_type": "pull_request", + "source_id": Uuid::new_v4(), + "target_type": "task", + "target_id": Uuid::new_v4(), + }) +} + +#[tokio::test] +async fn create_link_sends_the_field_names_the_api_requires() { + let workspace_id = Uuid::new_v4(); + let mut fields = link_fields(); + fields["workspace_id"] = json!(workspace_id); + let (request_line, body) = create_link_request(fields.clone()).await; + + assert!( + request_line.starts_with(&format!( + "POST /api/v1/integrations/workspaces/{workspace_id}/vcs/links" + )), + "unexpected request: {request_line}" + ); + // The API's request struct is `vcs_object_type`, `vcs_object_id`, + // `cs_object_type`, `cs_object_id`, all required. Comparing the whole body + // also proves the tool's own `source_*` and `target_*` names are not sent. + assert_eq!( + body, + json!({ + "vcs_object_type": "pull_request", + "vcs_object_id": fields["source_id"], + "cs_object_type": "task", + "cs_object_id": fields["target_id"], + }) + ); +} + +#[tokio::test] +async fn create_link_trims_its_fields_before_sending() { + let vcs_id = Uuid::new_v4(); + let cs_id = Uuid::new_v4(); + let (_, body) = create_link_request(json!({ + "source_type": " repo ", + "source_id": format!(" {vcs_id} "), + "target_type": " project ", + "target_id": format!("\t{cs_id}\n"), + })) + .await; + assert_eq!(body["vcs_object_type"], "repo"); + assert_eq!(body["vcs_object_id"], vcs_id.to_string()); + assert_eq!(body["cs_object_type"], "project"); + assert_eq!(body["cs_object_id"], cs_id.to_string()); +} + +#[tokio::test] +async fn create_link_names_the_field_that_is_missing_or_wrong() { + // Validation happens before any request, so no server is needed. + let tool = VcsTool::new(ContextStreamClient::new(TestFixtures::test_config())); + let full = link_fields(); + + for field in ["source_type", "source_id", "target_type", "target_id"] { + let mut input = full.clone(); + input["action"] = json!("create_link"); + input.as_object_mut().unwrap().remove(field); + let error = tool + .execute(input) + .await + .err() + .unwrap_or_else(|| panic!("a link without {field} must be refused")) + .to_string(); + assert!(error.contains(field), "{field}: {error}"); + } + + // The API takes both ids as UUIDs. Say so here, instead of passing the + // value on to be rejected by the server. + for field in ["source_id", "target_id"] { + let mut input = full.clone(); + input["action"] = json!("create_link"); + input[field] = json!("not-a-uuid"); + let error = tool + .execute(input) + .await + .err() + .unwrap_or_else(|| panic!("a non-UUID {field} must be refused")) + .to_string(); + assert!( + error.contains(field) && error.contains("UUID"), + "{field}: {error}" + ); + } +}