huskies: merge 1225 bug Work-item tools use active project as an implicit global; project arg ignored, creates/reads misfile silently
This commit is contained in:
@@ -350,7 +350,25 @@ pub async fn gateway_mcp_post_handler(
|
||||
Some(project) => {
|
||||
proxy_and_respond_for_project(&state, project, &bytes, rpc.id).await
|
||||
}
|
||||
None => proxy_and_respond(&state, &bytes, rpc.id).await,
|
||||
None if is_create_tool(tool_name) => {
|
||||
// Story 1225 AC 3: with >1 project registered, a
|
||||
// create call omitting `project` is ambiguous — fail
|
||||
// loudly instead of silently filing into whichever
|
||||
// project happens to be active.
|
||||
let project_count = state.projects.read().await.len();
|
||||
if project_count > 1 {
|
||||
to_json_response(JsonRpcResponse::error(
|
||||
rpc.id,
|
||||
-32602,
|
||||
format!(
|
||||
"'{tool_name}' requires an explicit `project` argument when more than one project is registered (see list_projects) — the active project is not used implicitly for creates."
|
||||
),
|
||||
))
|
||||
} else {
|
||||
proxy_and_respond_with_resolved_project(&state, &bytes, rpc.id).await
|
||||
}
|
||||
}
|
||||
None => proxy_and_respond_with_resolved_project(&state, &bytes, rpc.id).await,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -376,6 +394,50 @@ async fn proxy_and_respond(state: &GatewayState, bytes: &[u8], id: Option<Value>
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns `true` for tool names that create a new work item or resource
|
||||
/// (`create_bug`, `create_story`, `create_worktree`, ...), used to gate
|
||||
/// implicit active-project routing behind an explicit `project` argument
|
||||
/// when the destination is ambiguous (story 1225 AC 3).
|
||||
fn is_create_tool(tool_name: &str) -> bool {
|
||||
tool_name.starts_with("create_")
|
||||
}
|
||||
|
||||
/// Proxy a request to the active project and, on success, annotate the
|
||||
/// response with which project actually handled it (story 1225 AC 3) — so a
|
||||
/// caller who omitted `project` can see whether their call landed where they
|
||||
/// expected instead of silently trusting `active_project`.
|
||||
async fn proxy_and_respond_with_resolved_project(
|
||||
state: &GatewayState,
|
||||
bytes: &[u8],
|
||||
id: Option<Value>,
|
||||
) -> Response {
|
||||
let active = state.active_project.read().await.clone();
|
||||
match state.proxy_active_mcp(bytes).await {
|
||||
Ok(resp_body) => Response::builder()
|
||||
.status(StatusCode::OK)
|
||||
.header("Content-Type", "application/json")
|
||||
.body(Body::from(annotate_resolved_project(&resp_body, &active))),
|
||||
Err(e) => to_json_response(JsonRpcResponse::error(
|
||||
id,
|
||||
-32603,
|
||||
format!("proxy error: {e}"),
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
/// Insert a `resolved_project` field into a JSON-RPC `result` object,
|
||||
/// leaving the bytes unchanged if they don't parse as JSON or `result` isn't
|
||||
/// an object (e.g. an error response, which has no `result` at all).
|
||||
fn annotate_resolved_project(bytes: &[u8], project: &str) -> Vec<u8> {
|
||||
let Ok(mut value) = serde_json::from_slice::<Value>(bytes) else {
|
||||
return bytes.to_vec();
|
||||
};
|
||||
if let Some(result) = value.get_mut("result").and_then(|r| r.as_object_mut()) {
|
||||
result.insert("resolved_project".to_string(), json!(project));
|
||||
}
|
||||
serde_json::to_vec(&value).unwrap_or_else(|_| bytes.to_vec())
|
||||
}
|
||||
|
||||
/// Proxy a request to an explicitly named project (story 1208 AC 1) rather
|
||||
/// than whatever project is currently active, so a single ops/LLM session
|
||||
/// can address any registered project per-call without a prior
|
||||
@@ -1310,6 +1372,7 @@ async fn handle_pipeline_get(state: &GatewayState, id: Option<Value>) -> JsonRpc
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::service::gateway::config::{GatewayConfig, ProjectEntry};
|
||||
use poem::EndpointExt as _;
|
||||
use std::collections::BTreeMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
@@ -1688,4 +1751,232 @@ mod tests {
|
||||
inject_project_arg_schema(&mut tools);
|
||||
assert_eq!(tools[0], json!({"name": "no_schema"}));
|
||||
}
|
||||
|
||||
// ── project arg is honored end-to-end, not silently misrouted (story 1225) ─
|
||||
|
||||
/// Register a fake sled connection for `name` whose background task
|
||||
/// answers every `mcp_request` with a canned response naming itself, so
|
||||
/// tests can assert *which* project actually handled a routed call.
|
||||
async fn spawn_fake_sled(name: &str) -> gateway::SledConnection {
|
||||
let (tx, mut rx) =
|
||||
tokio::sync::mpsc::unbounded_channel::<crate::sled_uplink::UplinkEnvelope>();
|
||||
let in_flight: Arc<
|
||||
tokio::sync::Mutex<
|
||||
std::collections::HashMap<String, tokio::sync::oneshot::Sender<Value>>,
|
||||
>,
|
||||
> = Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new()));
|
||||
let in_flight_task = Arc::clone(&in_flight);
|
||||
let label = name.to_string();
|
||||
tokio::spawn(async move {
|
||||
while let Some(env) = rx.recv().await {
|
||||
if let Some(sender) = in_flight_task.lock().await.remove(&env.req_id) {
|
||||
let _ = sender.send(json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"result": { "content": [{ "type": "text", "text": format!("handled_by:{label}") }] }
|
||||
}));
|
||||
}
|
||||
}
|
||||
});
|
||||
gateway::SledConnection {
|
||||
tx,
|
||||
last_heartbeat_ms: Arc::new(std::sync::atomic::AtomicI64::new(
|
||||
chrono::Utc::now().timestamp_millis(),
|
||||
)),
|
||||
in_flight,
|
||||
}
|
||||
}
|
||||
|
||||
async fn make_two_project_state(dir: &std::path::Path) -> Arc<GatewayState> {
|
||||
let mut projects = BTreeMap::new();
|
||||
projects.insert("alpha".to_string(), ProjectEntry::with_url("http://a:3001"));
|
||||
projects.insert("beta".to_string(), ProjectEntry::with_url("http://b:3002"));
|
||||
let config = GatewayConfig {
|
||||
projects,
|
||||
sled_tokens: BTreeMap::new(),
|
||||
release_channels: BTreeMap::new(),
|
||||
};
|
||||
let state = Arc::new(GatewayState::new(config, dir.to_path_buf(), 3000).unwrap());
|
||||
assert_eq!(*state.active_project.read().await, "alpha");
|
||||
state
|
||||
.register_sled_connection("alpha".to_string(), spawn_fake_sled("alpha").await)
|
||||
.await;
|
||||
state
|
||||
.register_sled_connection("beta".to_string(), spawn_fake_sled("beta").await)
|
||||
.await;
|
||||
state
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn create_tool_with_explicit_project_lands_in_non_active_project() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let state = make_two_project_state(dir.path()).await;
|
||||
let app = poem::Route::new()
|
||||
.at("/mcp", poem::post(gateway_mcp_post_handler))
|
||||
.data(state);
|
||||
let cli = poem::test::TestClient::new(app);
|
||||
|
||||
let rpc_body = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "tools/call",
|
||||
"params": {
|
||||
"name": "create_bug",
|
||||
"arguments": { "project": "beta", "title": "x", "description": "y" }
|
||||
}
|
||||
}))
|
||||
.unwrap();
|
||||
let resp = cli
|
||||
.post("/mcp")
|
||||
.header("content-type", "application/json")
|
||||
.body(rpc_body)
|
||||
.send()
|
||||
.await;
|
||||
let parsed: Value = resp.0.into_body().into_json().await.unwrap();
|
||||
assert_eq!(
|
||||
parsed["result"]["content"][0]["text"], "handled_by:beta",
|
||||
"explicit project=beta must land in beta even though alpha is active: {parsed}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_tool_with_explicit_project_reads_from_non_active_project() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let state = make_two_project_state(dir.path()).await;
|
||||
let app = poem::Route::new()
|
||||
.at("/mcp", poem::post(gateway_mcp_post_handler))
|
||||
.data(state);
|
||||
let cli = poem::test::TestClient::new(app);
|
||||
|
||||
let rpc_body = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "tools/call",
|
||||
"params": {
|
||||
"name": "get_pipeline_status",
|
||||
"arguments": { "project": "beta" }
|
||||
}
|
||||
}))
|
||||
.unwrap();
|
||||
let resp = cli
|
||||
.post("/mcp")
|
||||
.header("content-type", "application/json")
|
||||
.body(rpc_body)
|
||||
.send()
|
||||
.await;
|
||||
let parsed: Value = resp.0.into_body().into_json().await.unwrap();
|
||||
assert_eq!(
|
||||
parsed["result"]["content"][0]["text"], "handled_by:beta",
|
||||
"explicit project=beta must read from beta even though alpha is active: {parsed}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn create_tool_without_project_errors_when_multiple_projects_registered() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let state = make_two_project_state(dir.path()).await;
|
||||
let app = poem::Route::new()
|
||||
.at("/mcp", poem::post(gateway_mcp_post_handler))
|
||||
.data(state);
|
||||
let cli = poem::test::TestClient::new(app);
|
||||
|
||||
let rpc_body = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "tools/call",
|
||||
"params": {
|
||||
"name": "create_bug",
|
||||
"arguments": { "title": "x", "description": "y" }
|
||||
}
|
||||
}))
|
||||
.unwrap();
|
||||
let resp = cli
|
||||
.post("/mcp")
|
||||
.header("content-type", "application/json")
|
||||
.body(rpc_body)
|
||||
.send()
|
||||
.await;
|
||||
let parsed: Value = resp.0.into_body().into_json().await.unwrap();
|
||||
assert!(
|
||||
parsed["error"]["message"]
|
||||
.as_str()
|
||||
.unwrap()
|
||||
.contains("requires an explicit `project`"),
|
||||
"create call omitting `project` with >1 project registered must error, not \
|
||||
silently file into the active project: {parsed}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_tool_without_project_annotates_resolved_project() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let state = make_two_project_state(dir.path()).await;
|
||||
let app = poem::Route::new()
|
||||
.at("/mcp", poem::post(gateway_mcp_post_handler))
|
||||
.data(state);
|
||||
let cli = poem::test::TestClient::new(app);
|
||||
|
||||
let rpc_body = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"method": "tools/call",
|
||||
"params": {
|
||||
"name": "get_pipeline_status",
|
||||
"arguments": {}
|
||||
}
|
||||
}))
|
||||
.unwrap();
|
||||
let resp = cli
|
||||
.post("/mcp")
|
||||
.header("content-type", "application/json")
|
||||
.body(rpc_body)
|
||||
.send()
|
||||
.await;
|
||||
let parsed: Value = resp.0.into_body().into_json().await.unwrap();
|
||||
assert_eq!(
|
||||
parsed["result"]["content"][0]["text"], "handled_by:alpha",
|
||||
"sanity: implicit routing hit the active project: {parsed}"
|
||||
);
|
||||
assert_eq!(
|
||||
parsed["result"]["resolved_project"], "alpha",
|
||||
"omitting `project` must echo which project was actually resolved: {parsed}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn annotate_resolved_project_inserts_field_into_result_object() {
|
||||
let bytes = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"result": { "content": [{ "type": "text", "text": "ok" }] }
|
||||
}))
|
||||
.unwrap();
|
||||
let annotated = annotate_resolved_project(&bytes, "alpha");
|
||||
let parsed: Value = serde_json::from_slice(&annotated).unwrap();
|
||||
assert_eq!(parsed["result"]["resolved_project"], "alpha");
|
||||
assert_eq!(parsed["result"]["content"][0]["text"], "ok");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn annotate_resolved_project_leaves_error_response_untouched() {
|
||||
let bytes = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0",
|
||||
"id": 1,
|
||||
"error": { "code": -32603, "message": "boom" }
|
||||
}))
|
||||
.unwrap();
|
||||
let annotated = annotate_resolved_project(&bytes, "alpha");
|
||||
let parsed: Value = serde_json::from_slice(&annotated).unwrap();
|
||||
assert_eq!(parsed["error"]["message"], "boom");
|
||||
assert!(parsed.get("result").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn is_create_tool_matches_create_prefixed_names_only() {
|
||||
assert!(is_create_tool("create_bug"));
|
||||
assert!(is_create_tool("create_story"));
|
||||
assert!(is_create_tool("create_worktree"));
|
||||
assert!(!is_create_tool("get_pipeline_status"));
|
||||
assert!(!is_create_tool("show"));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user