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 <dir>` can retry after an export failure, and tell users to pass --url when https://example.com/ does not load.
2026-09-10 13:01:23 +08:00 · 2026-08-23 21:26:40 +08:00
parent 8ea5477e11
commit 4677149e5e
8 changed files with 830 additions and 35 deletions
@@ -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<i64>,

    /// 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<String>,

@@ -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::<RecordStopParams, RecordStopResult>(
+            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::<RecordStopParams, RecordStopResult>(
-        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 <dir>` 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: FnOnce()>(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
+        );
+    }
 }