diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index 47e592f39..b7b487e7d 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 instead derives an attached GitHub checkout when it can do so truthfully and otherwise requires 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. 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/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 3c096d67b..8f774b7e9 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -1951,14 +1951,23 @@ async fn mcp_create_string_shorthand_deserializes_before_auth() { "origin", "https://github.com/fabro-sh/fabro.git", ]); - let missing_push = format!("file://{}/missing.git", context.temp_dir.display()); + let test_origin = context.temp_dir.join("origin.git"); run_git(&context.temp_dir, &[ - "remote", - "set-url", - "--push", - "origin", - &missing_push, + "init", + "--bare", + "--quiet", + test_origin + .to_str() + .expect("test origin path should be UTF-8"), ]); + let push_url = format!("file://{}", test_origin.display()); + run_git(&context.temp_dir, &[ + "remote", "set-url", "--push", "origin", &push_url, + ]); + let workflow_version_id = + fabro_manifest::collect_workflow_versions(&workflow, &context.temp_dir) + .expect("workflow fixture should package") + .root_id(); let client = spawn_mcp_client(&context, &["--server", &target_url]).await; let result = client @@ -1981,6 +1990,20 @@ async fn mcp_create_string_shorthand_deserializes_before_auth() { error.contains("Run `fabro auth login` to authenticate."), "{error}" ); + assert!( + harness + .api_requests + .contains("POST /api/v1/workflow-versions"), + "the shorthand request should reach workflow-version authentication" + ); + assert!( + !harness.workflow_version_exists(workflow_version_id).await, + "an unauthenticated shorthand request must not register a workflow version" + ); + assert!( + !harness.api_requests.contains("POST /api/v1/runs"), + "an unauthenticated shorthand request must not reach run creation" + ); assert_mcp_run_tool_count(&client).await; client diff --git a/lib/apps/fabro-cli/tests/it/support/auth_harness.rs b/lib/apps/fabro-cli/tests/it/support/auth_harness.rs index 315465096..d9fb34389 100644 --- a/lib/apps/fabro-cli/tests/it/support/auth_harness.rs +++ b/lib/apps/fabro-cli/tests/it/support/auth_harness.rs @@ -25,7 +25,9 @@ use fabro_server::jwt_auth::resolve_auth_mode_with_lookup; use fabro_server::server::{RouterOptions, build_router_with_options}; use fabro_server::test_support::TestAppStateBuilder; use fabro_static::EnvVars; +use fabro_store::Database; use fabro_test::{GitHubAppState, TestContext, apply_test_isolation}; +use fabro_types::WorkflowVersionId; use serde_json::Value; use tokio::net::TcpListener; use tokio::sync::oneshot; @@ -42,6 +44,7 @@ pub(crate) const TEST_DEV_TOKEN: &str = pub(crate) struct RealAuthHarness { pub(crate) api_base_url: String, api_server: RunningHttpServer, + store: Arc, twin: fabro_test::TwinGitHub, pub(crate) api_requests: ListenerRequestLog, } @@ -84,11 +87,13 @@ impl RealAuthHarness { if let Some(token) = dev_token.clone() { secrets.insert("FABRO_DEV_TOKEN".to_string(), token); } + let (store, artifact_store) = fabro_server::test_support::test_store_bundle(); let state = TestAppStateBuilder::new() .runtime_settings(settings, RunLayer::default()) .max_concurrent_runs(5) .env_lookup(|_| None) .server_secret_env(secrets) + .store_bundle(Arc::clone(&store), artifact_store) .vault_entries([ ("GITHUB_APP_CLIENT_SECRET", github_client_secret.as_str()), (EnvVars::OPENAI_API_KEY, "test-openai-api-key"), @@ -113,6 +118,7 @@ impl RealAuthHarness { Self { api_base_url, api_server, + store, twin, api_requests, } @@ -122,6 +128,15 @@ impl RealAuthHarness { format!("{}/api/v1", self.api_base_url) } + pub(crate) async fn workflow_version_exists(&self, id: WorkflowVersionId) -> bool { + let blob_hash = id.into(); + self.store + .blobs() + .exists(&blob_hash) + .await + .expect("workflow-version blob lookup should succeed") + } + pub(crate) async fn shutdown(self) { self.api_server.shutdown().await; self.twin.shutdown().await; diff --git a/lib/apps/fabro-server/src/run_tool_create.rs b/lib/apps/fabro-server/src/run_tool_create.rs index f4f36e8da..048f4d940 100644 --- a/lib/apps/fabro-server/src/run_tool_create.rs +++ b/lib/apps/fabro-server/src/run_tool_create.rs @@ -1,12 +1,10 @@ -use std::collections::HashMap; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, bail}; use async_trait::async_trait; -use fabro_config::RunLayer; use fabro_manifest::{ - CollectedWorkflowClosure, ResolvedLocalWorkflowPackage, RunOverrideInput, - collect_workflow_versions, observe_git_run_target, resolve_local_workflow_package, + CollectedWorkflowClosure, ResolvedLocalWorkflowPackage, collect_workflow_versions, + observe_git_run_target, resolve_local_workflow_package, }; use fabro_tool::{ CreateRunWorkflowSource, PreparedRunCreate, RunCreateAdapter, ValidatedCreateRunSpec, @@ -16,8 +14,6 @@ use fabro_types::{DirtyStatus, RunTarget}; use tokio::io::AsyncWriteExt; use tokio::{fs, task}; -use crate::manifest_validation; - #[derive(Clone, Debug)] pub struct ServerRunCreateAdapter { mode: RunCreateMode, @@ -145,13 +141,6 @@ impl ServerRunCreateAdapter { "target is required because the local checkout cannot be represented as a GitHub run target" ) })?; - let mut warnings = Vec::new(); - if observation.legacy_git_context.dirty == DirtyStatus::Dirty { - warnings.push( - "the local checkout has uncommitted changes; those changes are excluded from the run target" - .to_string(), - ); - } if observation .legacy_git_context .sha @@ -159,8 +148,14 @@ impl ServerRunCreateAdapter { .is_some_and(|sha| !sha.is_empty()) && target.sha.is_none() { + bail!( + "the exact local Git commit could not be made available from the canonical GitHub origin; push the commit and try again" + ); + } + let mut warnings = Vec::new(); + if observation.legacy_git_context.dirty == DirtyStatus::Dirty { warnings.push( - "the local HEAD commit is not fetchable and is not pinned; the remote branch will be selected" + "the local checkout has uncommitted changes; those changes are excluded from the run target" .to_string(), ); } @@ -221,7 +216,6 @@ impl RunCreateAdapter for ServerRunCreateAdapter { } CreateRunWorkflowSource::Inline(source) => collect_inline_workflow(source).await?, }; - validate_local_source(&closure, spec, goal.as_deref())?; let resolved_target = self.resolve_target(spec, cwd).await?; let versions = closure .versions() @@ -286,40 +280,13 @@ async fn collect_inline_workflow( ) })?; } - collect_workflow_versions(Path::new(source.entrypoint.as_str()), root.path()) - .map_err(anyhow::Error::new) -} - -fn validate_local_source( - closure: &CollectedWorkflowClosure, - spec: &ValidatedCreateRunSpec, - goal: Option<&str>, -) -> Result<()> { - let run_overrides = run_tool_run_overrides(spec, goal); - let inputs = spec - .inputs - .iter() - .map(|(key, value)| (key.clone(), value.toml().clone())) - .collect::>(); - let response = - manifest_validation::validate_collected_workflow(closure, run_overrides.as_ref(), &inputs)?; - if !response.ok { - bail!("workflow validation failed"); - } - Ok(()) -} - -fn run_tool_run_overrides(spec: &ValidatedCreateRunSpec, goal: Option<&str>) -> Option { - fabro_manifest::build_sparse_run_overrides(RunOverrideInput { - goal, - model: spec.model.as_deref(), - provider: spec.provider.as_deref(), - environment: spec.environment.as_deref(), - preserve_sandbox: spec.preserve_sandbox, - dry_run: spec.dry_run, - auto_approve: spec.auto_approve, - labels: spec.labels.clone(), + let entrypoint = source.entrypoint.clone(); + task::spawn_blocking(move || { + collect_workflow_versions(Path::new(entrypoint.as_str()), root.path()) + .map_err(anyhow::Error::new) }) + .await + .context("inline workflow package collection task failed")? } #[cfg(test)] @@ -328,7 +295,8 @@ mod tests { use std::process::Command; use std::sync::{Arc, Mutex}; - use fabro_tool::{FabroRunCreateParams, ValidatedCreateRuns}; + use fabro_tool::fabro_client::ClientBackend; + use fabro_tool::{FabroRunCreateParams, FabroToolBackend as _, ValidatedCreateRuns}; use fabro_types::{GitRunTarget, WorkflowVersion, WorkflowVersionId}; use httpmock::Method::POST; use httpmock::{HttpMockRequest, HttpMockResponse, MockServer}; @@ -585,24 +553,24 @@ mod tests { } #[tokio::test] - async fn workflow_version_local_content_is_validated_before_registration() { - let client = no_proxy_client("http://127.0.0.1:9"); + async fn workflow_version_is_registered_before_server_admission_rejection() { + let server = MockServer::start_async().await; + let registered = Arc::new(Mutex::new(Vec::new())); + let registration = + dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; + let admission = server + .mock_async(|when, then| { + when.method(POST).path("/api/v1/runs"); + then.status(422) + .header("content-type", "text/plain") + .body("server-authoritative workflow rejection"); + }) + .await; + let client = Arc::new(no_proxy_client(&server.url(""))); let adapter = ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, None, None); - - let invalid_graph = validated_spec(&json!({ - "workflow": { - "kind": "inline", - "entrypoint": "workflow.fabro", - "files": { "workflow.fabro": "this is not a graph" } - }, - "target": { "kind": "none" } - })); - adapter - .prepare(&client, &invalid_graph, Path::new("/ignored")) - .await - .expect_err("invalid graph should fail before registration"); - - let undefined_input = validated_spec(&json!({ + let backend = + ClientBackend::new(Arc::clone(&client)).with_run_create_adapter(Arc::new(adapter)); + let spec = validated_spec(&json!({ "workflow": { "kind": "inline", "entrypoint": "workflow.fabro", @@ -618,11 +586,20 @@ mod tests { }, "target": { "kind": "none" } })); - let error = adapter - .prepare(&client, &undefined_input, Path::new("/ignored")) + let error = backend + .create_run_from_spec(&spec, Path::new("/ignored"), None) .await - .expect_err("undefined input should fail before registration"); - assert!(error.to_string().contains("workflow validation failed")); + .expect_err("the server should reject the semantically invalid workflow"); + + registration.assert_calls_async(1).await; + admission.assert_calls_async(1).await; + assert_eq!(registered.lock().unwrap().len(), 1); + assert!( + error + .to_string() + .contains("server-authoritative workflow rejection"), + "unexpected error: {error:#}" + ); } #[tokio::test] @@ -684,7 +661,7 @@ mod tests { } #[tokio::test] - async fn workflow_version_standalone_git_fallback_reports_excluded_local_bytes() { + async fn workflow_version_standalone_rejects_unavailable_head_before_registration_or_create() { let temp = tempfile::tempdir().unwrap(); let workspace = temp.path().join("workspace"); fs::create_dir(&workspace).await.unwrap(); @@ -711,6 +688,81 @@ mod tests { run_git(&workspace, &[ "remote", "set-url", "--push", "origin", &missing, ]); + let spec = validated_spec(&json!({ + "workflow": { + "kind": "inline", + "entrypoint": "workflow.fabro", + "files": { + "workflow.fabro": "digraph W { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }" + } + } + })); + let server = MockServer::start_async().await; + let registered = Arc::new(Mutex::new(Vec::new())); + let registration = + dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; + let create = server + .mock_async(|when, then| { + when.method(POST).path("/api/v1/runs"); + then.status(500); + }) + .await; + let client = Arc::new(no_proxy_client(&server.url(""))); + let adapter = ServerRunCreateAdapter::standalone(None); + let backend = + ClientBackend::new(Arc::clone(&client)).with_run_create_adapter(Arc::new(adapter)); + + let error = backend + .create_run_from_spec(&spec, &workspace, None) + .await + .expect_err("an unavailable local HEAD must not degrade to a branch-only target"); + + registration.assert_calls_async(0).await; + create.assert_calls_async(0).await; + assert!(registered.lock().unwrap().is_empty()); + assert!( + error + .to_string() + .contains("exact local Git commit could not be made available"), + "unexpected error: {error:#}" + ); + } + + #[tokio::test] + async fn workflow_version_standalone_preserves_dirty_warning_for_exact_target() { + let temp = tempfile::tempdir().unwrap(); + let workspace = temp.path().join("workspace"); + let origin = temp.path().join("origin.git"); + fs::create_dir(&workspace).await.unwrap(); + run_git(temp.path(), &[ + "init", + "--bare", + "--quiet", + origin.to_str().unwrap(), + ]); + run_git(&workspace, &[ + "init", + "--quiet", + "--initial-branch", + "feature", + ]); + run_git(&workspace, &["config", "user.name", "Fabro Test"]); + run_git(&workspace, &["config", "user.email", "fabro@example.com"]); + fs::write(workspace.join("tracked.txt"), "committed") + .await + .unwrap(); + run_git(&workspace, &["add", "tracked.txt"]); + run_git(&workspace, &["commit", "--quiet", "-m", "initial"]); + run_git(&workspace, &[ + "remote", + "add", + "origin", + "https://github.com/acme/widgets.git", + ]); + let push_url = format!("file://{}", origin.display()); + run_git(&workspace, &[ + "remote", "set-url", "--push", "origin", &push_url, + ]); fs::write(workspace.join("dirty.txt"), "uncommitted") .await .unwrap(); @@ -732,18 +784,12 @@ mod tests { }; assert_eq!(target.repo, "acme/widgets"); assert_eq!(target.branch, "feature"); - assert_eq!(target.sha, None); + assert!(target.sha.is_some()); assert!( prepared .warnings .iter() .any(|warning| warning.contains("uncommitted changes")) ); - assert!( - prepared - .warnings - .iter() - .any(|warning| warning.contains("not fetchable") && warning.contains("not pinned")) - ); } } diff --git a/lib/components/fabro-tool/src/common.rs b/lib/components/fabro-tool/src/common.rs index 399606dbe..9cebd72d2 100644 --- a/lib/components/fabro-tool/src/common.rs +++ b/lib/components/fabro-tool/src/common.rs @@ -62,9 +62,11 @@ pub struct CreateRunSubmission { /// Trusted producer-local preparation for one tool-created run. /// -/// Implementations acquire permitted workflow bytes and goal files, validate -/// local content, resolve the independent target, and register immutable -/// workflow versions before returning intent-ready fields. +/// Implementations acquire permitted workflow bytes and goal files, enforce +/// producer capabilities and package structure, resolve the independent +/// target, and register immutable workflow versions before returning +/// intent-ready fields. The server remains authoritative for semantic run +/// admission. #[async_trait] pub trait RunCreateAdapter: Send + Sync { async fn prepare( diff --git a/lib/components/fabro-tool/src/create.rs b/lib/components/fabro-tool/src/create.rs index 53cb1800b..85cc97ae6 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 observable Git checkout when omitted.", + "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.", "anyOf": [ { "type": "null" }, { @@ -414,7 +414,6 @@ pub struct ValidatedCreateRunSpec { #[derive(Debug)] pub struct ValidatedRunInputValue { json: Value, - toml: toml::Value, } impl ValidatedRunInputValue { @@ -422,11 +421,6 @@ impl ValidatedRunInputValue { pub fn json(&self) -> &Value { &self.json } - - #[must_use] - pub fn toml(&self) -> &toml::Value { - &self.toml - } } impl TryFrom for ValidatedCreateRuns { @@ -501,8 +495,8 @@ impl TryFrom for ValidatedCreateRunSpec { .into_iter() .map(|(key, value)| { let json = value.into_inner(); - manifest::json_to_toml_value(&key, &json) - .map(|toml| (key, ValidatedRunInputValue { json, toml })) + manifest::json_to_toml_value(&key, &json)?; + Ok((key, ValidatedRunInputValue { json })) }) .collect::>>()?; let target = spec