mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-08 03:10:26 +00:00
Wire up missing hook invocations (#19)
This PR wires up three `HookEvent` variants—`StageRetrying`,
`ParallelStart`, and `ParallelComplete`—that were defined in the enum
and documented but never actually invoked by the engine. `StageRetrying`
hooks now fire at both retry sites in `execute_with_retry` (error-retry
and explicit-Retry-status paths) immediately before the backoff sleep.
`ParallelStart` and `ParallelComplete` hooks fire in the parallel
handler after their corresponding event emissions, using a new
`EngineServices::run_hooks()` convenience method since the handler
doesn't have direct access to the engine's hook method.
The two remaining unwired events, `SandboxReady` and `SandboxCleanup`,
are marked as reserved with doc comments on the enum variants and
annotated in the docs table, since wiring them requires sandbox
lifecycle changes outside the engine's scope. A small
`HookContext::set_node()` helper is introduced to reduce repeated field
assignment across all hook call sites, and existing
`StageStart`/`StageComplete` hook calls are refactored to use it.
### Fabro Details
<details>
<summary>Ran 10 stages in 25m 6s for $5.60</summary>
| Stage | Duration | Cost | Retries |
|---|---|---|---|
| start | 0s | – | 0 |
| toolchain | 0s | – | 0 |
| preflight_compile | 0s | – | 0 |
| preflight_lint | 0s | – | 0 |
| implement | 0s | $1.04 | 0 |
| simplify_opus | 0s | $1.51 | 0 |
| simplify_gemini | 0s | $1.44 | 0 |
| simplify_gpt | 0s | $1.62 | 0 |
| verify | 0s | – | 0 |
| fmt | 0s | – | 0 |
| **Total** | **25m 6s** | **$5.60** | **0** |
</details>
<details>
<summary>Ran <code>ImplementAndSimplify.fabro</code> (13 nodes and 16
edges)</summary>
```dot
digraph ImplementAndSimplify {
graph [
goal="Implement and simplify",
model_stylesheet="
* { backend: api; model: claude-opus-4-6;}
"
]
rankdir=LR
start [shape=Mdiamond, label="Start"]
exit [shape=Msquare, label="Exit"]
toolchain [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0]
preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check -q --workspace 2>&1", max_retries=0]
preflight_lint [label="Preflight Lint", shape=parallelogram, script="cargo clippy -q --workspace -- -D warnings 2>&1", max_retries=0]
fix_lints [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3]
implement [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD."]
simplify_opus [label="Simplify (Opus)", prompt="@prompts/simplify.md"]
simplify_gemini [label="Simplify (Gemini)", prompt="@prompts/simplify.md", model="gemini-3.1-pro-preview-customtools"]
simplify_gpt [label="Simplify (GPT-54)", prompt="@prompts/simplify.md", model="gpt-54"]
verify [label="Verify", shape=parallelogram, script="cargo clippy -q --workspace -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1", goal_gate=true, retry_target="fixup"]
fixup [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures.", max_visits=3]
fmt [label="Format", shape=parallelogram, script="cargo fmt --all 2>&1", goal_gate=true, max_retries=0]
start -> toolchain
toolchain -> preflight_compile [condition="outcome=success"]
toolchain -> exit
preflight_compile -> preflight_lint [condition="outcome=success"]
preflight_compile -> exit
preflight_lint -> implement [condition="outcome=success"]
preflight_lint -> fix_lints
fix_lints -> preflight_lint
implement -> simplify_opus -> simplify_gemini -> simplify_gpt -> verify
verify -> fmt [condition="outcome=success"]
verify -> fixup
fixup -> verify
fmt -> exit
}
```
</details>
⚒️ Generated with [Fabro](https://fabro.sh)
---------
Co-authored-by: Fabro <noreply@fabro.sh>
This commit is contained in:
parent
50c032576d
commit
d29c1d817e
5 changed files with 67 additions and 12 deletions
|
|
@ -94,8 +94,8 @@ Each hook fires on a specific lifecycle event:
|
|||
| `edge_selected` | After an edge is chosen for traversal | Yes |
|
||||
| `parallel_start` | Before parallel branches fan out | No |
|
||||
| `parallel_complete` | After parallel branches merge | No |
|
||||
| `sandbox_ready` | After the sandbox environment is created | No |
|
||||
| `sandbox_cleanup` | Before the sandbox is torn down | No |
|
||||
| `sandbox_ready` | After the sandbox environment is created (reserved — not yet wired) | No |
|
||||
| `sandbox_cleanup` | Before the sandbox is torn down (reserved — not yet wired) | No |
|
||||
| `checkpoint_saved` | After a checkpoint is written to disk | No |
|
||||
| `pre_tool_use` | Before an agent tool call executes | Yes |
|
||||
| `post_tool_use` | After an agent tool call succeeds | No |
|
||||
|
|
|
|||
|
|
@ -980,6 +980,26 @@ impl WorkflowRunEngine {
|
|||
let _ = self.run_hooks(&hook_ctx, work_dir).await;
|
||||
}
|
||||
|
||||
/// Fire a non-blocking StageRetrying hook.
|
||||
async fn stage_retrying_hook(
|
||||
&self,
|
||||
node: &Node,
|
||||
context: &Context,
|
||||
graph: &Graph,
|
||||
attempt: u32,
|
||||
policy: &RetryPolicy,
|
||||
) {
|
||||
let mut hook_ctx = HookContext::new(
|
||||
HookEvent::StageRetrying,
|
||||
context.run_id(),
|
||||
graph.name.clone(),
|
||||
);
|
||||
hook_ctx.set_node(node);
|
||||
hook_ctx.attempt = Some(usize::try_from(attempt).unwrap_or(usize::MAX));
|
||||
hook_ctx.max_attempts = Some(usize::try_from(policy.max_attempts).unwrap_or(usize::MAX));
|
||||
let _ = self.run_hooks(&hook_ctx, None).await;
|
||||
}
|
||||
|
||||
/// Mirror graph-level attributes into the context.
|
||||
fn mirror_graph_attributes(graph: &Graph, context: &Context) {
|
||||
if !graph.goal().is_empty() {
|
||||
|
|
@ -1129,6 +1149,8 @@ impl WorkflowRunEngine {
|
|||
.unwrap_or(usize::MAX),
|
||||
delay_ms: millis_u64(delay),
|
||||
});
|
||||
self.stage_retrying_hook(node, context, graph, attempt, policy)
|
||||
.await;
|
||||
tokio::time::sleep(delay).await;
|
||||
continue;
|
||||
}
|
||||
|
|
@ -1157,6 +1179,8 @@ impl WorkflowRunEngine {
|
|||
.unwrap_or(usize::MAX),
|
||||
delay_ms: millis_u64(delay),
|
||||
});
|
||||
self.stage_retrying_hook(node, context, graph, attempt, policy)
|
||||
.await;
|
||||
tokio::time::sleep(delay).await;
|
||||
continue;
|
||||
}
|
||||
|
|
@ -1633,9 +1657,7 @@ impl WorkflowRunEngine {
|
|||
let mut hook_ctx =
|
||||
HookContext::new(HookEvent::StageStart, run_id.clone(), graph.name.clone());
|
||||
hook_ctx.cwd = hook_work_dir.as_ref().map(|p| p.display().to_string());
|
||||
hook_ctx.node_id = Some(node.id.clone());
|
||||
hook_ctx.node_label = Some(node.label().to_string());
|
||||
hook_ctx.handler_type = node.handler_type().map(String::from);
|
||||
hook_ctx.set_node(node);
|
||||
hook_ctx.attempt = Some(1);
|
||||
hook_ctx.max_attempts =
|
||||
Some(usize::try_from(retry_policy.max_attempts).unwrap_or(usize::MAX));
|
||||
|
|
@ -1771,9 +1793,7 @@ impl WorkflowRunEngine {
|
|||
run_id.clone(),
|
||||
graph.name.clone(),
|
||||
);
|
||||
hook_ctx.node_id = Some(node.id.clone());
|
||||
hook_ctx.node_label = Some(node.label().to_string());
|
||||
hook_ctx.handler_type = node.handler_type().map(String::from);
|
||||
hook_ctx.set_node(node);
|
||||
hook_ctx.status = Some("fail".into());
|
||||
hook_ctx.failure_reason = outcome.failure_reason().map(String::from);
|
||||
let _ = self.run_hooks(&hook_ctx, hook_work_dir.as_deref()).await;
|
||||
|
|
@ -1805,9 +1825,7 @@ impl WorkflowRunEngine {
|
|||
run_id.clone(),
|
||||
graph.name.clone(),
|
||||
);
|
||||
hook_ctx.node_id = Some(node.id.clone());
|
||||
hook_ctx.node_label = Some(node.label().to_string());
|
||||
hook_ctx.handler_type = node.handler_type().map(String::from);
|
||||
hook_ctx.set_node(node);
|
||||
hook_ctx.status = Some(outcome.status.to_string());
|
||||
let _ = self.run_hooks(&hook_ctx, hook_work_dir.as_deref()).await;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@ use crate::engine::GitState;
|
|||
use crate::error::FabroError;
|
||||
use crate::event::EventEmitter;
|
||||
use crate::graph::{shape_to_handler_type, Graph, Node};
|
||||
use crate::hook::HookRunner;
|
||||
use crate::hook::{HookContext, HookDecision, HookRunner};
|
||||
use crate::interviewer::Interviewer;
|
||||
use crate::outcome::Outcome;
|
||||
|
||||
|
|
@ -53,6 +53,15 @@ impl EngineServices {
|
|||
*self.git_state.write().unwrap() = state;
|
||||
}
|
||||
|
||||
/// Run lifecycle hooks and return the merged decision.
|
||||
/// Returns `Proceed` if no hook runner is configured.
|
||||
pub async fn run_hooks(&self, hook_context: &HookContext) -> HookDecision {
|
||||
let Some(ref runner) = self.hook_runner else {
|
||||
return HookDecision::Proceed;
|
||||
};
|
||||
runner.run(hook_context, self.sandbox.clone(), None).await
|
||||
}
|
||||
|
||||
/// Test-only default: empty registry, no hooks, local sandbox at cwd.
|
||||
#[cfg(test)]
|
||||
pub fn test_default() -> Self {
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ use crate::context::Context;
|
|||
use crate::error::FabroError;
|
||||
use crate::event::WorkflowRunEvent;
|
||||
use crate::graph::{Graph, Node};
|
||||
use crate::hook::{HookContext, HookEvent};
|
||||
use crate::millis_u64;
|
||||
use crate::outcome::{Outcome, StageStatus};
|
||||
use fabro_agent::LocalSandbox;
|
||||
|
|
@ -299,6 +300,15 @@ impl Handler for ParallelHandler {
|
|||
join_policy: join_policy.to_string(),
|
||||
error_policy: error_policy.to_string(),
|
||||
});
|
||||
{
|
||||
let mut hook_ctx = HookContext::new(
|
||||
HookEvent::ParallelStart,
|
||||
context.run_id(),
|
||||
graph.name.clone(),
|
||||
);
|
||||
hook_ctx.set_node(node);
|
||||
let _ = services.run_hooks(&hook_ctx).await;
|
||||
}
|
||||
let max_parallel = node
|
||||
.attrs
|
||||
.get("max_parallel")
|
||||
|
|
@ -711,6 +721,15 @@ impl Handler for ParallelHandler {
|
|||
success_count,
|
||||
failure_count: fail_count,
|
||||
});
|
||||
{
|
||||
let mut hook_ctx = HookContext::new(
|
||||
HookEvent::ParallelComplete,
|
||||
context.run_id(),
|
||||
graph.name.clone(),
|
||||
);
|
||||
hook_ctx.set_node(node);
|
||||
let _ = services.run_hooks(&hook_ctx).await;
|
||||
}
|
||||
|
||||
// Evaluate join policy
|
||||
let status = match join_policy {
|
||||
|
|
|
|||
|
|
@ -14,7 +14,9 @@ pub enum HookEvent {
|
|||
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,
|
||||
|
|
@ -97,6 +99,13 @@ pub struct HookContext {
|
|||
}
|
||||
|
||||
impl HookContext {
|
||||
/// Populate node-related fields from a graph `Node`.
|
||||
pub fn set_node(&mut self, node: &crate::graph::Node) {
|
||||
self.node_id = Some(node.id.clone());
|
||||
self.node_label = Some(node.label().to_string());
|
||||
self.handler_type = node.handler_type().map(String::from);
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn new(event: HookEvent, run_id: String, workflow_name: String) -> Self {
|
||||
Self {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue