mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-07 08:27:12 +00:00
Add failing regression tests for 6 bugs in create/start/attach lifecycle
Unit tests (run_progress.rs, start.rs): - Bug 1: handle_json_line reads pre-rename field names (name/stage/branch/message) but real JSONL uses post-rename names (node_label/node_id/node_id/error) - Bug 5: start_run doesn't write Starting status before spawning engine - Bug 6: handle_json_line missing DevcontainerLifecycleStarted dispatch Integration tests (cli.rs): - Bug 2: _run_engine reads spec.workflow_path instead of cached graph.fabro - Bug 3: attach loop doesn't delete interview_request.json after handling - Bug 4: attach hardcodes verbose=false, ignoring spec.verbose Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp
parent
f9f0b439b2
commit
1490024823
3 changed files with 402 additions and 0 deletions
|
|
@ -1856,4 +1856,108 @@ mod tests {
|
|||
ui.handle_json_line("");
|
||||
ui.handle_json_line("{}"); // no event field
|
||||
}
|
||||
|
||||
// ── Bug regression tests (post-rename JSONL field names) ─────────
|
||||
|
||||
// Bug 1: handle_json_line reads pre-rename field names but real JSONL
|
||||
// uses post-rename names from rename_fields(). These tests use the
|
||||
// actual JSONL format produced by the engine.
|
||||
|
||||
#[test]
|
||||
fn bug1_stage_started_uses_node_label_not_name() {
|
||||
// Real JSONL: rename_fields renames "name" → "node_label"
|
||||
let mut ui = ProgressUI::new(true, false);
|
||||
let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"plan","node_label":"Plan","stage_index":0,"script":null,"attempt":1,"max_attempts":1}"#;
|
||||
ui.handle_json_line(started);
|
||||
|
||||
let stage = ui
|
||||
.active_stages
|
||||
.get("plan")
|
||||
.expect("stage should be tracked");
|
||||
assert_eq!(
|
||||
stage.display_name, "Plan",
|
||||
"display name should come from node_label field, not be '?'"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bug1_agent_tool_call_uses_node_id_not_stage() {
|
||||
// Real JSONL: rename_fields renames "stage" → "node_id" for Agent.* events
|
||||
let mut ui = ProgressUI::new(false, true);
|
||||
|
||||
// First create the stage
|
||||
let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"code","node_label":"Code","stage_index":0,"attempt":1,"max_attempts":1}"#;
|
||||
ui.handle_json_line(started);
|
||||
assert_eq!(ui.stage_counts.get("code").map(|c| c.1), Some(0));
|
||||
|
||||
// Tool call with post-rename field: "node_id" instead of "stage"
|
||||
let tc_start = r#"{"ts":"2026-01-01T12:00:01Z","event":"Agent.ToolCallStarted","node_id":"code","node_label":"code","tool_name":"read_file","tool_call_id":"tc1","arguments":{"path":"src/main.rs"}}"#;
|
||||
ui.handle_json_line(tc_start);
|
||||
|
||||
assert_eq!(
|
||||
ui.stage_counts.get("code").map(|c| c.1),
|
||||
Some(1),
|
||||
"tool call count should increment using node_id field"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bug1_agent_assistant_message_uses_node_id_not_stage() {
|
||||
// Real JSONL: rename_fields renames "stage" → "node_id"
|
||||
let mut ui = ProgressUI::new(false, true);
|
||||
|
||||
let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"code","node_label":"Code","stage_index":0,"attempt":1,"max_attempts":1}"#;
|
||||
ui.handle_json_line(started);
|
||||
assert_eq!(ui.stage_counts.get("code").map(|c| c.0), Some(0));
|
||||
|
||||
// AssistantMessage with post-rename field: "node_id" instead of "stage"
|
||||
let msg = r#"{"ts":"2026-01-01T12:00:01Z","event":"Agent.AssistantMessage","node_id":"code","node_label":"code","model":"claude-sonnet-4-20250514"}"#;
|
||||
ui.handle_json_line(msg);
|
||||
|
||||
assert_eq!(
|
||||
ui.stage_counts.get("code").map(|c| c.0),
|
||||
Some(1),
|
||||
"turn count should increment using node_id field"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bug1_parallel_branch_uses_node_id_not_branch() {
|
||||
// Real JSONL: rename_fields renames "branch" → "node_id"
|
||||
let mut ui = ProgressUI::new(true, false);
|
||||
|
||||
// Set up a parent stage and start parallel
|
||||
let parent = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"fork","node_label":"Fork","stage_index":0,"attempt":1,"max_attempts":1}"#;
|
||||
ui.handle_json_line(parent);
|
||||
let par = r#"{"ts":"2026-01-01T12:00:01Z","event":"ParallelStarted","branch_count":2,"join_policy":"wait_all","error_policy":"continue"}"#;
|
||||
ui.handle_json_line(par);
|
||||
assert!(ui.parallel_parent.is_some());
|
||||
|
||||
// ParallelBranchStarted with post-rename field: "node_id" instead of "branch"
|
||||
let branch = r#"{"ts":"2026-01-01T12:00:02Z","event":"ParallelBranchStarted","node_id":"lint","node_label":"lint","branch_index":0}"#;
|
||||
ui.handle_json_line(branch);
|
||||
|
||||
// Branch should have been registered as a tool_call entry on the parent
|
||||
let parent_stage = ui.active_stages.get("fork").unwrap();
|
||||
assert!(
|
||||
!parent_stage.tool_calls.is_empty(),
|
||||
"parallel branch should be registered using node_id field"
|
||||
);
|
||||
}
|
||||
|
||||
// Bug 5: start_run should write Starting status before spawning engine
|
||||
// (tested in start.rs)
|
||||
|
||||
// Bug 6: handle_json_line is missing devcontainer event dispatch
|
||||
|
||||
#[test]
|
||||
fn bug6_devcontainer_lifecycle_started_dispatched() {
|
||||
let mut ui = ProgressUI::new(false, false);
|
||||
let event = r#"{"ts":"2026-01-01T12:00:00Z","event":"DevcontainerLifecycleStarted","phase":"postCreate","command_count":2}"#;
|
||||
ui.handle_json_line(event);
|
||||
assert_eq!(
|
||||
ui.devcontainer_command_count, 2,
|
||||
"devcontainer_command_count should be set by DevcontainerLifecycleStarted"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -53,3 +53,54 @@ pub fn start_run(run_dir: &Path) -> Result<u32> {
|
|||
|
||||
Ok(pid)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use fabro_workflows::run_spec::RunSpec;
|
||||
use fabro_workflows::run_status::{write_run_status, RunStatus, RunStatusRecord};
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
|
||||
fn sample_spec() -> RunSpec {
|
||||
RunSpec {
|
||||
run_id: "run-test123".to_string(),
|
||||
workflow_path: PathBuf::from("/tmp/test-workflow.toml"),
|
||||
dot_source: "digraph { a -> b }".to_string(),
|
||||
working_directory: PathBuf::from("/tmp"),
|
||||
goal: None,
|
||||
model: "claude-sonnet-4-20250514".to_string(),
|
||||
provider: Some("anthropic".to_string()),
|
||||
sandbox_provider: "local".to_string(),
|
||||
labels: HashMap::new(),
|
||||
verbose: false,
|
||||
no_retro: true,
|
||||
ssh: false,
|
||||
preserve_sandbox: false,
|
||||
dry_run: false,
|
||||
auto_approve: true,
|
||||
resume: None,
|
||||
run_branch: None,
|
||||
}
|
||||
}
|
||||
|
||||
// Bug 5: start_run should write Starting status before spawning engine
|
||||
// to prevent duplicate engine processes from concurrent start calls.
|
||||
#[test]
|
||||
fn bug5_start_run_writes_starting_status_before_spawn() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
write_run_status(dir.path(), RunStatus::Submitted, None);
|
||||
sample_spec().save(dir.path()).unwrap();
|
||||
|
||||
// start_run may fail on spawn (test binary != fabro), but we only
|
||||
// care about the status file being updated before the spawn attempt.
|
||||
let _ = start_run(dir.path());
|
||||
|
||||
let record = RunStatusRecord::load(&dir.path().join("status.json")).unwrap();
|
||||
assert_eq!(
|
||||
record.status,
|
||||
RunStatus::Starting,
|
||||
"start_run should write Starting status before spawning to prevent duplicate engines"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -527,3 +527,250 @@ fn detach_conflicts_with_resume() {
|
|||
.failure()
|
||||
.stderr(predicate::str::contains("cannot be used with"));
|
||||
}
|
||||
|
||||
// == Bug regression: create/start/attach lifecycle ============================
|
||||
|
||||
/// Helper: create a minimal run directory that `resolve_run` can find.
|
||||
/// Sets up manifest.json, status.json, spec.json, and progress.jsonl.
|
||||
fn setup_run_dir(
|
||||
home: &std::path::Path,
|
||||
run_id: &str,
|
||||
spec_overrides: serde_json::Value,
|
||||
progress_lines: &[&str],
|
||||
) -> std::path::PathBuf {
|
||||
let run_dir = home.join(".fabro").join("runs").join(run_id);
|
||||
std::fs::create_dir_all(&run_dir).unwrap();
|
||||
|
||||
// manifest.json for resolve_run
|
||||
let manifest = serde_json::json!({
|
||||
"run_id": run_id,
|
||||
"workflow_name": "test",
|
||||
"goal": "",
|
||||
"start_time": "2026-01-01T00:00:00Z",
|
||||
"node_count": 1,
|
||||
"edge_count": 0
|
||||
});
|
||||
std::fs::write(
|
||||
run_dir.join("manifest.json"),
|
||||
serde_json::to_string(&manifest).unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
// Merge spec defaults with overrides
|
||||
let mut spec = serde_json::json!({
|
||||
"run_id": run_id,
|
||||
"workflow_path": "/tmp/test.fabro",
|
||||
"dot_source": "digraph { start -> exit }",
|
||||
"working_directory": "/tmp",
|
||||
"goal": null,
|
||||
"model": "test-model",
|
||||
"provider": null,
|
||||
"sandbox_provider": "local",
|
||||
"labels": {},
|
||||
"verbose": false,
|
||||
"no_retro": true,
|
||||
"ssh": false,
|
||||
"preserve_sandbox": false,
|
||||
"dry_run": true,
|
||||
"auto_approve": true,
|
||||
"resume": null,
|
||||
"run_branch": null
|
||||
});
|
||||
if let (Some(base), Some(overrides)) = (spec.as_object_mut(), spec_overrides.as_object()) {
|
||||
for (k, v) in overrides {
|
||||
base.insert(k.clone(), v.clone());
|
||||
}
|
||||
}
|
||||
std::fs::write(
|
||||
run_dir.join("spec.json"),
|
||||
serde_json::to_string(&spec).unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
// progress.jsonl
|
||||
std::fs::write(run_dir.join("progress.jsonl"), progress_lines.join("\n")).unwrap();
|
||||
|
||||
run_dir
|
||||
}
|
||||
|
||||
// Bug 2: _run_engine should use cached graph.fabro, not spec.workflow_path.
|
||||
// When the original workflow file is deleted between create and start,
|
||||
// the engine should read the snapshot saved at create time.
|
||||
#[test]
|
||||
fn bug2_run_engine_uses_cached_graph_not_original_path() {
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let run_dir = dir.path().join("run");
|
||||
std::fs::create_dir_all(&run_dir).unwrap();
|
||||
|
||||
let dot = "\
|
||||
digraph G {
|
||||
start [shape=Mdiamond, label=\"Start\"]
|
||||
exit [shape=Msquare, label=\"Exit\"]
|
||||
start -> exit
|
||||
}";
|
||||
|
||||
// spec.json: workflow_path points to a file that no longer exists
|
||||
let spec = serde_json::json!({
|
||||
"run_id": "test-bug2",
|
||||
"workflow_path": "/nonexistent/deleted-workflow.fabro",
|
||||
"dot_source": dot,
|
||||
"working_directory": run_dir.to_str().unwrap(),
|
||||
"goal": null,
|
||||
"model": "test-model",
|
||||
"provider": null,
|
||||
"sandbox_provider": "local",
|
||||
"labels": {},
|
||||
"verbose": false,
|
||||
"no_retro": true,
|
||||
"ssh": false,
|
||||
"preserve_sandbox": false,
|
||||
"dry_run": true,
|
||||
"auto_approve": true,
|
||||
"resume": null,
|
||||
"run_branch": null
|
||||
});
|
||||
std::fs::write(
|
||||
run_dir.join("spec.json"),
|
||||
serde_json::to_string(&spec).unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
// The cached graph snapshot saved by `fabro create`
|
||||
std::fs::write(run_dir.join("graph.fabro"), dot).unwrap();
|
||||
|
||||
// _run_engine should use graph.fabro and never reference the deleted file.
|
||||
// Bug: it reads spec.workflow_path → fails with file-not-found.
|
||||
let output = arc()
|
||||
.args(["_run_engine", "--run-dir", run_dir.to_str().unwrap()])
|
||||
.env("NO_COLOR", "1")
|
||||
.timeout(std::time::Duration::from_secs(15))
|
||||
.output()
|
||||
.expect("process should start");
|
||||
|
||||
let stderr = String::from_utf8(output.stderr).unwrap();
|
||||
assert!(
|
||||
!stderr.contains("deleted-workflow.fabro"),
|
||||
"bug2: engine should use cached graph.fabro, not the original \
|
||||
(deleted) workflow path.\nstderr: {stderr}"
|
||||
);
|
||||
}
|
||||
|
||||
// Bug 3: attach loop must delete interview_request.json after handling it
|
||||
// to prevent re-prompting the user on the next poll iteration.
|
||||
#[test]
|
||||
fn bug3_attach_cleans_up_interview_request_after_handling() {
|
||||
let home = tempfile::tempdir().unwrap();
|
||||
|
||||
let run_dir = setup_run_dir(
|
||||
home.path(),
|
||||
"bug3-test",
|
||||
serde_json::json!({}),
|
||||
&[
|
||||
r#"{"ts":"2026-01-01T00:00:01Z","run_id":"bug3","event":"StageStarted","node_id":"gate","name":"Gate","index":0,"attempt":1,"max_attempts":1}"#,
|
||||
],
|
||||
);
|
||||
|
||||
// Status: running
|
||||
std::fs::write(
|
||||
run_dir.join("status.json"),
|
||||
serde_json::json!({"status": "running", "updated_at": "2026-01-01T00:00:00Z"}).to_string(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
// interview_request.json — a question the engine wrote
|
||||
let question = serde_json::json!({
|
||||
"text": "Approve?",
|
||||
"question_type": "YesNo",
|
||||
"options": [],
|
||||
"allow_freeform": false,
|
||||
"default": {"value": "Yes", "selected_option": null, "selected_options": [], "text": null},
|
||||
"timeout_seconds": 1.0,
|
||||
"stage": "gate",
|
||||
"metadata": {}
|
||||
});
|
||||
std::fs::write(
|
||||
run_dir.join("interview_request.json"),
|
||||
serde_json::to_string(&question).unwrap(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
// Dead engine so attach exits after one iteration
|
||||
std::fs::write(run_dir.join("run.pid"), "99999999").unwrap();
|
||||
|
||||
// Pipe "y\n" so ConsoleInterviewer doesn't block on stdin
|
||||
let _ = arc()
|
||||
.env("HOME", home.path())
|
||||
.env("NO_COLOR", "1")
|
||||
.args(["attach", "bug3-test"])
|
||||
.write_stdin("y\n")
|
||||
.timeout(std::time::Duration::from_secs(5))
|
||||
.output();
|
||||
|
||||
// Bug: interview_request.json is never deleted by the attach loop.
|
||||
// After the fix it should be removed immediately after handling.
|
||||
assert!(
|
||||
!run_dir.join("interview_request.json").exists(),
|
||||
"bug3: interview_request.json should be deleted after being handled by attach"
|
||||
);
|
||||
}
|
||||
|
||||
// Bug 4: attach should respect the verbose flag from spec.json.
|
||||
// Currently ProgressUI is created with verbose=false regardless of spec.
|
||||
#[test]
|
||||
fn bug4_attach_respects_verbose_from_spec() {
|
||||
let home = tempfile::tempdir().unwrap();
|
||||
|
||||
// Use pre-rename field names so handle_json_line can parse them
|
||||
// (isolates this test from bug 1). With 2 turns and 1 tool call,
|
||||
// verbose mode should display "(2 turns, 1 tools, …)" in the output.
|
||||
let run_dir = setup_run_dir(
|
||||
home.path(),
|
||||
"bug4-test",
|
||||
serde_json::json!({"verbose": true}),
|
||||
&[
|
||||
r#"{"ts":"2026-01-01T12:00:00Z","run_id":"bug4","event":"StageStarted","node_id":"code","name":"Code","index":0,"attempt":1,"max_attempts":1}"#,
|
||||
r#"{"ts":"2026-01-01T12:00:01Z","run_id":"bug4","event":"Agent.AssistantMessage","stage":"code","model":"claude-sonnet"}"#,
|
||||
r#"{"ts":"2026-01-01T12:00:02Z","run_id":"bug4","event":"Agent.AssistantMessage","stage":"code","model":"claude-sonnet"}"#,
|
||||
r#"{"ts":"2026-01-01T12:00:03Z","run_id":"bug4","event":"Agent.ToolCallStarted","stage":"code","tool_name":"read_file","tool_call_id":"tc1","arguments":{}}"#,
|
||||
r#"{"ts":"2026-01-01T12:00:04Z","run_id":"bug4","event":"Agent.ToolCallCompleted","stage":"code","tool_name":"read_file","tool_call_id":"tc1","is_error":false}"#,
|
||||
r#"{"ts":"2026-01-01T12:00:10Z","run_id":"bug4","event":"StageCompleted","node_id":"code","name":"Code","index":0,"duration_ms":10000,"status":"success","usage":{"input_tokens":1000,"output_tokens":500}}"#,
|
||||
],
|
||||
);
|
||||
|
||||
// Succeeded status + conclusion so attach exits after reading events
|
||||
std::fs::write(
|
||||
run_dir.join("status.json"),
|
||||
serde_json::json!({"status": "succeeded", "updated_at": "2026-01-01T12:00:10Z"})
|
||||
.to_string(),
|
||||
)
|
||||
.unwrap();
|
||||
std::fs::write(
|
||||
run_dir.join("conclusion.json"),
|
||||
serde_json::json!({
|
||||
"timestamp": "2026-01-01T12:00:10Z",
|
||||
"status": "success",
|
||||
"duration_ms": 10000,
|
||||
"stages": [],
|
||||
"total_retries": 0
|
||||
})
|
||||
.to_string(),
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let output = arc()
|
||||
.env("HOME", home.path())
|
||||
.env("NO_COLOR", "1")
|
||||
.args(["attach", "bug4-test"])
|
||||
.timeout(std::time::Duration::from_secs(10))
|
||||
.output()
|
||||
.expect("process should start");
|
||||
|
||||
let stderr = String::from_utf8(output.stderr).unwrap();
|
||||
|
||||
// Bug: verbose is hardcoded false, so stats are suppressed.
|
||||
// Fix: load spec.verbose and pass it to ProgressUI.
|
||||
assert!(
|
||||
stderr.contains("turns") && stderr.contains("tools"),
|
||||
"bug4: attach should show verbose stats when spec.verbose=true.\nstderr: {stderr}"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue