From 45741a3e6ec17297d63c6a946d71bf18ecb1c979 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 1 Sep 2026 10:23:47 -0400 Subject: [PATCH] Fix RunIntent producer CI failures --- lib/apps/fabro-cli/src/commands/run/runner.rs | 14 ++-- lib/apps/fabro-cli/tests/it/cmd/mcp.rs | 43 ++++++++++- lib/apps/fabro-mcp-server/src/server.rs | 5 +- lib/apps/fabro-server/src/run_tool_create.rs | 76 ++++++++++++------- 4 files changed, 98 insertions(+), 40 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 12ec98cf0..a3d6ab858 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -18,8 +18,10 @@ use fabro_manifest::SuppliedWorkflowVersionPackager; use fabro_server::run_tool_create::ServerRunCreateAdapter; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_tool::fabro_client::ClientBackend; -use fabro_types::settings::run::{RunMode, RunNamespace}; -use fabro_types::{ArtifactUpload, BlobHash, EventBody, FailureReason, Principal, RunEvent, RunId}; +use fabro_types::settings::run::{EnvironmentProvider, RunMode, RunNamespace}; +use fabro_types::{ + ArtifactUpload, BlobHash, EventBody, FailureReason, Principal, RunEvent, RunId, RunTarget, +}; use fabro_vault::{SecretStore, Vault}; use fabro_workflow::artifact_upload::{ArtifactSink, StageArtifactUploader}; use fabro_workflow::event::{Emitter, RunEventSink}; @@ -231,8 +233,8 @@ fn build_fabro_run_tool_services( worker_token: &str, client: fabro_client::Client, current_run_id: RunId, - provider: fabro_types::settings::run::EnvironmentProvider, - inherited_target: Option, + provider: EnvironmentProvider, + inherited_target: Option, source_directory: Option<&str>, run_dir: &Path, ) -> Option { @@ -254,8 +256,8 @@ fn build_fabro_run_tool_services( } fn worker_run_create_adapter( - provider: fabro_types::settings::run::EnvironmentProvider, - inherited_target: Option, + provider: EnvironmentProvider, + inherited_target: Option, user_workflows_root: Option, ) -> ServerRunCreateAdapter { ServerRunCreateAdapter::worker(provider, inherited_target, user_workflows_root) diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index c07daff5a..3c096d67b 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -743,6 +743,7 @@ async fn mcp_create_and_search_manage_real_runs_with_cli_auth() { serde_json::json!({ "runs": [{ "workflow": workflow, + "target": { "kind": "none" }, "dry_run": true, "auto_approve": true, "labels": { "source": "mcp-test" } @@ -781,7 +782,7 @@ async fn mcp_create_and_search_manage_real_runs_with_cli_auth() { "labels": { "source": "mcp-test" }, - "source_directory": "[SOURCE_DIRECTORY]", + "source_directory": null, "repo_origin_url": null, "goal_preview": "Run tests and report results", "goal_truncated": false @@ -810,6 +811,7 @@ async fn mcp_run_tools_use_default_local_server_without_server_flag() { serde_json::json!({ "runs": [{ "workflow": workflow, + "target": { "kind": "none" }, "dry_run": true, "auto_approve": true, "labels": { "source": "mcp-default-server-test" }, @@ -1388,7 +1390,7 @@ async fn mcp_lifecycle_tools_manage_real_run() { "labels": { "source": "mcp-test" }, - "source_directory": "[SOURCE_DIRECTORY]", + "source_directory": null, "repo_origin_url": null, "goal": "Run tests and report results" } @@ -1934,6 +1936,29 @@ async fn mcp_create_string_shorthand_deserializes_before_auth() { RealAuthHarness::start_with_dev_token(fabro_test::GitHubAppState::default()).await; let target_url = harness.api_target(); let workflow = context.install_fixture("simple.fabro"); + context.git_init(); + run_git(&context.temp_dir, &["config", "user.name", "Fabro Test"]); + run_git(&context.temp_dir, &[ + "config", + "user.email", + "fabro@example.com", + ]); + run_git(&context.temp_dir, &["add", "simple.fabro"]); + run_git(&context.temp_dir, &["commit", "--quiet", "-m", "fixture"]); + run_git(&context.temp_dir, &[ + "remote", + "add", + "origin", + "https://github.com/fabro-sh/fabro.git", + ]); + let missing_push = format!("file://{}/missing.git", context.temp_dir.display()); + run_git(&context.temp_dir, &[ + "remote", + "set-url", + "--push", + "origin", + &missing_push, + ]); let client = spawn_mcp_client(&context, &["--server", &target_url]).await; let result = client @@ -2828,6 +2853,7 @@ async fn create_mcp_run(client: &McpClient, workflow: PathBuf, start: bool) -> S serde_json::json!({ "runs": [{ "workflow": workflow, + "target": { "kind": "none" }, "dry_run": true, "auto_approve": true, "labels": { "source": "mcp-test" }, @@ -2842,6 +2868,19 @@ async fn create_mcp_run(client: &McpClient, workflow: PathBuf, start: bool) -> S .to_string() } +fn run_git(cwd: &Path, args: &[&str]) { + let output = Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .expect("git command should run"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + fn seed_oauth_auth( home_dir: &Path, target: &fabro_client::ServerTarget, diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index 670fda34b..11dc6d8ab 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_server::run_tool_create::ServerRunCreateAdapter; use fabro_tool::fabro_client::ClientBackend; use fabro_tool::{self as run_tools, FabroToolBackend}; use fabro_util::version::FABRO_VERSION; @@ -279,9 +280,7 @@ impl FabroMcpServer { .map(|parent| parent.join("workflows")); Arc::new( ClientBackend::new(Arc::new(client)).with_run_create_adapter(Arc::new( - fabro_server::run_tool_create::ServerRunCreateAdapter::standalone( - user_workflows_root, - ), + ServerRunCreateAdapter::standalone(user_workflows_root), )).with_workflow_version_packager(Arc::new(SuppliedWorkflowVersionPackager)), ) as Arc }) diff --git a/lib/apps/fabro-server/src/run_tool_create.rs b/lib/apps/fabro-server/src/run_tool_create.rs index d2146f6b4..415b28b3f 100644 --- a/lib/apps/fabro-server/src/run_tool_create.rs +++ b/lib/apps/fabro-server/src/run_tool_create.rs @@ -13,6 +13,7 @@ use fabro_tool::{ }; use fabro_types::settings::run::EnvironmentProvider; use fabro_types::{DirtyStatus, RunTarget}; +use tokio::fs; use tokio::io::AsyncWriteExt; use crate::manifest_validation; @@ -95,7 +96,7 @@ impl ServerRunCreateAdapter { ); } let path = cwd.join(goal_file); - tokio::fs::read_to_string(&path) + fs::read_to_string(&path) .await .with_context(|| format!("failed to read goal file {}", path.display())) .map(Some) @@ -254,14 +255,14 @@ impl LocalWorkflowSource { for (path, content) in &source.files { let destination = root.path().join(path.as_str()); if let Some(parent) = destination.parent() { - tokio::fs::create_dir_all(parent).await.with_context(|| { + fs::create_dir_all(parent).await.with_context(|| { format!( "failed to create inline workflow directory {}", parent.display() ) })?; } - let mut file = tokio::fs::OpenOptions::new() + let mut file = fs::OpenOptions::new() .write(true) .create_new(true) .open(&destination) @@ -341,7 +342,7 @@ mod tests { use super::*; - fn validated_spec(value: serde_json::Value) -> ValidatedCreateRunSpec { + fn validated_spec(value: &serde_json::Value) -> ValidatedCreateRunSpec { let params: FabroRunCreateParams = serde_json::from_value(json!({ "runs": [value] })) .expect("create input should deserialize"); ValidatedCreateRuns::try_from(params) @@ -354,6 +355,10 @@ mod tests { fabro_client::Client::new_no_proxy(base_url).expect("test client should build") } + #[expect( + clippy::disallowed_methods, + reason = "test fixture setup uses the Git CLI against an isolated temporary repository" + )] fn run_git(cwd: &Path, args: &[&str]) { let output = Command::new("git") .args(args) @@ -367,10 +372,10 @@ mod tests { ); } - async fn dynamic_version_registration_mock<'a>( - server: &'a MockServer, + async fn dynamic_version_registration_mock( + server: &MockServer, registered: Arc>>, - ) -> httpmock::Mock<'a> { + ) -> httpmock::Mock<'_> { server .mock_async(move |when, then| { when.method(POST).path("/api/v1/workflow-versions"); @@ -396,7 +401,7 @@ mod tests { let registration = dynamic_version_registration_mock(&server, Arc::clone(®istered)).await; let client = no_proxy_client(&server.url("")); - let spec = validated_spec(json!({ + let spec = validated_spec(&json!({ "workflow": { "kind": "inline", "entrypoint": "root/workflow.fabro", @@ -468,7 +473,7 @@ mod tests { tag: Some("v1.0.0".to_string()), sha: Some("0123456789abcdef0123456789abcdef01234567".to_string()), }); - let spec = validated_spec(json!({ + let spec = validated_spec(&json!({ "workflow": { "kind": "stored", "workflow_version_id": workflow_version_id @@ -494,25 +499,29 @@ mod tests { let temp = tempfile::tempdir().unwrap(); let operation_cwd = temp.path().join("nested/operation"); let workflow_dir = temp.path().join(".fabro/workflows/demo"); - std::fs::create_dir_all(&operation_cwd).unwrap(); - std::fs::create_dir_all(&workflow_dir).unwrap(); - std::fs::write(temp.path().join(".fabro/project.toml"), "_version = 1\n").unwrap(); - std::fs::write( + fs::create_dir_all(&operation_cwd).await.unwrap(); + fs::create_dir_all(&workflow_dir).await.unwrap(); + fs::write(temp.path().join(".fabro/project.toml"), "_version = 1\n") + .await + .unwrap(); + fs::write( workflow_dir.join("workflow.toml"), "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", ) + .await .unwrap(); - std::fs::write( + fs::write( workflow_dir.join("workflow.fabro"), "digraph Demo { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", ) + .await .unwrap(); 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 client = no_proxy_client(&server.url("")); - let spec = validated_spec(json!({ + let spec = validated_spec(&json!({ "workflow": "demo", "target": { "kind": "none" } })); @@ -536,20 +545,22 @@ mod tests { async fn workflow_version_worker_capabilities_gate_selector_and_goal_file_before_reads() { let temp = tempfile::tempdir().unwrap(); let workflow = temp.path().join("same-name.fabro"); - std::fs::write( + fs::write( &workflow, "digraph HostCopy { start [shape=Mdiamond] exit [shape=Msquare] start -> exit }", ) + .await .unwrap(); - std::fs::write( + fs::write( temp.path().join("goal.md"), "host goal that must not be read", ) + .await .unwrap(); let client = no_proxy_client("http://127.0.0.1:9"); let adapter = ServerRunCreateAdapter::worker(EnvironmentProvider::Daytona, None, None); - let selector = validated_spec(json!({ + let selector = validated_spec(&json!({ "workflow": "same-name.fabro", "target": { "kind": "none" } })); @@ -564,7 +575,7 @@ mod tests { ); let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let goal_file = validated_spec(json!({ + let goal_file = validated_spec(&json!({ "workflow": { "kind": "stored", "workflow_version_id": workflow_version_id @@ -584,7 +595,7 @@ mod tests { let client = no_proxy_client("http://127.0.0.1:9"); let adapter = ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, None, None); - let invalid_graph = validated_spec(json!({ + let invalid_graph = validated_spec(&json!({ "workflow": { "kind": "inline", "entrypoint": "workflow.fabro", @@ -597,7 +608,7 @@ mod tests { .await .expect_err("invalid graph should fail before registration"); - let undefined_input = validated_spec(json!({ + let undefined_input = validated_spec(&json!({ "workflow": { "kind": "inline", "entrypoint": "workflow.fabro", @@ -624,7 +635,7 @@ mod tests { async fn workflow_version_target_failure_precedes_registration() { let client = no_proxy_client("http://127.0.0.1:9"); let adapter = ServerRunCreateAdapter::worker(EnvironmentProvider::Docker, None, None); - let spec = validated_spec(json!({ + let spec = validated_spec(&json!({ "workflow": { "kind": "inline", "entrypoint": "workflow.fabro", @@ -642,17 +653,20 @@ mod tests { assert!( error .to_string() - .contains("parent run has no canonical target") + .contains("parent run has no canonical target"), + "unexpected error: {error:#}" ); } #[tokio::test] async fn workflow_version_shared_goal_file_and_explicit_target_are_preserved() { let temp = tempfile::tempdir().unwrap(); - std::fs::write(temp.path().join("goal.md"), "goal from shared filesystem").unwrap(); + fs::write(temp.path().join("goal.md"), "goal from shared filesystem") + .await + .unwrap(); 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!({ + let spec = validated_spec(&json!({ "workflow": { "kind": "stored", "workflow_version_id": workflow_version_id @@ -679,7 +693,7 @@ mod tests { async fn workflow_version_standalone_git_fallback_reports_excluded_local_bytes() { let temp = tempfile::tempdir().unwrap(); let workspace = temp.path().join("workspace"); - std::fs::create_dir(&workspace).unwrap(); + fs::create_dir(&workspace).await.unwrap(); run_git(&workspace, &[ "init", "--quiet", @@ -688,7 +702,9 @@ mod tests { ]); run_git(&workspace, &["config", "user.name", "Fabro Test"]); run_git(&workspace, &["config", "user.email", "fabro@example.com"]); - std::fs::write(workspace.join("tracked.txt"), "committed").unwrap(); + 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, &[ @@ -701,10 +717,12 @@ mod tests { run_git(&workspace, &[ "remote", "set-url", "--push", "origin", &missing, ]); - std::fs::write(workspace.join("dirty.txt"), "uncommitted").unwrap(); + fs::write(workspace.join("dirty.txt"), "uncommitted") + .await + .unwrap(); let workflow_version_id: WorkflowVersionId = fabro_types::BlobHash::new(b"stored").into(); - let spec = validated_spec(json!({ + let spec = validated_spec(&json!({ "workflow": { "kind": "stored", "workflow_version_id": workflow_version_id