From 4677149e5e17c9802b36c6d5e558889e98d994f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=80=9Chaonan=E2=80=9D?= Date: Sat, 22 Aug 2026 21:34:13 +0800 Subject: [PATCH] fix(record): recover failed Trace exports and hint --url when the default start page fails Reject legacy JSON --output before recording starts, persist the completed Trace before writing the bundle so `bsk record stop --output ` can retry after an export failure, and tell users to pass --url when https://example.com/ does not load. --- apps/extension/src/tools/record.ts | 2 +- crates/bsk-cli/src/cli/mod.rs | 1 + crates/bsk-cli/src/cli/record.rs | 220 ++++++++++-- crates/bsk-cli/src/cli/record/export.rs | 163 ++++++++- crates/bsk-cli/src/cli/record_recovery.rs | 113 ++++++ crates/bsk-cli/src/cli/render_error.rs | 27 ++ crates/bsk-cli/src/daemon/paths.rs | 12 + .../bsk-cli/tests/record_export_recovery.rs | 327 ++++++++++++++++++ 8 files changed, 830 insertions(+), 35 deletions(-) create mode 100644 crates/bsk-cli/src/cli/record_recovery.rs create mode 100644 crates/bsk-cli/tests/record_export_recovery.rs diff --git a/apps/extension/src/tools/record.ts b/apps/extension/src/tools/record.ts index 71b122f..9aa125d 100644 --- a/apps/extension/src/tools/record.ts +++ b/apps/extension/src/tools/record.ts @@ -894,7 +894,7 @@ export async function handleRecordStart( code: "invalid_params", message: params.url ? `cannot record on restricted URL (${startUrl}); use an http(s) page` - : `cannot record on restricted URL (${startUrl}); default start page must be injectable http(s)`, + : `cannot record on restricted URL (${startUrl}); default start page https://example.com/ did not load — pass --url with a page you can open`, }; } diff --git a/crates/bsk-cli/src/cli/mod.rs b/crates/bsk-cli/src/cli/mod.rs index 9b7644e..cba2d49 100644 --- a/crates/bsk-cli/src/cli/mod.rs +++ b/crates/bsk-cli/src/cli/mod.rs @@ -22,6 +22,7 @@ pub mod navigate; pub mod network; pub mod observe; pub mod record; +pub mod record_recovery; pub mod record_state; pub mod render_error; pub mod screenshot; diff --git a/crates/bsk-cli/src/cli/record.rs b/crates/bsk-cli/src/cli/record.rs index 6414628..fbc2dfe 100644 --- a/crates/bsk-cli/src/cli/record.rs +++ b/crates/bsk-cli/src/cli/record.rs @@ -1,14 +1,14 @@ //! `bsk record start|stop` — capture user actions in the Agent Window. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::Context; -use bsk_protocol::Method; use bsk_protocol::tools::{ RecordAwaitParams, RecordAwaitResult, RecordStartParams, RecordStartResult, RecordStopParams, RecordStopResult, RecordedTrace, TRACE_VERSION_V3, }; +use bsk_protocol::{ErrorCode, Method}; use clap::{Args, Subcommand}; use crate::cli::TOOL_IPC_TIMEOUT; @@ -18,9 +18,13 @@ mod export; use crate::cli::business_rpc; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; +use crate::cli::record_recovery; use crate::cli::record_state; use crate::cli::session::{SessionStartOptions, start_session, stop_session}; -use export::{ExportMeta, export_recorded_trace, states_dir_for_output, trace_json_path}; +use export::{ + ExportMeta, export_with_recovery, states_dir_for_output, trace_json_path, + validate_record_output, +}; /// Max wait for the user to click 结束 in the browser (24 hours). const RECORD_AWAIT_TIMEOUT_MS: u32 = 86_400_000; @@ -53,7 +57,8 @@ pub struct RecordStartArgs { pub tab_id: Option, /// Navigate to this http(s) URL before recording. When omitted, defaults - /// to `https://example.com/`. + /// to `https://example.com/`. If that page does not load, pass `--url` + /// with a site you can open in this browser. #[arg(long)] pub url: Option, @@ -91,11 +96,7 @@ pub fn dispatch(cmd: RecordCmd, format: Format) -> Result<(), CliError> { } fn dispatch_start(args: RecordStartArgs, format: Format) -> Result<(), CliError> { - if record_state::read().is_ok() { - return Err(CliError::Local(anyhow::anyhow!( - "a recording is already in progress; run `bsk record stop` first" - ))); - } + prepare_record_start(&args.output)?; let info = ensure_daemon().context("ensure daemon is running")?; let session = start_session( @@ -109,7 +110,7 @@ fn dispatch_start(args: RecordStartArgs, format: Format) -> Result<(), CliError> let start_params = RecordStartParams { session_id: session.session_id.clone(), tab_id: args.tab_id, - url: args.url, + url: args.url.clone(), purpose: args.purpose.clone(), max_page_tokens: args.max_page_tokens, redact_values: Some(args.redact_values), @@ -127,7 +128,7 @@ fn dispatch_start(args: RecordStartArgs, format: Format) -> Result<(), CliError> Ok(result) => result, Err(err) => { let _ = stop_session(info.sock_path, &session.session_id); - return Err(err); + return Err(annotate_default_start_page_error(err, args.url.as_deref())); } }; @@ -158,7 +159,7 @@ fn dispatch_start(args: RecordStartArgs, format: Format) -> Result<(), CliError> // Keep write/render inside a Result so `?` cannot skip session teardown. let run_result: Result<(), CliError> = match await_result { Ok(result) => (|| { - let exported = export_recorded_trace(&args.output, &result.trace)?; + let exported = export_with_recovery(&args.output, &result.trace)?; render_finish(&result.trace, &args.output, &exported, format) })(), Err(err) => Err(err), @@ -173,32 +174,101 @@ fn dispatch_start(args: RecordStartArgs, format: Format) -> Result<(), CliError> } fn dispatch_stop(args: RecordStopArgs, format: Format) -> Result<(), CliError> { - let state = record_state::read().map_err(CliError::Local)?; - let info = ensure_daemon().context("ensure daemon is running")?; - let session_id = state.session_id.clone(); + validate_record_output(&args.output)?; - let params = RecordStopParams { - session_id: session_id.clone(), + if let Ok(state) = record_state::read() { + let info = ensure_daemon().context("ensure daemon is running")?; + let session_id = state.session_id.clone(); + + let params = RecordStopParams { + session_id: session_id.clone(), + }; + let result = business_rpc::call::( + info.sock_path.clone(), + "record-stop", + Method::ToolRecordStop, + Some(params), + TOOL_IPC_TIMEOUT, + )?; + + let run_result: Result<(), CliError> = (|| { + let exported = export_with_recovery(&args.output, &result.trace)?; + render_stop(&result, &args.output, &exported, format) + })(); + + let session_stop_result = stop_session(info.sock_path, &session_id); + record_state::clear(); + + run_result?; + session_stop_result?; + return Ok(()); + } + + let Some(trace) = record_recovery::load().map_err(CliError::Local)? else { + return Err(CliError::Local(anyhow::anyhow!( + "no recording in progress; run `bsk record start` first" + ))); }; - let result = business_rpc::call::( - info.sock_path.clone(), - "record-stop", - Method::ToolRecordStop, - Some(params), - TOOL_IPC_TIMEOUT, - )?; - let run_result: Result<(), CliError> = (|| { - let exported = export_recorded_trace(&args.output, &result.trace)?; - render_stop(&result, &args.output, &exported, format) - })(); + let exported = export_with_recovery(&args.output, &trace)?; + render_finish(&trace, &args.output, &exported, format) +} - let session_stop_result = stop_session(info.sock_path, &session_id); - record_state::clear(); +/// When `record start` omitted `--url` and the default example.com page +/// never loaded, rewrite the RPC error so the CLI hint points at `--url`. +fn annotate_default_start_page_error(err: CliError, user_url: Option<&str>) -> CliError { + if user_url.is_some() { + return err; + } + let CliError::Rpc { + code, + message, + data, + source, + } = err + else { + return err; + }; + if !is_default_start_page_failure(code, &message) { + return CliError::Rpc { + code, + message, + data, + source, + }; + } + CliError::Rpc { + code, + message, + data: Some(serde_json::json!({ + "reason": crate::cli::render_error::reason::RECORD_START_PAGE_UNREACHABLE + })), + source, + } +} - run_result?; - session_stop_result?; - Ok(()) +fn is_default_start_page_failure(code: ErrorCode, message: &str) -> bool { + match code { + ErrorCode::CdpFailed | ErrorCode::Timeout => true, + ErrorCode::InvalidParams => { + message.contains("restricted URL") || message.contains("about:blank") + } + _ => false, + } +} + +fn prepare_record_start(output: &Path) -> Result<(), CliError> { + if record_state::read().is_ok() { + return Err(CliError::Local(anyhow::anyhow!( + "a recording is already in progress; run `bsk record stop` first" + ))); + } + if record_recovery::exists() { + return Err(CliError::Local(anyhow::anyhow!( + "a previous recording was not exported; run `bsk record stop --output ` to recover it first" + ))); + } + validate_record_output(output) } fn record_await_ipc_timeout(timeout_ms: u32) -> Duration { @@ -284,6 +354,59 @@ fn render_stop( mod tests { use super::*; + fn with_temp_home(f: F) { + let _lock = record_recovery::test_env_lock(); + let tmp = tempfile::tempdir().unwrap(); + unsafe { + std::env::set_var(crate::daemon::paths::BSK_HOME_ENV, tmp.path()); + } + f(); + record_recovery::clear(); + record_state::clear(); + unsafe { + std::env::remove_var(crate::daemon::paths::BSK_HOME_ENV); + } + } + + #[test] + fn prepare_record_start_rejects_existing_trace_json_file() { + with_temp_home(|| { + let dir = tempfile::tempdir().unwrap(); + let output = dir.path().join("trace.json"); + std::fs::write(&output, "{}\n").unwrap(); + + let err = prepare_record_start(&output).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("JSON file") || msg.contains("json file"), + "{msg}" + ); + assert!( + msg.contains("states/") || msg.contains("--output trace"), + "{msg}" + ); + }); + } + + #[test] + fn prepare_record_start_rejects_unexported_recovery() { + with_temp_home(|| { + crate::daemon::paths::ensure_bsk_home().unwrap(); + std::fs::write( + crate::daemon::paths::record_recovery_path().unwrap(), + "{}\n", + ) + .unwrap(); + + let err = prepare_record_start(Path::new("trace")).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("not exported") || msg.contains("recover"), + "{msg}" + ); + }); + } + #[test] fn default_output_is_trace_dir() { let args = RecordStopArgs { @@ -311,4 +434,35 @@ mod tests { let got = record_await_ipc_timeout(RECORD_AWAIT_TIMEOUT_MS); assert!(got >= Duration::from_secs(86_400)); } + + #[test] + fn annotate_default_start_page_error_adds_url_reason() { + let err = CliError::from_rpc(bsk_protocol::RpcError { + code: ErrorCode::CdpFailed, + message: "Page.navigate rejected: net::ERR_ABORTED".into(), + data: None, + }); + let annotated = annotate_default_start_page_error(err, None); + assert_eq!( + crate::cli::render_error::reason_for_data(annotated.data()), + Some(crate::cli::render_error::reason::RECORD_START_PAGE_UNREACHABLE) + ); + let info = + crate::cli::render_error::info_for_error(annotated.code().unwrap(), annotated.data()); + assert!(info.hint.unwrap().contains("--url")); + } + + #[test] + fn annotate_default_start_page_error_skips_when_user_passed_url() { + let err = CliError::from_rpc(bsk_protocol::RpcError { + code: ErrorCode::CdpFailed, + message: "Page.navigate rejected: net::ERR_ABORTED".into(), + data: None, + }); + let annotated = annotate_default_start_page_error(err, Some("https://www.example.org/")); + assert_eq!( + crate::cli::render_error::reason_for_data(annotated.data()), + None + ); + } } diff --git a/crates/bsk-cli/src/cli/record/export.rs b/crates/bsk-cli/src/cli/record/export.rs index b406130..7842afb 100644 --- a/crates/bsk-cli/src/cli/record/export.rs +++ b/crates/bsk-cli/src/cli/record/export.rs @@ -9,6 +9,7 @@ use serde::Serialize; use crate::cli::error::CliError; +#[derive(Debug)] pub(super) struct ExportMeta { pub(super) states_dir: Option, pub(super) trace_version: u32, @@ -40,6 +41,89 @@ pub(super) fn export_recorded_trace( } } +/// Save the completed Trace first, then write the bundle. Recovery is +/// removed only after the export commits so a bad `--output` cannot drop +/// a recording the extension already returned. +pub(super) fn export_with_recovery( + output_dir: &Path, + trace: &RecordedTrace, +) -> Result { + let save_result = crate::cli::record_recovery::save(trace); + match export_recorded_trace(output_dir, trace) { + Ok(meta) => { + crate::cli::record_recovery::clear(); + Ok(meta) + } + Err(export_err) => { + let export_err = annotate_recovery_hint(export_err, save_result.is_ok()); + if let Err(save_err) = save_result { + return Err(match export_err { + CliError::Local(inner) => CliError::Local( + inner.context(format!("also failed to save recovery data: {save_err}")), + ), + other => other, + }); + } + Err(export_err) + } + } +} + +fn annotate_recovery_hint(err: CliError, recovery_saved: bool) -> CliError { + if !recovery_saved { + return err; + } + match err { + CliError::Local(inner) => CliError::Local(inner.context( + "export failed; the recorded trace was saved and can be recovered with `bsk record stop --output `", + )), + other => other, + } +} + +fn looks_like_json_output(path: &Path) -> bool { + path.extension() + .and_then(|ext| ext.to_str()) + .is_some_and(|ext| ext.eq_ignore_ascii_case("json")) +} + +fn legacy_json_output_error(path: &Path) -> CliError { + CliError::Local(anyhow::anyhow!( + "--output {} is a JSON file path; Trace v3 writes a bundle directory \ + (`/trace.json` and `/states/`), not a single file. \ + Use `--output trace` or another directory.", + path.display() + )) +} + +/// Reject legacy `--output trace.json` (and any existing non-directory) +/// before a recording starts, so the user is not blocked only after Finish. +pub(super) fn validate_record_output(path: &Path) -> Result<(), CliError> { + match fs::symlink_metadata(path) { + Ok(metadata) if metadata.file_type().is_symlink() => Err(CliError::Local(anyhow::anyhow!( + "--output {} must not be a symlink", + path.display() + ))), + Ok(metadata) if metadata.is_dir() => Ok(()), + Ok(_) if looks_like_json_output(path) => Err(legacy_json_output_error(path)), + Ok(_) => Err(CliError::Local(anyhow::anyhow!( + "--output {} is not a directory; Trace v3 writes a bundle directory \ + (`/trace.json` and `/states/`). Use `--output trace`.", + path.display() + ))), + Err(err) if err.kind() == io::ErrorKind::NotFound => { + if looks_like_json_output(path) { + Err(legacy_json_output_error(path)) + } else { + Ok(()) + } + } + Err(err) => Err(CliError::Local( + anyhow::Error::new(err).context(format!("inspect --output {}", path.display())), + )), + } +} + fn acquire_export_lock(output_dir: &Path) -> Result { validate_bundle_directory(output_dir, "output bundle directory")?; fs::create_dir_all(output_dir) @@ -126,8 +210,9 @@ fn validate_bundle_directory(path: &Path, description: &str) -> Result<(), CliEr path.display() ))), Ok(metadata) if metadata.is_dir() => Ok(()), + Ok(_) if looks_like_json_output(path) => Err(legacy_json_output_error(path)), Ok(_) => Err(CliError::Local(anyhow::anyhow!( - "{description} {} is not a directory", + "{description} {} is not a directory; Trace v3 writes `/trace.json` and `/states/`. Use `--output trace`.", path.display() ))), Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(()), @@ -812,4 +897,80 @@ mod tests { assert!(exported.states_dir.is_none()); assert!(output.join("trace.json").exists()); } + + #[test] + fn existing_trace_json_file_is_rejected_with_bundle_usage() { + let dir = tempfile::tempdir().unwrap(); + let output = dir.path().join("trace.json"); + fs::write(&output, "{}\n").unwrap(); + + let err = validate_record_output(&output).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("JSON file") || msg.contains("json file"), + "should name the legacy JSON file usage: {msg}" + ); + assert!( + msg.contains("states/") || msg.contains("--output trace"), + "should hint at the new bundle directory usage: {msg}" + ); + assert_eq!(fs::read_to_string(&output).unwrap(), "{}\n"); + } + + #[test] + fn missing_trace_json_path_is_rejected_as_legacy_output() { + let err = validate_record_output(Path::new("trace.json")).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("JSON file") || msg.contains("json file"), + "{msg}" + ); + assert!( + msg.contains("states/") || msg.contains("--output trace"), + "{msg}" + ); + } + + #[test] + fn export_failure_keeps_recovery_for_retry() { + let _lock = crate::cli::record_recovery::test_env_lock(); + let tmp = tempfile::tempdir().unwrap(); + unsafe { + std::env::set_var(crate::daemon::paths::BSK_HOME_ENV, tmp.path()); + } + + let dir = tempfile::tempdir().unwrap(); + let bad_output = dir.path().join("trace.json"); + fs::write(&bad_output, "{}\n").unwrap(); + let trace = RecordedTrace::V3(sample_trace("s1", "@vom 1\nL1 page")); + + let err = export_with_recovery(&bad_output, &trace).unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("recover") || msg.contains("record stop"), + "export failure should tell the user how to recover: {msg}" + ); + assert!( + crate::cli::record_recovery::exists(), + "recovery data must survive a failed bundle export" + ); + let recovered = crate::cli::record_recovery::load() + .unwrap() + .expect("recovery file"); + assert_eq!(recovered, trace); + assert_eq!(fs::read_to_string(&bad_output).unwrap(), "{}\n"); + + let good_output = dir.path().join("bundle"); + export_with_recovery(&good_output, &recovered).unwrap(); + assert!(good_output.join("trace.json").exists()); + assert!(good_output.join("states/s1.txt").exists()); + assert!( + !crate::cli::record_recovery::exists(), + "successful export must delete recovery data" + ); + + unsafe { + std::env::remove_var(crate::daemon::paths::BSK_HOME_ENV); + } + } } diff --git a/crates/bsk-cli/src/cli/record_recovery.rs b/crates/bsk-cli/src/cli/record_recovery.rs new file mode 100644 index 0000000..f52d1af --- /dev/null +++ b/crates/bsk-cli/src/cli/record_recovery.rs @@ -0,0 +1,113 @@ +//! Persist a completed Trace so a failed bundle export can be retried. + +use std::fs; + +use anyhow::{Context, Result}; +use bsk_protocol::tools::RecordedTrace; + +use crate::daemon::paths; + +pub fn save(trace: &RecordedTrace) -> Result<()> { + let path = paths::record_recovery_path()?; + paths::ensure_bsk_home()?; + let json = serde_json::to_string_pretty(trace).context("serialize record recovery")?; + let tmp = path.with_extension("tmp"); + fs::write(&tmp, format!("{json}\n")).with_context(|| format!("write {}", tmp.display()))?; + fs::rename(&tmp, &path).with_context(|| format!("commit {}", path.display()))?; + Ok(()) +} + +pub fn load() -> Result> { + let path = paths::record_recovery_path()?; + if !path.exists() { + return Ok(None); + } + let raw = fs::read_to_string(&path).with_context(|| format!("read {}", path.display()))?; + let trace = serde_json::from_str(&raw).with_context(|| format!("parse {}", path.display()))?; + Ok(Some(trace)) +} + +pub fn exists() -> bool { + paths::record_recovery_path() + .map(|path| path.exists()) + .unwrap_or(false) +} + +pub fn clear() { + if let Ok(path) = paths::record_recovery_path() { + let _ = fs::remove_file(path); + } +} + +#[cfg(test)] +pub(crate) fn test_env_lock() -> std::sync::MutexGuard<'static, ()> { + static GUARD: std::sync::Mutex<()> = std::sync::Mutex::new(()); + GUARD.lock().unwrap_or_else(|e| e.into_inner()) +} + +#[cfg(test)] +mod tests { + use super::*; + use bsk_protocol::tools::{ + RecorderInfo, StopReason, TRACE_VERSION_V3, TraceEntry, TraceStateV3, TraceV3, + VOM_FORMAT_VERSION, + }; + + fn sample_trace() -> RecordedTrace { + RecordedTrace::V3(TraceV3 { + version: TRACE_VERSION_V3, + recorded_at: "2026-08-10T02:12:55.360Z".into(), + stopped_by: StopReason::UserFinish, + entry: TraceEntry { + start_url: "https://example.com/".into(), + }, + recorder: RecorderInfo { + bsk: "0.1.10".into(), + vom: VOM_FORMAT_VERSION, + }, + states: vec![TraceStateV3 { + id: "s1".into(), + url: "https://example.com/".into(), + title: Some("Example".into()), + body: "@vom 1\nL1 page".into(), + truncated: false, + }], + steps: vec![], + purpose: None, + started_at: None, + }) + } + + fn with_temp_home(f: F) { + let _lock = test_env_lock(); + let tmp = tempfile::tempdir().unwrap(); + unsafe { + std::env::set_var(crate::daemon::paths::BSK_HOME_ENV, tmp.path()); + } + f(); + clear(); + unsafe { + std::env::remove_var(crate::daemon::paths::BSK_HOME_ENV); + } + } + + #[test] + fn save_load_round_trips() { + with_temp_home(|| { + let trace = sample_trace(); + save(&trace).unwrap(); + assert!(exists()); + assert_eq!(load().unwrap(), Some(trace)); + }); + } + + #[test] + fn clear_removes_recovery_file() { + with_temp_home(|| { + save(&sample_trace()).unwrap(); + clear(); + assert!(!exists()); + assert_eq!(load().unwrap(), None); + }); + } +} diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index 415387e..db52f47 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -50,6 +50,7 @@ pub mod reason { pub const BORROW_CONFLICT: &str = "borrow_conflict"; pub const SCREENSHOT_CAPTURE_FAILED: &str = "screenshot_capture_failed"; pub const SESSION_BUSY: &str = crate::rpc_reason::SESSION_BUSY; + pub const RECORD_START_PAGE_UNREACHABLE: &str = "record_start_page_unreachable"; } /// Per-code rendering metadata. The `summary` is what the CLI prints @@ -269,6 +270,16 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend ), exit_code: base.exit_code, }, + ( + ErrorCode::CdpFailed | ErrorCode::Timeout | ErrorCode::InvalidParams, + reason::RECORD_START_PAGE_UNREACHABLE, + ) => RenderInfo { + summary: "default start page https://example.com/ could not be opened", + hint: Some( + "that site may be unreachable from your network. Pass --url with a page you can open, e.g. `bsk record start --url https:///`", + ), + exit_code: base.exit_code, + }, _ => base, } } @@ -378,6 +389,22 @@ mod tests { assert_eq!(info.exit_code, 4); } + #[test] + fn record_start_page_unreachable_overrides_cdp_failed_copy() { + let data = serde_json::json!({ "reason": reason::RECORD_START_PAGE_UNREACHABLE }); + let info = info_for_error(ErrorCode::CdpFailed, Some(&data)); + assert_eq!( + info.summary, + "default start page https://example.com/ could not be opened" + ); + assert!( + info.hint.unwrap().contains("--url"), + "expected --url hint, got {:?}", + info.hint + ); + assert_eq!(info.exit_code, 3); + } + #[test] fn screenshot_capture_failed_overrides_cdp_failed_copy() { let data = serde_json::json!({ "reason": reason::SCREENSHOT_CAPTURE_FAILED }); diff --git a/crates/bsk-cli/src/daemon/paths.rs b/crates/bsk-cli/src/daemon/paths.rs index 6dde3f5..291a823 100644 --- a/crates/bsk-cli/src/daemon/paths.rs +++ b/crates/bsk-cli/src/daemon/paths.rs @@ -100,6 +100,14 @@ pub fn record_session_path() -> Result { Ok(bsk_home()?.join("record-session.json")) } +/// Path to a completed Trace saved before bundle export (`record-recovery.json`). +/// +/// Kept until export succeeds so a failed `--output` write cannot drop the +/// recording the extension already returned. +pub fn record_recovery_path() -> Result { + Ok(bsk_home()?.join("record-recovery.json")) +} + /// Windows named-pipe name. Include the resolved `BSK_HOME` path in the /// token so test homes and custom installs do not share a predictable /// per-username pipe. @@ -187,6 +195,10 @@ mod tests { record_session_path().unwrap(), home.join("record-session.json") ); + assert_eq!( + record_recovery_path().unwrap(), + home.join("record-recovery.json") + ); }); } diff --git a/crates/bsk-cli/tests/record_export_recovery.rs b/crates/bsk-cli/tests/record_export_recovery.rs new file mode 100644 index 0000000..956d535 --- /dev/null +++ b/crates/bsk-cli/tests/record_export_recovery.rs @@ -0,0 +1,327 @@ +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::Duration; + +use bsk::daemon::info::DaemonInfo; +use bsk::daemon::paths::BSK_HOME_ENV; +use bsk::daemon::{self, DaemonConfig}; +use bsk::ipc_client::IpcClient; +use bsk_protocol::system::{HandshakeParams, HandshakeResult}; +use bsk_protocol::tools::{ + RecordStopResult, RecordedTrace, RecorderInfo, SessionStartParams, SessionStartResult, + StopReason, TraceEntry, TraceStateV3, TraceV3, VOM_FORMAT_VERSION, +}; +use bsk_protocol::{BrowserPeerInfo, Frame, Method, RequestFrame, ResponseBody, ResponseFrame}; +use futures_util::{SinkExt, StreamExt}; +use serde_json::json; +use tokio::process::Command; +use tokio_tungstenite::tungstenite::handshake::client::generate_key; +use tokio_tungstenite::tungstenite::http::Request; +use tokio_tungstenite::tungstenite::protocol::Message; + +const TEST_EXT_ID: &str = "abcdefghijklmnopabcdefghijklmnop"; + +type Ws = + tokio_tungstenite::WebSocketStream>; + +fn bsk_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_bsk")) +} + +async fn connect_extension(addr: std::net::SocketAddr) -> Ws { + let origin = format!("chrome-extension://{TEST_EXT_ID}"); + let request = Request::builder() + .method("GET") + .uri(format!("ws://{addr}/")) + .header("Host", addr.to_string()) + .header("Upgrade", "websocket") + .header("Connection", "Upgrade") + .header("Sec-WebSocket-Version", "13") + .header("Sec-WebSocket-Key", generate_key()) + .header("Origin", origin) + .body(()) + .unwrap(); + tokio_tungstenite::connect_async(request).await.unwrap().0 +} + +async fn handshake(ws: &mut Ws) { + let request = RequestFrame { + id: "handshake".into(), + method: Method::SystemHandshake, + params: Some( + serde_json::to_value(HandshakeParams { + client: "browser-skill-extension".into(), + version: "0.1.10".parse().unwrap(), + protocol_version: "1.1".into(), + instance_id: TEST_EXT_ID.into(), + browser: BrowserPeerInfo { + name: "chrome".into(), + version: "131.0".into(), + }, + min_compatible_peer: Some("0.1.10".parse().unwrap()), + min_compatible_protocol: Some("1.1".into()), + label: "Record export recovery test".into(), + }) + .unwrap(), + ), + }; + ws.send(Message::Text(serde_json::to_string(&request).unwrap())) + .await + .unwrap(); + let Message::Text(response) = ws.next().await.unwrap().unwrap() else { + panic!("expected text handshake response"); + }; + let response: ResponseFrame = serde_json::from_str(&response).unwrap(); + let ResponseBody::Ok(value) = response.body else { + panic!("handshake failed"); + }; + let _: HandshakeResult = serde_json::from_value(value).unwrap(); +} + +fn sample_trace() -> TraceV3 { + TraceV3 { + version: 3, + purpose: Some("export recovery".into()), + started_at: Some("2026-08-12T10:00:00Z".into()), + recorded_at: "2026-08-12T10:01:00Z".into(), + stopped_by: StopReason::CliStop, + entry: TraceEntry { + start_url: "https://example.com/".into(), + }, + recorder: RecorderInfo { + bsk: "0.1.10".into(), + vom: VOM_FORMAT_VERSION, + }, + states: vec![TraceStateV3 { + id: "s1".into(), + url: "https://example.com/".into(), + title: Some("Example".into()), + body: "@vom 1\nL1 page".into(), + truncated: false, + }], + steps: vec![], + } +} + +fn run_extension( + mut ws: Ws, + record_stop_calls: Arc, + session_stop_calls: Arc, +) { + tokio::spawn(async move { + while let Some(Ok(Message::Text(text))) = ws.next().await { + let Ok(Frame::Request(request)) = serde_json::from_str::(&text) else { + continue; + }; + let body = match request.method { + Method::ToolSessionStart => { + let _: SessionStartParams = + serde_json::from_value(request.params.clone().unwrap()).unwrap(); + ResponseBody::Ok( + serde_json::to_value(SessionStartResult { + agent_window_id: Some(100), + }) + .unwrap(), + ) + } + Method::ToolRecordStop => { + record_stop_calls.fetch_add(1, Ordering::SeqCst); + ResponseBody::Ok( + serde_json::to_value(RecordStopResult { + trace: RecordedTrace::V3(sample_trace()), + }) + .unwrap(), + ) + } + Method::ToolSessionStop => { + session_stop_calls.fetch_add(1, Ordering::SeqCst); + ResponseBody::Ok(json!({})) + } + _ => ResponseBody::Err(bsk_protocol::RpcError { + code: bsk_protocol::ErrorCode::ProtocolError, + message: format!("unexpected method {:?}", request.method), + data: None, + }), + }; + let response = ResponseFrame { + id: request.id, + body, + }; + ws.send(Message::Text(serde_json::to_string(&response).unwrap())) + .await + .unwrap(); + } + }); +} + +async fn start_session(sock: &Path) -> String { + #[derive(serde::Deserialize)] + struct StartResult { + session_id: String, + } + + let mut client = IpcClient::connect(sock).await.unwrap(); + client + .call::<(), StartResult>( + "session-start", + Method::SessionStart, + None, + Duration::from_secs(5), + ) + .await + .unwrap() + .unwrap() + .session_id +} + +fn write_record_session(home: &Path, session_id: &str) { + std::fs::create_dir_all(home).unwrap(); + std::fs::write( + home.join("record-session.json"), + format!("{{\"session_id\":\"{session_id}\",\"started_at\":\"1\"}}\n"), + ) + .unwrap(); +} + +async fn run_record_stop(home: &Path, output: &Path) -> std::process::Output { + Command::new(bsk_bin()) + .arg("record") + .arg("stop") + .arg("--output") + .arg(output) + .env(BSK_HOME_ENV, home) + .output() + .await + .unwrap() +} + +async fn setup_daemon(temp: &Path) -> (bsk::daemon::DaemonHandle, PathBuf, PathBuf) { + let home = temp.join("bsk-home"); + let sock = temp.join("daemon.sock"); + let daemon = daemon::run(DaemonConfig::new(0), Some(sock.clone())) + .await + .unwrap(); + bsk::daemon::info::write_to_path( + &DaemonInfo::now( + std::process::id(), + sock.clone(), + daemon.ws_addr().port(), + env!("CARGO_PKG_VERSION"), + ), + &home.join("daemon.json"), + ) + .unwrap(); + (daemon, home, sock) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn existing_trace_json_file_is_rejected_before_record_stop() { + let temp = tempfile::Builder::new() + .prefix("bsk-record-json-") + .tempdir() + .unwrap(); + let (daemon, home, sock) = setup_daemon(temp.path()).await; + let output = temp.path().join("trace.json"); + std::fs::write(&output, "{}\n").unwrap(); + + let mut extension = connect_extension(daemon.ws_addr()).await; + handshake(&mut extension).await; + let record_stop_calls = Arc::new(AtomicUsize::new(0)); + let session_stop_calls = Arc::new(AtomicUsize::new(0)); + run_extension( + extension, + Arc::clone(&record_stop_calls), + Arc::clone(&session_stop_calls), + ); + + let session_id = start_session(&sock).await; + write_record_session(&home, &session_id); + + let first = run_record_stop(&home, &output).await; + let stderr = String::from_utf8_lossy(&first.stderr); + assert!( + !first.status.success(), + "legacy JSON --output should fail: {stderr}" + ); + assert!( + stderr.contains("JSON file") || stderr.contains("bundle directory"), + "{stderr}" + ); + let state: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(home.join("record-session.json")).unwrap()) + .unwrap(); + assert_eq!(state["session_id"], session_id); + assert_eq!( + record_stop_calls.load(Ordering::SeqCst), + 0, + "existing trace.json must be rejected before the extension returns the Trace" + ); + assert_eq!(session_stop_calls.load(Ordering::SeqCst), 0); + assert_eq!(std::fs::read_to_string(&output).unwrap(), "{}\n"); + + daemon.shutdown().await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn failed_bundle_export_can_be_recovered() { + let temp = tempfile::Builder::new() + .prefix("bsk-record-recov-") + .tempdir() + .unwrap(); + let (daemon, home, sock) = setup_daemon(temp.path()).await; + let output = temp.path().join("trace"); + std::fs::create_dir_all(&output).unwrap(); + std::fs::write(output.join("states"), "not a directory\n").unwrap(); + + let mut extension = connect_extension(daemon.ws_addr()).await; + handshake(&mut extension).await; + let record_stop_calls = Arc::new(AtomicUsize::new(0)); + let session_stop_calls = Arc::new(AtomicUsize::new(0)); + run_extension( + extension, + Arc::clone(&record_stop_calls), + Arc::clone(&session_stop_calls), + ); + + let session_id = start_session(&sock).await; + write_record_session(&home, &session_id); + + let first = run_record_stop(&home, &output).await; + let stderr = String::from_utf8_lossy(&first.stderr); + assert!( + !first.status.success(), + "blocked states path should fail export: {stderr}" + ); + assert!( + home.join("record-recovery.json").exists(), + "completed Trace must be saved before the failed export" + ); + assert!( + !home.join("record-session.json").exists(), + "session state is cleared after the extension returns the Trace" + ); + assert_eq!(record_stop_calls.load(Ordering::SeqCst), 1); + assert_eq!(session_stop_calls.load(Ordering::SeqCst), 1); + + std::fs::remove_file(output.join("states")).unwrap(); + let second = run_record_stop(&home, &output).await; + assert!( + second.status.success(), + "recovery export failed: {}", + String::from_utf8_lossy(&second.stderr) + ); + assert_eq!( + record_stop_calls.load(Ordering::SeqCst), + 1, + "recovery must not call the extension again" + ); + assert!(output.join("trace.json").exists()); + assert!(output.join("states/s1.txt").exists()); + assert!( + !home.join("record-recovery.json").exists(), + "successful export must delete recovery data" + ); + + daemon.shutdown().await; +}