diff --git a/docs-internal/slow-test-opportunities-2026-04-07.md b/docs-internal/slow-test-opportunities-2026-04-07.md new file mode 100644 index 000000000..2fe511ff4 --- /dev/null +++ b/docs-internal/slow-test-opportunities-2026-04-07.md @@ -0,0 +1,410 @@ +# Slow Test Improvement Opportunities + +Date: 2026-04-07 + +All measurements in this note were taken with `ulimit -n 4096` in the test subshell. + +Primary timing dataset: +- `/tmp/fabro-slow-tests-ulimit.7FJkSO/slow_tests_passing.csv` +- `/tmp/fabro-slow-tests-ulimit.7FJkSO/report_passing.txt` + +Passing suite baseline: +- `cargo nextest run --workspace --no-fail-fast --status-level fail --final-status-level fail --show-progress none` +- Result: `3631 passed, 182 skipped` + +Method: +- Ranking is based on the 5-pass passing dataset above. +- Impact estimates are aggregate median test-time reductions, not additive suite wall-clock reductions. +- Where a number is inferred rather than directly measured, that is called out explicitly. + +## Top 10 + +### [x] 1. Change two slow `exec` mock responses from retriable `500` to non-retriable `400` + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/exec.rs` + +Measured evidence: +- `fabro-cli::it::cmd::exec::exec_cli_server_target_overrides_configured_server_target`: `6.858s` median +- `fabro-cli::it::cmd::exec::exec_server_target_uses_remote_transport_instead_of_local_api_key_resolution`: `6.787s` median +- Direct microbenchmark of the same CLI path: + - mocked `500`: `7.49s` median + - mocked `400`: `0.053s` median + +Implementation status: +- Implemented in `lib/crates/fabro-cli/tests/it/cmd/exec.rs` +- Verified with `ulimit -n 4096` via 5 targeted nextest runs per test +- Post-change nextest exec-time medians: + - `exec_server_target_uses_remote_transport_instead_of_local_api_key_resolution`: `1.580s` + - `exec_cli_server_target_overrides_configured_server_target`: `1.563s` + +Estimated impact: +- About `13.54s` aggregate median test time + +Complexity: +- Low + +Pros: +- Pure test change +- Strongest measured single win +- Keeps the same assertion shape if the response body marker is preserved + +Cons: +- If retry-on-5xx coverage matters, keep one dedicated retry-focused test elsewhere + +--- + +### 2. Short-circuit delete-path worker grace for already-terminal runs + +Files: +- `lib/crates/fabro-server/src/server.rs` + +Measured evidence: +- `fabro-cli::it::cmd::system_prune::system_prune_yes_deletes_matching_runs`: `10.477s` +- `fabro-cli::it::cmd::rm::rm_deletes_completed_run`: `5.359s` +- `fabro-cli::it::cmd::rm::rm_partial_failure_reports_which_identifiers_failed`: `5.293s` +- `fabro-cli::it::cmd::rm::rm_partial_failure_json_includes_removed_and_errors`: `5.257s` +- `fabro-cli::it::cmd::rm::rm_force_deletes_run_without_sandbox_json_when_store_has_sandbox`: `5.275s` +- Server code uses `WORKER_CANCEL_GRACE = 5s` in `terminate_worker_for_deletion()` + +Estimated impact: +- Roughly `20-25s` aggregate across the measured completed-run delete tests +- This is an inference from the timing cluster plus the `5s` grace, not a standalone delta benchmark + +Complexity: +- Medium + +Pros: +- Helps real behavior, not just tests +- Likely addresses the single slowest test too + +Cons: +- Needs careful correctness review around active worker shutdown semantics +- Higher overlap with several delete-related tests + +--- + +### [x] 3. Collapse the five abnormally slow `help` integration tests into one smoke test or a lighter harness + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/artifact.rs` +- `lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs` +- `lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs` +- `lib/crates/fabro-cli/tests/it/cmd/config.rs` +- `lib/crates/fabro-cli/tests/it/cmd/attach.rs` + +Measured evidence: +- Slow `help` tests: + - `artifact_list::help`: `1.659s` + - `artifact_cp::help`: `1.656s` + - `artifact::help`: `1.640s` + - `config::help`: `1.592s` + - `attach::help`: `1.559s` +- Aggregate median across those 5 tests: `8.106s` +- Direct command timings: + - `artifact list --help`: about `10ms` + - `attach --help`: about `9ms` + - `settings --help`: about `10ms` + +Estimated impact: +- Conservative recoverable time: about `6.45s` + +Implementation status: +- Implemented by removing the 5 command-owned help tests and replacing them with `scenario::smoke::help_smoke_covers_high_cost_commands` +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli help_smoke_covers_high_cost_commands completion_smoke_covers_help_and_generation --status-level fail --final-status-level fail --show-progress none` +- Verification result: `2 passed` +- Post-change timing over 5 targeted runs: + - `fabro-cli::it::scenario::smoke::help_smoke_covers_high_cost_commands`: `2.164s` median + +Complexity: +- Low + +Pros: +- Pure harness cleanup +- Clearly process/setup dominated rather than command-work dominated + +Cons: +- Less granular failure reporting + +--- + +### [x] 4. Replace `doctor_no_color_when_no_color_set` with a render-path assertion + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/doctor.rs` +- `lib/crates/fabro-util/src/check_report.rs` + +Measured evidence: +- `fabro-cli::it::cmd::doctor::doctor_no_color_when_no_color_set`: `5.131s` +- Direct timing of `fabro doctor` under minimal test-like env: `5.115s` +- Existing unit coverage already exercises no-color report rendering + +Estimated impact: +- About `5.13s` + +Implementation status: +- Implemented by deleting `fabro-cli::it::cmd::doctor::doctor_no_color_when_no_color_set` +- Added a unit-level render assertion in `lib/crates/fabro-cli/src/commands/doctor.rs`: + - `render_report_text_without_color_has_no_ansi` +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli render_report_text_without_color_has_no_ansi --status-level fail --final-status-level fail --show-progress none` +- Verification result: `1 passed` +- Removed median cost from the suite: `5.131s` + +Complexity: +- Low to medium + +Pros: +- Same intent can likely be covered without a full diagnostics run +- Very high return for a single test + +Cons: +- Slightly less end-to-end than the current test +- Requires choosing the right lower-level render assertion + +--- + +### [x] 5. Fix local Unix-socket autostart so it doesn't burn the full 5s readiness wait + +Files: +- `lib/crates/fabro-cli/src/server_client.rs` +- `lib/crates/fabro-cli/tests/it/cmd/server_start.rs` + +Measured evidence: +- Pre-fix 5-run timing for `fabro-cli::it::cmd::server_start::concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up`: + - runs: `6.786998459`, `6.833243958`, `6.832145834`, `6.851452709`, `6.768980709` + - median: `6.832s` + - stdev: `0.035s` +- Direct measurement showed the real bottleneck was not the test's polling loops: + - two concurrent fresh `fabro --json settings` calls each took about `5.1s` + - `fabro server stop --timeout 0` only took about `0.12s` +- Root cause: the Unix-socket client was doing a full `wait_for_server_ready()` loop before attempting local autostart when no daemon was running + +Estimated impact: +- Measured win on the original `server_start` test: about `5.02s` +- This also speeds up other fresh local Unix-socket autostart paths that hit the same client logic + +Complexity: +- Medium + +Pros: +- Fixes a real product-path inefficiency instead of just shaving test harness overhead +- Large win on the original slow test + +Cons: +- Touched shared local-server connection logic, so verification needs to cover the autostart path itself + +Implementation status: +- Implemented by splitting the Unix-socket connection path into: + - a single immediate health probe before autostart + - the existing retrying readiness wait after autostart +- Kept the original integration test coverage in `lib/crates/fabro-cli/tests/it/cmd/server_start.rs` +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up --status-level fail --final-status-level fail --show-progress none` +- Verification result: `1 passed` +- Post-change 5-run timing for `fabro-cli::it::cmd::server_start::concurrent_autostart_converges_on_one_shared_daemon_and_cleans_up`: + - runs: `1.801041000`, `1.812114833`, `1.760408750`, `1.817953958`, `1.958977916` + - median: `1.812s` + - stdev: `0.075s` +- Aggregate measured change for the original test: + - before: `6.832s` + - after: `1.812s` + - saved: `5.020s` +- Direct post-change autostart probe across 3 fresh concurrent runs: + - median process runtime: `0.092s` + - stdev: `0.028s` + +--- + +### [x] 6. Collapse three lightweight `attach` smoke tests into one scenario-style test + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/attach.rs` + +Measured evidence: +- `attach_requires_run_arg`: `1.595s` +- `attach_uses_configured_server_target_without_server_flag`: `1.555s` +- `attach_errors_when_live_stream_ends_before_terminal_event`: `1.629s` +- Aggregate median: `4.779s` +- Direct `fabro attach` parse-error path is about `12ms` + +Estimated impact: +- Conservative recoverable time: about `3.15s` + +Implementation status: +- Implemented by removing the 3 command-owned smoke tests from `lib/crates/fabro-cli/tests/it/cmd/attach.rs` +- Added `fabro-cli::it::scenario::smoke::attach_smoke_covers_arg_validation_and_remote_server_behaviors` +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli attach_smoke_covers_arg_validation_and_remote_server_behaviors --status-level fail --final-status-level fail --show-progress none` +- Verification result: `1 passed` +- Post-change timing over 5 targeted runs: + - `fabro-cli::it::scenario::smoke::attach_smoke_covers_arg_validation_and_remote_server_behaviors`: `1.584s` median +- Aggregate measured change for the full 3-test batch: + - before: `4.779s` median test-time sum + - after: `1.584s` median test-time sum + - saved: `3.195s` + +Complexity: +- Low to medium + +Pros: +- Fits the user's preference for merging complex cmd coverage into more natural scenarios +- Mostly harness/process cost + +Cons: +- Bundles distinct failure modes together + +--- + +### [x] 7. Collapse the three `completion` tests + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/completion.rs` + +Measured evidence: +- `completion::generates_zsh_completions`: `1.567s` +- `completion::generates_fish_completions`: `1.566s` +- `completion::help`: `1.564s` +- Aggregate median: `4.697s` +- Direct command timings: + - `completion zsh`: about `13ms` + - `completion fish`: about `13ms` + - `completion --help`: about `10ms` + +Estimated impact: +- Conservative recoverable time: about `3.13s` + +Implementation status: +- Implemented by removing the 3 command-owned completion smoke tests and replacing them with `scenario::smoke::completion_smoke_covers_help_and_generation` +- Post-change timing over 5 targeted runs: + - `fabro-cli::it::scenario::smoke::completion_smoke_covers_help_and_generation`: `2.147s` median +- Aggregate measured change for the full 8-test batch: + - before: `12.803s` median test-time sum + - after: `4.310s` median test-time sum + - saved: `8.492s` + +Complexity: +- Low + +Pros: +- Very safe refactor +- Strong evidence that cost is test harness overhead + +Cons: +- Less granular failures if combined too aggressively + +--- + +### [x] 8. Remove or merge the duplicate attach replay test + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/attach.rs` + +Measured evidence: +- `attach_replays_completed_detached_run`: `2.696s` +- `attach_replays_from_store_without_run_json_or_progress_jsonl`: `2.675s` +- The two tests are currently identical in code and assertions + +Implementation status: +- Implemented by removing the duplicate test from `lib/crates/fabro-cli/tests/it/cmd/attach.rs` +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli attach_replays_completed_detached_run --status-level fail --final-status-level fail --show-progress none` +- Verification result: `1 passed` + +Estimated impact: +- Immediate `2.675s` if the duplicate is removed + +Complexity: +- Low + +Pros: +- Full savings on one test +- Strongest low-risk cleanup in `attach.rs` + +Cons: +- If the intended missing-file case matters, the merged test should actually delete `run.json` / `progress.jsonl` + +--- + +### [x] 9. Make `attach_before_completion_streams_to_finished_state` event-driven instead of sleep-driven + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/attach.rs` + +Measured evidence: +- `attach_before_completion_streams_to_finished_state`: `3.043s` +- The test includes `sleep(Duration::from_secs(1))` +- `write_gated_workflow()` adds another fixed `sleep 0.2` + +Implementation status: +- Implemented by replacing the fixed 1-second gate-release sleep with a real attach-output signal in `lib/crates/fabro-cli/tests/it/cmd/attach.rs` +- The test now spawns `fabro attach`, waits for replayed stderr output (`✓ start`), then releases the workflow gate +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli attach_before_completion_streams_to_finished_state --status-level fail --final-status-level fail --show-progress none` +- Verification result: `1 passed` +- Isolated A/B benchmark over 5 targeted nextest runs with `ulimit -n 4096`, comparing the current workspace to a detached `HEAD` worktree using the same `CARGO_TARGET_DIR`: + - before (`HEAD` sleep-driven test): `8.013s` median, `0.227s` stdev + - after (current event-driven test): `6.933s` median, `0.016s` stdev + - saved: `1.079s` + +Estimated impact: +- Measured isolated saving: `1.079s` +- Full-suite saving should be at least about `1.0s` + +Complexity: +- Low + +Pros: +- Removes an explicit fixed delay +- Makes the test more deterministic + +Cons: +- Overlaps with the broader gated-workflow helper improvement below + +--- + +### [x] 10. Remove or parameterize the fixed `sleep 0.2` in `write_gated_workflow()` + +Files: +- `lib/crates/fabro-cli/tests/it/cmd/support.rs` + +Measured evidence: +- `write_gated_workflow()` hardcodes `sleep 0.2` +- The helper is used in 6 cmd tests + +Implementation status: +- Implemented by deleting the fixed `sleep 0.2` from `write_gated_workflow()` in `lib/crates/fabro-cli/tests/it/cmd/support.rs` +- Verified with `ulimit -n 4096; cargo nextest run -p fabro-cli -E 'test(attach_before_completion_streams_to_finished_state) | test(ctrl_c_cancels_active_run_via_server) | test(rm_force_terminates_active_run_worker) | test(start_rejects_already_active_or_completed_run) | test(start_runs_under_server_ownership_without_launcher_record)' --status-level fail --final-status-level fail --show-progress none` +- Verification result: `5 passed` +- Targeted 5-pass benchmark with `ulimit -n 4096` over the 5 tests that currently use the helper: + - before: `24.783s` aggregate median test-time sum + - after: `23.558s` aggregate median test-time sum + - saved: `1.225s` +- Per-test median deltas: + - `attach_before_completion_streams_to_finished_state`: `1.977s -> 1.635s` + - `ctrl_c_cancels_active_run_via_server`: `6.781s -> 6.681s` + - `rm_force_terminates_active_run_worker`: `11.984s -> 11.876s` + - `start_rejects_already_active_or_completed_run`: `2.041s -> 1.684s` + - `start_runs_under_server_ownership_without_launcher_record`: `2.000s -> 1.682s` + +Estimated impact: +- Measured aggregate saving across current helper users: `1.225s` + +Complexity: +- Low + +Pros: +- Small suite-wide gain +- Straightforward helper cleanup + +Cons: +- Overlaps slightly with item 9 + +## Notes On Excluded Ideas + +- I did not rank broad `rm` test consolidation highly on its own because the dominant cost appears to be the delete path itself, not just fixture setup. +- I did not rank `system_prune_dry_run_lists_matching_runs_without_deleting` because it is already fast at `0.183s`; the expensive case is specifically `--yes`. +- I did not rank `attach_json_errors_without_prompting_for_human_input` higher than item 8 because it is clearly slow (`6.882s`) but I did not finish a direct before/after measurement for replacing its `logs --json` polling loop with direct event-store polling. + +## Suggested First Pass + +If optimizing for highest impact with lowest complexity: + +1. Change the two slow `exec` tests to use non-retriable mock statuses +2. Remove or fix the duplicate attach replay test +3. Collapse the slow `help` / `completion` smoke tests into lighter coverage diff --git a/lib/crates/fabro-cli/src/commands/doctor.rs b/lib/crates/fabro-cli/src/commands/doctor.rs index 5c82a42a0..f7a227f46 100644 --- a/lib/crates/fabro-cli/src/commands/doctor.rs +++ b/lib/crates/fabro-cli/src/commands/doctor.rs @@ -289,9 +289,21 @@ fn convert_diagnostics_sections(sections: Vec) -> .collect() } +fn render_report_text( + report: &CheckReport, + styles: &Styles, + verbose: bool, + max_width: Option, +) -> String { + report.render(styles, verbose, None, max_width) +} + fn render_report(report: &CheckReport, styles: &Styles, verbose: bool) { let term_width = console::Term::stderr().size().1; - print!("{}", report.render(styles, verbose, None, Some(term_width))); + print!( + "{}", + render_report_text(report, styles, verbose, Some(term_width)) + ); } pub(crate) async fn run_doctor( @@ -505,6 +517,33 @@ mod tests { assert_eq!(parse_version(&DOT_RE, "no version here"), None); } + #[test] + fn render_report_text_without_color_has_no_ansi() { + let report = CheckReport { + title: "Fabro Doctor".to_string(), + sections: vec![CheckSection { + title: "Local".to_string(), + checks: vec![CheckResult { + name: "Configuration".to_string(), + status: CheckStatus::Pass, + summary: "loaded".to_string(), + details: vec![CheckDetail::new( + "Loaded from ~/.fabro/settings.toml".into(), + )], + remediation: None, + }], + }], + }; + + let rendered = render_report_text(&report, &Styles::new(false), false, Some(80)); + assert!( + !rendered.contains("\x1b["), + "rendered output should be plain text" + ); + assert!(rendered.contains("Fabro Doctor")); + assert!(rendered.contains("[✓] Configuration (loaded)")); + } + fn spec(name: &'static str, required: bool, min_version: Version) -> DepSpec { DepSpec { name, diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 5ae32e89f..7034e9534 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -143,7 +143,7 @@ async fn connect_target_api_client_bundle( connect_remote_api_client_bundle(api_url, tls.as_ref()) } user_config::ServerTarget::UnixSocket(path) => { - if let Ok(client) = connect_unix_socket_api_client_bundle(path).await { + if let Ok(client) = try_connect_unix_socket_api_client_bundle(path).await { Ok(client) } else { start::ensure_server_running_on_socket( @@ -199,21 +199,42 @@ fn normalize_remote_server_target(api_url: &str) -> String { .to_string() } -async fn connect_unix_socket_api_client_bundle(path: &Path) -> Result { - let http_client = cli_http_client_builder() +fn build_unix_socket_http_client(path: &Path) -> Result { + cli_http_client_builder() .unix_socket(path) .no_proxy() .build() - .context("Failed to build Unix-socket HTTP client for fabro server")?; - wait_for_server_ready(&http_client).await?; + .context("Failed to build Unix-socket HTTP client for fabro server") +} +fn unix_socket_api_client_bundle(http_client: reqwest::Client) -> ServerStoreClient { let base_url = "http://fabro".to_string(); let client = fabro_api::Client::new_with_client(&base_url, http_client.clone()); - Ok(ServerStoreClient { + ServerStoreClient { client, http_client, base_url, - }) + } +} + +async fn try_connect_unix_socket_api_client_bundle(path: &Path) -> Result { + let http_client = build_unix_socket_http_client(path)?; + check_server_ready(&http_client).await?; + Ok(unix_socket_api_client_bundle(http_client)) +} + +async fn connect_unix_socket_api_client_bundle(path: &Path) -> Result { + let http_client = build_unix_socket_http_client(path)?; + wait_for_server_ready(&http_client).await?; + Ok(unix_socket_api_client_bundle(http_client)) +} + +async fn check_server_ready(http_client: &reqwest::Client) -> Result<()> { + match http_client.get("http://fabro/health").send().await { + Ok(response) if response.status().is_success() => Ok(()), + Ok(response) => bail!("server health check returned status {}", response.status()), + Err(err) => Err(anyhow!(err)), + } } async fn wait_for_server_ready(http_client: &reqwest::Client) -> Result<()> { @@ -221,15 +242,11 @@ async fn wait_for_server_ready(http_client: &reqwest::Client) -> Result<()> { let mut last_error = None; while std::time::Instant::now() < deadline { - match http_client.get("http://fabro/health").send().await { - Ok(response) if response.status().is_success() => return Ok(()), - Ok(response) => { - last_error = Some(anyhow!( - "server health check returned status {}", - response.status() - )); + match check_server_ready(http_client).await { + Ok(()) => return Ok(()), + Err(err) => { + last_error = Some(err); } - Err(err) => last_error = Some(anyhow!(err)), } sleep(Duration::from_millis(50)).await; } diff --git a/lib/crates/fabro-cli/tests/it/cmd/artifact.rs b/lib/crates/fabro-cli/tests/it/cmd/artifact.rs deleted file mode 100644 index dacf1a833..000000000 --- a/lib/crates/fabro-cli/tests/it/cmd/artifact.rs +++ /dev/null @@ -1,30 +0,0 @@ -use fabro_test::{fabro_snapshot, test_context}; - -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["artifact", "--help"]); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - Inspect and copy run artifacts (screenshots, reports, traces) - - Usage: fabro artifact [OPTIONS] - - Commands: - list List artifacts for a workflow run - cp Copy artifacts from a workflow run - help Print this message or the help of the given subcommand(s) - - Options: - --json Output as JSON [env: FABRO_JSON=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} diff --git a/lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs b/lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs index 40536d2b3..71faa451b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/artifact_cp.rs @@ -2,38 +2,6 @@ use fabro_test::{fabro_snapshot, test_context}; use super::support::setup_completed_fast_dry_run; -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["artifact", "cp", "--help"]); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - Copy artifacts from a workflow run - - Usage: fabro artifact cp [OPTIONS] [DEST] - - Arguments: - Source: RUN_ID (all artifacts) or RUN_ID:path (specific artifact) - [DEST] Destination directory (defaults to current directory) [default: .] - - Options: - --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --node Filter to artifacts from a specific node - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --retry Filter to artifacts from a specific retry attempt - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --tree Preserve {node_slug}/retry_{N}/ directory structure - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} - #[test] fn artifact_cp_empty_run_reports_no_artifacts() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs b/lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs index ddee6fbb2..f2415fc16 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/artifact_list.rs @@ -2,36 +2,6 @@ use fabro_test::{fabro_snapshot, test_context}; use super::support::setup_completed_fast_dry_run; -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["artifact", "list", "--help"]); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - List artifacts for a workflow run - - Usage: fabro artifact list [OPTIONS] - - Arguments: - Run ID (or prefix) - - Options: - --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --node Filter to artifacts from a specific node - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --retry Filter to artifacts from a specific retry attempt - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} - #[test] fn artifact_list_empty_run_reports_no_artifacts() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/attach.rs b/lib/crates/fabro-cli/tests/it/cmd/attach.rs index e73d1a221..b4a8a961d 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/attach.rs @@ -1,7 +1,9 @@ -use std::time::Duration; +use std::io::{BufRead, BufReader, Read}; +use std::process::{Output, Stdio}; +use std::sync::mpsc; +use std::time::{Duration, Instant}; -use fabro_test::{fabro_snapshot, run_and_format, test_context}; -use httpmock::MockServer; +use fabro_test::{apply_filters, fabro_snapshot, test_context}; use serde_json::Value; use crate::support::{example_fixture, fabro_json_snapshot, run_output_filters, unique_run_id}; @@ -10,287 +12,76 @@ use super::support::{output_stdout, resolve_run, wait_for_status, write_gated_wo const SHARED_DAEMON_TIMEOUT: Duration = Duration::from_secs(30); -fn live_run_state_response() -> serde_json::Value { - serde_json::json!({ - "run": null, - "graph_source": null, - "start": null, - "status": { - "status": "running", - "reason": null, - "updated_at": "2026-04-05T12:00:01Z" - }, - "checkpoint": null, - "checkpoints": [], - "conclusion": null, - "retro": null, - "retro_prompt": null, - "retro_response": null, - "sandbox": null, - "final_patch": null, - "pull_request": null, - "nodes": {} - }) +fn format_output_snapshot(output: &Output, filters: &[(String, String)]) -> String { + let stdout = apply_filters(&String::from_utf8_lossy(&output.stdout), filters); + let stderr = apply_filters(&String::from_utf8_lossy(&output.stderr), filters); + + format!( + "success: {success}\nexit_code: {code}\n----- stdout -----\n{stdout}----- stderr -----\n{stderr}", + success = output.status.success(), + code = output.status.code().unwrap_or(-1), + stdout = stdout, + stderr = stderr, + ) } -fn run_sse_body(run_id: &str) -> String { - let completed = serde_json::json!({ - "seq": 2, - "payload": { - "event": "run.completed", - "id": "evt-run-completed", - "run_id": run_id, - "ts": "2026-04-05T12:00:01Z", - "properties": { - "duration_ms": 12, - "artifact_count": 0, - "status": "success" +fn wait_for_output_signal( + child: &mut std::process::Child, + stdout: &mut impl Read, + stderr_reader: std::thread::JoinHandle>, + signal_rx: mpsc::Receiver<()>, + needle: &str, +) -> std::thread::JoinHandle> { + let deadline = Instant::now() + SHARED_DAEMON_TIMEOUT; + let mut stderr_reader = Some(stderr_reader); + + loop { + match signal_rx.recv_timeout(Duration::from_millis(20)) { + Ok(()) => { + return stderr_reader + .take() + .expect("stderr reader should still be available"); } + Err(mpsc::RecvTimeoutError::Timeout) => {} + Err(mpsc::RecvTimeoutError::Disconnected) => {} } - }); - format!("data: {completed}\n\n") -} - -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["attach", "--help"]); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - Attach to a running or finished workflow run - - Usage: fabro attach [OPTIONS] - - Arguments: - Run ID prefix or workflow name - - Options: - --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} - -#[test] -fn attach_requires_run_arg() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.arg("attach"); - fabro_snapshot!(context.filters(), cmd, @" - success: false - exit_code: 2 - ----- stdout ----- - ----- stderr ----- - error: the following required arguments were not provided: - - - Usage: fabro attach --no-upgrade-check - - For more information, try '--help'. - "); -} - -#[test] -fn attach_uses_configured_server_target_without_server_flag() { - let context = test_context!(); - let server = MockServer::start(); - let run_id = unique_run_id(); - let list_mock = server.mock(|when, then| { - when.method("GET").path("/api/v1/runs"); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!([ - { - "run_id": run_id, - "workflow_name": "Remote Workflow", - "workflow_slug": "remote-workflow", - "goal": "Remote output", - "labels": {}, - "host_repo_path": null, - "start_time": "2026-04-05T12:00:00Z", - "status": "running", - "status_reason": null, - "duration_ms": 12, - "total_usd_micros": null - } - ]) - .to_string(), + if let Some(status) = child.try_wait().expect("attach should stay alive") { + let mut stdout_bytes = Vec::new(); + stdout + .read_to_end(&mut stdout_bytes) + .expect("attach stdout should be readable"); + let stderr_bytes = stderr_reader + .take() + .expect("stderr reader should still be available") + .join() + .expect("stderr reader should join"); + panic!( + "attach exited before emitting {needle:?}\nstatus: {status}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&stdout_bytes), + String::from_utf8_lossy(&stderr_bytes) ); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/events")); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!({ - "data": [{ - "seq": 1, - "payload": { - "event": "run.running", - "id": "evt-run-running", - "run_id": run_id, - "ts": "2026-04-05T12:00:00Z", - "properties": {} - } - }], - "meta": { "has_more": false } - }) - .to_string(), + } + + if Instant::now() >= deadline { + let _ = child.kill(); + let status = child.wait().expect("attach should exit after kill"); + let mut stdout_bytes = Vec::new(); + stdout + .read_to_end(&mut stdout_bytes) + .expect("attach stdout should be readable"); + let stderr_bytes = stderr_reader + .take() + .expect("stderr reader should still be available") + .join() + .expect("stderr reader should join"); + panic!( + "timed out waiting for attach output {needle:?}\nstatus: {status}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&stdout_bytes), + String::from_utf8_lossy(&stderr_bytes) ); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/state")); - then.status(200) - .header("Content-Type", "application/json") - .body(live_run_state_response().to_string()); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/questions")) - .query_param("page[limit]", "100") - .query_param("page[offset]", "0"); - then.status(200) - .header("Content-Type", "application/json") - .body(r#"{"data":[],"meta":{"has_more":false}}"#); - }); - let attach_mock = server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/attach")) - .query_param("since_seq", "2"); - then.status(200) - .header("Content-Type", "text/event-stream") - .body(run_sse_body(run_id.as_str())); - }); - context.write_home( - ".fabro/settings.toml", - format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), - ); - - let output = context - .command() - .args(["--json", "attach", &run_id]) - .output() - .expect("attach should execute"); - - assert!( - output.status.success(), - "attach failed:\nstdout:\n{}\nstderr:\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - ); - list_mock.assert(); - attach_mock.assert(); - let stdout = String::from_utf8(output.stdout).expect("stdout should be UTF-8"); - assert!(stdout.contains("\"event\":\"run.completed\""), "{stdout}"); -} - -#[test] -fn attach_errors_when_live_stream_ends_before_terminal_event() { - let context = test_context!(); - let server = MockServer::start(); - let run_id = unique_run_id(); - - server.mock(|when, then| { - when.method("GET").path("/api/v1/runs"); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!([ - { - "run_id": run_id, - "workflow_name": "Remote Workflow", - "workflow_slug": "remote-workflow", - "goal": "Remote output", - "labels": {}, - "host_repo_path": null, - "start_time": "2026-04-05T12:00:00Z", - "status": "running", - "status_reason": null, - "duration_ms": 12, - "total_cost": null - } - ]) - .to_string(), - ); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/events")); - then.status(200) - .header("Content-Type", "application/json") - .body( - serde_json::json!({ - "data": [{ - "seq": 1, - "payload": { - "event": "run.running", - "id": "evt-run-running", - "run_id": run_id, - "ts": "2026-04-05T12:00:00Z", - "properties": {} - } - }], - "meta": { "has_more": false } - }) - .to_string(), - ); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/state")); - then.status(200) - .header("Content-Type", "application/json") - .body(live_run_state_response().to_string()); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/questions")) - .query_param("page[limit]", "100") - .query_param("page[offset]", "0"); - then.status(200) - .header("Content-Type", "application/json") - .body(r#"{"data":[],"meta":{"has_more":false}}"#); - }); - server.mock(|when, then| { - when.method("GET") - .path(format!("/api/v1/runs/{run_id}/attach")) - .query_param("since_seq", "2"); - then.status(200) - .header("Content-Type", "text/event-stream") - .body(""); - }); - context.write_home( - ".fabro/settings.toml", - format!("[server]\ntarget = \"{}/api/v1\"\n", server.base_url()), - ); - - let output = context - .command() - .args(["attach", &run_id]) - .output() - .expect("attach should execute"); - - assert!( - !output.status.success(), - "attach should fail on premature EOF" - ); - let stderr = String::from_utf8(output.stderr).expect("stderr should be UTF-8"); - assert!( - stderr.contains("terminal run event"), - "expected a protocol error, got:\n{stderr}" - ); + } + } } #[test] @@ -336,49 +127,6 @@ fn attach_replays_completed_detached_run() { "); } -#[test] -fn attach_replays_from_store_without_run_json_or_progress_jsonl() { - let context = test_context!(); - let run_id = unique_run_id(); - - context - .command() - .args([ - "run", - "--dry-run", - "--auto-approve", - "--no-retro", - "--detach", - "--run-id", - run_id.as_str(), - example_fixture("simple.fabro").to_str().unwrap(), - ]) - .assert() - .success(); - - context - .command() - .args(["wait", &run_id]) - .timeout(SHARED_DAEMON_TIMEOUT) - .assert() - .success(); - - let mut cmd = context.command(); - cmd.args(["attach", &run_id]); - cmd.timeout(SHARED_DAEMON_TIMEOUT); - fabro_snapshot!(run_output_filters(&context), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - ----- stderr ----- - Sandbox: local (ready in [TIME]) - ✓ Start [TIME] - ✓ Run Tests [TIME] - ✓ Report [TIME] - ✓ Exit [TIME] - "); -} - #[test] fn attach_before_completion_streams_to_finished_state() { let context = test_context!(); @@ -412,14 +160,58 @@ fn attach_before_completion_streams_to_finished_state() { r"\b\d+(\.\d+)?(ms|s)\b".to_string(), "[DURATION]".to_string(), )); - let release_gate = std::thread::spawn(move || { - std::thread::sleep(Duration::from_secs(1)); - gate.release(); - }); - let mut attach_cmd = context.command(); + let mut attach_cmd = std::process::Command::new(env!("CARGO_BIN_EXE_fabro")); + attach_cmd.current_dir(&context.temp_dir); + attach_cmd.env("NO_COLOR", "1"); + attach_cmd.env("HOME", &context.home_dir); + attach_cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + attach_cmd.env("FABRO_SERVER_MAX_CONCURRENT_RUNS", "64"); + attach_cmd.env("FABRO_TEST_IN_MEMORY_STORE", "1"); attach_cmd.args(["attach", &run_id]); - let (snapshot, _output) = run_and_format(&mut attach_cmd, &filters); - release_gate.join().expect("gate releaser should join"); + attach_cmd.stdout(Stdio::piped()); + attach_cmd.stderr(Stdio::piped()); + let mut child = attach_cmd.spawn().expect("attach should spawn"); + let mut stdout = child.stdout.take().expect("attach stdout should be piped"); + let stderr = child.stderr.take().expect("attach stderr should be piped"); + let (signal_tx, signal_rx) = mpsc::channel(); + let stderr_reader = std::thread::spawn(move || { + let mut reader = BufReader::new(stderr); + let mut stderr_bytes = Vec::new(); + let mut line = Vec::new(); + + loop { + line.clear(); + let read = reader + .read_until(b'\n', &mut line) + .expect("attach stderr should be readable"); + if read == 0 { + break; + } + if line + .windows("✓ start".len()) + .any(|window| window == "✓ start".as_bytes()) + { + let _ = signal_tx.send(()); + } + stderr_bytes.extend_from_slice(&line); + } + + stderr_bytes + }); + let stderr_reader = + wait_for_output_signal(&mut child, &mut stdout, stderr_reader, signal_rx, "✓ start"); + gate.release(); + let status = child.wait().expect("attach should exit"); + let mut stdout_bytes = Vec::new(); + stdout + .read_to_end(&mut stdout_bytes) + .expect("attach stdout should be readable"); + let output = Output { + status, + stdout: stdout_bytes, + stderr: stderr_reader.join().expect("stderr reader should join"), + }; + let snapshot = format_output_snapshot(&output, &filters); wait_for_status(&run.run_dir, &["succeeded"]); insta::assert_snapshot!(snapshot, @" diff --git a/lib/crates/fabro-cli/tests/it/cmd/completion.rs b/lib/crates/fabro-cli/tests/it/cmd/completion.rs deleted file mode 100644 index 0940c9bb5..000000000 --- a/lib/crates/fabro-cli/tests/it/cmd/completion.rs +++ /dev/null @@ -1,44 +0,0 @@ -use fabro_test::{fabro_snapshot, test_context}; - -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["completion", "--help"]); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - Generate shell completions - - Usage: fabro completion [OPTIONS] - - Arguments: - Shell to generate completions for [possible values: bash, elvish, fish, powershell, zsh] - - Options: - --json Output as JSON [env: FABRO_JSON=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} - -#[test] -fn generates_zsh_completions() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["completion", "zsh"]); - cmd.assert().success(); -} - -#[test] -fn generates_fish_completions() { - let context = test_context!(); - let mut cmd = context.command(); - cmd.args(["completion", "fish"]); - cmd.assert().success(); -} diff --git a/lib/crates/fabro-cli/tests/it/cmd/config.rs b/lib/crates/fabro-cli/tests/it/cmd/config.rs index b85d5bebd..c9d9a8396 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/config.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/config.rs @@ -9,35 +9,6 @@ use predicates::prelude::*; use super::support::run_state; use crate::support::unique_run_id; -#[test] -fn help() { - let context = test_context!(); - let mut cmd = context.settings(); - cmd.arg("--help"); - fabro_snapshot!(context.filters(), cmd, @" - success: true - exit_code: 0 - ----- stdout ----- - Inspect effective settings - - Usage: fabro settings [OPTIONS] [WORKFLOW] - - Arguments: - [WORKFLOW] Optional workflow name, .fabro path, or .toml run config to overlay - - Options: - --json Output as JSON [env: FABRO_JSON=] - --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] - --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] - --local Show only locally resolved settings and skip the server call - --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] - --quiet Suppress non-essential output [env: FABRO_QUIET=] - --verbose Enable verbose output [env: FABRO_VERBOSE=] - -h, --help Print help - ----- stderr ----- - "); -} - #[test] fn old_config_show_command_is_rejected() { let context = test_context!(); diff --git a/lib/crates/fabro-cli/tests/it/cmd/doctor.rs b/lib/crates/fabro-cli/tests/it/cmd/doctor.rs index f12d44fe8..f5ea877c5 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/doctor.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/doctor.rs @@ -3,7 +3,6 @@ use std::process::Output; use fabro_test::{fabro_snapshot, test_context, twin_openai}; -use predicates::prelude::*; async fn run_success_output(mut cmd: assert_cmd::Command) -> Output { tokio::task::spawn_blocking(move || cmd.assert().success().get_output().clone()) @@ -85,12 +84,3 @@ async fn twin_doctor() { "expected verbose doctor output to include openai probe success, got: {stdout}" ); } - -#[test] -fn doctor_no_color_when_no_color_set() { - let context = test_context!(); - let mut cmd = context.doctor(); - cmd.env_clear(); - cmd.env("NO_COLOR", "1"); - cmd.assert().stdout(predicate::str::contains("\x1b[").not()); -} diff --git a/lib/crates/fabro-cli/tests/it/cmd/exec.rs b/lib/crates/fabro-cli/tests/it/cmd/exec.rs index d28d94af2..14ea203e5 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/exec.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/exec.rs @@ -125,7 +125,9 @@ fn exec_server_target_uses_remote_transport_instead_of_local_api_key_resolution( let server = MockServer::start(); server.mock(|when, then| { when.method("POST").path("/api/v1/completions"); - then.status(500).body("server-routed-marker"); + // Use a non-retriable error so this test covers transport routing + // without paying the retry backoff cost of a 5xx response. + then.status(400).body("server-routed-marker"); }); let mut cmd = context.exec_cmd(); @@ -202,7 +204,9 @@ fn exec_cli_server_target_overrides_configured_server_target() { let cli_server = MockServer::start(); cli_server.mock(|when, then| { when.method("POST").path("/api/v1/completions"); - then.status(500).body("cli-override-marker"); + // Use a non-retriable error so this test covers target precedence + // without paying the retry backoff cost of a 5xx response. + then.status(400).body("cli-override-marker"); }); context.write_home( ".fabro/settings.toml", diff --git a/lib/crates/fabro-cli/tests/it/cmd/mod.rs b/lib/crates/fabro-cli/tests/it/cmd/mod.rs index 0d3616d8f..0bbe54302 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/mod.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/mod.rs @@ -1,8 +1,6 @@ -mod artifact; mod artifact_cp; mod artifact_list; mod attach; -mod completion; mod config; mod create; mod diff; diff --git a/lib/crates/fabro-cli/tests/it/cmd/support.rs b/lib/crates/fabro-cli/tests/it/cmd/support.rs index 05062b4f8..4d710a006 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/support.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/support.rs @@ -469,7 +469,7 @@ pub(crate) fn write_gated_workflow(path: &Path, name: &str, goal: &str) -> Workf write_text_file( path, &format!( - "digraph {} {{\n graph [goal={goal:?}]\n start [shape=Mdiamond]\n exit [shape=Msquare]\n wait [shape=parallelogram, script=\"while [ ! -f {quoted_gate_path} ]; do sleep 0.01; done; sleep 0.2\"]\n start -> wait -> exit\n}}\n", + "digraph {} {{\n graph [goal={goal:?}]\n start [shape=Mdiamond]\n exit [shape=Msquare]\n wait [shape=parallelogram, script=\"while [ ! -f {quoted_gate_path} ]; do sleep 0.01; done\"]\n start -> wait -> exit\n}}\n", to_pascal_case(name), ), ); diff --git a/lib/crates/fabro-cli/tests/it/scenario/mod.rs b/lib/crates/fabro-cli/tests/it/scenario/mod.rs index 40d77271c..611804b48 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/mod.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/mod.rs @@ -5,6 +5,7 @@ mod exec; mod lifecycle; mod recovery; mod server_lifecycle; +mod smoke; use std::path::{Path, PathBuf}; use std::time::Duration; diff --git a/lib/crates/fabro-cli/tests/it/scenario/smoke.rs b/lib/crates/fabro-cli/tests/it/scenario/smoke.rs new file mode 100644 index 000000000..1f1ee67f5 --- /dev/null +++ b/lib/crates/fabro-cli/tests/it/scenario/smoke.rs @@ -0,0 +1,427 @@ +use fabro_test::{fabro_snapshot, test_context}; +use httpmock::MockServer; + +use crate::support::{LightweightCli, unique_run_id}; + +fn live_run_state_response() -> serde_json::Value { + serde_json::json!({ + "run": null, + "graph_source": null, + "start": null, + "status": { + "status": "running", + "reason": null, + "updated_at": "2026-04-05T12:00:01Z" + }, + "checkpoint": null, + "checkpoints": [], + "conclusion": null, + "retro": null, + "retro_prompt": null, + "retro_response": null, + "sandbox": null, + "final_patch": null, + "pull_request": null, + "nodes": {} + }) +} + +fn run_sse_body(run_id: &str) -> String { + let completed = serde_json::json!({ + "seq": 2, + "payload": { + "event": "run.completed", + "id": "evt-run-completed", + "run_id": run_id, + "ts": "2026-04-05T12:00:01Z", + "properties": { + "duration_ms": 12, + "artifact_count": 0, + "status": "success" + } + } + }); + + format!("data: {completed}\n\n") +} + +#[test] +fn help_smoke_covers_high_cost_commands() { + let cli = LightweightCli::new(); + + let mut artifact = cli.command(); + artifact.args(["artifact", "--help"]); + fabro_snapshot!(artifact, @" + success: true + exit_code: 0 + ----- stdout ----- + Inspect and copy run artifacts (screenshots, reports, traces) + + Usage: fabro artifact [OPTIONS] + + Commands: + list List artifacts for a workflow run + cp Copy artifacts from a workflow run + help Print this message or the help of the given subcommand(s) + + Options: + --json Output as JSON [env: FABRO_JSON=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); + + let mut artifact_list = cli.command(); + artifact_list.args(["artifact", "list", "--help"]); + fabro_snapshot!(artifact_list, @" + success: true + exit_code: 0 + ----- stdout ----- + List artifacts for a workflow run + + Usage: fabro artifact list [OPTIONS] + + Arguments: + Run ID (or prefix) + + Options: + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --node Filter to artifacts from a specific node + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --retry Filter to artifacts from a specific retry attempt + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); + + let mut artifact_cp = cli.command(); + artifact_cp.args(["artifact", "cp", "--help"]); + fabro_snapshot!(artifact_cp, @" + success: true + exit_code: 0 + ----- stdout ----- + Copy artifacts from a workflow run + + Usage: fabro artifact cp [OPTIONS] [DEST] + + Arguments: + Source: RUN_ID (all artifacts) or RUN_ID:path (specific artifact) + [DEST] Destination directory (defaults to current directory) [default: .] + + Options: + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --node Filter to artifacts from a specific node + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --retry Filter to artifacts from a specific retry attempt + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --tree Preserve {node_slug}/retry_{N}/ directory structure + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); + + let mut settings = cli.command(); + settings.args(["settings", "--help"]); + fabro_snapshot!(settings, @" + success: true + exit_code: 0 + ----- stdout ----- + Inspect effective settings + + Usage: fabro settings [OPTIONS] [WORKFLOW] + + Arguments: + [WORKFLOW] Optional workflow name, .fabro path, or .toml run config to overlay + + Options: + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --local Show only locally resolved settings and skip the server call + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); + + let mut attach = cli.command(); + attach.args(["attach", "--help"]); + fabro_snapshot!(attach, @" + success: true + exit_code: 0 + ----- stdout ----- + Attach to a running or finished workflow run + + Usage: fabro attach [OPTIONS] + + Arguments: + Run ID prefix or workflow name + + Options: + --json Output as JSON [env: FABRO_JSON=] + --server Fabro server target: http(s) URL or absolute Unix socket path [env: FABRO_SERVER=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); +} + +#[test] +fn completion_smoke_covers_help_and_generation() { + let cli = LightweightCli::new(); + + let mut help = cli.command(); + help.args(["completion", "--help"]); + fabro_snapshot!(help, @" + success: true + exit_code: 0 + ----- stdout ----- + Generate shell completions + + Usage: fabro completion [OPTIONS] + + Arguments: + Shell to generate completions for [possible values: bash, elvish, fish, powershell, zsh] + + Options: + --json Output as JSON [env: FABRO_JSON=] + --debug Enable DEBUG-level logging (default is INFO) [env: FABRO_DEBUG=] + --no-upgrade-check Disable automatic upgrade check [env: FABRO_NO_UPGRADE_CHECK=true] + --quiet Suppress non-essential output [env: FABRO_QUIET=] + --verbose Enable verbose output [env: FABRO_VERBOSE=] + -h, --help Print help + ----- stderr ----- + "); + + let mut zsh = cli.command(); + zsh.args(["completion", "zsh"]); + zsh.assert().success(); + + let mut fish = cli.command(); + fish.args(["completion", "fish"]); + fish.assert().success(); +} + +#[test] +fn attach_smoke_covers_arg_validation_and_remote_server_behaviors() { + let context = test_context!(); + + let mut missing_arg = context.command(); + missing_arg.arg("attach"); + fabro_snapshot!(context.filters(), missing_arg, @" + success: false + exit_code: 2 + ----- stdout ----- + ----- stderr ----- + error: the following required arguments were not provided: + + + Usage: fabro attach --no-upgrade-check + + For more information, try '--help'. + "); + + let success_server = MockServer::start(); + let success_run_id = unique_run_id(); + let list_mock = success_server.mock(|when, then| { + when.method("GET").path("/api/v1/runs"); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!([ + { + "run_id": success_run_id, + "workflow_name": "Remote Workflow", + "workflow_slug": "remote-workflow", + "goal": "Remote output", + "labels": {}, + "host_repo_path": null, + "start_time": "2026-04-05T12:00:00Z", + "status": "running", + "status_reason": null, + "duration_ms": 12, + "total_usd_micros": null + } + ]) + .to_string(), + ); + }); + success_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{success_run_id}/events")); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "data": [{ + "seq": 1, + "payload": { + "event": "run.running", + "id": "evt-run-running", + "run_id": success_run_id, + "ts": "2026-04-05T12:00:00Z", + "properties": {} + } + }], + "meta": { "has_more": false } + }) + .to_string(), + ); + }); + success_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{success_run_id}/state")); + then.status(200) + .header("Content-Type", "application/json") + .body(live_run_state_response().to_string()); + }); + success_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{success_run_id}/questions")) + .query_param("page[limit]", "100") + .query_param("page[offset]", "0"); + then.status(200) + .header("Content-Type", "application/json") + .body(r#"{"data":[],"meta":{"has_more":false}}"#); + }); + let attach_mock = success_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{success_run_id}/attach")) + .query_param("since_seq", "2"); + then.status(200) + .header("Content-Type", "text/event-stream") + .body(run_sse_body(success_run_id.as_str())); + }); + context.write_home( + ".fabro/settings.toml", + format!( + "[server]\ntarget = \"{}/api/v1\"\n", + success_server.base_url() + ), + ); + + let success_output = context + .command() + .args(["--json", "attach", &success_run_id]) + .output() + .expect("attach should execute"); + + assert!( + success_output.status.success(), + "attach failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&success_output.stdout), + String::from_utf8_lossy(&success_output.stderr) + ); + list_mock.assert(); + attach_mock.assert(); + let success_stdout = String::from_utf8(success_output.stdout).expect("stdout should be UTF-8"); + assert!( + success_stdout.contains("\"event\":\"run.completed\""), + "{success_stdout}" + ); + + let eof_server = MockServer::start(); + let eof_run_id = unique_run_id(); + + eof_server.mock(|when, then| { + when.method("GET").path("/api/v1/runs"); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!([ + { + "run_id": eof_run_id, + "workflow_name": "Remote Workflow", + "workflow_slug": "remote-workflow", + "goal": "Remote output", + "labels": {}, + "host_repo_path": null, + "start_time": "2026-04-05T12:00:00Z", + "status": "running", + "status_reason": null, + "duration_ms": 12, + "total_cost": null + } + ]) + .to_string(), + ); + }); + eof_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{eof_run_id}/events")); + then.status(200) + .header("Content-Type", "application/json") + .body( + serde_json::json!({ + "data": [{ + "seq": 1, + "payload": { + "event": "run.running", + "id": "evt-run-running", + "run_id": eof_run_id, + "ts": "2026-04-05T12:00:00Z", + "properties": {} + } + }], + "meta": { "has_more": false } + }) + .to_string(), + ); + }); + eof_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{eof_run_id}/state")); + then.status(200) + .header("Content-Type", "application/json") + .body(live_run_state_response().to_string()); + }); + eof_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{eof_run_id}/questions")) + .query_param("page[limit]", "100") + .query_param("page[offset]", "0"); + then.status(200) + .header("Content-Type", "application/json") + .body(r#"{"data":[],"meta":{"has_more":false}}"#); + }); + eof_server.mock(|when, then| { + when.method("GET") + .path(format!("/api/v1/runs/{eof_run_id}/attach")) + .query_param("since_seq", "2"); + then.status(200) + .header("Content-Type", "text/event-stream") + .body(""); + }); + context.write_home( + ".fabro/settings.toml", + format!("[server]\ntarget = \"{}/api/v1\"\n", eof_server.base_url()), + ); + + let eof_output = context + .command() + .args(["attach", &eof_run_id]) + .output() + .expect("attach should execute"); + + assert!( + !eof_output.status.success(), + "attach should fail on premature EOF" + ); + let eof_stderr = String::from_utf8(eof_output.stderr).expect("stderr should be UTF-8"); + assert!( + eof_stderr.contains("terminal run event"), + "expected a protocol error, got:\n{eof_stderr}" + ); +} diff --git a/lib/crates/fabro-cli/tests/it/support/mod.rs b/lib/crates/fabro-cli/tests/it/support/mod.rs index bb5bc9a7d..48fa25e72 100644 --- a/lib/crates/fabro-cli/tests/it/support/mod.rs +++ b/lib/crates/fabro-cli/tests/it/support/mod.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; +use assert_cmd::Command; use fabro_test::TestContext; use fabro_types::RunId; macro_rules! fabro_json_snapshot { @@ -50,3 +51,28 @@ pub(crate) fn run_output_filters(context: &TestContext) -> Vec<(String, String)> pub(crate) fn unique_run_id() -> String { RunId::new().to_string() } + +pub(crate) struct LightweightCli { + home_dir: tempfile::TempDir, +} + +impl LightweightCli { + pub(crate) fn new() -> Self { + Self { + home_dir: tempfile::tempdir().expect("temp home dir should exist"), + } + } + + pub(crate) fn command(&self) -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_fabro")); + cmd.env_clear(); + if let Some(path) = std::env::var_os("PATH") { + cmd.env("PATH", path); + } + cmd.env("HOME", self.home_dir.path()); + cmd.env("NO_COLOR", "1"); + cmd.env("FABRO_NO_UPGRADE_CHECK", "true"); + cmd.current_dir(self.home_dir.path()); + cmd + } +}