fix: persist turns after approval and add agent-level tests (nearai#250)

* fix: persist turns after approval and add agent-level tests

Port relevant changes from PR nearai#112 that were not carried over to nearai#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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: 2 people

src/agent/compaction.rs

@@ -105,7 +105,16 @@ impl ContextCompactor {
 
         // Write to workspace if available
         let summary_written = if let Some(ws) = workspace {
-            self.write_summary_to_workspace(ws, &summary).await.is_ok()
+            match self.write_summary_to_workspace(ws, &summary).await {
+                Ok(()) => true,
+                Err(e) => {
+                    tracing::warn!(
+                        "Compaction summary write failed (turns will still be truncated): {}",
+                        e
+                    );
+                    false
+                }
+            }
         } else {
             false
         };
@@ -157,7 +166,16 @@ impl ContextCompactor {
         let content = format_turns_for_storage(old_turns);
 
         // Write to workspace
-        let written = self.write_context_to_workspace(ws, &content).await.is_ok();
+        let written = match self.write_context_to_workspace(ws, &content).await {
+            Ok(()) => true,
+            Err(e) => {
+                tracing::warn!(
+                    "Compaction context write failed (turns will still be truncated): {}",
+                    e
+                );
+                false
+            }
+        };
 
         // Truncate
         thread.truncate_turns(keep_recent);

src/agent/dispatcher.rs

@@ -402,7 +402,7 @@ impl Agent {
                         // and short-circuit: return the instructions directly so
                         // the LLM doesn't get a chance to hallucinate tool calls.
                         if let Some((ext_name, instructions)) =
-                            detect_auth_awaiting(&tc.name, &tool_result)
+                            check_auth_required(&tc.name, &tool_result)
                         {
                             let auth_data = parse_auth_result(&tool_result);
                             {
@@ -581,7 +581,7 @@ pub(super) fn parse_auth_result(result: &Result<String, Error>) -> ParsedAuthDat
 ///
 /// Returns `Some((extension_name, instructions))` if the tool result contains
 /// `awaiting_token: true`, meaning the thread should enter auth mode.
-pub(super) fn detect_auth_awaiting(
+pub(super) fn check_auth_required(
     tool_name: &str,
     result: &Result<String, Error>,
 ) -> Option<(String, String)> {
@@ -604,9 +604,213 @@ pub(super) fn detect_auth_awaiting(
 
 #[cfg(test)]
 mod tests {
+    use std::sync::Arc;
+    use std::time::Duration;
+
+    use async_trait::async_trait;
+    use rust_decimal::Decimal;
+
+    use crate::agent::agent_loop::{Agent, AgentDeps};
+    use crate::agent::cost_guard::{CostGuard, CostGuardConfig};
+    use crate::agent::session::Session;
+    use crate::channels::ChannelManager;
+    use crate::config::{AgentConfig, SafetyConfig, SkillsConfig};
+    use crate::context::ContextManager;
     use crate::error::Error;
+    use crate::hooks::HookRegistry;
+    use crate::llm::{
+        CompletionRequest, CompletionResponse, FinishReason, LlmProvider, ToolCall,
+        ToolCompletionRequest, ToolCompletionResponse,
+    };
+    use crate::safety::SafetyLayer;
+    use crate::tools::ToolRegistry;
+
+    use super::check_auth_required;
+
+    /// Minimal LLM provider for unit tests that always returns a static response.
+    struct StaticLlmProvider;
+
+    #[async_trait]
+    impl LlmProvider for StaticLlmProvider {
+        fn model_name(&self) -> &str {
+            "static-mock"
+        }
+
+        fn cost_per_token(&self) -> (Decimal, Decimal) {
+            (Decimal::ZERO, Decimal::ZERO)
+        }
+
+        async fn complete(
+            &self,
+            _request: CompletionRequest,
+        ) -> Result<CompletionResponse, crate::error::LlmError> {
+            Ok(CompletionResponse {
+                content: "ok".to_string(),
+                input_tokens: 0,
+                output_tokens: 0,
+                finish_reason: FinishReason::Stop,
+                response_id: None,
+            })
+        }
+
+        async fn complete_with_tools(
+            &self,
+            _request: ToolCompletionRequest,
+        ) -> Result<ToolCompletionResponse, crate::error::LlmError> {
+            Ok(ToolCompletionResponse {
+                content: Some("ok".to_string()),
+                tool_calls: Vec::new(),
+                input_tokens: 0,
+                output_tokens: 0,
+                finish_reason: FinishReason::Stop,
+                response_id: None,
+            })
+        }
+    }
+
+    /// Build a minimal `Agent` for unit testing (no DB, no workspace, no extensions).
+    fn make_test_agent() -> Agent {
+        let deps = AgentDeps {
+            store: None,
+            llm: Arc::new(StaticLlmProvider),
+            cheap_llm: None,
+            safety: Arc::new(SafetyLayer::new(&SafetyConfig {
+                max_output_length: 100_000,
+                injection_check_enabled: true,
+            })),
+            tools: Arc::new(ToolRegistry::new()),
+            workspace: None,
+            extension_manager: None,
+            skill_registry: None,
+            skills_config: SkillsConfig::default(),
+            hooks: Arc::new(HookRegistry::new()),
+            cost_guard: Arc::new(CostGuard::new(CostGuardConfig::default())),
+        };
+
+        Agent::new(
+            AgentConfig {
+                name: "test-agent".to_string(),
+                max_parallel_jobs: 1,
+                job_timeout: Duration::from_secs(60),
+                stuck_threshold: Duration::from_secs(60),
+                repair_check_interval: Duration::from_secs(30),
+                max_repair_attempts: 1,
+                use_planning: false,
+                session_idle_timeout: Duration::from_secs(300),
+                allow_local_tools: false,
+                max_cost_per_day_cents: None,
+                max_actions_per_hour: None,
+            },
+            deps,
+            ChannelManager::new(),
+            None,
+            None,
+            None,
+            Some(Arc::new(ContextManager::new(1))),
+            None,
+        )
+    }
+
+    #[test]
+    fn test_make_test_agent_succeeds() {
+        // Verify that a test agent can be constructed without panicking.
+        let _agent = make_test_agent();
+    }
+
+    #[test]
+    fn test_auto_approved_tool_is_respected() {
+        let _agent = make_test_agent();
+        let mut session = Session::new("user-1");
+        session.auto_approve_tool("http");
+
+        // A non-shell tool that is auto-approved should be approved.
+        assert!(session.is_tool_auto_approved("http"));
+        // A tool that hasn't been auto-approved should not be.
+        assert!(!session.is_tool_auto_approved("shell"));
+    }
+
+    #[test]
+    fn test_shell_destructive_command_requires_approval_for() {
+        // ShellTool::requires_approval_for should detect destructive commands.
+        // This exercises the same code path used inline in run_agentic_loop.
+        use crate::tools::builtin::shell::requires_explicit_approval;
+
+        let destructive_cmds = [
+            "rm -rf /tmp/test",
+            "git push --force origin main",
+            "git reset --hard HEAD~5",
+        ];
+        for cmd in &destructive_cmds {
+            assert!(
+                requires_explicit_approval(cmd),
+                "'{}' should require explicit approval",
+                cmd
+            );
+        }
+
+        let safe_cmds = ["git status", "cargo build", "ls -la"];
+        for cmd in &safe_cmds {
+            assert!(
+                !requires_explicit_approval(cmd),
+                "'{}' should not require explicit approval",
+                cmd
+            );
+        }
+    }
 
-    use super::detect_auth_awaiting;
+    #[test]
+    fn test_pending_approval_serialization_backcompat_without_deferred_calls() {
+        // PendingApproval from before the deferred_tool_calls field was added
+        // should deserialize with an empty vec (via #[serde(default)]).
+        let json = serde_json::json!({
+            "request_id": uuid::Uuid::new_v4(),
+            "tool_name": "http",
+            "parameters": {"url": "https://example.com", "method": "GET"},
+            "description": "Make HTTP request",
+            "tool_call_id": "call_123",
+            "context_messages": [{"role": "user", "content": "go"}]
+        })
+        .to_string();
+
+        let parsed: crate::agent::session::PendingApproval =
+            serde_json::from_str(&json).expect("should deserialize without deferred_tool_calls");
+
+        assert!(parsed.deferred_tool_calls.is_empty());
+        assert_eq!(parsed.tool_name, "http");
+        assert_eq!(parsed.tool_call_id, "call_123");
+    }
+
+    #[test]
+    fn test_pending_approval_serialization_roundtrip_with_deferred_calls() {
+        let pending = crate::agent::session::PendingApproval {
+            request_id: uuid::Uuid::new_v4(),
+            tool_name: "shell".to_string(),
+            parameters: serde_json::json!({"command": "echo hi"}),
+            description: "Run shell command".to_string(),
+            tool_call_id: "call_1".to_string(),
+            context_messages: vec![],
+            deferred_tool_calls: vec![
+                ToolCall {
+                    id: "call_2".to_string(),
+                    name: "http".to_string(),
+                    arguments: serde_json::json!({"url": "https://example.com"}),
+                },
+                ToolCall {
+                    id: "call_3".to_string(),
+                    name: "echo".to_string(),
+                    arguments: serde_json::json!({"message": "done"}),
+                },
+            ],
+        };
+
+        let json = serde_json::to_string(&pending).expect("serialize");
+        let parsed: crate::agent::session::PendingApproval =
+            serde_json::from_str(&json).expect("deserialize");
+
+        assert_eq!(parsed.deferred_tool_calls.len(), 2);
+        assert_eq!(parsed.deferred_tool_calls[0].name, "http");
+        assert_eq!(parsed.deferred_tool_calls[1].name, "echo");
+    }
 
     #[test]
     fn test_detect_auth_awaiting_positive() {
@@ -619,7 +823,7 @@ mod tests {
         })
         .to_string());
 
-        let detected = detect_auth_awaiting("tool_auth", &result);
+        let detected = check_auth_required("tool_auth", &result);
         assert!(detected.is_some());
         let (name, instructions) = detected.unwrap();
         assert_eq!(name, "telegram");
@@ -636,7 +840,7 @@ mod tests {
         })
         .to_string());
 
-        assert!(detect_auth_awaiting("tool_auth", &result).is_none());
+        assert!(check_auth_required("tool_auth", &result).is_none());
     }
 
     #[test]
@@ -647,14 +851,14 @@ mod tests {
         })
         .to_string());
 
-        assert!(detect_auth_awaiting("tool_list", &result).is_none());
+        assert!(check_auth_required("tool_list", &result).is_none());
     }
 
     #[test]
     fn test_detect_auth_awaiting_error_result() {
         let result: Result<String, Error> =
             Err(crate::error::ToolError::NotFound { name: "x".into() }.into());
-        assert!(detect_auth_awaiting("tool_auth", &result).is_none());
+        assert!(check_auth_required("tool_auth", &result).is_none());
     }
 
     #[test]
@@ -666,7 +870,7 @@ mod tests {
         })
         .to_string());
 
-        let (_, instructions) = detect_auth_awaiting("tool_auth", &result).unwrap();
+        let (_, instructions) = check_auth_required("tool_auth", &result).unwrap();
         assert_eq!(instructions, "Please provide your API token/key.");
     }
 
@@ -681,7 +885,7 @@ mod tests {
         })
         .to_string());
 
-        let detected = detect_auth_awaiting("tool_activate", &result);
+        let detected = check_auth_required("tool_activate", &result);
         assert!(detected.is_some());
         let (name, instructions) = detected.unwrap();
         assert_eq!(name, "slack");
@@ -697,6 +901,6 @@ mod tests {
         })
         .to_string());
 
-        assert!(detect_auth_awaiting("tool_activate", &result).is_none());
+        assert!(check_auth_required("tool_activate", &result).is_none());
     }
 }

src/agent/mod.rs

@@ -44,6 +44,6 @@ pub use self_repair::{BrokenTool, RepairResult, RepairTask, SelfRepair, StuckJob
 pub use session::{PendingApproval, PendingAuth, Session, Thread, ThreadState, Turn, TurnState};
 pub use session_manager::SessionManager;
 pub use submission::{Submission, SubmissionParser, SubmissionResult};
-pub use task::{Task, TaskContext, TaskHandler, TaskOutput, TaskStatus};
+pub use task::{Task, TaskContext, TaskHandler, TaskOutput};
 pub use undo::{Checkpoint, UndoManager};
 pub use worker::{Worker, WorkerDeps};