diff --git a/Cargo.lock b/Cargo.lock index 26a08c8c1..3661dad7b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2591,11 +2591,10 @@ dependencies = [ "async-trait", "fabro-agent", "fabro-auth", - "fabro-config", "fabro-http", "fabro-llm", "fabro-model", - "fabro-template", + "fabro-redact", "fabro-types", "fabro-util", "httpmock", diff --git a/docs/public/agents/hooks.mdx b/docs/public/agents/hooks.mdx index ef634b4f8..aafdd356f 100644 --- a/docs/public/agents/hooks.mdx +++ b/docs/public/agents/hooks.mdx @@ -28,16 +28,17 @@ POST the event context as JSON to an HTTP endpoint. Useful for webhooks, externa event = "run_complete" type = "http" url = "https://hooks.example.com/done" +allowed_env_vars = ["API_KEY"] [hooks.headers] -Authorization = "Bearer $API_KEY" +Authorization = "Bearer {{ env.API_KEY }}" ``` | Field | Description | |---|---| -| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. | -| `headers` | Optional HTTP headers. Values support `$VAR` interpolation from `allowed_env_vars`. | -| `allowed_env_vars` | List of environment variable names that may be interpolated into headers. | +| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. Supports `{{ env.NAME }}` interpolation. | +| `headers` | Optional HTTP headers. Values support `{{ env.NAME }}` interpolation, scoped to the names in `allowed_env_vars`. A token for any other env var fails to resolve and the hook blocks (fail-closed). | +| `allowed_env_vars` | Allowlist of environment variable names a header may read via `{{ env.NAME }}`. Empty (the default) means no env vars may be interpolated into headers. | | `tls` | TLS mode: `"verify"` (default), `"no_verify"`, or `"off"`. | ### Prompt diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index 80561ae6c..727c8cff5 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14225,10 +14225,19 @@ components: oneOf: - $ref: "#/components/schemas/StringMap" - type: "null" + description: >- + Optional HTTP headers for an http hook. Values support + `{{ env.NAME }}` interpolation, scoped to the names listed in + `allowed_env_vars`; a token for any other env var fails to resolve + and the hook blocks (fail-closed). allowed_env_vars: type: array items: type: string + description: >- + Allowlist of environment variable names that an http hook header may + read via `{{ env.NAME }}`. An empty list (the default) permits no env + vars in headers. tls: $ref: "#/components/schemas/TlsMode" prompt: 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 45f496b53..b22f101f9 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -462,11 +462,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(), @@ -487,15 +482,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 { @@ -512,11 +501,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; @@ -529,7 +531,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(), ) }; @@ -540,7 +542,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, @@ -551,17 +553,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/Cargo.toml b/lib/crates/fabro-hooks/Cargo.toml index 4875720dc..559a7499a 100644 --- a/lib/crates/fabro-hooks/Cargo.toml +++ b/lib/crates/fabro-hooks/Cargo.toml @@ -15,10 +15,9 @@ workspace = true [dependencies] fabro-agent = { path = "../fabro-agent" } fabro-auth = { path = "../fabro-auth" } -fabro-config = { path = "../fabro-config" } fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } -fabro-template = { path = "../fabro-template" } +fabro-redact.workspace = true fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } fabro-http.workspace = true diff --git a/lib/crates/fabro-hooks/src/config.rs b/lib/crates/fabro-hooks/src/config.rs index a6205c967..72ac52f2a 100644 --- a/lib/crates/fabro-hooks/src/config.rs +++ b/lib/crates/fabro-hooks/src/config.rs @@ -1,203 +1,8 @@ -//! Hook configuration runtime types. -//! -//! These types are the runtime shape that the hook executor consumes. The -//! v2 parse tree under `fabro_types::settings::run::HookEntry` is the -//! *config-file* shape; this module lives in `fabro-hooks` because the -//! behavior methods (`is_blocking`, `timeout`, `resolved_hook_type`, -//! `runs_in_sandbox`, `effective_name`) are runtime concerns owned by the -//! executor. - -use std::borrow::Cow; +//! Hook configuration runtime settings. +pub use fabro_types::settings::run::{HookDefinition, HookEvent, HookType, TlsMode}; use serde::{Deserialize, Serialize}; -/// Lifecycle events that can trigger user-defined hooks. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum HookEvent { - RunStart, - RunComplete, - RunFailed, - StageStart, - StageComplete, - StageFailed, - StageRetrying, - EdgeSelected, - ParallelStart, - ParallelComplete, - /// Reserved: hooks for this event are not yet invoked by the engine. - SandboxReady, - /// Reserved: hooks for this event are not yet invoked by the engine. - SandboxCleanup, - CheckpointSaved, - PreToolUse, - PostToolUse, - PostToolUseFailure, -} - -impl HookEvent { - /// Whether hooks for this event block execution by default. - #[must_use] - pub fn is_blocking_by_default(self) -> bool { - matches!( - self, - Self::RunStart - | Self::StageStart - | Self::EdgeSelected - | Self::PreToolUse - | Self::SandboxReady - ) - } -} - -impl std::fmt::Display for HookEvent { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(match self { - Self::RunStart => "run_start", - Self::RunComplete => "run_complete", - Self::RunFailed => "run_failed", - Self::StageStart => "stage_start", - Self::StageComplete => "stage_complete", - Self::StageFailed => "stage_failed", - Self::StageRetrying => "stage_retrying", - Self::EdgeSelected => "edge_selected", - Self::ParallelStart => "parallel_start", - Self::ParallelComplete => "parallel_complete", - Self::SandboxReady => "sandbox_ready", - Self::SandboxCleanup => "sandbox_cleanup", - Self::CheckpointSaved => "checkpoint_saved", - Self::PreToolUse => "pre_tool_use", - Self::PostToolUse => "post_tool_use", - Self::PostToolUseFailure => "post_tool_use_failure", - }) - } -} - -/// TLS verification mode for HTTP hooks. -#[derive(Debug, Clone, Copy, Deserialize, PartialEq, Eq, Default, Serialize)] -#[serde(rename_all = "snake_case")] -pub enum TlsMode { - /// Require `https://` and verify certificates (default). - #[default] - Verify, - /// Require `https://` but skip certificate verification. - NoVerify, - /// Allow `http://`; skip certificate verification for `https://`. - Off, -} - -/// How a hook is executed. -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] -#[serde(tag = "type", rename_all = "snake_case")] -pub enum HookType { - Command { - command: String, - }, - Http { - url: String, - headers: Option>, - #[serde(default)] - allowed_env_vars: Vec, - #[serde(default)] - tls: TlsMode, - }, - Prompt { - prompt: String, - model: Option, - }, - Agent { - prompt: String, - model: Option, - max_tool_rounds: Option, - }, -} - -/// A single hook definition. -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] -pub struct HookDefinition { - pub name: Option, - pub event: HookEvent, - /// Inline command shorthand — if set, implies `type = "command"`. - #[serde(default)] - pub command: Option, - /// Explicit hook type (command or http). If omitted and `command` is set, - /// defaults to `Command`. - #[serde(flatten)] - pub hook_type: Option, - /// Regex matched against node_id, handler_type, or event-specific fields. - pub matcher: Option, - /// Override the event's default blocking behavior. - pub blocking: Option, - /// Timeout in milliseconds (default: 60_000). - pub timeout_ms: Option, - /// Run inside the sandbox (true, default) or on the host (false). - pub sandbox: Option, -} - -impl HookDefinition { - /// Resolve the effective hook type: explicit `hook_type` wins, then - /// `command` shorthand, then error. - pub fn resolved_hook_type(&self) -> Option> { - if let Some(ref ht) = self.hook_type { - return Some(Cow::Borrowed(ht)); - } - self.command.as_ref().map(|cmd| { - Cow::Owned(HookType::Command { - command: cmd.clone(), - }) - }) - } - - /// Whether this hook is blocking for its event. - #[must_use] - pub fn is_blocking(&self) -> bool { - self.blocking - .unwrap_or_else(|| self.event.is_blocking_by_default()) - } - - /// Timeout duration for this hook. - /// - /// Defaults: 30s for prompt hooks, 60s for all others. - #[must_use] - pub fn timeout(&self) -> std::time::Duration { - if let Some(ms) = self.timeout_ms { - return std::time::Duration::from_millis(ms); - } - let default_ms = match self.resolved_hook_type().as_deref() { - Some(HookType::Prompt { .. }) => 30_000, - _ => 60_000, - }; - std::time::Duration::from_millis(default_ms) - } - - /// Whether this hook runs in the sandbox. - #[must_use] - pub fn runs_in_sandbox(&self) -> bool { - self.sandbox.unwrap_or(true) - } - - /// The effective name: explicit name or a generated one. - #[must_use] - pub fn effective_name(&self) -> String { - if let Some(ref n) = self.name { - return n.clone(); - } - 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)]; - format!("{event_str}:{short}") - } - Some(HookType::Http { ref url, .. }) => format!("{event_str}:{url}"), - Some(HookType::Prompt { ref prompt, .. } | HookType::Agent { ref prompt, .. }) => { - let short = &prompt[..prompt.floor_char_boundary(20)]; - format!("{event_str}:{short}") - } - None => event_str, - } - } -} - /// Top-level hook configuration: a list of hook definitions. #[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] pub struct HookSettings { diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index e6e72dc84..9fda710d5 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -12,8 +12,9 @@ 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_redact::redacted_url_for_log; +use fabro_types::settings::interp::Namespace; +use fabro_types::settings::{InterpString, ResolveError}; use fabro_util::env::{Env, SystemEnv}; use tokio::process::Command as TokioCommand; use tokio::time::timeout as tokio_timeout; @@ -56,26 +57,99 @@ 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.* }}` tokens against `env`. +/// +/// Only the `env` namespace is wired here; `{{ secrets.* }}`, `{{ vars.* }}`, +/// and `{{ inputs.* }}` tokens have no lookup in this context and resolve as +/// `Unavailable`, which is a hard error — so a hook that references one fails +/// closed rather than firing with a half-resolved value. +/// +/// 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. +/// +/// Returns the typed [`ResolveError`] so callers keep the source until the +/// decision boundary renders it; do not flatten it to a `String` here. +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], +#[expect( + clippy::disallowed_methods, + reason = "hook HTTP logs use the unresolved token source, not the resolved URL, so env-sourced \ + URL material is not logged; redacted_url_for_log masks literal credentials in \ + parseable source URLs and replaces unparseable sources with a placeholder" +)] +fn safe_url_source_for_log(url: &InterpString) -> String { + redacted_url_for_log(&url.as_source()) +} + +#[derive(Debug, Clone, PartialEq, Eq)] +enum HeaderResolveError { + NotAllowed { name: String }, + Resolve(ResolveError), +} + +impl fmt::Display for HeaderResolveError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::NotAllowed { name } => write!( + f, + "environment variable {name:?} referenced by an HTTP hook header is not listed in \ + allowed_env_vars" + ), + Self::Resolve(error) => error.fmt(f), + } + } +} + +impl std::error::Error for HeaderResolveError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::NotAllowed { .. } => None, + Self::Resolve(error) => Some(error), + } + } +} + +/// Resolve an HTTP-hook **header** value at fire time, scoping its +/// `{{ env.* }}` lookups to `allowed_env_vars`. +/// +/// Headers carry credentials, so unlike every other hook field they read env +/// through an allowlist: a `{{ env.NAME }}` token resolves only when `NAME` is +/// listed in the hook's `allowed_env_vars`. A name outside the allowlist fails +/// with a distinct error before any lookup, while an allowlisted-but-unset name +/// still surfaces as the normal `Missing` error. An empty `allowed_env_vars` +/// therefore permits no env vars in headers at all. This mirrors the previous +/// template-based `with_env_lookup_allowed` behavior without reviving any +/// template engine. +fn resolve_header( + value: &InterpString, + allowed_env_vars: &[String], env: &E, -) -> Result +) -> Result where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, + E: Env + ?Sized, { - let ctx = TemplateContext::new().with_env_lookup_allowed(env, allowed_vars); - render_template(value, &ctx).map_err(|error| error.to_string()) + if let Some(name) = value.names(Namespace::Env).into_iter().find(|name| { + !allowed_env_vars + .iter() + .any(|allowed| allowed.as_str() == *name) + }) { + return Err(HeaderResolveError::NotAllowed { + name: name.to_string(), + }); + } + + resolve_interp(value, env).map_err(HeaderResolveError::Resolve) } /// Executes hooks via shell commands or HTTP POST. @@ -105,55 +179,42 @@ 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: only `{{ env.* }}` is wired here; a missing env token (or a + /// token in any other, unavailable namespace) 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), ResolveError> 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, env: &E, ) -> HookDecision where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, + E: Env + ?Sized, { - let command = match resolve_interp_string(command, env) { + let command = match resolve_interp(command, env) { Ok(command) => command, Err(error) => { return HookDecision::Block { - reason: Some(error), + reason: Some(error.to_string()), }; } }; @@ -298,19 +359,24 @@ 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, catalog: Arc, ) -> HookDecision where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, + E: Env + ?Sized, { - 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.to_string()), + }; + } }; let resolved_model = Self::resolve_model(model.as_deref(), catalog.as_ref()); @@ -360,8 +426,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, @@ -370,11 +436,16 @@ impl HookExecutorImpl { catalog: Arc, ) -> HookDecision where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, + E: Env + ?Sized, { - 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.to_string()), + }; + } }; let resolved_model = Self::resolve_model(model.as_deref(), catalog.as_ref()); @@ -492,15 +563,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>, + url: &InterpString, + headers: Option<&HashMap>, allowed_env_vars: &[String], tls: &TlsMode, context: &HookContext, @@ -508,17 +580,19 @@ impl HookExecutorImpl { env: &E, ) -> HookDecision where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, + E: Env + ?Sized, { - 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!( + url_source = %safe_url_source_for_log(url), 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.to_string()), + }; } }; @@ -540,16 +614,22 @@ impl HookExecutorImpl { if let Some(hdrs) = headers { for (key, value) in hdrs { - let interpolated = match render_header_template(value, allowed_env_vars, env) { + // Headers resolve through the per-hook env allowlist: a + // `{{ env.NAME }}` not in `allowed_env_vars` blocks before any + // lookup, while an allowlisted-but-unset name still fails as + // missing. + let interpolated = match resolve_header(value, allowed_env_vars, env) { Ok(rendered) => rendered, Err(error) => { - tracing::warn!( - url = %resolved_url, + tracing::error!( + url_source = %safe_url_source_for_log(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.to_string()), + }; } }; request = request.header(key, interpolated); @@ -559,14 +639,18 @@ impl HookExecutorImpl { let response = match request.send().await { Ok(resp) => resp, Err(e) => { - tracing::warn!(url = %resolved_url, error = %e, "HTTP hook request failed, proceeding"); + tracing::warn!( + url_source = %safe_url_source_for_log(url), + error = %e, + "HTTP hook request failed, proceeding" + ); return HookDecision::Proceed; } }; if !response.status().is_success() { tracing::warn!( - url = %resolved_url, + url_source = %safe_url_source_for_log(url), status = response.status().as_u16(), "HTTP hook returned non-2xx, proceeding" ); @@ -576,7 +660,11 @@ impl HookExecutorImpl { let body = match response.text().await { Ok(text) => text, Err(e) => { - tracing::warn!(url = %resolved_url, error = %e, "HTTP hook body read failed, proceeding"); + tracing::warn!( + url_source = %safe_url_source_for_log(url), + error = %e, + "HTTP hook body read failed, proceeding" + ); return HookDecision::Proceed; } }; @@ -588,7 +676,11 @@ impl HookExecutorImpl { match serde_json::from_str::(body.trim()) { Ok(decision) => decision, Err(e) => { - tracing::warn!(url = %resolved_url, error = %e, "HTTP hook response parse failed, proceeding"); + tracing::warn!( + url_source = %safe_url_source_for_log(url), + error = %e, + "HTTP hook response parse failed, proceeding" + ); HookDecision::Proceed } } @@ -698,7 +790,7 @@ impl HookExecutor for HookExecutorImpl { Self::execute_prompt( definition, prompt, - model.as_deref(), + model.as_ref(), context, &env, llm_source, @@ -721,7 +813,7 @@ impl HookExecutor for HookExecutorImpl { Self::execute_agent( definition, prompt, - model.as_deref(), + model.as_ref(), *max_tool_rounds, context, sandbox, @@ -1053,7 +1145,7 @@ mod tests { ); } - // --- hook template helpers --- + // --- hook segment resolution helpers --- fn test_env(vars: &[(&str, &str)]) -> TestEnv { TestEnv( @@ -1063,11 +1155,37 @@ mod tests { ) } + fn interp(value: &str) -> InterpString { + InterpString::parse(value) + } + #[test] - fn render_header_template_resolves_allowlisted_var() { + fn safe_url_source_for_log_redacts_parseable_url_source() { + let safe = safe_url_source_for_log(&interp( + "https://user:secret@example.com/hook?token=literal&keep=value", + )); + + assert_eq!( + safe, + "https://user:****@example.com/hook?token=****&keep=value" + ); + } + + #[test] + fn safe_url_source_for_log_hides_unparseable_url_source() { + let safe = safe_url_source_for_log(&interp("{{ env.FABRO_TEST_HOOK_URL }}")); + + assert_eq!(safe, ""); + } + + // Headers resolve `{{ env.NAME }}` tokens through the per-hook + // `allowed_env_vars` allowlist: an allowlisted name resolves, anything else + // fails closed before lookup. + #[test] + fn header_resolves_allowlisted_var() { let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]); - let result = render_header_template( - "Bearer {{ env.FABRO_TEST_KEY_1 }}", + let result = resolve_header( + &interp("Bearer {{ env.FABRO_TEST_KEY_1 }}"), &["FABRO_TEST_KEY_1".to_string()], &env, ) @@ -1075,33 +1193,62 @@ mod tests { assert_eq!(result, "Bearer secret123"); } + // Fail-closed: a header may not read an env var that is set in the process + // but missing from `allowed_env_vars`. This is distinct from an unset + // allowlisted variable, so the block reason points at the allowlist. #[test] - fn render_header_template_rejects_unlisted_var() { + fn header_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")); + let err = resolve_header( + &interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), + &[], + &env, + ) + .unwrap_err(); + assert_eq!(err, HeaderResolveError::NotAllowed { + name: "FABRO_TEST_KEY_3".to_string(), + }); } #[test] - fn resolve_interp_string_resolves_embedded_var() { + fn header_missing_token_is_hard_error() { + let env = test_env(&[]); + let err = resolve_header( + &interp("prefix-{{ env.FABRO_TEST_KEY_3 }}-suffix"), + &["FABRO_TEST_KEY_3".to_string()], + &env, + ) + .unwrap_err(); + match err { + HeaderResolveError::Resolve(error) => assert_eq!(error.name, "FABRO_TEST_KEY_3"), + HeaderResolveError::NotAllowed { .. } => { + panic!("expected missing token resolve error, got {err:?}") + } + } + } + + // 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_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(); - assert!(err.contains("FABRO_TEST_NOEXIST")); + let err = resolve_interp(&interp("a{{ env.FABRO_TEST_NOEXIST }}-b"), &env).unwrap_err(); + assert_eq!(err.name, "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,7 +1271,7 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, &[], &TlsMode::Off, @@ -1153,7 +1300,7 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, &[], &TlsMode::Off, @@ -1180,7 +1327,7 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, &[], &TlsMode::Off, @@ -1199,7 +1346,7 @@ 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, @@ -1228,13 +1375,13 @@ 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, @@ -1248,6 +1395,53 @@ mod tests { assert_eq!(decision, HookDecision::Proceed); } + // Fail-closed: a header that references an env var set in the process but + // absent from `allowed_env_vars` must block and never fire the request. + #[tokio::test] + async fn http_hook_unlisted_header_var_blocks_without_firing() { + let env = test_env(&[("FABRO_TEST_TOKEN", "my-secret")]); + + 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_TOKEN }}"), + )]); + + let client = test_http_client(); + let decision = HookExecutorImpl::execute_http( + &client, + &interp(&server.url("/hook")), + Some(&headers), + // Empty allowlist: the env var is set, but headers may read nothing. + &[], + &TlsMode::Off, + &make_context(), + std::time::Duration::from_secs(5), + &env, + ) + .await; + + assert_eq!(mock.calls_async().await, 0); + match decision { + HookDecision::Block { reason } => { + assert!( + reason + .as_deref() + .is_some_and(|reason| reason.contains("FABRO_TEST_TOKEN")), + "block reason should name the unlisted token, got: {reason:?}" + ); + } + other => panic!("expected Block on unlisted header var, got {other:?}"), + } + } + #[tokio::test] async fn http_hook_resolves_url_before_dispatch() { let server = httpmock::MockServer::start_async().await; @@ -1262,7 +1456,7 @@ 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, @@ -1276,6 +1470,79 @@ 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), + // Allowlisted but unset: still blocks on the Missing lookup. + &["FABRO_TEST_MISSING_HEADER".to_string()], + &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,7 +1550,7 @@ 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, @@ -1301,7 +1568,7 @@ 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, @@ -1327,7 +1594,7 @@ mod tests { let client = test_http_client(); let decision = HookExecutorImpl::execute_http( &client, - &server.url("/hook"), + &interp(&server.url("/hook")), None, &[], &TlsMode::Off, @@ -1357,7 +1624,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 +1658,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 +1669,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 +1684,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 +1713,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-redact/src/lib.rs b/lib/crates/fabro-redact/src/lib.rs index d0fc361c3..170cf7a81 100644 --- a/lib/crates/fabro-redact/src/lib.rs +++ b/lib/crates/fabro-redact/src/lib.rs @@ -12,6 +12,20 @@ mod safe_url; pub use jsonl::{redact_json_value, redact_jsonl_line}; pub use safe_url::{DisplaySafeUrl, DisplaySafeUrlError}; +/// Redact a URL string for log or error output. +/// +/// Returns the credential-redacted form when `url` parses as a URL, or a fixed +/// `""` placeholder when it does not. This is the one place log +/// sites should reach for instead of re-rolling the +/// [`DisplaySafeUrl::parse`] + [`DisplaySafeUrl::redacted_string`] fallback +/// themselves, so an unparseable or credential-bearing URL never leaks into a +/// log line. +#[must_use] +pub fn redacted_url_for_log(url: &str) -> String { + DisplaySafeUrl::parse(url) + .map_or_else(|_| "".to_string(), |url| url.redacted_string()) +} + /// A byte range within a string that should be redacted. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Region { @@ -69,6 +83,19 @@ mod tests { assert_eq!(redact_string("hello world"), "hello world"); } + #[test] + fn redacted_url_for_log_redacts_credentials() { + assert_eq!( + redacted_url_for_log("https://user:secret@example.com/hook?token=literal&keep=value"), + "https://user:****@example.com/hook?token=****&keep=value" + ); + } + + #[test] + fn redacted_url_for_log_uses_placeholder_when_unparseable() { + assert_eq!(redacted_url_for_log("{{ env.HOOK_URL }}"), ""); + } + #[test] fn redact_string_with_aws_key() { let result = redact_string("key=AKIAYRWQG5EJLPZLBYNP"); diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index a61cd8134..a32a13dd7 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -3,7 +3,6 @@ use std::fmt; use std::sync::{Arc, Mutex}; use fabro_types::ManifestPath; -use fabro_util::env::Env; use miette::{LabeledSpan, NamedSource, SourceCode, SourceSpan}; use minijinja::value::{Object, Value}; use minijinja::{AutoEscape, Environment, ErrorKind, UndefinedBehavior}; @@ -53,7 +52,6 @@ pub struct TemplateContext { goal: Option, inputs: Value, vars: Value, - env: Option, } impl Default for TemplateContext { @@ -62,7 +60,6 @@ impl Default for TemplateContext { goal: None, inputs: Value::from_serialize(HashMap::::new()), vars: Value::from_serialize(HashMap::::new()), - env: None, } } } @@ -100,41 +97,11 @@ impl TemplateContext { Self::new().with_goal("{{ goal }}").with_inputs(inputs) } - #[must_use] - pub fn with_env_lookup(mut self, env: &E) -> Self - where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, - { - self.env = Some(Value::from_object(EnvLookup { - env: env.clone(), - allowlist: None, - })); - self - } - - #[must_use] - pub fn with_env_lookup_allowed(mut self, env: &E, allowlist: &[String]) -> Self - where - E: Env + Clone + Send + Sync + fmt::Debug + 'static, - { - self.env = Some(Value::from_object(EnvLookup { - env: env.clone(), - allowlist: Some(allowlist.to_vec()), - })); - self - } - fn into_value(self) -> Value { let goal = self.goal.map(Value::from); let inputs = self.inputs; let vars = self.vars; - let env = self.env; - Value::from_object(RenderContext { - goal, - inputs, - vars, - env, - }) + Value::from_object(RenderContext { goal, inputs, vars }) } } @@ -143,7 +110,6 @@ struct RenderContext { goal: Option, inputs: Value, vars: Value, - env: Option, } impl Object for RenderContext { @@ -152,33 +118,11 @@ impl Object for RenderContext { "goal" => self.goal.clone(), "inputs" => Some(self.inputs.clone()), "vars" => Some(self.vars.clone()), - "env" => self.env.clone(), _ => None, } } } -#[derive(Debug, Clone)] -pub struct EnvLookup { - env: E, - allowlist: Option>, -} - -impl Object for EnvLookup -where - E: Env + Send + Sync + fmt::Debug + 'static, -{ - fn get_value_by_str(self: &Arc, key: &str) -> Option { - if let Some(allowlist) = &self.allowlist { - if !allowlist.iter().any(|allowed| allowed == key) { - return None; - } - } - - self.env.var(key).ok().map(Value::from) - } -} - /// Errors from rendering a template. Each variant carries the typed fields /// MiniJinja knows about (offending expression, line) plus the original /// `minijinja::Error` as `#[source]`, so the cause chain is preserved across @@ -815,7 +759,6 @@ fn reject_loader_dependent_string(name: Option<&str>, template: &str) -> Result< mod tests { use std::collections::HashMap; - use fabro_util::env::TestEnv; use fabro_util::error; use toml::map::Map; @@ -916,39 +859,6 @@ mod tests { assert_eq!(rendered, "Repo fabro"); } - #[test] - fn renders_env_variable() { - let env = TestEnv(HashMap::from([( - "API_KEY".to_string(), - "secret".to_string(), - )])); - let ctx = TemplateContext::new().with_env_lookup(&env); - - let rendered = render("{{ env.API_KEY }}", &ctx).unwrap(); - - assert_eq!(rendered, "secret"); - } - - #[test] - fn renders_allowlisted_env_variable() { - let env = TestEnv(HashMap::from([("TOKEN".to_string(), "abc123".to_string())])); - let ctx = TemplateContext::new().with_env_lookup_allowed(&env, &["TOKEN".to_string()]); - - let rendered = render("Bearer {{ env.TOKEN }}", &ctx).unwrap(); - - assert_eq!(rendered, "Bearer abc123"); - } - - #[test] - fn rejects_non_allowlisted_env_variable() { - let env = TestEnv(HashMap::from([("SECRET".to_string(), "shh".to_string())])); - let ctx = TemplateContext::new().with_env_lookup_allowed(&env, &[]); - - let err = render("{{ env.SECRET }}", &ctx).unwrap_err(); - - assert!(matches!(err, TemplateError::UndefinedVariable { .. })); - } - #[test] fn render_lenient_treats_undefined_as_empty() { let ctx = TemplateContext::new(); diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index b4f3b4896..fe7f99178 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, @@ -1656,16 +1656,8 @@ impl HookDefinition { #[must_use] pub fn is_blocking(&self) -> bool { - self.blocking.unwrap_or({ - matches!( - self.event, - HookEvent::RunStart - | HookEvent::StageStart - | HookEvent::EdgeSelected - | HookEvent::PreToolUse - | HookEvent::SandboxReady - ) - }) + self.blocking + .unwrap_or_else(|| self.event.is_blocking_by_default()) } #[must_use] @@ -1686,19 +1678,26 @@ 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(); } - let event = format!("{:?}", self.event).to_lowercase(); + let event = self.event.to_string(); 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, @@ -1798,8 +1797,9 @@ pub enum AgentPermissions { Full, } -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, strum::Display)] #[serde(rename_all = "snake_case")] +#[strum(serialize_all = "snake_case")] pub enum HookEvent { RunStart, RunComplete, @@ -1819,6 +1819,20 @@ pub enum HookEvent { PostToolUseFailure, } +impl HookEvent { + #[must_use] + pub fn is_blocking_by_default(self) -> bool { + matches!( + self, + Self::RunStart + | Self::StageStart + | Self::EdgeSelected + | Self::PreToolUse + | Self::SandboxReady + ) + } +} + #[derive( Debug, Clone, diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 6174bb64b..e92e2ac0e 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -16,10 +16,9 @@ use fabro_sandbox::from_environment::{ use fabro_sandbox::{DockerSandboxOptions, SandboxSpec}; use fabro_static::EnvVars; use fabro_types::settings::run::{ - ApprovalMode, HookDefinition as ResolvedHookDefinition, HookEvent as ResolvedHookEvent, - HookType as ResolvedHookType, McpServerSettings as ResolvedMcpServerSettings, - PullRequestSettings, ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, - RunNamespace as ResolvedRunSettings, TlsMode as ResolvedTlsMode, + ApprovalMode, McpServerSettings as ResolvedMcpServerSettings, PullRequestSettings, + ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, + RunNamespace as ResolvedRunSettings, }; use fabro_types::settings::{ModelRegistry, ResolvedModelRef}; use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind}; @@ -476,7 +475,7 @@ impl RunSession { setup_command_timeout_ms: resolved.prepare.timeout_ms, }, hooks: fabro_hooks::HookSettings { - hooks: resolved.hooks.iter().map(runtime_hook_definition).collect(), + hooks: resolved.hooks.clone(), }, sandbox_env, seed_context: None, @@ -704,72 +703,6 @@ fn runtime_mcp_server( }) } -fn runtime_hook_definition(definition: &ResolvedHookDefinition) -> fabro_hooks::HookDefinition { - fabro_hooks::HookDefinition { - name: definition.name.clone(), - event: match definition.event { - ResolvedHookEvent::RunStart => fabro_hooks::HookEvent::RunStart, - ResolvedHookEvent::RunComplete => fabro_hooks::HookEvent::RunComplete, - ResolvedHookEvent::RunFailed => fabro_hooks::HookEvent::RunFailed, - ResolvedHookEvent::StageStart => fabro_hooks::HookEvent::StageStart, - ResolvedHookEvent::StageComplete => fabro_hooks::HookEvent::StageComplete, - ResolvedHookEvent::StageFailed => fabro_hooks::HookEvent::StageFailed, - ResolvedHookEvent::StageRetrying => fabro_hooks::HookEvent::StageRetrying, - ResolvedHookEvent::EdgeSelected => fabro_hooks::HookEvent::EdgeSelected, - ResolvedHookEvent::ParallelStart => fabro_hooks::HookEvent::ParallelStart, - ResolvedHookEvent::ParallelComplete => fabro_hooks::HookEvent::ParallelComplete, - ResolvedHookEvent::SandboxReady => fabro_hooks::HookEvent::SandboxReady, - ResolvedHookEvent::SandboxCleanup => fabro_hooks::HookEvent::SandboxCleanup, - ResolvedHookEvent::CheckpointSaved => fabro_hooks::HookEvent::CheckpointSaved, - ResolvedHookEvent::PreToolUse => fabro_hooks::HookEvent::PreToolUse, - ResolvedHookEvent::PostToolUse => fabro_hooks::HookEvent::PostToolUse, - ResolvedHookEvent::PostToolUseFailure => fabro_hooks::HookEvent::PostToolUseFailure, - }, - command: definition.command.clone(), - hook_type: definition.hook_type.as_ref().map(runtime_hook_type), - matcher: definition.matcher.clone(), - blocking: definition.blocking, - timeout_ms: definition.timeout_ms, - sandbox: definition.sandbox, - } -} - -fn runtime_hook_type(hook_type: &ResolvedHookType) -> fabro_hooks::HookType { - match hook_type { - ResolvedHookType::Command { command } => fabro_hooks::HookType::Command { - command: command.clone(), - }, - ResolvedHookType::Http { - url, - headers, - allowed_env_vars, - tls, - } => fabro_hooks::HookType::Http { - url: url.clone(), - headers: headers.clone(), - allowed_env_vars: allowed_env_vars.clone(), - tls: match tls { - ResolvedTlsMode::Verify => fabro_hooks::TlsMode::Verify, - ResolvedTlsMode::NoVerify => fabro_hooks::TlsMode::NoVerify, - ResolvedTlsMode::Off => fabro_hooks::TlsMode::Off, - }, - }, - ResolvedHookType::Prompt { prompt, model } => fabro_hooks::HookType::Prompt { - prompt: prompt.clone(), - model: model.clone(), - }, - ResolvedHookType::Agent { - prompt, - model, - max_tool_rounds, - } => fabro_hooks::HookType::Agent { - prompt: prompt.clone(), - model: model.clone(), - max_tool_rounds: *max_tool_rounds, - }, - } -} - impl RunSession { /// Shared engine: initialize, execute, finalize, pull_request. async fn run( 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);