From 04f270b692190e8661e5f27f1ba4dbc2eda6ef6c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 11 Apr 2026 11:16:15 -0400 Subject: [PATCH] refactor: simplify template migration code - Skip MiniJinja parse+render for plain-text strings (no {{ / {% / {#) - Remove dead VariableExpansionTransform type alias - Add From for FabroError, replace manual map_err with ? - Extract resolve_prompt_and_model helper in hooks executor Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-hooks/src/executor.rs | 70 ++++++++++--------- lib/crates/fabro-template/src/lib.rs | 9 +++ lib/crates/fabro-workflow/src/error.rs | 6 ++ .../fabro-workflow/src/handler/agent.rs | 2 +- .../fabro-workflow/src/transforms/mod.rs | 1 - .../src/transforms/variable_expansion.rs | 7 +- 6 files changed, 56 insertions(+), 39 deletions(-) diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index 53d43e263..34be7a808 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -96,6 +96,38 @@ impl HookExecutorImpl { } } + /// Resolve env vars in the prompt and optional model strings. + /// Returns `None` (with a warning) on resolution failure — callers should + /// proceed when that happens. + fn resolve_prompt_and_model( + prompt: &str, + model: Option<&str>, + env: &E, + hook_kind: &str, + ) -> Option<(String, Option)> + where + E: Env + ?Sized, + { + let prompt = match resolve_interp_string(prompt, env) { + Ok(prompt) => prompt, + Err(error) => { + tracing::warn!(error = %error, "{hook_kind} hook prompt env resolution failed, proceeding"); + return None; + } + }; + let model = match model + .map(|model| resolve_interp_string(model, env)) + .transpose() + { + Ok(model) => model, + Err(error) => { + tracing::warn!(error = %error, "{hook_kind} hook model env resolution failed, proceeding"); + return None; + } + }; + Some((prompt, model)) + } + /// Execute a command hook (sandbox or host). async fn execute_command( definition: &HookDefinition, @@ -256,22 +288,9 @@ impl HookExecutorImpl { where E: Env + Clone + Send + Sync + fmt::Debug + 'static, { - let prompt = match resolve_interp_string(prompt, env) { - Ok(prompt) => prompt, - Err(error) => { - tracing::warn!(error = %error, "prompt hook prompt env resolution failed, proceeding"); - return HookDecision::Proceed; - } - }; - let model = match model - .map(|model| resolve_interp_string(model, env)) - .transpose() - { - Ok(model) => model, - Err(error) => { - tracing::warn!(error = %error, "prompt hook model env resolution failed, proceeding"); - return HookDecision::Proceed; - } + let Some((prompt, model)) = Self::resolve_prompt_and_model(prompt, model, env, "prompt") + else { + return HookDecision::Proceed; }; let resolved_model = Self::resolve_model(model.as_deref()); @@ -323,22 +342,9 @@ impl HookExecutorImpl { where E: Env + Clone + Send + Sync + fmt::Debug + 'static, { - let prompt = match resolve_interp_string(prompt, env) { - Ok(prompt) => prompt, - Err(error) => { - tracing::warn!(error = %error, "agent hook prompt env resolution failed, proceeding"); - return HookDecision::Proceed; - } - }; - let model = match model - .map(|model| resolve_interp_string(model, env)) - .transpose() - { - Ok(model) => model, - Err(error) => { - tracing::warn!(error = %error, "agent hook model env resolution failed, proceeding"); - return HookDecision::Proceed; - } + let Some((prompt, model)) = Self::resolve_prompt_and_model(prompt, model, env, "agent") + else { + return HookDecision::Proceed; }; let resolved_model = Self::resolve_model(model.as_deref()); diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index 40f68fb6b..5302b4110 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -124,7 +124,16 @@ impl From for TemplateError { } } +/// Returns `true` when the string contains no MiniJinja delimiters and can +/// be returned as-is without paying for a full template parse+render cycle. +fn is_plain_text(template: &str) -> bool { + !template.contains("{{") && !template.contains("{%") && !template.contains("{#") +} + pub fn render(template: &str, ctx: &TemplateContext) -> Result { + if is_plain_text(template) { + return Ok(template.to_owned()); + } let mut env = Environment::new(); env.set_undefined_behavior(UndefinedBehavior::Strict); env.set_auto_escape_callback(|_| AutoEscape::None); diff --git a/lib/crates/fabro-workflow/src/error.rs b/lib/crates/fabro-workflow/src/error.rs index 942b442f1..2109da468 100644 --- a/lib/crates/fabro-workflow/src/error.rs +++ b/lib/crates/fabro-workflow/src/error.rs @@ -337,6 +337,12 @@ impl From for FabroError { } } +impl From for FabroError { + fn from(err: fabro_template::TemplateError) -> Self { + Self::Validation(err.to_string()) + } +} + impl From for FabroError { fn from(e: fabro_validate::ValidationError) -> Self { Self::Validation(e.0) diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 7deaad358..713e9a73f 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -80,7 +80,7 @@ pub(crate) fn expand_variables( let ctx = TemplateContext::new() .with_goal(graph.goal()) .with_inputs(inputs.clone()); - render_template(text, &ctx).map_err(|error| FabroError::Validation(error.to_string())) + Ok(render_template(text, &ctx)?) } /// Status fields that indicate a JSON object contains routing directives. diff --git a/lib/crates/fabro-workflow/src/transforms/mod.rs b/lib/crates/fabro-workflow/src/transforms/mod.rs index d321254b0..065eee393 100644 --- a/lib/crates/fabro-workflow/src/transforms/mod.rs +++ b/lib/crates/fabro-workflow/src/transforms/mod.rs @@ -21,4 +21,3 @@ pub use model_resolution::ModelResolutionTransform; pub use preamble::PreambleTransform; pub use stylesheet_application::StylesheetApplicationTransform; pub use variable_expansion::TemplateTransform; -pub type VariableExpansionTransform = TemplateTransform; diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index e33c2c8d5..0e3511619 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -18,9 +18,7 @@ impl TemplateTransform { ) -> Result<(), FabroError> { for value in attrs.values_mut() { if let AttrValue::String(text) = value { - let rendered = render_template(text, ctx) - .map_err(|error| FabroError::Validation(error.to_string()))?; - *text = rendered; + *text = render_template(text, ctx)?; } } Ok(()) @@ -30,8 +28,7 @@ impl TemplateTransform { let ctx = TemplateContext::new() .with_goal("{{ goal }}") .with_inputs(self.inputs.clone()); - render_template(graph.goal(), &ctx) - .map_err(|error| FabroError::Validation(error.to_string())) + Ok(render_template(graph.goal(), &ctx)?) } }