mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-27 16:10:09 +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]>
391 lines
13 KiB
Rust
391 lines
13 KiB
Rust
//! Self-repair for stuck jobs and broken tools.
|
|
|
|
use std::sync::Arc;
|
|
use std::time::Duration;
|
|
|
|
use async_trait::async_trait;
|
|
use chrono::{DateTime, Utc};
|
|
use uuid::Uuid;
|
|
|
|
use crate::context::{ContextManager, JobState};
|
|
use crate::db::Database;
|
|
use crate::error::RepairError;
|
|
use crate::tools::{BuildRequirement, Language, SoftwareBuilder, SoftwareType, ToolRegistry};
|
|
|
|
/// A job that has been detected as stuck.
|
|
#[derive(Debug, Clone)]
|
|
pub struct StuckJob {
|
|
pub job_id: Uuid,
|
|
pub last_activity: DateTime<Utc>,
|
|
pub stuck_duration: Duration,
|
|
pub last_error: Option<String>,
|
|
pub repair_attempts: u32,
|
|
}
|
|
|
|
/// A tool that has been detected as broken.
|
|
#[derive(Debug, Clone)]
|
|
pub struct BrokenTool {
|
|
pub name: String,
|
|
pub failure_count: u32,
|
|
pub last_error: Option<String>,
|
|
pub first_failure: DateTime<Utc>,
|
|
pub last_failure: DateTime<Utc>,
|
|
pub last_build_result: Option<serde_json::Value>,
|
|
pub repair_attempts: u32,
|
|
}
|
|
|
|
/// Result of a repair attempt.
|
|
#[derive(Debug)]
|
|
pub enum RepairResult {
|
|
/// Repair was successful.
|
|
Success { message: String },
|
|
/// Repair failed but can be retried.
|
|
Retry { message: String },
|
|
/// Repair failed permanently.
|
|
Failed { message: String },
|
|
/// Manual intervention required.
|
|
ManualRequired { message: String },
|
|
}
|
|
|
|
/// Trait for self-repair implementations.
|
|
#[async_trait]
|
|
pub trait SelfRepair: Send + Sync {
|
|
/// Detect stuck jobs.
|
|
async fn detect_stuck_jobs(&self) -> Vec<StuckJob>;
|
|
|
|
/// Attempt to repair a stuck job.
|
|
async fn repair_stuck_job(&self, job: &StuckJob) -> Result<RepairResult, RepairError>;
|
|
|
|
/// Detect broken tools.
|
|
async fn detect_broken_tools(&self) -> Vec<BrokenTool>;
|
|
|
|
/// Attempt to repair a broken tool.
|
|
async fn repair_broken_tool(&self, tool: &BrokenTool) -> Result<RepairResult, RepairError>;
|
|
}
|
|
|
|
/// Default self-repair implementation.
|
|
pub struct DefaultSelfRepair {
|
|
context_manager: Arc<ContextManager>,
|
|
// TODO: use for time-based stuck detection (currently only max_repair_attempts is checked)
|
|
#[allow(dead_code)]
|
|
stuck_threshold: Duration,
|
|
max_repair_attempts: u32,
|
|
store: Option<Arc<dyn Database>>,
|
|
builder: Option<Arc<dyn SoftwareBuilder>>,
|
|
// TODO: use for tool hot-reload after repair
|
|
#[allow(dead_code)]
|
|
tools: Option<Arc<ToolRegistry>>,
|
|
}
|
|
|
|
impl DefaultSelfRepair {
|
|
/// Create a new self-repair instance.
|
|
pub fn new(
|
|
context_manager: Arc<ContextManager>,
|
|
stuck_threshold: Duration,
|
|
max_repair_attempts: u32,
|
|
) -> Self {
|
|
Self {
|
|
context_manager,
|
|
stuck_threshold,
|
|
max_repair_attempts,
|
|
store: None,
|
|
builder: None,
|
|
tools: None,
|
|
}
|
|
}
|
|
|
|
/// Add a Store for tool failure tracking.
|
|
#[allow(dead_code)] // TODO: wire up in main.rs when persistence is needed
|
|
pub(crate) fn with_store(mut self, store: Arc<dyn Database>) -> Self {
|
|
self.store = Some(store);
|
|
self
|
|
}
|
|
|
|
/// Add a Builder and ToolRegistry for automatic tool repair.
|
|
#[allow(dead_code)] // TODO: wire up in main.rs when auto-repair is needed
|
|
pub(crate) fn with_builder(
|
|
mut self,
|
|
builder: Arc<dyn SoftwareBuilder>,
|
|
tools: Arc<ToolRegistry>,
|
|
) -> Self {
|
|
self.builder = Some(builder);
|
|
self.tools = Some(tools);
|
|
self
|
|
}
|
|
}
|
|
|
|
#[async_trait]
|
|
impl SelfRepair for DefaultSelfRepair {
|
|
async fn detect_stuck_jobs(&self) -> Vec<StuckJob> {
|
|
let stuck_ids = self.context_manager.find_stuck_jobs().await;
|
|
let mut stuck_jobs = Vec::new();
|
|
|
|
for job_id in stuck_ids {
|
|
if let Ok(ctx) = self.context_manager.get_context(job_id).await
|
|
&& ctx.state == JobState::Stuck
|
|
{
|
|
let stuck_duration = ctx
|
|
.started_at
|
|
.map(|start| {
|
|
let now = Utc::now();
|
|
let duration = now.signed_duration_since(start);
|
|
Duration::from_secs(duration.num_seconds().max(0) as u64)
|
|
})
|
|
.unwrap_or_default();
|
|
|
|
stuck_jobs.push(StuckJob {
|
|
job_id,
|
|
last_activity: ctx.started_at.unwrap_or(ctx.created_at),
|
|
stuck_duration,
|
|
last_error: None,
|
|
repair_attempts: ctx.repair_attempts,
|
|
});
|
|
}
|
|
}
|
|
|
|
stuck_jobs
|
|
}
|
|
|
|
async fn repair_stuck_job(&self, job: &StuckJob) -> Result<RepairResult, RepairError> {
|
|
// Check if we've exceeded max repair attempts
|
|
if job.repair_attempts >= self.max_repair_attempts {
|
|
return Ok(RepairResult::ManualRequired {
|
|
message: format!(
|
|
"Job {} has exceeded maximum repair attempts ({})",
|
|
job.job_id, self.max_repair_attempts
|
|
),
|
|
});
|
|
}
|
|
|
|
// Try to recover the job
|
|
let result = self
|
|
.context_manager
|
|
.update_context(job.job_id, |ctx| ctx.attempt_recovery())
|
|
.await;
|
|
|
|
match result {
|
|
Ok(Ok(())) => {
|
|
tracing::info!("Successfully recovered job {}", job.job_id);
|
|
Ok(RepairResult::Success {
|
|
message: format!("Job {} recovered and will be retried", job.job_id),
|
|
})
|
|
}
|
|
Ok(Err(e)) => {
|
|
tracing::warn!("Failed to recover job {}: {}", job.job_id, e);
|
|
Ok(RepairResult::Retry {
|
|
message: format!("Recovery attempt failed: {}", e),
|
|
})
|
|
}
|
|
Err(e) => Err(RepairError::Failed {
|
|
target_type: "job".to_string(),
|
|
target_id: job.job_id,
|
|
reason: e.to_string(),
|
|
}),
|
|
}
|
|
}
|
|
|
|
async fn detect_broken_tools(&self) -> Vec<BrokenTool> {
|
|
let Some(ref store) = self.store else {
|
|
return vec![];
|
|
};
|
|
|
|
// Threshold: 5 failures before considering a tool broken
|
|
match store.get_broken_tools(5).await {
|
|
Ok(tools) => {
|
|
if !tools.is_empty() {
|
|
tracing::info!("Detected {} broken tools needing repair", tools.len());
|
|
}
|
|
tools
|
|
}
|
|
Err(e) => {
|
|
tracing::warn!("Failed to detect broken tools: {}", e);
|
|
vec![]
|
|
}
|
|
}
|
|
}
|
|
|
|
async fn repair_broken_tool(&self, tool: &BrokenTool) -> Result<RepairResult, RepairError> {
|
|
let Some(ref builder) = self.builder else {
|
|
return Ok(RepairResult::ManualRequired {
|
|
message: format!("Builder not available for repairing tool '{}'", tool.name),
|
|
});
|
|
};
|
|
|
|
let Some(ref store) = self.store else {
|
|
return Ok(RepairResult::ManualRequired {
|
|
message: "Store not available for tracking repair".to_string(),
|
|
});
|
|
};
|
|
|
|
// Check repair attempt limit
|
|
if tool.repair_attempts >= self.max_repair_attempts {
|
|
return Ok(RepairResult::ManualRequired {
|
|
message: format!(
|
|
"Tool '{}' exceeded max repair attempts ({})",
|
|
tool.name, self.max_repair_attempts
|
|
),
|
|
});
|
|
}
|
|
|
|
tracing::info!(
|
|
"Attempting to repair tool '{}' (attempt {})",
|
|
tool.name,
|
|
tool.repair_attempts + 1
|
|
);
|
|
|
|
// Increment repair attempts
|
|
if let Err(e) = store.increment_repair_attempts(&tool.name).await {
|
|
tracing::warn!("Failed to increment repair attempts: {}", e);
|
|
}
|
|
|
|
// Create BuildRequirement for repair
|
|
let requirement = BuildRequirement {
|
|
name: tool.name.clone(),
|
|
description: format!(
|
|
"Repair broken WASM tool.\n\n\
|
|
Tool name: {}\n\
|
|
Previous error: {}\n\
|
|
Failure count: {}\n\n\
|
|
Analyze the error, fix the implementation, and rebuild.",
|
|
tool.name,
|
|
tool.last_error.as_deref().unwrap_or("Unknown error"),
|
|
tool.failure_count
|
|
),
|
|
software_type: SoftwareType::WasmTool,
|
|
language: Language::Rust,
|
|
input_spec: None,
|
|
output_spec: None,
|
|
dependencies: vec![],
|
|
capabilities: vec!["http".to_string(), "workspace".to_string()],
|
|
};
|
|
|
|
// Attempt to build/repair
|
|
match builder.build(&requirement).await {
|
|
Ok(result) if result.success => {
|
|
tracing::info!(
|
|
"Successfully rebuilt tool '{}' after {} iterations",
|
|
tool.name,
|
|
result.iterations
|
|
);
|
|
|
|
// Mark as repaired in database
|
|
if let Err(e) = store.mark_tool_repaired(&tool.name).await {
|
|
tracing::warn!("Failed to mark tool as repaired: {}", e);
|
|
}
|
|
|
|
// Log if the tool was auto-registered
|
|
if result.registered {
|
|
tracing::info!("Repaired tool '{}' auto-registered", tool.name);
|
|
}
|
|
|
|
Ok(RepairResult::Success {
|
|
message: format!(
|
|
"Tool '{}' repaired successfully after {} iterations",
|
|
tool.name, result.iterations
|
|
),
|
|
})
|
|
}
|
|
Ok(result) => {
|
|
// Build completed but failed
|
|
tracing::warn!(
|
|
"Repair build for '{}' completed but failed: {:?}",
|
|
tool.name,
|
|
result.error
|
|
);
|
|
Ok(RepairResult::Retry {
|
|
message: format!(
|
|
"Repair attempt {} for '{}' failed: {}",
|
|
tool.repair_attempts + 1,
|
|
tool.name,
|
|
result.error.unwrap_or_else(|| "Unknown error".to_string())
|
|
),
|
|
})
|
|
}
|
|
Err(e) => {
|
|
tracing::error!("Repair build for '{}' errored: {}", tool.name, e);
|
|
Ok(RepairResult::Retry {
|
|
message: format!("Repair build error: {}", e),
|
|
})
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
/// Background repair task that periodically checks for and repairs issues.
|
|
pub struct RepairTask {
|
|
repair: Arc<dyn SelfRepair>,
|
|
check_interval: Duration,
|
|
}
|
|
|
|
impl RepairTask {
|
|
/// Create a new repair task.
|
|
pub fn new(repair: Arc<dyn SelfRepair>, check_interval: Duration) -> Self {
|
|
Self {
|
|
repair,
|
|
check_interval,
|
|
}
|
|
}
|
|
|
|
/// Run the repair task.
|
|
pub async fn run(&self) {
|
|
loop {
|
|
tokio::time::sleep(self.check_interval).await;
|
|
|
|
// Check for stuck jobs
|
|
let stuck_jobs = self.repair.detect_stuck_jobs().await;
|
|
for job in stuck_jobs {
|
|
tracing::info!("Attempting to repair stuck job {}", job.job_id);
|
|
match self.repair.repair_stuck_job(&job).await {
|
|
Ok(RepairResult::Success { message }) => {
|
|
tracing::info!("Repair succeeded: {}", message);
|
|
}
|
|
Ok(RepairResult::Retry { message }) => {
|
|
tracing::warn!("Repair needs retry: {}", message);
|
|
}
|
|
Ok(RepairResult::Failed { message }) => {
|
|
tracing::error!("Repair failed: {}", message);
|
|
}
|
|
Ok(RepairResult::ManualRequired { message }) => {
|
|
tracing::warn!("Manual intervention needed: {}", message);
|
|
}
|
|
Err(e) => {
|
|
tracing::error!("Repair error: {}", e);
|
|
}
|
|
}
|
|
}
|
|
|
|
// Check for broken tools
|
|
let broken_tools = self.repair.detect_broken_tools().await;
|
|
for tool in broken_tools {
|
|
tracing::info!("Attempting to repair broken tool: {}", tool.name);
|
|
match self.repair.repair_broken_tool(&tool).await {
|
|
Ok(result) => {
|
|
tracing::info!("Tool repair result: {:?}", result);
|
|
}
|
|
Err(e) => {
|
|
tracing::error!("Tool repair error: {}", e);
|
|
}
|
|
}
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
#[test]
|
|
fn test_repair_result_variants() {
|
|
let success = RepairResult::Success {
|
|
message: "OK".to_string(),
|
|
};
|
|
assert!(matches!(success, RepairResult::Success { .. }));
|
|
|
|
let manual = RepairResult::ManualRequired {
|
|
message: "Help needed".to_string(),
|
|
};
|
|
assert!(matches!(manual, RepairResult::ManualRequired { .. }));
|
|
}
|
|
}
|