From 3bb3ee5fb81635d834a5b79a56d0346efc7da24a Mon Sep 17 00:00:00 2001 From: Fabro Date: Sat, 11 Jul 2026 21:49:06 +0000 Subject: [PATCH] fabro(01KX9EM9QF65A43PB064TF7TWY): simplify_fable (succeeded) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fabro-Run: 01KX9EM9QF65A43PB064TF7TWY Fabro-Completed: 6 Fabro-Checkpoint: c4777e3c5882f19219977286c5217f74f702f448 ⚒️ Generated with [Fabro](https://fabro.sh) --- Cargo.lock | 2 - docs/public/agents/hooks.mdx | 22 +- docs/public/api-reference/fabro-api.yaml | 6 +- lib/crates/fabro-config/src/resolve/run.rs | 7 +- lib/crates/fabro-hooks/Cargo.toml | 2 - lib/crates/fabro-hooks/src/bridge.rs | 14 +- lib/crates/fabro-hooks/src/config.rs | 50 +- lib/crates/fabro-hooks/src/executor.rs | 867 +++--------------- lib/crates/fabro-hooks/src/lib.rs | 5 +- lib/crates/fabro-hooks/src/runner.rs | 46 +- lib/crates/fabro-hooks/src/types.rs | 27 +- .../fabro-hooks/tests/host_command_hooks.rs | 24 +- lib/crates/fabro-types/src/settings/run.rs | 606 +++++++++++- .../fabro-workflow/src/operations/start.rs | 167 +++- .../fabro-workflow/src/pipeline/initialize.rs | 4 +- .../fabro-workflow/tests/it/integration.rs | 336 +++++-- 16 files changed, 1225 insertions(+), 960 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3d6b02edf..c8bce19d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2668,14 +2668,12 @@ dependencies = [ "fabro-model", "fabro-redact", "fabro-types", - "fabro-util", "httpmock", "regex", "serde", "serde_json", "tokio", "tokio-util", - "toml 0.8.23", "tracing", ] diff --git a/docs/public/agents/hooks.mdx b/docs/public/agents/hooks.mdx index aafdd356f..a4dfa9314 100644 --- a/docs/public/agents/hooks.mdx +++ b/docs/public/agents/hooks.mdx @@ -36,8 +36,8 @@ Authorization = "Bearer {{ env.API_KEY }}" | Field | Description | |---|---| -| `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). | +| `url` | The endpoint to POST to. Must use `https://` unless `tls = "off"`. Supports `{{ env.NAME }}` and `{{ secrets.NAME }}` interpolation. | +| `headers` | Optional HTTP headers. Values support `{{ env.NAME }}` interpolation, scoped to the names in `allowed_env_vars` — referencing an env var outside the allowlist fails the run at startup. `{{ secrets.NAME }}` tokens are not allowed in headers — use secret interpolation in a hook `command`, `prompt`, or `url` instead. | | `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"`. | @@ -134,6 +134,24 @@ sandbox = false | `timeout_ms` | Hook timeout in milliseconds. Default: `60000` (60s) for most types, `30000` (30s) for prompt hooks. | | `sandbox` | Run inside the sandbox (`true`, default) or on the host (`false`). | +### Interpolation + +The configurable string fields of a hook — `command`, `url`, header values, `prompt`, and `model` — support interpolation tokens: + +```toml title="run.toml" +[[hooks]] +name = "deploy-gate" +event = "run_start" +command = "./scripts/deploy-gate.sh --token {{ secrets.DEPLOY_TOKEN }} --region {{ env.AWS_REGION }}" +sandbox = false +``` + +- `{{ env.NAME }}` — an environment variable of the worker process. +- `{{ secrets.NAME }}` — a secret from the vault. Allowed everywhere **except HTTP hook header values**; put secret tokens in a hook `command`, `prompt`, or `url` instead. +- `{{ vars.NAME }}` — a run variable, substituted when the run is created. + +`env` and `secrets` tokens resolve **once, when the run starts** — the same boundary where MCP server and prepare-step tokens resolve. A referenced env var or secret that is not set fails the run at startup with an error naming the missing value, even if the hook would never have fired. Hooks never re-resolve tokens at fire time; per-firing data (event, node ID, tool name, …) arrives through the [hook context](#hook-context) instead. + ## Blocking vs. non-blocking Blocking hooks can affect workflow execution. Non-blocking hooks run for side effects only — their decisions are ignored. diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index d6e384405..89253d6a3 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -14273,8 +14273,10 @@ components: 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`; a token for any other env var, or any + `{{ secrets.NAME }}` token, fails hook resolution and the run + fails at startup. Use secret interpolation in a hook command, + prompt, or url instead of a header. allowed_env_vars: type: array items: diff --git a/lib/crates/fabro-config/src/resolve/run.rs b/lib/crates/fabro-config/src/resolve/run.rs index 46287d85d..15f2b7bef 100644 --- a/lib/crates/fabro-config/src/resolve/run.rs +++ b/lib/crates/fabro-config/src/resolve/run.rs @@ -526,12 +526,13 @@ 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. +/// preserving every `{{ ... }}` token for its one-time resolution at the run +/// boundary (`HookDefinition::resolve_env`). The join reconstructs the source +/// form once, in one audited place. #[expect( clippy::disallowed_methods, reason = "deliberate source reconstruction: argv parts are reassembled into one InterpString \ - whose tokens stay typed for resolution at hook fire time" + whose tokens stay typed for their one-time resolution at the run boundary" )] fn join_command(command: &[InterpString]) -> InterpString { InterpString::parse( diff --git a/lib/crates/fabro-hooks/Cargo.toml b/lib/crates/fabro-hooks/Cargo.toml index 559a7499a..8b4e11b5a 100644 --- a/lib/crates/fabro-hooks/Cargo.toml +++ b/lib/crates/fabro-hooks/Cargo.toml @@ -19,7 +19,6 @@ fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } fabro-redact.workspace = true fabro-types = { path = "../fabro-types" } -fabro-util = { path = "../fabro-util" } fabro-http.workspace = true serde.workspace = true serde_json.workspace = true @@ -32,4 +31,3 @@ tokio-util.workspace = true [dev-dependencies] httpmock = "0.8" tokio = { workspace = true, features = ["test-util", "macros"] } -toml.workspace = true diff --git a/lib/crates/fabro-hooks/src/bridge.rs b/lib/crates/fabro-hooks/src/bridge.rs index 6aa7e60f6..3fbfd2652 100644 --- a/lib/crates/fabro-hooks/src/bridge.rs +++ b/lib/crates/fabro-hooks/src/bridge.rs @@ -82,7 +82,7 @@ mod tests { use fabro_types::fixtures; use super::*; - use crate::config::{HookDefinition, HookSettings}; + use crate::config::{HookSettings, RuntimeHookDefinition, RuntimeHookType}; use crate::executor::HookExecutor; use crate::types::{HookContext, HookResult}; @@ -96,7 +96,7 @@ mod tests { impl HookExecutor for CapturingExecutor { async fn execute( &self, - _definition: &HookDefinition, + _definition: &RuntimeHookDefinition, context: &HookContext, _sandbox: Arc, execution_context: &HookExecutionContext, @@ -116,16 +116,18 @@ mod tests { } } - fn make_hook(event: HookEvent) -> HookDefinition { - HookDefinition { + fn make_hook(event: HookEvent) -> RuntimeHookDefinition { + RuntimeHookDefinition { name: Some("test-hook".into()), event, - command: Some("echo test".into()), - hook_type: None, + hook_type: Some(RuntimeHookType::Command { + command: "echo test".into(), + }), matcher: None, blocking: None, timeout_ms: None, sandbox: Some(false), + effective_name: "test-hook".into(), } } diff --git a/lib/crates/fabro-hooks/src/config.rs b/lib/crates/fabro-hooks/src/config.rs index 72ac52f2a..88d35cdb1 100644 --- a/lib/crates/fabro-hooks/src/config.rs +++ b/lib/crates/fabro-hooks/src/config.rs @@ -1,44 +1,16 @@ //! Hook configuration runtime settings. -pub use fabro_types::settings::run::{HookDefinition, HookEvent, HookType, TlsMode}; -use serde::{Deserialize, Serialize}; +pub use fabro_types::settings::run::{ + HookEvent, RuntimeHookDefinition, RuntimeHookType, RuntimeHttpHook, TlsMode, +}; -/// Top-level hook configuration: a list of hook definitions. -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] +/// Top-level hook configuration: the boundary-resolved hooks for one run. +/// +/// Every interpolatable hook field is resolved to a plain string at the run +/// boundary before it reaches this crate (see +/// `fabro_types::settings::run::HookDefinition::resolve_env`), so the runner +/// and executor never resolve tokens themselves. +#[derive(Debug, Clone, Default, PartialEq)] pub struct HookSettings { - #[serde(default)] - pub hooks: Vec, -} - -impl HookSettings { - /// Merge with another config. Concatenates lists; on name collisions, - /// `other` wins. - #[must_use] - pub fn merge(self, other: Self) -> Self { - let mut by_name: std::collections::HashMap = - std::collections::HashMap::new(); - let mut order: Vec = Vec::new(); - - for hook in self.hooks { - let name = hook.effective_name(); - if !by_name.contains_key(&name) { - order.push(name.clone()); - } - by_name.insert(name, hook); - } - for hook in other.hooks { - let name = hook.effective_name(); - if !by_name.contains_key(&name) { - order.push(name.clone()); - } - by_name.insert(name, hook); - } - - let hooks = order - .into_iter() - .filter_map(|name| by_name.remove(&name)) - .collect(); - - Self { hooks } - } + pub hooks: Vec, } diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index a409a49b7..40467d32c 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -1,6 +1,4 @@ -use std::borrow::Cow; use std::collections::HashMap; -use std::fmt; use std::sync::{Arc, LazyLock}; use std::time::Instant; @@ -13,14 +11,11 @@ use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::types::{Message, Request, ToolResult}; use fabro_model::Catalog; 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; use tokio_util::sync::CancellationToken; -use crate::config::{HookDefinition, HookType, TlsMode}; +use crate::config::{RuntimeHookDefinition, RuntimeHookType, RuntimeHttpHook, TlsMode}; use crate::types::{ HookContext, HookDecision, HookExecutionContext, HookResult, PromptHookResponse, }; @@ -44,11 +39,15 @@ fn duration_ms(duration: std::time::Duration) -> u64 { } /// Trait for executing hooks via different transports. +/// +/// Definitions arrive fully resolved from the run boundary +/// ([`RuntimeHookDefinition`]): the executor formats, dispatches, and merges +/// decisions — it resolves nothing. #[async_trait] pub trait HookExecutor: Send + Sync { async fn execute( &self, - definition: &HookDefinition, + definition: &RuntimeHookDefinition, context: &HookContext, sandbox: Arc, execution_context: &HookExecutionContext, @@ -57,99 +56,6 @@ pub trait HookExecutor: Send + Sync { ) -> HookResult; } -/// 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, -{ - value.resolve(|name| env.var(name).ok()) -} - -#[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 -where - E: Env + ?Sized, -{ - 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. pub struct HookExecutorImpl; @@ -177,45 +83,14 @@ impl HookExecutorImpl { } } - /// 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: &InterpString, - model: Option<&InterpString>, - env: &E, - ) -> Result<(String, Option), ResolveError> - where - E: Env + ?Sized, - { - 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: &InterpString, + async fn execute_command( + definition: &RuntimeHookDefinition, + command: &str, context: &HookContext, sandbox: &Arc, execution_context: &HookExecutionContext, - env: &E, - ) -> HookDecision - where - E: Env + ?Sized, - { - let command = match resolve_interp(command, env) { - Ok(command) => command, - Err(error) => { - return HookDecision::Block { - reason: Some(error.to_string()), - }; - } - }; + ) -> HookDecision { let context_json = serde_json::to_string(context).unwrap_or_default(); let timeout_ms = duration_ms(definition.timeout()); @@ -243,7 +118,7 @@ impl HookExecutorImpl { .map(|path| path.to_string_lossy().to_string()); match sandbox .exec_command( - &command, + command, timeout_ms, sandbox_work_dir.as_deref(), Some(&env_vars), @@ -258,7 +133,7 @@ impl HookExecutorImpl { } } else { let mut cmd = TokioCommand::new("sh"); - cmd.arg("-c").arg(&command); + cmd.arg("-c").arg(command); if let Some(wd) = execution_context.command_cwd_for(definition) { cmd.current_dir(wd); } @@ -355,30 +230,16 @@ impl HookExecutorImpl { } /// Execute a prompt hook: single-turn LLM call returning ok/block. - async fn execute_prompt( - definition: &HookDefinition, - prompt: &InterpString, - model: Option<&InterpString>, + async fn execute_prompt( + definition: &RuntimeHookDefinition, + prompt: &str, + model: Option<&str>, context: &HookContext, - env: &E, llm_source: &dyn CredentialSource, catalog: Arc, - ) -> HookDecision - where - E: Env + ?Sized, - { - 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()); - let user_msg = Self::build_hook_user_message(&prompt, context); + ) -> HookDecision { + let resolved_model = Self::resolve_model(model, catalog.as_ref()); + let user_msg = Self::build_hook_user_message(prompt, context); Self::execute_llm_with_timeout(definition.timeout(), "prompt", || async move { let client = match LlmClient::from_source(llm_source, catalog).await { @@ -422,32 +283,18 @@ impl HookExecutorImpl { /// Reuses the core `ToolRegistry` from `fabro_agent` so the agent hook has /// the same tools (read_file, write_file, shell, grep, glob, etc.) as /// a normal agent session. - async fn execute_agent( - definition: &HookDefinition, - prompt: &InterpString, - model: Option<&InterpString>, + async fn execute_agent( + definition: &RuntimeHookDefinition, + prompt: &str, + model: Option<&str>, max_tool_rounds: Option, context: &HookContext, sandbox: Arc, - env: &E, llm_source: &dyn CredentialSource, catalog: Arc, - ) -> HookDecision - where - E: Env + ?Sized, - { - 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()); - let user_msg = Self::build_hook_user_message(&prompt, context); + ) -> HookDecision { + let resolved_model = Self::resolve_model(model, catalog.as_ref()); + let user_msg = Self::build_hook_user_message(prompt, context); Self::execute_llm_with_timeout(definition.timeout(), "agent", || async move { let client = match LlmClient::from_source(llm_source, catalog).await { @@ -562,45 +409,24 @@ impl HookExecutorImpl { /// Execute an HTTP hook: POST context JSON and parse the response. /// - /// 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( + /// Transport outcomes (non-2xx, connection errors, unparseable body) are + /// fail-open and return `Proceed`. Logging uses `http.url_source` — the + /// unresolved config source carried on the runtime type — so the resolved + /// URL, which may embed secret material, is never logged. + async fn execute_http( client: &fabro_http::HttpClient, - url: &InterpString, - headers: Option<&HashMap>, - allowed_env_vars: &[String], - tls: &TlsMode, + http: &RuntimeHttpHook, context: &HookContext, timeout: std::time::Duration, - env: &E, - ) -> HookDecision - where - E: Env + ?Sized, - { - let resolved_url = match resolve_interp(url, env) { - Ok(url) => url, - Err(error) => { - tracing::error!( - url_source = %safe_url_source_for_log(url), - error = %error, - "HTTP hook URL env resolution failed, not firing" - ); - return HookDecision::Block { - reason: Some(error.to_string()), - }; - } - }; - + ) -> HookDecision { // Enforce URL scheme based on TLS mode - match tls { + match http.tls { TlsMode::Verify | TlsMode::NoVerify => { - if !resolved_url.starts_with("https://") { + if !http.url.starts_with("https://") { return HookDecision::Block { reason: Some(format!( - "HTTP hook URL must use https:// (tls mode is {tls:?})" + "HTTP hook URL must use https:// (tls mode is {:?})", + http.tls )), }; } @@ -608,29 +434,11 @@ impl HookExecutorImpl { TlsMode::Off => {} } - let mut request = client.post(&resolved_url).timeout(timeout).json(context); + let mut request = client.post(&http.url).timeout(timeout).json(context); - if let Some(hdrs) = headers { + if let Some(hdrs) = &http.headers { for (key, value) in hdrs { - // 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::error!( - url_source = %safe_url_source_for_log(url), - header = %key, - error = %error, - "HTTP hook header env resolution failed, not firing" - ); - return HookDecision::Block { - reason: Some(error.to_string()), - }; - } - }; - request = request.header(key, interpolated); + request = request.header(key, value); } } @@ -638,7 +446,7 @@ impl HookExecutorImpl { Ok(resp) => resp, Err(e) => { tracing::warn!( - url_source = %safe_url_source_for_log(url), + url_source = %redacted_url_for_log(&http.url_source), error = %e, "HTTP hook request failed, proceeding" ); @@ -648,7 +456,7 @@ impl HookExecutorImpl { if !response.status().is_success() { tracing::warn!( - url_source = %safe_url_source_for_log(url), + url_source = %redacted_url_for_log(&http.url_source), status = response.status().as_u16(), "HTTP hook returned non-2xx, proceeding" ); @@ -659,7 +467,7 @@ impl HookExecutorImpl { Ok(text) => text, Err(e) => { tracing::warn!( - url_source = %safe_url_source_for_log(url), + url_source = %redacted_url_for_log(&http.url_source), error = %e, "HTTP hook body read failed, proceeding" ); @@ -675,7 +483,7 @@ impl HookExecutorImpl { Ok(decision) => decision, Err(e) => { tracing::warn!( - url_source = %safe_url_source_for_log(url), + url_source = %redacted_url_for_log(&http.url_source), error = %e, "HTTP hook response parse failed, proceeding" ); @@ -720,7 +528,7 @@ impl Default for HttpClientCache { impl HookExecutor for HookExecutorImpl { async fn execute( &self, - definition: &HookDefinition, + definition: &RuntimeHookDefinition, context: &HookContext, sandbox: Arc, execution_context: &HookExecutionContext, @@ -731,91 +539,39 @@ impl HookExecutor for HookExecutorImpl { static HTTP_CLIENTS: OnceLock = OnceLock::new(); let start = Instant::now(); - let env = SystemEnv; - let decision = match definition.resolved_hook_type() { - Some( - Cow::Borrowed(HookType::Command { ref command }) - | Cow::Owned(HookType::Command { ref command }), - ) => { - Self::execute_command( - definition, - command, - context, - &sandbox, - execution_context, - &env, - ) - .await + let decision = match &definition.hook_type { + Some(RuntimeHookType::Command { command }) => { + Self::execute_command(definition, command, context, &sandbox, execution_context) + .await } - Some( - Cow::Borrowed(HookType::Http { - ref url, - ref headers, - ref allowed_env_vars, - ref tls, - }) - | Cow::Owned(HookType::Http { - ref url, - ref headers, - ref allowed_env_vars, - ref tls, - }), - ) => { + Some(RuntimeHookType::Http(http)) => { let clients = HTTP_CLIENTS.get_or_init(HttpClientCache::new); - Self::execute_http( - clients.get(*tls), - url, - headers.as_ref(), - allowed_env_vars, - tls, - context, - definition.timeout(), - &env, - ) - .await + Self::execute_http(clients.get(http.tls), http, context, definition.timeout()).await } - Some( - Cow::Borrowed(HookType::Prompt { - ref prompt, - ref model, - }) - | Cow::Owned(HookType::Prompt { - ref prompt, - ref model, - }), - ) => { + Some(RuntimeHookType::Prompt { prompt, model }) => { Self::execute_prompt( definition, prompt, - model.as_ref(), + model.as_deref(), context, - &env, llm_source, Arc::clone(&catalog), ) .await } - Some( - Cow::Borrowed(HookType::Agent { - ref prompt, - ref model, - ref max_tool_rounds, - }) - | Cow::Owned(HookType::Agent { - ref prompt, - ref model, - ref max_tool_rounds, - }), - ) => { + Some(RuntimeHookType::Agent { + prompt, + model, + max_tool_rounds, + }) => { Self::execute_agent( definition, prompt, - model.as_ref(), + model.as_deref(), *max_tool_rounds, context, sandbox, - &env, llm_source, Arc::clone(&catalog), ) @@ -839,10 +595,8 @@ impl HookExecutor for HookExecutorImpl { mod tests { use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_types::fixtures; - use fabro_util::env::TestEnv; use super::*; - use crate::config::HookType; use crate::types::HookEvent; fn make_context() -> HookContext { @@ -867,19 +621,25 @@ mod tests { HookExecutorImpl::build_http_client(TlsMode::Off) } - fn make_definition(command: &str) -> HookDefinition { - HookDefinition { - name: Some("test-hook".into()), - event: HookEvent::StageStart, - command: Some(command.into()), - hook_type: None, - matcher: None, - blocking: None, + fn make_typed_definition(hook_type: Option) -> RuntimeHookDefinition { + RuntimeHookDefinition { + name: Some("test-hook".into()), + event: HookEvent::StageStart, + hook_type, + matcher: None, + blocking: None, timeout_ms: Some(5000), - sandbox: Some(false), // host execution for tests + sandbox: Some(false), // host execution for tests + effective_name: "test-hook".into(), } } + fn make_definition(command: &str) -> RuntimeHookDefinition { + make_typed_definition(Some(RuntimeHookType::Command { + command: command.to_string(), + })) + } + #[test] fn parse_decision_exit_0_proceed() { assert_eq!( @@ -1045,16 +805,7 @@ mod tests { #[tokio::test] async fn no_hook_type_blocks() { let executor = HookExecutorImpl; - let def = HookDefinition { - name: None, - event: HookEvent::StageStart, - command: None, - hook_type: None, - matcher: None, - blocking: None, - timeout_ms: None, - sandbox: Some(false), - }; + let def = make_typed_definition(None); let ctx = make_context(); let sandbox = make_sandbox(); let source = test_llm_source(); @@ -1143,116 +894,31 @@ mod tests { ); } - // --- hook segment resolution helpers --- - - fn test_env(vars: &[(&str, &str)]) -> TestEnv { - TestEnv( - vars.iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(), - ) - } - - fn interp(value: &str) -> InterpString { - InterpString::parse(value) - } - - #[test] - 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 = resolve_header( - &interp("Bearer {{ env.FABRO_TEST_KEY_1 }}"), - &["FABRO_TEST_KEY_1".to_string()], - &env, - ) - .unwrap(); - 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 header_rejects_unlisted_var() { - let env = test_env(&[("FABRO_TEST_KEY_3", "should_not_appear")]); - 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 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 value = interp("x{{ env.FABRO_TEST_KEY_2 }}y"); - let result = resolve_interp(&value, &env).unwrap(); - assert_eq!(result, "xvaly"); - } - - #[test] - fn resolve_interp_errors_on_missing_var() { - let env = test_env(&[]); - 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_without_tokens_passes_through() { - let env = test_env(&[]); - assert_eq!( - resolve_interp(&interp("plain text"), &env).unwrap(), - "plain text" - ); - } + // Token resolution tests live with the boundary resolver + // (`HookDefinition::resolve_env` in fabro-types); the executor only ever + // sees resolved strings. // --- HTTP hook execution tests --- + /// Call `execute_http` with the URL doubling as its own (already literal) + /// source, which is exactly what the boundary produces for token-free + /// config URLs. + async fn execute_http_for_test( + url: &str, + headers: Option>, + tls: TlsMode, + timeout: std::time::Duration, + ) -> HookDecision { + let client = test_http_client(); + let http = RuntimeHttpHook { + url: url.to_string(), + url_source: url.to_string(), + headers, + tls, + }; + HookExecutorImpl::execute_http(&client, &http, &make_context(), timeout).await + } + #[tokio::test] async fn http_hook_posts_json_and_parses_decision() { let server = httpmock::MockServer::start_async().await; @@ -1266,16 +932,11 @@ mod tests { }) .await; - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp(&server.url("/hook")), + let decision = execute_http_for_test( + &server.url("/hook"), None, - &[], - &TlsMode::Off, - &make_context(), + TlsMode::Off, std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1295,16 +956,11 @@ mod tests { }) .await; - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp(&server.url("/hook")), + let decision = execute_http_for_test( + &server.url("/hook"), None, - &[], - &TlsMode::Off, - &make_context(), + TlsMode::Off, std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1322,16 +978,11 @@ mod tests { }) .await; - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp(&server.url("/hook")), + let decision = execute_http_for_test( + &server.url("/hook"), None, - &[], - &TlsMode::Off, - &make_context(), + TlsMode::Off, std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1341,16 +992,11 @@ mod tests { #[tokio::test] async fn http_hook_connection_failure_returns_proceed() { - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp("http://127.0.0.1:1"), + let decision = execute_http_for_test( + "http://127.0.0.1:1", None, - &[], - &TlsMode::Off, - &make_context(), + TlsMode::Off, std::time::Duration::from_secs(1), - &test_env(&[]), ) .await; @@ -1358,9 +1004,7 @@ mod tests { } #[tokio::test] - async fn http_hook_sends_interpolated_headers() { - let env = test_env(&[("FABRO_TEST_TOKEN", "my-secret")]); - + async fn http_hook_sends_configured_headers() { let server = httpmock::MockServer::start_async().await; let mock = server .mock_async(|when, then| { @@ -1371,21 +1015,14 @@ mod tests { }) .await; - let headers = HashMap::from([( - "Authorization".to_string(), - interp("Bearer {{ env.FABRO_TEST_TOKEN }}"), - )]); + let headers = + HashMap::from([("Authorization".to_string(), "Bearer my-secret".to_string())]); - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp(&server.url("/hook")), - Some(&headers), - &["FABRO_TEST_TOKEN".to_string()], - &TlsMode::Off, - &make_context(), + let decision = execute_http_for_test( + &server.url("/hook"), + Some(headers), + TlsMode::Off, std::time::Duration::from_secs(5), - &env, ) .await; @@ -1393,168 +1030,15 @@ 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; - let mock = server - .mock_async(|when, then| { - when.method("POST").path("/hook"); - then.status(200).body(""); - }) - .await; - - let client = test_http_client(); - let env = test_env(&[("FABRO_TEST_URL", &server.url("/hook"))]); - let decision = HookExecutorImpl::execute_http( - &client, - &interp("{{ env.FABRO_TEST_URL }}"), - None, - &[], - &TlsMode::Off, - &make_context(), - std::time::Duration::from_secs(5), - &env, - ) - .await; - - mock.assert_async().await; - 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] async fn http_hook_rejects_http_url_when_tls_verify() { - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp("http://example.com/hook"), + let decision = execute_http_for_test( + "http://example.com/hook", None, - &[], - &TlsMode::Verify, - &make_context(), + TlsMode::Verify, std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1563,16 +1047,11 @@ mod tests { #[tokio::test] async fn http_hook_rejects_http_url_when_tls_no_verify() { - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp("http://example.com/hook"), + let decision = execute_http_for_test( + "http://example.com/hook", None, - &[], - &TlsMode::NoVerify, - &make_context(), + TlsMode::NoVerify, std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1589,16 +1068,11 @@ mod tests { }) .await; - let client = test_http_client(); - let decision = HookExecutorImpl::execute_http( - &client, - &interp(&server.url("/hook")), + let decision = execute_http_for_test( + &server.url("/hook"), None, - &[], - &TlsMode::Off, - &make_context(), + TlsMode::Off, std::time::Duration::from_secs(5), - &test_env(&[]), ) .await; @@ -1617,20 +1091,20 @@ mod tests { .await; let executor = HookExecutorImpl; - let def = HookDefinition { - name: Some("http-test".into()), - event: HookEvent::StageStart, - command: None, - hook_type: Some(HookType::Http { - url: interp(&server.url("/hook")), - headers: None, - allowed_env_vars: vec![], - tls: TlsMode::Off, - }), - matcher: None, - blocking: None, - timeout_ms: Some(5000), - sandbox: Some(false), + let def = RuntimeHookDefinition { + name: Some("http-test".into()), + event: HookEvent::StageStart, + hook_type: Some(RuntimeHookType::Http(RuntimeHttpHook { + url: server.url("/hook"), + url_source: server.url("/hook"), + headers: None, + tls: TlsMode::Off, + })), + matcher: None, + blocking: None, + timeout_ms: Some(5000), + sandbox: Some(false), + effective_name: "http-test".into(), }; let ctx = make_context(); let sandbox = make_sandbox(); @@ -1650,67 +1124,4 @@ mod tests { assert_eq!(result.decision, HookDecision::Proceed); assert_eq!(result.hook_name.as_deref(), Some("http-test")); } - - #[tokio::test] - async fn command_hook_missing_env_blocks() { - let sandbox = make_sandbox(); - let decision = HookExecutorImpl::execute_command( - &make_definition("echo {{ env.MISSING_HOOK_VALUE }}"), - &interp("echo {{ env.MISSING_HOOK_VALUE }}"), - &make_context(), - &sandbox, - &HookExecutionContext::default(), - &test_env(&[]), - ) - .await; - - 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_blocks() { - let decision = HookExecutorImpl::execute_prompt( - &make_definition("unused"), - &interp("{{ env.MISSING_HOOK_VALUE }}"), - None, - &make_context(), - &test_env(&[]), - test_llm_source().as_ref(), - test_catalog(), - ) - .await; - - 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_blocks() { - let decision = HookExecutorImpl::execute_agent( - &make_definition("unused"), - &interp("{{ env.MISSING_HOOK_VALUE }}"), - None, - Some(1), - &make_context(), - make_sandbox(), - &test_env(&[]), - test_llm_source().as_ref(), - test_catalog(), - ) - .await; - - assert!(matches!(decision, HookDecision::Block { .. })); - } } diff --git a/lib/crates/fabro-hooks/src/lib.rs b/lib/crates/fabro-hooks/src/lib.rs index 79a77bb98..ed737eac6 100644 --- a/lib/crates/fabro-hooks/src/lib.rs +++ b/lib/crates/fabro-hooks/src/lib.rs @@ -5,9 +5,6 @@ pub mod runner; 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 config::{HookSettings, RuntimeHookDefinition, RuntimeHookType, RuntimeHttpHook, TlsMode}; pub use runner::HookRunner; pub use types::{HookContext, HookDecision, HookEvent, HookExecutionContext}; diff --git a/lib/crates/fabro-hooks/src/runner.rs b/lib/crates/fabro-hooks/src/runner.rs index 9435dba9a..ed2a52ce5 100644 --- a/lib/crates/fabro-hooks/src/runner.rs +++ b/lib/crates/fabro-hooks/src/runner.rs @@ -7,7 +7,7 @@ use fabro_auth::CredentialSource; use fabro_auth::EnvCredentialSource; use fabro_model::Catalog; -use crate::config::{HookDefinition, HookSettings}; +use crate::config::{HookSettings, RuntimeHookDefinition}; use crate::executor::{HookExecutor, HookExecutorImpl}; use crate::types::{HookContext, HookDecision, HookExecutionContext}; @@ -108,7 +108,7 @@ impl HookRunner { } /// Filter hooks that match the given event and context. - fn filter_hooks(&self, context: &HookContext) -> Vec<&HookDefinition> { + fn filter_hooks(&self, context: &HookContext) -> Vec<&RuntimeHookDefinition> { self.config .hooks .iter() @@ -118,7 +118,7 @@ impl HookRunner { } /// Check if a hook's matcher applies to this context. - fn matches(&self, hook: &HookDefinition, context: &HookContext) -> bool { + fn matches(&self, hook: &RuntimeHookDefinition, context: &HookContext) -> bool { let Some(ref pattern) = hook.matcher else { return true; }; @@ -139,7 +139,7 @@ impl HookRunner { async fn run_sequential( &self, - hooks: &[&HookDefinition], + hooks: &[&RuntimeHookDefinition], context: &HookContext, sandbox: Arc, execution_context: &HookExecutionContext, @@ -147,7 +147,7 @@ impl HookRunner { let mut merged = HookDecision::Proceed; for hook in hooks { tracing::debug!( - hook = %hook.effective_name(), + hook = %hook.effective_name, event = %context.event, "Executing hook" ); @@ -163,7 +163,7 @@ impl HookRunner { ) .await; tracing::debug!( - hook = %hook.effective_name(), + hook = %hook.effective_name, duration_ms = result.duration_ms, decision = ?result.decision, "Hook complete" @@ -174,7 +174,7 @@ impl HookRunner { // Short-circuit on Block if matches!(merged, HookDecision::Block { .. }) { tracing::error!( - hook = %hook.effective_name(), + hook = %hook.effective_name, event = %context.event, decision = ?merged, "Hook blocked execution" @@ -183,7 +183,7 @@ impl HookRunner { } } else if !result.decision.is_proceed() { tracing::warn!( - hook = %hook.effective_name(), + hook = %hook.effective_name, event = %context.event, decision = ?result.decision, "Non-blocking hook returned non-proceed, ignoring" @@ -195,14 +195,14 @@ impl HookRunner { async fn run_non_blocking( &self, - hooks: &[&HookDefinition], + hooks: &[&RuntimeHookDefinition], context: &HookContext, sandbox: Arc, execution_context: &HookExecutionContext, ) -> HookDecision { for hook in hooks { tracing::debug!( - hook = %hook.effective_name(), + hook = %hook.effective_name, event = %context.event, "Executing hook" ); @@ -218,14 +218,14 @@ impl HookRunner { ) .await; tracing::debug!( - hook = %hook.effective_name(), + hook = %hook.effective_name, duration_ms = result.duration_ms, decision = ?result.decision, "Hook complete" ); if !result.decision.is_proceed() { tracing::warn!( - hook = %hook.effective_name(), + hook = %hook.effective_name, event = %context.event, decision = ?result.decision, "Non-blocking hook failed, continuing" @@ -242,7 +242,7 @@ mod tests { use fabro_types::fixtures; use super::*; - use crate::config::HookSettings; + use crate::config::{HookSettings, RuntimeHookType}; use crate::types::{HookContext, HookEvent, HookResult}; struct MockExecutor { @@ -253,7 +253,7 @@ mod tests { impl HookExecutor for MockExecutor { async fn execute( &self, - definition: &HookDefinition, + definition: &RuntimeHookDefinition, _context: &HookContext, _sandbox: Arc, _execution_context: &HookExecutionContext, @@ -286,16 +286,18 @@ mod tests { Arc::new(Catalog::from_builtin().expect("default catalog should build")) } - fn make_hook(event: HookEvent, name: &str) -> HookDefinition { - HookDefinition { + fn make_hook(event: HookEvent, name: &str) -> RuntimeHookDefinition { + RuntimeHookDefinition { name: Some(name.into()), event, - command: Some("echo test".into()), - hook_type: None, + hook_type: Some(RuntimeHookType::Command { + command: "echo test".into(), + }), matcher: None, blocking: None, timeout_ms: None, sandbox: Some(false), + effective_name: name.into(), } } @@ -470,7 +472,9 @@ mod tests { let config = HookSettings { hooks: vec![{ let mut h = make_hook(HookEvent::RunStart, "echo-hook"); - h.command = Some("exit 0".into()); + h.hook_type = Some(RuntimeHookType::Command { + command: "exit 0".into(), + }); h }], }; @@ -488,7 +492,9 @@ mod tests { let config = HookSettings { hooks: vec![{ let mut h = make_hook(HookEvent::RunStart, "fail-hook"); - h.command = Some("exit 1".into()); + h.hook_type = Some(RuntimeHookType::Command { + command: "exit 1".into(), + }); h }], }; diff --git a/lib/crates/fabro-hooks/src/types.rs b/lib/crates/fabro-hooks/src/types.rs index 8a1cba192..7f2d523ec 100644 --- a/lib/crates/fabro-hooks/src/types.rs +++ b/lib/crates/fabro-hooks/src/types.rs @@ -3,8 +3,8 @@ use std::path::{Path, PathBuf}; use fabro_types::RunId; use serde::{Deserialize, Serialize}; -use crate::config::HookDefinition; pub use crate::config::HookEvent; +use crate::config::RuntimeHookDefinition; /// Rich JSON payload sent to hooks. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -128,7 +128,7 @@ pub struct HookExecutionContext { impl HookExecutionContext { #[must_use] - pub fn command_cwd_for(&self, definition: &HookDefinition) -> Option<&Path> { + pub fn command_cwd_for(&self, definition: &RuntimeHookDefinition) -> Option<&Path> { if definition.runs_in_sandbox() { self.sandbox_work_dir.as_deref() } else { @@ -152,17 +152,20 @@ mod tests { use fabro_types::fixtures; use super::*; + use crate::config::RuntimeHookType; - fn command_hook(sandbox: bool) -> HookDefinition { - HookDefinition { - name: Some("cwd-test".into()), - event: HookEvent::RunStart, - command: Some("pwd".into()), - hook_type: None, - matcher: None, - blocking: None, - timeout_ms: None, - sandbox: Some(sandbox), + fn command_hook(sandbox: bool) -> RuntimeHookDefinition { + RuntimeHookDefinition { + name: Some("cwd-test".into()), + event: HookEvent::RunStart, + hook_type: Some(RuntimeHookType::Command { + command: "pwd".into(), + }), + matcher: None, + blocking: None, + timeout_ms: None, + sandbox: Some(sandbox), + effective_name: "cwd-test".into(), } } diff --git a/lib/crates/fabro-hooks/tests/host_command_hooks.rs b/lib/crates/fabro-hooks/tests/host_command_hooks.rs index 4bbe0c5a3..b608a86b0 100644 --- a/lib/crates/fabro-hooks/tests/host_command_hooks.rs +++ b/lib/crates/fabro-hooks/tests/host_command_hooks.rs @@ -4,8 +4,8 @@ use std::sync::Arc; use fabro_agent::{LocalSandbox, Sandbox}; use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_hooks::{ - HookContext, HookDecision, HookDefinition, HookEvent, HookExecutionContext, HookRunner, - HookSettings, InterpString, + HookContext, HookDecision, HookEvent, HookExecutionContext, HookRunner, HookSettings, + RuntimeHookDefinition, RuntimeHookType, }; use fabro_model::Catalog; use fabro_types::RunId; @@ -41,15 +41,17 @@ async fn host_command_hook_uses_host_workdir_not_sandbox_workdir() { let runner = HookRunner::new( HookSettings { - hooks: vec![HookDefinition { - name: Some("host-marker".to_string()), - event: HookEvent::RunStart, - command: Some(InterpString::parse("printf ran > marker.txt")), - hook_type: None, - matcher: None, - blocking: Some(true), - timeout_ms: Some(5000), - sandbox: Some(false), + hooks: vec![RuntimeHookDefinition { + name: Some("host-marker".to_string()), + event: HookEvent::RunStart, + hook_type: Some(RuntimeHookType::Command { + command: "printf ran > marker.txt".to_string(), + }), + matcher: None, + blocking: Some(true), + timeout_ms: Some(5000), + sandbox: Some(false), + effective_name: "host-marker".to_string(), }], }, test_llm_source(), diff --git a/lib/crates/fabro-types/src/settings/run.rs b/lib/crates/fabro-types/src/settings/run.rs index 51410c44d..10c6769dc 100644 --- a/lib/crates/fabro-types/src/settings/run.rs +++ b/lib/crates/fabro-types/src/settings/run.rs @@ -7,6 +7,7 @@ //! behavior, and artifact collection. use std::collections::HashMap; +use std::fmt; use std::path::PathBuf; use std::time::Duration as StdDuration; @@ -1651,11 +1652,24 @@ fn resolve_env_string( if !value.contains("{{") { return Ok(()); } + *value = resolve_env_secrets(&InterpString::parse(value), env_lookup, secrets_lookup)?; + Ok(()) +} + +/// Resolve a typed [`InterpString`] against env + secrets lookups — the one +/// shared run-boundary resolution context. Tokens in any other namespace fail +/// loudly (`vars` should already be substituted server-side; `inputs` never +/// resolves in config fields), and a lookup miss is a hard error with no +/// fallback to the unresolved source. +fn resolve_env_secrets( + value: &InterpString, + env_lookup: &mut impl FnMut(&str) -> Option, + secrets_lookup: &mut impl FnMut(&str) -> Option, +) -> Result { let mut ctx = ResolveCtx::new() .with_env(&mut *env_lookup) .with_secrets(&mut *secrets_lookup); - *value = InterpString::parse(value).resolve_with(&mut ctx)?; - Ok(()) + value.resolve_with(&mut ctx) } #[cfg(test)] @@ -2256,33 +2270,10 @@ impl HookDefinition { }) } - #[must_use] - pub fn is_blocking(&self) -> bool { - self.blocking - .unwrap_or_else(|| self.event.is_blocking_by_default()) - } - - #[must_use] - pub fn timeout(&self) -> StdDuration { - if let Some(ms) = self.timeout_ms { - return StdDuration::from_millis(ms); - } - let default_ms = match self.resolved_hook_type().as_deref() { - Some(HookType::Prompt { .. }) => 30_000, - _ => 60_000, - }; - StdDuration::from_millis(default_ms) - } - - #[must_use] - pub fn runs_in_sandbox(&self) -> bool { - self.sandbox.unwrap_or(true) - } - #[must_use] #[expect( clippy::disallowed_methods, - reason = "effective_name builds a human/merge-identity label from the hook's unresolved \ + reason = "effective_name builds a human-readable label from the hook's unresolved \ template source; the source text is the intended display value here" )] pub fn effective_name(&self) -> String { @@ -2305,6 +2296,569 @@ impl HookDefinition { None => event, } } + + /// Resolve `{{ env.* }}` and `{{ secrets.* }}` tokens in this hook's + /// interpolatable fields (`command`, `url`, header values, `prompt`, + /// `model`) against the supplied lookups, producing the fully resolved + /// [`RuntimeHookDefinition`] the hook executor consumes. + /// + /// This is the late, use-time half of hook interpolation, the counterpart + /// to the server-side `{{ vars.* }}` substitution in + /// [`RunNamespace::substitute_variables`]: `{{ vars.* }}` are substituted + /// earlier, server-side, while `{{ env.* }}` and `{{ secrets.* }}` + /// resolve here — once, at the run boundary, in the worker process that + /// fires the hooks. Carrying the source form out of the config resolve + /// layer keeps `fabro validate` portable (it never requires env to be + /// set), and a referenced env var or secret that is unset is a hard error + /// — no fallback to the unresolved source and no fire-time resolution. + /// + /// HTTP hook headers keep their own, stricter policy, enforced here + /// before any lookup: a `{{ secrets.* }}` token is rejected outright + /// ([`HookResolveError::SecretsInHeader`]), and an `{{ env.* }}` token + /// must name a variable listed in the hook's `allowed_env_vars` + /// ([`HookResolveError::HeaderEnvNotAllowed`]). + pub fn resolve_env( + &self, + mut env_lookup: impl FnMut(&str) -> Option, + mut secrets_lookup: impl FnMut(&str) -> Option, + ) -> Result { + let hook_type = match self.resolved_hook_type().as_deref() { + Some(HookType::Command { command }) => Some(RuntimeHookType::Command { + command: resolve_hook_value(command, &mut env_lookup, &mut secrets_lookup)?, + }), + Some(HookType::Http { + url, + headers, + allowed_env_vars, + tls, + }) => { + let headers = headers + .as_ref() + .map(|headers| { + headers + .iter() + .map(|(name, value)| { + let value = + resolve_hook_header(value, allowed_env_vars, &mut env_lookup)?; + Ok((name.clone(), value)) + }) + .collect::, HookResolveError>>() + }) + .transpose()?; + #[expect( + clippy::disallowed_methods, + reason = "url_source deliberately carries the unresolved source so hook HTTP \ + logging never includes the resolved URL" + )] + let url_source = url.as_source(); + Some(RuntimeHookType::Http(RuntimeHttpHook { + url: resolve_hook_value(url, &mut env_lookup, &mut secrets_lookup)?, + url_source, + headers, + tls: *tls, + })) + } + Some(HookType::Prompt { prompt, model }) => Some(RuntimeHookType::Prompt { + prompt: resolve_hook_value(prompt, &mut env_lookup, &mut secrets_lookup)?, + model: model + .as_ref() + .map(|model| resolve_hook_value(model, &mut env_lookup, &mut secrets_lookup)) + .transpose()?, + }), + Some(HookType::Agent { + prompt, + model, + max_tool_rounds, + }) => Some(RuntimeHookType::Agent { + prompt: resolve_hook_value(prompt, &mut env_lookup, &mut secrets_lookup)?, + model: model + .as_ref() + .map(|model| resolve_hook_value(model, &mut env_lookup, &mut secrets_lookup)) + .transpose()?, + max_tool_rounds: *max_tool_rounds, + }), + None => None, + }; + + Ok(RuntimeHookDefinition { + name: self.name.clone(), + event: self.event, + hook_type, + matcher: self.matcher.clone(), + blocking: self.blocking, + timeout_ms: self.timeout_ms, + sandbox: self.sandbox, + effective_name: self.effective_name(), + }) + } +} + +/// Resolve `{{ env.* }}` and `{{ secrets.* }}` tokens in one hook field via +/// the shared run-boundary resolution context ([`resolve_env_secrets`]). +fn resolve_hook_value( + value: &InterpString, + env_lookup: &mut impl FnMut(&str) -> Option, + secrets_lookup: &mut impl FnMut(&str) -> Option, +) -> Result { + resolve_env_secrets(value, env_lookup, secrets_lookup).map_err(HookResolveError::Resolve) +} + +/// Resolve an HTTP hook **header** value, enforcing the header credential +/// policy before any lookup: `{{ secrets.* }}` tokens are rejected outright, +/// and `{{ env.* }}` lookups are scoped to `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-variable +/// error. An empty `allowed_env_vars` therefore permits no env vars in +/// headers at all. +fn resolve_hook_header( + value: &InterpString, + allowed_env_vars: &[String], + env_lookup: &mut impl FnMut(&str) -> Option, +) -> Result { + if let Some(name) = value.names(Namespace::Secrets).first() { + return Err(HookResolveError::SecretsInHeader { + name: (*name).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(HookResolveError::HeaderEnvNotAllowed { + name: name.to_string(), + }); + } + value + .resolve(&mut *env_lookup) + .map_err(HookResolveError::Resolve) +} + +/// An error from resolving a hook's interpolation tokens at the run boundary +/// (see [`HookDefinition::resolve_env`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum HookResolveError { + /// A `{{ secrets.* }}` token in an HTTP hook header value, rejected + /// before any vault lookup: header values travel to third-party + /// endpoints, so secrets stay out of them by policy. + SecretsInHeader { name: String }, + /// An `{{ env.* }}` token in an HTTP hook header naming a variable that + /// is not listed in the hook's `allowed_env_vars`. + HeaderEnvNotAllowed { name: String }, + /// A missing or out-of-scope token in any hook field. + Resolve(ResolveError), +} + +impl fmt::Display for HookResolveError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::SecretsInHeader { name } => write!( + f, + "secret {name:?} is not allowed in HTTP hook headers; use secret interpolation \ + in a hook command, prompt, or url instead" + ), + Self::HeaderEnvNotAllowed { 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 HookResolveError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + Self::Resolve(error) => Some(error), + Self::SecretsInHeader { .. } | Self::HeaderEnvNotAllowed { .. } => None, + } + } +} + +/// A hook definition with every interpolatable field resolved to a plain +/// string, produced at the run boundary by [`HookDefinition::resolve_env`]. +/// +/// This is the form the hook executor consumes: it formats, dispatches, and +/// merges decisions, but never resolves tokens — the hooks subsystem never +/// learns about process env or vault secrets. The type is deliberately not +/// serializable: resolved values may carry secrets and must not round-trip +/// through manifests or events. For the same reason, never log or +/// `Debug`-format a whole definition — log `effective_name` instead. +#[derive(Debug, Clone, PartialEq)] +pub struct RuntimeHookDefinition { + pub name: Option, + pub event: HookEvent, + pub hook_type: Option, + pub matcher: Option, + pub blocking: Option, + pub timeout_ms: Option, + pub sandbox: Option, + /// Human-readable label for logs, built at the boundary from the hook's + /// *unresolved* config sources so resolved secret material never reaches + /// log output. + pub effective_name: String, +} + +impl RuntimeHookDefinition { + #[must_use] + pub fn is_blocking(&self) -> bool { + self.blocking + .unwrap_or_else(|| self.event.is_blocking_by_default()) + } + + #[must_use] + pub fn timeout(&self) -> StdDuration { + if let Some(ms) = self.timeout_ms { + return StdDuration::from_millis(ms); + } + let default_ms = match self.hook_type { + Some(RuntimeHookType::Prompt { .. }) => 30_000, + _ => 60_000, + }; + StdDuration::from_millis(default_ms) + } + + #[must_use] + pub fn runs_in_sandbox(&self) -> bool { + self.sandbox.unwrap_or(true) + } +} + +/// The resolved counterpart of [`HookType`], carried by +/// [`RuntimeHookDefinition`]. +/// +/// Like [`RuntimeHookDefinition`], the resolved values may carry secrets: +/// never log or `Debug`-format them — log the definition's `effective_name` +/// (and, for HTTP hooks, the `url_source`) instead. +#[derive(Debug, Clone, PartialEq)] +pub enum RuntimeHookType { + Command { + command: String, + }, + Http(RuntimeHttpHook), + Prompt { + prompt: String, + model: Option, + }, + Agent { + prompt: String, + model: Option, + max_tool_rounds: Option, + }, +} + +/// The resolved payload of an HTTP hook, carried by [`RuntimeHookType::Http`]. +/// +/// A dedicated struct (rather than inline variant fields) so `url` and +/// `url_source` — two same-typed strings with very different logging rules — +/// can never be swapped positionally at a call site. +#[derive(Debug, Clone, PartialEq)] +pub struct RuntimeHttpHook { + pub url: String, + /// The unresolved source of `url`, carried so hook HTTP logging can keep + /// pointing at the config source instead of the resolved URL (which may + /// embed secret material). + pub url_source: String, + pub headers: Option>, + pub tls: TlsMode, +} + +#[cfg(test)] +mod hook_resolve_env_tests { + use std::collections::HashMap; + + use super::super::interp::{Namespace, ResolveErrorKind}; + use super::{ + HookDefinition, HookEvent, HookResolveError, HookType, InterpString, RuntimeHookType, + TlsMode, pair_lookup as env_lookup, pair_lookup as secret_lookup, + }; + + fn hook(hook_type: HookType) -> HookDefinition { + HookDefinition { + name: Some("test-hook".to_string()), + event: HookEvent::RunStart, + command: None, + hook_type: Some(hook_type), + matcher: None, + blocking: None, + timeout_ms: None, + sandbox: Some(false), + } + } + + fn http_hook(url: &str, headers: &[(&str, &str)], allowed_env_vars: &[&str]) -> HookDefinition { + hook(HookType::Http { + url: InterpString::parse(url), + headers: (!headers.is_empty()).then(|| { + headers + .iter() + .map(|(name, value)| ((*name).to_string(), InterpString::parse(value))) + .collect() + }), + allowed_env_vars: allowed_env_vars + .iter() + .map(|name| (*name).to_string()) + .collect(), + tls: TlsMode::Off, + }) + } + + #[test] + fn command_resolves_env_and_secret_tokens() { + let definition = hook(HookType::Command { + command: InterpString::parse( + "deploy --token {{ secrets.TOKEN }} --region {{ env.REGION }}", + ), + }); + + let resolved = definition + .resolve_env( + env_lookup(&[("REGION", "us-east-1")]), + secret_lookup(&[("TOKEN", "vault-value")]), + ) + .unwrap(); + + assert_eq!( + resolved.hook_type, + Some(RuntimeHookType::Command { + command: "deploy --token vault-value --region us-east-1".to_string(), + }) + ); + assert_eq!(resolved.effective_name, "test-hook"); + } + + #[test] + fn legacy_command_field_resolves() { + let definition = HookDefinition { + hook_type: None, + command: Some(InterpString::parse("echo {{ env.MARKER }}")), + ..hook(HookType::Command { + command: InterpString::parse("unused"), + }) + }; + + let resolved = definition + .resolve_env(env_lookup(&[("MARKER", "ok")]), secret_lookup(&[])) + .unwrap(); + + assert_eq!( + resolved.hook_type, + Some(RuntimeHookType::Command { + command: "echo ok".to_string(), + }) + ); + } + + #[test] + fn missing_secret_is_hard_error_naming_the_secret() { + let definition = hook(HookType::Command { + command: InterpString::parse("deploy {{ secrets.MISSING_TOKEN }}"), + }); + + let err = definition + .resolve_env(env_lookup(&[]), secret_lookup(&[])) + .unwrap_err(); + + let HookResolveError::Resolve(error) = err else { + panic!("expected resolve error, got {err:?}"); + }; + assert_eq!(error.namespace, Namespace::Secrets); + assert_eq!(error.name, "MISSING_TOKEN"); + assert_eq!(error.kind, ResolveErrorKind::Missing); + } + + // `inputs` stays template-only: a hook field referencing it fails loudly + // as an unavailable namespace, exactly as fire-time resolution did. + #[test] + fn inputs_token_is_unavailable_namespace_error() { + let definition = hook(HookType::Command { + command: InterpString::parse("echo {{ inputs.ticket }}"), + }); + + let err = definition + .resolve_env(env_lookup(&[]), secret_lookup(&[])) + .unwrap_err(); + + let HookResolveError::Resolve(error) = err else { + panic!("expected resolve error, got {err:?}"); + }; + assert_eq!(error.kind, ResolveErrorKind::Unavailable); + } + + #[test] + fn http_url_resolves_secret_and_carries_unresolved_source() { + let definition = http_hook("{{ secrets.HOOK_URL }}/notify", &[], &[]); + + let resolved = definition + .resolve_env( + env_lookup(&[]), + secret_lookup(&[("HOOK_URL", "https://user:pw@hooks.example.com")]), + ) + .unwrap(); + + let Some(RuntimeHookType::Http(http)) = resolved.hook_type else { + panic!("expected http hook type"); + }; + assert_eq!(http.url, "https://user:pw@hooks.example.com/notify"); + assert_eq!(http.url_source, "{{ secrets.HOOK_URL }}/notify"); + } + + #[test] + fn header_resolves_allowlisted_env_var() { + let definition = http_hook( + "https://hooks.example.com", + &[("Authorization", "Bearer {{ env.HOOK_KEY }}")], + &["HOOK_KEY"], + ); + + let resolved = definition + .resolve_env(env_lookup(&[("HOOK_KEY", "key-123")]), secret_lookup(&[])) + .unwrap(); + + let Some(RuntimeHookType::Http(http)) = resolved.hook_type else { + panic!("expected http hook type"); + }; + assert_eq!( + http.headers, + Some(HashMap::from([( + "Authorization".to_string(), + "Bearer key-123".to_string(), + )])) + ); + } + + // A secret token in a header is rejected before any vault lookup — the + // panicking secrets lookup proves the value is never fetched. + #[test] + fn header_secret_token_is_rejected_before_lookup() { + let definition = http_hook( + "https://hooks.example.com", + &[("Authorization", "Bearer {{ secrets.HOOK_KEY }}")], + &[], + ); + + let err = definition + .resolve_env(env_lookup(&[]), |_: &str| -> Option { + panic!("secrets lookup must not run for header values") + }) + .unwrap_err(); + + assert_eq!(err, HookResolveError::SecretsInHeader { + name: "HOOK_KEY".to_string(), + }); + assert_eq!( + err.to_string(), + "secret \"HOOK_KEY\" is not allowed in HTTP hook headers; use secret interpolation \ + in a hook command, prompt, or url instead" + ); + } + + // 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 error points at the allowlist. + #[test] + fn header_rejects_env_var_outside_allowlist() { + let definition = http_hook( + "https://hooks.example.com", + &[("Authorization", "Bearer {{ env.HOOK_KEY }}")], + &[], + ); + + let err = definition + .resolve_env( + env_lookup(&[("HOOK_KEY", "should_not_appear")]), + secret_lookup(&[]), + ) + .unwrap_err(); + + assert_eq!(err, HookResolveError::HeaderEnvNotAllowed { + name: "HOOK_KEY".to_string(), + }); + assert_eq!( + err.to_string(), + "environment variable \"HOOK_KEY\" referenced by an HTTP hook header is not listed \ + in allowed_env_vars" + ); + } + + #[test] + fn header_allowlisted_but_unset_env_var_is_missing_error() { + let definition = http_hook( + "https://hooks.example.com", + &[("Authorization", "Bearer {{ env.HOOK_KEY }}")], + &["HOOK_KEY"], + ); + + let err = definition + .resolve_env(env_lookup(&[]), secret_lookup(&[])) + .unwrap_err(); + + let HookResolveError::Resolve(error) = err else { + panic!("expected resolve error, got {err:?}"); + }; + assert_eq!(error.name, "HOOK_KEY"); + assert_eq!(error.kind, ResolveErrorKind::Missing); + } + + #[test] + fn prompt_and_model_resolve_tokens() { + let definition = hook(HookType::Prompt { + prompt: InterpString::parse("check {{ secrets.POLICY }}"), + model: Some(InterpString::parse("{{ env.HOOK_MODEL }}")), + }); + + let resolved = definition + .resolve_env( + env_lookup(&[("HOOK_MODEL", "haiku")]), + secret_lookup(&[("POLICY", "no-force-push")]), + ) + .unwrap(); + + assert_eq!( + resolved.hook_type, + Some(RuntimeHookType::Prompt { + prompt: "check no-force-push".to_string(), + model: Some("haiku".to_string()), + }) + ); + } + + #[test] + fn definition_without_hook_type_resolves_to_none() { + let definition = HookDefinition { + name: None, + event: HookEvent::StageStart, + command: None, + hook_type: None, + matcher: None, + blocking: None, + timeout_ms: None, + sandbox: None, + }; + + let resolved = definition + .resolve_env(env_lookup(&[]), secret_lookup(&[])) + .unwrap(); + + assert_eq!(resolved.hook_type, None); + assert_eq!(resolved.effective_name, "stage_start"); + } + + #[test] + fn runtime_defaults_match_config_semantics() { + let resolved = hook(HookType::Prompt { + prompt: InterpString::parse("ok?"), + model: None, + }) + .resolve_env(env_lookup(&[]), secret_lookup(&[])) + .unwrap(); + + // RunStart is blocking by default; prompt hooks default to 30s. + assert!(resolved.is_blocking()); + assert_eq!(resolved.timeout(), std::time::Duration::from_secs(30)); + assert!(!resolved.runs_in_sandbox()); + } } #[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 7fa47e938..bd9b46fba 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -16,9 +16,10 @@ use fabro_sandbox::from_environment::{ use fabro_sandbox::{DockerSandboxOptions, SandboxSpec}; use fabro_static::EnvVars; use fabro_types::settings::run::{ - ApprovalMode, McpServerSettings as ResolvedMcpServerSettings, PullRequestSettings, - ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, + ApprovalMode, HookDefinition, McpServerSettings as ResolvedMcpServerSettings, + PullRequestSettings, ResolvedMcpEntry, RunMode, RunModelSettings as ResolvedRunModelSettings, RunNamespace as ResolvedRunSettings, RunPrepareSettings as ResolvedRunPrepareSettings, + RuntimeHookDefinition, }; use fabro_types::settings::{ModelRegistry, ResolvedModelRef}; use fabro_types::{ManifestPath, RunId, RunRunnableSource, SandboxProviderKind}; @@ -463,6 +464,7 @@ impl RunSession { let pr_config = resolved.pull_request.clone(); let setup_commands = runtime_setup_commands(&resolved.prepare, process_env_var, secret_lookup)?; + let hooks = runtime_hooks(&resolved.hooks, process_env_var, secret_lookup)?; drop(vault_guard); Ok(Self { @@ -486,9 +488,7 @@ impl RunSession { setup_commands, setup_command_timeout_ms: resolved.prepare.timeout_ms, }, - hooks: fabro_hooks::HookSettings { - hooks: resolved.hooks.clone(), - }, + hooks: fabro_hooks::HookSettings { hooks }, sandbox_env, seed_context: None, run_store: services.run_store, @@ -764,6 +764,39 @@ fn runtime_setup_commands( .collect()) } +/// Build the launch-time hook definitions from resolved settings, resolving +/// any `{{ env.* }}` and `{{ secrets.* }}` tokens in each hook's +/// `command`/`url`/`headers`/`prompt`/`model` against the worker process +/// environment and vault — the run boundary, so the hook executor only ever +/// sees fully resolved strings and fire time involves no resolution at all. +/// +/// The resolution itself lives on the type ([`HookDefinition::resolve_env`]) +/// together with the HTTP-header policy (secret tokens are rejected in +/// headers; header env reads stay scoped to `allowed_env_vars`); this wrapper +/// just adds the hook's name to the error. Hook strings are carried in source +/// form out of the config resolve layer so `fabro validate` stays portable +/// (it never requires env to be set), and a referenced env var or secret that +/// is unset is a hard error that fails the run at startup — even for hooks +/// that would never have fired. +fn runtime_hooks( + hooks: &[HookDefinition], + mut env_lookup: impl FnMut(&str) -> Option, + mut secrets_lookup: impl FnMut(&str) -> Option, +) -> Result, Error> { + hooks + .iter() + .map(|hook| { + hook.resolve_env(&mut env_lookup, &mut secrets_lookup) + .map_err(|err| { + Error::engine_with_source( + format!("failed to resolve hook {:?}", hook.effective_name()), + err, + ) + }) + }) + .collect() +} + impl RunSession { /// Shared engine: initialize, execute, finalize, pull_request. async fn run( @@ -1125,8 +1158,8 @@ mod tests { }; use fabro_store::Database; use fabro_types::settings::run::{ - McpTransport as ResolvedMcpTransport, PreparedStep, PreparedStepRun, RunMode, - RunPrepareSettings, + HookEvent, HookType, McpTransport as ResolvedMcpTransport, PreparedStep, PreparedStepRun, + RunMode, RunPrepareSettings, RuntimeHookType, }; use fabro_types::settings::{InterpString, ModelRef}; use fabro_types::{ @@ -1489,6 +1522,85 @@ reasoning = false assert!(err.causes()[0].contains("GITHUB_APP_PRIVATE_KEY")); } + fn command_hook(name: &str, command: &str) -> HookDefinition { + HookDefinition { + name: Some(name.to_string()), + event: HookEvent::RunStart, + command: Some(InterpString::parse(command)), + hook_type: None, + matcher: None, + blocking: None, + timeout_ms: None, + sandbox: Some(false), + } + } + + #[test] + fn runtime_hooks_resolve_secret_tokens_from_vault() { + let vault = token_vault("HOOK_TOKEN", "vault-token"); + let hooks = [command_hook("guard", "deploy {{ secrets.HOOK_TOKEN }}")]; + + let resolved = runtime_hooks(&hooks, |_| None, vault_secret_lookup(&vault)).unwrap(); + + assert_eq!( + resolved[0].hook_type, + Some(RuntimeHookType::Command { + command: "deploy vault-token".to_string(), + }) + ); + } + + // Fail timing note: a missing hook secret used to surface as a fire-time + // Block; boundary resolution turns it into a startup failure, including + // for hooks that would never have fired. + #[test] + fn runtime_hooks_missing_secret_fails_naming_hook_and_secret() { + let vault = temp_vault(&[]); + let hooks = [command_hook("guard", "deploy {{ secrets.HOOK_TOKEN }}")]; + + let err = runtime_hooks(&hooks, |_| None, vault_secret_lookup(&vault)).unwrap_err(); + + assert_eq!( + err.to_string(), + "Engine error: failed to resolve hook \"guard\"" + ); + assert!(err.causes()[0].contains("HOOK_TOKEN")); + } + + #[test] + fn runtime_hooks_reject_secret_in_http_header_with_guidance() { + let vault = token_vault("HOOK_TOKEN", "vault-token"); + let hooks = [HookDefinition { + name: Some("notify".to_string()), + event: HookEvent::RunComplete, + command: None, + hook_type: Some(HookType::Http { + url: InterpString::parse("https://hooks.example.com"), + headers: Some(HashMap::from([( + "Authorization".to_string(), + InterpString::parse("Bearer {{ secrets.HOOK_TOKEN }}"), + )])), + allowed_env_vars: Vec::new(), + tls: fabro_types::settings::run::TlsMode::Verify, + }), + matcher: None, + blocking: None, + timeout_ms: None, + sandbox: None, + }]; + + let err = runtime_hooks(&hooks, |_| None, vault_secret_lookup(&vault)).unwrap_err(); + + assert_eq!( + err.to_string(), + "Engine error: failed to resolve hook \"notify\"" + ); + assert!(err.causes()[0].contains( + "not allowed in HTTP hook headers; use secret interpolation in a hook command, \ + prompt, or url instead" + )); + } + #[tokio::test] async fn run_session_new_resolves_secret_tokens_from_vault_at_boundary() { let temp = tempfile::tempdir().unwrap(); @@ -1525,6 +1637,7 @@ reasoning = false ..ResolvedMcpServerSettings::default() }), ); + settings.run.hooks = vec![command_hook("guard", "deploy {{ secrets.DEPLOY_TOKEN }}")]; let (persisted, store) = persisted_workflow_with_settings(MINIMAL_DOT, &storage_root, settings).await; let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); @@ -1546,6 +1659,12 @@ reasoning = false .map(String::as_str), Some("vault-token") ); + assert_eq!( + session.hooks.hooks[0].hook_type, + Some(RuntimeHookType::Command { + command: "deploy vault-token".to_string(), + }) + ); assert_eq!( session.lifecycle.setup_commands[0] .env @@ -1605,6 +1724,40 @@ reasoning = false assert!(err.causes()[0].contains("DEPLOY_TOKEN")); } + #[tokio::test] + async fn run_session_new_missing_hook_secret_fails_startup() { + let temp = tempfile::tempdir().unwrap(); + let (storage_root, _run_dir) = storage_root_and_run_dir(&temp); + let mut settings = settings_from_run_layer(RunLayer { + execution: Some(RunExecutionLayer { + mode: Some(RunMode::DryRun), + ..RunExecutionLayer::default() + }), + ..RunLayer::default() + }); + settings.run.hooks = vec![command_hook("guard", "deploy {{ secrets.HOOK_TOKEN }}")]; + let (persisted, store) = + persisted_workflow_with_settings(MINIMAL_DOT, &storage_root, settings).await; + let emitter = Arc::new(Emitter::new(fixtures::RUN_1)); + let registry = Arc::new(test_registry()); + let vault = Arc::new(AsyncRwLock::new(temp_vault(&[]))); + + let Err(err) = RunSession::new(&persisted, StartServices { + vault: Some(vault), + ..test_start_services(&store, &storage_root, emitter, registry).await + }) + .await + else { + panic!("missing hook secret should fail run startup"); + }; + + assert_eq!( + err.to_string(), + "Engine error: failed to resolve hook \"guard\"" + ); + assert!(err.causes()[0].contains("HOOK_TOKEN")); + } + #[test] fn runtime_docker_config_maps_environment_hints() { let settings = settings_from_run_layer(RunLayer { diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 54a6f1f45..d6c40f7f4 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -283,11 +283,13 @@ pub async fn initialize( let sandbox_git = Arc::new(SandboxGitRuntime::new()); let metadata_runtime = Arc::new(RunMetadataRuntime::new()); + // Move (not clone) the hooks into the runner: boundary-resolved hooks + // carry resolved secret values, so keep a single copy alive for the run. let hook_runner = if options.hooks.hooks.is_empty() { None } else { Some(Arc::new(HookRunner::new( - options.hooks.clone(), + std::mem::take(&mut options.hooks), Arc::clone(&llm_source), Arc::clone(&catalog), ))) diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 3ceeced5d..6de6238af 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -8174,7 +8174,9 @@ fn subgraph_without_label_no_class_derived() { // Hook System E2E Tests // --------------------------------------------------------------------------- -fn hook_runner_from_defs(hooks: Vec) -> Arc { +fn hook_runner_from_defs( + hooks: Vec, +) -> Arc { Arc::new(fabro_hooks::HookRunner::new( fabro_hooks::HookSettings { hooks }, Arc::new(fabro_auth::EnvCredentialSource::new()), @@ -8227,7 +8229,7 @@ fn emitter_with_events() -> (Arc, Arc>>) (Arc::new(emitter), events) } -fn engine_with_hooks(hooks: Vec) -> HookTestRunner { +fn engine_with_hooks(hooks: Vec) -> HookTestRunner { HookTestRunner { emitter: Arc::new(Emitter::default()), hook_runner: hook_runner_from_defs(hooks), @@ -8235,7 +8237,7 @@ fn engine_with_hooks(hooks: Vec) -> HookTestRunner } fn engine_with_hooks_and_events( - hooks: Vec, + hooks: Vec, ) -> (HookTestRunner, Arc>>) { let (emitter, events) = emitter_with_events(); ( @@ -8264,16 +8266,18 @@ fn make_run_options(dir: &std::path::Path) -> RunOptions { } } -fn make_hook(event: fabro_hooks::HookEvent, command: &str) -> fabro_hooks::HookDefinition { - fabro_hooks::HookDefinition { +fn make_hook(event: fabro_hooks::HookEvent, command: &str) -> fabro_hooks::RuntimeHookDefinition { + fabro_hooks::RuntimeHookDefinition { name: None, event, - command: Some(command.into()), - hook_type: None, + hook_type: Some(fabro_hooks::RuntimeHookType::Command { + command: command.to_string(), + }), matcher: None, blocking: None, timeout_ms: Some(5000), sandbox: Some(false), // run on host for test reliability + effective_name: format!("{event}:{command}"), } } @@ -8837,97 +8841,12 @@ async fn hook_stage_start_exit_2_blocks() { assert!(result.is_err(), "exit 2 should block"); } -// --- Config merge tests (server + run) --- - -#[tokio::test] -async fn hook_config_merge_concatenates() { - use fabro_hooks::{HookDefinition, HookEvent, HookSettings}; - - let server_hooks = HookSettings { - hooks: vec![HookDefinition { - name: Some("server-hook".into()), - event: HookEvent::RunStart, - command: Some("exit 0".into()), - hook_type: None, - matcher: None, - blocking: None, - timeout_ms: None, - sandbox: Some(false), - }], - }; - let run_hooks = HookSettings { - hooks: vec![HookDefinition { - name: Some("run-hook".into()), - event: HookEvent::StageComplete, - command: Some("exit 0".into()), - hook_type: None, - matcher: None, - blocking: None, - timeout_ms: None, - sandbox: Some(false), - }], - }; - - let merged = server_hooks.merge(run_hooks); - assert_eq!(merged.hooks.len(), 2); - assert_eq!(merged.hooks[0].name.as_deref(), Some("server-hook")); - assert_eq!(merged.hooks[1].name.as_deref(), Some("run-hook")); -} - -#[tokio::test] -async fn hook_config_merge_run_overrides_by_name() { - use fabro_hooks::{HookDefinition, HookEvent, HookSettings}; - - let server_hooks = HookSettings { - hooks: vec![HookDefinition { - name: Some("shared".into()), - event: HookEvent::RunStart, - command: Some("exit 1".into()), // would block - hook_type: None, - matcher: None, - blocking: None, - timeout_ms: None, - sandbox: Some(false), - }], - }; - let run_hooks = HookSettings { - hooks: vec![HookDefinition { - name: Some("shared".into()), - event: HookEvent::RunStart, - command: Some("exit 0".into()), // allows - hook_type: None, - matcher: None, - blocking: None, - timeout_ms: None, - sandbox: Some(false), - }], - }; - - 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_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); - let graph = parse(simple_linear_dot()).unwrap(); - let dir = tempfile::tempdir().unwrap(); - let run_options = make_run_options(dir.path()); - - let outcome = engine.run(&graph, &run_options).await.unwrap(); - assert_eq!(outcome.status, StageOutcome::Succeeded); -} - // The legacy `Settings`-based TOML parsing tests were deleted in Stage // 6.3b. Hook TOML parsing now flows through the v2 config parser path, // with coverage in fabro-config unit tests and the fabro-cli integration -// tests under `cmd::config`. +// tests under `cmd::config`. Server/run hook config merging likewise lives +// in the config layer resolution; the resolved list arrives here already +// merged, so the deleted `HookSettings::merge` had no production callers. // --- Blocking vs non-blocking behavior --- @@ -9122,6 +9041,233 @@ async fn hooks_do_not_duplicate_workflow_events() { assert_eq!(run_failed, 0, "Should have 0 WorkflowRunFailed"); } +// --- Boundary-resolved interpolation (env + vault secrets) --- +// +// Hook InterpStrings resolve once, at the run boundary +// (`operations::start::runtime_hooks`); these tests drive the same resolver +// (`HookDefinition::resolve_env`) against a hermetic temp-dir vault and then +// run the resolved hooks through the engine, proving the executor fires +// correctly on fully resolved strings. + +fn boundary_resolved_hooks( + hooks: &[fabro_types::settings::run::HookDefinition], + env: &'static [(&'static str, &'static str)], + vault: &fabro_vault::Vault, +) -> Vec { + hooks + .iter() + .map(|hook| { + hook.resolve_env( + |name| { + env.iter() + .find_map(|(key, value)| (*key == name).then(|| (*value).to_string())) + }, + |name| fabro_auth::vault_get_token(vault, name).ok().flatten(), + ) + .expect("hook should resolve at the run boundary") + }) + .collect() +} + +fn config_command_hook( + event: fabro_hooks::HookEvent, + command: &str, + sandbox: bool, +) -> fabro_types::settings::run::HookDefinition { + fabro_types::settings::run::HookDefinition { + name: None, + event, + command: Some(fabro_types::settings::InterpString::parse(command)), + hook_type: None, + matcher: None, + blocking: None, + timeout_ms: Some(5000), + sandbox: Some(sandbox), + } +} + +fn hook_test_vault(dir: &std::path::Path, entries: &[(&str, &str)]) -> fabro_vault::Vault { + let mut vault = fabro_vault::Vault::load(dir.join("secrets.json")) + .expect("temp-dir vault should load for hook tests"); + for (name, value) in entries { + fabro_auth::vault_set_token(&mut vault, name, value) + .expect("test secret should store in temp vault"); + } + vault +} + +#[tokio::test] +async fn hook_command_secret_resolves_from_vault_and_proceeds() { + let dir = tempfile::tempdir().unwrap(); + let vault = hook_test_vault(dir.path(), &[("HOOK_TOKEN", "hook-secret-value")]); + let hooks = boundary_resolved_hooks( + &[config_command_hook( + fabro_hooks::HookEvent::RunStart, + r#"test "{{ secrets.HOOK_TOKEN }}" = "hook-secret-value""#, + false, + )], + &[], + &vault, + ); + + let engine = engine_with_hooks(hooks); + let graph = parse(simple_linear_dot()).unwrap(); + let run_options = make_run_options(dir.path()); + + let outcome = engine.run(&graph, &run_options).await.unwrap(); + assert_eq!(outcome.status, StageOutcome::Succeeded); +} + +#[tokio::test] +async fn hook_http_secret_url_resolves_from_vault_and_fires() { + 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 dir = tempfile::tempdir().unwrap(); + let vault = hook_test_vault(dir.path(), &[("HOOK_URL", &server.url("/hook"))]); + let hooks = boundary_resolved_hooks( + &[fabro_types::settings::run::HookDefinition { + name: Some("notify".into()), + event: fabro_hooks::HookEvent::RunStart, + command: None, + hook_type: Some(fabro_types::settings::run::HookType::Http { + url: fabro_types::settings::InterpString::parse( + "{{ secrets.HOOK_URL }}", + ), + headers: None, + allowed_env_vars: Vec::new(), + tls: fabro_hooks::TlsMode::Off, + }), + matcher: None, + blocking: None, + timeout_ms: Some(5000), + sandbox: Some(false), + }], + &[], + &vault, + ); + + let engine = engine_with_hooks(hooks); + let graph = parse(simple_linear_dot()).unwrap(); + let run_options = make_run_options(dir.path()); + + let outcome = engine.run(&graph, &run_options).await.unwrap(); + assert_eq!(outcome.status, StageOutcome::Succeeded); + mock.assert_async().await; +} + +// Hook secrets get the standard content-based redaction coverage: a block +// reason that echoes a resolved credential-shaped secret value is redacted +// where events are serialized into the run store. Low-entropy values are out +// of coverage by design, so this asserts only on a credential-shaped marker. +#[tokio::test] +async fn hook_block_reason_echoing_secret_is_redacted_in_stored_events() { + // Same distinctive credential-shaped test marker as the fabro-redact and + // event-redaction tests; never a real credential. + const CREDENTIAL_SHAPED_SECRET: &str = "sk-ant-api03-xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"; + + let dir = tempfile::tempdir().unwrap(); + let vault = hook_test_vault(dir.path(), &[("HOOK_TOKEN", CREDENTIAL_SHAPED_SECRET)]); + let hooks = boundary_resolved_hooks( + &[config_command_hook( + fabro_hooks::HookEvent::RunStart, + r#"echo '{"decision": "block", "reason": "denied by {{ secrets.HOOK_TOKEN }}"}'"#, + false, + )], + &[], + &vault, + ); + + let engine = engine_with_hooks(hooks); + let graph = parse(simple_linear_dot()).unwrap(); + let run_options = make_run_options(dir.path()); + + let result = engine.run(&graph, &run_options).await; + assert!(result.is_err(), "blocking hook should fail the run"); + + // Reopen the run store and inspect the stored (redacted) event payloads. + let store_dir = test_store_dir(&run_options.run_dir); + let store = Database::new( + Arc::new(LocalFileSystem::new_with_prefix(&store_dir).unwrap()), + "", + Duration::from_millis(1), + None, + ); + let run_store = store.open_run_reader(&run_options.run_id).await.unwrap(); + let events = run_store.list_events().await.unwrap(); + let stored_text = events + .iter() + .map(|event| event.event.to_value().unwrap().to_string()) + .collect::>() + .join("\n"); + + assert!( + stored_text.contains("denied by"), + "block reason should reach stored events: {stored_text}" + ); + assert!( + !stored_text.contains(CREDENTIAL_SHAPED_SECRET), + "credential-shaped secret must not appear in stored events" + ); + assert!( + stored_text.contains("REDACTED"), + "redaction marker should replace the secret value: {stored_text}" + ); +} + +// Env-only hooks keep working through boundary resolution, on both dispatch +// paths: host-side (`sandbox = false`) and sandbox-side (`sandbox = true`). +#[tokio::test] +async fn hook_env_tokens_resolve_at_boundary_for_host_and_sandbox_dispatch() { + let dir = tempfile::tempdir().unwrap(); + let vault = hook_test_vault(dir.path(), &[]); + let host_marker = dir.path().join("host_marker.txt"); + let sandbox_marker = dir.path().join("sandbox_marker.txt"); + let hooks = boundary_resolved_hooks( + &[ + config_command_hook( + fabro_hooks::HookEvent::RunStart, + &format!( + r#"printf %s "{{{{ env.HOOK_MARKER }}}}" > {}"#, + host_marker.display() + ), + false, + ), + config_command_hook( + fabro_hooks::HookEvent::RunStart, + &format!( + r#"printf %s "{{{{ env.HOOK_MARKER }}}}" > {}"#, + sandbox_marker.display() + ), + true, + ), + ], + &[("HOOK_MARKER", "marker-value")], + &vault, + ); + + let engine = engine_with_hooks(hooks); + let graph = parse(simple_linear_dot()).unwrap(); + let run_options = make_run_options(dir.path()); + + let outcome = engine.run(&graph, &run_options).await.unwrap(); + assert_eq!(outcome.status, StageOutcome::Succeeded); + + assert_eq!( + std::fs::read_to_string(&host_marker).unwrap(), + "marker-value" + ); + assert_eq!( + std::fs::read_to_string(&sandbox_marker).unwrap(), + "marker-value" + ); +} + // --------------------------------------------------------------------------- // Fidelity preamble injection: verify prompt.md contains preamble + prompt // for each fidelity mode, using script → codergen pipeline with no live LLM.