refactor: simplify template migration code

- Skip MiniJinja parse+render for plain-text strings (no {{ / {% / {#)
- Remove dead VariableExpansionTransform type alias
- Add From<TemplateError> 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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-11 11:16:15 -04:00
parent a64f4d0cd8
commit 04f270b692
No known key found for this signature in database
6 changed files with 56 additions and 39 deletions

View file

@ -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<E>(
prompt: &str,
model: Option<&str>,
env: &E,
hook_kind: &str,
) -> Option<(String, Option<String>)>
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<E>(
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());

View file

@ -124,7 +124,16 @@ impl From<minijinja::Error> 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<String, TemplateError> {
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);

View file

@ -337,6 +337,12 @@ impl From<GraphvizError> for FabroError {
}
}
impl From<fabro_template::TemplateError> for FabroError {
fn from(err: fabro_template::TemplateError) -> Self {
Self::Validation(err.to_string())
}
}
impl From<fabro_validate::ValidationError> for FabroError {
fn from(e: fabro_validate::ValidationError) -> Self {
Self::Validation(e.0)

View file

@ -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.

View file

@ -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;

View file

@ -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)?)
}
}