From 75af34140c0872dc42bafa08437e9fa79b41d98b Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 11 May 2026 09:36:08 -0400 Subject: [PATCH] fix(mcp): tighten run tool contract --- lib/crates/fabro-cli/tests/it/cmd/mcp.rs | 212 ++++++++++++++++++- lib/crates/fabro-mcp-server/src/run_tools.rs | 56 ++++- 2 files changed, 262 insertions(+), 6 deletions(-) diff --git a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs index 0693ba02d..23955a693 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs @@ -18,8 +18,10 @@ use fabro_test::{fabro_json_snapshot, fabro_snapshot, test_context}; use httpmock::Method::{GET, POST}; use httpmock::MockServer; -use super::support::mock_resolved_run; -use crate::support::{RealAuthHarness, TEST_DEV_TOKEN, seed_dev_token_auth, unique_run_id}; +use super::support::{mock_resolved_run, remote_run_summary_json}; +use crate::support::{ + RealAuthHarness, TEST_DEV_TOKEN, run_projection_json, seed_dev_token_auth, unique_run_id, +}; #[test] fn help() { @@ -593,6 +595,70 @@ async fn mcp_search_filters_status_dates_and_paginates() { harness.shutdown().await; } +#[tokio::test(flavor = "multi_thread")] +async fn mcp_search_includes_archived_runs_by_default() { + let context = test_context!(); + let server = MockServer::start(); + let target_url = format!("{}/api/v1", server.base_url()); + let target: fabro_client::ServerTarget = target_url.parse().unwrap(); + seed_dev_token_auth(&context.home_dir, &target, TEST_DEV_TOKEN); + let active_id = unique_run_id(); + let archived_id = unique_run_id(); + let active = remote_run_summary_json( + &active_id, + "Simple", + "simple", + "Active run", + &serde_json::json!({ "kind": "succeeded", "reason": "completed" }), + "2026-04-05T12:00:00Z", + ); + let mut archived = remote_run_summary_json( + &archived_id, + "Simple", + "simple", + "Archived run", + &serde_json::json!({ "kind": "succeeded", "reason": "completed" }), + "2026-04-05T12:01:00Z", + ); + archived["lifecycle"]["archived"] = serde_json::json!(true); + archived["lifecycle"]["archived_at"] = serde_json::json!("2026-04-05T12:02:00Z"); + let list_runs = server.mock(|when, then| { + when.method(GET) + .path("/api/v1/runs") + .query_param("include_archived", "true") + .query_param("page[limit]", "100") + .query_param("page[offset]", "0"); + then.status(200) + .header("Content-Type", "application/json") + .json_body(serde_json::json!({ + "data": [active, archived], + "meta": { "has_more": false } + })); + }); + + let client = spawn_mcp_client(&context, &["--server", &target_url]).await; + let result = call_tool_json( + &client, + "fabro_run_search", + serde_json::json!({ "run_ids": [active_id, archived_id], "first": 10 }), + ) + .await; + + assert_eq!(result["runs"].as_array().unwrap().len(), 2); + assert!( + result["runs"] + .as_array() + .unwrap() + .iter() + .any(|run| run["archived"] == true) + ); + list_runs.assert(); + client + .shutdown() + .await + .expect("MCP client should shut down"); +} + #[tokio::test(flavor = "multi_thread")] async fn mcp_lifecycle_tools_manage_real_run() { let context = test_context!(); @@ -783,6 +849,123 @@ async fn mcp_interact_error_does_not_stop_server() { .expect("MCP client should shut down"); } +#[tokio::test(flavor = "multi_thread")] +async fn mcp_interact_actions_resolve_selector_and_call_expected_endpoints() { + let context = test_context!(); + let server = MockServer::start(); + let target_url = format!("{}/api/v1", server.base_url()); + let target: fabro_client::ServerTarget = target_url.parse().unwrap(); + seed_dev_token_auth(&context.home_dir, &target, TEST_DEV_TOKEN); + let run_id = unique_run_id(); + let selector = "nightly"; + let resolve = mock_resolved_run(&server, selector, &run_id); + let retrieve = server.mock(|when, then| { + when.method(GET).path(format!("/api/v1/runs/{run_id}")); + then.status(200) + .header("Content-Type", "application/json") + .json_body(remote_run_summary_json( + &run_id, + "Simple", + "simple", + "Run tests", + &serde_json::json!({ "kind": "running" }), + "2026-04-05T12:00:00Z", + )); + }); + let projection = server.mock(|when, then| { + when.method(GET) + .path(format!("/api/v1/runs/{run_id}/state")); + then.status(200) + .header("Content-Type", "application/json") + .json_body(run_projection_json( + &run_id, + &serde_json::json!({ "kind": "running" }), + )); + }); + let start = server.mock(|when, then| { + when.method(POST) + .path(format!("/api/v1/runs/{run_id}/start")) + .json_body(serde_json::json!({ "resume": false })); + then.status(200) + .header("Content-Type", "application/json") + .json_body(remote_run_summary_json( + &run_id, + "Simple", + "simple", + "Run tests", + &serde_json::json!({ "kind": "running" }), + "2026-04-05T12:00:00Z", + )); + }); + let message = server.mock(|when, then| { + when.method(POST) + .path(format!("/api/v1/runs/{run_id}/steer")) + .json_body(serde_json::json!({ "text": "continue", "interrupt": true })); + then.status(202); + }); + let cancel = server.mock(|when, then| { + when.method(POST) + .path(format!("/api/v1/runs/{run_id}/cancel")); + then.status(200) + .header("Content-Type", "application/json") + .json_body(remote_run_summary_json( + &run_id, + "Simple", + "simple", + "Run tests", + &serde_json::json!({ "kind": "running" }), + "2026-04-05T12:00:00Z", + )); + }); + + let client = spawn_mcp_client(&context, &["--server", &target_url]).await; + let get = call_tool_json( + &client, + "fabro_run_interact", + serde_json::json!({ "run_id": selector, "action": "get" }), + ) + .await; + let start_result = call_tool_json( + &client, + "fabro_run_interact", + serde_json::json!({ "run_id": selector, "action": "start" }), + ) + .await; + let message_result = call_tool_json( + &client, + "fabro_run_interact", + serde_json::json!({ + "run_id": selector, + "action": "message", + "message": "continue", + "interrupt": true + }), + ) + .await; + let cancel_result = call_tool_json( + &client, + "fabro_run_interact", + serde_json::json!({ "run_id": selector, "action": "cancel" }), + ) + .await; + + assert_eq!(get["result"]["summary"]["run_id"], run_id); + assert_eq!(start_result["result"]["summary"]["run_id"], run_id); + assert_eq!(message_result["result"]["message"], "continue"); + assert_eq!(message_result["result"]["interrupt"], true); + assert_eq!(cancel_result["result"]["summary"]["run_id"], run_id); + resolve.assert_calls(4); + retrieve.assert_calls(3); + projection.assert(); + start.assert(); + message.assert(); + cancel.assert(); + client + .shutdown() + .await + .expect("MCP client should shut down"); +} + #[tokio::test(flavor = "multi_thread")] async fn mcp_create_validation_errors_happen_before_auth_or_network() { let context = test_context!(); @@ -825,6 +1008,31 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { .expect("MCP client should shut down"); } +#[tokio::test(flavor = "multi_thread")] +async fn mcp_interact_answer_validation_happens_before_auth_or_network() { + let context = test_context!(); + let client = spawn_mcp_client(&context, &["--server", "http://127.0.0.1:9"]).await; + + let error = call_tool_error_text( + &client, + "fabro_run_interact", + serde_json::json!({ + "run_id": "nightly", + "action": "answer", + "question_id": "q-1", + "answer": { "value": "yes" } + }), + ) + .await; + + assert!(error.contains("option, options, text"), "{error}"); + assert_eq!(client.list_tools().await.unwrap().len(), 5); + client + .shutdown() + .await + .expect("MCP client should shut down"); +} + #[tokio::test(flavor = "multi_thread")] async fn mcp_interact_questions_and_answers_use_api_wire_contract() { let context = test_context!(); diff --git a/lib/crates/fabro-mcp-server/src/run_tools.rs b/lib/crates/fabro-mcp-server/src/run_tools.rs index 7902c0dac..11e6d2c58 100644 --- a/lib/crates/fabro-mcp-server/src/run_tools.rs +++ b/lib/crates/fabro-mcp-server/src/run_tools.rs @@ -211,9 +211,10 @@ impl TryFrom for ValidatedInteractRun { "question_id is required for action answer", )); } - if params.answer.is_none() { + let Some(answer) = params.answer.as_ref() else { return Err(ToolError::message("answer is required for action answer")); - } + }; + answer_to_submit_request(answer.clone())?; } Ok(Self { raw: params }) } @@ -847,16 +848,23 @@ fn build_mcp_run_manifest( } fn mcp_manifest_args(spec: &CreateRunSpec) -> Option { - let label = spec + let mut input = spec + .inputs + .iter() + .map(|(key, value)| format!("{key}={value}")) + .collect::>(); + input.sort(); + let mut label = spec .labels .iter() .map(|(key, value)| format!("{key}={value}")) .collect::>(); + label.sort(); let payload = types::ManifestArgs { auto_approve: spec.auto_approve.filter(|value| *value), docker_image: None, dry_run: spec.dry_run.filter(|value| *value), - input: Vec::new(), + input, label, model: spec.model.clone(), preserve_sandbox: spec.preserve_sandbox.filter(|value| *value), @@ -1090,4 +1098,44 @@ mod tests { assert!(err.as_str().contains("option, options, text")); } + + #[test] + fn interact_answer_validation_rejects_unsupported_json_before_api_calls() { + let err = ValidatedInteractRun::try_from(FabroRunInteractParams { + action: RunInteractAction::Answer, + run_id: "run_123".to_string(), + message: None, + interrupt: None, + question_id: Some("question-1".to_string()), + answer: Some(json!({ "value": "yes" })), + }) + .unwrap_err(); + + assert!(err.as_str().contains("option, options, text")); + } + + #[test] + fn mcp_manifest_args_preserve_input_provenance() { + let args = mcp_manifest_args(&CreateRunSpec { + workflow: "simple".to_string(), + run_id: None, + cwd: None, + goal: None, + inputs: HashMap::from([ + ("count".to_string(), json!(3)), + ("decision".to_string(), json!("approve")), + ]), + labels: HashMap::new(), + model: None, + provider: None, + sandbox: None, + dry_run: None, + auto_approve: None, + preserve_sandbox: None, + start: None, + }) + .expect("input args should be present"); + + assert_eq!(args.input, vec![r"count=3", r#"decision="approve""#]); + } }