mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
* fix: persist turns after approval and add agent-level tests Port relevant changes from PR #112 that were not carried over to #237: - Add persist_turn calls in process_approval for the response, error, and auth-required paths. Previously, turns completed after tool approval were never persisted to DB — if the process crashed after approval the entire turn (user message + assistant response) was lost. - Add agent-level unit tests: StaticLlmProvider mock, make_test_agent helper, tests for auto-approval logic, destructive shell command detection, and PendingApproval backward-compatible deserialization (without deferred_tool_calls field). - Remove unused _thread_state binding in process_approval. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address 14 audit findings in src/agent/ Audit of the agent module found 2 High, 7 Medium, 3 Low, and 2 Nit severity issues. This commit fixes all of them: High: - Remove 4 `.expect()` calls in session.rs (entry API, match, direct indexing, if-let) to eliminate panic paths in production - Add typed RoutineError enum replacing Result<_, String> across routine.rs, routine_engine.rs, and callers in history/store.rs and db/libsql/mod.rs Medium: - Sanitize routine names in path construction to prevent directory traversal (routine_engine.rs) - Log warnings for 5 silently-swallowed errors in scheduler.rs, compaction.rs, and worker.rs - Extract shared handle_auth_intercept helper to deduplicate auth interception in thread_ops.rs - Add session count warning threshold in session_manager.rs - Make FullJob stub degradation visible via warn-level log and prepended warning in output Low: - Restrict dead code visibility with #[cfg(test)] on 19 unused items in submission.rs, task.rs, and undo.rs - Narrow pub to pub(crate) on self_repair.rs builder methods - Remove TaskStatus from mod.rs re-exports (test-only type) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address PR review comments - Reorder persist_turn before persist_response_chain so the conversation row exists before the metadata UPDATE runs - Add persist_response_chain call to handle_auth_intercept so auth-required paths preserve the response chain - Harden sanitize_routine_name to use allowlist (alphanumeric, dash, underscore) instead of denylist replacements - Fix stale active_thread ID in get_or_create_thread: fall back to create_thread() when the stored ID is missing from the map - Persist turn on approval rejection so user messages survive crashes after a tool is rejected Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
377 lines
12 KiB
Rust
377 lines
12 KiB
Rust
//! Undo system with checkpoints.
|
|
//!
|
|
//! Provides the ability to roll back the conversation state to a previous point.
|
|
//! Checkpoints are created automatically at the start of each turn.
|
|
|
|
use std::collections::VecDeque;
|
|
|
|
use serde::{Deserialize, Serialize};
|
|
use uuid::Uuid;
|
|
|
|
use crate::llm::ChatMessage;
|
|
|
|
/// Maximum number of checkpoints to keep by default.
|
|
const DEFAULT_MAX_CHECKPOINTS: usize = 20;
|
|
|
|
/// A snapshot of conversation state at a point in time.
|
|
#[derive(Debug, Clone, Serialize, Deserialize)]
|
|
pub struct Checkpoint {
|
|
/// Unique checkpoint ID.
|
|
pub id: Uuid,
|
|
/// Turn number this checkpoint was created at.
|
|
pub turn_number: usize,
|
|
/// Snapshot of messages at this point.
|
|
pub messages: Vec<ChatMessage>,
|
|
/// Description of what happened at this checkpoint.
|
|
pub description: String,
|
|
}
|
|
|
|
impl Checkpoint {
|
|
/// Create a new checkpoint.
|
|
pub fn new(
|
|
turn_number: usize,
|
|
messages: Vec<ChatMessage>,
|
|
description: impl Into<String>,
|
|
) -> Self {
|
|
Self {
|
|
id: Uuid::new_v4(),
|
|
turn_number,
|
|
messages,
|
|
description: description.into(),
|
|
}
|
|
}
|
|
}
|
|
|
|
/// Manager for undo/redo functionality.
|
|
///
|
|
/// Each undo/redo operation pops from one stack and pushes the current state
|
|
/// onto the other, so `undo_count() + redo_count()` stays constant across
|
|
/// undo/redo cycles (only `checkpoint()` and `clear()` change the total).
|
|
pub struct UndoManager {
|
|
/// Stack of past checkpoints (for undo).
|
|
undo_stack: VecDeque<Checkpoint>,
|
|
/// Stack of future checkpoints (for redo).
|
|
redo_stack: Vec<Checkpoint>,
|
|
/// Maximum checkpoints to keep.
|
|
max_checkpoints: usize,
|
|
}
|
|
|
|
impl UndoManager {
|
|
/// Create a new undo manager.
|
|
pub fn new() -> Self {
|
|
Self {
|
|
undo_stack: VecDeque::new(),
|
|
redo_stack: Vec::new(),
|
|
max_checkpoints: DEFAULT_MAX_CHECKPOINTS,
|
|
}
|
|
}
|
|
|
|
/// Create with a custom checkpoint limit.
|
|
#[cfg(test)]
|
|
pub fn with_max_checkpoints(mut self, max: usize) -> Self {
|
|
self.max_checkpoints = max;
|
|
self
|
|
}
|
|
|
|
/// Push a checkpoint onto the undo stack, trimming oldest entries if over limit.
|
|
fn push_undo(&mut self, checkpoint: Checkpoint) {
|
|
self.undo_stack.push_back(checkpoint);
|
|
while self.undo_stack.len() > self.max_checkpoints {
|
|
self.undo_stack.pop_front();
|
|
}
|
|
}
|
|
|
|
/// Create a checkpoint at the current state.
|
|
///
|
|
/// This clears the redo stack since we're creating a new history branch.
|
|
pub fn checkpoint(
|
|
&mut self,
|
|
turn_number: usize,
|
|
messages: Vec<ChatMessage>,
|
|
description: impl Into<String>,
|
|
) {
|
|
// Clear redo stack (new branch of history)
|
|
self.redo_stack.clear();
|
|
|
|
let checkpoint = Checkpoint::new(turn_number, messages, description);
|
|
self.push_undo(checkpoint);
|
|
}
|
|
|
|
/// Undo: pop the last checkpoint and return it.
|
|
///
|
|
/// Saves the current state to the redo stack and pops the most recent
|
|
/// checkpoint from the undo stack so that repeated undos walk backwards
|
|
/// through history.
|
|
///
|
|
/// Takes ownership of `current_messages`; callers must clone first if
|
|
/// they need to retain a copy.
|
|
pub fn undo(
|
|
&mut self,
|
|
current_turn: usize,
|
|
current_messages: Vec<ChatMessage>,
|
|
) -> Option<Checkpoint> {
|
|
if self.undo_stack.is_empty() {
|
|
return None;
|
|
}
|
|
|
|
// Save current state to redo stack
|
|
let current = Checkpoint::new(
|
|
current_turn,
|
|
current_messages,
|
|
format!("Turn {}", current_turn),
|
|
);
|
|
self.redo_stack.push(current);
|
|
|
|
// Pop and return the most recent checkpoint
|
|
self.undo_stack.pop_back()
|
|
}
|
|
|
|
/// Pop the last checkpoint from the undo stack.
|
|
#[cfg(test)]
|
|
pub fn pop_undo(&mut self) -> Option<Checkpoint> {
|
|
self.undo_stack.pop_back()
|
|
}
|
|
|
|
/// Redo: restore a previously undone state.
|
|
///
|
|
/// Saves the current state to the undo stack and pops the most recent
|
|
/// checkpoint from the redo stack.
|
|
///
|
|
/// Takes ownership of `current_messages`; callers must clone first if
|
|
/// they need to retain a copy.
|
|
pub fn redo(
|
|
&mut self,
|
|
current_turn: usize,
|
|
current_messages: Vec<ChatMessage>,
|
|
) -> Option<Checkpoint> {
|
|
if self.redo_stack.is_empty() {
|
|
return None;
|
|
}
|
|
|
|
// Save current state to undo stack
|
|
let current = Checkpoint::new(
|
|
current_turn,
|
|
current_messages,
|
|
format!("Turn {}", current_turn),
|
|
);
|
|
self.push_undo(current);
|
|
|
|
self.redo_stack.pop()
|
|
}
|
|
|
|
/// Check if undo is available.
|
|
pub fn can_undo(&self) -> bool {
|
|
!self.undo_stack.is_empty()
|
|
}
|
|
|
|
/// Check if redo is available.
|
|
pub fn can_redo(&self) -> bool {
|
|
!self.redo_stack.is_empty()
|
|
}
|
|
|
|
/// Get the number of undo steps available.
|
|
pub fn undo_count(&self) -> usize {
|
|
self.undo_stack.len()
|
|
}
|
|
|
|
/// Get the number of redo steps available.
|
|
pub fn redo_count(&self) -> usize {
|
|
self.redo_stack.len()
|
|
}
|
|
|
|
/// Get a checkpoint by ID.
|
|
#[cfg(test)]
|
|
pub fn get_checkpoint(&self, id: Uuid) -> Option<&Checkpoint> {
|
|
self.undo_stack
|
|
.iter()
|
|
.find(|c| c.id == id)
|
|
.or_else(|| self.redo_stack.iter().find(|c| c.id == id))
|
|
}
|
|
|
|
/// List all available checkpoints (for UI display).
|
|
#[cfg(test)]
|
|
pub fn list_checkpoints(&self) -> Vec<&Checkpoint> {
|
|
self.undo_stack.iter().collect()
|
|
}
|
|
|
|
/// Clear all checkpoints.
|
|
pub fn clear(&mut self) {
|
|
self.undo_stack.clear();
|
|
self.redo_stack.clear();
|
|
}
|
|
|
|
/// Restore to a specific checkpoint by ID.
|
|
///
|
|
/// This invalidates all checkpoints after this one.
|
|
pub fn restore(&mut self, checkpoint_id: Uuid) -> Option<Checkpoint> {
|
|
// Find the checkpoint position
|
|
let pos = self.undo_stack.iter().position(|c| c.id == checkpoint_id)?;
|
|
|
|
// Clear redo stack
|
|
self.redo_stack.clear();
|
|
|
|
// Remove all checkpoints after this one
|
|
while self.undo_stack.len() > pos + 1 {
|
|
self.undo_stack.pop_back();
|
|
}
|
|
|
|
// Pop and return the target checkpoint
|
|
self.undo_stack.pop_back()
|
|
}
|
|
}
|
|
|
|
impl Default for UndoManager {
|
|
fn default() -> Self {
|
|
Self::new()
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
#[test]
|
|
fn test_checkpoint_creation() {
|
|
let mut manager = UndoManager::new();
|
|
|
|
manager.checkpoint(0, vec![], "Initial state");
|
|
manager.checkpoint(1, vec![ChatMessage::user("Hello")], "Turn 1");
|
|
|
|
assert_eq!(manager.undo_count(), 2);
|
|
}
|
|
|
|
#[test]
|
|
fn test_undo_redo() {
|
|
let mut manager = UndoManager::new();
|
|
|
|
manager.checkpoint(0, vec![], "Turn 0");
|
|
manager.checkpoint(1, vec![ChatMessage::user("Hello")], "Turn 1");
|
|
|
|
assert!(manager.can_undo());
|
|
assert!(!manager.can_redo());
|
|
|
|
// Undo - returns owned Checkpoint now
|
|
let current = vec![ChatMessage::user("Hello"), ChatMessage::assistant("Hi")];
|
|
let checkpoint = manager.undo(2, current);
|
|
assert!(checkpoint.is_some());
|
|
let checkpoint = checkpoint.unwrap();
|
|
assert_eq!(checkpoint.turn_number, 1);
|
|
assert!(manager.can_redo());
|
|
|
|
// Redo - now requires current state parameters
|
|
let restored = manager.redo(checkpoint.turn_number, checkpoint.messages);
|
|
assert!(restored.is_some());
|
|
}
|
|
|
|
#[test]
|
|
fn test_max_checkpoints() {
|
|
let mut manager = UndoManager::new().with_max_checkpoints(3);
|
|
|
|
for i in 0..5 {
|
|
manager.checkpoint(i, vec![], format!("Turn {}", i));
|
|
}
|
|
|
|
assert_eq!(manager.undo_count(), 3);
|
|
}
|
|
|
|
#[test]
|
|
fn test_restore_to_checkpoint() {
|
|
let mut manager = UndoManager::new();
|
|
|
|
manager.checkpoint(0, vec![], "Turn 0");
|
|
let checkpoint_id = manager.undo_stack.back().unwrap().id;
|
|
manager.checkpoint(1, vec![], "Turn 1");
|
|
manager.checkpoint(2, vec![], "Turn 2");
|
|
|
|
let restored = manager.restore(checkpoint_id);
|
|
assert!(restored.is_some());
|
|
assert_eq!(manager.undo_count(), 0);
|
|
}
|
|
|
|
#[test]
|
|
fn test_repeated_undo_advances_through_stack() {
|
|
let mut manager = UndoManager::new();
|
|
|
|
// Create 3 checkpoints at turns 0, 1, 2
|
|
manager.checkpoint(0, vec![], "Turn 0");
|
|
manager.checkpoint(1, vec![ChatMessage::user("msg1")], "Turn 1");
|
|
manager.checkpoint(2, vec![ChatMessage::user("msg2")], "Turn 2");
|
|
assert_eq!(manager.undo_count(), 3);
|
|
|
|
// First undo: should return turn 2 checkpoint, stack shrinks to 2
|
|
let cp1 = manager
|
|
.undo(3, vec![ChatMessage::user("msg3")])
|
|
.expect("first undo should succeed");
|
|
assert_eq!(cp1.turn_number, 2);
|
|
assert_eq!(manager.undo_count(), 2);
|
|
|
|
// Second undo: should return turn 1 checkpoint (different!), stack shrinks to 1
|
|
let cp2 = manager
|
|
.undo(cp1.turn_number, cp1.messages)
|
|
.expect("second undo should succeed");
|
|
assert_eq!(cp2.turn_number, 1);
|
|
assert_eq!(manager.undo_count(), 1);
|
|
|
|
// Verify we walked backwards through distinct checkpoints
|
|
assert_ne!(cp1.turn_number, cp2.turn_number);
|
|
}
|
|
|
|
#[test]
|
|
fn test_undo_redo_cycle_preserves_state() {
|
|
let mut manager = UndoManager::new();
|
|
|
|
let msgs_t0: Vec<ChatMessage> = vec![];
|
|
let msgs_t1 = vec![ChatMessage::user("hello")];
|
|
let msgs_t2 = vec![ChatMessage::user("hello"), ChatMessage::assistant("hi")];
|
|
|
|
manager.checkpoint(0, msgs_t0, "Turn 0");
|
|
manager.checkpoint(1, msgs_t1, "Turn 1");
|
|
|
|
// Undo from turn 2 -> get turn 1 checkpoint
|
|
let cp_undo1 = manager
|
|
.undo(2, msgs_t2.clone())
|
|
.expect("undo should succeed");
|
|
assert_eq!(cp_undo1.turn_number, 1);
|
|
|
|
// Redo from turn 1 -> get turn 2 state back
|
|
let cp_redo = manager
|
|
.redo(cp_undo1.turn_number, cp_undo1.messages)
|
|
.expect("redo should succeed");
|
|
assert_eq!(cp_redo.turn_number, 2);
|
|
assert_eq!(cp_redo.messages.len(), 2);
|
|
|
|
// Undo again from turn 2 -> should go back to turn 1 again
|
|
let cp_undo2 = manager
|
|
.undo(cp_redo.turn_number, cp_redo.messages)
|
|
.expect("second undo should succeed");
|
|
assert_eq!(cp_undo2.turn_number, 1);
|
|
}
|
|
|
|
#[test]
|
|
fn test_undo_redo_stack_sizes_consistent() {
|
|
let mut manager = UndoManager::new();
|
|
|
|
manager.checkpoint(0, vec![], "Turn 0");
|
|
manager.checkpoint(1, vec![ChatMessage::user("a")], "Turn 1");
|
|
manager.checkpoint(2, vec![ChatMessage::user("b")], "Turn 2");
|
|
|
|
// Start: undo=3, redo=0, total=3
|
|
let total = manager.undo_count() + manager.redo_count();
|
|
assert_eq!(total, 3);
|
|
|
|
// After undo: total should still be 3 (one moved from undo to redo,
|
|
// plus the current state pushed to redo)
|
|
// Actually: undo pops one (3->2), pushes current to redo (0->1), total=3
|
|
let cp = manager.undo(3, vec![]).unwrap();
|
|
assert_eq!(manager.undo_count() + manager.redo_count(), 3);
|
|
|
|
// After redo: redo pops one (1->0), pushes current to undo (2->3), total=3
|
|
let cp2 = manager.redo(cp.turn_number, cp.messages).unwrap();
|
|
assert_eq!(manager.undo_count() + manager.redo_count(), 3);
|
|
|
|
// After another undo: same invariant
|
|
let _cp3 = manager.undo(cp2.turn_number, cp2.messages).unwrap();
|
|
assert_eq!(manager.undo_count() + manager.redo_count(), 3);
|
|
}
|
|
}
|