diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index b7b487e7d..6aac52f62 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -153,7 +153,7 @@ You can also reuse an exact immutable workflow version without uploading content } ``` -Workflow source and run target are independent. Object-form calls can provide an explicit Git, `none`, or folder target. A workflow agent inherits its parent run's canonical target when `target` is omitted. Standalone MCP derives an attached GitHub checkout only when it can prove that the exact local HEAD is available from the canonical origin; otherwise, push the commit or provide an explicit target. Docker and Daytona agents must send inline files or a stored version ID—never send a sandbox path as a selector, because the run worker cannot use that path to read the sandbox's files. +Workflow source and run target are independent. Object-form calls can provide an explicit Git, `none`, or folder target. A workflow agent inherits its parent run's canonical target when `target` is omitted. Folder targets are available only to standalone MCP and Local workflow agents with a shared host filesystem; Docker and Daytona parents cannot select a server-host folder, even by naming a Local child environment. Standalone MCP derives an attached GitHub checkout only when it can prove that the exact local HEAD is available from the canonical origin; otherwise, push the commit or provide an explicit target. Docker and Daytona agents must send inline files or a stored version ID—never send a sandbox path as a selector, because the run worker cannot use that path to read the sandbox's files. 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. Like selectors, `goal_file` requires a native/shared filesystem; Docker and Daytona agents must send `goal` text by value. diff --git a/docs/public/execution/child-runs.mdx b/docs/public/execution/child-runs.mdx index a01110e31..09bcae97a 100644 --- a/docs/public/execution/child-runs.mdx +++ b/docs/public/execution/child-runs.mdx @@ -134,7 +134,7 @@ Reuse content already registered with Fabro by supplying its exact immutable ID: } ``` -Workflow content and workspace target are separate choices. If `target` is omitted, a child inherits the parent's full canonical Git, `none`, or folder target. An explicit Git, `none`, or folder target overrides that inheritance while the current run remains the forced parent. +Workflow content and workspace target are separate choices. If `target` is omitted, a child inherits the parent's full canonical Git, `none`, or folder target. An explicit Git, `none`, or folder target overrides that inheritance while the current run remains the forced parent. Folder targets require a Local parent with a shared host filesystem; Docker and Daytona parents cannot select a server-host folder, even by naming a Local child environment. `goal_file` is also a shared-filesystem feature. Local agents can read it relative to the operation `cwd`; Docker and Daytona agents must send the resolved `goal` text by value. diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 76755495d..243adf0d9 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -6,7 +6,7 @@ use std::time::Duration; use anyhow::{Context, Result, anyhow}; use async_trait::async_trait; use fabro_client::ServerTarget; -use fabro_config::user::active_settings_path; +use fabro_config::user::default_workflows_dir; use fabro_config::{ServerSettingsBuilder, Storage}; use fabro_interview::{ AnswerSubmission, ControlInterviewer, WORKER_CONTROL_INVALID_CURSOR_REASON, @@ -241,12 +241,8 @@ fn build_fabro_run_tool_services( if worker_token.trim().is_empty() { return None; } - let settings_path = active_settings_path(None); - let user_workflows_root = settings_path - .parent() - .map(|parent| parent.join("workflows")); let backend = ClientBackend::new(Arc::new(client)).with_run_create_adapter(Arc::new( - ServerRunCreateAdapter::worker(provider, inherited_target, user_workflows_root), + ServerRunCreateAdapter::worker(provider, inherited_target, Some(default_workflows_dir())), )).with_workflow_version_packager(Arc::new(SuppliedWorkflowVersionPackager)); Some(FabroRunToolServices { backend: Arc::new(backend), diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index 11dc6d8ab..9cf72a437 100644 --- a/lib/apps/fabro-mcp-server/src/server.rs +++ b/lib/apps/fabro-mcp-server/src/server.rs @@ -4,6 +4,7 @@ use std::time::Duration; use anyhow::Result; use fabro_manifest::SuppliedWorkflowVersionPackager; +use fabro_config::user; use fabro_server::run_tool_create::ServerRunCreateAdapter; use fabro_tool::fabro_client::ClientBackend; use fabro_tool::{self as run_tools, FabroToolBackend}; @@ -273,14 +274,11 @@ impl FabroMcpServer { (self.settings.client_factory)() .await .map(|client| { - let user_workflows_root = self - .settings - .config_path - .parent() - .map(|parent| parent.join("workflows")); Arc::new( ClientBackend::new(Arc::new(client)).with_run_create_adapter(Arc::new( - ServerRunCreateAdapter::standalone(user_workflows_root), + ServerRunCreateAdapter::standalone(Some( + user::default_workflows_dir(), + )).with_workflow_version_packager(Arc::new(SuppliedWorkflowVersionPackager)), )).with_workflow_version_packager(Arc::new(SuppliedWorkflowVersionPackager)), ) as Arc }) diff --git a/lib/apps/fabro-server/src/run_intent.rs b/lib/apps/fabro-server/src/run_intent.rs index 68aa7c7a8..0a04fc7cf 100644 --- a/lib/apps/fabro-server/src/run_intent.rs +++ b/lib/apps/fabro-server/src/run_intent.rs @@ -7,8 +7,8 @@ use fabro_environment::{EnvironmentId, EnvironmentValidationError}; use fabro_manifest::CollectedWorkflowClosure; use fabro_types::settings::InterpString; use fabro_types::{ - GitContext, ManifestPath, RunTarget, SandboxProviderKind, TargetValidationError, WorkflowPath, - WorkflowVersion, WorkflowVersionId, + GitContext, ManifestPath, RunId, RunTarget, SandboxProviderKind, TargetValidationError, + WorkflowPath, WorkflowVersion, WorkflowVersionId, }; use fabro_workflow::git; use fabro_workflow::workflow_bundle::{BundledWorkflow, ParsedWorkflowConfig, WorkflowBundle}; @@ -40,6 +40,12 @@ pub(crate) enum RunIntentAdmissionError { #[source] source: fabro_variable::Error, }, + #[error("originating worker run `{run_id}` could not be loaded")] + WorkerRun { + run_id: RunId, + #[source] + source: fabro_store::Error, + }, } #[derive(Debug, Error)] diff --git a/lib/apps/fabro-server/src/run_tool_create.rs b/lib/apps/fabro-server/src/run_tool_create.rs index 048f4d940..664bf5de3 100644 --- a/lib/apps/fabro-server/src/run_tool_create.rs +++ b/lib/apps/fabro-server/src/run_tool_create.rs @@ -104,6 +104,11 @@ impl ServerRunCreateAdapter { cwd: &Path, ) -> Result { if let Some(target) = &spec.target { + if matches!(target, RunTarget::Folder { .. }) && !self.has_shared_filesystem() { + bail!( + "folder targets require a shared Local filesystem; Docker and Daytona parents cannot select server-host folders" + ); + } return Ok(ResolvedTarget { target: target.clone(), warnings: Vec::new(), @@ -552,6 +557,58 @@ mod tests { assert!(goal_error.to_string().contains("send goal text by value")); } + #[tokio::test] + async fn workflow_version_clone_based_workers_reject_explicit_folder_targets() { + let client = no_proxy_client("http://127.0.0.1:9"); + let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); + let spec = validated_spec(&json!({ + "workflow": { + "kind": "stored", + "workflow_version_id": workflow_version_id + }, + "target": { "kind": "folder", "path": "/srv/server-workspace" } + })); + + for provider in [EnvironmentProvider::Docker, EnvironmentProvider::Daytona] { + let adapter = ServerRunCreateAdapter::worker(provider, None, None); + let error = adapter + .prepare(&client, &spec, Path::new("/ignored")) + .await + .expect_err("clone-based workers must not select server-host folders"); + + assert!( + error + .to_string() + .contains("cannot select server-host folders"), + "unexpected error for {provider:?}: {error:#}" + ); + } + } + + #[tokio::test] + async fn workflow_version_local_worker_accepts_explicit_folder_target() { + let client = no_proxy_client("http://127.0.0.1:9"); + let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); + let target = RunTarget::Folder { + path: "/srv/server-workspace".to_string(), + }; + let spec = validated_spec(&json!({ + "workflow": { + "kind": "stored", + "workflow_version_id": workflow_version_id + }, + "target": target + })); + let adapter = ServerRunCreateAdapter::worker(EnvironmentProvider::Local, None, None); + + let prepared = adapter + .prepare(&client, &spec, Path::new("/ignored")) + .await + .unwrap(); + + assert_eq!(prepared.target, target); + } + #[tokio::test] async fn workflow_version_is_registered_before_server_admission_rejection() { let server = MockServer::start_async().await; diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index 1066b0cce..07170f769 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -631,6 +631,9 @@ pub(crate) async fn create_run_from_intent( Ok(validated) => validated, Err(error) => return run_intent_admission_error(error.into()), }; + if let Err(error) = validate_intent_actor_target(&state, &actor, &target).await { + return run_intent_admission_error(error); + } let environment_id = match select_intent_environment_id( &state, intent @@ -985,6 +988,7 @@ fn run_intent_admission_error(error: RunIntentAdmissionError) -> Response { match &error { RunIntentAdmissionError::VersionStore { .. } | RunIntentAdmissionError::VariableSnapshot { .. } + | RunIntentAdmissionError::WorkerRun { .. } | RunIntentAdmissionError::Environment(EnvironmentSelectionError::CredentialStore { .. }) => { @@ -1076,9 +1080,48 @@ fn run_intent_admission_error(error: RunIntentAdmissionError) -> Response { "failed to load run variables", "variable_store_error", ), + RunIntentAdmissionError::WorkerRun { .. } => intent_error( + StatusCode::INTERNAL_SERVER_ERROR, + "failed to inspect originating worker run", + "worker_run_store_error", + ), } } +async fn validate_intent_actor_target( + state: &AppState, + actor: &Principal, + target: &RunTarget, +) -> Result<(), RunIntentAdmissionError> { + let (Principal::Worker { run_id }, RunTarget::Folder { .. }) = (actor, target) else { + return Ok(()); + }; + let run_store = state + .stores + .runs + .open_run_reader(run_id) + .await + .map_err(|source| RunIntentAdmissionError::WorkerRun { + run_id: *run_id, + source, + })?; + let projection = + run_store + .state() + .await + .map_err(|source| RunIntentAdmissionError::WorkerRun { + run_id: *run_id, + source, + })?; + if !projection.spec.settings.run.environment.provider.is_local() { + return Err(EnvironmentSelectionError::TargetUnsupported { + detail: "folder targets created by a worker require a Local parent environment", + } + .into()); + } + Ok(()) +} + fn select_intent_environment_id( state: &AppState, value: &str, diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index a0a4533f6..d34398253 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -4494,6 +4494,89 @@ enabled = false ); } +#[tokio::test] +async fn run_tools_worker_cannot_select_server_folder_from_clone_based_parent() { + let dir = tempfile::tempdir().unwrap(); + let missing_target = dir.path().join("missing"); + let (state, app) = jwt_auth_app(); + let user_token = issue_test_user_jwt(); + let parent_run_id = create_run_with_bearer(&app, &user_token).await; + let worker_token = issue_test_run_tools_worker_token(&parent_run_id); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; + let mut intent = folder_intent(workflow_version_id, missing_target.to_string_lossy()); + intent["environment_id"] = json!("local"); + intent["parent_id"] = json!(parent_run_id); + + let response = app + .oneshot(json_bearer_request( + Method::POST, + "/runs", + &worker_token, + &intent, + )) + .await + .unwrap(); + let body = response_json!(response, StatusCode::UNPROCESSABLE_ENTITY).await; + + assert_eq!(body["errors"][0]["code"], "target_environment_unsupported"); + assert_eq!( + body["errors"][0]["detail"], + "folder targets created by a worker require a Local parent environment" + ); + assert_eq!( + state + .stores + .run_summaries + .list_identities() + .await + .unwrap() + .len(), + 1, + "the rejected child must not be persisted" + ); +} + +#[tokio::test] +async fn run_tools_worker_can_select_server_folder_from_local_parent() { + let dir = tempfile::tempdir().unwrap(); + let (state, app) = jwt_auth_app(); + let user_token = issue_test_user_jwt(); + let workflow_version_id = store_workflow_version(&state, MINIMAL_DOT, None).await; + let mut parent_intent = folder_intent(workflow_version_id, dir.path().to_string_lossy()); + parent_intent["environment_id"] = json!("local"); + + let response = app + .clone() + .oneshot(json_bearer_request( + Method::POST, + "/runs", + &user_token, + &parent_intent, + )) + .await + .unwrap(); + let parent = response_json!(response, StatusCode::CREATED).await; + let parent_run_id = parent["id"].as_str().unwrap().parse::().unwrap(); + let worker_token = issue_test_run_tools_worker_token(&parent_run_id); + let mut child_intent = folder_intent(workflow_version_id, dir.path().to_string_lossy()); + child_intent["environment_id"] = json!("local"); + child_intent["parent_id"] = json!(parent_run_id); + + let response = app + .oneshot(json_bearer_request( + Method::POST, + "/runs", + &worker_token, + &child_intent, + )) + .await + .unwrap(); + let child = response_json!(response, StatusCode::CREATED).await; + + assert_eq!(child["parent_id"], parent_run_id.to_string()); + assert_eq!(child["lifecycle"]["status"]["kind"], "submitted"); +} + #[tokio::test] async fn post_runs_run_intent_accepts_none_target_with_ready_daytona_environment() { let state = TestAppStateBuilder::new() diff --git a/lib/components/fabro-tool/src/create.rs b/lib/components/fabro-tool/src/create.rs index 85cc97ae6..889e34aee 100644 --- a/lib/components/fabro-tool/src/create.rs +++ b/lib/components/fabro-tool/src/create.rs @@ -128,7 +128,7 @@ impl JsonSchema for CreateRunSpecInput { "description": "Optional parent run id or selector." }, "target": { - "description": "Canonical run workspace target. Worker calls inherit the parent target when omitted; standalone calls require an attached GitHub checkout whose exact local HEAD is available from the canonical origin.", + "description": "Canonical run workspace target. Worker calls inherit the parent target when omitted; standalone calls require an attached GitHub checkout whose exact local HEAD is available from the canonical origin. Folder targets require a standalone or Local-worker filesystem context.", "anyOf": [ { "type": "null" }, { diff --git a/lib/foundation/fabro-config/src/user.rs b/lib/foundation/fabro-config/src/user.rs index 303f8cc2a..00c403699 100644 --- a/lib/foundation/fabro-config/src/user.rs +++ b/lib/foundation/fabro-config/src/user.rs @@ -23,6 +23,10 @@ pub fn default_storage_dir() -> PathBuf { Home::from_env().root().join("storage") } +pub fn default_workflows_dir() -> PathBuf { + Home::from_env().workflows_dir() +} + pub fn default_socket_path() -> PathBuf { Home::from_env().root().join("fabro.sock") } @@ -77,8 +81,8 @@ mod tests { use temp_env::with_var; use super::{ - SETTINGS_CONFIG_FILENAME, active_settings_path_with_lookup, default_settings_path, - default_socket_path, default_storage_dir, + SETTINGS_CONFIG_FILENAME, active_settings_path, active_settings_path_with_lookup, + default_settings_path, default_socket_path, default_storage_dir, default_workflows_dir, }; #[test] @@ -91,10 +95,29 @@ mod tests { home.join(".fabro").join(SETTINGS_CONFIG_FILENAME) ); assert_eq!(default_storage_dir(), home.join(".fabro/storage")); + assert_eq!(default_workflows_dir(), home.join(".fabro/workflows")); assert_eq!(default_socket_path(), home.join(".fabro/fabro.sock")); }); } + #[test] + fn workflows_path_uses_fabro_home_when_config_is_elsewhere() { + let dir = tempfile::tempdir().unwrap(); + let fabro_home = dir.path().join("fabro-home"); + let custom_config = dir.path().join("config/settings.toml"); + + with_var(EnvVars::FABRO_HOME, Some(fabro_home.as_os_str()), || { + with_var( + EnvVars::FABRO_CONFIG, + Some(custom_config.as_os_str()), + || { + assert_eq!(active_settings_path(None), custom_config); + assert_eq!(default_workflows_dir(), fabro_home.join("workflows")); + }, + ); + }); + } + #[test] fn active_settings_path_honors_fabro_config_env() { let dir = tempfile::tempdir().unwrap();