From d5dec0fffb6e6bee8e94379e51fb9292120342e8 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 8 Sep 2026 12:39:21 -0400 Subject: [PATCH 01/15] Add content-based workflow version registration tools --- Cargo.lock | 2 + docs/public/agents/mcp.mdx | 42 +- docs/public/api-reference/fabro-api.yaml | 3 + docs/public/execution/run-configuration.mdx | 2 +- lib/apps/fabro-cli/src/commands/run/runner.rs | 4 +- lib/apps/fabro-cli/tests/it/cmd/mcp.rs | 19 + lib/apps/fabro-mcp-server/src/server.rs | 82 +++- lib/apps/fabro-server/src/lib.rs | 1 + .../src/server/handler/workflow_versions.rs | 5 +- lib/apps/fabro-server/src/server/tests.rs | 70 +++ .../fabro-server/src/workflow_version_tool.rs | 461 ++++++++++++++++++ lib/components/fabro-manifest/src/lib.rs | 1 + .../fabro-manifest/src/workflow_bundler.rs | 28 +- .../src/workflow_version_collector.rs | 10 +- lib/components/fabro-tool/src/common.rs | 28 ++ lib/components/fabro-tool/src/fabro_client.rs | 30 +- lib/components/fabro-tool/src/lib.rs | 9 +- .../fabro-tool/src/workflow_version.rs | 234 +++++++++ lib/foundation/fabro-config/src/project.rs | 15 + lib/foundation/fabro-types/Cargo.toml | 2 + lib/foundation/fabro-types/src/lib.rs | 1 + .../fabro-types/src/workflow_version.rs | 97 +++- .../src/api/workflow-versions-api.ts | 8 +- 23 files changed, 1109 insertions(+), 45 deletions(-) create mode 100644 lib/apps/fabro-server/src/workflow_version_tool.rs create mode 100644 lib/components/fabro-tool/src/workflow_version.rs diff --git a/Cargo.lock b/Cargo.lock index 58a7ae412..b7c953564 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3214,6 +3214,8 @@ dependencies = [ "thiserror 2.0.18", "toml 0.8.23", "ulid", + "unicase", + "unicode-normalization", "url", ] diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index b0920090f..9ee9bf5a1 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -5,7 +5,7 @@ description: "Connect MCP tools to agents and expose Fabro runs to MCP clients" MCP ([Model Context Protocol](https://modelcontextprotocol.io/)) lets you connect external tool servers to Fabro agents. An MCP server exposes tools over a standardized protocol — databases, APIs, file systems, custom services — and Fabro discovers and registers them automatically. Agents call MCP tools the same way they call built-in tools. -Fabro can also run as an MCP server. MCP clients can use Fabro's run-management tools to create, inspect, control, wait for, and read events from workflow runs through the authenticated `fabro` CLI. +Fabro can also run as an MCP server. MCP clients can register reusable workflow versions and use Fabro's run-management tools to create, inspect, control, wait for, and read events from workflow runs through the authenticated `fabro` CLI. Workflow agents can opt in to that same run-management tool catalog with `[run.agent] fabro_tools = true`. This is not the same as configuring external MCP servers for the agent. When a workflow agent calls `fabro_run_create`, created runs are always [child runs](/execution/child-runs) of the current run; an explicit `parent_id` must match the current run ID. @@ -39,6 +39,7 @@ fabro mcp init claude --name fabro-testing --server https://fabro-testing.exampl | Tool | Purpose | |---|---| +| `fabro_workflow_version_create` | Register supplied workflow contents and local dependencies as an immutable version ID, without creating a run. | | `fabro_run_create` | Create one or more workflow runs, optionally under a parent run, starting them by default. | | `fabro_run_search` | Search runs by ID, parent, workflow, labels, status, archive state, and creation time. | | `fabro_run_get` | Read-only inspection of a run: returns its summary, projection, and pending questions without mutating state. | @@ -47,6 +48,45 @@ fabro mcp init claude --name fabro-testing --server https://fabro-testing.exampl | `fabro_run_pair` | Inspect, start, message, end, or read transcript for a live run pairing session. | | `fabro_run_events` | List, inspect, or search stored events for a run. | +### Register workflow contents from a sandbox + +Use shell and read tools in your sandbox to acquire the workflow and all its local +config, graph, prompt, script, import, and child-workflow files. For example, clone +a repository with your sandbox's shell tool, then read `workflow.fabro` and its +referenced `prompt.md`. Submit the actual contents: + +```json +{ + "entrypoint": "workflow.fabro", + "files": { + "workflow.fabro": "digraph W { start [shape=Mdiamond] work [prompt=\"@prompt.md\"] exit [shape=Msquare] start -> work -> exit }", + "prompt.md": "Review the implementation." + } +} +``` + +Call `fabro_workflow_version_create` with this object and keep the returned +`workflow_version_id`. You can reuse it in a `RunIntent` submitted through the +[Create Run API](/api-reference/runs/create-run). Registration packages and uploads +child workflows before their parents; callers do not calculate dependency IDs. + +`entrypoint` is an exact supplied key, including when it has no extension. File +values are text, never host paths or URLs to fetch. Missing references and paths +that escape the supplied tree fail. The source tree is limited to 512 files, +512 KiB per file, and 2 MiB of text; each resulting serialized version must also +fit the existing 2 MiB API limit. Case-insensitive file and ancestor collisions +are rejected before staging. Collection follows declared file references; command +`script` values remain literal text, and paths embedded in shell commands are not +inspected or acquired. + +Registration resolves no runtime secrets, selects no environment, and starts no +execution. It requires a user credential or a worker token with `agent:run_tools`; +ordinary worker tokens and same-run Ask Fabro sessions cannot register versions. +If an upload fails, retry the same contents: previously registered immutable +versions remain reusable. The existing `fabro_run_create` input is unchanged. + +### Create runs + For a simple create call, `fabro_run_create` accepts a workflow selector string: ```json diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 9ec0dbb2d..96ca10147 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -1086,6 +1086,9 @@ paths: description: >- Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. + Requires an authenticated user or a worker token with the `agent:run_tools` + capability. Ordinary worker tokens cannot register versions. Registration + creates no run and starts no execution. requestBody: required: true content: diff --git a/docs/public/execution/run-configuration.mdx b/docs/public/execution/run-configuration.mdx index 1a617e977..79637e882 100644 --- a/docs/public/execution/run-configuration.mdx +++ b/docs/public/execution/run-configuration.mdx @@ -509,7 +509,7 @@ Configure workflow agent behavior that is not tied to a single stage. fabro_tools = true ``` -`fabro_tools` defaults to `false`. Set it to `true` only for runs whose agents should be able to use the same Fabro run-management MCP tool catalog exposed to human MCP clients: create, search, get, interact, gather, events, and pair. +`fabro_tools` defaults to `false`. Set it to `true` only for runs whose agents should be able to use the same Fabro run-management MCP tool catalog exposed to human MCP clients: workflow version registration, create, search, get, interact, gather, events, and pair. One workflow-agent exception is intentional: `fabro_run_create` always creates [child runs](/execution/child-runs) parented to the current run. If an agent supplies `parent_id`, it must match the current run ID. diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 16776bc88..df7b2f531 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -16,6 +16,7 @@ use fabro_interview::{ WorkerControlMessage, }; use fabro_server::run_tool_manifest; +use fabro_server::workflow_version_tool::ServerWorkflowVersionCreateAdapter; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_tool::fabro_client::ClientBackend; use fabro_types::settings::run::{RunMode, RunNamespace}; @@ -236,7 +237,8 @@ fn build_fabro_run_tool_services( return None; } let backend = ClientBackend::new(Arc::new(client)) - .with_manifest_builder(Arc::new(WorkerRunManifestBuilder)); + .with_manifest_builder(Arc::new(WorkerRunManifestBuilder)) + .with_workflow_version_create_adapter(Arc::new(ServerWorkflowVersionCreateAdapter)); Some(FabroRunToolServices { backend: Arc::new(backend), current_run_id, diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 552134dcb..d791334b4 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -38,6 +38,7 @@ const MCP_RUN_TOOL_NAMES: &[&str] = &[ "fabro_run_interact", "fabro_run_pair", "fabro_run_search", + "fabro_workflow_version_create", ]; async fn assert_mcp_run_tool_count(client: &McpClient) { @@ -1847,6 +1848,24 @@ async fn mcp_get_rejects_blank_run_id_before_auth_or_network() { .expect("MCP client should shut down"); } +#[tokio::test(flavor = "multi_thread")] +async fn mcp_workflow_version_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_workflow_version_create", + serde_json::json!({"entrypoint":"workflow","files":{}}), + ) + .await; + assert_eq!(error, "entrypoint must be an exact supplied file key"); + assert_mcp_run_tool_count(&client).await; + 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!(); diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index 6a9c9941d..794da973a 100644 --- a/lib/apps/fabro-mcp-server/src/server.rs +++ b/lib/apps/fabro-mcp-server/src/server.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Result; +use fabro_server::workflow_version_tool::ServerWorkflowVersionCreateAdapter; use fabro_tool::fabro_client::ClientBackend; use fabro_tool::{self as run_tools, FabroToolBackend}; use fabro_util::version::FABRO_VERSION; @@ -74,7 +75,7 @@ impl ServerHandler for FabroMcpServer { fn get_info(&self) -> ServerInfo { ServerInfo::new(ServerCapabilities::builder().enable_tools().build()) .with_server_info(Implementation::new(SERVER_NAME, FABRO_VERSION).with_title("Fabro")) - .with_instructions("Use these tools to create, inspect, control, wait for, and read events from Fabro workflow runs.") + .with_instructions("Use these tools to register workflow versions and create, inspect, control, wait for, and read events from Fabro workflow runs.") } } @@ -90,6 +91,27 @@ impl FabroMcpServer { } } + #[tool( + name = "fabro_workflow_version_create", + description = "Register supplied workflow file contents and all local dependencies as a reusable immutable workflow version ID. Obtain files with shell/read tools first; this does not create or start a run." + )] + async fn fabro_workflow_version_create( + &self, + params: Parameters, + ) -> Result { + if let Err(err) = params.0.validate() { + return Ok(error_result(&err)); + } + let backend = match self.backend().await { + Ok(backend) => backend, + Err(err) => return Ok(error_result(&err)), + }; + match run_tools::create_workflow_version(backend, params.0).await { + Ok(result) => success_result(&result, run_tools::workflow_version_create_text(&result)), + Err(err) => Ok(error_result(&err)), + } + } + #[tool( name = "fabro_run_create", description = "Create one or more Fabro workflow runs, optionally under a parent run, starting them by default." @@ -252,7 +274,10 @@ impl FabroMcpServer { .map(|client| { Arc::new( ClientBackend::new(Arc::new(client)) - .with_manifest_builder(Arc::new(McpRunManifestBuilder)), + .with_manifest_builder(Arc::new(McpRunManifestBuilder)) + .with_workflow_version_create_adapter(Arc::new( + ServerWorkflowVersionCreateAdapter, + )), ) as Arc }) .map_err(|err| run_tools::ToolError::from_anyhow(&err)) @@ -283,6 +308,7 @@ fn error_result(err: &run_tools::ToolError) -> CallToolResult { #[cfg(test)] mod tests { + use std::collections::BTreeMap; use std::path::PathBuf; use std::sync::Arc; @@ -291,6 +317,58 @@ mod tests { use super::*; use crate::FabroMcpServerSettings; + #[tokio::test] + async fn workflow_version_mcp_surface_matches_catalog_and_returns_minimal_result() { + let mock = httpmock::MockServer::start_async().await; + let version = fabro_types::WorkflowVersion::new( + "workflow".parse().unwrap(), + BTreeMap::from([("workflow".parse().unwrap(), "digraph W {}".to_string())]), + BTreeMap::new(), + ) + .unwrap(); + let id = version.id().unwrap(); + let upload = mock + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(&version); + then.status(201) + .json_body(serde_json::json!({"workflow_version_id":id})); + }) + .await; + let url = mock.url(""); + let server = FabroMcpServer::new(Arc::new(FabroMcpServerSettings { + cwd: PathBuf::from("/does-not-exist"), + config_path: PathBuf::from("/does-not-exist"), + client_factory: Arc::new(move || { + let url = url.clone(); + Box::pin(async move { fabro_client::Client::new_no_proxy(&url) }) + }), + })); + let tools = server.tool_router.list_all(); + let tool = tools + .iter() + .find(|tool| tool.name == "fabro_workflow_version_create") + .unwrap(); + let definition = run_tools::tool_definitions() + .iter() + .find(|tool| tool.name == "fabro_workflow_version_create") + .unwrap(); + let mut expected = definition.parameters.clone(); + expected.as_object_mut().unwrap().remove("$schema"); + let mut actual = Value::Object(tool.input_schema.as_ref().clone()); + actual.as_object_mut().unwrap().remove("$schema"); + assert_eq!(actual, expected); + assert_eq!(tool.description.as_deref(), Some(definition.description)); + let result = server.fabro_workflow_version_create(Parameters(serde_json::from_value(serde_json::json!({"entrypoint":"workflow","files":{"workflow":"digraph W {}"}})).unwrap())).await.unwrap(); + assert_eq!( + result.structured_content, + Some(serde_json::json!({"workflow_version_id":id})) + ); + assert_ne!(result.is_error, Some(true)); + upload.assert_calls_async(1).await; + } + #[test] fn server_info_reports_fabro_version() { let settings = FabroMcpServerSettings { diff --git a/lib/apps/fabro-server/src/lib.rs b/lib/apps/fabro-server/src/lib.rs index 98f7a2c77..227855389 100644 --- a/lib/apps/fabro-server/src/lib.rs +++ b/lib/apps/fabro-server/src/lib.rs @@ -55,6 +55,7 @@ pub mod web_auth; mod worker_control; mod worker_runtime; mod worker_token; +pub mod workflow_version_tool; pub use error::{ApiError, Error, Result}; pub use run_manifest::workflow_bundle_from_manifest; diff --git a/lib/apps/fabro-server/src/server/handler/workflow_versions.rs b/lib/apps/fabro-server/src/server/handler/workflow_versions.rs index 69845679b..cd8daafc7 100644 --- a/lib/apps/fabro-server/src/server/handler/workflow_versions.rs +++ b/lib/apps/fabro-server/src/server/handler/workflow_versions.rs @@ -10,8 +10,9 @@ use fabro_workflow_version::{ }; use super::super::{ - ApiError, AppState, IntoResponse, Json, RequiredUser, Response, Router, State, StatusCode, post, + ApiError, AppState, IntoResponse, Json, Response, Router, State, StatusCode, post, }; +use crate::principal_middleware::RequiredRunManagementActor; const INVALID_JSON_CODE: &str = "invalid_json"; const INVALID_VERSION_CODE: &str = "workflow_version_invalid"; @@ -26,7 +27,7 @@ pub(super) fn routes() -> Router> { } async fn create_workflow_version( - _auth: RequiredUser, + _auth: RequiredRunManagementActor, State(state): State>, payload: Result, JsonRejection>, ) -> Result { diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index 3be6e1a29..e81a3b576 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -19738,3 +19738,73 @@ fn validate_github_slug_rejects_overlong() { let long = "a".repeat(40); assert!(super::validate_github_slug("owner", &long, 39).is_err()); } + +#[tokio::test] +async fn workflow_version_registration_requires_user_or_run_tools_capability() { + let (state, app) = jwt_auth_app(); + let run_id = RunId::new(); + let body = json!({"entrypoint":"workflow.fabro","files":{"workflow.fabro":"digraph W {}"},"workflow_dependencies":{}}); + for (token, expected) in [ + (issue_test_user_jwt(), StatusCode::CREATED), + ( + issue_test_run_tools_worker_token(&run_id), + StatusCode::CREATED, + ), + (issue_test_worker_token(&run_id), StatusCode::FORBIDDEN), + ] { + let response = app + .clone() + .oneshot(json_bearer_request( + Method::POST, + "/workflow-versions", + &token, + &body, + )) + .await + .unwrap(); + fabro_test::expect_axum_status(response, expected, "POST /workflow-versions actor matrix") + .await; + } + for body in [serde_json::to_string(&body).unwrap(), "{".to_string()] { + let response = app + .clone() + .oneshot( + Request::builder() + .method(Method::POST) + .uri(api("/workflow-versions")) + .header(header::CONTENT_TYPE, "application/json") + .body(Body::from(body)) + .unwrap(), + ) + .await + .unwrap(); + fabro_test::expect_axum_status( + response, + StatusCode::UNAUTHORIZED, + "anonymous POST /workflow-versions", + ) + .await; + } + let response = app + .oneshot(bearer_request( + Method::GET, + "/runs", + &issue_test_user_jwt(), + Body::empty(), + )) + .await + .unwrap(); + let listed = + fabro_test::expect_axum_json(response, StatusCode::OK, "GET /runs after registration") + .await; + assert_eq!(listed["data"], json!([])); + assert!( + state + .stores + .runs + .load_run_projection(&run_id) + .await + .unwrap() + .is_none() + ); +} diff --git a/lib/apps/fabro-server/src/workflow_version_tool.rs b/lib/apps/fabro-server/src/workflow_version_tool.rs new file mode 100644 index 000000000..27081eea0 --- /dev/null +++ b/lib/apps/fabro-server/src/workflow_version_tool.rs @@ -0,0 +1,461 @@ +use std::path::Path; + +use anyhow::Context as _; +use async_trait::async_trait; +use fabro_client::Client; +use fabro_config::project::WorkflowLocation; +use fabro_manifest::CollectedWorkflowClosure; +use fabro_tool::{FabroWorkflowVersionCreateParams, ToolError, WorkflowVersionCreateAdapter}; +use fabro_types::WorkflowVersionId; +use tokio::task; + +/// Content-only registration shared by standalone MCP and capable run workers. +pub struct ServerWorkflowVersionCreateAdapter; + +#[async_trait] +impl WorkflowVersionCreateAdapter for ServerWorkflowVersionCreateAdapter { + async fn create_workflow_version( + &self, + params: FabroWorkflowVersionCreateParams, + client: &Client, + ) -> anyhow::Result { + params.validate()?; + let closure = task::spawn_blocking(move || { + let staging = tempfile::Builder::new().prefix("fabro-workflow-version-").tempdir()?; + collect_supplied_workflow(¶ms, staging) + }) + .await + .context("workflow packaging task failed")? + // Parser diagnostics may contain supplied source. Keep them off the + // tool result boundary, including nested error chains. + .map_err(|_| ToolError::message("workflow source could not be packaged; check configuration, syntax, local references, and package limits"))?; + let versions = closure + .versions() + .map(|(_, version)| version.version()) + .collect::>(); + client.register_workflow_versions(versions).await?; + Ok(closure.root_id()) + } +} + +#[expect( + clippy::disallowed_methods, + reason = "bounded file staging and collection run on spawn_blocking" +)] +fn collect_supplied_workflow( + params: &FabroWorkflowVersionCreateParams, + staging: tempfile::TempDir, +) -> anyhow::Result { + // TempDir owns cleanup on every return path, including collection errors. + let root = staging.path().canonicalize()?; + for (path, contents) in ¶ms.files { + let destination = root.join(path.as_str()); + if let Some(parent) = destination.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::write(destination, contents)?; + } + let location = WorkflowLocation::from_exact_path(Path::new(params.entrypoint.as_str()), &root)?; + let closure = fabro_manifest::collect_workflow_versions_at_location( + &location, + &root, + Path::new(params.entrypoint.as_str()), + )?; + // Collection validates the whole closure, including serialized request budgets. + // A case-insensitive host must not satisfy a missing exact source key. + for (_, version) in closure.versions() { + for (path, content) in version.version().files() { + anyhow::ensure!( + params.files.get(path) == Some(content), + "collected file does not match supplied source" + ); + } + } + drop(staging); + Ok(closure) +} + +#[cfg(test)] +mod tests { + #![expect( + clippy::disallowed_methods, + reason = "hermetic temporary source fixtures" + )] + use std::path::Path; + use std::sync::{Arc, Mutex}; + + use axum::http::StatusCode; + use axum::routing::post; + use axum::{Json, Router}; + use fabro_types::WorkflowVersion; + use serde_json::json; + + use super::*; + + fn params(entrypoint: &str, files: &[(&str, &str)]) -> FabroWorkflowVersionCreateParams { + FabroWorkflowVersionCreateParams { + entrypoint: entrypoint.parse().unwrap(), + files: files + .iter() + .map(|(path, content)| (path.parse().unwrap(), (*content).to_string())) + .collect(), + } + } + + fn fixture() -> FabroWorkflowVersionCreateParams { + params("workflow.toml", &[ + ( + "workflow.toml", + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", + ), + ( + "workflow.fabro", + r#"digraph W { p [prompt="@prompt.md"] child [stack.child_workflow="child.fabro"] }"#, + ), + ( + "prompt.md", + "Keep {{ secrets.TEST }} and {{ env.TEST }} for runtime.", + ), + ("child.fabro", "digraph Child {}"), + ]) + } + + fn collect(params: &FabroWorkflowVersionCreateParams) -> CollectedWorkflowClosure { + params.validate().unwrap(); + collect_supplied_workflow(params, tempfile::tempdir().unwrap()).unwrap() + } + + #[test] + fn workflow_version_content_matches_existing_collector_and_cleans_staging() { + for input in [ + params("workflow.fabro", &[("workflow.fabro", "digraph W {}")]), + fixture(), + ] { + let source = tempfile::tempdir().unwrap(); + for (path, content) in &input.files { + std::fs::write(source.path().join(path.as_str()), content).unwrap(); + } + let expected = fabro_manifest::collect_workflow_versions( + Path::new(input.entrypoint.as_str()), + source.path(), + ) + .unwrap(); + let staging = tempfile::tempdir().unwrap(); + let path = staging.path().to_owned(); + let actual = collect_supplied_workflow(&input, staging).unwrap(); + assert!(!path.exists()); + assert_eq!(actual.root_id(), expected.root_id()); + assert_eq!( + actual + .versions() + .map(|(_, v)| v.version()) + .collect::>(), + expected + .versions() + .map(|(_, v)| v.version()) + .collect::>() + ); + } + } + + #[test] + fn workflow_version_exact_extensionless_entrypoint_and_child_ignore_selectors() { + let input = params("workflow", &[ + ( + "workflow", + r#"digraph W { child [stack.child_workflow="child"] }"#, + ), + ("child", "digraph Child {}"), + ( + ".fabro/project.toml", + "malformed project config must not be read", + ), + ( + ".fabro/workflows/workflow/workflow.toml", + "misleading named workflow", + ), + ]); + let closure = collect(&input); + let versions = closure.versions().collect::>(); + assert_eq!(versions.len(), 2); + assert_eq!(versions[1].1.version().entrypoint().as_str(), "workflow"); + assert_eq!(versions[0].1.version().entrypoint().as_str(), "child"); + } + + #[test] + fn workflow_version_rejects_missing_and_escaping_references_and_cleans_failure() { + let parent = tempfile::tempdir().unwrap(); + std::fs::write( + parent.path().join("outside.md"), + "host content must never satisfy a reference", + ) + .unwrap(); + std::fs::write(parent.path().join("child.fabro"), "digraph Host {}").unwrap(); + for (index, input) in [ + params("workflow.fabro", &[ + ("workflow.fabro", r#"digraph W { p [prompt="@prompt.md"] }"#), + ("Prompt.md", "wrong case"), + ]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [prompt="@../outside.md"] }"#, + )]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [prompt="@sub/../../outside.md"] }"#, + )]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [prompt="@outside.md"] }"#, + )]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [output_schema="@../outside.md"] }"#, + )]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="../child.fabro"] }"#, + )]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="sub/../../child.fabro"] }"#, + )]), + params("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="missing"] }"#, + )]), + params("workflow.toml", &[( + "workflow.toml", + "_version = 1\n[workflow]\ngraph = \"../child.fabro\"\n", + )]), + params("workflow.fabro", &[( + "workflow.fabro", + "invalid source PRIVATE_CONTENT", + )]), + ] + .into_iter() + .enumerate() + { + let staging = tempfile::tempdir_in(parent.path()).unwrap(); + let path = staging.path().to_owned(); + assert!( + collect_supplied_workflow(&input, staging).is_err(), + "accepted invalid fixture {index}" + ); + assert!(!path.exists()); + } + } + + #[test] + fn workflow_version_registration_preserves_literal_scripts_without_executing() { + let directory = tempfile::tempdir().unwrap(); + let marker = directory.path().join("must-not-exist"); + // Script is literal command text in Fabro, not an @file import. + let graph = format!( + "digraph W {{ command [script=\"touch {}\"] }}", + marker.display() + ); + let input = params("workflow", &[("workflow", &graph)]); + let closure = collect(&input); + let root = closure.versions().last().unwrap().1.version(); + assert_eq!(root.files()[&"workflow".parse().unwrap()], graph); + assert!(!marker.exists()); + } + + #[test] + fn workflow_version_map_order_is_irrelevant_and_reachable_changes_change_ids() { + let input = fixture(); + let first = collect(&input); + let mut reordered = input.clone(); + reordered.files = input.files.into_iter().rev().collect(); + assert_eq!(first.root_id(), collect(&reordered).root_id()); + reordered + .files + .insert("prompt.md".parse().unwrap(), "changed".into()); + let changed = collect(&reordered); + assert_ne!(first.root_id(), changed.root_id()); + assert_eq!( + first.versions().next().unwrap().0, + changed.versions().next().unwrap().0 + ); + reordered + .files + .insert("child.fabro".parse().unwrap(), "digraph Changed {}".into()); + let changed_child = collect(&reordered); + assert_ne!(changed.root_id(), changed_child.root_id()); + assert_ne!( + changed.versions().next().unwrap().0, + changed_child.versions().next().unwrap().0 + ); + } + + #[tokio::test] + async fn workflow_version_uploads_dependencies_first_and_retries_immutable_content() { + let uploads = Arc::new(Mutex::new(Vec::::new())); + let seen = uploads.clone(); + let app = Router::new().route( + "/api/v1/workflow-versions", + post(move |Json(version): Json| { + let seen = seen.clone(); + async move { + let mut seen = seen.lock().unwrap(); + for id in version.workflow_dependencies().values() { + assert!( + seen.iter().any(|prior| prior.id().unwrap() == *id), + "dependency must be registered first" + ); + } + let id = version.id().unwrap(); + seen.push(version); + (StatusCode::CREATED, Json(json!({"workflow_version_id":id}))) + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let client = + Client::new_no_proxy(&format!("http://{}", listener.local_addr().unwrap())).unwrap(); + let server = tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + let expected = collect(&fixture()).root_id(); + for _ in 0..2 { + let id = ServerWorkflowVersionCreateAdapter + .create_workflow_version(fixture(), &client) + .await + .unwrap(); + assert_eq!(id, expected); + } + assert_eq!(uploads.lock().unwrap().len(), 4); + server.abort(); + assert!(server.await.unwrap_err().is_cancelled()); + } + + #[tokio::test] + async fn workflow_version_invalid_closure_has_no_uploads_or_source_in_errors() { + let server = httpmock::MockServer::start_async().await; + let upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST); + then.status(500); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + let mut invalid = fixture(); + // Child is valid, but the root fails after its dependency is assembled. + invalid.files.insert("workflow.toml".parse().unwrap(), "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.goal]\nfile = \"missing.md\"".into()); + let mut oversized = fixture(); + let huge_prompt = "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1); + oversized + .files + .insert("prompt.md".parse().unwrap(), huge_prompt); + for input in [ + invalid, + oversized, + params("workflow", &[( + "workflow", + "PRIVATE_CONTENT invalid source", + )]), + ] { + let error = ServerWorkflowVersionCreateAdapter + .create_workflow_version(input, &client) + .await + .unwrap_err(); + assert!(!format!("{error:#}").contains("PRIVATE_CONTENT")); + } + upload.assert_calls_async(0).await; + } + + #[tokio::test] + async fn workflow_version_root_upload_failure_leaves_child_for_safe_retry() { + let server = httpmock::MockServer::start_async().await; + let closure = collect(&fixture()); + let child = closure.versions().next().unwrap().1.version(); + let root = closure.versions().last().unwrap().1.version(); + let child_upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(child); + then.status(201) + .json_body(json!({"workflow_version_id":child.id().unwrap()})); + }) + .await; + let failed_root = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(root); + then.status(400); + }) + .await; + let deletion = server + .mock_async(|when, then| { + when.method(httpmock::Method::DELETE); + then.status(500); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + assert!( + ServerWorkflowVersionCreateAdapter + .create_workflow_version(fixture(), &client) + .await + .is_err() + ); + child_upload.assert_calls_async(1).await; + failed_root.assert_calls_async(1).await; + failed_root.delete_async().await; + let root_upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(root); + then.status(201) + .json_body(json!({"workflow_version_id":root.id().unwrap()})); + }) + .await; + assert_eq!( + ServerWorkflowVersionCreateAdapter + .create_workflow_version(fixture(), &client) + .await + .unwrap(), + closure.root_id() + ); + child_upload.assert_calls_async(2).await; + root_upload.assert_calls_async(1).await; + deletion.assert_calls_async(0).await; + } + + #[tokio::test] + async fn workflow_version_failed_upload_and_wrong_server_id_never_return_success() { + for wrong_id in [false, true] { + let server = httpmock::MockServer::start_async().await; + let closure = collect(&fixture()); + let child = closure.versions().next().unwrap().1.version(); + let upload = server.mock_async(|when, then| { + when.method(httpmock::Method::POST).path("/api/v1/workflow-versions").json_body_obj(child); + if wrong_id { + then.status(201).json_body(json!({"workflow_version_id": WorkflowVersionId::from(fabro_types::BlobHash::new(b"wrong"))})); + } else { then.status(400); } + }).await; + let root = closure.versions().last().unwrap().1.version(); + let root_upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(root); + then.status(201) + .json_body(json!({"workflow_version_id":root.id().unwrap()})); + }) + .await; + let client = Client::new_no_proxy(&server.url("")).unwrap(); + assert!( + ServerWorkflowVersionCreateAdapter + .create_workflow_version(fixture(), &client) + .await + .is_err() + ); + upload.assert_calls_async(1).await; + root_upload.assert_calls_async(0).await; + } + } +} diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index db317abb1..a3f36c3e4 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -37,6 +37,7 @@ pub use crate::local_workflow_package::{ use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ CollectedWorkflowClosure, WorkflowVersionCollectError, collect_workflow_versions, + collect_workflow_versions_at_location, }; #[derive(Debug, Default)] diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index e01eb629d..4aaa989f5 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -145,17 +145,27 @@ impl<'a> WorkflowBundler<'a> { /// `~` rejected) before resolution, so the file read matches the manifest /// key. Returns the collected workflow's manifest key. fn collect_workflow_entry(&mut self, workflow: &Path, resolve_from: &Path) -> Result { - let normalized_workflow = if workflow.extension().is_some() && workflow.is_relative() { - normalize_absolute_path(resolve_from, &workflow.to_string_lossy()).ok_or_else(|| { - anyhow!( - "unsupported manifest workflow reference: {}", - workflow.display() - ) - })? + let location = if self.workflow_version_projection { + let exact = normalize_absolute_path(resolve_from, &workflow.to_string_lossy()) + .ok_or_else(|| anyhow!("unsupported workflow reference"))?; + // Check containment before location resolution can read a config. + manifest_path_from_absolute(&exact, self.package_root)?; + WorkflowLocation::from_exact_path(&exact, self.package_root)? } else { - workflow.to_path_buf() + let normalized_workflow = if workflow.extension().is_some() && workflow.is_relative() { + normalize_absolute_path(resolve_from, &workflow.to_string_lossy()).ok_or_else( + || { + anyhow!( + "unsupported manifest workflow reference: {}", + workflow.display() + ) + }, + )? + } else { + workflow.to_path_buf() + }; + WorkflowLocation::resolve(&normalized_workflow, resolve_from)? }; - let location = WorkflowLocation::resolve(&normalized_workflow, resolve_from)?; self.collect_workflow_location(&location) } diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index a0fb815b6..8928cfd81 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -137,7 +137,9 @@ pub(super) fn canonicalize_location( }) } -pub(super) fn collect_workflow_versions_at_location( +/// Collect an already resolved exact location inside a canonical package root. +/// Every dependency is validated before this returns; no registration occurs. +pub fn collect_workflow_versions_at_location( location: &WorkflowLocation, package_root: &Path, workflow: &Path, @@ -263,6 +265,12 @@ fn workflow_files( for (path, file) in workflow.files { insert_file(&mut files, entrypoint, workflow_path(&path)?, file.content)?; } + fabro_types::validate_workflow_source_paths(files.keys()).map_err(|source| { + WorkflowVersionCollectError::InvalidShape { + entrypoint: entrypoint.clone(), + source, + } + })?; Ok(files) } diff --git a/lib/components/fabro-tool/src/common.rs b/lib/components/fabro-tool/src/common.rs index 159ec426d..f88249fa2 100644 --- a/lib/components/fabro-tool/src/common.rs +++ b/lib/components/fabro-tool/src/common.rs @@ -48,6 +48,13 @@ pub type ToolResult = Result; #[async_trait] pub trait FabroToolBackend: Send + Sync { + async fn create_workflow_version( + &self, + _params: crate::FabroWorkflowVersionCreateParams, + ) -> anyhow::Result { + anyhow::bail!("fabro_workflow_version_create is not available") + } + async fn create_run_from_spec( &self, spec: &crate::ValidatedCreateRunSpec, @@ -170,6 +177,7 @@ pub struct ToolDefinition { pub parameters: Value, } +pub const FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME: &str = "fabro_workflow_version_create"; pub const FABRO_RUN_CREATE_TOOL_NAME: &str = "fabro_run_create"; pub const FABRO_RUN_SEARCH_TOOL_NAME: &str = "fabro_run_search"; pub const FABRO_RUN_GET_TOOL_NAME: &str = "fabro_run_get"; @@ -180,6 +188,10 @@ pub const FABRO_RUN_PAIR_TOOL_NAME: &str = "fabro_run_pair"; static TOOL_DEFINITIONS: LazyLock> = LazyLock::new(|| { vec![ + tool_definition::( + FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME, + "Register supplied workflow file contents and all local dependencies as a reusable immutable workflow version ID. Obtain files with shell/read tools first; this does not create or start a run.", + ), tool_definition::( FABRO_RUN_CREATE_TOOL_NAME, "Create one or more Fabro workflow runs, optionally under a parent run, starting them by default.", @@ -323,6 +335,7 @@ mod tests { #[test] fn shared_tool_definitions_include_run_management_catalog() { assert_eq!(shared_tool_names(), vec![ + FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME, FABRO_RUN_CREATE_TOOL_NAME, FABRO_RUN_SEARCH_TOOL_NAME, FABRO_RUN_GET_TOOL_NAME, @@ -333,6 +346,21 @@ mod tests { ]); } + #[test] + fn workflow_version_create_has_strict_content_schema() { + let definition = tool_definitions() + .iter() + .find(|definition| definition.name == "fabro_workflow_version_create") + .expect("workflow version creation should be in the shared catalog"); + let schema = &definition.parameters; + assert_eq!(schema["additionalProperties"], false); + assert_eq!(schema["properties"].as_object().unwrap().len(), 2); + assert_eq!( + schema["required"], + serde_json::json!(["entrypoint", "files"]) + ); + } + #[test] fn pair_tool_definition_exposes_pair_schema() { let definition = tool_definitions() diff --git a/lib/components/fabro-tool/src/fabro_client.rs b/lib/components/fabro-tool/src/fabro_client.rs index 59b935a85..eb5599c5e 100644 --- a/lib/components/fabro-tool/src/fabro_client.rs +++ b/lib/components/fabro-tool/src/fabro_client.rs @@ -12,9 +12,10 @@ use crate::{FabroToolBackend, RunManifestBuilder, ToolError}; #[derive(Clone)] pub struct ClientBackend { - client: Arc<::fabro_client::Client>, + client: Arc<::fabro_client::Client>, manifest_builder: Option>, - run_scope: Option, + run_scope: Option, + workflow_version_create_adapter: Option>, } impl ClientBackend { @@ -24,6 +25,7 @@ impl ClientBackend { client, manifest_builder: None, run_scope: None, + workflow_version_create_adapter: None, } } @@ -33,6 +35,15 @@ impl ClientBackend { self } + #[must_use] + pub fn with_workflow_version_create_adapter( + mut self, + adapter: Arc, + ) -> Self { + self.workflow_version_create_adapter = Some(adapter); + self + } + /// Restrict this backend to a single run. /// /// Ask Fabro sessions use this with a same-run worker token so accidental @@ -55,6 +66,21 @@ impl ClientBackend { #[async_trait] impl FabroToolBackend for ClientBackend { + async fn create_workflow_version( + &self, + params: crate::FabroWorkflowVersionCreateParams, + ) -> anyhow::Result { + anyhow::ensure!( + self.run_scope.is_none(), + "workflow version creation is outside this tool session's run scope" + ); + let adapter = self + .workflow_version_create_adapter + .as_ref() + .ok_or_else(|| anyhow::anyhow!("fabro_workflow_version_create is not available"))?; + adapter.create_workflow_version(params, &self.client).await + } + async fn create_run_from_spec( &self, spec: &crate::ValidatedCreateRunSpec, diff --git a/lib/components/fabro-tool/src/lib.rs b/lib/components/fabro-tool/src/lib.rs index e854c26e3..64ac87f27 100644 --- a/lib/components/fabro-tool/src/lib.rs +++ b/lib/components/fabro-tool/src/lib.rs @@ -14,12 +14,13 @@ mod interact; mod manifest; mod pair; mod search; +mod workflow_version; pub use common::{ FABRO_RUN_CREATE_TOOL_NAME, FABRO_RUN_EVENTS_TOOL_NAME, FABRO_RUN_GATHER_TOOL_NAME, FABRO_RUN_GET_TOOL_NAME, FABRO_RUN_INTERACT_TOOL_NAME, FABRO_RUN_PAIR_TOOL_NAME, - FABRO_RUN_SEARCH_TOOL_NAME, FabroToolBackend, RunManifestBuilder, RunSummaryResult, - ToolDefinition, ToolError, ToolResult, tool_definitions, + FABRO_RUN_SEARCH_TOOL_NAME, FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME, FabroToolBackend, + RunManifestBuilder, RunSummaryResult, ToolDefinition, ToolError, ToolResult, tool_definitions, }; pub use create::{ CreateRunOptions, CreateRunSpec, CreateRunSpecInput, CreateRunsResult, CreatedRunResult, @@ -47,3 +48,7 @@ pub use search::{ FabroRunSearchParams, SearchRunSummaryResult, SearchRunsResult, ValidatedSearchRuns, search_runs, search_runs_text, }; +pub use workflow_version::{ + FabroWorkflowVersionCreateParams, WorkflowVersionCreateAdapter, create_workflow_version, + workflow_version_create_text, +}; diff --git a/lib/components/fabro-tool/src/workflow_version.rs b/lib/components/fabro-tool/src/workflow_version.rs new file mode 100644 index 000000000..11567df8a --- /dev/null +++ b/lib/components/fabro-tool/src/workflow_version.rs @@ -0,0 +1,234 @@ +use std::collections::BTreeMap; +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_api::types::CreateWorkflowVersionResponse; +use fabro_types::{ + MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES, + WorkflowPath, WorkflowVersionId, +}; +use schemars::JsonSchema; +use serde::de::{Error as _, MapAccess, Visitor}; +use serde::{Deserialize, Serialize}; + +use crate::{FabroToolBackend, ToolError, ToolResult}; + +/// Caller-supplied source content, before packaging resolves workflow +/// dependencies. +#[derive(Clone, Deserialize, Serialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub struct FabroWorkflowVersionCreateParams { + /// Exact package-relative key of the graph or workflow configuration file. + #[schemars(with = "String")] + pub entrypoint: WorkflowPath, + /// All local dependencies, keyed by package-relative path. Values are text + /// contents. + #[serde(deserialize_with = "deserialize_files")] + #[schemars(with = "BTreeMap")] + pub files: BTreeMap, +} + +fn deserialize_files<'de, D: serde::Deserializer<'de>>( + deserializer: D, +) -> Result, D::Error> { + struct FilesVisitor; + impl<'de> Visitor<'de> for FilesVisitor { + type Value = BTreeMap; + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("workflow files with unique path keys and text contents") + } + fn visit_map>(self, mut map: A) -> Result { + let mut files = BTreeMap::new(); + while let Some((path, content)) = map.next_entry()? { + if files.insert(path, content).is_some() { + return Err(A::Error::custom("duplicate workflow file key")); + } + } + Ok(files) + } + } + deserializer.deserialize_map(FilesVisitor) +} + +impl FabroWorkflowVersionCreateParams { + /// Validate the complete supplied tree before any filesystem writes. + pub fn validate(&self) -> ToolResult<()> { + if !self.files.contains_key(&self.entrypoint) { + return Err(ToolError::message( + "entrypoint must be an exact supplied file key", + )); + } + if self.files.len() > MAX_WORKFLOW_VERSION_FILES { + return Err(ToolError::message("workflow source exceeds 512 files")); + } + let mut total = 0; + for content in self.files.values() { + if content.len() > MAX_WORKFLOW_VERSION_FILE_BYTES { + return Err(ToolError::message("workflow source file exceeds 512 KiB")); + } + total += content.len(); + } + if total > MAX_WORKFLOW_VERSION_BYTES { + return Err(ToolError::message("workflow source exceeds 2 MiB")); + } + fabro_types::validate_workflow_source_paths(self.files.keys()) + .map_err(|_| ToolError::message("workflow source paths collide"))?; + Ok(()) + } +} + +/// Application seam for packaging and registering content without a dependency +/// cycle. Implementations must validate before staging, confine reads to +/// supplied files, validate the entire closure before uploading, and register +/// dependencies first. +#[async_trait] +pub trait WorkflowVersionCreateAdapter: Send + Sync { + async fn create_workflow_version( + &self, + params: FabroWorkflowVersionCreateParams, + client: &fabro_client::Client, + ) -> anyhow::Result; +} + +pub async fn create_workflow_version( + backend: Arc, + params: FabroWorkflowVersionCreateParams, +) -> ToolResult { + params.validate()?; + let workflow_version_id = backend + .create_workflow_version(params) + .await + .map_err(|err| ToolError::from_anyhow(&err))?; + Ok(CreateWorkflowVersionResponse { + workflow_version_id, + }) +} + +#[must_use] +pub fn workflow_version_create_text(result: &CreateWorkflowVersionResponse) -> String { + serde_json::to_string(result).expect("workflow version response should serialize") +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + use crate::fabro_client::ClientBackend; + + #[test] + fn workflow_version_request_rejects_unknown_fields_and_invalid_paths() { + let valid = json!({"entrypoint": "workflow", "files": {"workflow": "digraph W {}"}}); + let params: FabroWorkflowVersionCreateParams = + serde_json::from_value(valid.clone()).unwrap(); + params.validate().unwrap(); + assert!( + serde_json::from_str::( + r#"{"entrypoint":"workflow","files":{"workflow":"a","workflow":"b"}}"# + ) + .is_err() + ); + for field in [ + "cwd", + "url", + "workflow", + "environment", + "parent_id", + "workflow_dependencies", + ] { + let mut value = valid.clone(); + value[field] = json!("unexpected"); + assert!(serde_json::from_value::(value).is_err()); + } + for path in [ + "../workflow", + "/workflow", + "a/../workflow", + "a//b", + "a\\b", + "~/workflow", + "", + ] { + for value in [ + json!({"entrypoint":path,"files":{"workflow":"x"}}), + json!({"entrypoint":"workflow","files":{path:"x"}}), + ] { + assert!(serde_json::from_value::(value).is_err()); + } + } + } + + #[test] + fn workflow_version_source_enforces_presence_collisions_and_budgets() { + for files in [ + json!({}), + json!({"A":"x","a":"y"}), + json!({"A":"x","a/b.md":"y"}), + json!({"a":"x","A/b.md":"y"}), + ] { + let mut files = files.as_object().unwrap().clone(); + files.insert("workflow".into(), json!("digraph W {}")); + let mut params: FabroWorkflowVersionCreateParams = + serde_json::from_value(json!({"entrypoint":"workflow","files":files})).unwrap(); + if params.files.len() == 1 { + params.entrypoint = "missing".parse().unwrap(); + } + assert!(params.validate().is_err()); + } + let mut params = FabroWorkflowVersionCreateParams { + entrypoint: "workflow".parse().unwrap(), + files: BTreeMap::from([( + "workflow".parse().unwrap(), + "x".repeat(MAX_WORKFLOW_VERSION_FILE_BYTES + 1), + )]), + }; + assert!(params.validate().is_err()); + params.files = (0..MAX_WORKFLOW_VERSION_FILES) + .map(|i| (format!("file{i}").parse().unwrap(), String::new())) + .collect(); + params + .files + .insert(params.entrypoint.clone(), String::new()); + assert!(params.validate().is_err()); + params.files = (0..5) + .map(|i| { + ( + format!("file{i}").parse().unwrap(), + "x".repeat(MAX_WORKFLOW_VERSION_FILE_BYTES), + ) + }) + .collect(); + params + .files + .insert(params.entrypoint.clone(), String::new()); + assert!(params.validate().is_err()); + } + + #[tokio::test] + async fn workflow_version_same_run_backend_denies_before_adapter() { + let client = fabro_client::Client::new_no_proxy("http://127.0.0.1:1").unwrap(); + let backend = ClientBackend::new(Arc::new(client)) + .with_workflow_version_create_adapter(Arc::new(UnreachableAdapter)) + .with_run_scope("01KRBZW4DW0000000000000002".parse().unwrap()); + let params = serde_json::from_value( + json!({"entrypoint":"workflow","files":{"workflow":"digraph W {}"}}), + ) + .unwrap(); + let error = create_workflow_version(Arc::new(backend), params) + .await + .unwrap_err(); + assert!(error.as_str().contains("run scope")); + } + + struct UnreachableAdapter; + #[async_trait] + impl WorkflowVersionCreateAdapter for UnreachableAdapter { + async fn create_workflow_version( + &self, + _: FabroWorkflowVersionCreateParams, + _: &fabro_client::Client, + ) -> anyhow::Result { + panic!("scoped backend must not invoke the adapter") + } + } +} diff --git a/lib/foundation/fabro-config/src/project.rs b/lib/foundation/fabro-config/src/project.rs index 4fc58e61e..c33f3fc79 100644 --- a/lib/foundation/fabro-config/src/project.rs +++ b/lib/foundation/fabro-config/src/project.rs @@ -54,6 +54,21 @@ impl WorkflowLocation { } } + /// Resolve an exact file path without workflow-name or ambient config + /// lookup. Relative paths are interpreted against the supplied + /// directory only. + pub fn from_exact_path(path: &Path, directory: &Path) -> Result { + let path = directory.join(path); + if path + .extension() + .is_some_and(|extension| extension == "toml") + { + Self::from_toml(path) + } else { + Ok(Self::from_graph(path)) + } + } + fn from_toml(toml_path: PathBuf) -> Result { let cfg = match run::load_run_config(&toml_path) { Ok(cfg) => cfg, diff --git a/lib/foundation/fabro-types/Cargo.toml b/lib/foundation/fabro-types/Cargo.toml index 2e777317b..005fe228f 100644 --- a/lib/foundation/fabro-types/Cargo.toml +++ b/lib/foundation/fabro-types/Cargo.toml @@ -33,6 +33,8 @@ strum.workspace = true thiserror.workspace = true toml.workspace = true ulid.workspace = true +unicase = "2" +unicode-normalization = "0.1" url.workspace = true [dev-dependencies] diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index b4b26fe74..e678a8a1f 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -198,5 +198,6 @@ pub use workflow_path::{ pub use workflow_version::{ MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_DEPENDENCIES, MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, + validate_workflow_source_paths, }; pub use workflow_version_id::{WorkflowVersionId, WorkflowVersionIdParseError}; diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index 125544aa3..996a26444 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -5,6 +5,8 @@ use std::marker::PhantomData; use serde::de::{Error as _, MapAccess, Visitor}; use serde::{Deserialize, Deserializer, Serialize}; use thiserror::Error; +use unicase::UniCase; +use unicode_normalization::UnicodeNormalization as _; use crate::{BlobHash, WorkflowPath, WorkflowVersionId}; @@ -149,31 +151,52 @@ impl WorkflowVersion { } fn validate_path_collisions(&self) -> Result<(), WorkflowVersionShapeError> { - // Keys are unique within each map, so equality can only collide - // across files and workflow dependencies. - let mut by_text = - HashMap::with_capacity(self.files.len() + self.workflow_dependencies.len()); - for path in self.files.keys().chain(self.workflow_dependencies.keys()) { - if let Some(existing) = by_text.insert(path.as_str(), path) { + validate_path_collisions( + self.files.keys().chain(self.workflow_dependencies.keys()), + false, + ) + } +} + +/// Reject file and directory aliases before materializing a portable source +/// tree, including Unicode case folding and normalization. Canonical versions +/// themselves retain their exact, case-sensitive +/// semantics. +pub fn validate_workflow_source_paths<'a>( + paths: impl IntoIterator, +) -> Result<(), WorkflowVersionShapeError> { + validate_path_collisions(paths, true) +} + +fn validate_path_collisions<'a>( + paths: impl IntoIterator, + case_insensitive: bool, +) -> Result<(), WorkflowVersionShapeError> { + let mut by_text = HashMap::new(); + for path in paths { + let text = if case_insensitive { + UniCase::new(path.as_str()).to_folded_case().nfc().collect() + } else { + path.as_str().to_owned() + }; + if let Some(existing) = by_text.insert(text, path) { + return Err(WorkflowVersionShapeError::PathCollision { + first: existing.clone(), + second: path.clone(), + }); + } + } + for (text, path) in &by_text { + for (index, _) in text.match_indices('/') { + if let Some(ancestor) = by_text.get(&text[..index]) { return Err(WorkflowVersionShapeError::PathCollision { - first: existing.clone(), - second: path.clone(), + first: (*ancestor).clone(), + second: (*path).clone(), }); } } - for path in self.files.keys().chain(self.workflow_dependencies.keys()) { - let text = path.as_str(); - for (index, _) in text.match_indices('/') { - if let Some(ancestor) = by_text.get(&text[..index]) { - return Err(WorkflowVersionShapeError::PathCollision { - first: (*ancestor).clone(), - second: path.clone(), - }); - } - } - } - Ok(()) } + Ok(()) } impl<'de> Deserialize<'de> for WorkflowVersion { @@ -509,3 +532,37 @@ mod tests { assert!(serde_json::from_str::(duplicate).is_err()); } } + +#[cfg(test)] +mod source_path_tests { + use super::*; + + #[test] + fn workflow_source_collisions_are_portable_in_both_orders() { + for pair in [ + ["A", "a"], + ["A", "a/b.md"], + ["a", "A/b.md"], + ["é", "É/b"], + ["ΟΣ", "οσ/b"], + ["é", "e\u{301}/b"], + ["Straße", "STRASSE/b"], + ] { + let paths = pair.map(|path| WorkflowPath::new(path).unwrap()); + assert!(validate_workflow_source_paths(paths.iter()).is_err()); + assert!(validate_workflow_source_paths(paths.iter().rev()).is_err()); + } + let version = WorkflowVersion::new( + WorkflowPath::new("A").unwrap(), + BTreeMap::from([ + (WorkflowPath::new("A").unwrap(), "x".into()), + (WorkflowPath::new("a").unwrap(), "y".into()), + ]), + BTreeMap::new(), + ); + assert!( + version.is_ok(), + "canonical versions keep exact path semantics" + ); + } +} diff --git a/lib/packages/fabro-api-client/src/api/workflow-versions-api.ts b/lib/packages/fabro-api-client/src/api/workflow-versions-api.ts index 4285c1849..14173fac5 100644 --- a/lib/packages/fabro-api-client/src/api/workflow-versions-api.ts +++ b/lib/packages/fabro-api-client/src/api/workflow-versions-api.ts @@ -33,7 +33,7 @@ import type { WorkflowVersion } from '../models'; export const WorkflowVersionsApiAxiosParamCreator = function (configuration?: Configuration) { return { /** - * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. + * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. Requires an authenticated user or a worker token with the `agent:run_tools` capability. Ordinary worker tokens cannot register versions. Registration creates no run and starts no execution. * @summary Create Workflow Version * @param {WorkflowVersion} workflowVersion * @param {*} [options] Override http request option. @@ -83,7 +83,7 @@ export const WorkflowVersionsApiFp = function(configuration?: Configuration) { const localVarAxiosParamCreator = WorkflowVersionsApiAxiosParamCreator(configuration) return { /** - * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. + * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. Requires an authenticated user or a worker token with the `agent:run_tools` capability. Ordinary worker tokens cannot register versions. Registration creates no run and starts no execution. * @summary Create Workflow Version * @param {WorkflowVersion} workflowVersion * @param {*} [options] Override http request option. @@ -105,7 +105,7 @@ export const WorkflowVersionsApiFactory = function (configuration?: Configuratio const localVarFp = WorkflowVersionsApiFp(configuration) return { /** - * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. + * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. Requires an authenticated user or a worker token with the `agent:run_tools` capability. Ordinary worker tokens cannot register versions. Registration creates no run and starts no execution. * @summary Create Workflow Version * @param {WorkflowVersion} workflowVersion * @param {*} [options] Override http request option. @@ -122,7 +122,7 @@ export const WorkflowVersionsApiFactory = function (configuration?: Configuratio */ export class WorkflowVersionsApi extends BaseAPI { /** - * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. + * Validates and stores an immutable workflow package in content-addressed storage. Repeating the same canonical content returns the same identifier. Requires an authenticated user or a worker token with the `agent:run_tools` capability. Ordinary worker tokens cannot register versions. Registration creates no run and starts no execution. * @summary Create Workflow Version * @param {WorkflowVersion} workflowVersion * @param {*} [options] Override http request option. From bafdd880f5880d7596637c0962e9dd1a0c5ffd80 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 14:23:45 -0600 Subject: [PATCH 02/15] Simplify workflow version registration tool layering Move supplied-content packaging into fabro-manifest beside the checkout collector, and narrow the injected seam to a packager that returns the dependency-ordered closure so ClientBackend registers versions with the client it already owns. Validate the tool input once through a ValidatedWorkflowVersionCreate newtype, matching the other tools, instead of re-validating at three layers. Reuse the fabro-types unique-map deserializer and the shared "not available" error helper, derive budget messages from the limit constants, and render the tool result through the shared summary+JSON path used by sibling tools. Share one extension dispatch between WorkflowLocation::resolve and from_exact_path, compute the bundler's normalized reference once, key path-collision checks by a Cow so the canonical exact check no longer allocates, and log the full packaging error chain before returning the curated tool message. Replace the hand-rolled axum test server with httpmock and declare the new unicode dependencies at the workspace. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 1 + Cargo.toml | 2 + lib/apps/fabro-cli/src/commands/run/runner.rs | 4 +- lib/apps/fabro-mcp-server/src/server.rs | 22 +- lib/apps/fabro-server/src/server/tests.rs | 6 +- .../fabro-server/src/workflow_version_tool.rs | 484 +++--------------- lib/components/fabro-manifest/Cargo.toml | 2 +- lib/components/fabro-manifest/src/lib.rs | 2 + .../fabro-manifest/src/supplied_workflow.rs | 276 ++++++++++ .../fabro-manifest/src/workflow_bundler.rs | 40 +- lib/components/fabro-tool/Cargo.toml | 1 + lib/components/fabro-tool/src/common.rs | 13 +- lib/components/fabro-tool/src/fabro_client.rs | 157 +++++- lib/components/fabro-tool/src/lib.rs | 4 +- .../fabro-tool/src/workflow_version.rs | 185 +++---- lib/foundation/fabro-config/src/project.rs | 24 +- lib/foundation/fabro-types/Cargo.toml | 4 +- lib/foundation/fabro-types/src/lib.rs | 2 +- .../fabro-types/src/workflow_version.rs | 36 +- 19 files changed, 693 insertions(+), 572 deletions(-) create mode 100644 lib/components/fabro-manifest/src/supplied_workflow.rs diff --git a/Cargo.lock b/Cargo.lock index b7c953564..8923ac91a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3169,6 +3169,7 @@ dependencies = [ "fabro-types", "fabro-util", "futures", + "httpmock", "schemars 1.2.1", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index 8bbbfb212..20d1ecf54 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -65,6 +65,8 @@ console = "0.15" dialoguer = "0.12" git2 = { version = "0.20", default-features = false, features = ["vendored-libgit2", "vendored-openssl", "https"] } tracing = "0.1" +unicase = "2" +unicode-normalization = "0.1" tracing-subscriber = { version = "0.3", features = ["fmt", "env-filter"] } tracing-appender = "0.2" rmcp = { version = "1.4", default-features = false } diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index df7b2f531..36f149e3a 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -16,7 +16,7 @@ use fabro_interview::{ WorkerControlMessage, }; use fabro_server::run_tool_manifest; -use fabro_server::workflow_version_tool::ServerWorkflowVersionCreateAdapter; +use fabro_server::workflow_version_tool::ServerWorkflowVersionPackager; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_tool::fabro_client::ClientBackend; use fabro_types::settings::run::{RunMode, RunNamespace}; @@ -238,7 +238,7 @@ fn build_fabro_run_tool_services( } let backend = ClientBackend::new(Arc::new(client)) .with_manifest_builder(Arc::new(WorkerRunManifestBuilder)) - .with_workflow_version_create_adapter(Arc::new(ServerWorkflowVersionCreateAdapter)); + .with_workflow_version_packager(Arc::new(ServerWorkflowVersionPackager)); Some(FabroRunToolServices { backend: Arc::new(backend), current_run_id, diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index 794da973a..914833318 100644 --- a/lib/apps/fabro-mcp-server/src/server.rs +++ b/lib/apps/fabro-mcp-server/src/server.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Result; -use fabro_server::workflow_version_tool::ServerWorkflowVersionCreateAdapter; +use fabro_server::workflow_version_tool::ServerWorkflowVersionPackager; use fabro_tool::fabro_client::ClientBackend; use fabro_tool::{self as run_tools, FabroToolBackend}; use fabro_util::version::FABRO_VERSION; @@ -99,14 +99,15 @@ impl FabroMcpServer { &self, params: Parameters, ) -> Result { - if let Err(err) = params.0.validate() { - return Ok(error_result(&err)); - } + let source = match run_tools::ValidatedWorkflowVersionCreate::try_from(params.0) { + Ok(source) => source, + Err(err) => return Ok(error_result(&err)), + }; let backend = match self.backend().await { Ok(backend) => backend, Err(err) => return Ok(error_result(&err)), }; - match run_tools::create_workflow_version(backend, params.0).await { + match run_tools::create_workflow_version(backend, source).await { Ok(result) => success_result(&result, run_tools::workflow_version_create_text(&result)), Err(err) => Ok(error_result(&err)), } @@ -275,8 +276,8 @@ impl FabroMcpServer { Arc::new( ClientBackend::new(Arc::new(client)) .with_manifest_builder(Arc::new(McpRunManifestBuilder)) - .with_workflow_version_create_adapter(Arc::new( - ServerWorkflowVersionCreateAdapter, + .with_workflow_version_packager(Arc::new( + ServerWorkflowVersionPackager, )), ) as Arc }) @@ -360,7 +361,12 @@ mod tests { actual.as_object_mut().unwrap().remove("$schema"); assert_eq!(actual, expected); assert_eq!(tool.description.as_deref(), Some(definition.description)); - let result = server.fabro_workflow_version_create(Parameters(serde_json::from_value(serde_json::json!({"entrypoint":"workflow","files":{"workflow":"digraph W {}"}})).unwrap())).await.unwrap(); + let params = + serde_json::json!({"entrypoint":"workflow","files":{"workflow":"digraph W {}"}}); + let result = server + .fabro_workflow_version_create(Parameters(serde_json::from_value(params).unwrap())) + .await + .unwrap(); assert_eq!( result.structured_content, Some(serde_json::json!({"workflow_version_id":id})) diff --git a/lib/apps/fabro-server/src/server/tests.rs b/lib/apps/fabro-server/src/server/tests.rs index e81a3b576..a0a4533f6 100644 --- a/lib/apps/fabro-server/src/server/tests.rs +++ b/lib/apps/fabro-server/src/server/tests.rs @@ -19743,7 +19743,11 @@ fn validate_github_slug_rejects_overlong() { async fn workflow_version_registration_requires_user_or_run_tools_capability() { let (state, app) = jwt_auth_app(); let run_id = RunId::new(); - let body = json!({"entrypoint":"workflow.fabro","files":{"workflow.fabro":"digraph W {}"},"workflow_dependencies":{}}); + let body = json!({ + "entrypoint": "workflow.fabro", + "files": {"workflow.fabro": "digraph W {}"}, + "workflow_dependencies": {}, + }); for (token, expected) in [ (issue_test_user_jwt(), StatusCode::CREATED), ( diff --git a/lib/apps/fabro-server/src/workflow_version_tool.rs b/lib/apps/fabro-server/src/workflow_version_tool.rs index 27081eea0..816c526b5 100644 --- a/lib/apps/fabro-server/src/workflow_version_tool.rs +++ b/lib/apps/fabro-server/src/workflow_version_tool.rs @@ -1,99 +1,50 @@ -use std::path::Path; - -use anyhow::Context as _; use async_trait::async_trait; -use fabro_client::Client; -use fabro_config::project::WorkflowLocation; -use fabro_manifest::CollectedWorkflowClosure; -use fabro_tool::{FabroWorkflowVersionCreateParams, ToolError, WorkflowVersionCreateAdapter}; -use fabro_types::WorkflowVersionId; +use fabro_tool::{ + PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, +}; use tokio::task; +use tracing::warn; -/// Content-only registration shared by standalone MCP and capable run workers. -pub struct ServerWorkflowVersionCreateAdapter; +/// Packages supplied workflow contents for standalone MCP and capable run +/// workers; the backend that owns the API client performs registration. +pub struct ServerWorkflowVersionPackager; + +const PACKAGING_FAILED: &str = "workflow source could not be packaged; check configuration, \ + syntax, local references, and package limits"; #[async_trait] -impl WorkflowVersionCreateAdapter for ServerWorkflowVersionCreateAdapter { - async fn create_workflow_version( +impl WorkflowVersionPackager for ServerWorkflowVersionPackager { + async fn package( &self, - params: FabroWorkflowVersionCreateParams, - client: &Client, - ) -> anyhow::Result { - params.validate()?; + source: ValidatedWorkflowVersionCreate, + ) -> anyhow::Result { let closure = task::spawn_blocking(move || { - let staging = tempfile::Builder::new().prefix("fabro-workflow-version-").tempdir()?; - collect_supplied_workflow(¶ms, staging) + fabro_manifest::collect_supplied_workflow_versions(&source.entrypoint, &source.files) + }) + .await + .map_err(|err| anyhow::anyhow!("workflow packaging task failed: {err}"))? + .map_err(|err| { + // Parser diagnostics may quote supplied source, so the chain stays + // in the log and only a generic message crosses the tool boundary. + warn!(error = %format!("{err:#}"), "workflow version packaging failed"); + ToolError::message(PACKAGING_FAILED) + })?; + Ok(PackagedWorkflowVersions { + root_id: closure.root_id(), + versions: closure + .versions() + .map(|(_, version)| version.version().clone()) + .collect(), }) - .await - .context("workflow packaging task failed")? - // Parser diagnostics may contain supplied source. Keep them off the - // tool result boundary, including nested error chains. - .map_err(|_| ToolError::message("workflow source could not be packaged; check configuration, syntax, local references, and package limits"))?; - let versions = closure - .versions() - .map(|(_, version)| version.version()) - .collect::>(); - client.register_workflow_versions(versions).await?; - Ok(closure.root_id()) } } -#[expect( - clippy::disallowed_methods, - reason = "bounded file staging and collection run on spawn_blocking" -)] -fn collect_supplied_workflow( - params: &FabroWorkflowVersionCreateParams, - staging: tempfile::TempDir, -) -> anyhow::Result { - // TempDir owns cleanup on every return path, including collection errors. - let root = staging.path().canonicalize()?; - for (path, contents) in ¶ms.files { - let destination = root.join(path.as_str()); - if let Some(parent) = destination.parent() { - std::fs::create_dir_all(parent)?; - } - std::fs::write(destination, contents)?; - } - let location = WorkflowLocation::from_exact_path(Path::new(params.entrypoint.as_str()), &root)?; - let closure = fabro_manifest::collect_workflow_versions_at_location( - &location, - &root, - Path::new(params.entrypoint.as_str()), - )?; - // Collection validates the whole closure, including serialized request budgets. - // A case-insensitive host must not satisfy a missing exact source key. - for (_, version) in closure.versions() { - for (path, content) in version.version().files() { - anyhow::ensure!( - params.files.get(path) == Some(content), - "collected file does not match supplied source" - ); - } - } - drop(staging); - Ok(closure) -} - #[cfg(test)] mod tests { - #![expect( - clippy::disallowed_methods, - reason = "hermetic temporary source fixtures" - )] - use std::path::Path; - use std::sync::{Arc, Mutex}; - - use axum::http::StatusCode; - use axum::routing::post; - use axum::{Json, Router}; - use fabro_types::WorkflowVersion; - use serde_json::json; - use super::*; - fn params(entrypoint: &str, files: &[(&str, &str)]) -> FabroWorkflowVersionCreateParams { - FabroWorkflowVersionCreateParams { + fn source(entrypoint: &str, files: &[(&str, &str)]) -> ValidatedWorkflowVersionCreate { + ValidatedWorkflowVersionCreate { entrypoint: entrypoint.parse().unwrap(), files: files .iter() @@ -102,8 +53,8 @@ mod tests { } } - fn fixture() -> FabroWorkflowVersionCreateParams { - params("workflow.toml", &[ + fn fixture() -> ValidatedWorkflowVersionCreate { + source("workflow.toml", &[ ( "workflow.toml", "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", @@ -112,350 +63,77 @@ mod tests { "workflow.fabro", r#"digraph W { p [prompt="@prompt.md"] child [stack.child_workflow="child.fabro"] }"#, ), - ( - "prompt.md", - "Keep {{ secrets.TEST }} and {{ env.TEST }} for runtime.", - ), + ("prompt.md", "Review the implementation."), ("child.fabro", "digraph Child {}"), ]) } - fn collect(params: &FabroWorkflowVersionCreateParams) -> CollectedWorkflowClosure { - params.validate().unwrap(); - collect_supplied_workflow(params, tempfile::tempdir().unwrap()).unwrap() - } - - #[test] - fn workflow_version_content_matches_existing_collector_and_cleans_staging() { - for input in [ - params("workflow.fabro", &[("workflow.fabro", "digraph W {}")]), - fixture(), - ] { - let source = tempfile::tempdir().unwrap(); - for (path, content) in &input.files { - std::fs::write(source.path().join(path.as_str()), content).unwrap(); - } - let expected = fabro_manifest::collect_workflow_versions( - Path::new(input.entrypoint.as_str()), - source.path(), - ) + #[tokio::test] + async fn packager_returns_dependencies_before_root() { + let packaged = ServerWorkflowVersionPackager + .package(fixture()) + .await .unwrap(); - let staging = tempfile::tempdir().unwrap(); - let path = staging.path().to_owned(); - let actual = collect_supplied_workflow(&input, staging).unwrap(); - assert!(!path.exists()); - assert_eq!(actual.root_id(), expected.root_id()); - assert_eq!( - actual - .versions() - .map(|(_, v)| v.version()) - .collect::>(), - expected - .versions() - .map(|(_, v)| v.version()) - .collect::>() - ); - } - } - - #[test] - fn workflow_version_exact_extensionless_entrypoint_and_child_ignore_selectors() { - let input = params("workflow", &[ - ( - "workflow", - r#"digraph W { child [stack.child_workflow="child"] }"#, - ), - ("child", "digraph Child {}"), - ( - ".fabro/project.toml", - "malformed project config must not be read", - ), - ( - ".fabro/workflows/workflow/workflow.toml", - "misleading named workflow", - ), - ]); - let closure = collect(&input); - let versions = closure.versions().collect::>(); - assert_eq!(versions.len(), 2); - assert_eq!(versions[1].1.version().entrypoint().as_str(), "workflow"); - assert_eq!(versions[0].1.version().entrypoint().as_str(), "child"); - } - - #[test] - fn workflow_version_rejects_missing_and_escaping_references_and_cleans_failure() { - let parent = tempfile::tempdir().unwrap(); - std::fs::write( - parent.path().join("outside.md"), - "host content must never satisfy a reference", - ) - .unwrap(); - std::fs::write(parent.path().join("child.fabro"), "digraph Host {}").unwrap(); - for (index, input) in [ - params("workflow.fabro", &[ - ("workflow.fabro", r#"digraph W { p [prompt="@prompt.md"] }"#), - ("Prompt.md", "wrong case"), - ]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [prompt="@../outside.md"] }"#, - )]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [prompt="@sub/../../outside.md"] }"#, - )]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [prompt="@outside.md"] }"#, - )]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [output_schema="@../outside.md"] }"#, - )]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [stack.child_workflow="../child.fabro"] }"#, - )]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [stack.child_workflow="sub/../../child.fabro"] }"#, - )]), - params("workflow.fabro", &[( - "workflow.fabro", - r#"digraph W { p [stack.child_workflow="missing"] }"#, - )]), - params("workflow.toml", &[( - "workflow.toml", - "_version = 1\n[workflow]\ngraph = \"../child.fabro\"\n", - )]), - params("workflow.fabro", &[( - "workflow.fabro", - "invalid source PRIVATE_CONTENT", - )]), - ] - .into_iter() - .enumerate() - { - let staging = tempfile::tempdir_in(parent.path()).unwrap(); - let path = staging.path().to_owned(); - assert!( - collect_supplied_workflow(&input, staging).is_err(), - "accepted invalid fixture {index}" - ); - assert!(!path.exists()); - } - } - - #[test] - fn workflow_version_registration_preserves_literal_scripts_without_executing() { - let directory = tempfile::tempdir().unwrap(); - let marker = directory.path().join("must-not-exist"); - // Script is literal command text in Fabro, not an @file import. - let graph = format!( - "digraph W {{ command [script=\"touch {}\"] }}", - marker.display() - ); - let input = params("workflow", &[("workflow", &graph)]); - let closure = collect(&input); - let root = closure.versions().last().unwrap().1.version(); - assert_eq!(root.files()[&"workflow".parse().unwrap()], graph); - assert!(!marker.exists()); - } - - #[test] - fn workflow_version_map_order_is_irrelevant_and_reachable_changes_change_ids() { - let input = fixture(); - let first = collect(&input); - let mut reordered = input.clone(); - reordered.files = input.files.into_iter().rev().collect(); - assert_eq!(first.root_id(), collect(&reordered).root_id()); - reordered - .files - .insert("prompt.md".parse().unwrap(), "changed".into()); - let changed = collect(&reordered); - assert_ne!(first.root_id(), changed.root_id()); + assert_eq!(packaged.versions.len(), 2); + assert_eq!(packaged.versions[0].entrypoint().as_str(), "child.fabro"); assert_eq!( - first.versions().next().unwrap().0, - changed.versions().next().unwrap().0 + packaged.versions[1].id().unwrap(), + packaged.root_id, + "root version must be last" ); - reordered - .files - .insert("child.fabro".parse().unwrap(), "digraph Changed {}".into()); - let changed_child = collect(&reordered); - assert_ne!(changed.root_id(), changed_child.root_id()); - assert_ne!( - changed.versions().next().unwrap().0, - changed_child.versions().next().unwrap().0 + let child_id = packaged.versions[0].id().unwrap(); + assert!( + packaged.versions[1] + .workflow_dependencies() + .values() + .any(|id| *id == child_id) ); } #[tokio::test] - async fn workflow_version_uploads_dependencies_first_and_retries_immutable_content() { - let uploads = Arc::new(Mutex::new(Vec::::new())); - let seen = uploads.clone(); - let app = Router::new().route( - "/api/v1/workflow-versions", - post(move |Json(version): Json| { - let seen = seen.clone(); - async move { - let mut seen = seen.lock().unwrap(); - for id in version.workflow_dependencies().values() { - assert!( - seen.iter().any(|prior| prior.id().unwrap() == *id), - "dependency must be registered first" - ); - } - let id = version.id().unwrap(); - seen.push(version); - (StatusCode::CREATED, Json(json!({"workflow_version_id":id}))) - } - }), - ); - let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); - let client = - Client::new_no_proxy(&format!("http://{}", listener.local_addr().unwrap())).unwrap(); - let server = tokio::spawn(async move { - axum::serve(listener, app).await.unwrap(); - }); - let expected = collect(&fixture()).root_id(); - for _ in 0..2 { - let id = ServerWorkflowVersionCreateAdapter - .create_workflow_version(fixture(), &client) - .await - .unwrap(); - assert_eq!(id, expected); - } - assert_eq!(uploads.lock().unwrap().len(), 4); - server.abort(); - assert!(server.await.unwrap_err().is_cancelled()); - } - - #[tokio::test] - async fn workflow_version_invalid_closure_has_no_uploads_or_source_in_errors() { - let server = httpmock::MockServer::start_async().await; - let upload = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST); - then.status(500); - }) - .await; - let client = Client::new_no_proxy(&server.url("")).unwrap(); - let mut invalid = fixture(); + async fn packaging_errors_never_quote_supplied_source() { + let mut invalid_root = fixture(); // Child is valid, but the root fails after its dependency is assembled. - invalid.files.insert("workflow.toml".parse().unwrap(), "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.goal]\nfile = \"missing.md\"".into()); + invalid_root.files.insert( + "workflow.toml".parse().unwrap(), + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.goal]\nfile = \"missing.md\"" + .into(), + ); let mut oversized = fixture(); - let huge_prompt = "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1); - oversized - .files - .insert("prompt.md".parse().unwrap(), huge_prompt); + oversized.files.insert( + "prompt.md".parse().unwrap(), + "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1), + ); for input in [ - invalid, + invalid_root, oversized, - params("workflow", &[( + source("workflow", &[( "workflow", "PRIVATE_CONTENT invalid source", )]), ] { - let error = ServerWorkflowVersionCreateAdapter - .create_workflow_version(input, &client) + let error = ServerWorkflowVersionPackager + .package(input) .await .unwrap_err(); - assert!(!format!("{error:#}").contains("PRIVATE_CONTENT")); + let rendered = format!("{error:#}"); + assert!(!rendered.contains("PRIVATE_CONTENT"), "{rendered}"); + assert_eq!(rendered, PACKAGING_FAILED); } - upload.assert_calls_async(0).await; - } - - #[tokio::test] - async fn workflow_version_root_upload_failure_leaves_child_for_safe_retry() { - let server = httpmock::MockServer::start_async().await; - let closure = collect(&fixture()); - let child = closure.versions().next().unwrap().1.version(); - let root = closure.versions().last().unwrap().1.version(); - let child_upload = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST) - .path("/api/v1/workflow-versions") - .json_body_obj(child); - then.status(201) - .json_body(json!({"workflow_version_id":child.id().unwrap()})); - }) - .await; - let failed_root = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST) - .path("/api/v1/workflow-versions") - .json_body_obj(root); - then.status(400); - }) - .await; - let deletion = server - .mock_async(|when, then| { - when.method(httpmock::Method::DELETE); - then.status(500); - }) - .await; - let client = Client::new_no_proxy(&server.url("")).unwrap(); + let mut wrong_case = fixture(); + let prompt = wrong_case + .files + .remove(&"prompt.md".parse().unwrap()) + .unwrap(); + wrong_case + .files + .insert("Prompt.md".parse().unwrap(), prompt); assert!( - ServerWorkflowVersionCreateAdapter - .create_workflow_version(fixture(), &client) + ServerWorkflowVersionPackager + .package(wrong_case) .await - .is_err() + .is_err(), + "a case-insensitive host must not satisfy an exact reference" ); - child_upload.assert_calls_async(1).await; - failed_root.assert_calls_async(1).await; - failed_root.delete_async().await; - let root_upload = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST) - .path("/api/v1/workflow-versions") - .json_body_obj(root); - then.status(201) - .json_body(json!({"workflow_version_id":root.id().unwrap()})); - }) - .await; - assert_eq!( - ServerWorkflowVersionCreateAdapter - .create_workflow_version(fixture(), &client) - .await - .unwrap(), - closure.root_id() - ); - child_upload.assert_calls_async(2).await; - root_upload.assert_calls_async(1).await; - deletion.assert_calls_async(0).await; - } - - #[tokio::test] - async fn workflow_version_failed_upload_and_wrong_server_id_never_return_success() { - for wrong_id in [false, true] { - let server = httpmock::MockServer::start_async().await; - let closure = collect(&fixture()); - let child = closure.versions().next().unwrap().1.version(); - let upload = server.mock_async(|when, then| { - when.method(httpmock::Method::POST).path("/api/v1/workflow-versions").json_body_obj(child); - if wrong_id { - then.status(201).json_body(json!({"workflow_version_id": WorkflowVersionId::from(fabro_types::BlobHash::new(b"wrong"))})); - } else { then.status(400); } - }).await; - let root = closure.versions().last().unwrap().1.version(); - let root_upload = server - .mock_async(|when, then| { - when.method(httpmock::Method::POST) - .path("/api/v1/workflow-versions") - .json_body_obj(root); - then.status(201) - .json_body(json!({"workflow_version_id":root.id().unwrap()})); - }) - .await; - let client = Client::new_no_proxy(&server.url("")).unwrap(); - assert!( - ServerWorkflowVersionCreateAdapter - .create_workflow_version(fixture(), &client) - .await - .is_err() - ); - upload.assert_calls_async(1).await; - root_upload.assert_calls_async(0).await; - } } } diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index 1ac7e1eca..dcb395052 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -23,6 +23,7 @@ fabro-types = { path = "../../foundation/fabro-types" } fabro-workflow = { path = "../fabro-workflow" } fabro-workflow-version = { path = "../fabro-workflow-version" } git2.workspace = true +tempfile = "3" thiserror.workspace = true toml.workspace = true @@ -31,5 +32,4 @@ fabro-test.workspace = true fabro-util = { path = "../../foundation/fabro-util" } insta.workspace = true serde_json.workspace = true -tempfile = "3" temp-env = "0.3" diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index a3f36c3e4..9f46f2be7 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -4,6 +4,7 @@ )] mod local_workflow_package; +mod supplied_workflow; mod workflow_bundler; mod workflow_version_collector; @@ -34,6 +35,7 @@ use fabro_workflow::git::{self, GitSyncStatus}; pub use crate::local_workflow_package::{ LocalWorkflowPackageError, ResolvedLocalWorkflowPackage, resolve_local_workflow_package, }; +pub use crate::supplied_workflow::collect_supplied_workflow_versions; use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ CollectedWorkflowClosure, WorkflowVersionCollectError, collect_workflow_versions, diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs new file mode 100644 index 000000000..3d0d7ab5b --- /dev/null +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -0,0 +1,276 @@ +//! Package workflow versions from caller-supplied file contents instead of a +//! checkout on disk. + +use std::collections::BTreeMap; +use std::path::Path; + +use anyhow::Result; +use fabro_config::project::WorkflowLocation; +use fabro_types::WorkflowPath; +use tempfile::TempDir; + +use crate::CollectedWorkflowClosure; + +/// Stage `files` in a private temporary directory and collect the workflow +/// closure rooted at `entrypoint` with the same collector used for checkouts. +/// Only supplied files can satisfy references; the staging directory is +/// removed on every return path. Every dependency is validated before this +/// returns and nothing is registered. +pub fn collect_supplied_workflow_versions( + entrypoint: &WorkflowPath, + files: &BTreeMap, +) -> Result { + let staging = tempfile::Builder::new() + .prefix("fabro-workflow-version-") + .tempdir()?; + collect_in_staging(entrypoint, files, &staging) +} + +fn collect_in_staging( + entrypoint: &WorkflowPath, + files: &BTreeMap, + staging: &TempDir, +) -> Result { + let root = staging.path().canonicalize()?; + for (path, contents) in files { + let destination = root.join(path.as_str()); + if let Some(parent) = destination.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::write(destination, contents)?; + } + let entrypoint = Path::new(entrypoint.as_str()); + let location = WorkflowLocation::from_exact_path(entrypoint, &root)?; + let closure = crate::collect_workflow_versions_at_location(&location, &root, entrypoint)?; + // A case-insensitive host must not satisfy a reference that is missing + // from the supplied tree under its exact key. + for (_, version) in closure.versions() { + for path in version.version().files().keys() { + anyhow::ensure!( + files.contains_key(path), + "collected file `{path}` was not supplied" + ); + } + } + Ok(closure) +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::*; + + struct Supplied { + entrypoint: WorkflowPath, + files: BTreeMap, + } + + fn supplied(entrypoint: &str, files: &[(&str, &str)]) -> Supplied { + Supplied { + entrypoint: entrypoint.parse().unwrap(), + files: files + .iter() + .map(|(path, content)| (path.parse().unwrap(), (*content).to_string())) + .collect(), + } + } + + fn fixture() -> Supplied { + supplied("workflow.toml", &[ + ( + "workflow.toml", + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", + ), + ( + "workflow.fabro", + r#"digraph W { p [prompt="@prompt.md"] child [stack.child_workflow="child.fabro"] }"#, + ), + ( + "prompt.md", + "Keep {{ secrets.TEST }} and {{ env.TEST }} for runtime.", + ), + ("child.fabro", "digraph Child {}"), + ]) + } + + fn collect(input: &Supplied) -> CollectedWorkflowClosure { + collect_supplied_workflow_versions(&input.entrypoint, &input.files).unwrap() + } + + /// Consumes `staging` so the tests can assert cleanup after return. + fn collect_with_staging( + input: &Supplied, + staging: TempDir, + ) -> Result { + let result = collect_in_staging(&input.entrypoint, &input.files, &staging); + drop(staging); + result + } + + #[test] + fn supplied_content_matches_checkout_collector_and_cleans_staging() { + for input in [ + supplied("workflow.fabro", &[("workflow.fabro", "digraph W {}")]), + fixture(), + ] { + let source = tempfile::tempdir().unwrap(); + for (path, content) in &input.files { + std::fs::write(source.path().join(path.as_str()), content).unwrap(); + } + let expected = crate::collect_workflow_versions( + Path::new(input.entrypoint.as_str()), + source.path(), + ) + .unwrap(); + let staging = tempfile::tempdir().unwrap(); + let path = staging.path().to_owned(); + let actual = collect_with_staging(&input, staging).unwrap(); + assert!(!path.exists()); + assert_eq!(actual.root_id(), expected.root_id()); + assert_eq!( + actual + .versions() + .map(|(_, v)| v.version()) + .collect::>(), + expected + .versions() + .map(|(_, v)| v.version()) + .collect::>() + ); + } + } + + #[test] + fn exact_extensionless_entrypoint_and_child_ignore_selectors() { + let input = supplied("workflow", &[ + ( + "workflow", + r#"digraph W { child [stack.child_workflow="child"] }"#, + ), + ("child", "digraph Child {}"), + ( + ".fabro/project.toml", + "malformed project config must not be read", + ), + ( + ".fabro/workflows/workflow/workflow.toml", + "misleading named workflow", + ), + ]); + let closure = collect(&input); + let versions = closure.versions().collect::>(); + assert_eq!(versions.len(), 2); + assert_eq!(versions[1].1.version().entrypoint().as_str(), "workflow"); + assert_eq!(versions[0].1.version().entrypoint().as_str(), "child"); + } + + #[test] + fn rejects_missing_and_escaping_references_and_cleans_failure() { + let parent = tempfile::tempdir().unwrap(); + std::fs::write( + parent.path().join("outside.md"), + "host content must never satisfy a reference", + ) + .unwrap(); + std::fs::write(parent.path().join("child.fabro"), "digraph Host {}").unwrap(); + for (index, input) in [ + supplied("workflow.fabro", &[ + ("workflow.fabro", r#"digraph W { p [prompt="@prompt.md"] }"#), + ("Prompt.md", "wrong case"), + ]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [prompt="@../outside.md"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [prompt="@sub/../../outside.md"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [prompt="@outside.md"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [output_schema="@../outside.md"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="../child.fabro"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="sub/../../child.fabro"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="missing"] }"#, + )]), + supplied("workflow.toml", &[( + "workflow.toml", + "_version = 1\n[workflow]\ngraph = \"../child.fabro\"\n", + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + "invalid source PRIVATE_CONTENT", + )]), + ] + .into_iter() + .enumerate() + { + let staging = tempfile::tempdir_in(parent.path()).unwrap(); + let path = staging.path().to_owned(); + assert!( + collect_with_staging(&input, staging).is_err(), + "accepted invalid fixture {index}" + ); + assert!(!path.exists()); + } + } + + #[test] + fn preserves_literal_scripts_without_executing() { + let directory = tempfile::tempdir().unwrap(); + let marker = directory.path().join("must-not-exist"); + // Script is literal command text in Fabro, not an @file import. + let graph = format!( + "digraph W {{ command [script=\"touch {}\"] }}", + marker.display() + ); + let input = supplied("workflow", &[("workflow", &graph)]); + let closure = collect(&input); + let root = closure.versions().last().unwrap().1.version(); + assert_eq!(root.files()[&"workflow".parse().unwrap()], graph); + assert!(!marker.exists()); + } + + #[test] + fn map_order_is_irrelevant_and_reachable_changes_change_ids() { + let input = fixture(); + let first = collect(&input); + let mut reordered = Supplied { + entrypoint: input.entrypoint.clone(), + files: input.files.into_iter().rev().collect(), + }; + assert_eq!(first.root_id(), collect(&reordered).root_id()); + reordered + .files + .insert("prompt.md".parse().unwrap(), "changed".into()); + let changed = collect(&reordered); + assert_ne!(first.root_id(), changed.root_id()); + assert_eq!( + first.versions().next().unwrap().0, + changed.versions().next().unwrap().0 + ); + reordered + .files + .insert("child.fabro".parse().unwrap(), "digraph Changed {}".into()); + let changed_child = collect(&reordered); + assert_ne!(changed.root_id(), changed_child.root_id()); + assert_ne!( + changed.versions().next().unwrap().0, + changed_child.versions().next().unwrap().0 + ); + } +} diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 4aaa989f5..6e256d6a3 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -143,28 +143,28 @@ impl<'a> WorkflowBundler<'a> { /// Relative workflow references with an extension are lexically /// normalized (`..` segments resolved without consulting the filesystem, /// `~` rejected) before resolution, so the file read matches the manifest - /// key. Returns the collected workflow's manifest key. + /// key. Workflow-version projection normalizes every reference and + /// resolves it as an exact path inside the package root, with no + /// workflow-name lookup. Returns the collected workflow's manifest key. fn collect_workflow_entry(&mut self, workflow: &Path, resolve_from: &Path) -> Result { - let location = if self.workflow_version_projection { - let exact = normalize_absolute_path(resolve_from, &workflow.to_string_lossy()) - .ok_or_else(|| anyhow!("unsupported workflow reference"))?; - // Check containment before location resolution can read a config. - manifest_path_from_absolute(&exact, self.package_root)?; - WorkflowLocation::from_exact_path(&exact, self.package_root)? + let normalize = self.workflow_version_projection + || (workflow.extension().is_some() && workflow.is_relative()); + let normalized = if normalize { + normalize_absolute_path(resolve_from, &workflow.to_string_lossy()).ok_or_else(|| { + anyhow!( + "unsupported manifest workflow reference: {}", + workflow.display() + ) + })? } else { - let normalized_workflow = if workflow.extension().is_some() && workflow.is_relative() { - normalize_absolute_path(resolve_from, &workflow.to_string_lossy()).ok_or_else( - || { - anyhow!( - "unsupported manifest workflow reference: {}", - workflow.display() - ) - }, - )? - } else { - workflow.to_path_buf() - }; - WorkflowLocation::resolve(&normalized_workflow, resolve_from)? + workflow.to_path_buf() + }; + let location = if self.workflow_version_projection { + // Check containment before location resolution can read a config. + manifest_path_from_absolute(&normalized, self.package_root)?; + WorkflowLocation::from_exact_path(&normalized, self.package_root)? + } else { + WorkflowLocation::resolve(&normalized, resolve_from)? }; self.collect_workflow_location(&location) } diff --git a/lib/components/fabro-tool/Cargo.toml b/lib/components/fabro-tool/Cargo.toml index 0d3a2862d..892b7a6a7 100644 --- a/lib/components/fabro-tool/Cargo.toml +++ b/lib/components/fabro-tool/Cargo.toml @@ -30,4 +30,5 @@ toml.workspace = true [dev-dependencies] fabro-types = { path = "../../foundation/fabro-types", features = ["test-support"] } +httpmock = "0.8" tempfile = "3" diff --git a/lib/components/fabro-tool/src/common.rs b/lib/components/fabro-tool/src/common.rs index f88249fa2..f154759b8 100644 --- a/lib/components/fabro-tool/src/common.rs +++ b/lib/components/fabro-tool/src/common.rs @@ -50,9 +50,9 @@ pub type ToolResult = Result; pub trait FabroToolBackend: Send + Sync { async fn create_workflow_version( &self, - _params: crate::FabroWorkflowVersionCreateParams, + _source: crate::ValidatedWorkflowVersionCreate, ) -> anyhow::Result { - anyhow::bail!("fabro_workflow_version_create is not available") + Err(workflow_version_tool_unavailable_error()) } async fn create_run_from_spec( @@ -142,6 +142,13 @@ fn pair_tool_unavailable_error() -> anyhow::Error { ToolError::message(format!("{FABRO_RUN_PAIR_TOOL_NAME} is not available")).into() } +pub(crate) fn workflow_version_tool_unavailable_error() -> anyhow::Error { + ToolError::message(format!( + "{FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME} is not available" + )) + .into() +} + pub trait RunManifestBuilder: Send + Sync { fn build_run_manifest( &self, @@ -350,7 +357,7 @@ mod tests { fn workflow_version_create_has_strict_content_schema() { let definition = tool_definitions() .iter() - .find(|definition| definition.name == "fabro_workflow_version_create") + .find(|definition| definition.name == FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME) .expect("workflow version creation should be in the shared catalog"); let schema = &definition.parameters; assert_eq!(schema["additionalProperties"], false); diff --git a/lib/components/fabro-tool/src/fabro_client.rs b/lib/components/fabro-tool/src/fabro_client.rs index eb5599c5e..d01ebb1bd 100644 --- a/lib/components/fabro-tool/src/fabro_client.rs +++ b/lib/components/fabro-tool/src/fabro_client.rs @@ -8,14 +8,14 @@ use fabro_types::{ PairTranscriptResponse, Run, RunId, RunPairStatusResponse, RunProjection, StageId, }; -use crate::{FabroToolBackend, RunManifestBuilder, ToolError}; +use crate::{FabroToolBackend, RunManifestBuilder, ToolError, common}; #[derive(Clone)] pub struct ClientBackend { - client: Arc<::fabro_client::Client>, - manifest_builder: Option>, - run_scope: Option, - workflow_version_create_adapter: Option>, + client: Arc<::fabro_client::Client>, + manifest_builder: Option>, + run_scope: Option, + workflow_version_packager: Option>, } impl ClientBackend { @@ -25,7 +25,7 @@ impl ClientBackend { client, manifest_builder: None, run_scope: None, - workflow_version_create_adapter: None, + workflow_version_packager: None, } } @@ -36,11 +36,11 @@ impl ClientBackend { } #[must_use] - pub fn with_workflow_version_create_adapter( + pub fn with_workflow_version_packager( mut self, - adapter: Arc, + packager: Arc, ) -> Self { - self.workflow_version_create_adapter = Some(adapter); + self.workflow_version_packager = Some(packager); self } @@ -66,19 +66,26 @@ impl ClientBackend { #[async_trait] impl FabroToolBackend for ClientBackend { + /// Package the supplied tree, then register dependencies before parents. + /// Versions are immutable and content-addressed, so a failed upload can be + /// retried with the same contents without cleanup. async fn create_workflow_version( &self, - params: crate::FabroWorkflowVersionCreateParams, + source: crate::ValidatedWorkflowVersionCreate, ) -> anyhow::Result { anyhow::ensure!( self.run_scope.is_none(), "workflow version creation is outside this tool session's run scope" ); - let adapter = self - .workflow_version_create_adapter + let packager = self + .workflow_version_packager .as_ref() - .ok_or_else(|| anyhow::anyhow!("fabro_workflow_version_create is not available"))?; - adapter.create_workflow_version(params, &self.client).await + .ok_or_else(common::workflow_version_tool_unavailable_error)?; + let packaged = packager.package(source).await?; + self.client + .register_workflow_versions(&packaged.versions) + .await?; + Ok(packaged.root_id) } async fn create_run_from_spec( @@ -278,3 +285,125 @@ impl FabroToolBackend for ClientBackend { .await } } + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use async_trait::async_trait; + use fabro_types::{WorkflowVersion, WorkflowVersionId}; + use serde_json::json; + + use super::*; + use crate::{ + PackagedWorkflowVersions, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, + }; + + struct FixedPackager(PackagedWorkflowVersions); + + #[async_trait] + impl WorkflowVersionPackager for FixedPackager { + async fn package( + &self, + _: ValidatedWorkflowVersionCreate, + ) -> anyhow::Result { + Ok(self.0.clone()) + } + } + + fn version( + entrypoint: &str, + dependencies: BTreeMap, + ) -> WorkflowVersion { + WorkflowVersion::new( + entrypoint.parse().unwrap(), + BTreeMap::from([( + entrypoint.parse().unwrap(), + format!("digraph {entrypoint} {{}}"), + )]), + dependencies, + ) + .unwrap() + } + + fn source() -> ValidatedWorkflowVersionCreate { + ValidatedWorkflowVersionCreate { + entrypoint: "root".parse().unwrap(), + files: BTreeMap::new(), + } + } + + #[tokio::test] + async fn create_workflow_version_registers_packaged_closure_and_retries_after_failure() { + let server = httpmock::MockServer::start_async().await; + let child = version("child", BTreeMap::new()); + let child_id = child.id().unwrap(); + let root = version( + "root", + BTreeMap::from([("child".parse().unwrap(), child_id)]), + ); + let root_id = root.id().unwrap(); + let child_upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(&child); + then.status(201) + .json_body(json!({"workflow_version_id": child_id})); + }) + .await; + let failed_root = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(&root); + then.status(400); + }) + .await; + let client = ::fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); + let backend = ClientBackend::new(Arc::new(client)).with_workflow_version_packager( + Arc::new(FixedPackager(PackagedWorkflowVersions { + root_id, + versions: vec![child, root.clone()], + })), + ); + + assert!(backend.create_workflow_version(source()).await.is_err()); + child_upload.assert_calls_async(1).await; + failed_root.assert_calls_async(1).await; + + // Immutable content: retrying re-sends the child and completes the root. + failed_root.delete_async().await; + let root_upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(&root); + then.status(201) + .json_body(json!({"workflow_version_id": root_id})); + }) + .await; + assert_eq!( + backend.create_workflow_version(source()).await.unwrap(), + root_id + ); + child_upload.assert_calls_async(2).await; + root_upload.assert_calls_async(1).await; + } + + #[tokio::test] + async fn create_workflow_version_without_packager_is_unavailable() { + let client = ::fabro_client::Client::new_no_proxy("http://127.0.0.1:1").unwrap(); + let error = ClientBackend::new(Arc::new(client)) + .create_workflow_version(source()) + .await + .unwrap_err(); + assert_eq!( + error.to_string(), + format!( + "{} is not available", + crate::FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME + ) + ); + } +} diff --git a/lib/components/fabro-tool/src/lib.rs b/lib/components/fabro-tool/src/lib.rs index 64ac87f27..d3ade2e3e 100644 --- a/lib/components/fabro-tool/src/lib.rs +++ b/lib/components/fabro-tool/src/lib.rs @@ -49,6 +49,6 @@ pub use search::{ search_runs, search_runs_text, }; pub use workflow_version::{ - FabroWorkflowVersionCreateParams, WorkflowVersionCreateAdapter, create_workflow_version, - workflow_version_create_text, + FabroWorkflowVersionCreateParams, PackagedWorkflowVersions, ValidatedWorkflowVersionCreate, + WorkflowVersionPackager, create_workflow_version, workflow_version_create_text, }; diff --git a/lib/components/fabro-tool/src/workflow_version.rs b/lib/components/fabro-tool/src/workflow_version.rs index 11567df8a..8adb193f8 100644 --- a/lib/components/fabro-tool/src/workflow_version.rs +++ b/lib/components/fabro-tool/src/workflow_version.rs @@ -5,10 +5,9 @@ use async_trait::async_trait; use fabro_api::types::CreateWorkflowVersionResponse; use fabro_types::{ MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES, - WorkflowPath, WorkflowVersionId, + WorkflowPath, WorkflowVersion, WorkflowVersionId, }; use schemars::JsonSchema; -use serde::de::{Error as _, MapAccess, Visitor}; use serde::{Deserialize, Serialize}; use crate::{FabroToolBackend, ToolError, ToolResult}; @@ -23,80 +22,83 @@ pub struct FabroWorkflowVersionCreateParams { pub entrypoint: WorkflowPath, /// All local dependencies, keyed by package-relative path. Values are text /// contents. - #[serde(deserialize_with = "deserialize_files")] + #[serde(deserialize_with = "fabro_types::deserialize_unique_map")] #[schemars(with = "BTreeMap")] pub files: BTreeMap, } -fn deserialize_files<'de, D: serde::Deserializer<'de>>( - deserializer: D, -) -> Result, D::Error> { - struct FilesVisitor; - impl<'de> Visitor<'de> for FilesVisitor { - type Value = BTreeMap; - fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - formatter.write_str("workflow files with unique path keys and text contents") - } - fn visit_map>(self, mut map: A) -> Result { - let mut files = BTreeMap::new(); - while let Some((path, content)) = map.next_entry()? { - if files.insert(path, content).is_some() { - return Err(A::Error::custom("duplicate workflow file key")); - } - } - Ok(files) - } - } - deserializer.deserialize_map(FilesVisitor) +/// A supplied source tree whose entrypoint, budgets, and portable path +/// collisions have been checked, so it is safe to stage on a filesystem. +#[derive(Clone, Debug)] +pub struct ValidatedWorkflowVersionCreate { + pub entrypoint: WorkflowPath, + pub files: BTreeMap, } -impl FabroWorkflowVersionCreateParams { - /// Validate the complete supplied tree before any filesystem writes. - pub fn validate(&self) -> ToolResult<()> { - if !self.files.contains_key(&self.entrypoint) { +impl TryFrom for ValidatedWorkflowVersionCreate { + type Error = ToolError; + + fn try_from(params: FabroWorkflowVersionCreateParams) -> Result { + let FabroWorkflowVersionCreateParams { entrypoint, files } = params; + if !files.contains_key(&entrypoint) { return Err(ToolError::message( "entrypoint must be an exact supplied file key", )); } - if self.files.len() > MAX_WORKFLOW_VERSION_FILES { - return Err(ToolError::message("workflow source exceeds 512 files")); + if files.len() > MAX_WORKFLOW_VERSION_FILES { + return Err(ToolError::message(format!( + "workflow source exceeds {MAX_WORKFLOW_VERSION_FILES} files" + ))); } let mut total = 0; - for content in self.files.values() { + for content in files.values() { if content.len() > MAX_WORKFLOW_VERSION_FILE_BYTES { - return Err(ToolError::message("workflow source file exceeds 512 KiB")); + return Err(ToolError::message(format!( + "workflow source file exceeds {} KiB", + MAX_WORKFLOW_VERSION_FILE_BYTES / 1024 + ))); } total += content.len(); } if total > MAX_WORKFLOW_VERSION_BYTES { - return Err(ToolError::message("workflow source exceeds 2 MiB")); + return Err(ToolError::message(format!( + "workflow source exceeds {} MiB", + MAX_WORKFLOW_VERSION_BYTES / (1024 * 1024) + ))); } - fabro_types::validate_workflow_source_paths(self.files.keys()) + fabro_types::validate_workflow_source_paths(files.keys()) .map_err(|_| ToolError::message("workflow source paths collide"))?; - Ok(()) + Ok(Self { entrypoint, files }) } } -/// Application seam for packaging and registering content without a dependency -/// cycle. Implementations must validate before staging, confine reads to -/// supplied files, validate the entire closure before uploading, and register -/// dependencies first. +/// The complete validated closure for one supplied source tree. +#[derive(Clone, Debug)] +pub struct PackagedWorkflowVersions { + pub root_id: WorkflowVersionId, + /// Every version in the closure, dependencies before the versions that + /// reference them, so callers can register them in this order. + pub versions: Vec, +} + +/// Application seam for packaging supplied content. The manifest crates that +/// own collection depend on this crate, so the packager is injected instead. +/// Implementations confine reads to supplied files and validate the entire +/// closure before returning. #[async_trait] -pub trait WorkflowVersionCreateAdapter: Send + Sync { - async fn create_workflow_version( +pub trait WorkflowVersionPackager: Send + Sync { + async fn package( &self, - params: FabroWorkflowVersionCreateParams, - client: &fabro_client::Client, - ) -> anyhow::Result; + source: ValidatedWorkflowVersionCreate, + ) -> anyhow::Result; } pub async fn create_workflow_version( backend: Arc, - params: FabroWorkflowVersionCreateParams, + source: ValidatedWorkflowVersionCreate, ) -> ToolResult { - params.validate()?; let workflow_version_id = backend - .create_workflow_version(params) + .create_workflow_version(source) .await .map_err(|err| ToolError::from_anyhow(&err))?; Ok(CreateWorkflowVersionResponse { @@ -106,7 +108,7 @@ pub async fn create_workflow_version( #[must_use] pub fn workflow_version_create_text(result: &CreateWorkflowVersionResponse) -> String { - serde_json::to_string(result).expect("workflow version response should serialize") + format!("Registered workflow version {}", result.workflow_version_id) } #[cfg(test)] @@ -116,12 +118,15 @@ mod tests { use super::*; use crate::fabro_client::ClientBackend; + fn validate(value: serde_json::Value) -> ToolResult { + let params: FabroWorkflowVersionCreateParams = serde_json::from_value(value).unwrap(); + ValidatedWorkflowVersionCreate::try_from(params) + } + #[test] fn workflow_version_request_rejects_unknown_fields_and_invalid_paths() { let valid = json!({"entrypoint": "workflow", "files": {"workflow": "digraph W {}"}}); - let params: FabroWorkflowVersionCreateParams = - serde_json::from_value(valid.clone()).unwrap(); - params.validate().unwrap(); + validate(valid.clone()).unwrap(); assert!( serde_json::from_str::( r#"{"entrypoint":"workflow","files":{"workflow":"a","workflow":"b"}}"# @@ -160,75 +165,75 @@ mod tests { #[test] fn workflow_version_source_enforces_presence_collisions_and_budgets() { + assert!( + validate(json!({"entrypoint":"missing","files":{"workflow":"digraph W {}"}})).is_err() + ); for files in [ - json!({}), json!({"A":"x","a":"y"}), json!({"A":"x","a/b.md":"y"}), json!({"a":"x","A/b.md":"y"}), ] { let mut files = files.as_object().unwrap().clone(); files.insert("workflow".into(), json!("digraph W {}")); - let mut params: FabroWorkflowVersionCreateParams = - serde_json::from_value(json!({"entrypoint":"workflow","files":files})).unwrap(); - if params.files.len() == 1 { - params.entrypoint = "missing".parse().unwrap(); - } - assert!(params.validate().is_err()); + assert!(validate(json!({"entrypoint":"workflow","files":files})).is_err()); } - let mut params = FabroWorkflowVersionCreateParams { + let oversized_file = FabroWorkflowVersionCreateParams { entrypoint: "workflow".parse().unwrap(), files: BTreeMap::from([( "workflow".parse().unwrap(), "x".repeat(MAX_WORKFLOW_VERSION_FILE_BYTES + 1), )]), }; - assert!(params.validate().is_err()); - params.files = (0..MAX_WORKFLOW_VERSION_FILES) - .map(|i| (format!("file{i}").parse().unwrap(), String::new())) - .collect(); - params + assert!(ValidatedWorkflowVersionCreate::try_from(oversized_file).is_err()); + let mut too_many_files = FabroWorkflowVersionCreateParams { + entrypoint: "workflow".parse().unwrap(), + files: (0..MAX_WORKFLOW_VERSION_FILES) + .map(|i| (format!("file{i}").parse().unwrap(), String::new())) + .collect(), + }; + too_many_files .files - .insert(params.entrypoint.clone(), String::new()); - assert!(params.validate().is_err()); - params.files = (0..5) - .map(|i| { - ( - format!("file{i}").parse().unwrap(), - "x".repeat(MAX_WORKFLOW_VERSION_FILE_BYTES), - ) - }) - .collect(); - params + .insert(too_many_files.entrypoint.clone(), String::new()); + assert!(ValidatedWorkflowVersionCreate::try_from(too_many_files).is_err()); + let mut oversized_total = FabroWorkflowVersionCreateParams { + entrypoint: "workflow".parse().unwrap(), + files: (0..5) + .map(|i| { + ( + format!("file{i}").parse().unwrap(), + "x".repeat(MAX_WORKFLOW_VERSION_FILE_BYTES), + ) + }) + .collect(), + }; + oversized_total .files - .insert(params.entrypoint.clone(), String::new()); - assert!(params.validate().is_err()); + .insert(oversized_total.entrypoint.clone(), String::new()); + assert!(ValidatedWorkflowVersionCreate::try_from(oversized_total).is_err()); } #[tokio::test] - async fn workflow_version_same_run_backend_denies_before_adapter() { + async fn workflow_version_same_run_backend_denies_before_packaging() { let client = fabro_client::Client::new_no_proxy("http://127.0.0.1:1").unwrap(); let backend = ClientBackend::new(Arc::new(client)) - .with_workflow_version_create_adapter(Arc::new(UnreachableAdapter)) + .with_workflow_version_packager(Arc::new(UnreachablePackager)) .with_run_scope("01KRBZW4DW0000000000000002".parse().unwrap()); - let params = serde_json::from_value( - json!({"entrypoint":"workflow","files":{"workflow":"digraph W {}"}}), - ) - .unwrap(); - let error = create_workflow_version(Arc::new(backend), params) + let source = + validate(json!({"entrypoint":"workflow","files":{"workflow":"digraph W {}"}})).unwrap(); + let error = create_workflow_version(Arc::new(backend), source) .await .unwrap_err(); assert!(error.as_str().contains("run scope")); } - struct UnreachableAdapter; + struct UnreachablePackager; #[async_trait] - impl WorkflowVersionCreateAdapter for UnreachableAdapter { - async fn create_workflow_version( + impl WorkflowVersionPackager for UnreachablePackager { + async fn package( &self, - _: FabroWorkflowVersionCreateParams, - _: &fabro_client::Client, - ) -> anyhow::Result { - panic!("scoped backend must not invoke the adapter") + _: ValidatedWorkflowVersionCreate, + ) -> anyhow::Result { + panic!("scoped backend must not invoke the packager") } } } diff --git a/lib/foundation/fabro-config/src/project.rs b/lib/foundation/fabro-config/src/project.rs index c33f3fc79..f61ae2bff 100644 --- a/lib/foundation/fabro-config/src/project.rs +++ b/lib/foundation/fabro-config/src/project.rs @@ -46,23 +46,19 @@ impl WorkflowLocation { /// graph file (e.g. `workflow.fabro`); the three forms produce the same /// shape. pub fn resolve(arg: &Path, cwd: &Path) -> Result { - let resolved = resolve_workflow_arg_from(arg, cwd)?; - if resolved.extension().is_some_and(|ext| ext == "toml") { - Self::from_toml(resolved) - } else { - Ok(Self::from_graph(resolved)) - } + Self::from_resolved_path(resolve_workflow_arg_from(arg, cwd)?) } - /// Resolve an exact file path without workflow-name or ambient config - /// lookup. Relative paths are interpreted against the supplied - /// directory only. + /// Resolve an exact file path without workflow-name lookup. Relative paths + /// are interpreted against the supplied directory only. pub fn from_exact_path(path: &Path, directory: &Path) -> Result { - let path = directory.join(path); - if path - .extension() - .is_some_and(|extension| extension == "toml") - { + Self::from_resolved_path(directory.join(path)) + } + + /// Dispatch a resolved file path on its extension: `workflow.toml` loads + /// run config, anything else is treated as a graph file. + fn from_resolved_path(path: PathBuf) -> Result { + if path.extension().is_some_and(|ext| ext == "toml") { Self::from_toml(path) } else { Ok(Self::from_graph(path)) diff --git a/lib/foundation/fabro-types/Cargo.toml b/lib/foundation/fabro-types/Cargo.toml index 005fe228f..1fb90fb25 100644 --- a/lib/foundation/fabro-types/Cargo.toml +++ b/lib/foundation/fabro-types/Cargo.toml @@ -33,8 +33,8 @@ strum.workspace = true thiserror.workspace = true toml.workspace = true ulid.workspace = true -unicase = "2" -unicode-normalization = "0.1" +unicase.workspace = true +unicode-normalization.workspace = true url.workspace = true [dev-dependencies] diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index e678a8a1f..a398b827b 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -197,7 +197,7 @@ pub use workflow_path::{ }; pub use workflow_version::{ MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_DEPENDENCIES, MAX_WORKFLOW_VERSION_FILE_BYTES, - MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, + MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, deserialize_unique_map, validate_workflow_source_paths, }; pub use workflow_version_id::{WorkflowVersionId, WorkflowVersionIdParseError}; diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index 996a26444..8b448eaee 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::collections::{BTreeMap, HashMap}; use std::fmt; use std::marker::PhantomData; @@ -153,33 +154,35 @@ impl WorkflowVersion { fn validate_path_collisions(&self) -> Result<(), WorkflowVersionShapeError> { validate_path_collisions( self.files.keys().chain(self.workflow_dependencies.keys()), - false, + Cow::Borrowed, ) } } /// Reject file and directory aliases before materializing a portable source /// tree, including Unicode case folding and normalization. Canonical versions -/// themselves retain their exact, case-sensitive -/// semantics. +/// themselves retain their exact, case-sensitive semantics. pub fn validate_workflow_source_paths<'a>( paths: impl IntoIterator, ) -> Result<(), WorkflowVersionShapeError> { - validate_path_collisions(paths, true) + validate_path_collisions(paths, |text| { + if text.is_ascii() { + Cow::Owned(text.to_ascii_lowercase()) + } else { + Cow::Owned(UniCase::unicode(text).to_folded_case().nfc().collect()) + } + }) } +/// Detect colliding paths under a comparison key: identical keys, or a key +/// that names an ancestor directory of another. fn validate_path_collisions<'a>( paths: impl IntoIterator, - case_insensitive: bool, + key: impl Fn(&'a str) -> Cow<'a, str>, ) -> Result<(), WorkflowVersionShapeError> { let mut by_text = HashMap::new(); for path in paths { - let text = if case_insensitive { - UniCase::new(path.as_str()).to_folded_case().nfc().collect() - } else { - path.as_str().to_owned() - }; - if let Some(existing) = by_text.insert(text, path) { + if let Some(existing) = by_text.insert(key(path.as_str()), path) { return Err(WorkflowVersionShapeError::PathCollision { first: existing.clone(), second: path.clone(), @@ -218,6 +221,17 @@ impl<'de> Deserialize<'de> for WorkflowVersion { } } +/// Deserialize a map while rejecting duplicate keys, which serde would +/// otherwise silently collapse to the last value. +pub fn deserialize_unique_map<'de, D, K, V>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, + K: Deserialize<'de> + Ord + fmt::Display, + V: Deserialize<'de>, +{ + UniqueBTreeMap::deserialize(deserializer).map(|map| map.0) +} + struct UniqueBTreeMap(BTreeMap); impl<'de, K, V> Deserialize<'de> for UniqueBTreeMap From 111e737d5376e78b7e3dc033bb82c164f328cf8a Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:22:04 -0600 Subject: [PATCH 03/15] Reject escaping workflow references before resolving them The pre-resolution containment check in the version bundler was a no-op: ManifestPath::from_absolute happily returns a `..`-prefixed path for locations outside the package root, so an escaping stack.child_workflow reference reached WorkflowLocation resolution, which probes and parses config files on the host before the real containment check in read_package_file ran. The request still failed, but the TOML parser's diagnostic quoted the host file. Check that the normalized reference stays under the package root before resolving it, and extend the supplied-workflow test to plant malformed host files that any parser would quote. Co-Authored-By: Claude Fable 5.1 --- .../fabro-manifest/src/supplied_workflow.rs | 29 +++++++++++++++++-- .../fabro-manifest/src/workflow_bundler.rs | 13 +++++++-- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index 3d0d7ab5b..9f247294c 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -174,6 +174,17 @@ mod tests { ) .unwrap(); std::fs::write(parent.path().join("child.fabro"), "digraph Host {}").unwrap(); + // Malformed on purpose: a parser that reaches this file would quote it. + std::fs::write( + parent.path().join("secret.toml"), + "HOST_SECRET = [unterminated", + ) + .unwrap(); + std::fs::write( + parent.path().join("workflow.toml"), + "HOST_SECRET = [unterminated", + ) + .unwrap(); for (index, input) in [ supplied("workflow.fabro", &[ ("workflow.fabro", r#"digraph W { p [prompt="@prompt.md"] }"#), @@ -203,6 +214,14 @@ mod tests { "workflow.fabro", r#"digraph W { p [stack.child_workflow="sub/../../child.fabro"] }"#, )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="../secret.toml"] }"#, + )]), + supplied("workflow.fabro", &[( + "workflow.fabro", + r#"digraph W { p [stack.child_workflow="../graph.fabro"] }"#, + )]), supplied("workflow.fabro", &[( "workflow.fabro", r#"digraph W { p [stack.child_workflow="missing"] }"#, @@ -221,9 +240,15 @@ mod tests { { let staging = tempfile::tempdir_in(parent.path()).unwrap(); let path = staging.path().to_owned(); + let error = collect_with_staging(&input, staging) + .err() + .unwrap_or_else(|| panic!("accepted invalid fixture {index}")); + let rendered = format!("{error:#}"); + // Escaping references must fail before any host file is opened, + // so no host diagnostic (parse error, exists-vs-missing) leaks. assert!( - collect_with_staging(&input, staging).is_err(), - "accepted invalid fixture {index}" + !rendered.contains("HOST_SECRET") && !rendered.contains("secret.toml:"), + "fixture {index} read a host file: {rendered}" ); assert!(!path.exists()); } diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 6e256d6a3..9eb0add4a 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -160,8 +160,17 @@ impl<'a> WorkflowBundler<'a> { workflow.to_path_buf() }; let location = if self.workflow_version_projection { - // Check containment before location resolution can read a config. - manifest_path_from_absolute(&normalized, self.package_root)?; + // Location resolution probes and parses config files, so reject + // references that leave the package root before it can touch a + // host file. `ManifestPath::from_absolute` accepts `..`-prefixed + // results and is not a containment check. + if !normalized.starts_with(self.package_root) { + bail!( + "workflow reference `{}` escapes source root `{}`", + workflow.display(), + self.package_root.display() + ); + } WorkflowLocation::from_exact_path(&normalized, self.package_root)? } else { WorkflowLocation::resolve(&normalized, resolve_from)? From b998f88cde8412e006c93580035a0deb5393e7ff Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:23:32 -0600 Subject: [PATCH 04/15] Normalize workflow paths before case folding validate_workflow_source_paths folded case before applying NFC, but case folding is not closed under canonical equivalence: a decomposed sequence and its precomposed form can fold to different strings. Two supplied paths that a normalization-insensitive filesystem treats as one entry therefore passed the collision check, and staging silently overwrote one file with the other. Apply NFC first, then fold, then normalize again, and add the Greek pair that reproduced the gap. Co-Authored-By: Claude Fable 5.1 --- lib/foundation/fabro-types/src/workflow_version.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index 8b448eaee..555be2814 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -169,7 +169,16 @@ pub fn validate_workflow_source_paths<'a>( if text.is_ascii() { Cow::Owned(text.to_ascii_lowercase()) } else { - Cow::Owned(UniCase::unicode(text).to_folded_case().nfc().collect()) + // Normalize before folding: case folding is not closed under + // canonical equivalence, so folding a decomposed sequence and + // folding its precomposed form can yield different strings. + let normalized: String = text.nfc().collect(); + Cow::Owned( + UniCase::unicode(normalized) + .to_folded_case() + .nfc() + .collect(), + ) } }) } @@ -561,6 +570,9 @@ mod source_path_tests { ["ΟΣ", "οσ/b"], ["é", "e\u{301}/b"], ["Straße", "STRASSE/b"], + // Canonically equivalent, but folding before normalizing yields + // different keys (U+03B1 U+03AF vs U+03AC U+03B9). + ["α\u{345}\u{301}.md", "\u{1FB4}.md"], ] { let paths = pair.map(|path| WorkflowPath::new(path).unwrap()); assert!(validate_workflow_source_paths(paths.iter()).is_err()); From 6a951652a70ad5b981083461b7ac9b82f3210b2e Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:24:10 -0600 Subject: [PATCH 05/15] Report path collisions in a deterministic order The refactored ancestor-collision loop iterated the HashMap of folded keys, so when a version contained more than one ancestor collision the reported pair depended on the hasher seed. The same request could produce different 422 bodies from POST /workflow-versions on repeated submissions. Collect the input into a Vec and walk it in order for the ancestor pass, matching the previous behavior, and add a test with two collisions that runs the check repeatedly. Co-Authored-By: Claude Fable 5.1 --- .../fabro-types/src/workflow_version.rs | 36 ++++++++++++++++--- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index 555be2814..297e485d6 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -189,16 +189,22 @@ fn validate_path_collisions<'a>( paths: impl IntoIterator, key: impl Fn(&'a str) -> Cow<'a, str>, ) -> Result<(), WorkflowVersionShapeError> { - let mut by_text = HashMap::new(); - for path in paths { - if let Some(existing) = by_text.insert(key(path.as_str()), path) { + let keyed: Vec<(Cow<'a, str>, &WorkflowPath)> = paths + .into_iter() + .map(|path| (key(path.as_str()), path)) + .collect(); + let mut by_text = HashMap::with_capacity(keyed.len()); + for (text, path) in &keyed { + if let Some(existing) = by_text.insert(text.as_ref(), *path) { return Err(WorkflowVersionShapeError::PathCollision { first: existing.clone(), - second: path.clone(), + second: (*path).clone(), }); } } - for (text, path) in &by_text { + // Walk the input order, not the map, so the reported pair is stable when + // more than one ancestor collision exists. + for (text, path) in &keyed { for (index, _) in text.match_indices('/') { if let Some(ancestor) = by_text.get(&text[..index]) { return Err(WorkflowVersionShapeError::PathCollision { @@ -560,6 +566,26 @@ mod tests { mod source_path_tests { use super::*; + #[test] + fn ancestor_collision_reports_the_first_pair_in_input_order() { + let paths = ["assets", "assets/item.txt", "libs", "libs/child.fabro"] + .map(|path| WorkflowPath::new(path).unwrap()); + for _ in 0..32 { + let error = validate_workflow_source_paths(paths.iter()).unwrap_err(); + assert_eq!( + error.to_string(), + "workflow paths collide: `assets` and `assets/item.txt`" + ); + let version = WorkflowVersion::new( + paths[1].clone(), + paths.iter().map(|p| (p.clone(), String::new())).collect(), + BTreeMap::new(), + ) + .unwrap_err(); + assert_eq!(version.to_string(), error.to_string()); + } + } + #[test] fn workflow_source_collisions_are_portable_in_both_orders() { for pair in [ From 4b9db2602738a0363ace3962c37a91402a7929f2 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:24:44 -0600 Subject: [PATCH 06/15] Keep portable path validation out of the checkout collector validate_workflow_source_paths ran inside workflow_files for every collected version, including the pre-existing checkout callers behind automation materialization and `fabro run`. A repository on a case-sensitive filesystem whose graph legitimately references two paths that differ only by case or Unicode normalization packaged before this branch and would have started failing. The check is also redundant for the supplied-content path that motivated it: the tool request validates the full key set before staging, and the supplied collector confines collected keys to that set. Remove it from the collector so existing callers are unchanged. Co-Authored-By: Claude Fable 5.1 --- .../fabro-manifest/src/workflow_version_collector.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 8928cfd81..3c2fbe063 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -265,12 +265,6 @@ fn workflow_files( for (path, file) in workflow.files { insert_file(&mut files, entrypoint, workflow_path(&path)?, file.content)?; } - fabro_types::validate_workflow_source_paths(files.keys()).map_err(|source| { - WorkflowVersionCollectError::InvalidShape { - entrypoint: entrypoint.clone(), - source, - } - })?; Ok(files) } From 70b1090f58ecd7cefb2375c5d69715e74a7a88eb Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:25:34 -0600 Subject: [PATCH 07/15] Drop the duplicate-key guard that no tool route can reach FabroWorkflowVersionCreateParams deserialized `files` through a duplicate-rejecting map, but both production routes (rmcp Parameters and the native LLM tool dispatch) deserialize from an already-parsed serde_json::Value in which duplicate keys have collapsed last-wins. The only test that exercised the guard used serde_json::from_str, the one entry point production never uses, so the safeguard was misleading. Remove the attribute and its byte-level test, and drop the deserialize_unique_map export that existed only for it. The canonical WorkflowVersion wire type keeps its own duplicate-key rejection, which does run on the byte-level HTTP route. Co-Authored-By: Claude Fable 5.1 --- lib/components/fabro-tool/src/workflow_version.rs | 11 +++-------- lib/foundation/fabro-types/src/lib.rs | 2 +- lib/foundation/fabro-types/src/workflow_version.rs | 11 ----------- 3 files changed, 4 insertions(+), 20 deletions(-) diff --git a/lib/components/fabro-tool/src/workflow_version.rs b/lib/components/fabro-tool/src/workflow_version.rs index 8adb193f8..38be81c8b 100644 --- a/lib/components/fabro-tool/src/workflow_version.rs +++ b/lib/components/fabro-tool/src/workflow_version.rs @@ -21,8 +21,9 @@ pub struct FabroWorkflowVersionCreateParams { #[schemars(with = "String")] pub entrypoint: WorkflowPath, /// All local dependencies, keyed by package-relative path. Values are text - /// contents. - #[serde(deserialize_with = "fabro_types::deserialize_unique_map")] + /// contents. Tool arguments arrive as an already-parsed JSON value on + /// every production route, so duplicate keys have collapsed (last wins) + /// before this type sees them; there is no byte-level guard to add here. #[schemars(with = "BTreeMap")] pub files: BTreeMap, } @@ -127,12 +128,6 @@ mod tests { fn workflow_version_request_rejects_unknown_fields_and_invalid_paths() { let valid = json!({"entrypoint": "workflow", "files": {"workflow": "digraph W {}"}}); validate(valid.clone()).unwrap(); - assert!( - serde_json::from_str::( - r#"{"entrypoint":"workflow","files":{"workflow":"a","workflow":"b"}}"# - ) - .is_err() - ); for field in [ "cwd", "url", diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index a398b827b..e678a8a1f 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -197,7 +197,7 @@ pub use workflow_path::{ }; pub use workflow_version::{ MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_DEPENDENCIES, MAX_WORKFLOW_VERSION_FILE_BYTES, - MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, deserialize_unique_map, + MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, validate_workflow_source_paths, }; pub use workflow_version_id::{WorkflowVersionId, WorkflowVersionIdParseError}; diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index 297e485d6..a957f1b36 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -236,17 +236,6 @@ impl<'de> Deserialize<'de> for WorkflowVersion { } } -/// Deserialize a map while rejecting duplicate keys, which serde would -/// otherwise silently collapse to the last value. -pub fn deserialize_unique_map<'de, D, K, V>(deserializer: D) -> Result, D::Error> -where - D: Deserializer<'de>, - K: Deserialize<'de> + Ord + fmt::Display, - V: Deserialize<'de>, -{ - UniqueBTreeMap::deserialize(deserializer).map(|map| map.0) -} - struct UniqueBTreeMap(BTreeMap); impl<'de, K, V> Deserialize<'de> for UniqueBTreeMap From 44dccfa3d221558a1af23d6aa47b4e966ea077df Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:26:49 -0600 Subject: [PATCH 08/15] Require version config to be workflow.toml beside its graph WorkflowLocation dispatches any `.toml` path to the config loader, so a supplied entrypoint such as `sub/run.toml` was accepted, its graph became the version entrypoint, and the config file was registered under its own name. Runtime only reads WorkflowVersion::config_path(), the fixed sibling `workflow.toml`, so the version's goal, environment, and Dockerfile settings were silently dropped on every run. In workflow-version projection, reject a config whose collected path is not the graph's sibling `workflow.toml`. This applies to every caller that packages versions, including `fabro run /other.toml`, which previously registered the config and then ignored it; failing at packaging replaces a silent drop. Manifest bundling for the legacy run path does not project versions and is unchanged. Co-Authored-By: Claude Fable 5.1 --- .../fabro-manifest/src/supplied_workflow.rs | 40 +++++++++++++++++++ .../fabro-manifest/src/workflow_bundler.rs | 17 +++++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index 9f247294c..0970bf9ff 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -254,6 +254,46 @@ mod tests { } } + #[test] + fn config_entrypoint_must_be_workflow_toml_beside_its_graph() { + let config = "_version = 1\n[workflow]\ngraph = \"g.fabro\"\n[run]\ngoal = \"hello\"\n"; + let accepted = supplied("sub/workflow.toml", &[ + ("sub/workflow.toml", config), + ("sub/g.fabro", "digraph W {}"), + ]); + let root = collect(&accepted) + .versions() + .last() + .unwrap() + .1 + .version() + .clone(); + assert!(root.files().contains_key(&root.config_path())); + + // Same tree under another config name: runtime would never read it. + let renamed = supplied("sub/run.toml", &[ + ("sub/run.toml", config), + ("sub/g.fabro", "digraph W {}"), + ]); + let error = + collect_supplied_workflow_versions(&renamed.entrypoint, &renamed.files).unwrap_err(); + assert!( + format!("{error:#}").contains("must be `sub/workflow.toml`"), + "{error:#}" + ); + // A config that selects a graph in another directory is not its sibling. + let elsewhere = supplied("sub/workflow.toml", &[ + ( + "sub/workflow.toml", + "_version = 1\n[workflow]\ngraph = \"../g.fabro\"\n", + ), + ("g.fabro", "digraph W {}"), + ]); + assert!( + collect_supplied_workflow_versions(&elsewhere.entrypoint, &elsewhere.files).is_err() + ); + } + #[test] fn preserves_literal_scripts_without_executing() { let directory = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 9eb0add4a..98202fbe1 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -97,9 +97,22 @@ impl<'a> WorkflowBundler<'a> { let source = self.read_package_file(&location.graph)?; let config = if let Some(workflow_toml_path) = location.toml.as_ref() { + let config_path = manifest_path_from_absolute(workflow_toml_path, self.package_root)?; + if self.workflow_version_projection { + // A version's config is read at run time from the fixed + // sibling path only, so a config file under any other name + // would be registered and then silently ignored. + let expected = dot_path.parent_or_dot().join("workflow.toml"); + if config_path.as_path() != expected { + bail!( + "workflow configuration `{config_path}` must be `{}` beside its graph \ + `{dot_path}`", + expected.display() + ); + } + } Some(types::ManifestWorkflowConfig { - path: manifest_path_from_absolute(workflow_toml_path, self.package_root)? - .to_string(), + path: config_path.to_string(), source: self.read_package_file(workflow_toml_path)?, }) } else { From e6eb59537a0b3b7a6d0af5754e27fc4e8773db85 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:30:37 -0600 Subject: [PATCH 09/15] Move the supplied-content packager next to its collector ServerWorkflowVersionPackager was a pure adapter over fabro_manifest::collect_supplied_workflow_versions that touched no server state, yet it lived in fabro-server and was imported from there by the standalone MCP server and the CLI run worker. fabro-manifest can depend on fabro-tool without a cycle, so the adapter now lives beside the collector as SuppliedWorkflowVersionPackager and fabro-server no longer exports a non-server module for it. The adapter also cloned every version's file map out of a closure it already owned. CollectedWorkflowClosure::into_versions hands the versions over by value inside the blocking task instead. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 4 ++ lib/apps/fabro-cli/src/commands/run/runner.rs | 4 +- lib/apps/fabro-mcp-server/src/server.rs | 4 +- lib/apps/fabro-server/src/lib.rs | 1 - lib/components/fabro-manifest/Cargo.toml | 4 ++ lib/components/fabro-manifest/src/lib.rs | 2 + .../src/workflow_version_collector.rs | 10 +++++ .../src/workflow_version_packager.rs} | 42 ++++++++++--------- 8 files changed, 46 insertions(+), 25 deletions(-) rename lib/{apps/fabro-server/src/workflow_version_tool.rs => components/fabro-manifest/src/workflow_version_packager.rs} (77%) diff --git a/Cargo.lock b/Cargo.lock index 8923ac91a..2de971b13 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2754,12 +2754,14 @@ name = "fabro-manifest" version = "0.354.0-nightly.0" dependencies = [ "anyhow", + "async-trait", "fabro-api", "fabro-config", "fabro-github", "fabro-graphviz", "fabro-template", "fabro-test", + "fabro-tool", "fabro-types", "fabro-util", "fabro-workflow", @@ -2770,7 +2772,9 @@ dependencies = [ "temp-env", "tempfile", "thiserror 2.0.18", + "tokio", "toml 0.8.23", + "tracing", ] [[package]] diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 36f149e3a..506f2d83a 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -15,8 +15,8 @@ use fabro_interview::{ WORKER_CONTROL_WS_PING_INTERVAL, WorkerControlDeliveryFrame, WorkerControlEnvelope, WorkerControlMessage, }; +use fabro_manifest::SuppliedWorkflowVersionPackager; use fabro_server::run_tool_manifest; -use fabro_server::workflow_version_tool::ServerWorkflowVersionPackager; use fabro_store::{EventEnvelope, RunProjection, RunProjectionReducer}; use fabro_tool::fabro_client::ClientBackend; use fabro_types::settings::run::{RunMode, RunNamespace}; @@ -238,7 +238,7 @@ fn build_fabro_run_tool_services( } let backend = ClientBackend::new(Arc::new(client)) .with_manifest_builder(Arc::new(WorkerRunManifestBuilder)) - .with_workflow_version_packager(Arc::new(ServerWorkflowVersionPackager)); + .with_workflow_version_packager(Arc::new(SuppliedWorkflowVersionPackager)); Some(FabroRunToolServices { backend: Arc::new(backend), current_run_id, diff --git a/lib/apps/fabro-mcp-server/src/server.rs b/lib/apps/fabro-mcp-server/src/server.rs index 914833318..fba1fc5ac 100644 --- a/lib/apps/fabro-mcp-server/src/server.rs +++ b/lib/apps/fabro-mcp-server/src/server.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Result; -use fabro_server::workflow_version_tool::ServerWorkflowVersionPackager; +use fabro_manifest::SuppliedWorkflowVersionPackager; use fabro_tool::fabro_client::ClientBackend; use fabro_tool::{self as run_tools, FabroToolBackend}; use fabro_util::version::FABRO_VERSION; @@ -277,7 +277,7 @@ impl FabroMcpServer { ClientBackend::new(Arc::new(client)) .with_manifest_builder(Arc::new(McpRunManifestBuilder)) .with_workflow_version_packager(Arc::new( - ServerWorkflowVersionPackager, + SuppliedWorkflowVersionPackager, )), ) as Arc }) diff --git a/lib/apps/fabro-server/src/lib.rs b/lib/apps/fabro-server/src/lib.rs index 227855389..98f7a2c77 100644 --- a/lib/apps/fabro-server/src/lib.rs +++ b/lib/apps/fabro-server/src/lib.rs @@ -55,7 +55,6 @@ pub mod web_auth; mod worker_control; mod worker_runtime; mod worker_token; -pub mod workflow_version_tool; pub use error::{ApiError, Error, Result}; pub use run_manifest::workflow_bundle_from_manifest; diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index dcb395052..62c6b2d0b 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -14,18 +14,22 @@ workspace = true [dependencies] anyhow.workspace = true +async-trait.workspace = true fabro-api = { path = "../../foundation/fabro-api" } fabro-config = { path = "../../foundation/fabro-config" } fabro-github = { path = "../fabro-github" } fabro-graphviz = { path = "../fabro-graphviz" } fabro-template = { path = "../../foundation/fabro-template" } +fabro-tool = { path = "../fabro-tool" } fabro-types = { path = "../../foundation/fabro-types" } fabro-workflow = { path = "../fabro-workflow" } fabro-workflow-version = { path = "../fabro-workflow-version" } git2.workspace = true tempfile = "3" thiserror.workspace = true +tokio.workspace = true toml.workspace = true +tracing.workspace = true [dev-dependencies] fabro-test.workspace = true diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index 9f46f2be7..bac761ffb 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -7,6 +7,7 @@ mod local_workflow_package; mod supplied_workflow; mod workflow_bundler; mod workflow_version_collector; +mod workflow_version_packager; use std::collections::HashMap; use std::path::{Component, Path, PathBuf}; @@ -41,6 +42,7 @@ pub use crate::workflow_version_collector::{ CollectedWorkflowClosure, WorkflowVersionCollectError, collect_workflow_versions, collect_workflow_versions_at_location, }; +pub use crate::workflow_version_packager::SuppliedWorkflowVersionPackager; #[derive(Debug, Default)] pub struct ManifestBuildInput { diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 3c2fbe063..3c7dd95e4 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -31,6 +31,16 @@ impl CollectedWorkflowClosure { ) -> impl Iterator + '_ { self.versions.iter().map(|(id, version)| (*id, version)) } + + /// Consume the closure, yielding every version with dependencies before + /// parents, for callers that hand the versions on without cloning. + #[must_use] + pub fn into_versions(self) -> Vec { + self.versions + .into_iter() + .map(|(_, version)| version.into_version()) + .collect() + } } #[derive(Debug, Error)] diff --git a/lib/apps/fabro-server/src/workflow_version_tool.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs similarity index 77% rename from lib/apps/fabro-server/src/workflow_version_tool.rs rename to lib/components/fabro-manifest/src/workflow_version_packager.rs index 816c526b5..16a4f51a5 100644 --- a/lib/apps/fabro-server/src/workflow_version_tool.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -1,3 +1,6 @@ +//! Application adapter that packages caller-supplied workflow contents for +//! the `fabro_workflow_version_create` tool. + use async_trait::async_trait; use fabro_tool::{ PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, @@ -7,35 +10,34 @@ use tracing::warn; /// Packages supplied workflow contents for standalone MCP and capable run /// workers; the backend that owns the API client performs registration. -pub struct ServerWorkflowVersionPackager; +pub struct SuppliedWorkflowVersionPackager; const PACKAGING_FAILED: &str = "workflow source could not be packaged; check configuration, \ syntax, local references, and package limits"; #[async_trait] -impl WorkflowVersionPackager for ServerWorkflowVersionPackager { +impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { async fn package( &self, source: ValidatedWorkflowVersionCreate, ) -> anyhow::Result { - let closure = task::spawn_blocking(move || { - fabro_manifest::collect_supplied_workflow_versions(&source.entrypoint, &source.files) + task::spawn_blocking(move || { + let closure = + crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) + .map_err(|err| { + // Parser diagnostics may quote supplied source, so the + // chain stays in the log and only a generic message + // crosses the tool boundary. + warn!(error = %format!("{err:#}"), "workflow version packaging failed"); + ToolError::message(PACKAGING_FAILED) + })?; + Ok(PackagedWorkflowVersions { + root_id: closure.root_id(), + versions: closure.into_versions(), + }) }) .await .map_err(|err| anyhow::anyhow!("workflow packaging task failed: {err}"))? - .map_err(|err| { - // Parser diagnostics may quote supplied source, so the chain stays - // in the log and only a generic message crosses the tool boundary. - warn!(error = %format!("{err:#}"), "workflow version packaging failed"); - ToolError::message(PACKAGING_FAILED) - })?; - Ok(PackagedWorkflowVersions { - root_id: closure.root_id(), - versions: closure - .versions() - .map(|(_, version)| version.version().clone()) - .collect(), - }) } } @@ -70,7 +72,7 @@ mod tests { #[tokio::test] async fn packager_returns_dependencies_before_root() { - let packaged = ServerWorkflowVersionPackager + let packaged = SuppliedWorkflowVersionPackager .package(fixture()) .await .unwrap(); @@ -112,7 +114,7 @@ mod tests { "PRIVATE_CONTENT invalid source", )]), ] { - let error = ServerWorkflowVersionPackager + let error = SuppliedWorkflowVersionPackager .package(input) .await .unwrap_err(); @@ -129,7 +131,7 @@ mod tests { .files .insert("Prompt.md".parse().unwrap(), prompt); assert!( - ServerWorkflowVersionPackager + SuppliedWorkflowVersionPackager .package(wrong_case) .await .is_err(), From 9bc6bf42277372e19a09580e2124f86ed63af8bc Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:34:49 -0600 Subject: [PATCH 10/15] Surface path-only packaging failures to the tool caller Every packaging failure collapsed into one generic message, so an LLM caller that omitted a child workflow, referenced a prompt with the wrong case, or exceeded the canonical size limit could not tell what to fix. The collector's error type already separates variants whose messages carry only paths and counts from the ones whose sources quote supplied content. collect_supplied_workflow_versions now returns the typed collector error, with new variants for a referenced file missing from the package root, a collected file the caller did not supply, and staging I/O failures. The bundler reports missing files with their package-relative path so the collector can recognize them. The packager renders the full cause chain for path-only variants and stops at the last path-only level, plus a hint, for graph, TOML, and template failures whose diagnostics quote source. The tool-side raw byte total remains a cheap lower bound; the canonical limit now surfaces with its own message instead of the generic one. Co-Authored-By: Claude Fable 5.1 --- lib/components/fabro-manifest/Cargo.toml | 2 +- .../fabro-manifest/src/supplied_workflow.rs | 47 +++++--- .../fabro-manifest/src/workflow_bundler.rs | 20 +++- .../src/workflow_version_collector.rs | 34 +++++- .../src/workflow_version_packager.rs | 101 ++++++++++++++---- 5 files changed, 159 insertions(+), 45 deletions(-) diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index 62c6b2d0b..dfeba6f01 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -22,6 +22,7 @@ fabro-graphviz = { path = "../fabro-graphviz" } fabro-template = { path = "../../foundation/fabro-template" } fabro-tool = { path = "../fabro-tool" } fabro-types = { path = "../../foundation/fabro-types" } +fabro-util = { path = "../../foundation/fabro-util" } fabro-workflow = { path = "../fabro-workflow" } fabro-workflow-version = { path = "../fabro-workflow-version" } git2.workspace = true @@ -33,7 +34,6 @@ tracing.workspace = true [dev-dependencies] fabro-test.workspace = true -fabro-util = { path = "../../foundation/fabro-util" } insta.workspace = true serde_json.workspace = true temp-env = "0.3" diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index 0970bf9ff..e74b533c7 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -4,12 +4,13 @@ use std::collections::BTreeMap; use std::path::Path; -use anyhow::Result; use fabro_config::project::WorkflowLocation; use fabro_types::WorkflowPath; use tempfile::TempDir; -use crate::CollectedWorkflowClosure; +use crate::{CollectedWorkflowClosure, WorkflowVersionCollectError}; + +type Result = std::result::Result; /// Stage `files` in a private temporary directory and collect the workflow /// closure rooted at `entrypoint` with the same collector used for checkouts. @@ -22,34 +23,49 @@ pub fn collect_supplied_workflow_versions( ) -> Result { let staging = tempfile::Builder::new() .prefix("fabro-workflow-version-") - .tempdir()?; + .tempdir() + .map_err(stage_error)?; collect_in_staging(entrypoint, files, &staging) } +fn stage_error(source: std::io::Error) -> WorkflowVersionCollectError { + WorkflowVersionCollectError::Stage { source } +} + fn collect_in_staging( entrypoint: &WorkflowPath, files: &BTreeMap, staging: &TempDir, ) -> Result { - let root = staging.path().canonicalize()?; + let root = staging.path().canonicalize().map_err(stage_error)?; for (path, contents) in files { let destination = root.join(path.as_str()); if let Some(parent) = destination.parent() { - std::fs::create_dir_all(parent)?; + std::fs::create_dir_all(parent).map_err(stage_error)?; } - std::fs::write(destination, contents)?; + std::fs::write(destination, contents).map_err(stage_error)?; } let entrypoint = Path::new(entrypoint.as_str()); - let location = WorkflowLocation::from_exact_path(entrypoint, &root)?; + let location = + WorkflowLocation::from_exact_path(entrypoint, &root).map_err(|source| match source { + fabro_config::Error::WorkflowNotFound(_) => { + WorkflowVersionCollectError::WorkflowNotFound { + path: entrypoint.to_path_buf(), + } + } + source => WorkflowVersionCollectError::Collect { + path: entrypoint.to_path_buf(), + source: source.into(), + }, + })?; let closure = crate::collect_workflow_versions_at_location(&location, &root, entrypoint)?; // A case-insensitive host must not satisfy a reference that is missing // from the supplied tree under its exact key. for (_, version) in closure.versions() { for path in version.version().files().keys() { - anyhow::ensure!( - files.contains_key(path), - "collected file `{path}` was not supplied" - ); + if !files.contains_key(path) { + return Err(WorkflowVersionCollectError::NotSupplied { path: path.clone() }); + } } } Ok(closure) @@ -59,6 +75,8 @@ fn collect_in_staging( mod tests { use std::path::Path; + use fabro_util::error::collect_chain; + use super::*; struct Supplied { @@ -243,7 +261,7 @@ mod tests { let error = collect_with_staging(&input, staging) .err() .unwrap_or_else(|| panic!("accepted invalid fixture {index}")); - let rendered = format!("{error:#}"); + let rendered = collect_chain(&error).join(": "); // Escaping references must fail before any host file is opened, // so no host diagnostic (parse error, exists-vs-missing) leaks. assert!( @@ -277,9 +295,10 @@ mod tests { ]); let error = collect_supplied_workflow_versions(&renamed.entrypoint, &renamed.files).unwrap_err(); + let rendered = collect_chain(&error).join(": "); assert!( - format!("{error:#}").contains("must be `sub/workflow.toml`"), - "{error:#}" + rendered.contains("must be `sub/workflow.toml`"), + "{rendered}" ); // A config that selects a graph in another directory is not its sibling. let elsewhere = supplied("sub/workflow.toml", &[ diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 98202fbe1..f79a15d20 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -38,6 +38,15 @@ pub(super) struct CollectedWorkflowSource { pub(super) dependency_keys: BTreeSet, } +/// A referenced file that does not exist under the package root, reported +/// with its package-relative path so callers can surface it without the +/// staging directory or any file content. +#[derive(Debug, thiserror::Error)] +#[error("workflow package file `{path}` is missing")] +pub(super) struct MissingPackageFile { + pub(super) path: String, +} + impl<'a> WorkflowBundler<'a> { pub(super) fn new(package_root: &'a Path, inputs: &'a HashMap) -> Self { Self { @@ -512,11 +521,16 @@ impl<'a> WorkflowBundler<'a> { return std::fs::read_to_string(path) .with_context(|| format!("Failed to read {}", path.display())); } - let canonical = path.canonicalize().with_context(|| { - format!( + let canonical = path.canonicalize().map_err(|source| { + if source.kind() == std::io::ErrorKind::NotFound { + let path = ManifestPath::from_absolute(path, self.package_root) + .map_or_else(|| path.display().to_string(), |path| path.to_string()); + return anyhow::Error::new(MissingPackageFile { path }); + } + anyhow::Error::new(source).context(format!( "failed to canonicalize workflow package file `{}`", path.display() - ) + )) })?; if !canonical.starts_with(self.package_root) { bail!( diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 3c7dd95e4..c6437b170 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -10,7 +10,9 @@ use fabro_types::{ use fabro_workflow_version::{ValidatedWorkflowVersion, WorkflowVersionError}; use thiserror::Error; -use crate::workflow_bundler::{CollectedWorkflowSource, CollectedWorkflowSources, WorkflowBundler}; +use crate::workflow_bundler::{ + CollectedWorkflowSource, CollectedWorkflowSources, MissingPackageFile, WorkflowBundler, +}; /// One locally packaged workflow-version closure in dependency-first order. #[derive(Debug)] @@ -80,6 +82,20 @@ pub enum WorkflowVersionCollectError { DependencyCycle { path: WorkflowPath }, #[error("collected workflow dependency `{path}` is missing")] MissingWorkflow { path: String }, + /// A referenced file is absent from the package root. Surfaced separately + /// from [`Self::Collect`] because the path is the whole message. + #[error("referenced file `{path}` is missing from the workflow source")] + MissingPackageFile { path: String }, + /// Supplied-content packaging: the collector read a file whose exact key + /// the caller did not supply (a case-insensitive host satisfied a + /// reference that differs from the supplied key). + #[error("collected file `{path}` was not supplied")] + NotSupplied { path: WorkflowPath }, + #[error("failed to stage supplied workflow files")] + Stage { + #[source] + source: std::io::Error, + }, } /// Package one workflow and every separately runnable dependency from a local @@ -157,9 +173,19 @@ pub fn collect_workflow_versions_at_location( let inputs = HashMap::new(); let collected = WorkflowBundler::new(package_root, &inputs) .collect_versions(location) - .map_err(|source| WorkflowVersionCollectError::Collect { - path: workflow.to_path_buf(), - source, + .map_err(|source| { + let missing = source + .chain() + .find_map(|cause| cause.downcast_ref::()); + match missing { + Some(missing) => WorkflowVersionCollectError::MissingPackageFile { + path: missing.path.clone(), + }, + None => WorkflowVersionCollectError::Collect { + path: workflow.to_path_buf(), + source, + }, + } })?; VersionAssembler::new(collected).assemble() } diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index 16a4f51a5..eecacaa2e 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -5,15 +5,17 @@ use async_trait::async_trait; use fabro_tool::{ PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, }; +use fabro_workflow_version::WorkflowVersionError; use tokio::task; use tracing::warn; +use crate::WorkflowVersionCollectError; + /// Packages supplied workflow contents for standalone MCP and capable run /// workers; the backend that owns the API client performs registration. pub struct SuppliedWorkflowVersionPackager; -const PACKAGING_FAILED: &str = "workflow source could not be packaged; check configuration, \ - syntax, local references, and package limits"; +const PACKAGING_HINT: &str = "check configuration, syntax, local references, and package limits"; #[async_trait] impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { @@ -25,11 +27,8 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { let closure = crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) .map_err(|err| { - // Parser diagnostics may quote supplied source, so the - // chain stays in the log and only a generic message - // crosses the tool boundary. warn!(error = %format!("{err:#}"), "workflow version packaging failed"); - ToolError::message(PACKAGING_FAILED) + ToolError::message(render_packaging_error(&err)) })?; Ok(PackagedWorkflowVersions { root_id: closure.root_id(), @@ -41,6 +40,34 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { } } +/// Render a packaging failure for the tool caller. Every collector variant's +/// own message names paths and counts only, so most render their full cause +/// chain and the caller can fix the input. The graph parser, TOML parser, and +/// template engine quote the offending source in their diagnostics, so +/// failures that reach them stop at the last path-only level and add a hint. +fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { + let quotes_source = match err { + WorkflowVersionCollectError::Collect { .. } => true, + WorkflowVersionCollectError::InvalidVersion { source, .. } => matches!( + source, + WorkflowVersionError::GraphParse { .. } + | WorkflowVersionError::Template { .. } + | WorkflowVersionError::Config { .. } + ), + _ => false, + }; + if !quotes_source { + return fabro_util::error::collect_chain(err).join(": "); + } + let summary = match err { + // `WorkflowVersionError` names the offending path; only its source + // quotes content. + WorkflowVersionCollectError::InvalidVersion { source, .. } => format!("{err}: {source}"), + _ => err.to_string(), + }; + format!("{summary}; {PACKAGING_HINT}") +} + #[cfg(test)] mod tests { use super::*; @@ -70,6 +97,14 @@ mod tests { ]) } + async fn package_error(input: ValidatedWorkflowVersionCreate) -> String { + let error = SuppliedWorkflowVersionPackager + .package(input) + .await + .unwrap_err(); + format!("{error:#}") + } + #[tokio::test] async fn packager_returns_dependencies_before_root() { let packaged = SuppliedWorkflowVersionPackager @@ -101,27 +136,34 @@ mod tests { "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run.goal]\nfile = \"missing.md\"" .into(), ); - let mut oversized = fixture(); - oversized.files.insert( - "prompt.md".parse().unwrap(), - "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1), + let mut invalid_config = fixture(); + invalid_config.files.insert( + "workflow.toml".parse().unwrap(), + "_version = 1\nPRIVATE_CONTENT = [unterminated".into(), ); for input in [ invalid_root, - oversized, + invalid_config, source("workflow", &[( "workflow", "PRIVATE_CONTENT invalid source", )]), ] { - let error = SuppliedWorkflowVersionPackager - .package(input) - .await - .unwrap_err(); - let rendered = format!("{error:#}"); + let rendered = package_error(input).await; assert!(!rendered.contains("PRIVATE_CONTENT"), "{rendered}"); - assert_eq!(rendered, PACKAGING_FAILED); } + } + + #[tokio::test] + async fn path_only_failures_tell_the_caller_what_to_fix() { + let mut missing_child = fixture(); + missing_child.files.remove(&"child.fabro".parse().unwrap()); + let rendered = package_error(missing_child).await; + assert!( + rendered.contains("`child.fabro`") && rendered.contains("missing"), + "{rendered}" + ); + let mut wrong_case = fixture(); let prompt = wrong_case .files @@ -130,12 +172,25 @@ mod tests { wrong_case .files .insert("Prompt.md".parse().unwrap(), prompt); - assert!( - SuppliedWorkflowVersionPackager - .package(wrong_case) - .await - .is_err(), - "a case-insensitive host must not satisfy an exact reference" + // A case-insensitive host reads the file and reports the unsupplied + // key; a case-sensitive host reports it missing. Either names the + // path the graph asked for. + let rendered = package_error(wrong_case).await; + assert!(rendered.contains("`prompt.md`"), "{rendered}"); + + let mut oversized = fixture(); + oversized.files.insert( + "prompt.md".parse().unwrap(), + "\u{1}".repeat(fabro_types::MAX_WORKFLOW_VERSION_FILE_BYTES - 1), ); + let rendered = package_error(oversized).await; + assert!(rendered.contains("canonical bytes"), "{rendered}"); + + let rendered = package_error(source("workflow", &[( + "workflow", + "PRIVATE_CONTENT invalid source", + )])) + .await; + assert!(rendered.ends_with(PACKAGING_HINT), "{rendered}"); } } From 97a7bb06e59dad88c0a01d8b9cdc5cd09330017d Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:37:39 -0600 Subject: [PATCH 11/15] Pin sibling config resolution to the supplied file keys Supplied files are staged on the host filesystem and WorkflowLocation probes the fixed sibling name `workflow.toml` there. A request that supplied `Workflow.toml` beside its graph therefore attached the config on a case-insensitive host (and then failed the not-supplied check), while the identical request on ext4 registered a version with no config. The outcome of a content-addressed registration depended on the server's filesystem. After collection, every version is checked against the supplied map: when no exact sibling `workflow.toml` was supplied, no supplied key may alias that name under the same case and normalization rules the tool already applies to supplied keys. A supplied sibling config still attaches only to the graph it selects, matching checkouts, since several graphs may share one directory. Co-Authored-By: Claude Fable 5.1 --- .../fabro-manifest/src/supplied_workflow.rs | 113 ++++++++++++++++-- .../src/workflow_version_collector.rs | 8 ++ 2 files changed, 114 insertions(+), 7 deletions(-) diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index e74b533c7..db95dac8e 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -59,18 +59,50 @@ fn collect_in_staging( }, })?; let closure = crate::collect_workflow_versions_at_location(&location, &root, entrypoint)?; - // A case-insensitive host must not satisfy a reference that is missing - // from the supplied tree under its exact key. for (_, version) in closure.versions() { - for path in version.version().files().keys() { - if !files.contains_key(path) { - return Err(WorkflowVersionCollectError::NotSupplied { path: path.clone() }); - } - } + confine_to_supplied(version.version(), files)?; } Ok(closure) } +/// The collector resolves references against the staged tree, so its result +/// can depend on the host filesystem's case and normalization rules. Reject +/// every version whose collected files are not exactly the supplied keys, and +/// pin the one implicit probe (the sibling `workflow.toml`) to the supplied +/// map so the same request packages identically on every host. +fn confine_to_supplied( + version: &fabro_types::WorkflowVersion, + files: &BTreeMap, +) -> Result<()> { + // A supplied sibling config attaches only when its `[workflow].graph` + // selects this entrypoint, exactly as for a checkout; several graphs may + // share one directory. When no exact sibling was supplied, the probe must + // not find one either. + let config_path = version.config_path(); + if !files.contains_key(&config_path) { + let alias = files.keys().find(|path| { + fabro_types::validate_workflow_source_paths([*path, &config_path]).is_err() + }); + if let Some(alias) = alias { + // A case-insensitive host would probe this key as the config and + // a case-sensitive one would not; neither outcome is what was + // asked for. + return Err(WorkflowVersionCollectError::ConfigAlias { + config_path, + alias: alias.clone(), + }); + } + } + // A case-insensitive host must not satisfy a reference that is missing + // from the supplied tree under its exact key. + for path in version.files().keys() { + if !files.contains_key(path) { + return Err(WorkflowVersionCollectError::NotSupplied { path: path.clone() }); + } + } + Ok(()) +} + #[cfg(test)] mod tests { use std::path::Path; @@ -313,6 +345,73 @@ mod tests { ); } + #[test] + fn sibling_config_resolution_does_not_depend_on_the_host_filesystem() { + let graph = r#"digraph W { child [stack.child_workflow="sub/child.fabro"] }"#; + let selects = |target: &str| format!("_version = 1\n[workflow]\ngraph = \"{target}\"\n"); + + // Exact sibling configs attach to the root and to the child. + let attached = supplied("workflow.fabro", &[ + ("workflow.fabro", graph), + ("workflow.toml", &selects("workflow.fabro")), + ("sub/child.fabro", "digraph Child {}"), + ("sub/workflow.toml", &selects("child.fabro")), + ]); + for (_, version) in collect(&attached).versions() { + let version = version.version(); + assert!( + version.files().contains_key(&version.config_path()), + "{} lost its config", + version.entrypoint() + ); + } + + // A case variant of the implicit probe name is rejected everywhere, + // instead of attaching on APFS and vanishing on ext4. + for (entrypoint, files) in [ + ("workflow.fabro", vec![ + ("workflow.fabro", graph.to_owned()), + ("Workflow.toml", selects("workflow.fabro")), + ("sub/child.fabro", "digraph Child {}".to_owned()), + ]), + ("workflow.fabro", vec![ + ("workflow.fabro", graph.to_owned()), + ("sub/child.fabro", "digraph Child {}".to_owned()), + ("sub/WORKFLOW.toml", selects("child.fabro")), + ]), + ] { + let files = files + .iter() + .map(|(path, content)| (*path, content.as_str())) + .collect::>(); + let input = supplied(entrypoint, &files); + let error = + collect_supplied_workflow_versions(&input.entrypoint, &input.files).unwrap_err(); + assert!( + matches!(error, WorkflowVersionCollectError::ConfigAlias { .. }), + "{error}" + ); + } + + // Several graphs may share a directory: a sibling config attaches only + // to the graph it selects, as for a checkout. + let shared_dir = supplied("workflow.fabro", &[ + ("workflow.fabro", graph), + ("sub/child.fabro", "digraph Child {}"), + ("sub/other.fabro", "digraph Other {}"), + ("sub/workflow.toml", &selects("other.fabro")), + ]); + let closure = collect(&shared_dir); + let (_, child) = closure.versions().next().unwrap(); + assert_eq!(child.version().entrypoint().as_str(), "sub/child.fabro"); + assert!( + !child + .version() + .files() + .contains_key(&child.version().config_path()) + ); + } + #[test] fn preserves_literal_scripts_without_executing() { let directory = tempfile::tempdir().unwrap(); diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index c6437b170..57faa0d43 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -91,6 +91,14 @@ pub enum WorkflowVersionCollectError { /// reference that differs from the supplied key). #[error("collected file `{path}` was not supplied")] NotSupplied { path: WorkflowPath }, + /// Supplied-content packaging: a supplied key differs from the implicit + /// sibling config name `{config_path}` only by case or normalization, so + /// hosts with different filesystem rules would package different trees. + #[error("supplied file `{alias}` aliases the workflow config name `{config_path}`")] + ConfigAlias { + config_path: WorkflowPath, + alias: WorkflowPath, + }, #[error("failed to stage supplied workflow files")] Stage { #[source] From 709d15f9083433987b3f1a57921941e4a658a044 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 15:38:59 -0600 Subject: [PATCH 12/15] Keep supplied workflow source out of packaging failure logs The packager logged the full packaging error chain at WARN. That chain embeds caller-supplied workflow and prompt source: the graph parser's diagnostic includes the unparsed remainder and the TOML parser prints the offending line. The logging strategy prohibits user file contents in tracing events at every level, and this adapter runs inside `fabro mcp` and run workers at the default filter. Log the collector error's own path-only message at DEBUG, since a malformed request is an expected input error, together with the entrypoint and file count. Wrap the blocking-task join error with `context` instead of interpolating it. A test installs a TRACE-level subscriber around the blocking path and checks that the fixture's source marker, which the full chain does contain, never reaches the log. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 1 + lib/components/fabro-manifest/Cargo.toml | 1 + .../src/workflow_version_packager.rs | 113 +++++++++++++++--- 3 files changed, 99 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2de971b13..f45bc947f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2775,6 +2775,7 @@ dependencies = [ "tokio", "toml 0.8.23", "tracing", + "tracing-subscriber", ] [[package]] diff --git a/lib/components/fabro-manifest/Cargo.toml b/lib/components/fabro-manifest/Cargo.toml index dfeba6f01..edc222c0f 100644 --- a/lib/components/fabro-manifest/Cargo.toml +++ b/lib/components/fabro-manifest/Cargo.toml @@ -34,6 +34,7 @@ tracing.workspace = true [dev-dependencies] fabro-test.workspace = true +tracing-subscriber.workspace = true insta.workspace = true serde_json.workspace = true temp-env = "0.3" diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index eecacaa2e..54cb39521 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -1,13 +1,15 @@ //! Application adapter that packages caller-supplied workflow contents for //! the `fabro_workflow_version_create` tool. +use anyhow::Context as _; use async_trait::async_trait; use fabro_tool::{ PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, }; +use fabro_util::error::collect_chain; use fabro_workflow_version::WorkflowVersionError; use tokio::task; -use tracing::warn; +use tracing::debug; use crate::WorkflowVersionCollectError; @@ -23,23 +25,38 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { &self, source: ValidatedWorkflowVersionCreate, ) -> anyhow::Result { - task::spawn_blocking(move || { - let closure = - crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) - .map_err(|err| { - warn!(error = %format!("{err:#}"), "workflow version packaging failed"); - ToolError::message(render_packaging_error(&err)) - })?; - Ok(PackagedWorkflowVersions { - root_id: closure.root_id(), - versions: closure.into_versions(), - }) - }) - .await - .map_err(|err| anyhow::anyhow!("workflow packaging task failed: {err}"))? + let packaged = task::spawn_blocking(move || package_blocking(&source)) + .await + .context("workflow packaging task failed")??; + Ok(packaged) } } +/// Stage, collect, and validate on the calling thread. +/// +/// Packaging failures are expected input errors, so they log at DEBUG. The +/// event carries the collector error's own message only: parser and template +/// diagnostics further down the chain quote supplied file contents, which +/// must not reach the log at any level. +fn package_blocking( + source: &ValidatedWorkflowVersionCreate, +) -> Result { + let closure = crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) + .map_err(|err| { + debug!( + entrypoint = %source.entrypoint, + file_count = source.files.len(), + error = %err, + "workflow version packaging failed" + ); + ToolError::message(render_packaging_error(&err)) + })?; + Ok(PackagedWorkflowVersions { + root_id: closure.root_id(), + versions: closure.into_versions(), + }) +} + /// Render a packaging failure for the tool caller. Every collector variant's /// own message names paths and counts only, so most render their full cause /// chain and the caller can fix the input. The graph parser, TOML parser, and @@ -57,7 +74,7 @@ fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { _ => false, }; if !quotes_source { - return fabro_util::error::collect_chain(err).join(": "); + return collect_chain(err).join(": "); } let summary = match err { // `WorkflowVersionError` names the offending path; only its source @@ -70,8 +87,37 @@ fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { #[cfg(test)] mod tests { + #[expect( + clippy::disallowed_types, + reason = "test log capture writes synchronously into memory" + )] + use std::io::Write; + use std::sync::{Arc, Mutex}; + + use tracing::{Level, subscriber}; + use super::*; + #[derive(Clone, Default)] + struct CapturedLog(Arc>>); + + impl Write for CapturedLog { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl CapturedLog { + fn text(&self) -> String { + String::from_utf8(self.0.lock().unwrap().clone()).unwrap() + } + } + fn source(entrypoint: &str, files: &[(&str, &str)]) -> ValidatedWorkflowVersionCreate { ValidatedWorkflowVersionCreate { entrypoint: entrypoint.parse().unwrap(), @@ -154,6 +200,41 @@ mod tests { } } + #[test] + fn packaging_failure_log_never_carries_supplied_source() { + let log = CapturedLog::default(); + let writer = log.clone(); + let subscriber = tracing_subscriber::fmt() + .with_max_level(Level::TRACE) + .with_ansi(false) + .with_writer(move || writer.clone()) + .finish(); + let inputs = [ + source("workflow", &[( + "workflow", + "PRIVATE_CONTENT invalid source", + )]), + source("workflow.toml", &[( + "workflow.toml", + "_version = 1\nPRIVATE_CONTENT = [unterminated", + )]), + ]; + // The guard is load-bearing: the full chain does quote the source. + let leaky = + crate::collect_supplied_workflow_versions(&inputs[0].entrypoint, &inputs[0].files) + .unwrap_err(); + assert!(collect_chain(&leaky).join(": ").contains("PRIVATE_CONTENT")); + subscriber::with_default(subscriber, || { + for input in &inputs { + package_blocking(input).unwrap_err(); + } + }); + let text = log.text(); + assert!(text.contains("workflow version packaging failed"), "{text}"); + assert!(!text.contains("PRIVATE_CONTENT"), "{text}"); + assert!(text.contains("DEBUG"), "{text}"); + } + #[tokio::test] async fn path_only_failures_tell_the_caller_what_to_fix() { let mut missing_child = fixture(); From 410651d7814d184e1bba9d72903ce662584dfeea Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 11 Sep 2026 18:57:39 -0600 Subject: [PATCH 13/15] Bound workflow packaging depth and reject invalid supplied configs --- docs/public/agents/mcp.mdx | 5 +- lib/components/fabro-manifest/src/lib.rs | 4 +- .../fabro-manifest/src/supplied_workflow.rs | 13 ++- .../fabro-manifest/src/workflow_bundler.rs | 35 +++++-- .../src/workflow_version_collector.rs | 68 +++++++++++++ .../src/workflow_version_packager.rs | 98 ++++++++++++++++++- 6 files changed, 211 insertions(+), 12 deletions(-) diff --git a/docs/public/agents/mcp.mdx b/docs/public/agents/mcp.mdx index 9ee9bf5a1..e769a4bf3 100644 --- a/docs/public/agents/mcp.mdx +++ b/docs/public/agents/mcp.mdx @@ -75,7 +75,10 @@ values are text, never host paths or URLs to fetch. Missing references and paths that escape the supplied tree fail. The source tree is limited to 512 files, 512 KiB per file, and 2 MiB of text; each resulting serialized version must also fit the existing 2 MiB API limit. Case-insensitive file and ancestor collisions -are rejected before staging. Collection follows declared file references; command +are rejected before staging. Graph nesting through child workflows and imports +is limited to 64 levels, including the entrypoint. A supplied sibling +`workflow.toml` must be valid even when a graph is the entrypoint; a valid config +that selects another graph is omitted. Collection follows declared file references; command `script` values remain literal text, and paths embedded in shell commands are not inspected or acquired. diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index bac761ffb..bfd5f0162 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -39,8 +39,8 @@ pub use crate::local_workflow_package::{ pub use crate::supplied_workflow::collect_supplied_workflow_versions; use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ - CollectedWorkflowClosure, WorkflowVersionCollectError, collect_workflow_versions, - collect_workflow_versions_at_location, + CollectedWorkflowClosure, MAX_WORKFLOW_VERSION_DEPTH, WorkflowVersionCollectError, + collect_workflow_versions, collect_workflow_versions_at_location, }; pub use crate::workflow_version_packager::SuppliedWorkflowVersionPackager; diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index db95dac8e..064516f64 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -60,7 +60,7 @@ fn collect_in_staging( })?; let closure = crate::collect_workflow_versions_at_location(&location, &root, entrypoint)?; for (_, version) in closure.versions() { - confine_to_supplied(version.version(), files)?; + confine_to_supplied(version.version(), files, &root)?; } Ok(closure) } @@ -73,6 +73,7 @@ fn collect_in_staging( fn confine_to_supplied( version: &fabro_types::WorkflowVersion, files: &BTreeMap, + root: &Path, ) -> Result<()> { // A supplied sibling config attaches only when its `[workflow].graph` // selects this entrypoint, exactly as for a checkout; several graphs may @@ -92,6 +93,16 @@ fn confine_to_supplied( alias: alias.clone(), }); } + } else if !version.files().contains_key(&config_path) { + // Graph discovery deliberately ignores sibling configs that it cannot + // load. A supplied config may be omitted only if it is valid and selects + // a different graph; malformed settings must not silently disappear. + WorkflowLocation::from_exact_path(Path::new(config_path.as_str()), root).map_err( + |source| WorkflowVersionCollectError::InvalidSuppliedConfig { + path: config_path, + source: Box::new(source), + }, + )?; } // A case-insensitive host must not satisfy a reference that is missing // from the supplied tree under its exact key. diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index f79a15d20..2c56a1310 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -17,7 +17,7 @@ use fabro_template::{ use fabro_types::ManifestPath; use fabro_types::graph::ReferenceKind; -use crate::{manifest_path_from_absolute, normalize_absolute_path}; +use crate::{manifest_path_from_absolute, normalize_absolute_path, workflow_version_collector}; pub(super) struct WorkflowBundler<'a> { package_root: &'a Path, @@ -64,7 +64,7 @@ impl<'a> WorkflowBundler<'a> { workflow: &Path, project_config: Option<(&ManifestPath, &str)>, ) -> Result> { - let root_key = self.collect_workflow_entry(workflow, self.package_root)?; + let root_key = self.collect_workflow_entry(workflow, self.package_root, 1)?; if let Some((config_path, source)) = project_config { let mut root = self @@ -89,7 +89,7 @@ impl<'a> WorkflowBundler<'a> { root: &WorkflowLocation, ) -> Result { self.workflow_version_projection = true; - let root_key = self.collect_workflow_location(root)?; + let root_key = self.collect_workflow_location(root, 1)?; Ok(CollectedWorkflowSources { root_key, workflows: self.workflows, @@ -97,12 +97,19 @@ impl<'a> WorkflowBundler<'a> { } /// Collects the workflow at `location` and returns its manifest key. - fn collect_workflow_location(&mut self, location: &WorkflowLocation) -> Result { + fn collect_workflow_location( + &mut self, + location: &WorkflowLocation, + depth: usize, + ) -> Result { let dot_path = manifest_path_from_absolute(&location.graph, self.package_root)?; let dot_key = dot_path.to_string(); if !self.visited_workflows.insert(dot_key.clone()) { return Ok(dot_key); } + if self.workflow_version_projection { + workflow_version_collector::check_workflow_depth(depth, &dot_key)?; + } let source = self.read_package_file(&location.graph)?; let config = if let Some(workflow_toml_path) = location.toml.as_ref() { @@ -147,6 +154,7 @@ impl<'a> WorkflowBundler<'a> { &mut visited_imports, &mut dependency_keys, GraphPosition::Entrypoint, + depth, )?; self.workflows @@ -168,7 +176,12 @@ impl<'a> WorkflowBundler<'a> { /// key. Workflow-version projection normalizes every reference and /// resolves it as an exact path inside the package root, with no /// workflow-name lookup. Returns the collected workflow's manifest key. - fn collect_workflow_entry(&mut self, workflow: &Path, resolve_from: &Path) -> Result { + fn collect_workflow_entry( + &mut self, + workflow: &Path, + resolve_from: &Path, + depth: usize, + ) -> Result { let normalize = self.workflow_version_projection || (workflow.extension().is_some() && workflow.is_relative()); let normalized = if normalize { @@ -197,7 +210,7 @@ impl<'a> WorkflowBundler<'a> { } else { WorkflowLocation::resolve(&normalized, resolve_from)? }; - self.collect_workflow_location(&location) + self.collect_workflow_location(&location, depth) } fn collect_workflow_files( @@ -207,7 +220,14 @@ impl<'a> WorkflowBundler<'a> { visited_imports: &mut HashSet, dependency_keys: &mut BTreeSet, position: GraphPosition, + depth: usize, ) -> Result<()> { + if self.workflow_version_projection { + workflow_version_collector::check_workflow_depth( + depth, + &workflow.dot_path.to_string(), + )?; + } let graph = parser::parse(&workflow.source) .with_context(|| format!("Failed to parse {}", workflow.absolute_dot_path.display()))?; let workflow_base_dir = workflow @@ -305,12 +325,13 @@ impl<'a> WorkflowBundler<'a> { visited_imports, dependency_keys, GraphPosition::Imported, + depth + 1, )?; } } for child in children { let dependency_key = - self.collect_workflow_entry(Path::new(child), workflow_base_dir)?; + self.collect_workflow_entry(Path::new(child), workflow_base_dir, depth + 1)?; dependency_keys.insert(dependency_key); } diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index 57faa0d43..b3667d808 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -14,6 +14,23 @@ use crate::workflow_bundler::{ CollectedWorkflowSource, CollectedWorkflowSources, MissingPackageFile, WorkflowBundler, }; +/// Maximum active graph nesting while collecting or assembling a version. +/// Bounds native stack use independently of file-count and byte budgets. +pub const MAX_WORKFLOW_VERSION_DEPTH: usize = 64; + +pub(super) fn check_workflow_depth( + depth: usize, + path: &str, +) -> Result<(), WorkflowVersionCollectError> { + if depth > MAX_WORKFLOW_VERSION_DEPTH { + return Err(WorkflowVersionCollectError::DepthExceeded { + path: path.to_owned(), + maximum: MAX_WORKFLOW_VERSION_DEPTH, + }); + } + Ok(()) +} + /// One locally packaged workflow-version closure in dependency-first order. #[derive(Debug)] pub struct CollectedWorkflowClosure { @@ -47,6 +64,8 @@ impl CollectedWorkflowClosure { #[derive(Debug, Error)] pub enum WorkflowVersionCollectError { + #[error("workflow dependency nesting at `{path}` exceeds {maximum} levels")] + DepthExceeded { path: String, maximum: usize }, #[error("workflow `{path}` was not found")] WorkflowNotFound { path: PathBuf }, #[error("failed to collect workflow `{path}`")] @@ -99,6 +118,12 @@ pub enum WorkflowVersionCollectError { config_path: WorkflowPath, alias: WorkflowPath, }, + #[error("supplied workflow configuration `{path}` is invalid")] + InvalidSuppliedConfig { + path: WorkflowPath, + #[source] + source: Box, + }, #[error("failed to stage supplied workflow files")] Stage { #[source] @@ -182,6 +207,10 @@ pub fn collect_workflow_versions_at_location( let collected = WorkflowBundler::new(package_root, &inputs) .collect_versions(location) .map_err(|source| { + let source = match source.downcast::() { + Ok(error) => return error, + Err(source) => source, + }; let missing = source .chain() .find_map(|cause| cause.downcast_ref::()); @@ -244,6 +273,7 @@ impl VersionAssembler { if let Some(id) = self.ids.get(key) { return Ok(*id); } + check_workflow_depth(self.visiting.len() + 1, key)?; if !self.visiting.insert(key.to_owned()) { return Err(WorkflowVersionCollectError::DependencyCycle { path: workflow_path(key)?, @@ -349,6 +379,44 @@ mod tests { use super::*; + #[test] + fn version_assembly_bounds_its_own_dependency_traversal() { + // Collection and assembly visit shared dependencies in different orders. + // Assembly must bound its stack even if collection already cached nodes. + for count in [MAX_WORKFLOW_VERSION_DEPTH, MAX_WORKFLOW_VERSION_DEPTH + 1] { + let workflows = (0..count) + .map(|index| { + let child = (index + 1 < count).then(|| format!("f{}.fabro", index + 1)); + let source = child.as_ref().map_or_else( + || "digraph W {}".to_owned(), + |child| format!("digraph W {{ child [stack.child_workflow=\"{child}\"] }}"), + ); + (format!("f{index}.fabro"), CollectedWorkflowSource { + workflow: types::ManifestWorkflow { + config: None, + files: HashMap::new(), + source, + }, + dependency_keys: child.into_iter().collect(), + }) + }) + .collect(); + let result = VersionAssembler::new(CollectedWorkflowSources { + root_key: "f0.fabro".to_owned(), + workflows, + }) + .assemble(); + if count == MAX_WORKFLOW_VERSION_DEPTH { + assert_eq!(result.unwrap().versions().count(), count); + } else { + assert!(matches!( + result, + Err(WorkflowVersionCollectError::DepthExceeded { .. }) + )); + } + } + } + fn write(root: &Path, path: &str, content: &str) { let path = root.join(path); fs::create_dir_all(path.parent().expect("fixture path should have a parent")).unwrap(); diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index 54cb39521..c13067f5f 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -64,7 +64,8 @@ fn package_blocking( /// failures that reach them stop at the last path-only level and add a hint. fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { let quotes_source = match err { - WorkflowVersionCollectError::Collect { .. } => true, + WorkflowVersionCollectError::Collect { .. } + | WorkflowVersionCollectError::InvalidSuppliedConfig { .. } => true, WorkflowVersionCollectError::InvalidVersion { source, .. } => matches!( source, WorkflowVersionError::GraphParse { .. } @@ -87,6 +88,7 @@ fn render_packaging_error(err: &WorkflowVersionCollectError) -> String { #[cfg(test)] mod tests { + use std::collections::BTreeMap; #[expect( clippy::disallowed_types, reason = "test log capture writes synchronously into memory" @@ -151,6 +153,93 @@ mod tests { format!("{error:#}") } + #[tokio::test] + async fn deeply_nested_graphs_return_errors_on_the_packaging_thread() { + for kind in ["child", "import", "mixed"] { + for count in [ + crate::MAX_WORKFLOW_VERSION_DEPTH, + crate::MAX_WORKFLOW_VERSION_DEPTH + 1, + 384, + 512, + ] { + let files: BTreeMap<_, _> = (0..count) + .map(|index| { + let attribute = if kind == "import" || (kind == "mixed" && index % 2 == 0) { + "import" + } else { + "stack.child_workflow" + }; + let graph = if index + 1 < count { + format!( + "digraph W {{ node{index} [{attribute}=\"f{}.fabro\"] }}", + index + 1 + ) + } else { + "digraph W {}".to_owned() + }; + (format!("f{index}.fabro").parse().unwrap(), graph) + }) + .collect(); + let input = ValidatedWorkflowVersionCreate::try_from( + fabro_tool::FabroWorkflowVersionCreateParams { + entrypoint: "f0.fabro".parse().unwrap(), + files, + }, + ) + .unwrap(); + let result = SuppliedWorkflowVersionPackager.package(input).await; + if count == crate::MAX_WORKFLOW_VERSION_DEPTH { + assert!(result.is_ok(), "{kind} at limit: {result:?}"); + } else { + let error = result.unwrap_err(); + assert!( + error.to_string().contains("exceeds 64 levels"), + "{kind}/{count}: {error:#}" + ); + } + } + } + } + + #[tokio::test] + async fn invalid_supplied_sibling_configs_fail_without_quoting_source() { + for config in [ + "_version = 1\nPRIVATE_CONTENT = [unterminated", + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n[run]\ngoal = \"PRIVATE_CONTENT\"\nunknown_setting = true\n", + ] { + for child in [false, true] { + let input = if child { + source("root.fabro", &[ + ( + "root.fabro", + r#"digraph W { child [stack.child_workflow="sub/workflow.fabro"] }"#, + ), + ("sub/workflow.fabro", "digraph Child {}"), + ("sub/workflow.toml", config), + ]) + } else { + source("workflow.fabro", &[ + ("workflow.fabro", "digraph W {}"), + ("workflow.toml", config), + ]) + }; + let error = package_error(input).await; + let path = if child { + "sub/workflow.toml" + } else { + "workflow.toml" + }; + assert!( + error.contains(&format!( + "supplied workflow configuration `{path}` is invalid" + )), + "{error}" + ); + assert!(!error.contains("PRIVATE_CONTENT"), "{error}"); + } + } + } + #[tokio::test] async fn packager_returns_dependencies_before_root() { let packaged = SuppliedWorkflowVersionPackager @@ -218,6 +307,13 @@ mod tests { "workflow.toml", "_version = 1\nPRIVATE_CONTENT = [unterminated", )]), + source("workflow.fabro", &[ + ("workflow.fabro", "digraph W {}"), + ( + "workflow.toml", + "_version = 1\nPRIVATE_CONTENT = [unterminated", + ), + ]), ]; // The guard is load-bearing: the full chain does quote the source. let leaky = From ec81170c8ad15a79c4f73578867b9ed1d4ead4cd Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 12 Sep 2026 10:14:53 -0600 Subject: [PATCH 14/15] Share workflow packaging results and validation across callers --- Cargo.lock | 1 + lib/apps/fabro-cli/tests/it/cmd/mcp.rs | 2 +- lib/components/fabro-manifest/src/lib.rs | 7 ++- .../fabro-manifest/src/supplied_workflow.rs | 35 +---------- .../fabro-manifest/src/test_support.rs | 29 +++++++++ .../fabro-manifest/src/workflow_bundler.rs | 18 +++--- .../src/workflow_version_collector.rs | 63 ++++--------------- .../src/workflow_version_packager.rs | 56 +++++------------ lib/components/fabro-tool/Cargo.toml | 1 + lib/components/fabro-tool/src/fabro_client.rs | 46 +++++++++----- lib/components/fabro-tool/src/lib.rs | 4 +- .../fabro-tool/src/workflow_version.rs | 43 +++---------- .../fabro-workflow-version/src/closure.rs | 38 +++++++++++ .../fabro-workflow-version/src/lib.rs | 3 +- lib/foundation/fabro-types/src/lib.rs | 2 +- .../fabro-types/src/workflow_version.rs | 51 +++++++++------ 16 files changed, 184 insertions(+), 215 deletions(-) create mode 100644 lib/components/fabro-manifest/src/test_support.rs create mode 100644 lib/components/fabro-workflow-version/src/closure.rs diff --git a/Cargo.lock b/Cargo.lock index f45bc947f..a84eec268 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3173,6 +3173,7 @@ dependencies = [ "fabro-client", "fabro-types", "fabro-util", + "fabro-workflow-version", "futures", "httpmock", "schemars 1.2.1", diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index d791334b4..64181a159 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -1858,7 +1858,7 @@ async fn mcp_workflow_version_validation_happens_before_auth_or_network() { serde_json::json!({"entrypoint":"workflow","files":{}}), ) .await; - assert_eq!(error, "entrypoint must be an exact supplied file key"); + assert_eq!(error, "entrypoint `workflow` is not present in workflow files"); assert_mcp_run_tool_count(&client).await; client .shutdown() diff --git a/lib/components/fabro-manifest/src/lib.rs b/lib/components/fabro-manifest/src/lib.rs index bfd5f0162..f020b5828 100644 --- a/lib/components/fabro-manifest/src/lib.rs +++ b/lib/components/fabro-manifest/src/lib.rs @@ -5,6 +5,8 @@ mod local_workflow_package; mod supplied_workflow; +#[cfg(test)] +mod test_support; mod workflow_bundler; mod workflow_version_collector; mod workflow_version_packager; @@ -32,6 +34,7 @@ use fabro_types::{ WorkflowSettings, }; use fabro_workflow::git::{self, GitSyncStatus}; +pub use fabro_workflow_version::CollectedWorkflowClosure; pub use crate::local_workflow_package::{ LocalWorkflowPackageError, ResolvedLocalWorkflowPackage, resolve_local_workflow_package, @@ -39,8 +42,8 @@ pub use crate::local_workflow_package::{ pub use crate::supplied_workflow::collect_supplied_workflow_versions; use crate::workflow_bundler::WorkflowBundler; pub use crate::workflow_version_collector::{ - CollectedWorkflowClosure, MAX_WORKFLOW_VERSION_DEPTH, WorkflowVersionCollectError, - collect_workflow_versions, collect_workflow_versions_at_location, + MAX_WORKFLOW_VERSION_DEPTH, WorkflowVersionCollectError, collect_workflow_versions, + collect_workflow_versions_at_location, }; pub use crate::workflow_version_packager::SuppliedWorkflowVersionPackager; diff --git a/lib/components/fabro-manifest/src/supplied_workflow.rs b/lib/components/fabro-manifest/src/supplied_workflow.rs index 064516f64..64f824328 100644 --- a/lib/components/fabro-manifest/src/supplied_workflow.rs +++ b/lib/components/fabro-manifest/src/supplied_workflow.rs @@ -118,42 +118,11 @@ fn confine_to_supplied( mod tests { use std::path::Path; + use fabro_tool::ValidatedWorkflowVersionCreate as Supplied; use fabro_util::error::collect_chain; use super::*; - - struct Supplied { - entrypoint: WorkflowPath, - files: BTreeMap, - } - - fn supplied(entrypoint: &str, files: &[(&str, &str)]) -> Supplied { - Supplied { - entrypoint: entrypoint.parse().unwrap(), - files: files - .iter() - .map(|(path, content)| (path.parse().unwrap(), (*content).to_string())) - .collect(), - } - } - - fn fixture() -> Supplied { - supplied("workflow.toml", &[ - ( - "workflow.toml", - "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", - ), - ( - "workflow.fabro", - r#"digraph W { p [prompt="@prompt.md"] child [stack.child_workflow="child.fabro"] }"#, - ), - ( - "prompt.md", - "Keep {{ secrets.TEST }} and {{ env.TEST }} for runtime.", - ), - ("child.fabro", "digraph Child {}"), - ]) - } + use crate::test_support::{fixture, source as supplied}; fn collect(input: &Supplied) -> CollectedWorkflowClosure { collect_supplied_workflow_versions(&input.entrypoint, &input.files).unwrap() diff --git a/lib/components/fabro-manifest/src/test_support.rs b/lib/components/fabro-manifest/src/test_support.rs new file mode 100644 index 000000000..8ee12307a --- /dev/null +++ b/lib/components/fabro-manifest/src/test_support.rs @@ -0,0 +1,29 @@ +use fabro_tool::ValidatedWorkflowVersionCreate; + +pub(super) fn source(entrypoint: &str, files: &[(&str, &str)]) -> ValidatedWorkflowVersionCreate { + ValidatedWorkflowVersionCreate { + entrypoint: entrypoint.parse().unwrap(), + files: files + .iter() + .map(|(path, content)| (path.parse().unwrap(), (*content).to_string())) + .collect(), + } +} + +pub(super) fn fixture() -> ValidatedWorkflowVersionCreate { + source("workflow.toml", &[ + ( + "workflow.toml", + "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", + ), + ( + "workflow.fabro", + r#"digraph W { p [prompt="@prompt.md"] child [stack.child_workflow="child.fabro"] }"#, + ), + ( + "prompt.md", + "Keep {{ secrets.TEST }} and {{ env.TEST }} for runtime.", + ), + ("child.fabro", "digraph Child {}"), + ]) +} diff --git a/lib/components/fabro-manifest/src/workflow_bundler.rs b/lib/components/fabro-manifest/src/workflow_bundler.rs index 2c56a1310..da1588bcd 100644 --- a/lib/components/fabro-manifest/src/workflow_bundler.rs +++ b/lib/components/fabro-manifest/src/workflow_bundler.rs @@ -17,7 +17,10 @@ use fabro_template::{ use fabro_types::ManifestPath; use fabro_types::graph::ReferenceKind; -use crate::{manifest_path_from_absolute, normalize_absolute_path, workflow_version_collector}; +use crate::{ + WorkflowVersionCollectError, manifest_path_from_absolute, normalize_absolute_path, + workflow_version_collector, +}; pub(super) struct WorkflowBundler<'a> { package_root: &'a Path, @@ -38,15 +41,6 @@ pub(super) struct CollectedWorkflowSource { pub(super) dependency_keys: BTreeSet, } -/// A referenced file that does not exist under the package root, reported -/// with its package-relative path so callers can surface it without the -/// staging directory or any file content. -#[derive(Debug, thiserror::Error)] -#[error("workflow package file `{path}` is missing")] -pub(super) struct MissingPackageFile { - pub(super) path: String, -} - impl<'a> WorkflowBundler<'a> { pub(super) fn new(package_root: &'a Path, inputs: &'a HashMap) -> Self { Self { @@ -546,7 +540,9 @@ impl<'a> WorkflowBundler<'a> { if source.kind() == std::io::ErrorKind::NotFound { let path = ManifestPath::from_absolute(path, self.package_root) .map_or_else(|| path.display().to_string(), |path| path.to_string()); - return anyhow::Error::new(MissingPackageFile { path }); + return anyhow::Error::new(WorkflowVersionCollectError::MissingPackageFile { + path, + }); } anyhow::Error::new(source).context(format!( "failed to canonicalize workflow package file `{}`", diff --git a/lib/components/fabro-manifest/src/workflow_version_collector.rs b/lib/components/fabro-manifest/src/workflow_version_collector.rs index b3667d808..131f9990c 100644 --- a/lib/components/fabro-manifest/src/workflow_version_collector.rs +++ b/lib/components/fabro-manifest/src/workflow_version_collector.rs @@ -7,12 +7,12 @@ use fabro_types::{ WorkflowPath, WorkflowPathParseError, WorkflowVersion, WorkflowVersionId, WorkflowVersionShapeError, }; -use fabro_workflow_version::{ValidatedWorkflowVersion, WorkflowVersionError}; +use fabro_workflow_version::{ + CollectedWorkflowClosure, ValidatedWorkflowVersion, WorkflowVersionError, +}; use thiserror::Error; -use crate::workflow_bundler::{ - CollectedWorkflowSource, CollectedWorkflowSources, MissingPackageFile, WorkflowBundler, -}; +use crate::workflow_bundler::{CollectedWorkflowSource, CollectedWorkflowSources, WorkflowBundler}; /// Maximum active graph nesting while collecting or assembling a version. /// Bounds native stack use independently of file-count and byte budgets. @@ -31,37 +31,6 @@ pub(super) fn check_workflow_depth( Ok(()) } -/// One locally packaged workflow-version closure in dependency-first order. -#[derive(Debug)] -pub struct CollectedWorkflowClosure { - root_id: WorkflowVersionId, - versions: Vec<(WorkflowVersionId, ValidatedWorkflowVersion)>, -} - -impl CollectedWorkflowClosure { - #[must_use] - pub fn root_id(&self) -> WorkflowVersionId { - self.root_id - } - - /// Iterate over every unique version with dependencies before parents. - pub fn versions( - &self, - ) -> impl Iterator + '_ { - self.versions.iter().map(|(id, version)| (*id, version)) - } - - /// Consume the closure, yielding every version with dependencies before - /// parents, for callers that hand the versions on without cloning. - #[must_use] - pub fn into_versions(self) -> Vec { - self.versions - .into_iter() - .map(|(_, version)| version.into_version()) - .collect() - } -} - #[derive(Debug, Error)] pub enum WorkflowVersionCollectError { #[error("workflow dependency nesting at `{path}` exceeds {maximum} levels")] @@ -207,22 +176,12 @@ pub fn collect_workflow_versions_at_location( let collected = WorkflowBundler::new(package_root, &inputs) .collect_versions(location) .map_err(|source| { - let source = match source.downcast::() { - Ok(error) => return error, - Err(source) => source, - }; - let missing = source - .chain() - .find_map(|cause| cause.downcast_ref::()); - match missing { - Some(missing) => WorkflowVersionCollectError::MissingPackageFile { - path: missing.path.clone(), - }, - None => WorkflowVersionCollectError::Collect { + source + .downcast::() + .unwrap_or_else(|source| WorkflowVersionCollectError::Collect { path: workflow.to_path_buf(), source, - }, - } + }) })?; VersionAssembler::new(collected).assemble() } @@ -260,10 +219,10 @@ impl VersionAssembler { fn assemble(mut self) -> Result { let root_key = std::mem::take(&mut self.root_key); let root_id = self.assemble_one(&root_key)?; - Ok(CollectedWorkflowClosure { + Ok(CollectedWorkflowClosure::from_dependency_order( root_id, - versions: self.versions, - }) + self.versions, + )) } fn assemble_one( diff --git a/lib/components/fabro-manifest/src/workflow_version_packager.rs b/lib/components/fabro-manifest/src/workflow_version_packager.rs index c13067f5f..566161a9a 100644 --- a/lib/components/fabro-manifest/src/workflow_version_packager.rs +++ b/lib/components/fabro-manifest/src/workflow_version_packager.rs @@ -3,11 +3,9 @@ use anyhow::Context as _; use async_trait::async_trait; -use fabro_tool::{ - PackagedWorkflowVersions, ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, -}; +use fabro_tool::{ToolError, ValidatedWorkflowVersionCreate, WorkflowVersionPackager}; use fabro_util::error::collect_chain; -use fabro_workflow_version::WorkflowVersionError; +use fabro_workflow_version::{CollectedWorkflowClosure, WorkflowVersionError}; use tokio::task; use tracing::debug; @@ -24,7 +22,7 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { async fn package( &self, source: ValidatedWorkflowVersionCreate, - ) -> anyhow::Result { + ) -> anyhow::Result { let packaged = task::spawn_blocking(move || package_blocking(&source)) .await .context("workflow packaging task failed")??; @@ -40,7 +38,7 @@ impl WorkflowVersionPackager for SuppliedWorkflowVersionPackager { /// must not reach the log at any level. fn package_blocking( source: &ValidatedWorkflowVersionCreate, -) -> Result { +) -> Result { let closure = crate::collect_supplied_workflow_versions(&source.entrypoint, &source.files) .map_err(|err| { debug!( @@ -51,10 +49,7 @@ fn package_blocking( ); ToolError::message(render_packaging_error(&err)) })?; - Ok(PackagedWorkflowVersions { - root_id: closure.root_id(), - versions: closure.into_versions(), - }) + Ok(closure) } /// Render a packaging failure for the tool caller. Every collector variant's @@ -120,30 +115,7 @@ mod tests { } } - fn source(entrypoint: &str, files: &[(&str, &str)]) -> ValidatedWorkflowVersionCreate { - ValidatedWorkflowVersionCreate { - entrypoint: entrypoint.parse().unwrap(), - files: files - .iter() - .map(|(path, content)| (path.parse().unwrap(), (*content).to_string())) - .collect(), - } - } - - fn fixture() -> ValidatedWorkflowVersionCreate { - source("workflow.toml", &[ - ( - "workflow.toml", - "_version = 1\n[workflow]\ngraph = \"workflow.fabro\"\n", - ), - ( - "workflow.fabro", - r#"digraph W { p [prompt="@prompt.md"] child [stack.child_workflow="child.fabro"] }"#, - ), - ("prompt.md", "Review the implementation."), - ("child.fabro", "digraph Child {}"), - ]) - } + use crate::test_support::{fixture, source}; async fn package_error(input: ValidatedWorkflowVersionCreate) -> String { let error = SuppliedWorkflowVersionPackager @@ -246,16 +218,20 @@ mod tests { .package(fixture()) .await .unwrap(); - assert_eq!(packaged.versions.len(), 2); - assert_eq!(packaged.versions[0].entrypoint().as_str(), "child.fabro"); + let versions = packaged + .versions() + .map(|(_, v)| v.version()) + .collect::>(); + assert_eq!(versions.len(), 2); + assert_eq!(versions[0].entrypoint().as_str(), "child.fabro"); assert_eq!( - packaged.versions[1].id().unwrap(), - packaged.root_id, + versions[1].id().unwrap(), + packaged.root_id(), "root version must be last" ); - let child_id = packaged.versions[0].id().unwrap(); + let child_id = versions[0].id().unwrap(); assert!( - packaged.versions[1] + versions[1] .workflow_dependencies() .values() .any(|id| *id == child_id) diff --git a/lib/components/fabro-tool/Cargo.toml b/lib/components/fabro-tool/Cargo.toml index 892b7a6a7..1e5395b9e 100644 --- a/lib/components/fabro-tool/Cargo.toml +++ b/lib/components/fabro-tool/Cargo.toml @@ -20,6 +20,7 @@ fabro-api = { path = "../../foundation/fabro-api" } fabro-client = { path = "../../foundation/fabro-client" } fabro-types = { path = "../../foundation/fabro-types" } fabro-util = { path = "../../foundation/fabro-util" } +fabro-workflow-version = { path = "../fabro-workflow-version" } futures.workspace = true schemars = "1.2.1" serde.workspace = true diff --git a/lib/components/fabro-tool/src/fabro_client.rs b/lib/components/fabro-tool/src/fabro_client.rs index d01ebb1bd..120176183 100644 --- a/lib/components/fabro-tool/src/fabro_client.rs +++ b/lib/components/fabro-tool/src/fabro_client.rs @@ -82,10 +82,12 @@ impl FabroToolBackend for ClientBackend { .as_ref() .ok_or_else(common::workflow_version_tool_unavailable_error)?; let packaged = packager.package(source).await?; - self.client - .register_workflow_versions(&packaged.versions) - .await?; - Ok(packaged.root_id) + let versions = packaged + .versions() + .map(|(_, v)| v.version()) + .collect::>(); + self.client.register_workflow_versions(versions).await?; + Ok(packaged.root_id()) } async fn create_run_from_spec( @@ -292,22 +294,29 @@ mod tests { use async_trait::async_trait; use fabro_types::{WorkflowVersion, WorkflowVersionId}; + use fabro_workflow_version::{CollectedWorkflowClosure, ValidatedWorkflowVersion}; use serde_json::json; use super::*; - use crate::{ - PackagedWorkflowVersions, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, - }; + use crate::{ValidatedWorkflowVersionCreate, WorkflowVersionPackager}; - struct FixedPackager(PackagedWorkflowVersions); + struct FixedPackager(Vec); #[async_trait] impl WorkflowVersionPackager for FixedPackager { async fn package( &self, _: ValidatedWorkflowVersionCreate, - ) -> anyhow::Result { - Ok(self.0.clone()) + ) -> anyhow::Result { + let versions = self + .0 + .iter() + .map(|v| Ok((v.id()?, ValidatedWorkflowVersion::new(v.clone())?))) + .collect::>>()?; + Ok(CollectedWorkflowClosure::from_dependency_order( + versions.last().unwrap().0, + versions, + )) } } @@ -319,7 +328,14 @@ mod tests { entrypoint.parse().unwrap(), BTreeMap::from([( entrypoint.parse().unwrap(), - format!("digraph {entrypoint} {{}}"), + format!( + "digraph {entrypoint} {{ {} }}", + dependencies + .keys() + .map(|p| format!("child [stack.child_workflow=\"{p}\"]")) + .collect::>() + .join(" ") + ), )]), dependencies, ) @@ -361,12 +377,8 @@ mod tests { }) .await; let client = ::fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); - let backend = ClientBackend::new(Arc::new(client)).with_workflow_version_packager( - Arc::new(FixedPackager(PackagedWorkflowVersions { - root_id, - versions: vec![child, root.clone()], - })), - ); + let backend = ClientBackend::new(Arc::new(client)) + .with_workflow_version_packager(Arc::new(FixedPackager(vec![child, root.clone()]))); assert!(backend.create_workflow_version(source()).await.is_err()); child_upload.assert_calls_async(1).await; diff --git a/lib/components/fabro-tool/src/lib.rs b/lib/components/fabro-tool/src/lib.rs index d3ade2e3e..5145e7608 100644 --- a/lib/components/fabro-tool/src/lib.rs +++ b/lib/components/fabro-tool/src/lib.rs @@ -49,6 +49,6 @@ pub use search::{ search_runs, search_runs_text, }; pub use workflow_version::{ - FabroWorkflowVersionCreateParams, PackagedWorkflowVersions, ValidatedWorkflowVersionCreate, - WorkflowVersionPackager, create_workflow_version, workflow_version_create_text, + FabroWorkflowVersionCreateParams, ValidatedWorkflowVersionCreate, WorkflowVersionPackager, + create_workflow_version, workflow_version_create_text, }; diff --git a/lib/components/fabro-tool/src/workflow_version.rs b/lib/components/fabro-tool/src/workflow_version.rs index 38be81c8b..5a3e51fcf 100644 --- a/lib/components/fabro-tool/src/workflow_version.rs +++ b/lib/components/fabro-tool/src/workflow_version.rs @@ -3,10 +3,8 @@ use std::sync::Arc; use async_trait::async_trait; use fabro_api::types::CreateWorkflowVersionResponse; -use fabro_types::{ - MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES, - WorkflowPath, WorkflowVersion, WorkflowVersionId, -}; +use fabro_types::{MAX_WORKFLOW_VERSION_BYTES, WorkflowPath}; +use fabro_workflow_version::CollectedWorkflowClosure; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -41,26 +39,9 @@ impl TryFrom for ValidatedWorkflowVersionCreat fn try_from(params: FabroWorkflowVersionCreateParams) -> Result { let FabroWorkflowVersionCreateParams { entrypoint, files } = params; - if !files.contains_key(&entrypoint) { - return Err(ToolError::message( - "entrypoint must be an exact supplied file key", - )); - } - if files.len() > MAX_WORKFLOW_VERSION_FILES { - return Err(ToolError::message(format!( - "workflow source exceeds {MAX_WORKFLOW_VERSION_FILES} files" - ))); - } - let mut total = 0; - for content in files.values() { - if content.len() > MAX_WORKFLOW_VERSION_FILE_BYTES { - return Err(ToolError::message(format!( - "workflow source file exceeds {} KiB", - MAX_WORKFLOW_VERSION_FILE_BYTES / 1024 - ))); - } - total += content.len(); - } + fabro_types::validate_workflow_files(&entrypoint, &files) + .map_err(|err| ToolError::message(err.to_string()))?; + let total: usize = files.values().map(String::len).sum(); if total > MAX_WORKFLOW_VERSION_BYTES { return Err(ToolError::message(format!( "workflow source exceeds {} MiB", @@ -73,15 +54,6 @@ impl TryFrom for ValidatedWorkflowVersionCreat } } -/// The complete validated closure for one supplied source tree. -#[derive(Clone, Debug)] -pub struct PackagedWorkflowVersions { - pub root_id: WorkflowVersionId, - /// Every version in the closure, dependencies before the versions that - /// reference them, so callers can register them in this order. - pub versions: Vec, -} - /// Application seam for packaging supplied content. The manifest crates that /// own collection depend on this crate, so the packager is injected instead. /// Implementations confine reads to supplied files and validate the entire @@ -91,7 +63,7 @@ pub trait WorkflowVersionPackager: Send + Sync { async fn package( &self, source: ValidatedWorkflowVersionCreate, - ) -> anyhow::Result; + ) -> anyhow::Result; } pub async fn create_workflow_version( @@ -114,6 +86,7 @@ pub fn workflow_version_create_text(result: &CreateWorkflowVersionResponse) -> S #[cfg(test)] mod tests { + use fabro_types::{MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES}; use serde_json::json; use super::*; @@ -227,7 +200,7 @@ mod tests { async fn package( &self, _: ValidatedWorkflowVersionCreate, - ) -> anyhow::Result { + ) -> anyhow::Result { panic!("scoped backend must not invoke the packager") } } diff --git a/lib/components/fabro-workflow-version/src/closure.rs b/lib/components/fabro-workflow-version/src/closure.rs new file mode 100644 index 000000000..0e438cba1 --- /dev/null +++ b/lib/components/fabro-workflow-version/src/closure.rs @@ -0,0 +1,38 @@ +use fabro_types::WorkflowVersionId; + +use crate::ValidatedWorkflowVersion; + +/// Validated workflow versions in dependency-first order, with the root last. +/// Owns source contents; consumers borrow versions instead of cloning them. +#[derive(Debug)] +pub struct CollectedWorkflowClosure { + root_id: WorkflowVersionId, + versions: Vec<(WorkflowVersionId, ValidatedWorkflowVersion)>, +} + +impl CollectedWorkflowClosure { + /// Assemble the result of a collector that has already ordered and + /// validated the dependency graph. The caller supplies matching IDs, + /// unique versions, and dependencies before parents, with `root_id` + /// identifying the last entry. This preserves the collector's ordering + /// without traversing or hashing again. + #[must_use] + pub fn from_dependency_order( + root_id: WorkflowVersionId, + versions: Vec<(WorkflowVersionId, ValidatedWorkflowVersion)>, + ) -> Self { + Self { root_id, versions } + } + + #[must_use] + pub fn root_id(&self) -> WorkflowVersionId { + self.root_id + } + + /// Iterate over every version with dependencies before parents. + pub fn versions( + &self, + ) -> impl Iterator + '_ { + self.versions.iter().map(|(id, version)| (*id, version)) + } +} diff --git a/lib/components/fabro-workflow-version/src/lib.rs b/lib/components/fabro-workflow-version/src/lib.rs index bf0d89a44..b9f8eb6ed 100644 --- a/lib/components/fabro-workflow-version/src/lib.rs +++ b/lib/components/fabro-workflow-version/src/lib.rs @@ -23,8 +23,9 @@ use fabro_types::settings::InterpString; use fabro_types::{ManifestPath, WorkflowPath, WorkflowPathParseError, WorkflowVersion}; use thiserror::Error; +mod closure; mod store; - +pub use closure::CollectedWorkflowClosure; pub use store::{LoadedWorkflowVersionClosure, WorkflowVersionStore, WorkflowVersionStoreError}; #[derive(Debug, Error)] diff --git a/lib/foundation/fabro-types/src/lib.rs b/lib/foundation/fabro-types/src/lib.rs index e678a8a1f..2259c3814 100644 --- a/lib/foundation/fabro-types/src/lib.rs +++ b/lib/foundation/fabro-types/src/lib.rs @@ -198,6 +198,6 @@ pub use workflow_path::{ pub use workflow_version::{ MAX_WORKFLOW_VERSION_BYTES, MAX_WORKFLOW_VERSION_DEPENDENCIES, MAX_WORKFLOW_VERSION_FILE_BYTES, MAX_WORKFLOW_VERSION_FILES, WorkflowVersion, WorkflowVersionShapeError, - validate_workflow_source_paths, + validate_workflow_files, validate_workflow_source_paths, }; pub use workflow_version_id::{WorkflowVersionId, WorkflowVersionIdParseError}; diff --git a/lib/foundation/fabro-types/src/workflow_version.rs b/lib/foundation/fabro-types/src/workflow_version.rs index a957f1b36..58fb8e0ad 100644 --- a/lib/foundation/fabro-types/src/workflow_version.rs +++ b/lib/foundation/fabro-types/src/workflow_version.rs @@ -122,32 +122,13 @@ impl WorkflowVersion { } fn validate_shape(&self) -> Result<(), WorkflowVersionShapeError> { - if self.files.len() > MAX_WORKFLOW_VERSION_FILES { - return Err(WorkflowVersionShapeError::TooManyFiles { - actual: self.files.len(), - maximum: MAX_WORKFLOW_VERSION_FILES, - }); - } + validate_workflow_files(&self.entrypoint, &self.files)?; if self.workflow_dependencies.len() > MAX_WORKFLOW_VERSION_DEPENDENCIES { return Err(WorkflowVersionShapeError::TooManyWorkflowDependencies { actual: self.workflow_dependencies.len(), maximum: MAX_WORKFLOW_VERSION_DEPENDENCIES, }); } - for (path, content) in &self.files { - if content.len() > MAX_WORKFLOW_VERSION_FILE_BYTES { - return Err(WorkflowVersionShapeError::FileTooLarge { - path: path.clone(), - actual: content.len(), - maximum: MAX_WORKFLOW_VERSION_FILE_BYTES, - }); - } - } - if !self.files.contains_key(&self.entrypoint) { - return Err(WorkflowVersionShapeError::MissingEntrypoint { - path: self.entrypoint.clone(), - }); - } self.validate_path_collisions() } @@ -159,6 +140,36 @@ impl WorkflowVersion { } } +/// Validate the file limits and entrypoint shared by source trees and versions. +/// Aggregate source bytes, canonical bytes, and path policies are checked +/// separately. +pub fn validate_workflow_files( + entrypoint: &WorkflowPath, + files: &BTreeMap, +) -> Result<(), WorkflowVersionShapeError> { + if files.len() > MAX_WORKFLOW_VERSION_FILES { + return Err(WorkflowVersionShapeError::TooManyFiles { + actual: files.len(), + maximum: MAX_WORKFLOW_VERSION_FILES, + }); + } + for (path, content) in files { + if content.len() > MAX_WORKFLOW_VERSION_FILE_BYTES { + return Err(WorkflowVersionShapeError::FileTooLarge { + path: path.clone(), + actual: content.len(), + maximum: MAX_WORKFLOW_VERSION_FILE_BYTES, + }); + } + } + if !files.contains_key(entrypoint) { + return Err(WorkflowVersionShapeError::MissingEntrypoint { + path: entrypoint.clone(), + }); + } + Ok(()) +} + /// Reject file and directory aliases before materializing a portable source /// tree, including Unicode case folding and normalization. Canonical versions /// themselves retain their exact, case-sensitive semantics. From d611ef2bb1c7fec8687de03be3823b883709ee86 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Sat, 12 Sep 2026 10:23:34 -0600 Subject: [PATCH 15/15] Port workflow version creation to Pebble native tools --- Cargo.lock | 2 + lib/apps/fabro-cli/tests/it/cmd/mcp.rs | 5 +- lib/components/fabro-workflow/Cargo.toml | 2 + .../src/handler/llm/fabro_tools.rs | 97 ++++++++++++++++++- 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a84eec268..df85b11db 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3313,6 +3313,7 @@ dependencies = [ "fabro-api", "fabro-auth", "fabro-checkpoint", + "fabro-client", "fabro-config", "fabro-core", "fabro-dump", @@ -3337,6 +3338,7 @@ dependencies = [ "fabro-validate", "fabro-vault", "fabro-workflow", + "fabro-workflow-version", "futures", "git2", "hex", diff --git a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs index 64181a159..c07daff5a 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/mcp.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/mcp.rs @@ -1858,7 +1858,10 @@ async fn mcp_workflow_version_validation_happens_before_auth_or_network() { serde_json::json!({"entrypoint":"workflow","files":{}}), ) .await; - assert_eq!(error, "entrypoint `workflow` is not present in workflow files"); + assert_eq!( + error, + "entrypoint `workflow` is not present in workflow files" + ); assert_mcp_run_tool_count(&client).await; client .shutdown() diff --git a/lib/components/fabro-workflow/Cargo.toml b/lib/components/fabro-workflow/Cargo.toml index 314a53e42..8574796f1 100644 --- a/lib/components/fabro-workflow/Cargo.toml +++ b/lib/components/fabro-workflow/Cargo.toml @@ -77,6 +77,8 @@ tempfile = "3" toml.workspace = true fabro-vault = { path = "../../foundation/fabro-vault" } [dev-dependencies] +fabro-client = { path = "../../foundation/fabro-client" } +fabro-workflow-version = { path = "../fabro-workflow-version" } fabro-llm = { path = "../fabro-llm", features = ["test-support"] } fabro-store = { path = "../fabro-store", features = ["test-support"] } fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } diff --git a/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs b/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs index 67d3df89f..c2903a64a 100644 --- a/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs +++ b/lib/components/fabro-workflow/src/handler/llm/fabro_tools.rs @@ -1,4 +1,4 @@ -//! The Fabro run tools (`fabro_run_*`) as application tools a pebble coding +//! The Fabro workflow and run tools as application tools a pebble coding //! agent can call. use std::sync::Arc; @@ -66,6 +66,15 @@ pub(crate) async fn execute_fabro_run_tool( services: &FabroRunToolServices, ) -> fabro_tool::ToolResult { match name { + fabro_tool::FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME => { + let params = + parse_fabro_tool_args::(name, args)?; + let source = fabro_tool::ValidatedWorkflowVersionCreate::try_from(params)?; + let result = + fabro_tool::create_workflow_version(Arc::clone(&services.backend), source).await?; + let summary = fabro_tool::workflow_version_create_text(&result); + render_fabro_tool_result(&summary, &result) + } fabro_tool::FABRO_RUN_CREATE_TOOL_NAME => { let params = parse_fabro_tool_args::(name, args)?; ensure_current_run_parent(¶ms, services.current_run_id)?; @@ -196,3 +205,89 @@ where })?; Ok(format!("{summary}\n{json}")) } + +#[cfg(test)] +mod tests { + use std::collections::BTreeMap; + + use async_trait::async_trait; + use fabro_tool::fabro_client::ClientBackend; + use fabro_tool::{ValidatedWorkflowVersionCreate, WorkflowVersionPackager}; + use fabro_types::WorkflowVersion; + use fabro_workflow_version::{CollectedWorkflowClosure, ValidatedWorkflowVersion}; + use serde_json::json; + + use super::*; + + struct SingleGraphPackager; + + #[async_trait] + impl WorkflowVersionPackager for SingleGraphPackager { + async fn package( + &self, + source: ValidatedWorkflowVersionCreate, + ) -> anyhow::Result { + let version = WorkflowVersion::new(source.entrypoint, source.files, BTreeMap::new())?; + let id = version.id()?; + Ok(CollectedWorkflowClosure::from_dependency_order(id, vec![( + id, + ValidatedWorkflowVersion::new(version)?, + )])) + } + } + + #[tokio::test] + async fn workflow_version_native_dispatch_registers_and_returns_version() { + let server = httpmock::MockServer::start_async().await; + let version = WorkflowVersion::new( + "workflow".parse().unwrap(), + BTreeMap::from([("workflow".parse().unwrap(), "digraph W {}".into())]), + BTreeMap::new(), + ) + .unwrap(); + let id = version.id().unwrap(); + let upload = server + .mock_async(|when, then| { + when.method(httpmock::Method::POST) + .path("/api/v1/workflow-versions") + .json_body_obj(&version); + then.status(201) + .json_body(json!({"workflow_version_id": id})); + }) + .await; + let client = fabro_client::Client::new_no_proxy(&server.url("")).unwrap(); + let services = FabroRunToolServices { + backend: Arc::new( + ClientBackend::new(Arc::new(client)) + .with_workflow_version_packager(Arc::new(SingleGraphPackager)), + ), + current_run_id: "01KRBZW4DW0000000000000002".parse().unwrap(), + base_cwd: "unused".into(), + user_settings_path: "unused".into(), + }; + let name = fabro_tool::FABRO_WORKFLOW_VERSION_CREATE_TOOL_NAME; + assert_eq!(register_named_fabro_run_tools(&services, &[name]).len(), 1); + let output = execute_fabro_run_tool( + name, + json!({"entrypoint":"workflow", "files":{"workflow":"digraph W {}"}}), + &services, + ) + .await + .unwrap(); + let (summary, body) = output.split_once('\n').unwrap(); + assert_eq!(summary, format!("Registered workflow version {id}")); + assert_eq!( + serde_json::from_str::(body).unwrap(), + json!({"workflow_version_id": id}) + ); + let error = execute_fabro_run_tool( + name, + json!({"entrypoint":"missing", "files":{"workflow":"digraph W {}"}}), + &services, + ) + .await + .unwrap_err(); + assert!(error.to_string().contains("not present")); + upload.assert_calls_async(1).await; + } +}