mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-02 23:56:24 +08:00
4dea5dd5da
* fix: bug bash 4/16 triage — error boundary, TEE secrets, pairing, rehydration Addresses six bug-bash tickets that cluster into five focused fixes. Grouped into one commit because the changes are all small, independent, and share the same release window — split per-file reviewability is preserved by the touched-surface list below and each change carries a regression test. - #2540 — Orchestrator VM timeout is now configurable via `IRONCLAW_ORCHESTRATOR_MAX_DURATION_SECS` (30..=3600s, default 300s). Timeout, memory-limit, and Python-traceback errors map to user-safe messages instead of leaking the Monty interpreter's internal trace. - #1994, #2546 — New `LlmError::BadGateway { provider, status, retry_after }` variant. Upstream 502/503/504 from `nearai_chat` now map here (body logged at debug, never carried on the error) and are retried by `RetryProvider` + counted transient by the circuit breaker. Root cause of #2546's raw-traceback leak was the response body being wrapped into `RequestFailed.reason` and nested three layers deep on the way out; that path is gone. - #1537 — `AppBuilder::init_secrets` always installs a secrets store: persistent when the master key + DB handles resolve, ephemeral in-memory otherwise. This mirrors the ExtensionManager fallback so `WasmToolLoader` and `setup_wasm_channels` get a store on hosted TEE deployments where `SECRETS_MASTER_KEY` is absent, restoring the fail-closed credential-injection path instead of silently dropping into unauthenticated HTTP. - #1839 — Slack `chat.postMessage` returns HTTP 200 on scope/token failures with `{"ok": false, "error": ...}` in the body. Response parsing was extracted into a testable `slack_post_message_result` helper that now surfaces the failure, and `send_pairing_reply` errors are logged with scope guidance (`chat:write`, `im:write`) instead of being swallowed by `let _ = ...`. - #1993 — Chat rehydration's `reconcile_in_progress_with_turns` now requires BOTH a final response AND all recorded tool calls having `has_result && !has_error` before dropping the in-progress flag. Previously a 502 mid-turn would persist the agent's "Done!" claim while the tool call errored, and reopen showed fabricated success. The deeper fix (engine-v2 side-effect gate for the forward path at #2544 / #2541) is a follow-up. Touched surfaces: - channels-src/slack/src/lib.rs - crates/ironclaw_engine/src/executor/orchestrator.rs - src/app.rs - src/channels/web/features/chat/mod.rs - src/llm/{error,nearai_chat,retry,circuit_breaker}.rs Regression tests: - `orchestrator::tests::failure_reason_*` (4 cases covering timeout, memory limit, traceback strip, pass-through) - `llm::retry::tests::test_is_retryable_classification` (BadGateway arm) - `app::tests::ephemeral_secrets_store_is_constructible_and_usable` - `slack::tests::slack_post_message_result_{accepts,rejects,empty}` - `chat::tests::test_reconcile_retains_in_progress_when_tool_call_failed` Out of scope / deferred: - #2544, #2541 — engine-v2 hard side-effect gate (documented as aspirational in `.claude/rules/tool-evidence.md`; design belongs in its own PR). - #2437 — closed upstream, no code change; see https://github.com/nearai/ironclaw/issues/2437#issuecomment-4282541384 - #2543 — likely fixed by #2515, needs retest on staging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tee): surface persistent-store failures and probe in doctor Follow-up to the #1537 ephemeral-store fallback. The fallback alone doesn't tell an operator *why* the persistent store is missing on a hosted TEE — that was #1537's real ergonomic pain. Three diagnostic improvements: 1. `install_ephemeral_secrets_store` now takes a `reason` tag and logs at `warn!` with the specific path (no master key / crypto failure / no DB handles / feature-flag mismatch / unexpected create_secrets_store None). Previously the install was silent at `debug!`, so operators had no signal the fallback had fired. 2. `ironclaw doctor`'s `check_secrets` now runs the same `SecretsConfig::resolve` path `AppBuilder::init_secrets` uses, then calls `create_secrets_store` to probe that the backing store is actually reachable. The old check only read `settings.secrets_master_key_source`, which misses the exact hosted-TEE failure mode: master key resolves to `Env`/`Keychain` but the DB handle isn't wired, so the store factory returns None and runtime silently falls back to ephemeral. 3. `src/db/CLAUDE.md` note claiming `LibSqlSecretsStore` is "not plumbed through the main startup path" was stale — the factory dispatches on `DatabaseHandles` (init_secrets path) and `DatabaseBackend` (CLI helper) and both wire libSQL. Note updated to reflect the actual wiring plus the #1537 ephemeral-fallback contract. The two existing `check_secrets` unit tests asserted the old settings- only behavior; rewritten as "does-not-panic" checks because the new function reads real env and the outcome is test-host dependent (matches the shape of `check_docker_daemon_does_not_panic`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm,app): address PR #2753 review comments Three fixes from Copilot + Gemini review on PR #2753: 1. **BadGateway retry_after no longer forces 60s sleeps.** Copilot flagged that `retry_after_header` was always `Some(parse_retry_after(...))`, and `parse_retry_after` returns a 60s default when the header is absent. That meant 502/503/504 responses without a Retry-After header would sleep ~60s between attempts instead of using exponential backoff (1s → 2s → 4s). Now the header is parsed only when present; absent header → `None` → `RetryProvider` falls through to `retry_backoff_delay`. Existing 429 rate-limit behavior is preserved (60s fallback kept explicit at the 429 call site). 2. **HTTP 500 is now mapped to BadGateway.** Gemini (security-medium) pointed out that upstream application errors frequently return 500 with a Python traceback in the body, and my prior change only mapped 502–504. 500 was falling through to `RequestFailed { reason: "HTTP 500: <body>" }` — exactly the leak #2546 describes. Match broadened to `500..=599`; the `status` field still records the specific code for operators. Matches the intent documented in `.claude/rules/error-handling.md` ("raw HTTP 5xx → temporarily unavailable"). 3. **Ephemeral secrets store now fails loud.** Copilot observed that `build_ephemeral_secrets_store` returning `None` + the fallback install silently dropping it left `self.secrets_store = None` possible, which would blow up much later in `init_extensions` with a less-actionable "secrets store not initialized" error. Changed to return `Result`; `install_ephemeral_secrets_store` propagates via `?` so startup aborts at the real root cause. Regression tests: - `llm::retry::tests::bad_gateway_without_retry_after_does_not_match_some_arm` (fix 1 — guards against the `Some(_)` match arm catching a None value) - `llm::retry::tests::test_is_retryable_classification` gains a `BadGateway { status: 500, .. }` case (fix 2) - `app::tests::ephemeral_secrets_store_is_constructible_and_usable` already exercised `.expect(...)` on the builder — now validates the `Result` contract (fix 3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway): typed orchestrator failure + preserve debug detail Addresses the remaining PR #2753 review feedback (Copilot + serrrfirat): - Introduce OrchestratorFailure / OrchestratorFailureKind typed enum in the engine's error module. Replaces the format!()-built `reason` that fed EngineError::Effect. Parse, start, resume, and NameLookup panic paths all route through the typed classifier — user-safe message via Display, raw detail preserved in `debug_detail`. - EngineError gains an Orchestrator(OrchestratorFailure) variant and a debug_detail() accessor. ThreadOutcome::Failed carries the detail through to the channel edge. - bridge/router.rs: new `gateway_debug_errors_enabled()` helper reads IRONCLAW_DEBUG_ERRORS and appends the preserved detail to the reply when on. Off by default — low-level detail still goes to tracing::debug. - Tighten the orchestrator timeout substring match from the bare "duration" to "timed out" / "timeout" / "duration limit" / "max_duration" / "maximum duration" so unrelated runtime errors no longer get misclassified as time-budget exhaustion. - doctor's check_secrets is now read-only: uses crate::secrets:: resolve_master_key (env + keychain only) instead of the auto- persisting SecretsConfig::resolve. Missing key reports as Skip without mutating ~/.ironclaw/.env. - Chat reload: turn_tool_calls_succeeded keys off the *trailing* tool call rather than every tool call in turn history, so a turn that errored once and recovered via a later successful retry no longer stays pinned to Processing forever. Regression tests: - failure_reason_does_not_treat_bare_duration_as_timeout - failure_reason_strips_python_traceback asserts debug_detail retains raw trace - test_reconcile_allows_recovery_from_earlier_tool_error Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(gateway): surface engine debug detail to Debug Inspector + logs Replaces the IRONCLAW_DEBUG_ERRORS env-var gate with unconditional visibility in the two places it actually belongs: the gateway's Debug Inspector panel and debug text logs. The chat reply stays sanitized. - Drop gateway_debug_errors_enabled() and the env-var-gated append in bridge_outcome_for_failed_thread. The flag was only there because the only delivery path was the chat reply, which can't carry raw detail. - Extend AppEvent::Error with an optional debug_detail field. Serialized onto the SSE `error` event so any listener (Debug Inspector, future tooling) sees it. - On ThreadOutcome::Failed, broadcast AppEvent::Error with {sanitized message, raw debug_detail, thread_id} so the inspector picks it up even though the chat reply is sanitized. - debug-panel.js renders debug_detail underneath the sanitized message on the Activity tab so operators can triage without tailing logs. - tracing::warn! on the failure path now includes debug_detail, which flows through log_layer into the gateway's log event stream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR #2753 follow-up review fixes Addresses four Copilot comments on commits3c08e0c3/042c2ee7: - orchestrator.rs: OrchestratorFailureKind::Other no longer renders the raw err_msg in Display. Channel-edge surfaces that bypass `user_facing_thread_failure` (runtime/mission.rs builds `format!("Mission failed: {error}")` directly) would have leaked tracebacks / internal file paths via unclassified Monty errors. User-facing text is now a generic "internal orchestrator failure"; the raw message is preserved on OrchestratorFailure::debug_detail as before. Dropped the now-unused `message` field on Other. - bridge/router.rs: the failure-path `warn!` now logs only `debug_detail_bytes`, not the full detail. Full raw text is emitted at `debug!` level so higher-severity logs don't carry multi-KB tracebacks. Operators still see the complete detail in the Debug Inspector (via AppEvent::Error.debug_detail) or with `RUST_LOG=ironclaw::bridge::router=debug`. - cli/doctor.rs: source_label had an unreachable KeySource::None arm. Since the key-present guard above already returned Skip, `source` is only ever Env or Keychain here — folded the match into the existing env-wins branch. Regression test renamed: `failure_reason_hides_unknown_raw_message_from_user_text` now asserts `Other`'s Display does not leak `NameError` while debug_detail still preserves it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR #2753 follow-up round 2 Addresses serrrfirat's review on commit82d06410— four issues that remained after the previous fix landed: - router.rs: a failed engine v2 thread on the web flow used to broadcast both AppEvent::Error on SSE and BridgeOutcome::Respond. GatewayChannel::respond then re-broadcast the same sanitized text as a response frame, so the browser rendered the same failure twice. The helper now takes sse_will_deliver_to_user and returns NoResponse when the originating channel is the gateway, so the SSE error card is the single user-visible surface. Non-gateway channels (telegram, relay, cli) still get Respond(sanitized) for primary delivery. - AppEvent::Error: debug_detail travelled on the default scoped SSE error event, where every authenticated consumer (chat UI, devtools, custom clients) sees it. Raw Monty tracebacks / upstream HTTP bodies must not cross that boundary. The field is removed from the wire payload; detail stays server-side via the existing tracing::debug! edge. The Debug Inspector now renders only the sanitized message. - doctor.rs: check_secrets probed the runtime via db::create_secrets_store, which opens a fresh backend and runs migrations — side-effectful, and not the same path that failed on hosted-TEE in #1537. The probe now uses connect_without_migrations + secrets::create_secrets_store(crypto, &handles), exercising the exact DatabaseHandles→Option<Arc<SecretsStore>> dispatch that AppBuilder::init_secrets runs. No migrations fire. - orchestrator.rs: the OrchestratorFailureKind::TimeLimit classifier caught any err_msg containing "timeout"/"timed out", so upstream LLM/network timeouts (Request timed out, Connection timed out) got mapped to TimeLimit and the user-facing message advised raising IRONCLAW_ORCHESTRATOR_MAX_DURATION_SECS — wrong remediation. The predicate set is narrowed to unmistakable Monty wall-clock markers (duration limit / max_duration / maximum duration / execution duration exceeded / orchestrator timed out). Upstream timeouts now fall through to Other. Regression tests: - failed_thread_outcome_is_no_response_when_sse_will_deliver locks in the single-surface contract for the gateway web flow. - failure_reason_does_not_treat_upstream_timeout_as_time_limit asserts four upstream-timeout shapes classify as Other (not TimeLimit) and their user message does NOT advise the budget knob. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR #2753 follow-up round 3 Addresses Copilot review comments on8489a978plus four review-derived nits surfaced during triage: - Update stale rehydration comment in `reconcile_in_progress_with_turns` to describe trailing-tool-call semantics (earlier failed attempts are allowed if a later retry succeeded) rather than the old "every recorded tool call completed successfully" wording. - Update stale rustdoc on `check_secrets` to describe the read-only `resolve_master_key()` probe instead of the dropped `SecretsConfig:: resolve` path that used to auto-generate keys. - Export `GATEWAY_CHANNEL_NAME` from `channels::web` and reference it from both the `Channel::name()` impl and `bridge::router`, eliminating the duplicated string literal. - Split `parse_retry_after` into two helpers. The existing `Option<&HeaderValue> -> Duration` stays for rate-limit callers (60s default on missing). New `parse_retry_after_value(&HeaderValue) -> Duration` is for 5xx paths that want to distinguish "absent" from "unparseable" so missing headers fall through to exponential backoff. - Strengthen doctor secrets tests: add `check_secrets_reports_env_source_when_env_key_is_set` which, under ENV_MUTEX, sets SECRETS_MASTER_KEY and asserts the rendered message surfaces the env source label plus the settings-vs-runtime drift warning — pinning the exact #1537 hosted-TEE axis the prior "doesn't panic" test couldn't detect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(doctor): allow await_holding_lock on env-guarded test `check_secrets_reports_env_source_when_env_key_is_set` holds the global `ENV_MUTEX` from `config::helpers::lock_env()` (a `std::sync::Mutex`) across `check_secrets(..).await`, which the `clippy::await_holding_lock` lint flags. The env vars the guard protects (`SECRETS_MASTER_KEY`) must stay pinned through the await because `check_secrets` reads them internally — dropping the guard early would let a concurrent test race on the env var. Mirrors the existing pattern in `bridge::auth_manager` (six existing sites). Local `cargo clippy --lib` missed this; CI runs with `--tests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>