Simplify hook config reuse

This commit is contained in:
Scott Werner 2026-06-30 13:09:33 -04:00
parent 5c25c938dc
commit 2c2fdc7552
6 changed files with 154 additions and 355 deletions

3
Cargo.lock generated
View file

@ -2524,11 +2524,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",

View file

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

View file

@ -1,211 +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.
//! Hook configuration runtime settings.
use std::borrow::Cow;
use fabro_types::settings::InterpString;
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: InterpString,
},
Http {
url: InterpString,
headers: Option<std::collections::HashMap<String, InterpString>>,
#[serde(default)]
allowed_env_vars: Vec<String>,
#[serde(default)]
tls: TlsMode,
},
Prompt {
prompt: InterpString,
model: Option<InterpString>,
},
Agent {
prompt: InterpString,
model: Option<InterpString>,
max_tool_rounds: Option<u32>,
},
}
/// A single hook definition.
#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)]
pub struct HookDefinition {
pub name: Option<String>,
pub event: HookEvent,
/// Inline command shorthand — if set, implies `type = "command"`.
#[serde(default)]
pub command: Option<InterpString>,
/// Explicit hook type (command or http). If omitted and `command` is set,
/// defaults to `Command`.
#[serde(flatten)]
pub hook_type: Option<HookType>,
/// Regex matched against node_id, handler_type, or event-specific fields.
pub matcher: Option<String>,
/// Override the event's default blocking behavior.
pub blocking: Option<bool>,
/// Timeout in milliseconds (default: 60_000).
pub timeout_ms: Option<u64>,
/// Run inside the sandbox (true, default) or on the host (false).
pub sandbox: Option<bool>,
}
impl HookDefinition {
/// Resolve the effective hook type: explicit `hook_type` wins, then
/// `command` shorthand, then error.
pub fn resolved_hook_type(&self) -> Option<Cow<'_, HookType>> {
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]
#[expect(
clippy::disallowed_methods,
reason = "effective_name builds a human/merge-identity label from the hook's unresolved \
template source; the source text is the intended display value here"
)]
pub fn effective_name(&self) -> String {
if let Some(ref n) = self.name {
return n.clone();
}
let event_str = self.event.to_string();
match self.resolved_hook_type().as_deref() {
Some(HookType::Command { ref command }) => {
let source = command.as_source();
let short = &source[..source.floor_char_boundary(20)];
format!("{event_str}:{short}")
}
Some(HookType::Http { ref url, .. }) => format!("{event_str}:{}", url.as_source()),
Some(HookType::Prompt { ref prompt, .. } | HookType::Agent { ref prompt, .. }) => {
let source = prompt.as_source();
let short = &source[..source.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 {

View file

@ -12,6 +12,8 @@ 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_redact::DisplaySafeUrl;
use fabro_types::settings::interp::Namespace;
use fabro_types::settings::{InterpString, ResolveError};
use fabro_util::env::{Env, SystemEnv};
use tokio::process::Command as TokioCommand;
@ -80,34 +82,78 @@ where
.map(|resolved| resolved.value)
}
#[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; DisplaySafeUrl handles literal credentials in parseable \
source URLs, and unparseable sources are not logged"
)]
fn safe_url_source_for_log(url: &InterpString) -> String {
let source = url.as_source();
DisplaySafeUrl::parse(&source).map_or_else(
|_| "<unparseable url source>".to_string(),
|url| url.redacted_string(),
)
}
#[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 looks
/// up as absent, so it surfaces as the same fail-closed `Missing` error as an
/// unset variable and the hook blocks instead of firing. 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.
/// 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<E>(
value: &InterpString,
allowed_env_vars: &[String],
env: &E,
) -> Result<String, ResolveError>
) -> Result<String, HeaderResolveError>
where
E: Env + ?Sized,
{
value
.resolve(|name| {
if allowed_env_vars.iter().any(|allowed| allowed == name) {
env.var(name).ok()
} else {
None
}
})
.map(|resolved| resolved.value)
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.
@ -166,7 +212,7 @@ impl HookExecutorImpl {
env: &E,
) -> HookDecision
where
E: Env + Clone + Send + Sync + fmt::Debug + 'static,
E: Env + ?Sized,
{
let command = match resolve_interp(command, env) {
Ok(command) => command,
@ -325,7 +371,7 @@ impl HookExecutorImpl {
catalog: Arc<Catalog>,
) -> HookDecision
where
E: Env + Clone + Send + Sync + fmt::Debug + 'static,
E: Env + ?Sized,
{
let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model, env) {
Ok(resolved) => resolved,
@ -394,7 +440,7 @@ impl HookExecutorImpl {
catalog: Arc<Catalog>,
) -> HookDecision
where
E: Env + Clone + Send + Sync + fmt::Debug + 'static,
E: Env + ?Sized,
{
let (prompt, model) = match Self::resolve_prompt_and_model(prompt, model, env) {
Ok(resolved) => resolved,
@ -538,27 +584,16 @@ impl HookExecutorImpl {
env: &E,
) -> HookDecision
where
E: Env + Clone + Send + Sync + fmt::Debug + 'static,
E: Env + ?Sized,
{
let resolved_url = match resolve_interp(url, env) {
Ok(url) => url,
Err(error) => {
// Log the unresolved source, never the resolved URL, so a
// credential embedded in the URL (userinfo/query) cannot leak.
#[expect(
clippy::disallowed_methods,
reason = "logging the unresolved token source on a failure path is \
deliberate: it avoids materializing a URL-embedded credential \
in logs, and resolution has already failed so there is nothing \
resolved to log"
)]
{
tracing::error!(
url_source = %url.as_source(),
error = %error,
"HTTP hook URL env resolution failed, not firing"
);
}
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()),
};
@ -584,27 +619,18 @@ impl HookExecutorImpl {
if let Some(hdrs) = headers {
for (key, value) in hdrs {
// Headers resolve through the per-hook env allowlist: a
// `{{ env.NAME }}` not in `allowed_env_vars` looks up as absent
// and blocks (fail-closed), exactly like an unset variable.
// `{{ 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) => {
// Log the unresolved URL source, not the resolved URL,
// so a URL-embedded credential cannot leak to logs.
#[expect(
clippy::disallowed_methods,
reason = "logging the unresolved URL token source on a failure \
path is deliberate: it avoids materializing a \
URL-embedded credential in logs"
)]
{
tracing::error!(
url_source = %url.as_source(),
header = %key,
error = %error,
"HTTP hook header env resolution failed, not firing"
);
}
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()),
};
@ -617,14 +643,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"
);
@ -634,7 +664,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;
}
};
@ -646,7 +680,11 @@ impl HookExecutorImpl {
match serde_json::from_str::<HookDecision>(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
}
}
@ -1125,9 +1163,28 @@ mod tests {
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, "<unparseable url source>");
}
// Headers resolve `{{ env.NAME }}` tokens through the per-hook
// `allowed_env_vars` allowlist: an allowlisted name resolves, anything else
// looks up as absent and fails closed.
// fails closed before lookup.
#[test]
fn header_resolves_allowlisted_var() {
let env = test_env(&[("FABRO_TEST_KEY_1", "secret123")]);
@ -1141,8 +1198,8 @@ mod tests {
}
// Fail-closed: a header may not read an env var that is set in the process
// but missing from `allowed_env_vars`. It resolves as absent (a hard error),
// so the value never reaches the request.
// 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")]);
@ -1152,7 +1209,9 @@ mod tests {
&env,
)
.unwrap_err();
assert_eq!(err.name, "FABRO_TEST_KEY_3");
assert_eq!(err, HeaderResolveError::NotAllowed {
name: "FABRO_TEST_KEY_3".to_string(),
});
}
#[test]
@ -1164,7 +1223,12 @@ mod tests {
&env,
)
.unwrap_err();
assert_eq!(err.name, "FABRO_TEST_KEY_3");
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

View file

@ -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]
@ -1695,7 +1687,7 @@ impl HookDefinition {
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 source = command.as_source();
@ -1805,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,
@ -1826,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,

View file

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