diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index e44976a1b..39b91c5ab 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -49,6 +49,7 @@ Use the object form when you need create options: "workflow": "sleeper", "auto_approve": true, "dry_run": true, + "goal_file": "plans/ship-it.md", "labels": { "source": "mcp" }, "start": true } @@ -56,6 +57,8 @@ Use the object form when you need create options: } ``` +Use `goal` for inline goal text or `goal_file` to read the run goal from a file. They are mutually exclusive. Relative `goal_file` paths resolve from the run's `cwd`, or from the MCP server working directory when `cwd` is omitted. + Run summaries returned by the MCP server include parent metadata. Use `parent_id` on `fabro_run_create` to create a child run, `parent_id` on `fabro_run_search` to list direct children, and the `link_parent` or `unlink_parent` actions on `fabro_run_interact` to change an existing run's parent. ## Fabro agents as MCP clients diff --git a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs index b48ac90fd..536bca4f8 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mcp.rs @@ -1489,10 +1489,26 @@ async fn mcp_create_validation_errors_happen_before_auth_or_network() { }), ) .await; + let conflicting_goal_sources = call_tool_error_text( + &client, + "fabro_run_create", + serde_json::json!({ + "runs": [{ + "workflow": "simple.fabro", + "goal": "inline goal", + "goal_file": "plans/goal.md" + }] + }), + ) + .await; assert!(empty.contains("runs"), "{empty}"); assert!(many.contains("runs"), "{many}"); assert!(null.contains("decision"), "{null}"); + assert!( + conflicting_goal_sources.contains("goal and goal_file are mutually exclusive"), + "{conflicting_goal_sources}" + ); assert_eq!(client.list_tools().await.unwrap().len(), 6); client .shutdown() @@ -2267,6 +2283,10 @@ fn assert_create_schema_accepts_string_and_object_specs(schema: &serde_json::Val object_variant.pointer("/properties/workflow").is_some(), "object create spec should include workflow property: {schema}" ); + assert!( + object_variant.pointer("/properties/goal_file").is_some(), + "object create spec should include goal_file property: {schema}" + ); assert!( object_variant .get("required") diff --git a/lib/crates/fabro-mcp-server/src/server.rs b/lib/crates/fabro-mcp-server/src/server.rs index 1735ed2a4..9f3a3f3b6 100644 --- a/lib/crates/fabro-mcp-server/src/server.rs +++ b/lib/crates/fabro-mcp-server/src/server.rs @@ -293,6 +293,10 @@ mod tests { object_variant.pointer("/properties/workflow").is_some(), "object create spec should expose workflow property: {schema}" ); + assert!( + object_variant.pointer("/properties/goal_file").is_some(), + "object create spec should expose goal_file property: {schema}" + ); assert!( object_variant .get("required") diff --git a/lib/crates/fabro-server/src/run_tool_manifest.rs b/lib/crates/fabro-server/src/run_tool_manifest.rs index c1239c3bf..ad7b9804d 100644 --- a/lib/crates/fabro-server/src/run_tool_manifest.rs +++ b/lib/crates/fabro-server/src/run_tool_manifest.rs @@ -2,10 +2,11 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use fabro_api::types; -use fabro_config::{CliLayer, RunLayer}; +use fabro_config::{CliLayer, RunGoalLayer, RunLayer}; use fabro_manifest::{ManifestBuildInput, RunOverrideInput}; use fabro_model::Catalog; use fabro_tool::{ToolError, ToolResult, ValidatedCreateRunSpec}; +use fabro_types::settings::interp::InterpString; use crate::manifest_validation; @@ -68,7 +69,7 @@ pub fn run_tool_manifest_args(spec: &ValidatedCreateRunSpec) -> Option Option { - fabro_manifest::build_sparse_run_overrides(RunOverrideInput { + let mut run = fabro_manifest::build_run_overrides(RunOverrideInput { goal: spec.goal.as_deref(), model: spec.model.as_deref(), provider: spec.provider.as_deref(), @@ -78,7 +79,18 @@ pub fn run_tool_run_overrides(spec: &ValidatedCreateRunSpec) -> Option dry_run: spec.dry_run, auto_approve: spec.auto_approve, labels: spec.labels.clone(), - }) + }); + if let Some(goal_file) = spec.goal_file.as_ref() { + run.goal = Some(RunGoalLayer::File { + file: InterpString::parse(&goal_file.to_string_lossy()), + }); + } + (run.goal.is_some() + || !run.metadata.is_empty() + || run.model.is_some() + || run.sandbox.is_some() + || run.execution.is_some()) + .then_some(run) } #[cfg(test)] @@ -98,6 +110,7 @@ mod tests { parent_id: None, cwd: None, goal: None, + goal_file: None, inputs: HashMap::from([ ("count".to_string(), json!(3).into()), ("decision".to_string(), json!("approve").into()), @@ -116,4 +129,32 @@ mod tests { assert_eq!(args.input, vec![r"count=3", r#"decision="approve""#]); } + + #[test] + fn run_overrides_preserve_goal_file_as_file_goal() { + let spec = ValidatedCreateRunSpec::try_from(CreateRunSpec { + workflow: "implement-plan".to_string(), + run_id: None, + parent_id: None, + cwd: None, + goal: None, + goal_file: Some(PathBuf::from("plans/ship-it.md")), + inputs: HashMap::new(), + labels: HashMap::new(), + model: None, + provider: None, + sandbox: None, + dry_run: None, + auto_approve: None, + preserve_sandbox: None, + start: None, + }) + .expect("create spec with goal_file should validate"); + + let run = run_tool_run_overrides(&spec).expect("goal_file should produce run overrides"); + let Some(fabro_config::RunGoalLayer::File { file }) = run.goal else { + panic!("goal_file should become a file goal override"); + }; + assert_eq!(file.as_source(), "plans/ship-it.md"); + } } diff --git a/lib/crates/fabro-tool/src/create.rs b/lib/crates/fabro-tool/src/create.rs index 7c50fd8f7..0d283fc06 100644 --- a/lib/crates/fabro-tool/src/create.rs +++ b/lib/crates/fabro-tool/src/create.rs @@ -113,6 +113,13 @@ impl JsonSchema for CreateRunSpecInput { ], "description": "Optional goal override for the run." }, + "goal_file": { + "anyOf": [ + { "type": "string" }, + { "type": "null" } + ], + "description": "Read the run goal from a file. Mutually exclusive with goal. Relative paths are resolved from the run cwd." + }, "inputs": { "type": "object", "description": "Workflow input overrides keyed by input name.", @@ -194,6 +201,7 @@ pub struct CreateRunSpec { pub run_id: Option, pub parent_id: Option, pub goal: Option, + pub goal_file: Option, #[serde(default)] pub inputs: HashMap, #[serde(default)] @@ -257,6 +265,7 @@ pub struct ValidatedCreateRunSpec { pub run_id: Option, pub parent_id: Option, pub goal: Option, + pub goal_file: Option, pub inputs: HashMap, pub labels: HashMap, pub dry_run: Option, @@ -298,6 +307,7 @@ impl TryFrom for ValidatedCreateRunSpec { run_id: None, parent_id: None, goal: None, + goal_file: None, inputs: HashMap::new(), labels: HashMap::new(), dry_run: None, @@ -335,6 +345,18 @@ impl TryFrom for ValidatedCreateRunSpec { if spec.parent_id.is_some() && parent_id.is_none() { return Err(ToolError::message("parent_id must not be blank")); } + if spec.goal.is_some() && spec.goal_file.is_some() { + return Err(ToolError::message( + "goal and goal_file are mutually exclusive; use exactly one", + )); + } + if spec + .goal_file + .as_ref() + .is_some_and(|path| path.as_os_str().is_empty()) + { + return Err(ToolError::message("goal_file must not be blank")); + } let inputs = spec .inputs .into_iter() @@ -349,6 +371,7 @@ impl TryFrom for ValidatedCreateRunSpec { run_id, parent_id, goal: spec.goal, + goal_file: spec.goal_file, inputs, labels: spec.labels, dry_run: spec.dry_run, @@ -517,6 +540,7 @@ mod tests { run_id: None, parent_id: Some(" nightly-parent ".to_string()), goal: None, + goal_file: None, inputs: HashMap::new(), labels: HashMap::new(), dry_run: None, @@ -577,6 +601,46 @@ mod tests { assert_eq!(spec.start, Some(false)); } + #[test] + fn create_params_preserve_goal_file_option() { + let params: FabroRunCreateParams = serde_json::from_value(json!({ + "runs": [{ + "workflow": "implement-plan", + "goal_file": "plans/ship-it.md", + "start": false + }] + })) + .expect("object form with goal_file should deserialize"); + + let params = ValidatedCreateRuns::try_from(params).expect("goal_file should validate"); + let spec = ¶ms.runs[0]; + assert_eq!(spec.goal, None); + assert_eq!( + spec.goal_file.as_deref(), + Some(Path::new("plans/ship-it.md")) + ); + } + + #[test] + fn create_params_reject_goal_and_goal_file_together() { + let params: FabroRunCreateParams = serde_json::from_value(json!({ + "runs": [{ + "workflow": "implement-plan", + "goal": "inline goal", + "goal_file": "plans/ship-it.md" + }] + })) + .expect("object form with both goal forms should deserialize before validation"); + + let err = ValidatedCreateRuns::try_from(params) + .expect_err("goal and goal_file should be mutually exclusive"); + assert!( + err.to_string() + .contains("goal and goal_file are mutually exclusive"), + "{err}" + ); + } + #[test] fn create_params_reject_blank_string_shorthand_workflow() { let params: FabroRunCreateParams = serde_json::from_value(json!({ @@ -621,6 +685,7 @@ mod tests { run_id: None, parent_id: Some("nightly-parent".to_string()), goal: None, + goal_file: None, inputs: HashMap::new(), labels: HashMap::new(), dry_run: Some(true), @@ -670,6 +735,7 @@ mod tests { run_id: None, parent_id: Some("nightly-parent".to_string()), goal: None, + goal_file: None, inputs: HashMap::new(), labels: HashMap::new(), dry_run: Some(true), @@ -718,6 +784,7 @@ mod tests { run_id: None, parent_id: Some(parent_id.to_string()), goal: None, + goal_file: None, inputs: HashMap::new(), labels: HashMap::new(), dry_run: Some(true),