From 1490024823227d14d718b134dde152f0b8dae527 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 20 Mar 2026 12:28:19 -0400 Subject: [PATCH] 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) --- .../fabro-cli/src/commands/run_progress.rs | 104 ++++++++ lib/crates/fabro-cli/src/commands/start.rs | 51 ++++ lib/crates/fabro-cli/tests/cli.rs | 247 ++++++++++++++++++ 3 files changed, 402 insertions(+) diff --git a/lib/crates/fabro-cli/src/commands/run_progress.rs b/lib/crates/fabro-cli/src/commands/run_progress.rs index dc0bebea8..b78c44558 100644 --- a/lib/crates/fabro-cli/src/commands/run_progress.rs +++ b/lib/crates/fabro-cli/src/commands/run_progress.rs @@ -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" + ); + } } diff --git a/lib/crates/fabro-cli/src/commands/start.rs b/lib/crates/fabro-cli/src/commands/start.rs index 840d89198..e409b0be9 100644 --- a/lib/crates/fabro-cli/src/commands/start.rs +++ b/lib/crates/fabro-cli/src/commands/start.rs @@ -53,3 +53,54 @@ pub fn start_run(run_dir: &Path) -> Result { 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" + ); + } +} diff --git a/lib/crates/fabro-cli/tests/cli.rs b/lib/crates/fabro-cli/tests/cli.rs index 60fdd6abc..302c3a51d 100644 --- a/lib/crates/fabro-cli/tests/cli.rs +++ b/lib/crates/fabro-cli/tests/cli.rs @@ -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}" + ); +}