merge: resolve conflicts for PR #800 and #822 into staging (#881)

* fix(ci): secrets can't be used in step if conditions [skip-regression-check] (#787) GitHub Actions step-level `if:` doesn't have access to `secrets` context. Replace `if: secrets.X != ''` with `continue-on-error: true` and let the Set token step handle the fallback. Co-authored-by: Claude Sonnet 4.6 <[email protected]> * fix(ci): clean up staging pipeline — remove hacks, skip redundant checks [skip-regression-check] (#794) - Remove continue-on-error from staging-ci.yml app token steps (secrets are configured) - Skip test.yml and code_style.yml on PRs targeting staging (staging-ci.yml already runs tests before promoting, promotion PR gets full CI on main) - Allow ironclaw-ci[bot] in Claude Code review for bot-created promotion PRs Co-authored-by: Claude Opus 4.6 <[email protected]> * fix(ci): run fmt + clippy on staging PRs, skip Windows clippy [skip-regression-check] (#802) - Remove branches:[main] filter from code_style.yml so it runs on all PRs - Gate clippy-windows with `if: github.base_ref == 'main'` (skip on staging PRs) - Update rollup job to allow skipped clippy-windows - Simplify claude-review.yml to only trigger on labeled event (avoids duplicate runs) Co-authored-by: Claude Opus 4.6 <[email protected]> * feat: persist user_id in save_job and expose job_id on routine runs (#709) * feat: persist worker events to DB and fix activity tab rendering In-process Worker (used by Scheduler::dispatch_job) now persists events via save_job_event at key execution points: plan creation, LLM responses, tool_use, tool_result, and job completion/failure/stuck. Event data shapes match the container worker format so the gateway activity tab renders them correctly. Frontend: tool_result errors now show a red X icon with danger styling instead of a silent empty output. The result event falls back to the error field when message is absent. Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: wire RoutineEngine into gateway for direct manual trigger firing Replace the message-channel hack in routines_trigger_handler with a direct call to RoutineEngine::fire_manual(), ensuring FullJob routines dispatch correctly when triggered from the web UI. Inject the engine into GatewayState from Agent::run after construction. Also persists user_id in save_job for both PG and libSQL backends, removes the source='sandbox' filter so all jobs are visible, and exposes job_id on RoutineRunInfo for the frontend job link. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: remove stale gateway_state argument from Agent::new test call sites The gateway_state parameter was removed from Agent::new during rebase (replaced by post-construction set_routine_engine_slot), but three test call sites still passed the extra None argument. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address PR review — restore sandbox source filter, remove blank lines - Revert removal of `source = 'sandbox'` filter in all SandboxStore queries (8 sites across PG and libSQL). Sandbox-specific APIs should stay scoped to sandbox jobs; unified job listing for the Jobs tab should use a separate query path. - Remove extra blank lines in agent_loop.rs and worker.rs that caused formatting CI failure. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address review — regenerate Cargo.lock, add user_id regression test - Regenerate Cargo.lock from main's lockfile to eliminate dependency version downgrades (anyhow, syn, etc.) that were churn from rebase. - Add regression test verifying user_id round-trips through save_job and get_job in the libSQL backend. Co-Authored-By: Claude Opus 4.6 <[email protected]> * style: remove trailing blank line in libsql jobs.rs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <[email protected]> * test: add Postgres-side regression test for user_id persistence in save_job Mirrors the existing libSQL test (test_save_job_persists_user_id) for the Postgres backend. Gated behind #[cfg(feature = "postgres")] + #[ignore] since it requires a running PostgreSQL instance (integration tier). Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Claude Opus 4.6 <[email protected]> * refactor: unify three agentic loops into single AgenticLoop engine (#654) Replace three independent copy-pasted agentic loops (dispatcher, worker, container runtime) with a single shared engine in `agentic_loop.rs` that all consumers customize via the `LoopDelegate` trait. Phase 1 — Shared engine (`src/agent/agentic_loop.rs`, 205 lines): - `run_agentic_loop()` owns the core LLM → tool exec → repeat cycle - `LoopDelegate` trait (Send + Sync, &dyn dispatch) with 6 hook points - Tool intent nudge logic consolidated (was duplicated in 3 files) - Iteration limit + force-text behavior preserved Phase 2 — Three delegate implementations: - `ChatDelegate` (dispatcher.rs): 3-phase approval flow, hooks, cost guard, context compaction, skill attenuation, interruption - `JobDelegate` (worker/job.rs): planning pre-loop phase, parallel JoinSet exec, mark_completed/stuck/failed, SSE streaming, self-repair - `ContainerDelegate` (worker/container.rs): sequential tool exec, HTTP-proxied LLM, container-safe tools, credential injection Phase 3 — File moves and cleanup: - Delete `src/agent/worker.rs` — job logic moved to `src/worker/job.rs` - Rename `src/worker/runtime.rs` → `src/worker/container.rs` - Re-export `Worker`/`WorkerDeps` from `crate::worker` in `agent/mod.rs` - Update `scheduler.rs` imports to new worker location Shared helpers (`src/tools/execute.rs`): - `execute_tool_with_safety()` replaces 4 copies of validate → timeout → execute → serialize - `process_tool_result()` replaces 3 copies of sanitize → wrap → ChatMessage (also used by thread_ops.rs approval resume paths) Net result: -2,408 lines, zero duplicated loop logic, single code path for tool intent nudge and completion detection. Closes #654 Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address review feedback from Copilot 1. scheduler.rs: Replace `unwrap_or` fallback with proper error propagation when parsing tool output JSON — surfaces bugs instead of silently changing the output type. 2. worker/job.rs: Drop MutexGuard before the cancellation `.await` in `check_signals()` to avoid holding a lock across an async I/O call (prevents `await_holding_lock` lint). 3. worker/job.rs: Restore consecutive rate-limit counter (MAX_CONSECUTIVE_RATE_LIMITS = 10) so sustained rate limiting marks the job stuck with "Persistent rate limiting" instead of silently burning through max_iterations. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: incorporate staging changes — token budget tracking + mark_failed Merge staging's changes into the refactored JobDelegate: - Add token budget tracking in call_llm (update_context/add_tokens) - mark_stuck → mark_failed for iteration cap and rate-limit exhaustion (aligns with staging's #788 fix) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address zmanian's PR review — eliminate type erasure, clean up Address all 6 review points from zmanian on PR #800: 1. Replace LoopOutcome::Custom(Box<dyn Any>) with typed LoopOutcome::NeedApproval(Box<PendingApproval>) — eliminates type erasure and downcast, resolves clippy large_enum_variant. 2. Remove dead max_tool_iterations field from ChatDelegate struct. 3. Add on_tool_intent_nudge() hook to LoopDelegate trait with implementations in Job and Container delegates for observability. 4. Fix SSE events in job worker to emit raw sanitized content instead of XML-wrapped <tool_output> tags. 5. Remove 4 duplicate completion tests from job.rs that were already covered by the shared util module. 6. Avoid logging full tool results — use result_size_bytes in debug logs (execute.rs, job.rs). Also updates path references in CLAUDE.md, COVERAGE_PLAN.md, and add-sse-event.md command. Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat(doctor): expand diagnostics from 7 to 16 health checks * test: add unit tests for agentic_loop and execute shared modules Add 16 tests covering the two new critical shared modules: agentic_loop.rs (10 tests): - Text response exits loop immediately - Tool call → text response continuation - LoopSignal::Stop exits before LLM call - LoopSignal::InjectMessage adds user message to context - Max iterations terminates with LoopOutcome::MaxIterations - Tool intent nudge fires twice then caps - before_llm_call early exit bypasses LLM - truncate_for_preview: short string, long string, multibyte safety execute.rs (6 tests): - execute_tool_with_safety success path - Missing tool returns ToolError::NotFound - Tool execution failure propagates - Per-tool timeout enforcement (50ms) - process_tool_result XML wrapping on success - process_tool_result error formatting All 2,777 unit tests pass, 0 clippy warnings. Co-Authored-By: Claude Opus 4.6 <[email protected]> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address code review — 9 issues across agentic loop, job worker, container CRITICAL fixes: - Rate-limit exhaustion now returns Err(LlmError::RateLimited) instead of Ok(Text("")), stopping the loop immediately with no ghost iteration. Below-threshold retries still use Text("") with an explicit empty-string guard in handle_text_response to skip injection. - check_signals drains the entire message channel before returning, prioritizing Stop over UserMessage. Previously returned early on first UserMessage, silently dropping any queued Stop or additional messages. - check_signals now detects all non-progressing job states (Cancelled, Failed, Stuck, Completed, Submitted, Accepted) instead of only Cancelled and Failed. HIGH fixes: - Error path in process_tool_result_job applies truncate_for_preview to bound error strings in SSE/DB events (was unbounded). - Document Send+Sync lifetime constraint on LoopDelegate trait. - Test mock before_llm_call refactored from double-lock to single lock acquisition, eliminating deadlock risk on refactor. MEDIUM fixes: - CompletionReport includes actual iteration count via shared Arc<Mutex<u32>> tracker (was hardcoded 0). - process_tool_result_job return type changed from Result<bool> to Result<()> — the bool was always false (dead API). - Deduplicate truncate in container.rs; now uses truncate_for_preview from agentic_loop. Verified: 0 clippy warnings, 2781 tests pass, cargo fmt clean. Co-Authored-By: Claude Opus 4.6 <[email protected]> --------- Co-authored-by: Henry Park <[email protected]> Co-authored-by: Claude Sonnet 4.6 <[email protected]> Co-authored-by: Illia Polosukhin <[email protected]> Co-authored-by: Umesh Kumar Singh <[email protected]> Co-authored-by: reidliu41 <[email protected]>
2026-08-27 08:00:17 +00:00 · 2026-03-10 11:19:23 -07:00
parent ebb22094a5
commit 88f4894a18
18 changed files with 3253 additions and 1904 deletions
@@ -0,0 +1,391 @@
+//! Shared tool execution pipeline.
+//!
+//! Provides a single implementation of the validate → timeout → execute → serialize
+//! pipeline used by all agentic loop consumers (chat, job, container) and the
+//! scheduler's subtask execution.
+
+use crate::context::JobContext;
+use crate::error::Error;
+use crate::llm::ChatMessage;
+use crate::safety::SafetyLayer;
+use crate::tools::{ToolRegistry, redact_params};
+
+/// Execute a tool with safety checks: lookup → validate → timeout → execute → serialize.
+///
+/// This is the single canonical implementation of tool execution. All consumers
+/// (chat dispatcher, job worker, container runtime, scheduler subtasks) use this
+/// function instead of maintaining their own copies.
+pub async fn execute_tool_with_safety(
+    tools: &ToolRegistry,
+    safety: &SafetyLayer,
+    tool_name: &str,
+    params: &serde_json::Value,
+    job_ctx: &JobContext,
+) -> Result<String, Error> {
+    let tool = tools
+        .get(tool_name)
+        .await
+        .ok_or_else(|| crate::error::ToolError::NotFound {
+            name: tool_name.to_string(),
+        })?;
+
+    // Validate tool parameters
+    let validation = safety.validator().validate_tool_params(params);
+    if !validation.is_valid {
+        let details = validation
+            .errors
+            .iter()
+            .map(|e| format!("{}: {}", e.field, e.message))
+            .collect::<Vec<_>>()
+            .join("; ");
+        return Err(crate::error::ToolError::InvalidParameters {
+            name: tool_name.to_string(),
+            reason: format!("Invalid tool parameters: {}", details),
+        }
+        .into());
+    }
+
+    let safe_params = redact_params(params, tool.sensitive_params());
+    tracing::debug!(
+        tool = %tool_name,
+        params = %safe_params,
+        "Tool call started"
+    );
+
+    // Execute with per-tool timeout
+    let timeout = tool.execution_timeout();
+    let start = std::time::Instant::now();
+    let result = tokio::time::timeout(timeout, async {
+        tool.execute(params.clone(), job_ctx).await
+    })
+    .await;
+    let elapsed = start.elapsed();
+
+    match &result {
+        Ok(Ok(output)) => {
+            let result_size = serde_json::to_string(&output.result)
+                .map(|s| s.len())
+                .unwrap_or(0);
+            tracing::debug!(
+                tool = %tool_name,
+                elapsed_ms = elapsed.as_millis() as u64,
+                result_size_bytes = result_size,
+                "Tool call succeeded"
+            );
+        }
+        Ok(Err(e)) => {
+            tracing::debug!(
+                tool = %tool_name,
+                elapsed_ms = elapsed.as_millis() as u64,
+                error = %e,
+                "Tool call failed"
+            );
+        }
+        Err(_) => {
+            tracing::debug!(
+                tool = %tool_name,
+                elapsed_ms = elapsed.as_millis() as u64,
+                timeout_secs = timeout.as_secs(),
+                "Tool call timed out"
+            );
+        }
+    }
+
+    let result = result
+        .map_err(|_| crate::error::ToolError::Timeout {
+            name: tool_name.to_string(),
+            timeout,
+        })?
+        .map_err(|e| crate::error::ToolError::ExecutionFailed {
+            name: tool_name.to_string(),
+            reason: e.to_string(),
+        })?;
+
+    serde_json::to_string_pretty(&result.result).map_err(|e| {
+        crate::error::ToolError::ExecutionFailed {
+            name: tool_name.to_string(),
+            reason: format!("Failed to serialize result: {}", e),
+        }
+        .into()
+    })
+}
+
+/// Process a tool result into a `ChatMessage::tool_result` with safety sanitization.
+///
+/// On success: sanitize → wrap → ChatMessage::tool_result.
+/// On error: format error → ChatMessage::tool_result.
+///
+/// Returns the content string and the ChatMessage.
+pub fn process_tool_result(
+    safety: &SafetyLayer,
+    tool_name: &str,
+    tool_call_id: &str,
+    result: &Result<String, impl std::fmt::Display>,
+) -> (String, ChatMessage) {
+    let content = match result {
+        Ok(output) => {
+            let sanitized = safety.sanitize_tool_output(tool_name, output);
+            safety.wrap_for_llm(tool_name, &sanitized.content, sanitized.was_modified)
+        }
+        Err(e) => format!("Error: {}", e),
+    };
+    let message = ChatMessage::tool_result(tool_call_id, tool_name, content.clone());
+    (content, message)
+}
+
+/// Execute a tool with safety checks, returning a string error (for container runtime).
+///
+/// This is a thin wrapper around `execute_tool_with_safety` that converts
+/// `Error` to `String` for the container runtime's simpler error model.
+pub async fn execute_tool_simple(
+    tools: &ToolRegistry,
+    safety: &SafetyLayer,
+    tool_name: &str,
+    params: &serde_json::Value,
+    job_ctx: &JobContext,
+) -> Result<String, String> {
+    execute_tool_with_safety(tools, safety, tool_name, params, job_ctx)
+        .await
+        .map_err(|e| e.to_string())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::tools::tool::{Tool, ToolError, ToolOutput};
+    use std::sync::Arc;
+    use std::time::Duration;
+
+    struct EchoTool;
+
+    #[async_trait::async_trait]
+    impl Tool for EchoTool {
+        fn name(&self) -> &str {
+            "echo"
+        }
+        fn description(&self) -> &str {
+            "Echoes input"
+        }
+        fn parameters_schema(&self) -> serde_json::Value {
+            serde_json::json!({"type": "object", "properties": {}})
+        }
+        async fn execute(
+            &self,
+            params: serde_json::Value,
+            _ctx: &JobContext,
+        ) -> Result<ToolOutput, ToolError> {
+            Ok(ToolOutput::success(params, Duration::default()))
+        }
+        fn requires_sanitization(&self) -> bool {
+            false
+        }
+    }
+
+    struct FailTool;
+
+    #[async_trait::async_trait]
+    impl Tool for FailTool {
+        fn name(&self) -> &str {
+            "fail_tool"
+        }
+        fn description(&self) -> &str {
+            "Always fails"
+        }
+        fn parameters_schema(&self) -> serde_json::Value {
+            serde_json::json!({"type": "object", "properties": {}})
+        }
+        async fn execute(
+            &self,
+            _: serde_json::Value,
+            _: &JobContext,
+        ) -> Result<ToolOutput, ToolError> {
+            Err(ToolError::ExecutionFailed(
+                "intentional failure".to_string(),
+            ))
+        }
+        fn requires_sanitization(&self) -> bool {
+            false
+        }
+    }
+
+    struct SlowTool;
+
+    #[async_trait::async_trait]
+    impl Tool for SlowTool {
+        fn name(&self) -> &str {
+            "slow_tool"
+        }
+        fn description(&self) -> &str {
+            "Sleeps forever"
+        }
+        fn parameters_schema(&self) -> serde_json::Value {
+            serde_json::json!({"type": "object", "properties": {}})
+        }
+        async fn execute(
+            &self,
+            _: serde_json::Value,
+            _: &JobContext,
+        ) -> Result<ToolOutput, ToolError> {
+            tokio::time::sleep(Duration::from_secs(60)).await;
+            unreachable!()
+        }
+        fn execution_timeout(&self) -> Duration {
+            Duration::from_millis(50)
+        }
+        fn requires_sanitization(&self) -> bool {
+            false
+        }
+    }
+
+    fn test_safety() -> SafetyLayer {
+        SafetyLayer::new(&crate::config::SafetyConfig {
+            max_output_length: 100_000,
+            injection_check_enabled: false,
+        })
+    }
+
+    fn test_job_ctx() -> JobContext {
+        JobContext::default()
+    }
+
+    async fn registry_with(tools: Vec<Arc<dyn Tool>>) -> ToolRegistry {
+        let registry = ToolRegistry::new();
+        for tool in tools {
+            registry.register(tool).await;
+        }
+        registry
+    }
+
+    #[tokio::test]
+    async fn test_execute_success() {
+        let registry = registry_with(vec![Arc::new(EchoTool)]).await;
+        let safety = test_safety();
+        let params = serde_json::json!({"message": "hello"});
+
+        let result =
+            execute_tool_with_safety(&registry, &safety, "echo", &params, &test_job_ctx()).await;
+
+        assert!(result.is_ok(), "Echo tool should succeed");
+        let output = result.unwrap();
+        assert!(
+            output.contains("hello"),
+            "Output should contain the echoed input"
+        );
+    }
+
+    #[tokio::test]
+    async fn test_execute_missing_tool() {
+        let registry = registry_with(vec![]).await;
+        let safety = test_safety();
+
+        let result = execute_tool_with_safety(
+            &registry,
+            &safety,
+            "nonexistent",
+            &serde_json::json!({}),
+            &test_job_ctx(),
+        )
+        .await;
+
+        assert!(result.is_err(), "Missing tool should return error");
+        let err = result.unwrap_err().to_string();
+        assert!(
+            err.contains("nonexistent") || err.contains("not found"),
+            "Error should mention the tool: {}",
+            err
+        );
+    }
+
+    #[tokio::test]
+    async fn test_execute_tool_failure() {
+        let registry = registry_with(vec![Arc::new(FailTool)]).await;
+        let safety = test_safety();
+
+        let result = execute_tool_with_safety(
+            &registry,
+            &safety,
+            "fail_tool",
+            &serde_json::json!({}),
+            &test_job_ctx(),
+        )
+        .await;
+
+        assert!(result.is_err(), "FailTool should return error");
+        let err = result.unwrap_err().to_string();
+        assert!(
+            err.contains("intentional failure"),
+            "Error should contain the failure reason: {}",
+            err
+        );
+    }
+
+    #[tokio::test]
+    async fn test_execute_tool_timeout() {
+        let registry = registry_with(vec![Arc::new(SlowTool)]).await;
+        let safety = test_safety();
+
+        let start = std::time::Instant::now();
+        let result = execute_tool_with_safety(
+            &registry,
+            &safety,
+            "slow_tool",
+            &serde_json::json!({}),
+            &test_job_ctx(),
+        )
+        .await;
+        let elapsed = start.elapsed();
+
+        assert!(result.is_err(), "SlowTool should timeout");
+        let err = result.unwrap_err().to_string();
+        assert!(
+            err.to_lowercase().contains("timeout") || err.to_lowercase().contains("timed out"),
+            "Error should mention timeout: {}",
+            err
+        );
+        assert!(
+            elapsed < Duration::from_secs(1),
+            "Should timeout quickly, not wait 60s"
+        );
+    }
+
+    #[test]
+    fn test_process_tool_result_success() {
+        let safety = test_safety();
+        let result: Result<String, String> = Ok("tool output data".to_string());
+
+        let (content, message) = process_tool_result(&safety, "echo", "call_1", &result);
+
+        assert!(
+            content.contains("tool_output"),
+            "Content should be XML-wrapped: {}",
+            content
+        );
+        assert!(
+            content.contains("tool output data"),
+            "Content should contain the output: {}",
+            content
+        );
+        assert_eq!(message.role, crate::llm::Role::Tool);
+        assert_eq!(message.name.as_deref(), Some("echo"));
+    }
+
+    #[test]
+    fn test_process_tool_result_error() {
+        let safety = test_safety();
+        let result: Result<String, String> = Err("something went wrong".to_string());
+
+        let (content, message) = process_tool_result(&safety, "echo", "call_1", &result);
+
+        assert!(
+            content.contains("Error:"),
+            "Error content should start with 'Error:': {}",
+            content
+        );
+        assert!(
+            content.contains("something went wrong"),
+            "Error content should contain the message: {}",
+            content
+        );
+        assert_eq!(message.role, crate::llm::Role::Tool);
+    }
+}
@@ -9,6 +9,7 @@

 pub mod builder;
 pub mod builtin;
+pub mod execute;
 pub mod mcp;
 pub mod rate_limiter;
 pub mod schema_validator;