fix(engine): guard consecutive-error checks against None limit#2460
fix(engine): guard consecutive-error checks against None limit#2460serrrfirat merged 1 commit intostagingfrom
Conversation
ThreadConfig::default() had max_consecutive_errors: None, which serializes to null. The Python orchestrator's config.get(..., 5) returns None (not 5) when the key is present with a null value, so the guard `consecutive_action_errors >= max_consecutive_errors + 2` crashed with TypeError on the first action error. Fix both sides: default to Some(5) in Rust so the happy path sends a real int, and treat None as "no limit" in Python so callers can explicitly disable the guard without blowing up arithmetic.
There was a problem hiding this comment.
Pull request overview
This PR addresses a crash in the v2 engine orchestrator when max_consecutive_errors is explicitly null/None, and also introduces a broad set of additional platform features (admin defaults/settings scope, admin usage endpoints/UI assets, generated-image handling, CLI/profile improvements, and release/build workflow updates).
Changes:
- Fix
max_consecutive_errorsdefault/guarding across Rust + Monty orchestrator to preventNone/nullmath crashes and better track tool-batch failures. - Add admin-scoped settings defaults and new admin usage summary endpoints + supporting DB queries/migrations and admin SPA assets/routes.
- Improve image tooling and history handling (generated-image sentinels, endpoint URL normalization, media type inference), plus assorted CLI/e2e/test/build updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/scenarios/test_webhook.py | Removes explicit pytest asyncio markers (repo uses asyncio_mode=auto). |
| tests/e2e/scenarios/test_tool_approval.py | Adds e2e coverage ensuring approval resolution posts the card’s thread_id and slash-approve is thread-scoped. |
| tests/e2e/scenarios/test_telegram_e2e.py | Expands Telegram e2e flow (verification handling, pairing helpers, routine visibility for paired users). |
| tests/e2e/scenarios/test_sse_reconnect.py | Adds scenario ensuring refresh without URL hash reopens active thread history. |
| tests/e2e/conftest.py | Refactors Telegram server fixture to support routines-enabled variant. |
| src/worker/job.rs | Tracks whether the last tool batch fully failed for agentic-loop duplicate tool detection. |
| src/worker/container.rs | Same “all tools failed” tracking for container delegate execution. |
| src/util.rs | Adds shared canonicalize_json_value helper and tests. |
| src/tunnel/mod.rs | Updates custom tunnel construction to propagate errors from CustomTunnel::new. |
| src/tunnel/custom.rs | Reuses a configured reqwest client for health checks; new() now returns Result. |
| src/tools/mcp/config.rs | Makes NEAR AI MCP env bootstrap return structured ConfigError on invalid config. |
| src/tools/builtin/mod.rs | Adds shared image API endpoint builder that avoids double /v1 suffixing. |
| src/tools/builtin/image_gen.rs | Uses shared endpoint builder; infers image media type from base64 header; expands tests. |
| src/tools/builtin/image_edit.rs | Uses shared endpoint builder and inferred media type for edited/generated images; adds tests. |
| src/tenant.rs | Adds get_setting_with_admin_fallback() + tests for admin default inheritance. |
| src/setup/channels.rs | Prints a Telegram-specific setup mode note; adds a unit test. |
| src/setup/README.md | Documents Telegram pairing vs open mode implications for web history continuity. |
| src/settings.rs | Adds temperature setting and changes default CLI mode to tui. |
| src/registry/catalog.rs | Hardens registry directory detection to avoid matching unrelated “registry” dirs; adds tests. |
| src/main.rs | Adds profile CLI command routing. |
| src/llm/reasoning.rs | Adds per-context temperature support (clamped) and tool-batch failure flag on context. |
| src/llm/models.rs | Populates additional top-level LlmConfig fields for NEAR AI model fetch config. |
| src/llm/mod.rs | Normalizes OpenAI-compatible base URLs; moves retry/cb/cache config to top-level LlmConfig; adds tests. |
| src/llm/error.rs | Improves AuthFailed message with provider-specific setup guidance + snapshot tests. |
| src/llm/config.rs | Adds top-level retry/circuit-breaker/response-cache settings in LlmConfig. |
| src/llm/CLAUDE.md | Updates docs to reflect LlmConfig (not NearAiConfig) ownership of retry/cb/cache config. |
| src/lib.rs | Exposes generated_images module internally. |
| src/history/store.rs | Adds postgres-backed admin_usage_summary query + ignored PG integration test. |
| src/generated_images.rs | New module to parse/compact generated-image sentinel payloads with size caps + tests. |
| src/db/postgres.rs | Wires admin_usage_summary into postgres UserStore impl. |
| src/db/mod.rs | Adds AdminUsageSummary DTO and UserStore::admin_usage_summary trait method. |
| src/db/libsql_migrations.rs | Adds libSQL migration for llm_calls.created_at index. |
| src/db/libsql/users.rs | Implements admin_usage_summary for libSQL + tests validating the time window. |
| src/config/profile.rs | Makes BUILTIN_PROFILES visible to the profile CLI. |
| src/config/mod.rs | Adds config layering: TOML -> admin DB defaults -> per-user DB; adds regression tests for admin-only key stripping. |
| src/config/channels.rs | Defaults CLI mode to "tui" when unset. |
| src/cli/snapshots/ironclaw__cli__tests__long_help_output_without_import.snap | Updates CLI help snapshot for new wording + profile command. |
| src/cli/snapshots/ironclaw__cli__tests__help_output_without_import.snap | Updates short help snapshot for new wording + profile command. |
| src/cli/profile.rs | Adds ironclaw profile list [--json] command + tests for descriptions. |
| src/cli/models.rs | Improves user guidance in errors/output and reminds about required API keys after provider switch. |
| src/cli/mod.rs | Adds profile subcommand and rewrites long help/command descriptions. |
| src/channels/web/types.rs | Adds generated-images info to turn DTOs; adds multiple admin API DTOs; adds SettingScopeQuery. |
| src/channels/web/tests/multi_tenant.rs | Adds admin usage summary route enforcement + admin API contract tests (libsql feature). |
| src/channels/web/server.rs | Adds admin usage summary endpoint route and serves admin SPA assets; adds generated image extraction/budget enforcement in history responses. |
| src/channels/web/responses_api.rs | Updates error message when per-request temperature is provided (still unsupported). |
| src/channels/web/mod.rs | Extends ImageGenerated status update to include event_id. |
| src/channels/web/handlers/settings.rs | Adds ?scope=admin support for settings get/set/delete with admin-only authorization. |
| src/channels/web/handlers/chat.rs | Removes duplicate history handler and keeps only shared helpers + thread handlers. |
| src/channels/web/CLAUDE.md | Documents new /api/admin/usage/summary and /theme.css route. |
| src/channels/channel.rs | Adds event_id to StatusUpdate::ImageGenerated. |
| src/bridge/router.rs | Scopes gateway /approve to active thread to avoid cross-thread gate resolution; adds test. |
| src/bridge/llm_adapter.rs | Forwards per-call model override into completion/tool requests; adds tests. |
| src/app.rs | Updates default image generation model fallback. |
| src/agent/session.rs | Summarizes generated-image tool results instead of replaying full data URLs into context; adds test. |
| src/agent/routine.rs | Uses shared util::canonicalize_json_value. |
| src/agent/attachments.rs | Sets image URL detail to "auto" for provider compatibility; adds test. |
| skills/llm-council/SKILL.md | Adds a new skill doc describing multi-model “council” usage patterns and limitations. |
| scripts/check_no_panics.py | Excludes Rust test-only paths from panic scanning; adds unit test. |
| release-plz.toml | Configures release-plz tagging/publish behavior for workspace packages. |
| registry/tools/github.json | Bumps GitHub tool registry version + artifact URL/SHA. |
| registry/tools/composio.json | Adds artifact metadata for composio tool. |
| registry/channels/whatsapp.json | Updates channel artifact URL/SHA. |
| registry/channels/telegram.json | Updates channel artifact URL/SHA. |
| registry/channels/slack.json | Updates channel artifact URL/SHA. |
| registry/channels/feishu.json | Updates channel artifact URL/SHA. |
| registry/channels/discord.json | Updates channel artifact URL/SHA. |
| railway.toml | Removes explicit docker build target (relies on Dockerfile default/last stage). |
| migrations/V24__llm_calls_created_at_index.sql | Adds postgres migration to index llm_calls.created_at. |
| deny.toml | Ignores a new RUSTSEC advisory with a revisit note. |
| crates/ironclaw_tui/Cargo.toml | Marks crate as publish = false. |
| crates/ironclaw_safety/Cargo.toml | Bumps crate version to 0.2.1. |
| crates/ironclaw_safety/CHANGELOG.md | Adds changelog entries for 0.2.1. |
| crates/ironclaw_gateway/static/theme.css | Adds shared theme tokens stylesheet. |
| crates/ironclaw_gateway/static/index.html | Loads theme.css and adds Docs link in UI header. |
| crates/ironclaw_gateway/static/i18n/zh-CN.js | Adds Docs tab text and temperature setting strings. |
| crates/ironclaw_gateway/static/i18n/ko.js | Adds Docs tab text and temperature setting strings. |
| crates/ironclaw_gateway/static/i18n/en.js | Adds Docs tab text and temperature setting strings. |
| crates/ironclaw_gateway/static/app.js | Adds generated-image caching/rendering, thread-aware approvals, active-thread restore behavior, and temperature setting UI support. |
| crates/ironclaw_gateway/static/admin.html | Adds admin SPA HTML shell (token entry + nav). |
| crates/ironclaw_gateway/src/assets.rs | Embeds theme.css + admin SPA assets. |
| crates/ironclaw_gateway/Cargo.toml | Marks crate as publish = false. |
| crates/ironclaw_engine/src/types/thread.rs | Sets default max_consecutive_errors to Some(5) + regression test. |
| crates/ironclaw_engine/src/traits/llm.rs | Adds per-call model override field to engine LlmCallConfig. |
| crates/ironclaw_engine/prompts/codeact_preamble.md | Documents model= and models= overrides and updates Monty limitations section. |
| crates/ironclaw_engine/orchestrator/default.py | Guards consecutive error checks against None; ensures tool call/result correspondence; tracks consecutive action errors. |
| crates/ironclaw_engine/MONTY.md | Updates Monty pin to v0.0.11 and refreshes limitation notes. |
| crates/ironclaw_engine/Cargo.toml | Bumps monty dependency and ironclaw_common version. |
| channels-src/telegram/src/lib.rs | Fixes Telegram message chunking to respect UTF-16 unit limit; adds tests. |
| channels-src/feishu/src/lib.rs | Minor formatting refactor around pairing request logging. |
| build.rs | Adds directory-level registry rerun watches to detect new/removed manifest files. |
| Dockerfile | Adds explicit runtime target and changes wasm-builder layer caching strategy; makes runtime-staging last for Railway. |
| Cargo.toml | Bumps root version to 0.25.0 and updates shared crate versions. |
| Cargo.lock | Updates lockfile for version bumps and monty tag pin. |
| .github/workflows/test.yml | Builds Docker with --target runtime. |
| .github/workflows/release.yml | Changes tag trigger pattern and adds docker image workflow job. |
| .github/workflows/pr-label-scope.yml | Ensures “DB MIGRATION” label exists before running labeler. |
| .github/scripts/create-labels.sh | Adds creation of “DB MIGRATION” label. |
| .github/labeler.yml | Adds “DB MIGRATION” label mapping and clarifies header comment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request updates the application to version 0.25.0, introducing a new Admin Panel SPA for user management and usage monitoring. Key features include an enhanced agentic loop with duplicate failing tool call detection, support for per-request model overrides, and improved image generation handling with data URL capping. The PR also optimizes database performance for usage aggregations and implements admin-level setting fallbacks. Review feedback suggests trimming user display names during creation to handle whitespace and implementing server-side pagination for the user list to improve scalability and prevent performance bottlenecks.
I am having trouble creating individual review comments. Click here to see my feedback.
src/channels/web/handlers/users.rs (81-88)
The display_name should be trimmed before validation and persistence to avoid issues with leading or trailing whitespace. Additionally, it is better to ensure the name is not empty after trimming.
let display_name = body
.get("display_name")
.and_then(|v| v.as_str())
.map(|s| s.trim())
.filter(|s| !s.is_empty())
.ok_or((
StatusCode::BAD_REQUEST,
"display_name is required and cannot be empty".to_string(),
))?
.to_string();
src/channels/web/handlers/users.rs (205)
The users_list_handler currently fetches and processes the entire user list and their associated LLM usage stats in a single request. This approach will not scale as the number of users and LLM calls grows, especially in multi-tenant deployments. Consider implementing server-side pagination for the user list and limiting the usage stats aggregation to only the users being displayed on the current page to prevent performance bottlenecks and DoS vulnerabilities.
References
- Use targeted database queries to fetch specific records instead of loading all records and filtering in the application, especially for public endpoints, to prevent performance bottlenecks and DoS vulnerabilities.
serrrfirat
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix. Both the Rust default change and Python None guards are correct. All three comparison sites in default.py are covered, backward compatibility with old serialized configs is maintained, and the regression tests are appropriate.
Summary
ThreadConfig::default()hadmax_consecutive_errors: None, which serializes tonull. In the Python orchestrator,config.get("max_consecutive_errors", 5)returnsNone(not5) when the key is present with a null value, soconsecutive_action_errors >= max_consecutive_errors + 2crashed with TypeError on the first action error (e.g. a missingcommitments/README.mdonmemory_read).Some(5)so callers usingThreadConfig::default()send a real int; Python guards (default.py:661,:870,:877) now short-circuit onNoneand treat it as "no limit", matching theOption<u32>semantics used elsewhere (max_tokens_total,max_budget_usd).Test plan
cargo test -p ironclaw_engine --lib -- default_config_has_concrete none_limit— new regression tests cover the Rust default and both Python guards (action errors + code errors) withmax_consecutive_errors = Nonecargo check -p ironclaw_enginecleancargo fmt