mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-10 03:30:59 +00:00
parent
003478dc96
commit
8ec5ff52a7
6 changed files with 572 additions and 10 deletions
File diff suppressed because one or more lines are too long
208
nodes/simplify_gemini/prompt.md
Normal file
208
nodes/simplify_gemini/prompt.md
Normal file
|
|
@ -0,0 +1,208 @@
|
|||
Goal: # Plan: Move sandbox lifecycle into the engine
|
||||
|
||||
## Context
|
||||
|
||||
`SandboxReady` and `SandboxCleanup` hook events are defined in `HookEvent` but never fired. They can't fire naturally because sandbox init/cleanup happens in `cli/run.rs` — outside the engine where all other hooks fire. The root cause is that `cli/run.rs` is a ~2000-line god function orchestrating everything: sandbox creation, init, setup commands, devcontainer lifecycle, engine construction, engine run, retro, PR creation, and cleanup. The engine receives an already-initialized sandbox and has no role in lifecycle management.
|
||||
|
||||
This refactor moves sandbox initialization and setup into the engine so `SandboxReady` fires naturally alongside `RunStart`. Cleanup stays callable from the CLI (the retro agent needs the sandbox alive after the graph completes) but fires through an engine method so `SandboxCleanup` also uses the hook infrastructure.
|
||||
|
||||
## Design
|
||||
|
||||
### Two new engine methods
|
||||
|
||||
**`run_with_lifecycle(graph, config, lifecycle, checkpoint?) -> Result<Outcome>`**
|
||||
1. `sandbox.initialize()`
|
||||
2. Fire `SandboxReady` hook (blocking — can abort run)
|
||||
3. Emit `SandboxInitialized` event (CLI listener writes `sandbox.json`, updates progress UI)
|
||||
4. Remote git setup if `sandbox.is_remote()` — produces `base_sha`, `run_branch`, `base_branch`, merged into `config`
|
||||
5. Run setup commands inside sandbox
|
||||
6. Run devcontainer lifecycle phases inside sandbox
|
||||
7. Call existing `run_internal()` (unchanged — fires RunStart → graph → RunComplete)
|
||||
8. Return outcome (sandbox still alive)
|
||||
|
||||
**`cleanup_sandbox(run_id, workflow_name, preserve) -> Result<()>`**
|
||||
1. Fire `SandboxCleanup` hook (non-blocking)
|
||||
2. If `!preserve`: call `sandbox.cleanup()`
|
||||
|
||||
Existing `run()` is unchanged — API server and integration tests keep using it with pre-initialized sandboxes.
|
||||
|
||||
### New config struct
|
||||
|
||||
```rust
|
||||
// engine.rs
|
||||
pub struct LifecycleConfig {
|
||||
pub setup_commands: Vec<String>,
|
||||
pub setup_command_timeout_ms: u64,
|
||||
pub devcontainer_phases: Vec<(String, Vec<fabro_devcontainer::Command>)>,
|
||||
}
|
||||
```
|
||||
|
||||
`run_with_lifecycle` takes `config: RunConfig` by value (currently `&RunConfig`) so it can fill in remote git values. `run_internal` continues to take `&RunConfig`.
|
||||
|
||||
### CLI flow after refactor
|
||||
|
||||
```
|
||||
sandbox = create_sandbox() // unchanged
|
||||
sandbox = ReadBeforeWriteSandbox::new(sandbox) // moved before init (delegate_sandbox! delegates initialize)
|
||||
engine = build_engine(sandbox, hook_runner, ...)
|
||||
outcome = engine.run_with_lifecycle(graph, config, lifecycle)
|
||||
// retro, conclusion, PR creation — sandbox still alive
|
||||
engine.cleanup_sandbox(run_id, workflow_name, preserve)
|
||||
```
|
||||
|
||||
Single scopeguard around the entire block that calls `engine.cleanup_sandbox()` on panic.
|
||||
|
||||
## Steps
|
||||
|
||||
### 1. `hook/types.rs` — Make `SandboxReady` blocking by default
|
||||
- Add `Self::SandboxReady` to `is_blocking_by_default()` match arm (line 29-32)
|
||||
- `SandboxCleanup` stays non-blocking (correct default)
|
||||
|
||||
### 2. `engine.rs` — Add `LifecycleConfig` struct and `run_with_lifecycle` method
|
||||
- Define `LifecycleConfig` (setup_commands, setup_command_timeout_ms, devcontainer_phases)
|
||||
- Add `pub async fn run_with_lifecycle(self, graph, config, lifecycle, checkpoint) -> Result<Outcome>` that:
|
||||
- Calls `self.services.sandbox.initialize()`
|
||||
- Fires `SandboxReady` hook via `self.run_hooks()`
|
||||
- Emits `WorkflowRunEvent::SandboxInitialized { working_directory }` via emitter
|
||||
- Calls remote git setup if `sandbox.is_remote()`, fills config.base_sha/run_branch/base_branch/git_checkpoint_enabled
|
||||
- Runs setup commands via `sandbox.exec_command()`, emitting Setup* events
|
||||
- Runs devcontainer lifecycle via `devcontainer_bridge::run_devcontainer_lifecycle()`
|
||||
- Calls `self.run_internal()` (or `run_from_checkpoint` path)
|
||||
- Returns outcome
|
||||
|
||||
### 3. `engine.rs` — Add `cleanup_sandbox` method
|
||||
- `pub async fn cleanup_sandbox(&self, run_id, workflow_name, preserve) -> Result<(), String>`
|
||||
- Fires `SandboxCleanup` hook
|
||||
- If `!preserve`: calls `self.services.sandbox.cleanup()`
|
||||
|
||||
### 4. `engine.rs` — Move `setup_remote_git` from `cli/run.rs`
|
||||
- Move the `setup_remote_git()` function (cli/run.rs line 1636-1679) into engine.rs
|
||||
- It only uses `sandbox.exec_command()` and `run_id` — no CLI dependencies
|
||||
|
||||
### 5. `event.rs` — Add `SandboxInitialized` event variant
|
||||
- Add `WorkflowRunEvent::SandboxInitialized { working_directory: String }` variant
|
||||
- This replaces the inline sandbox.json writing in cli/run.rs
|
||||
|
||||
### 6. `cli/run.rs` — Register event listener for sandbox.json
|
||||
- Before calling `run_with_lifecycle`, register a listener on the emitter for `SandboxInitialized`
|
||||
- Listener captures the pre-built `SandboxRecord` template (all provider-specific fields filled, `working_directory` empty)
|
||||
- On event: fill `working_directory` from event, call `record.save()`
|
||||
- Also update progress UI `set_working_directory` in the same listener
|
||||
|
||||
### 7. `cli/run.rs` — Refactor `run_command` to use new engine methods
|
||||
- Move `ReadBeforeWriteSandbox` wrapping to before engine construction (currently at line 966, after init — delegate_sandbox! macro delegates initialize so wrapping before init works)
|
||||
- Move `HookRunner` creation earlier (before engine construction) — currently line 1222, move to ~line 800
|
||||
- Remove: `sandbox.initialize()` (line 886), remote git setup (lines 982-996), setup commands (lines 1031-1072), devcontainer lifecycle (lines 1074-1091)
|
||||
- Build `LifecycleConfig` from `setup_commands` and `devcontainer_config`
|
||||
- Build `RunConfig` without remote git fields (leave base_sha/run_branch/base_branch as None for remote — engine fills them)
|
||||
- Call `engine.run_with_lifecycle()` instead of `engine.run()`
|
||||
- Replace cleanup section (lines 1587-1604) with `engine.cleanup_sandbox()`
|
||||
- Replace two scopeguards with one that calls `engine.cleanup_sandbox()` on panic
|
||||
- Remove `status_guard` for SandboxInitFailed — engine handles init errors
|
||||
|
||||
### 8. `cli/run.rs` — Refactor `run_from_branch` to use new engine methods
|
||||
- Use `run_with_lifecycle` with empty `LifecycleConfig` (no setup commands, no devcontainer)
|
||||
- Add `cleanup_sandbox()` call (currently no scopeguard — this is an improvement)
|
||||
- This gives the resume path hooks for free (currently has zero hooks)
|
||||
|
||||
### 9. `docs/agents/hooks.mdx` — Update docs
|
||||
- Remove any "reserved" annotations for `sandbox_ready` / `sandbox_cleanup`
|
||||
- Note that `sandbox_ready` is blocking by default
|
||||
|
||||
## Files to modify
|
||||
- `lib/crates/fabro-workflows/src/hook/types.rs` — SandboxReady blocking default
|
||||
- `lib/crates/fabro-workflows/src/engine.rs` — LifecycleConfig, run_with_lifecycle, cleanup_sandbox, setup_remote_git
|
||||
- `lib/crates/fabro-workflows/src/event.rs` — SandboxInitialized event variant
|
||||
- `lib/crates/fabro-workflows/src/cli/run.rs` — major simplification of run_command and run_from_branch
|
||||
- `docs/agents/hooks.mdx` — remove "reserved" annotations
|
||||
|
||||
## Files unchanged
|
||||
- `lib/crates/fabro-workflows/src/handler/mod.rs` — EngineServices unchanged
|
||||
- `lib/crates/fabro-workflows/src/hook/runner.rs` — handles any HookEvent generically
|
||||
- `lib/crates/fabro-agent/src/sandbox.rs` — Sandbox trait unchanged
|
||||
- `lib/crates/fabro-agent/src/read_before_write_sandbox.rs` — delegate_sandbox! already delegates initialize/cleanup
|
||||
- All sandbox implementations — unchanged
|
||||
|
||||
## Verification
|
||||
1. `cargo build --workspace` — compile check
|
||||
2. `cargo test --workspace` — all existing tests pass (existing `run()` path unchanged)
|
||||
3. `cargo clippy --workspace -- -D warnings` — no new warnings
|
||||
4. Manual test: `fabro run` with a workflow that has `sandbox_ready` and `sandbox_cleanup` hooks configured — verify hooks fire
|
||||
5. Manual test: `fabro run --sandbox daytona` — verify remote git setup still works through the engine
|
||||
6. Manual test: `fabro run` with `--preserve-sandbox` — verify cleanup is skipped but SandboxCleanup hook still fires
|
||||
|
||||
|
||||
## Completed stages
|
||||
- **toolchain**: success
|
||||
- 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`
|
||||
- Stdout:
|
||||
```
|
||||
cargo 1.94.0 (85eff7c80 2026-01-15)
|
||||
```
|
||||
- Stderr: (empty)
|
||||
- **preflight_compile**: success
|
||||
- Script: `cargo check -q --workspace 2>&1`
|
||||
- Stdout: (empty)
|
||||
- Stderr: (empty)
|
||||
- **preflight_lint**: success
|
||||
- Script: `cargo clippy -q --workspace -- -D warnings 2>&1`
|
||||
- Stdout: (empty)
|
||||
- Stderr: (empty)
|
||||
- **implement**: success
|
||||
- Model: claude-opus-4-6, 172.3k tokens in / 48.5k out
|
||||
- Files: /home/daytona/workspace/docs/agents/hooks.mdx, /home/daytona/workspace/lib/crates/fabro-workflows/src/cli/run.rs, /home/daytona/workspace/lib/crates/fabro-workflows/src/engine.rs, /home/daytona/workspace/lib/crates/fabro-workflows/src/event.rs, /home/daytona/workspace/lib/crates/fabro-workflows/src/hook/types.rs
|
||||
- **simplify_opus**: success
|
||||
- Model: claude-opus-4-6, 90.5k tokens in / 16.9k out
|
||||
- Files: /home/daytona/workspace/lib/crates/fabro-workflows/src/cli/run.rs, /home/daytona/workspace/lib/crates/fabro-workflows/src/engine.rs
|
||||
|
||||
|
||||
# Simplify: Code Review and Cleanup
|
||||
|
||||
Review all changed files for reuse, quality, and efficiency. Fix any issues found.
|
||||
|
||||
## Phase 1: Identify Changes
|
||||
|
||||
Run git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.
|
||||
|
||||
## Phase 2: Launch Three Review Agents in Parallel
|
||||
|
||||
Use the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.
|
||||
|
||||
### Agent 1: Code Reuse Review
|
||||
|
||||
For each change:
|
||||
|
||||
1. Search for existing utilities and helpers that could replace newly written code. Use Grep to find similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.
|
||||
2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.
|
||||
3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.
|
||||
|
||||
Note: This is a greenfield app, so focus on maximizing simplicity and don't worry about changing things to achieve it.
|
||||
|
||||
### Agent 2: Code Quality Review
|
||||
|
||||
Review the same changes for hacky patterns:
|
||||
|
||||
1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls
|
||||
2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones
|
||||
3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction
|
||||
4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries
|
||||
5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase
|
||||
|
||||
Note: This is a greenfield app, so be aggressive in optimizing quality.
|
||||
|
||||
### Agent 3: Efficiency Review
|
||||
|
||||
Review the same changes for efficiency:
|
||||
|
||||
1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns
|
||||
2. Missed concurrency: independent operations run sequentially when they could run in parallel
|
||||
3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths
|
||||
4. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error
|
||||
5. Memory: unbounded data structures, missing cleanup, event listener leaks
|
||||
6. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one
|
||||
|
||||
## Phase 3: Fix Issues
|
||||
|
||||
Wait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.
|
||||
|
||||
When done, briefly summarize what was fixed (or confirm the code was already clean).
|
||||
5
nodes/simplify_gemini/provider_used.json
Normal file
5
nodes/simplify_gemini/provider_used.json
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
{
|
||||
"mode": "agent",
|
||||
"provider": "anthropic",
|
||||
"model": "claude-opus-4-6"
|
||||
}
|
||||
15
nodes/simplify_gemini/response.md
Normal file
15
nodes/simplify_gemini/response.md
Normal file
|
|
@ -0,0 +1,15 @@
|
|||
Everything passes. Here's a summary of what was fixed:
|
||||
|
||||
## Summary
|
||||
|
||||
### Fixed Issues
|
||||
|
||||
1. **Stringly-typed provider comparison** (`cli/run.rs`): Replaced `provider_name == "docker"` and `provider_name == "ssh"` string comparisons with `provider == SandboxProvider::Docker` and `provider == SandboxProvider::Ssh` enum comparisons. This eliminated the intermediate `sandbox_provider_name` match arm and the `provider_name: String` allocation, using the `Copy`-implementing `SandboxProvider` enum directly in the event listener closure.
|
||||
|
||||
2. **Test boilerplate** (`engine.rs`): Extracted `test_run_config(run_dir, run_id)` and `test_lifecycle(setup_commands)` helper functions that build default `RunConfig` and `LifecycleConfig` for tests. The 5 new lifecycle tests each had ~20 lines of identical config construction that now reduce to single-line calls. This removed ~95 lines of duplicated test code.
|
||||
|
||||
### Findings Not Fixed (by design)
|
||||
|
||||
- **`FinalizeConfig`**: While it copies fields from `RunConfig`, it serves a genuine purpose — the error path needs config values when `run_with_lifecycle` fails and doesn't return a `RunConfig`. Eliminating it would require restructuring the entire post-engine control flow, which would be a larger refactor with no functional benefit.
|
||||
- **`deferred_sandbox` pattern (`Arc<Mutex<Option<Arc<dyn Sandbox>>>>`**: This is inherently needed because event listeners are registered before the sandbox is created, but need access to it later. The sandbox can't be created earlier (it depends on provider config), and the listeners must be registered before the emitter is wrapped in `Arc`. The pattern is local to this scope and the mutex is only locked once (when the event fires).
|
||||
- **Existing 43 `RunConfig` constructions in tests**: The new helpers only apply to the 5 new tests. Backfilling all existing tests would be scope creep unrelated to this PR.
|
||||
6
nodes/simplify_gemini/status.json
Normal file
6
nodes/simplify_gemini/status.json
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"status": "success",
|
||||
"notes": "Stage completed: simplify_gemini",
|
||||
"failure_reason": null,
|
||||
"timestamp": "2026-03-16T04:50:45.014901+00:00"
|
||||
}
|
||||
299
nodes/simplify_opus/diff.patch
Normal file
299
nodes/simplify_opus/diff.patch
Normal file
|
|
@ -0,0 +1,299 @@
|
|||
diff --git a/docs/agents/hooks.mdx b/docs/agents/hooks.mdx
|
||||
index 9fdffd1..82a7a73 100644
|
||||
--- a/docs/agents/hooks.mdx
|
||||
+++ b/docs/agents/hooks.mdx
|
||||
@@ -399,4 +399,4 @@ event = "post_tool_use"
|
||||
command = "cargo fmt"
|
||||
matcher = "write_file|edit_file|apply_patch"
|
||||
blocking = true
|
||||
-```
|
||||
\ No newline at end of file
|
||||
+```
|
||||
diff --git a/lib/crates/fabro-workflows/src/cli/run.rs b/lib/crates/fabro-workflows/src/cli/run.rs
|
||||
index 157439c..4df34b9 100644
|
||||
--- a/lib/crates/fabro-workflows/src/cli/run.rs
|
||||
+++ b/lib/crates/fabro-workflows/src/cli/run.rs
|
||||
@@ -826,19 +826,14 @@ pub async fn run_command(
|
||||
.set_working_directory(working_directory.clone());
|
||||
|
||||
// Build sandbox record from template
|
||||
- let sandbox_info_opt = deferred_sb
|
||||
- .lock()
|
||||
- .unwrap()
|
||||
- .as_ref()
|
||||
- .map(|sb| {
|
||||
- let info = sb.sandbox_info();
|
||||
- if info.is_empty() {
|
||||
- None
|
||||
- } else {
|
||||
- Some(info)
|
||||
- }
|
||||
- })
|
||||
- .unwrap_or(None);
|
||||
+ let sandbox_info_opt = deferred_sb.lock().unwrap().as_ref().and_then(|sb| {
|
||||
+ let info = sb.sandbox_info();
|
||||
+ if info.is_empty() {
|
||||
+ None
|
||||
+ } else {
|
||||
+ Some(info)
|
||||
+ }
|
||||
+ });
|
||||
|
||||
let record = crate::sandbox_record::SandboxRecord {
|
||||
provider: provider_name.clone(),
|
||||
@@ -1172,7 +1167,7 @@ pub async fn run_command(
|
||||
|
||||
// Build lifecycle config for sandbox init, setup commands, and devcontainer phases
|
||||
let lifecycle = crate::engine::LifecycleConfig {
|
||||
- setup_commands: setup_commands.clone(),
|
||||
+ setup_commands,
|
||||
setup_command_timeout_ms: 300_000,
|
||||
devcontainer_phases: if let Some(ref dc) = devcontainer_config {
|
||||
vec![
|
||||
@@ -1188,20 +1183,6 @@ pub async fn run_command(
|
||||
// Defuse the status guard — engine.run() will write "running" and conclusion handles "concluded"
|
||||
scopeguard::ScopeGuard::into_inner(status_guard);
|
||||
|
||||
- // Save config fields needed after run_with_lifecycle consumes config
|
||||
- let finalize_config = FinalizeConfig {
|
||||
- run_id: config.run_id.clone(),
|
||||
- meta_branch: config.meta_branch.clone(),
|
||||
- host_repo_path: config.host_repo_path.clone(),
|
||||
- git_author: config.git_author.clone(),
|
||||
- github_app: config.github_app.clone(),
|
||||
- base_branch: config.base_branch.clone(),
|
||||
- run_branch: config.run_branch.clone(),
|
||||
- git_checkpoint_enabled: config.git_checkpoint_enabled,
|
||||
- pull_request_enabled: config.pull_request_enabled,
|
||||
- pull_request_draft: config.pull_request_draft,
|
||||
- };
|
||||
-
|
||||
// Safety net: if we panic or return early, best-effort cleanup via spawn.
|
||||
let sandbox_for_cleanup = Arc::clone(&sandbox);
|
||||
let cleanup_guard = scopeguard::guard((), move |()| {
|
||||
@@ -1232,15 +1213,24 @@ pub async fn run_command(
|
||||
// Restore cwd (worktree is kept for `fabro cp` access; pruned separately)
|
||||
let _ = std::env::set_current_dir(&original_cwd);
|
||||
|
||||
+ // Build FinalizeConfig from the (potentially mutated) config returned by the engine.
|
||||
+ // For remote sandboxes, run_with_lifecycle fills run_branch, base_sha, base_branch, etc.
|
||||
+ let finalize_config = match &engine_result {
|
||||
+ Ok((_, ref config)) => FinalizeConfig::from_run_config(config),
|
||||
+ Err(_) => {
|
||||
+ FinalizeConfig::from_run_config_owned(run_id.clone(), None, Some(original_cwd.clone()))
|
||||
+ }
|
||||
+ };
|
||||
+
|
||||
{
|
||||
let (status, failure_reason) = match &engine_result {
|
||||
- Ok(o) => (o.status.clone(), o.failure_reason().map(String::from)),
|
||||
+ Ok((ref o, _)) => (o.status.clone(), o.failure_reason().map(String::from)),
|
||||
Err(e) => (crate::outcome::StageStatus::Fail, Some(e.to_string())),
|
||||
};
|
||||
|
||||
// Map engine result to RunStatus + StatusReason
|
||||
let (run_status, status_reason) = match &engine_result {
|
||||
- Ok(o) => match o.status {
|
||||
+ Ok((ref o, _)) => match o.status {
|
||||
StageStatus::Success | StageStatus::Skipped => (
|
||||
crate::run_status::RunStatus::Succeeded,
|
||||
Some(crate::run_status::StatusReason::Completed),
|
||||
@@ -1318,7 +1308,7 @@ pub async fn run_command(
|
||||
// Auto-derive retro (always, cheap) and optionally run retro agent
|
||||
if !args.no_retro && super::project_config::is_retro_enabled() {
|
||||
let (failed, failure_reason) = match &engine_result {
|
||||
- Ok(o) => (
|
||||
+ Ok((ref o, _)) => (
|
||||
o.status == StageStatus::Fail,
|
||||
o.failure_reason().map(String::from),
|
||||
),
|
||||
@@ -1358,7 +1348,7 @@ pub async fn run_command(
|
||||
debug!("Skipping PR creation: dry-run mode");
|
||||
} else if let Err(ref e) = engine_result {
|
||||
debug!(error = %e, "Skipping PR creation: engine returned an error");
|
||||
- } else if let Ok(ref outcome) = engine_result {
|
||||
+ } else if let Ok((ref outcome, _)) = engine_result {
|
||||
if !matches!(
|
||||
outcome.status,
|
||||
StageStatus::Success | StageStatus::PartialSuccess
|
||||
@@ -1424,7 +1414,7 @@ pub async fn run_command(
|
||||
}
|
||||
}
|
||||
|
||||
- let outcome = engine_result?;
|
||||
+ let (outcome, _) = engine_result?;
|
||||
|
||||
// 8. Print result
|
||||
eprintln!("\n{}", styles.bold.apply_to("=== Run Result ==="),);
|
||||
@@ -1807,20 +1797,6 @@ async fn run_from_branch(
|
||||
devcontainer_phases: Vec::new(),
|
||||
};
|
||||
|
||||
- // Save config fields needed after run_with_lifecycle consumes config
|
||||
- let finalize_config = FinalizeConfig {
|
||||
- run_id: config.run_id.clone(),
|
||||
- meta_branch: config.meta_branch.clone(),
|
||||
- host_repo_path: config.host_repo_path.clone(),
|
||||
- git_author: config.git_author.clone(),
|
||||
- github_app: config.github_app.clone(),
|
||||
- base_branch: config.base_branch.clone(),
|
||||
- run_branch: config.run_branch.clone(),
|
||||
- git_checkpoint_enabled: config.git_checkpoint_enabled,
|
||||
- pull_request_enabled: config.pull_request_enabled,
|
||||
- pull_request_draft: config.pull_request_draft,
|
||||
- };
|
||||
-
|
||||
let run_start = Instant::now();
|
||||
let engine_result = engine
|
||||
.run_with_lifecycle(&graph, config, lifecycle, Some(&checkpoint))
|
||||
@@ -1830,10 +1806,18 @@ async fn run_from_branch(
|
||||
// Restore cwd (worktree is kept for `fabro cp` access; pruned separately)
|
||||
let _ = std::env::set_current_dir(&original_cwd);
|
||||
|
||||
+ // Build FinalizeConfig from the (potentially mutated) config returned by the engine.
|
||||
+ let finalize_config = match &engine_result {
|
||||
+ Ok((_, ref config)) => FinalizeConfig::from_run_config(config),
|
||||
+ Err(_) => {
|
||||
+ FinalizeConfig::from_run_config_owned(run_id.clone(), None, Some(original_cwd.clone()))
|
||||
+ }
|
||||
+ };
|
||||
+
|
||||
// Auto-derive retro
|
||||
if !args.no_retro && super::project_config::is_retro_enabled() {
|
||||
let (failed, failure_reason) = match &engine_result {
|
||||
- Ok(o) => (
|
||||
+ Ok((ref o, _)) => (
|
||||
o.status == StageStatus::Fail,
|
||||
o.failure_reason().map(String::from),
|
||||
),
|
||||
@@ -1873,7 +1857,7 @@ async fn run_from_branch(
|
||||
.cleanup_sandbox(&finalize_config.run_id, &graph.name, false)
|
||||
.await;
|
||||
|
||||
- let outcome = engine_result?;
|
||||
+ let (outcome, _) = engine_result?;
|
||||
|
||||
eprintln!("\n{}", styles.bold.apply_to("=== Run Result ==="),);
|
||||
eprintln!("{}", styles.dim.apply_to(format!("Run: {run_id}")));
|
||||
@@ -2308,6 +2292,44 @@ struct FinalizeConfig {
|
||||
pull_request_draft: bool,
|
||||
}
|
||||
|
||||
+impl FinalizeConfig {
|
||||
+ /// Build from the (potentially mutated) `RunConfig` returned by the engine.
|
||||
+ fn from_run_config(config: &RunConfig) -> Self {
|
||||
+ Self {
|
||||
+ run_id: config.run_id.clone(),
|
||||
+ meta_branch: config.meta_branch.clone(),
|
||||
+ host_repo_path: config.host_repo_path.clone(),
|
||||
+ git_author: config.git_author.clone(),
|
||||
+ github_app: config.github_app.clone(),
|
||||
+ base_branch: config.base_branch.clone(),
|
||||
+ run_branch: config.run_branch.clone(),
|
||||
+ git_checkpoint_enabled: config.git_checkpoint_enabled,
|
||||
+ pull_request_enabled: config.pull_request_enabled,
|
||||
+ pull_request_draft: config.pull_request_draft,
|
||||
+ }
|
||||
+ }
|
||||
+
|
||||
+ /// Minimal fallback for error paths where the engine didn't return a config.
|
||||
+ fn from_run_config_owned(
|
||||
+ run_id: String,
|
||||
+ meta_branch: Option<String>,
|
||||
+ host_repo_path: Option<PathBuf>,
|
||||
+ ) -> Self {
|
||||
+ Self {
|
||||
+ run_id,
|
||||
+ meta_branch,
|
||||
+ host_repo_path,
|
||||
+ git_author: crate::git::GitAuthor::default(),
|
||||
+ github_app: None,
|
||||
+ base_branch: None,
|
||||
+ run_branch: None,
|
||||
+ git_checkpoint_enabled: false,
|
||||
+ pull_request_enabled: false,
|
||||
+ pull_request_draft: false,
|
||||
+ }
|
||||
+ }
|
||||
+}
|
||||
+
|
||||
/// Write a finalize commit to the shadow branch with retro.json and final node files.
|
||||
///
|
||||
/// This captures the last diff.patch (written after the final checkpoint) and retro.json.
|
||||
diff --git a/lib/crates/fabro-workflows/src/engine.rs b/lib/crates/fabro-workflows/src/engine.rs
|
||||
index bf6aa40..3dcefa2 100644
|
||||
--- a/lib/crates/fabro-workflows/src/engine.rs
|
||||
+++ b/lib/crates/fabro-workflows/src/engine.rs
|
||||
@@ -1265,13 +1265,17 @@ impl WorkflowRunEngine {
|
||||
///
|
||||
/// The sandbox is left alive after return so the caller can run retro, PR creation, etc.
|
||||
/// Call `cleanup_sandbox()` when done.
|
||||
+ ///
|
||||
+ /// Returns both the outcome and the (potentially mutated) config — remote git setup
|
||||
+ /// fills `run_branch`, `base_sha`, `base_branch`, `meta_branch`, and
|
||||
+ /// `git_checkpoint_enabled`, which callers need for PR creation and finalize commits.
|
||||
pub async fn run_with_lifecycle(
|
||||
&self,
|
||||
graph: &Graph,
|
||||
mut config: RunConfig,
|
||||
lifecycle: LifecycleConfig,
|
||||
checkpoint: Option<&Checkpoint>,
|
||||
- ) -> Result<Outcome> {
|
||||
+ ) -> Result<(Outcome, RunConfig)> {
|
||||
// 1. Initialize sandbox
|
||||
self.services
|
||||
.sandbox
|
||||
@@ -1385,7 +1389,7 @@ impl WorkflowRunEngine {
|
||||
}
|
||||
|
||||
// 7. Execute the workflow graph
|
||||
- if let Some(cp) = checkpoint {
|
||||
+ let outcome = if let Some(cp) = checkpoint {
|
||||
let loop_state = LoopState {
|
||||
node_visits: HashMap::new(),
|
||||
loop_failure_signatures: cp.loop_failure_signatures.clone(),
|
||||
@@ -1394,13 +1398,14 @@ impl WorkflowRunEngine {
|
||||
let (outcome, _context) = self
|
||||
.run_internal(graph, &config, Some(cp), None, None, loop_state)
|
||||
.await?;
|
||||
- Ok(outcome)
|
||||
+ outcome
|
||||
} else {
|
||||
let (outcome, _context) = self
|
||||
.run_internal(graph, &config, None, None, None, LoopState::default())
|
||||
.await?;
|
||||
- Ok(outcome)
|
||||
- }
|
||||
+ outcome
|
||||
+ };
|
||||
+ Ok((outcome, config))
|
||||
}
|
||||
|
||||
/// Fire the `SandboxCleanup` hook and optionally clean up the sandbox.
|
||||
@@ -5704,7 +5709,7 @@ mod tests {
|
||||
setup_command_timeout_ms: 300_000,
|
||||
devcontainer_phases: Vec::new(),
|
||||
};
|
||||
- let outcome = engine
|
||||
+ let (outcome, _) = engine
|
||||
.run_with_lifecycle(&g, config, lifecycle, None)
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -5759,7 +5764,7 @@ mod tests {
|
||||
setup_command_timeout_ms: 300_000,
|
||||
devcontainer_phases: Vec::new(),
|
||||
};
|
||||
- let outcome = engine
|
||||
+ let (outcome, _) = engine
|
||||
.run_with_lifecycle(&g, config, lifecycle, None)
|
||||
.await
|
||||
.unwrap();
|
||||
@@ -5810,7 +5815,7 @@ mod tests {
|
||||
};
|
||||
let result = engine.run_with_lifecycle(&g, config, lifecycle, None).await;
|
||||
assert!(result.is_err());
|
||||
- let err = result.unwrap_err().to_string();
|
||||
+ let err = result.err().unwrap().to_string();
|
||||
assert!(
|
||||
err.contains("Setup command failed"),
|
||||
"expected setup failure error, got: {err}"
|
||||
Loading…
Add table
Reference in a new issue