diff --git a/lib/crates/fabro-config/src/resolve/mod.rs b/lib/crates/fabro-config/src/resolve/mod.rs index ef2e8d917..f0c6e5c47 100644 --- a/lib/crates/fabro-config/src/resolve/mod.rs +++ b/lib/crates/fabro-config/src/resolve/mod.rs @@ -101,6 +101,7 @@ pub(crate) fn warn_if_demoted_template(field: &str, value: Option<&str>) { mod tests { use std::collections::HashMap; + use fabro_types::settings::InterpString; use fabro_types::settings::run::{ HookType, McpHttpProtocol, McpTransport, ResolvedMcpEntry, TlsMode, }; @@ -201,10 +202,10 @@ Authorization = "Bearer {{ env.HOOK_TOKEN }}" assert_eq!( hook.resolved_hook_type().as_deref(), Some(&HookType::Http { - url: "https://hooks.example.com".to_string(), + url: InterpString::parse("https://hooks.example.com"), headers: Some(HashMap::from([( "Authorization".to_string(), - "Bearer {{ env.HOOK_TOKEN }}".to_string(), + InterpString::parse("Bearer {{ env.HOOK_TOKEN }}"), )])), allowed_env_vars: Vec::new(), tls: TlsMode::Verify, diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 7452a3ff5..2c7bbf60b 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -412,11 +412,6 @@ fn resolve_mcp_command( .unwrap_or_default() } -#[expect( - clippy::disallowed_methods, - reason = "intentional source preservation: the hook executor re-resolves {{ env.* }} \ - tokens at hook fire time" -)] fn resolve_hook(hook: &HookEntry, index: usize, errors: &mut Vec) -> HookDefinition { let variants = [ hook.script.is_some() || hook.command.is_some(), @@ -437,15 +432,9 @@ fn resolve_hook(hook: &HookEntry, index: usize, errors: &mut Vec) let hook_type = resolve_hook_type(hook); let command = if let Some(script) = &hook.script { - Some(script.as_source()) + Some(script.clone()) } else { - hook.command.as_ref().map(|command| { - command - .iter() - .map(InterpString::as_source) - .collect::>() - .join(" ") - }) + hook.command.as_ref().map(|command| join_command(command)) }; HookDefinition { @@ -462,11 +451,24 @@ fn resolve_hook(hook: &HookEntry, index: usize, errors: &mut Vec) } } +/// Join an argv-style `command` into a single space-separated [`InterpString`], +/// preserving every `{{ ... }}` token so the executor resolves it at hook fire +/// time. The join reconstructs the source form once, in one audited place. #[expect( clippy::disallowed_methods, - reason = "intentional source preservation: the hook executor re-resolves {{ env.* }} \ - tokens at hook fire time" + reason = "deliberate source reconstruction: argv parts are reassembled into one InterpString \ + whose tokens stay typed for resolution at hook fire time" )] +fn join_command(command: &[InterpString]) -> InterpString { + InterpString::parse( + &command + .iter() + .map(InterpString::as_source) + .collect::>() + .join(" "), + ) +} + fn resolve_hook_type(hook: &HookEntry) -> Option { if hook.script.is_some() || hook.command.is_some() { return None; @@ -479,7 +481,7 @@ fn resolve_hook_type(hook: &HookEntry) -> Option { Some( hook.headers .iter() - .map(|(key, value)| (key.clone(), value.as_source())) + .map(|(key, value)| (key.clone(), value.clone())) .collect(), ) }; @@ -490,7 +492,7 @@ fn resolve_hook_type(hook: &HookEntry) -> Option { None => TlsMode::default(), }; return Some(HookType::Http { - url: url.as_source(), + url: url.clone(), headers, allowed_env_vars: hook.allowed_env_vars.clone(), tls, @@ -501,17 +503,16 @@ fn resolve_hook_type(hook: &HookEntry) -> Option { return Some(HookType::Agent { prompt: hook .prompt - .as_ref() - .map(InterpString::as_source) - .unwrap_or_default(), - model: hook.model.as_ref().map(InterpString::as_source), + .clone() + .unwrap_or_else(|| InterpString::parse("")), + model: hook.model.clone(), max_tool_rounds: hook.max_tool_rounds, }); } hook.prompt.as_ref().map(|prompt| HookType::Prompt { - prompt: prompt.as_source(), - model: hook.model.as_ref().map(InterpString::as_source), + prompt: prompt.clone(), + model: hook.model.clone(), }) } diff --git a/lib/crates/fabro-hooks/src/config.rs b/lib/crates/fabro-hooks/src/config.rs index a6205c967..24665de39 100644 --- a/lib/crates/fabro-hooks/src/config.rs +++ b/lib/crates/fabro-hooks/src/config.rs @@ -9,6 +9,7 @@ use std::borrow::Cow; +use fabro_types::settings::InterpString; use serde::{Deserialize, Serialize}; /// Lifecycle events that can trigger user-defined hooks. @@ -91,23 +92,23 @@ pub enum TlsMode { #[serde(tag = "type", rename_all = "snake_case")] pub enum HookType { Command { - command: String, + command: InterpString, }, Http { - url: String, - headers: Option>, + url: InterpString, + headers: Option>, #[serde(default)] allowed_env_vars: Vec, #[serde(default)] tls: TlsMode, }, Prompt { - prompt: String, - model: Option, + prompt: InterpString, + model: Option, }, Agent { - prompt: String, - model: Option, + prompt: InterpString, + model: Option, max_tool_rounds: Option, }, } @@ -119,7 +120,7 @@ pub struct HookDefinition { pub event: HookEvent, /// Inline command shorthand — if set, implies `type = "command"`. #[serde(default)] - pub command: Option, + pub command: Option, /// Explicit hook type (command or http). If omitted and `command` is set, /// defaults to `Command`. #[serde(flatten)] @@ -178,6 +179,11 @@ impl HookDefinition { /// The effective name: explicit name or a generated one. #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "effective_name builds a human/merge-identity label from the hook's unresolved \ + template source; the source text is the intended display value here" + )] pub fn effective_name(&self) -> String { if let Some(ref n) = self.name { return n.clone(); @@ -185,12 +191,14 @@ impl HookDefinition { let event_str = self.event.to_string(); match self.resolved_hook_type().as_deref() { Some(HookType::Command { ref command }) => { - let short = &command[..command.floor_char_boundary(20)]; + let source = command.as_source(); + let short = &source[..source.floor_char_boundary(20)]; format!("{event_str}:{short}") } - Some(HookType::Http { ref url, .. }) => format!("{event_str}:{url}"), + Some(HookType::Http { ref url, .. }) => format!("{event_str}:{}", url.as_source()), Some(HookType::Prompt { ref prompt, .. } | HookType::Agent { ref prompt, .. }) => { - let short = &prompt[..prompt.floor_char_boundary(20)]; + let source = prompt.as_source(); + let short = &source[..source.floor_char_boundary(20)]; format!("{event_str}:{short}") } None => event_str, diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index e6e72dc84..520b05e89 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -12,7 +12,6 @@ use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::types::{Message, Request, ToolResult}; use fabro_model::Catalog; -use fabro_template::{TemplateContext, render as render_template}; use fabro_types::settings::InterpString; use fabro_util::env::{Env, SystemEnv}; use tokio::process::Command as TokioCommand; @@ -56,28 +55,24 @@ pub trait HookExecutor: Send + Sync { ) -> HookResult; } -fn resolve_interp_string(value: &str, env: &E) -> Result +/// Resolve a typed [`InterpString`] hook segment at fire time, looking up +/// `{{ env.* }}` / `{{ secrets.* }}` tokens against `env`. +/// +/// The value stays typed end-to-end: it is carried as an `InterpString` +/// through the config resolve layer and resolved here from its segments — +/// there is no `InterpString -> String -> InterpString` re-parse. A missing or +/// out-of-scope token is a hard error (fail-closed); there is no fallback to +/// the unresolved source. +fn resolve_interp(value: &InterpString, env: &E) -> Result where E: Env + ?Sized, { - InterpString::parse(value) + value .resolve(|name| env.var(name).ok()) .map(|resolved| resolved.value) .map_err(|error| error.to_string()) } -fn render_header_template( - value: &str, - allowed_vars: &[String], - env: &E, -) -> Result -where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, -{ - let ctx = TemplateContext::new().with_env_lookup_allowed(env, allowed_vars); - render_template(value, &ctx).map_err(|error| error.to_string()) -} - /// Executes hooks via shell commands or HTTP POST. pub struct HookExecutorImpl; @@ -105,42 +100,29 @@ 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. + /// Resolve the prompt and optional model segments at fire time. + /// + /// Fail-closed: a missing or out-of-scope `{{ env.* }}`/`{{ secrets.* }}` + /// token is a hard error so the hook never fires with a half-resolved + /// value. The caller turns the error into a `Block` decision, matching the + /// command-hook behavior. fn resolve_prompt_and_model( - prompt: &str, - model: Option<&str>, + prompt: &InterpString, + model: Option<&InterpString>, env: &E, - hook_kind: &str, - ) -> Option<(String, Option)> + ) -> Result<(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)) + let prompt = resolve_interp(prompt, env)?; + let model = model.map(|model| resolve_interp(model, env)).transpose()?; + Ok((prompt, model)) } /// Execute a command hook (sandbox or host). async fn execute_command( definition: &HookDefinition, - command: &str, + command: &InterpString, context: &HookContext, sandbox: &Arc, execution_context: &HookExecutionContext, @@ -149,7 +131,7 @@ impl HookExecutorImpl { where E: Env + Clone + Send + Sync + fmt::Debug + 'static, { - let command = match resolve_interp_string(command, env) { + let command = match resolve_interp(command, env) { Ok(command) => command, Err(error) => { return HookDecision::Block { @@ -298,8 +280,8 @@ impl HookExecutorImpl { /// Execute a prompt hook: single-turn LLM call returning ok/block. async fn execute_prompt( definition: &HookDefinition, - prompt: &str, - model: Option<&str>, + prompt: &InterpString, + model: Option<&InterpString>, context: &HookContext, env: &E, llm_source: &dyn CredentialSource, @@ -308,9 +290,14 @@ impl HookExecutorImpl { where E: Env + Clone + Send + Sync + fmt::Debug + 'static, { - let Some((prompt, model)) = Self::resolve_prompt_and_model(prompt, model, env, "prompt") - else { - return HookDecision::Proceed; + let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model, env) { + Ok(resolved) => resolved, + Err(error) => { + tracing::error!(error = %error, "prompt hook env resolution failed, not firing"); + return HookDecision::Block { + reason: Some(error), + }; + } }; let resolved_model = Self::resolve_model(model.as_deref(), catalog.as_ref()); @@ -360,8 +347,8 @@ impl HookExecutorImpl { /// a normal agent session. async fn execute_agent( definition: &HookDefinition, - prompt: &str, - model: Option<&str>, + prompt: &InterpString, + model: Option<&InterpString>, max_tool_rounds: Option, context: &HookContext, sandbox: Arc, @@ -372,9 +359,14 @@ impl HookExecutorImpl { where E: Env + Clone + Send + Sync + fmt::Debug + 'static, { - let Some((prompt, model)) = Self::resolve_prompt_and_model(prompt, model, env, "agent") - else { - return HookDecision::Proceed; + let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model, env) { + Ok(resolved) => resolved, + Err(error) => { + tracing::error!(error = %error, "agent hook env resolution failed, not firing"); + return HookDecision::Block { + reason: Some(error), + }; + } }; let resolved_model = Self::resolve_model(model.as_deref(), catalog.as_ref()); @@ -492,16 +484,16 @@ impl HookExecutorImpl { } /// Execute an HTTP hook: POST context JSON and parse the response. - /// Fail-open: non-2xx and connection errors return `Proceed`. - #[allow( - clippy::too_many_arguments, - reason = "HTTP hook execution needs separate client, TLS, env, and payload inputs." - )] + /// + /// Token resolution is fail-closed: a missing or out-of-scope token in the + /// URL or a header is a hard `Block`, so the hook never fires with a + /// half-resolved URL or an empty credential header. Transport outcomes + /// (non-2xx, connection errors, unparseable body) stay fail-open and + /// return `Proceed`. async fn execute_http( client: &fabro_http::HttpClient, - url: &str, - headers: Option<&HashMap>, - allowed_env_vars: &[String], + url: &InterpString, + headers: Option<&HashMap>, tls: &TlsMode, context: &HookContext, timeout: std::time::Duration, @@ -510,15 +502,16 @@ impl HookExecutorImpl { where E: Env + Clone + Send + Sync + fmt::Debug + 'static, { - let resolved_url = match resolve_interp_string(url, env) { + let resolved_url = match resolve_interp(url, env) { Ok(url) => url, Err(error) => { - tracing::warn!( - url = %url, + tracing::error!( error = %error, - "HTTP hook URL env resolution failed, proceeding" + "HTTP hook URL env resolution failed, not firing" ); - return HookDecision::Proceed; + return HookDecision::Block { + reason: Some(error), + }; } }; @@ -540,16 +533,18 @@ impl HookExecutorImpl { if let Some(hdrs) = headers { for (key, value) in hdrs { - let interpolated = match render_header_template(value, allowed_env_vars, env) { + let interpolated = match resolve_interp(value, env) { Ok(rendered) => rendered, Err(error) => { - tracing::warn!( + tracing::error!( url = %resolved_url, header = %key, error = %error, - "HTTP hook header template render failed, proceeding" + "HTTP hook header env resolution failed, not firing" ); - return HookDecision::Proceed; + return HookDecision::Block { + reason: Some(error), + }; } }; request = request.header(key, interpolated); @@ -662,13 +657,13 @@ impl HookExecutor for HookExecutorImpl { Cow::Borrowed(HookType::Http { ref url, ref headers, - ref allowed_env_vars, + allowed_env_vars: _, ref tls, }) | Cow::Owned(HookType::Http { ref url, ref headers, - ref allowed_env_vars, + allowed_env_vars: _, ref tls, }), ) => { @@ -677,7 +672,6 @@ impl HookExecutor for HookExecutorImpl { clients.get(*tls), url, headers.as_ref(), - allowed_env_vars, tls, context, definition.timeout(), @@ -698,7 +692,7 @@ impl HookExecutor for HookExecutorImpl { Self::execute_prompt( definition, prompt, - model.as_deref(), + model.as_ref(), context, &env, llm_source, @@ -721,7 +715,7 @@ impl HookExecutor for HookExecutorImpl { Self::execute_agent( definition, prompt, - model.as_deref(), + model.as_ref(), *max_tool_rounds, context, sandbox, @@ -1053,7 +1047,7 @@ mod tests { ); } - // --- hook template helpers --- + // --- hook segment resolution helpers --- fn test_env(vars: &[(&str, &str)]) -> TestEnv { TestEnv( @@ -1063,45 +1057,49 @@ mod tests { ) } + fn interp(value: &str) -> InterpString { + InterpString::parse(value) + } + + // Headers resolve via the same narrow `{{ ns.NAME }}` token resolver as + // every other hook field — no template engine, no allowlist. #[test] - fn render_header_template_resolves_allowlisted_var() { + fn header_resolves_narrow_token() { let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]); - let result = render_header_template( - "Bearer {{ env.FABRO_TEST_KEY_1 }}", - &["FABRO_TEST_KEY_1".to_string()], - &env, - ) - .unwrap(); + let result = resolve_interp(&interp("Bearer {{ env.FABRO_TEST_KEY_1 }}"), &env).unwrap(); assert_eq!(result, "Bearer secret123"); } #[test] - fn render_header_template_rejects_unlisted_var() { - let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); - let err = render_header_template("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix", &[], &env) - .unwrap_err(); - assert!(err.contains("undefined")); + fn header_missing_token_is_hard_error() { + let env = test_env(&[]); + let err = + resolve_interp(&interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), &env).unwrap_err(); + assert!(err.contains("FABRO_TEST_KEY_3")); } + // The value stays a typed `InterpString`: it resolves at fire time from its + // segments, never via a String -> InterpString re-parse. #[test] - fn resolve_interp_string_resolves_embedded_var() { + fn resolve_interp_resolves_embedded_token_from_typed_value() { let env = test_env(&[("FABRO_TEST_KEY_2", "val")]); - let result = resolve_interp_string("x{{ env.FABRO_TEST_KEY_2 }}y", &env).unwrap(); + let value = interp("x{{ env.FABRO_TEST_KEY_2 }}y"); + let result = resolve_interp(&value, &env).unwrap(); assert_eq!(result, "xvaly"); } #[test] - fn resolve_interp_string_errors_on_missing_var() { + fn resolve_interp_errors_on_missing_var() { let env = test_env(&[]); - let err = resolve_interp_string("a{{ env.FABRO_TEST_NOEXIST }}-b", &env).unwrap_err(); + let err = resolve_interp(&interp("a{{ env.FABRO_TEST_NOEXIST }}-b"), &env).unwrap_err(); assert!(err.contains("FABRO_TEST_NOEXIST")); } #[test] - fn resolve_interp_string_without_vars_passes_through() { + fn resolve_interp_without_tokens_passes_through() { let env = test_env(&[]); assert_eq!( - resolve_interp_string("plain text", &env).unwrap(), + resolve_interp(&interp("plain text"), &env).unwrap(), "plain text" ); } @@ -1124,9 +1122,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1153,9 +1150,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1180,9 +1176,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1199,9 +1194,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - "http://127.0.0.1:1", + &interp("http://127.0.0.1:1"), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(1), @@ -1228,15 +1222,14 @@ mod tests { let headers = HashMap::from([( "Authorization".to_string(), - "Bearer {{ env.FABRO_TEST_TOKEN }}".to_string(), + interp("Bearer {{ env.FABRO_TEST_TOKEN }}"), )]); let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), Some(&headers), - &["FABRO_TEST_TOKEN".to_string()], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1262,9 +1255,8 @@ mod tests { let env = test_env(&[("FABRO_TEST_URL", &server.url("/hook"))]); let decision = HookExecutorImpl::execute_http( &client, - "{{ env.FABRO_TEST_URL }}", + &interp("{{ env.FABRO_TEST_URL }}"), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1276,6 +1268,76 @@ mod tests { assert_eq!(decision, HookDecision::Proceed); } + #[tokio::test] + async fn http_hook_missing_url_token_blocks_without_firing() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method("POST").path("/hook"); + then.status(200).body(""); + }) + .await; + + let client = test_http_client(); + let decision = HookExecutorImpl::execute_http( + &client, + &interp("{{ env.FABRO_TEST_MISSING_URL }}/hook"), + None, + &TlsMode::Off, + &make_context(), + std::time::Duration::from_secs(5), + &test_env(&[]), + ) + .await; + + // Fail-closed: the missing token must not fire the hook at all. + assert_eq!(mock.calls_async().await, 0); + match decision { + HookDecision::Block { reason } => { + assert!( + reason + .as_deref() + .is_some_and(|reason| reason.contains("FABRO_TEST_MISSING_URL")), + "block reason should name the missing token, got: {reason:?}" + ); + } + other => panic!("expected Block on missing url token, got {other:?}"), + } + } + + #[tokio::test] + async fn http_hook_missing_header_token_blocks_without_firing() { + let server = httpmock::MockServer::start_async().await; + let mock = server + .mock_async(|when, then| { + when.method("POST").path("/hook"); + then.status(200).body(""); + }) + .await; + + let headers = HashMap::from([( + "Authorization".to_string(), + interp("Bearer {{ env.FABRO_TEST_MISSING_HEADER }}"), + )]); + + let client = test_http_client(); + let decision = HookExecutorImpl::execute_http( + &client, + &interp(&server.url("/hook")), + Some(&headers), + &TlsMode::Off, + &make_context(), + std::time::Duration::from_secs(5), + &test_env(&[]), + ) + .await; + + // Fail-closed: a missing header token must not fire the hook with an + // empty credential header. + assert_eq!(mock.calls_async().await, 0); + assert!(matches!(decision, HookDecision::Block { .. })); + } + // --- TLS mode enforcement tests --- #[tokio::test] @@ -1283,9 +1345,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - "http://example.com/hook", + &interp("http://example.com/hook"), None, - &[], &TlsMode::Verify, &make_context(), std::time::Duration::from_secs(5), @@ -1301,9 +1362,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - "http://example.com/hook", + &interp("http://example.com/hook"), None, - &[], &TlsMode::NoVerify, &make_context(), std::time::Duration::from_secs(5), @@ -1327,9 +1387,8 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, - &[], &TlsMode::Off, &make_context(), std::time::Duration::from_secs(5), @@ -1357,7 +1416,7 @@ mod tests { event: HookEvent::StageStart, command: None, hook_type: Some(HookType::Http { - url: server.url("/hook"), + url: interp(&server.url("/hook")), headers: None, allowed_env_vars: vec![], tls: TlsMode::Off, @@ -1391,7 +1450,7 @@ mod tests { let sandbox = make_sandbox(); let decision = HookExecutorImpl::execute_command( &make_definition("echo {{ env.MISSING_HOOK_VALUE }}"), - "echo {{ env.MISSING_HOOK_VALUE }}", + &interp("echo {{ env.MISSING_HOOK_VALUE }}"), &make_context(), &sandbox, &HookExecutionContext::default(), @@ -1402,11 +1461,13 @@ mod tests { assert!(matches!(decision, HookDecision::Block { .. })); } + // Fail-closed: a prompt hook with a missing token does not fire the LLM + // call; it blocks with the resolution error, matching command hooks. #[tokio::test] - async fn prompt_hook_missing_env_proceeds() { + async fn prompt_hook_missing_env_blocks() { let decision = HookExecutorImpl::execute_prompt( &make_definition("unused"), - "{{ env.MISSING_HOOK_VALUE }}", + &interp("{{ env.MISSING_HOOK_VALUE }}"), None, &make_context(), &test_env(&[]), @@ -1415,14 +1476,25 @@ mod tests { ) .await; - assert_eq!(decision, HookDecision::Proceed); + match decision { + HookDecision::Block { reason } => { + assert!( + reason + .as_deref() + .is_some_and(|reason| reason.contains("MISSING_HOOK_VALUE")), + "block reason should name the missing token, got: {reason:?}" + ); + } + other => panic!("expected Block on missing prompt token, got {other:?}"), + } } + // Fail-closed: an agent hook with a missing token blocks instead of firing. #[tokio::test] - async fn agent_hook_missing_env_proceeds() { + async fn agent_hook_missing_env_blocks() { let decision = HookExecutorImpl::execute_agent( &make_definition("unused"), - "{{ env.MISSING_HOOK_VALUE }}", + &interp("{{ env.MISSING_HOOK_VALUE }}"), None, Some(1), &make_context(), @@ -1433,6 +1505,6 @@ mod tests { ) .await; - assert_eq!(decision, HookDecision::Proceed); + assert!(matches!(decision, HookDecision::Block { .. })); } } diff --git a/lib/crates/fabro-hooks/src/lib.rs b/lib/crates/fabro-hooks/src/lib.rs index 5a29208ab..79a77bb98 100644 --- a/lib/crates/fabro-hooks/src/lib.rs +++ b/lib/crates/fabro-hooks/src/lib.rs @@ -6,5 +6,8 @@ pub mod types; pub use bridge::WorkflowToolHookCallback; pub use config::{HookDefinition, HookSettings, HookType, TlsMode}; +// Re-exported because the interpolatable fields of `HookType` are typed as +// `InterpString`; constructing a hook definition requires it. +pub use fabro_types::settings::InterpString; pub use runner::HookRunner; pub use types::{HookContext, HookDecision, HookEvent, HookExecutionContext}; diff --git a/lib/crates/fabro-hooks/tests/host_command_hooks.rs b/lib/crates/fabro-hooks/tests/host_command_hooks.rs index 4937b8135..4bbe0c5a3 100644 --- a/lib/crates/fabro-hooks/tests/host_command_hooks.rs +++ b/lib/crates/fabro-hooks/tests/host_command_hooks.rs @@ -5,7 +5,7 @@ use fabro_agent::{LocalSandbox, Sandbox}; use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_hooks::{ HookContext, HookDecision, HookDefinition, HookEvent, HookExecutionContext, HookRunner, - HookSettings, + HookSettings, InterpString, }; use fabro_model::Catalog; use fabro_types::RunId; @@ -44,7 +44,7 @@ async fn host_command_hook_uses_host_workdir_not_sandbox_workdir() { hooks: vec![HookDefinition { name: Some("host-marker".to_string()), event: HookEvent::RunStart, - command: Some("printf ran > marker.txt".to_string()), + command: Some(InterpString::parse("printf ran > marker.txt")), hook_type: None, matcher: None, blocking: Some(true), diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index c34fab921..32ed7d1c3 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -117,7 +117,7 @@ impl RunNamespace { } for hook in &mut self.hooks { substitute_option_string(&mut hook.name, &mut lookup)?; - substitute_option_string(&mut hook.command, &mut lookup)?; + substitute_option(&mut hook.command, &mut lookup)?; substitute_option_string(&mut hook.matcher, &mut lookup)?; if let Some(hook_type) = &mut hook.hook_type { substitute_hook_type(hook_type, &mut lookup)?; @@ -320,17 +320,17 @@ where F: FnMut(&str) -> Option, { match hook_type { - HookType::Command { command } => substitute_string(command, lookup), + HookType::Command { command } => substitute(command, lookup), HookType::Http { url, headers, .. } => { - substitute_string(url, lookup)?; + substitute(url, lookup)?; if let Some(headers) = headers { - substitute_string_map(headers, lookup)?; + substitute_map(headers, lookup)?; } Ok(()) } HookType::Prompt { prompt, model } | HookType::Agent { prompt, model, .. } => { - substitute_string(prompt, lookup)?; - substitute_option_string(model, lookup) + substitute(prompt, lookup)?; + substitute_option(model, lookup) } } } @@ -386,10 +386,10 @@ mod run_namespace_variable_substitution_tests { event: HookEvent::RunComplete, command: None, hook_type: Some(HookType::Http { - url: "https://hooks.example/{{ vars.ENV }}".to_string(), + url: InterpString::parse("https://hooks.example/{{ vars.ENV }}"), headers: Some(HashMap::from([( "X-Env".to_string(), - "{{ vars.ENV }}".to_string(), + InterpString::parse("{{ vars.ENV }}"), )])), allowed_env_vars: Vec::new(), tls: super::TlsMode::Verify, @@ -432,13 +432,13 @@ mod run_namespace_variable_substitution_tests { } match run.hooks[0].hook_type.as_ref().unwrap() { HookType::Http { url, headers, .. } => { - assert_eq!(url, "https://hooks.example/prod"); + assert_eq!(url.as_source(), "https://hooks.example/prod"); assert_eq!( headers .as_ref() .and_then(|headers| headers.get("X-Env")) - .map(String::as_str), - Some("prod") + .map(InterpString::as_source), + Some("prod".to_string()) ); } other => panic!("expected http hook type, got {other:?}"), @@ -1607,23 +1607,23 @@ pub enum TlsMode { #[serde(tag = "type", rename_all = "snake_case")] pub enum HookType { Command { - command: String, + command: InterpString, }, Http { - url: String, - headers: Option>, + url: InterpString, + headers: Option>, #[serde(default)] allowed_env_vars: Vec, #[serde(default)] tls: TlsMode, }, Prompt { - prompt: String, - model: Option, + prompt: InterpString, + model: Option, }, Agent { - prompt: String, - model: Option, + prompt: InterpString, + model: Option, max_tool_rounds: Option, }, } @@ -1633,7 +1633,7 @@ pub struct HookDefinition { pub name: Option, pub event: HookEvent, #[serde(default)] - pub command: Option, + pub command: Option, #[serde(flatten)] pub hook_type: Option, pub matcher: Option, @@ -1686,6 +1686,11 @@ impl HookDefinition { } #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "effective_name builds a human/merge-identity label from the hook's unresolved \ + template source; the source text is the intended display value here" + )] pub fn effective_name(&self) -> String { if let Some(ref name) = self.name { return name.clone(); @@ -1693,12 +1698,14 @@ impl HookDefinition { let event = format!("{:?}", self.event).to_lowercase(); match self.resolved_hook_type().as_deref() { Some(HookType::Command { command }) => { - let short = &command[..command.floor_char_boundary(20)]; + let source = command.as_source(); + let short = &source[..source.floor_char_boundary(20)]; format!("{event}:{short}") } - Some(HookType::Http { url, .. }) => format!("{event}:{url}"), + Some(HookType::Http { url, .. }) => format!("{event}:{}", url.as_source()), Some(HookType::Prompt { prompt, .. } | HookType::Agent { prompt, .. }) => { - let short = &prompt[..prompt.floor_char_boundary(20)]; + let source = prompt.as_source(); + let short = &source[..source.floor_char_boundary(20)]; format!("{event}:{short}") } None => event, diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 9524a1761..3ceeced5d 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -8906,7 +8906,13 @@ async fn hook_config_merge_run_overrides_by_name() { let merged = server_hooks.merge(run_hooks); assert_eq!(merged.hooks.len(), 1); // Run config wins — command should be "exit 0" - assert_eq!(merged.hooks[0].command.as_deref(), Some("exit 0")); + assert_eq!( + merged.hooks[0] + .command + .as_ref() + .map(fabro_hooks::InterpString::as_source), + Some("exit 0".to_string()) + ); // Verify it actually works end-to-end let engine = engine_with_hooks(merged.hooks);