From 64786c1932cfceb462c4d8855a2f063f0881ce06 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 28 Jul 2026 14:29:03 -0400 Subject: [PATCH] refactor: pass the catalog by value and correct the RenderMode docs - Take `Arc` by value again through the validation entry points. `AppState::catalog()` returns an owned `Arc`, so `&state.catalog()` was cloning, borrowing the temporary, then cloning again at the leaf. Every consumer ends up owning the `Arc`, so by-value is the honest shape and it drops one clone per call. The one caller holding the catalog in a field now says `Arc::clone(&self.catalog)` explicitly. - Correct the `RenderMode` doc comment. It claimed `Strict` is "used by run-create", but run-create renders `Structural` and promotes the resulting warnings to errors itself; `Strict` has no production caller. Co-Authored-By: Claude Opus 5 (1M context) --- lib/apps/fabro-cli/src/commands/run/runner.rs | 7 +++++- .../fabro-mcp-server/src/manifest_builder.rs | 2 +- .../fabro-server/src/manifest_validation.rs | 2 +- lib/apps/fabro-server/src/run_manifest.rs | 24 +++++++++---------- .../fabro-server/src/run_tool_manifest.rs | 2 +- .../fabro-server/src/server/handler/graph.rs | 2 +- .../fabro-server/src/server/handler/runs.rs | 22 +++++++++-------- .../src/handler/manager_loop.rs | 2 +- .../fabro-workflow/src/operations/create.rs | 2 +- .../fabro-workflow/src/operations/validate.rs | 11 ++++----- .../src/transforms/variable_expansion.rs | 14 ++++++----- 11 files changed, 48 insertions(+), 42 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/runner.rs b/lib/apps/fabro-cli/src/commands/run/runner.rs index 4c6fcf578..4e6b72b8b 100644 --- a/lib/apps/fabro-cli/src/commands/run/runner.rs +++ b/lib/apps/fabro-cli/src/commands/run/runner.rs @@ -263,7 +263,12 @@ impl fabro_tool::RunManifestBuilder for WorkerRunManifestBuilder { cwd: &Path, user_settings_path: &Path, ) -> fabro_tool::ToolResult { - run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, &self.catalog) + run_tool_manifest::build_run_tool_manifest( + spec, + cwd, + user_settings_path, + Arc::clone(&self.catalog), + ) } } diff --git a/lib/apps/fabro-mcp-server/src/manifest_builder.rs b/lib/apps/fabro-mcp-server/src/manifest_builder.rs index 8c1cb8125..53e899f1e 100644 --- a/lib/apps/fabro-mcp-server/src/manifest_builder.rs +++ b/lib/apps/fabro-mcp-server/src/manifest_builder.rs @@ -32,5 +32,5 @@ fn build_mcp_run_manifest( Catalog::from_builtin_with_overrides(&llm_catalog_settings) .map_err(|err| ToolError::message(err.to_string()))?, ); - run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, &catalog) + run_tool_manifest::build_run_tool_manifest(spec, cwd, user_settings_path, catalog) } diff --git a/lib/apps/fabro-server/src/manifest_validation.rs b/lib/apps/fabro-server/src/manifest_validation.rs index 3d872954f..cf081b69f 100644 --- a/lib/apps/fabro-server/src/manifest_validation.rs +++ b/lib/apps/fabro-server/src/manifest_validation.rs @@ -25,7 +25,7 @@ pub fn validate_manifest( pub fn validate_manifest_with_catalog( manifest_run_defaults: &RunLayer, manifest: &types::RunManifest, - catalog: &Arc, + catalog: Arc, ) -> Result { let prepared = prepare(manifest_run_defaults, manifest)?; let validated = diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 76a81db51..c075b9382 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -188,7 +188,7 @@ pub(crate) fn prepare_manifest_with_environment_defaults( pub(crate) fn validate_prepared_manifest( prepared: &PreparedManifest, - catalog: &Arc, + catalog: Arc, ) -> Result { validate_prepared_manifest_with_vars(prepared, catalog, HashMap::new()) } @@ -201,7 +201,7 @@ pub(crate) fn validate_prepared_manifest_structural( pub(crate) fn validate_prepared_manifest_with_vars( prepared: &PreparedManifest, - catalog: &Arc, + catalog: Arc, vars: HashMap, ) -> Result { validate_with_catalog(manifest_validate_input(prepared, vars), catalog) @@ -209,7 +209,7 @@ pub(crate) fn validate_prepared_manifest_with_vars( pub(crate) fn validate_prepared_manifest_for_preflight( prepared: &PreparedManifest, - catalog: &Arc, + catalog: Arc, vars: HashMap, ready_providers: &[ProviderId], ) -> Result { @@ -1554,7 +1554,7 @@ digraph Demo {{ .unwrap(); let validated = validate_prepared_manifest_for_preflight( &prepared, - &state.catalog(), + state.catalog(), HashMap::new(), &ready_providers, ) @@ -1639,7 +1639,7 @@ enabled = {clone_enabled} &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); let resolved = materialize_run( prepared.settings.clone(), validated.graph(), @@ -2199,7 +2199,7 @@ name = "Control Plane" &invalid_manifest(), ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); assert!(validated.has_errors()); @@ -2244,7 +2244,7 @@ issues = "read" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); assert!(!validated.has_errors()); let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) @@ -2294,7 +2294,7 @@ id = "local" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); assert!(!validated.has_errors()); @@ -2403,7 +2403,7 @@ id = "daytona" &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); let (response, _ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) .await @@ -2471,7 +2471,7 @@ digraph Demo { &manifest, ) .unwrap(); - let validated = validate_prepared_manifest(&prepared, &test_catalog()).unwrap(); + let validated = validate_prepared_manifest(&prepared, test_catalog()).unwrap(); let (response, ok) = resolve_and_run_preflight(state.as_ref(), &prepared, &validated) .await @@ -2585,7 +2585,7 @@ digraph Demo { &manifest, ) .unwrap(); - let Err(error) = validate_prepared_manifest(&prepared, &test_catalog()) else { + let Err(error) = validate_prepared_manifest(&prepared, test_catalog()) else { panic!("unknown provider should fail static validation"); }; @@ -2652,7 +2652,7 @@ digraph Demo { assert!(ready_providers.is_empty()); let validated = validate_prepared_manifest_for_preflight( &prepared, - &state.catalog(), + state.catalog(), HashMap::new(), &ready_providers, ) diff --git a/lib/apps/fabro-server/src/run_tool_manifest.rs b/lib/apps/fabro-server/src/run_tool_manifest.rs index 66204e208..a7f1f869a 100644 --- a/lib/apps/fabro-server/src/run_tool_manifest.rs +++ b/lib/apps/fabro-server/src/run_tool_manifest.rs @@ -14,7 +14,7 @@ pub fn build_run_tool_manifest( spec: &ValidatedCreateRunSpec, cwd: &Path, user_settings_path: &Path, - catalog: &Arc, + catalog: Arc, ) -> ToolResult { let built = fabro_manifest::build_run_manifest(ManifestBuildInput { workflow: PathBuf::from(&spec.workflow), diff --git a/lib/apps/fabro-server/src/server/handler/graph.rs b/lib/apps/fabro-server/src/server/handler/graph.rs index ea95b674a..364b19cad 100644 --- a/lib/apps/fabro-server/src/server/handler/graph.rs +++ b/lib/apps/fabro-server/src/server/handler/graph.rs @@ -52,7 +52,7 @@ async fn render_graph_from_manifest( Ok(prepared) => prepared, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; - let validated = match run_manifest::validate_prepared_manifest(&prepared, &state.catalog()) { + let validated = match run_manifest::validate_prepared_manifest(&prepared, state.catalog()) { Ok(validated) => validated, Err(err) => return ApiError::bad_request(err.to_string()).into_response(), }; diff --git a/lib/apps/fabro-server/src/server/handler/runs.rs b/lib/apps/fabro-server/src/server/handler/runs.rs index a0cbcbf70..0e2c8cbcb 100644 --- a/lib/apps/fabro-server/src/server/handler/runs.rs +++ b/lib/apps/fabro-server/src/server/handler/runs.rs @@ -829,7 +829,7 @@ async fn run_preflight( let (llm_result, ready_providers) = state.resolve_llm_client_with_ready_ids().await; let mut validated = match run_manifest::validate_prepared_manifest_for_preflight( &prepared, - &state.catalog(), + state.catalog(), vars, &ready_providers, ) { @@ -879,15 +879,17 @@ async fn validate_run_manifest( return ApiError::bad_request(format!("Run config variable interpolation failed: {err}")) .into_response(); } - let validated = - match run_manifest::validate_prepared_manifest_with_vars(&prepared, &state.catalog(), vars) - { - Ok(validated) => validated, - Err(WorkflowError::Parse(_)) => { - return ApiError::bad_request("Validation failed").into_response(); - } - Err(err) => return ApiError::bad_request(err.to_string()).into_response(), - }; + let validated = match run_manifest::validate_prepared_manifest_with_vars( + &prepared, + state.catalog(), + vars, + ) { + Ok(validated) => validated, + Err(WorkflowError::Parse(_)) => { + return ApiError::bad_request("Validation failed").into_response(); + } + Err(err) => return ApiError::bad_request(err.to_string()).into_response(), + }; ( StatusCode::OK, Json(run_manifest::validate_response(&prepared, &validated)), diff --git a/lib/components/fabro-workflow/src/handler/manager_loop.rs b/lib/components/fabro-workflow/src/handler/manager_loop.rs index 73c068a73..5668954c6 100644 --- a/lib/components/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/components/fabro-workflow/src/handler/manager_loop.rs @@ -131,7 +131,7 @@ fn validate_child_workflow( cwd, custom_transforms: Vec::new(), }, - &services.run.catalog, + Arc::clone(&services.run.catalog), )?; validated.promote_template_undefined_variables_to_errors(); validated.raise_on_errors()?; diff --git a/lib/components/fabro-workflow/src/operations/create.rs b/lib/components/fabro-workflow/src/operations/create.rs index 5d5478b24..4f810cb54 100644 --- a/lib/components/fabro-workflow/src/operations/create.rs +++ b/lib/components/fabro-workflow/src/operations/create.rs @@ -559,7 +559,7 @@ reasoning = false cwd: PathBuf::from("."), custom_transforms: Vec::new(), }, - &test_catalog(), + test_catalog(), ) .unwrap() } diff --git a/lib/components/fabro-workflow/src/operations/validate.rs b/lib/components/fabro-workflow/src/operations/validate.rs index 41413eb9e..dfcb38777 100644 --- a/lib/components/fabro-workflow/src/operations/validate.rs +++ b/lib/components/fabro-workflow/src/operations/validate.rs @@ -35,12 +35,9 @@ pub fn validate(input: ValidateInput) -> Result { /// Parse, transform, and validate a DOT source string against `catalog`. pub fn validate_with_catalog( input: ValidateInput, - catalog: &Arc, + catalog: Arc, ) -> Result { - validate_resolving_models( - input, - Some(ModelResolutionTransform::new(Arc::clone(catalog))), - ) + validate_resolving_models(input, Some(ModelResolutionTransform::new(catalog))) } /// Parse, transform, and validate, resolving models against the ready @@ -48,14 +45,14 @@ pub fn validate_with_catalog( /// provider-readiness selection failures. pub fn validate_with_ready_providers( input: ValidateInput, - catalog: &Arc, + catalog: Arc, ready_providers: &[ProviderId], ) -> Result { validate_resolving_models( input, Some( ModelResolutionTransform::for_eligible( - Arc::clone(catalog), + catalog, ready_providers.iter().cloned().collect(), ) .with_catalog_fallback(true), diff --git a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs index 66434266a..311c47189 100644 --- a/lib/components/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/components/fabro-workflow/src/transforms/variable_expansion.rs @@ -19,17 +19,19 @@ use crate::static_reference::{ /// How the template-expansion pass should treat undefined input variables. /// -/// Validate is structural — it should not fail just because the user has not -/// bound `{{ inputs.* }}` yet. Run-start is strict — missing inputs are real -/// errors. Splitting the two lets validate work on a bare `.fabro` while -/// run-start preserves its current hard-fail behavior. +/// Neither validate nor run-create should fail just because the user has not +/// bound `{{ inputs.* }}` yet, so both render structurally. Run-create then +/// promotes the resulting warnings to errors itself, which keeps its hard-fail +/// behavior while still reporting every undefined variable in one pass rather +/// than aborting on the first. #[derive(Clone, Copy, Debug)] pub enum RenderMode { - /// Undefined inputs are hard errors. Used by run-create. + /// Undefined inputs abort the pass with a hard error. No production caller + /// uses this today; run-create promotes structural warnings instead. Strict, /// Undefined inputs render as empty and become warning diagnostics on the /// returned `Validated`, so structural lints still run. Used by - /// `fabro validate`. + /// `fabro validate` and by run-create. Structural, }