mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-06 08:18:58 +00:00
fix(mcp): tighten run tool contract
This commit is contained in:
Bryan Helmkamp
parent
9fc0ec3061
commit
75af34140c
2 changed files with 262 additions and 6 deletions
|
|
@ -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!();
|
||||
|
|
|
|||
|
|
@ -211,9 +211,10 @@ impl TryFrom<FabroRunInteractParams> 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<types::ManifestArgs> {
|
||||
let label = spec
|
||||
let mut input = spec
|
||||
.inputs
|
||||
.iter()
|
||||
.map(|(key, value)| format!("{key}={value}"))
|
||||
.collect::<Vec<_>>();
|
||||
input.sort();
|
||||
let mut label = spec
|
||||
.labels
|
||||
.iter()
|
||||
.map(|(key, value)| format!("{key}={value}"))
|
||||
.collect::<Vec<_>>();
|
||||
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""#]);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue