From 9a91deae7c2fb91f77d32d483c98cfeb2cd197a3 Mon Sep 17 00:00:00 2001 From: Val Alexander <68980965+BunsDev@users.noreply.github.com> Date: Sun, 18 Oct 2026 08:52:36 -0500 Subject: [PATCH] fix(cli): surface session persistence failures Report failed interactive session saves without claiming a rename persisted. Keep a persistent warning until retry succeeds and preserve private diagnostics and existing goal indexing behavior. Coven-Harness: Codex / GPT-6 --- scripts/test_cli_save_errors.py | 156 ++++++++++++++++++ .../crates/cli/src/goal_dispatch_tests.rs | 17 +- src-rust/crates/cli/src/main.rs | 67 ++++++-- src-rust/crates/cli/src/session_save_tests.rs | 139 ++++++++++++++++ 4 files changed, 362 insertions(+), 17 deletions(-) create mode 100644 scripts/test_cli_save_errors.py create mode 100644 src-rust/crates/cli/src/session_save_tests.rs diff --git a/scripts/test_cli_save_errors.py b/scripts/test_cli_save_errors.py new file mode 100644 index 00000000..ec89162e --- /dev/null +++ b/scripts/test_cli_save_errors.py @@ -0,0 +1,156 @@ +#!/usr/bin/env python3 +"""Owned, offline tmux regression for explicit rename persistence failures. + +No model request is needed. An owned loopback HTTP/proxy trap records unexpected +requests; startup model refresh is disabled and update caches are seeded fresh. +""" +import argparse +import hashlib +import http.server +import json +import os +from pathlib import Path +import shutil +import subprocess +import tempfile +import threading +import time + + +def run_case(binary, blocked, private_diagnostics): + requests = [] + + class Trap(http.server.BaseHTTPRequestHandler): + def reject(self): + requests.append((self.command, self.path)) + self.send_error(503) + do_GET = reject + do_POST = reject + do_CONNECT = reject + def log_message(self, *_): + pass + + server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Trap) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + with tempfile.TemporaryDirectory(prefix="coven-save-ui-") as temp: + root = Path(temp) + home, project = root / "home", root / "project" + engine = home / "engine" + sessions = engine / "sessions" + sessions.mkdir(parents=True) + project.mkdir() + empty_path = root / "bin" + empty_path.mkdir() + (engine / "settings.json").write_text(json.dumps({"hasCompletedOnboarding": True})) + for cache in [home / "Library/Caches", home / ".cache"]: + directory = cache / "coven-code" + directory.mkdir(parents=True) + (directory / "update_check.txt").write_text("") + session_id = "11111111-1111-4111-8111-111111111111" + path = sessions / (session_id + ".json") + original = json.dumps({ + "id": session_id, "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", "model": "claude-sonnet-4-6", + "title": "original-title", "working_dir": str(project), + "messages": [{"role": "user", "content": "history-fixture-loaded"}], + }).encode() + path.write_bytes(original) + proxy = f"http://127.0.0.1:{server.server_port}" + env = { + "HOME": str(home), "USERPROFILE": str(home), + "COVEN_CODE_HOME": str(engine), "COVEN_HOME": str(home / "coven"), + "COVEN_CAVE_HOME": str(home / "cave"), + "XDG_CACHE_HOME": str(home / ".cache"), "PATH": str(empty_path), + "SHELL": "/bin/sh", "TERM": "xterm-256color", + "SAVE_STDERR": str(root / "stderr"), + "ANTHROPIC_API_KEY": "owned-fixture-no-credential", + "COVEN_CODE_DISABLE_MODELS_FETCH": "1", + "HTTP_PROXY": proxy, "HTTPS_PROXY": proxy, "ALL_PROXY": proxy, + "http_proxy": proxy, "https_proxy": proxy, "all_proxy": proxy, + "NO_PROXY": "127.0.0.1", "no_proxy": "127.0.0.1", + } + tmux = [shutil.which("tmux"), "-f", "/dev/null", "-L", f"save-{os.getpid()}-{int(blocked)}"] + def call(*args): + return subprocess.run(tmux + list(args), check=True, capture_output=True, text=True, timeout=10).stdout + def capture(): + return call("capture-pane", "-t", "fixture", "-p") + def wait_for(needle): + deadline = time.monotonic() + 20 + while time.monotonic() < deadline: + pane = capture() + if needle in pane: + return pane + time.sleep(0.1) + raise AssertionError(f"missing {needle!r}:\n{pane}") + try: + launch = [str(binary), "--resume", session_id, "--api-base", proxy] + if not private_diagnostics: + launch = ["/bin/sh", "-c", 'exec "$@" 2>"$SAVE_STDERR"', "fixture", *launch] + call("new-session", "-d", "-s", "fixture", "-x", "140", "-y", "45", + "-c", str(project), "/usr/bin/env", "-i", + *[f"{k}={v}" for k, v in env.items()], *launch) + wait_for("history-fixture-loaded") + if blocked: + path.rename(root / "original.json") + path.mkdir() + (path / "sentinel").write_bytes(b"untouched") + call("send-keys", "-t", "fixture", "-l", "/session rename fixture-title") + call("send-keys", "-t", "fixture", "Enter") + pane = wait_for("Session not saved; retry before exit." if blocked else 'Session renamed to "fixture-title".') + if blocked: + assert 'Session renamed to "fixture-title".' not in pane, pane + assert "Session renamed in memory; not saved." in pane, pane + assert (root / "original.json").read_bytes() == original + assert (path / "sentinel").read_bytes() == b"untouched" + assert sorted(p.name for p in path.iterdir()) == ["sentinel"] + if private_diagnostics: + logs = "".join(p.read_text() for p in (engine / "logs").glob("session-*.log")) + else: + logs = (root / "stderr").read_text() + assert "Failed to persist session" in logs and session_id in logs and "rename" in logs, logs + assert "Is a directory" in logs, logs + assert "fixture-title" not in logs and "history-fixture-loaded" not in logs, logs + (path / "sentinel").unlink() + path.rmdir() + call("send-keys", "-t", "fixture", "-l", "/session rename retry-title") + call("send-keys", "-t", "fixture", "Enter") + pane = wait_for('Session renamed to "retry-title".') + assert "Session not saved" not in pane, pane + saved = json.loads(path.read_text()) + assert saved["title"] == "retry-title" + assert saved["messages"][0]["content"] == "history-fixture-loaded" + else: + saved = json.loads(path.read_text()) + assert saved["title"] == "fixture-title" + assert saved["messages"][0]["content"] == "history-fixture-loaded" + assert "Session not saved" not in pane + assert requests == [], requests + print(f"PASS {'blocked' if blocked else 'writable'} rename; no HTTP requests") + finally: + subprocess.run(tmux + ["kill-server"], capture_output=True, timeout=10) + server.shutdown() + server.server_close() + thread.join(timeout=5) + assert requests == [], requests + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("binary", type=Path) + parser.add_argument("--private-diagnostics", action="store_true") + args = parser.parse_args() + binary = args.binary.resolve() + assert binary.is_file() and shutil.which("tmux") + print(f"Binary SHA256 {hashlib.sha256(binary.read_bytes()).hexdigest()}") + failures = [] + for blocked in [False, True]: + try: + run_case(binary, blocked, args.private_diagnostics) + except Exception as error: + failures.append(f"blocked={blocked}: {error}") + assert not failures, "\n".join(failures) + + +if __name__ == "__main__": + main() diff --git a/src-rust/crates/cli/src/goal_dispatch_tests.rs b/src-rust/crates/cli/src/goal_dispatch_tests.rs index b4edbf5e..39b42a15 100644 --- a/src-rust/crates/cli/src/goal_dispatch_tests.rs +++ b/src-rust/crates/cli/src/goal_dispatch_tests.rs @@ -25,6 +25,7 @@ fn isolated(name: &str) -> bool { .env("COVEN_CODE_GOAL_DISPATCH_TEST", &name) .env("COVEN_CODE_HOME", &home) .env("COVEN_HOME", dir.path().join("coven")) + .env("COVEN_CAVE_HOME", dir.path().join("cave")) .env("CLAUDE_CONFIG_DIR", dir.path().join("claude")) .env("COVEN_CODE_CLAUDE_BIN", dir.path().join("absent-claude")) .env("COVEN_DAEMON_SOCKET", dir.path().join("absent.sock")) @@ -333,7 +334,8 @@ fn goal_dispatch_rate_limited_retry_does_not_adopt_replacement() { serde_json::to_string(&messages).unwrap(), serde_json::to_string(&before).unwrap() ); - persist_interactive_session(&session, Some(&owner)).await; + persist_interactive_session(&session, Some(&owner), &mut app.notifications) + .await; assert_eq!( serde_json::to_string( &claurst_core::history::load_session(&session.id) @@ -411,7 +413,7 @@ fn goal_dispatch_revoked_unattempted_uuid_is_absent_from_persistence() { ), "idle frames must not rewrite an already rolled-back session" ); - persist_interactive_session(&session, Some(&owner)).await; + persist_interactive_session(&session, Some(&owner), &mut app.notifications).await; assert_eq!( store.try_get_goal(&session.id).unwrap().unwrap(), replacement @@ -758,7 +760,7 @@ fn goal_dispatch_preparation_failure_then_revoke_rolls_back_json_and_index() { assert!(!turn.request_attempted()); messages = history.lock().await.clone(); session.messages = messages.clone(); - persist_interactive_session(&session, Some(&owner)).await; + persist_interactive_session(&session, Some(&owner), &mut app.notifications).await; let index = claurst_core::SqliteSessionStore::open( &Settings::config_dir().join("sessions.db"), ) @@ -796,7 +798,8 @@ fn goal_dispatch_preparation_failure_then_revoke_rolls_back_json_and_index() { assert!(turn.request_attempted()); assert_eq!(requests.load(std::sync::atomic::Ordering::SeqCst), 1); session.messages = history.lock().await.clone(); - persist_interactive_session(&session, Some(&owner)).await; + persist_interactive_session(&session, Some(&owner), &mut app.notifications) + .await; assert_eq!( index .list_sessions() @@ -833,7 +836,7 @@ fn goal_dispatch_preparation_failure_then_revoke_rolls_back_json_and_index() { &mut command_messages )); *history.lock().await = messages.clone(); - persist_interactive_session(&session, Some(&owner)).await; + persist_interactive_session(&session, Some(&owner), &mut app.notifications).await; let loaded = claurst_core::history::load_session(&session.id) .await .unwrap(); @@ -991,7 +994,7 @@ fn goal_dispatch_repeated_partial_failures_keep_manual_and_synthetic_baselines() assert!(goal.time_used_secs >= 3); } session.messages = messages; - persist_interactive_session(&session, Some(&owner)).await; + persist_interactive_session(&session, Some(&owner), &mut app.notifications).await; if synthetic { let index = claurst_core::SqliteSessionStore::open( &Settings::config_dir().join("sessions.db"), @@ -1310,7 +1313,7 @@ fn goal_dispatch_abandoned_inflight_outcome_does_not_rearm_recovery() { assert!(app.rate_limit_recovery.auto_retry_deadline.is_none()); messages = history.lock().await.clone(); session.messages = messages.clone(); - persist_interactive_session(&session, owner.as_ref()).await; + persist_interactive_session(&session, owner.as_ref(), &mut app.notifications).await; settle_abandoned_turn( &mut owner, false, diff --git a/src-rust/crates/cli/src/main.rs b/src-rust/crates/cli/src/main.rs index 33ad3ec8..983afe85 100644 --- a/src-rust/crates/cli/src/main.rs +++ b/src-rust/crates/cli/src/main.rs @@ -2350,7 +2350,7 @@ async fn settle_abandoned_turn( session, command_messages, ) { - persist_interactive_session(session, owner.as_ref()).await; + persist_interactive_session(session, owner.as_ref(), &mut app.notifications).await; } // Keep authority until the cancelled task has drained its partial output. if !query_running { @@ -2401,9 +2401,38 @@ impl InteractiveQuery { } } +const SESSION_SAVE_WARNING: &str = "Session not saved; retry before exit."; + +async fn save_session_with_warning( + session: &claurst_core::history::ConversationSession, + notifications: &mut claurst_tui::notifications::NotificationQueue, + operation: &str, +) -> bool { + use claurst_tui::notifications::NotificationKind; + match claurst_core::history::save_session(session).await { + Ok(()) => { + notifications.notifications.retain(|notification| { + notification.kind != NotificationKind::Warning + || notification.message != SESSION_SAVE_WARNING + }); + true + } + Err(error) => { + tracing::warn!(session_id = %session.id, operation, %error, "Failed to persist session"); + notifications.push( + NotificationKind::Warning, + SESSION_SAVE_WARNING.to_owned(), + None, + ); + false + } + } +} + async fn persist_interactive_session( session: &claurst_core::history::ConversationSession, owner: Option<&InteractiveTurn>, + notifications: &mut claurst_tui::notifications::NotificationQueue, ) { // A recoverable preparation/auth failure can finish without an attempt. // Delay indexing its synthetic UUID until attempted; a later stale retry @@ -2417,7 +2446,7 @@ async fn persist_interactive_session( }) .and_then(|owner| owner.synthetic_uuid.as_deref()); // JSON is primary storage; index only the finalized transcript. - let _ = claurst_core::history::save_session(session).await; + save_session_with_warning(session, notifications, "transcript update").await; // Also index into SQLite for /search support { @@ -3366,10 +3395,13 @@ async fn run_interactive( session.title = Some(title.clone()); session.updated_at = chrono::Utc::now(); cmd_ctx.session_title = session.title.clone(); - let _ = claurst_core::history::save_session(&session).await; + let saved = save_session_with_warning(&session, &mut app.notifications, "rename").await; claurst_tui::update_terminal_title(Some(&title)); - app.status_message = - Some(format!("Session renamed to \"{}\".", title)); + app.status_message = Some(if saved { + format!("Session renamed to \"{}\".", title) + } else { + "Session renamed in memory; not saved.".to_owned() + }); } Some(CommandResult::RefreshProviderState) => { if app.is_streaming || current_query.is_some() { @@ -3679,7 +3711,7 @@ async fn run_interactive( if rollback_unattempted_turn(goal_turn.as_ref(), true, &mut messages, &mut app, &mut session, &mut cmd_ctx.messages) { - persist_interactive_session(&session, goal_turn.as_ref()).await; + persist_interactive_session(&session, goal_turn.as_ref(), &mut app.notifications).await; } app.rate_limit_recovery.dismiss(); app.rate_limit_recovery.take_abandoned(); @@ -4352,7 +4384,12 @@ async fn run_interactive( // Persist the session URL into the saved session record. session.remote_session_url = Some(session_url.clone()); session.updated_at = chrono::Utc::now(); - let _ = claurst_core::history::save_session(&session).await; + save_session_with_warning( + &session, + &mut app.notifications, + "remote connection", + ) + .await; // Wire the BridgeSessionInfo relay so live tool/text events reach // the web UI via /api/bridge/sessions. This runs alongside @@ -4453,7 +4490,12 @@ async fn run_interactive( session.updated_at = chrono::Utc::now(); cmd_ctx.session_title = Some(title.clone()); app.session_title = Some(title); - let _ = claurst_core::history::save_session(&session).await; + save_session_with_warning( + &session, + &mut app.notifications, + "remote rename", + ) + .await; } Ok(TuiBridgeEvent::Error(msg)) => { app.bridge_state = BridgeConnectionState::Failed { @@ -4519,7 +4561,8 @@ async fn run_interactive( &mut session, &mut cmd_ctx.messages, ) { - persist_interactive_session(&session, goal_turn.as_ref()).await; + persist_interactive_session(&session, goal_turn.as_ref(), &mut app.notifications) + .await; } app.rate_limit_recovery.dismiss(); app.rate_limit_recovery.take_abandoned(); @@ -5024,7 +5067,8 @@ async fn run_interactive( &mut cmd_ctx.messages, ); *msgs_arc.lock().await = messages.clone(); - persist_interactive_session(&session, goal_turn.as_ref()).await; + persist_interactive_session(&session, goal_turn.as_ref(), &mut app.notifications) + .await; // --- Goal continuation --- // After every completed turn check if there is an active goal. @@ -6845,3 +6889,6 @@ mod tests { #[cfg(test)] mod goal_dispatch_tests; + +#[cfg(test)] +mod session_save_tests; diff --git a/src-rust/crates/cli/src/session_save_tests.rs b/src-rust/crates/cli/src/session_save_tests.rs new file mode 100644 index 00000000..c362d606 --- /dev/null +++ b/src-rust/crates/cli/src/session_save_tests.rs @@ -0,0 +1,139 @@ +use super::*; +use claurst_tui::notifications::{NotificationKind, NotificationQueue}; + +#[test] +fn session_save_failures_and_recovery_are_visible() { + let temp = tempfile::tempdir().unwrap(); + let log = temp.path().join("stderr"); + let mut child = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "session_save_tests::session_save_worker", + "--ignored", + "--nocapture", + ]) + .env_clear() + .env("COVEN_CODE_HOME", temp.path()) + .env("COVEN_HOME", temp.path().join("coven")) + .env("COVEN_CAVE_HOME", temp.path().join("cave")) + .env("HOME", temp.path()) + .env("USERPROFILE", temp.path()) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::fs::File::create(&log).unwrap()) + .spawn() + .unwrap(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + let status = loop { + match child.try_wait() { + Ok(Some(status)) => break status, + Ok(None) if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(10)) + } + result => { + let _ = child.kill(); + let _ = child.wait(); + panic!("bounded session save worker: {result:?}"); + } + } + }; + assert!( + status.success(), + "{}", + std::fs::read_to_string(log).unwrap() + ); + assert_eq!(std::fs::read(temp.path().join("done")).unwrap(), b"checked"); +} + +#[test] +#[ignore = "isolated session save environment"] +fn session_save_worker() { + let home = PathBuf::from(std::env::var_os("COVEN_CODE_HOME").unwrap()); + assert_eq!(Settings::config_dir(), home); + let diagnostic = home.join("diagnostic"); + let subscriber = tracing_subscriber::fmt() + .with_ansi(false) + .without_time() + .with_max_level(tracing::Level::WARN) + .with_writer(std::sync::Mutex::new( + std::fs::File::create(&diagnostic).unwrap(), + )) + .finish(); + let _guard = tracing::subscriber::set_default(subscriber); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + runtime.block_on(async { + for (index, operation) in [ + "rename", + "remote connection", + "remote rename", + "turn completion", + "transcript update", + ] + .iter() + .enumerate() + { + let mut session = + claurst_core::history::ConversationSession::new("fixture-model".into()); + session.id = format!("fixture-{index}"); + session.title = Some("private-title-marker".into()); + session + .messages + .push(claurst_core::types::Message::user("private-message-marker")); + let before = serde_json::to_vec_pretty(&session).unwrap(); + let path = home.join("sessions").join(format!("{}.json", session.id)); + std::fs::create_dir_all(&path).unwrap(); + std::fs::write(path.join("sentinel"), b"untouched").unwrap(); + let cause = std::fs::OpenOptions::new() + .write(true) + .open(&path) + .unwrap_err() + .to_string(); + let mut notifications = NotificationQueue::new(); + notifications.push(NotificationKind::Warning, "unrelated warning".into(), None); + for _ in 0..2 { + if *operation == "transcript update" { + persist_interactive_session(&session, None, &mut notifications).await; + } else { + assert!( + !save_session_with_warning(&session, &mut notifications, operation).await + ); + } + } + assert_eq!(notifications.notifications.len(), 2); + let warning = notifications.current().unwrap(); + assert_eq!(warning.kind, NotificationKind::Warning); + assert_eq!(warning.message, SESSION_SAVE_WARNING); + assert!(warning.expires_at.is_none()); + notifications.tick(); + assert!(!notifications.current_is_error()); + assert_eq!(std::fs::read(path.join("sentinel")).unwrap(), b"untouched"); + assert_eq!(std::fs::read_dir(&path).unwrap().count(), 1); + assert_eq!(serde_json::to_vec_pretty(&session).unwrap(), before); + let logs = std::fs::read_to_string(&diagnostic).unwrap(); + assert!(logs.contains("Failed to persist session")); + assert!(logs.contains(&session.id)); + assert!(logs.contains(operation)); + assert!(logs.contains(&cause)); + assert!(!logs.contains("private-title-marker")); + assert!(!logs.contains("private-message-marker")); + std::fs::remove_file(path.join("sentinel")).unwrap(); + std::fs::remove_dir(&path).unwrap(); + if *operation == "transcript update" { + persist_interactive_session(&session, None, &mut notifications).await; + } else { + assert!(save_session_with_warning(&session, &mut notifications, operation).await); + } + assert_eq!(std::fs::read(&path).unwrap(), before); + assert_eq!(notifications.notifications.len(), 1); + assert_eq!( + notifications.current().unwrap().message, + "unrelated warning" + ); + assert_eq!(std::fs::read_to_string(&diagnostic).unwrap(), logs); + } + }); + std::fs::write(home.join("done"), b"checked").unwrap(); +}