From 577e26eff4960cd31932e22ef017ae1599f3fbc6 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 9 Mar 2026 16:41:31 -0700 Subject: [PATCH 1/7] fix(ci): secrets can't be used in step if conditions [skip-regression-check] 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 --- .github/workflows/staging-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/staging-ci.yml b/.github/workflows/staging-ci.yml index 9e887436..22d70030 100644 --- a/.github/workflows/staging-ci.yml +++ b/.github/workflows/staging-ci.yml @@ -115,7 +115,7 @@ jobs: - name: Generate GitHub App token id: app-token - if: ${{ secrets.GH_RELEASES_MANAGER_APP_ID != '' }} + continue-on-error: true uses: actions/create-github-app-token@v2 with: app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }} @@ -230,7 +230,7 @@ jobs: - name: Generate GitHub App token id: app-token - if: ${{ secrets.GH_RELEASES_MANAGER_APP_ID != '' }} + continue-on-error: true uses: actions/create-github-app-token@v2 with: app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }} From f4b7309523973ec6c6288d570dde6792d4da0640 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 9 Mar 2026 17:59:59 -0700 Subject: [PATCH 2/7] fix(ci): cherry-pick CI cleanup onto staging [skip-regression-check] (#798) Cherry-pick of #794: remove continue-on-error hack, skip redundant checks on staging PRs, allow ironclaw-ci[bot] in Claude Code review. Co-authored-by: Claude Opus 4.6 --- .github/workflows/claude-review.yml | 1 + .github/workflows/code_style.yml | 2 ++ .github/workflows/staging-ci.yml | 2 -- .github/workflows/test.yml | 2 ++ 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 86d1bb2f..24d2fe98 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -28,6 +28,7 @@ jobs: uses: anthropics/claude-code-action@v1 with: anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + allowed_bots: "ironclaw-ci[bot]" claude_args: "--max-turns 50 --model claude-haiku-4-5-20251001 --allowedTools 'Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh search:*),Bash(git blame:*),Bash(git log:*),Bash(git diff:*)'" prompt: | Code review this pull request. Follow these steps precisely: diff --git a/.github/workflows/code_style.yml b/.github/workflows/code_style.yml index 526c7740..c65aa0df 100644 --- a/.github/workflows/code_style.yml +++ b/.github/workflows/code_style.yml @@ -1,6 +1,8 @@ name: Code Style on: pull_request: + branches: + - main jobs: format: diff --git a/.github/workflows/staging-ci.yml b/.github/workflows/staging-ci.yml index 22d70030..8e3693b2 100644 --- a/.github/workflows/staging-ci.yml +++ b/.github/workflows/staging-ci.yml @@ -115,7 +115,6 @@ jobs: - name: Generate GitHub App token id: app-token - continue-on-error: true uses: actions/create-github-app-token@v2 with: app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }} @@ -230,7 +229,6 @@ jobs: - name: Generate GitHub App token id: app-token - continue-on-error: true uses: actions/create-github-app-token@v2 with: app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index efa28648..bb29dd2a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -2,6 +2,8 @@ name: Run Tests on: workflow_call: pull_request: + branches: + - main push: branches: - main From 7d8576a4642652ce643a4ccba23a8c3abebbba98 Mon Sep 17 00:00:00 2001 From: Nick Pismenkov <50764773+nickpismenkov@users.noreply.github.com> Date: Mon, 9 Mar 2026 18:03:39 -0700 Subject: [PATCH 3/7] fix: destructive actions from ambiguous user prompts (#782) * fix: destructive actions from ambiguous user prompts * review fixes * review fixes --- src/agent/dispatcher.rs | 90 ++++++++++++++++++++++++++++ src/tools/builtin/extension_tools.rs | 36 +++++++++-- src/tools/builtin/skill_tools.rs | 36 ++++++++++- 3 files changed, 155 insertions(+), 7 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index f5306644..6118ec7d 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -1263,6 +1263,96 @@ mod tests { } } + #[test] + fn test_always_approval_requirement_bypasses_session_auto_approve() { + // Regression test: even if tool is auto-approved in session, + // ApprovalRequirement::Always must still trigger approval. + use crate::tools::ApprovalRequirement; + + let mut session = Session::new("user-1"); + let tool_name = "tool_remove"; + + // Manually auto-approve tool_remove in this session + session.auto_approve_tool(tool_name); + assert!( + session.is_tool_auto_approved(tool_name), + "tool should be auto-approved" + ); + + // However, ApprovalRequirement::Always should always require approval + // This is verified by the dispatcher logic: Always => true (ignores session state) + let always_req = ApprovalRequirement::Always; + let requires_approval = match always_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name), + ApprovalRequirement::Always => true, + }; + + assert!( + requires_approval, + "ApprovalRequirement::Always must require approval even when tool is auto-approved" + ); + } + + #[test] + fn test_always_approval_requirement_vs_unless_auto_approved() { + // Verify the two requirements behave differently + use crate::tools::ApprovalRequirement; + + let mut session = Session::new("user-2"); + let tool_name = "http"; + + // Scenario 1: Tool is auto-approved + session.auto_approve_tool(tool_name); + + // UnlessAutoApproved → doesn't require approval if auto-approved + let unless_req = ApprovalRequirement::UnlessAutoApproved; + let unless_needs = match unless_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name), + ApprovalRequirement::Always => true, + }; + assert!( + !unless_needs, + "UnlessAutoApproved should not need approval when auto-approved" + ); + + // Always → always requires approval + let always_req = ApprovalRequirement::Always; + let always_needs = match always_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name), + ApprovalRequirement::Always => true, + }; + assert!( + always_needs, + "Always must always require approval, even when auto-approved" + ); + + // Scenario 2: Tool is NOT auto-approved + let new_tool = "new_tool"; + assert!(!session.is_tool_auto_approved(new_tool)); + + // UnlessAutoApproved → requires approval + let unless_needs = match unless_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(new_tool), + ApprovalRequirement::Always => true, + }; + assert!( + unless_needs, + "UnlessAutoApproved should need approval when not auto-approved" + ); + + // Always → always requires approval + let always_needs = match always_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(new_tool), + ApprovalRequirement::Always => true, + }; + assert!(always_needs, "Always must always require approval"); + } + #[test] fn test_pending_approval_serialization_backcompat_without_deferred_calls() { // PendingApproval from before the deferred_tool_calls field was added diff --git a/src/tools/builtin/extension_tools.rs b/src/tools/builtin/extension_tools.rs index 7ba4ef0c..00c79548 100644 --- a/src/tools/builtin/extension_tools.rs +++ b/src/tools/builtin/extension_tools.rs @@ -451,8 +451,8 @@ impl Tool for ToolRemoveTool { } fn description(&self) -> &str { - "Remove an installed extension (channel, tool, or MCP server). \ - Unregisters tools and deletes configuration." + "Permanently remove an installed extension (channel, tool, or MCP server) from disk. \ + This action cannot be undone — the WASM binary and configuration files will be deleted." } fn parameters_schema(&self) -> serde_json::Value { @@ -492,7 +492,7 @@ impl Tool for ToolRemoveTool { } fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always } } @@ -701,10 +701,38 @@ mod tests { assert_eq!(tool.name(), "tool_remove"); assert_eq!( tool.requires_approval(&serde_json::json!({})), - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always ); } + #[test] + fn tool_remove_always_requires_approval_regardless_of_params() { + use crate::tools::tool::ApprovalRequirement; + let tool = ToolRemoveTool { + manager: test_manager_stub(), + }; + + let test_cases = vec![ + ("no params", serde_json::json!({})), + ("empty name", serde_json::json!({"name": ""})), + ("slack", serde_json::json!({"name": "slack"})), + ("github-cli", serde_json::json!({"name": "github-cli"})), + ( + "with extra fields", + serde_json::json!({"name": "tool", "extra": "field"}), + ), + ]; + + for (case_name, params) in test_cases { + assert_eq!( + tool.requires_approval(¶ms), + ApprovalRequirement::Always, + "tool_remove must always require approval for case: {}", + case_name + ); + } + } + #[test] fn test_tool_upgrade_schema() { use crate::tools::tool::ApprovalRequirement; diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs index 84c889ae..a7581ac4 100644 --- a/src/tools/builtin/skill_tools.rs +++ b/src/tools/builtin/skill_tools.rs @@ -709,7 +709,8 @@ impl Tool for SkillRemoveTool { } fn description(&self) -> &str { - "Remove an installed skill by name. Only user-installed skills can be removed." + "Permanently remove an installed skill from disk. This action cannot be undone — \ + the skill files will be deleted." } fn parameters_schema(&self) -> serde_json::Value { @@ -770,7 +771,7 @@ impl Tool for SkillRemoveTool { } fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always } } @@ -837,12 +838,41 @@ mod tests { assert_eq!(tool.name(), "skill_remove"); assert_eq!( tool.requires_approval(&serde_json::json!({})), - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always ); let schema = tool.parameters_schema(); assert!(schema["properties"].get("name").is_some()); } + #[test] + fn skill_remove_always_requires_approval_regardless_of_params() { + use crate::tools::tool::ApprovalRequirement; + let tool = SkillRemoveTool::new(test_registry()); + + let test_cases = vec![ + ("no params", serde_json::json!({})), + ("empty name", serde_json::json!({"name": ""})), + ( + "deployment skill", + serde_json::json!({"name": "deployment"}), + ), + ("custom skill", serde_json::json!({"name": "custom-skill"})), + ( + "with extra fields", + serde_json::json!({"name": "skill", "extra": "field"}), + ), + ]; + + for (case_name, params) in test_cases { + assert_eq!( + tool.requires_approval(¶ms), + ApprovalRequirement::Always, + "skill_remove must always require approval for case: {}", + case_name + ); + } + } + #[test] fn test_validate_fetch_url_allows_https() { assert!(super::validate_fetch_url("https://clawhub.ai/api/v1/download?slug=foo").is_ok()); From a5f88b32fd0e716df19eaaba4238efc99aa81cc3 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Tue, 10 Mar 2026 01:26:12 +0000 Subject: [PATCH 4/7] fix(setup): pass NEARAI_API_KEY to provider in model selection step (#799) When users authenticate via NEAR AI Cloud API key (option 4) during onboarding, the key is stored as an env var but fetch_nearai_models() was hardcoding api_key: None. This caused resolve_bearer_token() to re-trigger the interactive auth prompt at step 4 (model selection). Co-authored-by: Claude Opus 4.6 --- src/setup/wizard.rs | 135 +++++++++++++++++++++++++++++++++----------- 1 file changed, 103 insertions(+), 32 deletions(-) diff --git a/src/setup/wizard.rs b/src/setup/wizard.rs index 38a5d46d..97cc86fa 100644 --- a/src/setup/wizard.rs +++ b/src/setup/wizard.rs @@ -1573,46 +1573,18 @@ impl SetupWizard { } /// Fetch available models from the NEAR AI API. + /// + /// Uses [`build_nearai_model_fetch_config`] to construct the provider config, + /// which reads `NEARAI_API_KEY` from the environment when present. async fn fetch_nearai_models(&self) -> Vec { let session = match self.session_manager { Some(ref s) => Arc::clone(s), None => return vec![], }; - use crate::config::LlmConfig; use crate::llm::create_llm_provider; - let base_url = std::env::var("NEARAI_BASE_URL") - .unwrap_or_else(|_| "https://private.near.ai".to_string()); - let auth_base_url = std::env::var("NEARAI_AUTH_URL") - .unwrap_or_else(|_| "https://private.near.ai".to_string()); - - let config = LlmConfig { - backend: "nearai".to_string(), - session: crate::llm::session::SessionConfig { - auth_base_url, - session_path: crate::config::llm::default_session_path(), - }, - nearai: crate::config::NearAiConfig { - model: "dummy".to_string(), - cheap_model: None, - base_url, - api_key: None, - fallback_model: None, - max_retries: 3, - circuit_breaker_threshold: None, - circuit_breaker_recovery_secs: 30, - response_cache_enabled: false, - response_cache_ttl_secs: 3600, - response_cache_max_entries: 1000, - failover_cooldown_secs: 300, - failover_cooldown_threshold: 3, - smart_routing_cascade: true, - }, - provider: None, - bedrock: None, - request_timeout_secs: 120, - }; + let config = build_nearai_model_fetch_config(); match create_llm_provider(&config, session).await { Ok(provider) => match provider.list_models().await { @@ -3240,6 +3212,52 @@ async fn discover_wasm_channels(dir: &std::path::Path) -> Vec<(String, ChannelCa /// Mask an API key for display: show first 6 + last 4 chars. /// /// Uses char-based indexing to avoid panicking on multi-byte UTF-8. +/// Build the `LlmConfig` used by `fetch_nearai_models` to list available models. +/// +/// Reads `NEARAI_API_KEY` from the environment so that users who authenticated +/// via Cloud API key (option 4) don't get re-prompted during model selection. +fn build_nearai_model_fetch_config() -> crate::config::LlmConfig { + let base_url = std::env::var("NEARAI_BASE_URL") + .unwrap_or_else(|_| "https://private.near.ai".to_string()); + let auth_base_url = std::env::var("NEARAI_AUTH_URL") + .unwrap_or_else(|_| "https://private.near.ai".to_string()); + + // If the user authenticated via API key (option 4), the key is stored + // as an env var. Pass it through so `resolve_bearer_token()` doesn't + // re-trigger the interactive auth prompt. + let api_key = std::env::var("NEARAI_API_KEY") + .ok() + .filter(|k| !k.is_empty()) + .map(secrecy::SecretString::from); + + crate::config::LlmConfig { + backend: "nearai".to_string(), + session: crate::llm::session::SessionConfig { + auth_base_url, + session_path: crate::config::llm::default_session_path(), + }, + nearai: crate::config::NearAiConfig { + model: "dummy".to_string(), + cheap_model: None, + base_url, + api_key, + fallback_model: None, + max_retries: 3, + circuit_breaker_threshold: None, + circuit_breaker_recovery_secs: 30, + response_cache_enabled: false, + response_cache_ttl_secs: 3600, + response_cache_max_entries: 1000, + failover_cooldown_secs: 300, + failover_cooldown_threshold: 3, + smart_routing_cascade: true, + }, + provider: None, + bedrock: None, + request_timeout_secs: 120, + } +} + fn mask_api_key(key: &str) -> String { let chars: Vec = key.chars().collect(); if chars.len() < 12 { @@ -3640,6 +3658,14 @@ mod tests { } impl EnvGuard { + fn set(key: &'static str, value: &str) -> Self { + let original = std::env::var(key).ok(); + unsafe { + std::env::set_var(key, value); + } + Self { key, original } + } + fn clear(key: &'static str) -> Self { let original = std::env::var(key).ok(); unsafe { @@ -3826,4 +3852,49 @@ mod tests { }; assert!(settings.secrets_master_key_hex.is_some()); } + + /// Regression test for #799: `fetch_nearai_models` hardcoded `api_key: None`, + /// causing the auth prompt to re-appear during model selection when the user + /// had authenticated via NEAR AI Cloud API key (option 4). + #[test] + fn test_build_nearai_model_fetch_config_picks_up_api_key_env() { + use secrecy::ExposeSecret; + + let _guard = EnvGuard::set("NEARAI_API_KEY", "test-cloud-api-key-12345"); + + let config = build_nearai_model_fetch_config(); + assert!( + config.nearai.api_key.is_some(), + "config should include NEARAI_API_KEY from env" + ); + assert_eq!( + config.nearai.api_key.as_ref().unwrap().expose_secret(), + "test-cloud-api-key-12345" + ); + } + + /// Regression test for #799: when NEARAI_API_KEY is absent or empty, + /// the config should have `api_key: None` (session token path). + #[test] + fn test_build_nearai_model_fetch_config_none_when_no_api_key() { + let _guard = EnvGuard::clear("NEARAI_API_KEY"); + + let config = build_nearai_model_fetch_config(); + assert!( + config.nearai.api_key.is_none(), + "config should have no api_key when env var is absent" + ); + } + + /// Regression test for #799: empty NEARAI_API_KEY should be treated as absent. + #[test] + fn test_build_nearai_model_fetch_config_none_when_empty_api_key() { + let _guard = EnvGuard::set("NEARAI_API_KEY", ""); + + let config = build_nearai_model_fetch_config(); + assert!( + config.nearai.api_key.is_none(), + "config should have no api_key when env var is empty" + ); + } } From 7de639e7824f961add5324879e9295b87c56d0d2 Mon Sep 17 00:00:00 2001 From: Henry Park Date: Mon, 9 Mar 2026 18:44:57 -0700 Subject: [PATCH 5/7] fix(ci): cherry-pick fmt + clippy for staging PRs [skip-regression-check] (#803) Cherry-pick of #802: run fmt + clippy on staging PRs, skip Windows clippy, simplify claude-review trigger to labeled-only. Co-authored-by: Claude Opus 4.6 --- .github/workflows/claude-review.yml | 2 +- .github/workflows/code_style.yml | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 24d2fe98..3836a5f9 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -2,7 +2,7 @@ name: Claude Code Review on: pull_request: - types: [opened, labeled] + types: [labeled] permissions: contents: read diff --git a/.github/workflows/code_style.yml b/.github/workflows/code_style.yml index c65aa0df..620760ae 100644 --- a/.github/workflows/code_style.yml +++ b/.github/workflows/code_style.yml @@ -1,8 +1,6 @@ name: Code Style on: pull_request: - branches: - - main jobs: format: @@ -46,6 +44,7 @@ jobs: clippy-windows: name: Clippy Windows (${{ matrix.name }}) + if: github.base_ref == 'main' runs-on: windows-latest strategy: fail-fast: false @@ -78,7 +77,12 @@ jobs: needs: [format, clippy, clippy-windows] steps: - run: | - if [[ "${{ needs.format.result }}" != "success" || "${{ needs.clippy.result }}" != "success" || "${{ needs.clippy-windows.result }}" != "success" ]]; then + if [[ "${{ needs.format.result }}" != "success" || "${{ needs.clippy.result }}" != "success" ]]; then echo "One or more jobs failed" exit 1 fi + # clippy-windows only runs on main PRs, so skip/success are both acceptable + if [[ "${{ needs.clippy-windows.result }}" == "failure" ]]; then + echo "Windows clippy failed" + exit 1 + fi From 764be8547f136260549f5b2a2b6b502a6235a868 Mon Sep 17 00:00:00 2001 From: Nick Pismenkov <50764773+nickpismenkov@users.noreply.github.com> Date: Mon, 9 Mar 2026 19:14:36 -0700 Subject: [PATCH 6/7] fix: fmt (#805) --- src/setup/wizard.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/setup/wizard.rs b/src/setup/wizard.rs index 97cc86fa..30be9896 100644 --- a/src/setup/wizard.rs +++ b/src/setup/wizard.rs @@ -3217,10 +3217,10 @@ async fn discover_wasm_channels(dir: &std::path::Path) -> Vec<(String, ChannelCa /// Reads `NEARAI_API_KEY` from the environment so that users who authenticated /// via Cloud API key (option 4) don't get re-prompted during model selection. fn build_nearai_model_fetch_config() -> crate::config::LlmConfig { - let base_url = std::env::var("NEARAI_BASE_URL") - .unwrap_or_else(|_| "https://private.near.ai".to_string()); - let auth_base_url = std::env::var("NEARAI_AUTH_URL") - .unwrap_or_else(|_| "https://private.near.ai".to_string()); + let base_url = + std::env::var("NEARAI_BASE_URL").unwrap_or_else(|_| "https://private.near.ai".to_string()); + let auth_base_url = + std::env::var("NEARAI_AUTH_URL").unwrap_or_else(|_| "https://private.near.ai".to_string()); // If the user authenticated via API key (option 4), the key is stored // as an env var. Pass it through so `resolve_bearer_token()` doesn't From 83950d11a43ded67b8762b5cf76043d3880f3d7a Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Tue, 10 Mar 2026 02:19:56 +0000 Subject: [PATCH 7/7] =?UTF-8?q?fix:=20job=20token=20budget,=20iteration=20?= =?UTF-8?q?cap=20=E2=86=92=20Failed,=20web=20cancel=20stops=20worker=20(#7?= =?UTF-8?q?88)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * 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 * 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 * 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 * style: remove trailing blank line in libsql jobs.rs [skip-regression-check] Co-Authored-By: Claude Opus 4.6 * 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 --------- Co-authored-by: Claude Opus 4.6 * fix: add job token budget, change iteration cap to Failed, fix web cancel (#698) Jobs could enter infinite retry loops because: (1) no token budget was enforced, (2) iteration cap marked jobs as Stuck (allowing self-repair to restart them), and (3) the web UI cancel button only updated the DB without stopping the running worker. - Add `max_tokens_per_job` config (settings.json + AGENT_MAX_TOKENS_PER_JOB env var, default 0 = unlimited) with per-job metadata override - Track token usage after respond_with_tools() and fail the job on budget exceeded - Change iteration cap and persistent rate limiting from mark_stuck to mark_failed, preventing self-repair restart loops - Fix web cancel handler to call scheduler.stop() which updates in-memory state AND aborts the worker task, falling back to DB-only update Co-Authored-By: Claude Opus 4.6 * fix: address PR review — always persist cancel to DB, simplify token check - Cancel handler now always persists Cancelled to DB regardless of whether scheduler.stop() ran, fixing the edge case where stop() returns Ok(()) for jobs not in the scheduler map - Collapse nested ifs per clippy (let-chains) - Add NOTE comment about select_tools() not exposing TokenUsage [skip-regression-check] Co-Authored-By: Claude Opus 4.6 * fix: rustfmt formatting in wizard.rs (pre-existing) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- .env.example | 2 + src/agent/dispatcher.rs | 3 + src/agent/scheduler.rs | 16 ++++ src/agent/worker.rs | 103 +++++++++++++++++++++++++- src/channels/web/handlers/jobs.rs | 16 +++- src/channels/web/handlers/routines.rs | 2 + src/channels/web/server.rs | 2 + src/channels/web/types.rs | 1 + src/config/agent.rs | 7 ++ src/db/libsql/jobs.rs | 5 +- src/db/libsql/mod.rs | 18 +++++ src/history/store.rs | 37 ++++++++- src/settings.rs | 5 ++ 13 files changed, 211 insertions(+), 6 deletions(-) diff --git a/.env.example b/.env.example index 1200400d..5c21e995 100644 --- a/.env.example +++ b/.env.example @@ -115,6 +115,8 @@ AGENT_NAME=ironclaw AGENT_MAX_PARALLEL_JOBS=5 AGENT_JOB_TIMEOUT_SECS=3600 AGENT_STUCK_THRESHOLD_SECS=300 +# Maximum tokens per job (0 = unlimited, also settable via settings.json agent.max_tokens_per_job) +# AGENT_MAX_TOKENS_PER_JOB=0 # Enable planning phase before tool execution (default: true) AGENT_USE_PLANNING=true diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 6118ec7d..99feed9d 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -1205,6 +1205,7 @@ mod tests { max_tool_iterations: 50, auto_approve_tools: false, default_timezone: "UTC".to_string(), + max_tokens_per_job: 0, }, deps, Arc::new(ChannelManager::new()), @@ -2043,6 +2044,7 @@ mod tests { max_tool_iterations, auto_approve_tools: true, default_timezone: "UTC".to_string(), + max_tokens_per_job: 0, }, deps, Arc::new(ChannelManager::new()), @@ -2159,6 +2161,7 @@ mod tests { max_tool_iterations: max_iter, auto_approve_tools: true, default_timezone: "UTC".to_string(), + max_tokens_per_job: 0, }, deps, Arc::new(ChannelManager::new()), diff --git a/src/agent/scheduler.rs b/src/agent/scheduler.rs index 99386d7f..85f3f6eb 100644 --- a/src/agent/scheduler.rs +++ b/src/agent/scheduler.rs @@ -160,6 +160,13 @@ impl Scheduler { .create_job_for_user(user_id, title, description) .await?; + // Apply token budget from config, allowing per-job metadata override. + let max_tokens = metadata + .as_ref() + .and_then(|m| m.get("max_tokens")) + .and_then(|v| v.as_u64()) + .unwrap_or(self.config.max_tokens_per_job); + // Apply metadata if provided if let Some(meta) = metadata { self.context_manager @@ -169,6 +176,15 @@ impl Scheduler { .await?; } + // Set token budget (separate update to avoid overwriting metadata) + if max_tokens > 0 { + self.context_manager + .update_context(job_id, |ctx| { + ctx.max_tokens = max_tokens; + }) + .await?; + } + // Persist to DB before scheduling so the worker's FK references are valid if let Some(ref store) = self.store { let ctx = self.context_manager.get_context(job_id).await?; diff --git a/src/agent/worker.rs b/src/agent/worker.rs index 3604cea9..19bfc8e5 100644 --- a/src/agent/worker.rs +++ b/src/agent/worker.rs @@ -417,7 +417,8 @@ Report when the job is complete or if you encounter issues you cannot resolve."# iteration += 1; if iteration > max_iterations { - self.mark_stuck("Maximum iterations exceeded").await?; + self.mark_failed("Maximum iterations exceeded: job hit the iteration cap") + .await?; return Ok(()); } @@ -437,7 +438,8 @@ Report when the job is complete or if you encounter issues you cannot resolve."# "LLM rate limited during tool selection, backing off" ); if consecutive_rate_limits >= MAX_CONSECUTIVE_RATE_LIMITS { - self.mark_stuck("Persistent rate limiting").await?; + self.mark_failed("Persistent rate limiting: exceeded retry limit") + .await?; return Ok(()); } self.log_event( @@ -467,7 +469,8 @@ Report when the job is complete or if you encounter issues you cannot resolve."# "LLM rate limited during respond_with_tools, backing off" ); if consecutive_rate_limits >= MAX_CONSECUTIVE_RATE_LIMITS { - self.mark_stuck("Persistent rate limiting").await?; + self.mark_failed("Persistent rate limiting: exceeded retry limit") + .await?; return Ok(()); } self.log_event( @@ -483,6 +486,20 @@ Report when the job is complete or if you encounter issues you cannot resolve."# Err(e) => return Err(e.into()), }; + // Track token usage from LLM call against the job budget. + // NOTE: select_tools() also makes LLM calls but doesn't expose + // TokenUsage; only respond_with_tools() usage is tracked here. + let total_tokens = respond_output.usage.total() as u64; + if total_tokens > 0 + && let Err(msg) = self + .context_manager() + .update_context(self.job_id, |ctx| ctx.add_tokens(total_tokens)) + .await? + { + self.mark_failed(&msg).await?; + return Ok(()); + } + match respond_output.result { RespondResult::Text(response) => { // Check for explicit completion phrases. Use word-boundary @@ -1762,4 +1779,84 @@ mod tests { "Always tool should be allowed with permission" ); } + + #[tokio::test] + async fn test_token_budget_exceeded_fails_job() { + let worker = make_worker(vec![]).await; + + // Transition to InProgress (required for mark_failed) + worker + .context_manager() + .update_context(worker.job_id, |ctx| { + ctx.transition_to(JobState::InProgress, None) + }) + .await + .unwrap() + .unwrap(); + + // Set a token budget + worker + .context_manager() + .update_context(worker.job_id, |ctx| { + ctx.max_tokens = 100; + }) + .await + .unwrap(); + + // Simulate adding tokens that exceed the budget + let budget_result = worker + .context_manager() + .update_context(worker.job_id, |ctx| ctx.add_tokens(200)) + .await + .unwrap(); + + assert!( + budget_result.is_err(), + "Should return error when token budget exceeded" + ); + + // Verify that mark_failed transitions job to Failed + worker + .mark_failed(&budget_result.unwrap_err()) + .await + .unwrap(); + let ctx = worker + .context_manager() + .get_context(worker.job_id) + .await + .unwrap(); + assert_eq!(ctx.state, JobState::Failed); + } + + #[tokio::test] + async fn test_iteration_cap_marks_failed_not_stuck() { + let worker = make_worker(vec![]).await; + + // Transition to InProgress (required for mark_failed) + worker + .context_manager() + .update_context(worker.job_id, |ctx| { + ctx.transition_to(JobState::InProgress, None) + }) + .await + .unwrap() + .unwrap(); + + // Simulate what the execution loop does when max_iterations is exceeded + worker + .mark_failed("Maximum iterations exceeded: job hit the iteration cap") + .await + .unwrap(); + + let ctx = worker + .context_manager() + .get_context(worker.job_id) + .await + .unwrap(); + assert_eq!( + ctx.state, + JobState::Failed, + "Iteration cap should transition to Failed, not Stuck" + ); + } } diff --git a/src/channels/web/handlers/jobs.rs b/src/channels/web/handlers/jobs.rs index 8a127243..5a94e055 100644 --- a/src/channels/web/handlers/jobs.rs +++ b/src/channels/web/handlers/jobs.rs @@ -276,11 +276,25 @@ pub async fn jobs_cancel_handler( }))); } - // Fall back to agent job cancellation via DB status update. + // Fall back to agent job cancellation: stop the worker via the scheduler + // (which updates the in-memory ContextManager AND aborts the task handle), + // then persist the status to the DB as a fallback. if let Some(ref store) = state.store && let Ok(Some(job)) = store.get_job(job_id).await { if job.state.is_active() { + // Try to stop via scheduler (aborts the worker task + updates + // in-memory ContextManager). This is best-effort — the job may + // not be in the scheduler map if it already finished. + if let Some(ref slot) = state.scheduler + && let Some(ref scheduler) = *slot.read().await + { + let _ = scheduler.stop(job_id).await; + } + + // Always persist cancellation to the DB so the state is + // consistent even if the scheduler wasn't available or the + // job wasn't in its in-memory map. store .update_job_status( job_id, diff --git a/src/channels/web/handlers/routines.rs b/src/channels/web/handlers/routines.rs index d7c4f764..8fbcc97b 100644 --- a/src/channels/web/handlers/routines.rs +++ b/src/channels/web/handlers/routines.rs @@ -108,6 +108,7 @@ pub async fn routines_detail_handler( status: format!("{:?}", run.status), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, + job_id: run.job_id, }) .collect(); @@ -252,6 +253,7 @@ pub async fn routines_runs_handler( status: format!("{:?}", run.status), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, + job_id: run.job_id, }) .collect(); diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index d6605eee..f454363d 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -2017,6 +2017,7 @@ async fn routines_detail_handler( status: format!("{:?}", run.status), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, + job_id: run.job_id, }) .collect(); @@ -2169,6 +2170,7 @@ async fn routines_runs_handler( status: format!("{:?}", run.status), result_summary: run.result_summary.clone(), tokens_used: run.tokens_used, + job_id: run.job_id, }) .collect(); diff --git a/src/channels/web/types.rs b/src/channels/web/types.rs index 4d85c671..b6d0d05a 100644 --- a/src/channels/web/types.rs +++ b/src/channels/web/types.rs @@ -776,6 +776,7 @@ pub struct RoutineRunInfo { pub status: String, pub result_summary: Option, pub tokens_used: Option, + pub job_id: Option, } // --- Settings --- diff --git a/src/config/agent.rs b/src/config/agent.rs index 096c141f..cb09707d 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -29,6 +29,8 @@ pub struct AgentConfig { pub auto_approve_tools: bool, /// Default timezone for new sessions (IANA name, e.g. "America/New_York"). pub default_timezone: String, + /// Maximum tokens per job (0 = unlimited). + pub max_tokens_per_job: u64, } impl AgentConfig { @@ -50,6 +52,7 @@ impl AgentConfig { max_tool_iterations: 10, auto_approve_tools: true, default_timezone: "UTC".to_string(), + max_tokens_per_job: 0, } } @@ -105,6 +108,10 @@ impl AgentConfig { } tz }, + max_tokens_per_job: parse_optional_env( + "AGENT_MAX_TOKENS_PER_JOB", + settings.agent.max_tokens_per_job, + )?, }) } } diff --git a/src/db/libsql/jobs.rs b/src/db/libsql/jobs.rs index d5172360..0750873d 100644 --- a/src/db/libsql/jobs.rs +++ b/src/db/libsql/jobs.rs @@ -28,14 +28,16 @@ impl JobStore for LibSqlBackend { r#" INSERT INTO agent_jobs ( id, conversation_id, title, description, category, status, source, + user_id, budget_amount, budget_token, bid_amount, estimated_cost, estimated_time_secs, actual_cost, repair_attempts, created_at, started_at, completed_at - ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17) + ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17, ?18) ON CONFLICT (id) DO UPDATE SET title = excluded.title, description = excluded.description, category = excluded.category, status = excluded.status, + user_id = excluded.user_id, estimated_cost = excluded.estimated_cost, estimated_time_secs = excluded.estimated_time_secs, actual_cost = excluded.actual_cost, @@ -51,6 +53,7 @@ impl JobStore for LibSqlBackend { opt_text(ctx.category.as_deref()), status, "direct", + ctx.user_id.as_str(), opt_text_owned(ctx.budget.map(|d| d.to_string())), opt_text(ctx.budget_token.as_deref()), opt_text_owned(ctx.bid_amount.map(|d| d.to_string())), diff --git a/src/db/libsql/mod.rs b/src/db/libsql/mod.rs index 2845c757..404441e6 100644 --- a/src/db/libsql/mod.rs +++ b/src/db/libsql/mod.rs @@ -482,6 +482,24 @@ mod tests { assert_eq!(timeout, 5000); } + /// Regression test: save_job must persist user_id and get_job must return it. + #[tokio::test] + async fn test_save_job_persists_user_id() { + use crate::context::JobContext; + use crate::db::JobStore; + + let dir = tempfile::tempdir().unwrap(); + let db_path = dir.path().join("test_user_id.db"); + let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); + backend.run_migrations().await.unwrap(); + + let ctx = JobContext::with_user("test-user-42", "Test Job", "A test job"); + backend.save_job(&ctx).await.unwrap(); + + let loaded = backend.get_job(ctx.job_id).await.unwrap().unwrap(); + assert_eq!(loaded.user_id, "test-user-42"); + } + #[tokio::test] async fn test_concurrent_writes_succeed() { // Use a temp file so connections share state (in-memory DBs are connection-local) diff --git a/src/history/store.rs b/src/history/store.rs index f0b0b144..1153f3e4 100644 --- a/src/history/store.rs +++ b/src/history/store.rs @@ -149,14 +149,16 @@ impl Store { r#" INSERT INTO agent_jobs ( id, conversation_id, title, description, category, status, source, + user_id, budget_amount, budget_token, bid_amount, estimated_cost, estimated_time_secs, actual_cost, repair_attempts, created_at, started_at, completed_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17) + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) ON CONFLICT (id) DO UPDATE SET title = EXCLUDED.title, description = EXCLUDED.description, category = EXCLUDED.category, status = EXCLUDED.status, + user_id = EXCLUDED.user_id, estimated_cost = EXCLUDED.estimated_cost, estimated_time_secs = EXCLUDED.estimated_time_secs, actual_cost = EXCLUDED.actual_cost, @@ -172,6 +174,7 @@ impl Store { &ctx.category, &status, &"direct", // source + &ctx.user_id, &ctx.budget, &ctx.budget_token, &ctx.bid_amount, @@ -2133,4 +2136,36 @@ mod tests { assert_eq!(summary.channel, ch); } } + + /// Regression test: save_job must persist user_id and get_job must return it. + /// Requires a running PostgreSQL instance (integration tier). + #[cfg(feature = "postgres")] + #[tokio::test] + #[ignore] + async fn test_save_job_persists_user_id() { + use crate::config::Config; + use crate::context::JobContext; + + let _ = dotenvy::dotenv(); + let config = Config::from_env().await.expect("Failed to load config"); + let store = Store::new(&config.database) + .await + .expect("Failed to connect to database"); + store + .run_migrations() + .await + .expect("Failed to run migrations"); + + let ctx = JobContext::with_user("test-user-42", "PG user_id test", "regression test"); + store.save_job(&ctx).await.unwrap(); + + let loaded = store.get_job(ctx.job_id).await.unwrap().unwrap(); + assert_eq!(loaded.user_id, "test-user-42"); + + // Clean up + let conn = store.conn().await.unwrap(); + conn.execute("DELETE FROM agent_jobs WHERE id = $1", &[&ctx.job_id]) + .await + .unwrap(); + } } diff --git a/src/settings.rs b/src/settings.rs index 836d1d2c..63535aef 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -386,6 +386,10 @@ pub struct AgentSettings { /// Default timezone for new sessions (IANA name, e.g. "America/New_York"). #[serde(default = "default_timezone")] pub default_timezone: String, + + /// Maximum tokens per job (0 = unlimited). + #[serde(default)] + pub max_tokens_per_job: u64, } fn default_agent_name() -> String { @@ -442,6 +446,7 @@ impl Default for AgentSettings { max_tool_iterations: default_max_tool_iterations(), auto_approve_tools: false, default_timezone: default_timezone(), + max_tokens_per_job: 0, } } }