mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-02 23:56:24 +08:00
staging
1325 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
firat.sertgoz
|
b0cb2335dc | fix(libsql): parse scientific notation cost aggregates (#3296) | ||
|
Nick Pismenkov
|
53e18ec4b2 |
fix: disable canary issues creation (#3119)
* fix: disable canara issues creation * ci: re-trigger regression-test-check after adding skip-regression-check label |
||
|
Henry Park
|
2a65da7c2a | chore: bump github tool and slack channel registry versions (#3057) | ||
|
Henry Park
|
8a6cbcf717 | test: update approval e2e expectations (#3054) | ||
|
Henry Park
|
8e54e51f62 |
fix(engine): centralize tool permission defaults (#3041)
* fix(engine): centralize tool permission defaults * fix(engine): preserve approval floors for v2 permissions * fix(engine): address permission review cleanup * fix(engine): avoid duplicate permission canonicalization |
||
|
Illia Polosukhin
|
7194808f11 |
fix(web): keep Routines tab after engine v1 → v2 upgrade (#2982) (#2992)
* fix(web): keep Routines tab after engine v1 → v2 upgrade (#2982) Users upgrading from a v1 install (e.g. 0.24.0 → 0.26.0) lost the UI affordance to view or manage existing routines: `applyEngineModeToTabs()` and `applyEngineModeUi()` unconditionally hid the v1-only Routines tab whenever ENGINE_V2 was enabled, even though the routines were still in the database and the API still served them. The fix adds a `userHasLegacyRoutines` flag, populated from `/api/routines/summary` on first gateway-status poll. The Routines tab stays visible (and `#/routines/<id>` still resolves to the legacy detail view) when the user has any v1 routines. Also fixes a wire-contract drift in `gateway-tee.js`: it read `data.engine_v2` for the activity store and `data.engine_v2_enabled` for the global, with `applyEngineModeUi()` running before the global was set. Per `.claude/rules/types.md` ("Wire-contract field naming"), the duplicate `engine_v2` field is removed from `GatewayStatusResponse`; the JS now reads the single canonical name once and sets the global before any UI helper consults it. * fix(web): address PR #2992 review notes — race guard, dedup, post-delete refresh Three review-driven hardening tweaks plus expanded Playwright coverage, all on the same #2982 fix: - gateway-tee.js: flip `engineModeApplied = true` synchronously so a second status poll firing while the first refresh is still in flight cannot kick off a duplicate `/api/routines/summary` request. The trailing `.then()` still runs on fetch failure (the `.catch()` chain resolves to undefined), so the UI still settles. - projects.js: route the routines-tab visibility branch through `shouldHideRoutinesTab()` instead of duplicating the predicate inline. Single source of truth for the rule. - routines.js: refresh `userHasLegacyRoutines` after a successful `deleteRoutine` so the v2 user who just removed their last legacy routine sees the tab fall back to hidden without a page reload. Playwright coverage grew from 5 to 11 cases: route-mocked summary, zero-total clears the flag, fetch failure preserves the prior value, post-delete refresh hides the tab, dual back-to-back first polls fan out only one summary fetch, and `restoreFromHash` routes correctly when legacy data exists. |
||
|
jinxin
|
2476672a5d | fix(web): surface NEAR AI session token to configure UI (#3014) | ||
|
Nick Pismenkov
|
5a5beec1c2 |
feat: canary report (#2874)
* fix(oauth): remove pending flow on provider-error callback The /oauth/callback handler's ?error= branch (RFC 6749 §4.1.2.1 provider-side failures — user cancels consent, scope denied, etc.) returned the error page immediately without removing the flow from ext_mgr.pending_oauth_flows(). The ghost entry then lingered until the 5-minute expiry sweep, and any subsequent auth dance for the same (extension, user) pair had to dedupe against it. Mirror the happy-path cleanup: decode the state param, remove the keyed flow, then return the error page. Surfaced during live-canary auth-full repro: after test_wasm_tool_oauth_provider_error_leaves_extension_unauthed ran, the stale flow sat in the shared auth_matrix_server fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): widen auth OAuth matrix timeouts for CI load Four tests in live-canary auth-full were failing in CI with `Page.wait_for_function: Timeout 60000ms exceeded`, `ClientConnectionError('Connection closed')`, and `Timed out waiting for OAuth refresh request` — all inside 60/20s deadlines that are tuned for a dev laptop and don't leave margin for ubuntu-latest's 2-vCPU runner under full suite load. Raise the per-call deadlines so the inner budgets fit comfortably inside pyproject.toml's 120s per-test cap: _wait_for_refresh_request default: 20.0s -> 60.0s _wait_for_auth_event call site: 60 -> 90 _wait_for_auth_prompt call site: 60 -> 90 send_chat_and_wait_for_terminal_message call sites: 60000 -> 90000 _wait_for_mock_google_tokens call site: 60.0 -> 90.0 _wait_for_response_contains (gmail) call site: 60.0 -> 90.0 Strictly widening; no passing test is slowed, no semantics change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(canary): Haiku-powered Slack report job Replace the team's raw Slack subscription (firehose of workflow notifications) with one curated per-run summary: Canary: 9 passed, 1 failed of 10 lanes ❌ auth-full (mock) — 12/13 passed, 1 failed in 350s > test_wasm_tool_first_chat_auth_attempt_emits_auth_url timed > out waiting for auth_required SSE event on the fresh thread tools: shell, http_request, gmail (~6 calls) ... commit `abc1234` • <github run link> New `canary-report` job (needs: every lane, if: always) downloads all lane artifacts, parses junit + summary + log tail per lane, and asks claude-haiku-4-5 to return a compact JSON per lane ({status, reason, tool_calls_total, tools_used, notable}). That's aggregated into a single Slack block message and posted via incoming webhook. Safety shape: - Script exits 0 even on Haiku/Slack failure so the notifier never masks the underlying canary signal. - Missing ANTHROPIC_API_KEY falls back to raw junit-only phrasing. - Slack POST failure falls back to plain-text "X/Y lanes failed" with the GH run URL so the channel still hears something. - No new Python deps — pure stdlib (urllib.request, xml.etree). - 20 KB log-tail cap per lane to keep Haiku token usage bounded. Secrets: - ANTHROPIC_API_KEY (already present, used by provider-matrix) - SLACK_WEBHOOK_URL (new — create an incoming webhook in Slack and add as repo secret; notifier prints to stdout otherwise) Testing: - Trigger manually via Actions -> "Live Canary" -> "Run workflow" with any single lane; canary-report runs after regardless of which lanes executed. - Run locally with --dry-run to preview the Slack payload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(canary): post_json error handling + robust Haiku JSON extraction Address gemini-code-assist review on scripts/live-canary/notify_slack.py: 1. `post_json` unreachable error branch: `urllib.request.urlopen` raises `urllib.error.HTTPError` for 4xx/5xx before reaching the `if resp.status >= 300` check, so the error body was never surfaced. Wrap in try/except and read the body from the HTTPError instance — that's where Anthropic's "invalid API key" / "rate limited" detail lives. 2. Haiku JSON extraction was fragile: `startswith("```")` assumed the response had no prose preamble and only handled one fence shape. Replace with `re.search(r"\{.*\}", text, re.DOTALL)` so we pick the outermost JSON object regardless of any wrapper markdown or leading/trailing text. Greedy + DOTALL is correct for the single top-level object our schema requires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): raise pytest timeout + bump multi-user chat wait to 180s The CI run on feat/canary-report surfaced that 90s was still not enough for test_mcp_same_server_multi_user_via_browser on ubuntu-latest — it timed out at the inner Playwright wait_for_function deadline with "Timeout 90000ms exceeded" after 118s of total test time. The test opens two browser contexts + two SSE streams and drives a full chat turn per user in sequence. Under 2-vCPU contention the compound pipeline genuinely takes over 90s. - tests/e2e/pyproject.toml: timeout 120 -> 240 (pytest-level cap) - test_v2_auth_oauth_matrix.py: send_chat_and_wait_for_terminal_message call sites 90000 -> 180000 (two owner/member turns, each budgeted for one runner-slow turn) 180s < 240s, so the inner deadline fires first with the useful Playwright traceback instead of the generic pytest SIGTERM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): fix pytest-timeout CLI override + widen Mode-C deadlines The previous commit ( |
||
|
jinxin
|
93d0305547 | fix(web): drop SSE plan_update/approval_needed events without thread_id (#2986) | ||
|
Henry Park
|
983a95cc98 |
Merge pull request #3002 from nearai/main
Main |
||
|
Henry Park
|
f11a49be0b |
[codex] Fix bridge restart approval floor (#2978)
* fix bridge restart approval floor * address bridge permission review cleanup |
||
|
Henry Park
|
91c4c7ca7b | fix: resolve v2 tool_info action inventory lookup (#2994) | ||
|
firat.sertgoz
|
56613ee763 |
docs(reborn): contract freeze review packet (#2983)
* docs(reborn): add contract freeze packet * docs(reborn): clarify implementation status in review packet * docs(reborn): clarify implementation status labels * docs(reborn): distinguish backend support from capabilities * docs(reborn): address contract review scope gaps * docs(reborn): sync contract updates with implementation * docs(reborn): clarify cutover dependency graph * docs(reborn): define kernel loop boundary * docs(reborn): refresh architecture map * docs(reborn): add product manager architecture guide * docs(reborn): diagram product manager guide |
||
|
Illia Polosukhin
|
e7d9922ce0 |
fix(engine): make mission threads_today reset timezone-aware (#2989)
* fix(engine): make mission threads_today reset timezone-aware The daily-budget reset added in #2570 compared `last_fire_at.date_naive()` against `now.date_naive()`, both in UTC. Cron missions configured with a non-UTC timezone (e.g. `America/Los_Angeles`) expect their budget to refresh at the user's local midnight, not at 00:00 UTC — under the old logic such a mission could stay stuck at "exhausted" for up to ~17 hours into the new local day. Extract the staleness check into `threads_today_is_stale(&mission)` and use the mission's cron timezone (when set) for the day boundary; UTC remains the fallback for manual / event-driven cadences and for cron missions without a configured timezone. Tests: - cron_mission_threads_today_resets_via_tick locks in the tick + cron path; the existing reset test only covered fire_on_system_event. - threads_today_resets_at_cron_local_midnight uses Pacific/Auckland to produce a `last_fire_at` that is yesterday-local but same UTC day, which the old logic would not have reset. - threads_today_is_stale_predicate covers the boundary helper directly. Fixes #1945 * review: address reviewer feedback on threads_today_is_stale - Inject `now: DateTime<Utc>` into `threads_today_is_stale` so the predicate is unit-testable against fixed instants and so the call site can pin a single timestamp across the staleness check and the cooldown check (Gemini, Copilot). - Capture `now` once at the top of the staleness/cooldown block in `fire_mission` and reuse it for the cooldown comparison so the two cannot disagree across a midnight tick. - Reword the helper doc to drop the hard-coded "5 PM local" claim, which varies under DST (Copilot). - Consolidate the prior wall-clock-based Auckland integration test into deterministic synthetic-instant cases inside `threads_today_is_stale_predicate`. The previous test could pass even when the timezone branch was disabled, depending on when of day it ran (Copilot). The new case asserts: same UTC date, but Auckland local dates straddle the boundary — exactly the regression the timezone branch fixes. - Document why `last_fire_at = None` with a non-zero counter must return `true` (recovery direction), not `false` — `false` would re-introduce the permanent-exhaustion bug this helper exists to fix. |
||
|
Robert Yan
|
4b6d52e501 | fix: incorrect ironclaw version in staging (#2981) | ||
|
Henry Park
|
7404e7d647 |
[codex] fix llm tool schema shaping for near ai (#2951)
* fix llm tool schema shaping for near ai * fix clippy in tool schema shaping * fix tool schema object-like handling * fix explicit non-object schema flattening * fix schema hint truncation handling |
||
|
Henry Park
|
2ef7d2c982 |
engine-v2: make available_actions callable-only for blocked providers (#2868)
* engine-v2: make available_actions callable-only for blocked providers * fix(engine): address review fixture tempdir leak (#2868) * engine-v2: refresh canonical prompt metadata on resume (#2869) * fix(engine): align prompt metadata refresh with resume state * fix(engine): finish prompt refresh compaction coverage (#2869) * fix(engine): preserve prompt refresh on resume (#2869) * Add engine v2 action discovery metadata (#2876) * Add engine v2 action discovery metadata * fix(engine): address action discovery review (#2876) * fix(engine): address follow-up review comments (#2876) * fix(engine): satisfy clippy in orchestrator lookup * fix(engine): propagate action snapshots in executor paths (#2876) * fix(bridge): restrict tool_info to callable actions (#2876) * [codex] Finish engine v2 deferred action inventory cleanup (#2889) * Add deferred action inventory groundwork * fix(engine): address deferred action inventory follow-up * fix(engine): address deferred inventory review feedback * test: fix fmt and clippy failures * engine-v2: trim unused callable discovery payload * tests: restore env vars in review-fix cases * engine-v2: populate callable snapshots consistently * Unify v2 integration enablement on tool_activate * engine-v2: tighten tool_info inventory and approvals * llm: normalize tool_info hint syntax * engine-v2: tighten tool_activate install approval lookup * tests: align gmail settings-first flow with approval contract * engine-v2: fix remaining tool surface review issues * engine-v2: restore auto-approve defaults * fix(engine): align v2 tool permissions with defaults * fix(engine): close v2 callable snapshot gaps * fix(bridge): label latent-only providers accurately |
||
|
jinxin
|
444bf6f4d1 |
fix(web): resolve empty “Fetch available models” result for NEAR AI in settings (#2890)
* fix(web): match subdomains of private-chat-stg.near.ai as NEAR AI private endpoint
* fix(web): remove LLM provider restart notices now that hot-reload is supported
LLM provider changes (switch/add/configure/update) now apply without a
restart, so the inline "Changes take effect after restart" banner in the
LLM Providers section, the mirrored banner in the Inference settings
panel, and the "(restart to apply)" suffix on the provider toasts are
all stale. Drops the HTML banner, the dynamic mirror in settings.js,
the three show-calls in config.js, the now-unused .config-notice CSS,
the config.restartNotice i18n key, and the suffix from providerConfigured
/ providerActivated / providerAdded / providerUpdated across en / zh-CN
/ ko. RESTART_REQUIRED_KEYS (embeddings, tunnel, gateway) is untouched —
those still require a restart.
* fix(web): avoid /v1/v1/models for NEAR AI private hosts with /v1 suffix
fetch_provider_models unconditionally appended /v1 for any NEAR AI
private host, so operators configuring a base URL that already ends in
/v1 (e.g. https://us.private-chat-stg.near.ai/v1) got /v1/v1/models and
404s from "Fetch available models". The Anthropic branch already had
the guard; the NEAR AI branch did not.
Extract models_endpoint_base(adapter, base) as a pure helper covering
both adapters, and swap the inline logic in fetch_provider_models for a
single call. Adds caller-level regression tests around the URL
construction path per .claude/rules/testing.md — the previous helper-
only tests on is_nearai_private_endpoint would have stayed green
through this bug.
* chore: minor
* fix(web): atomically switch llm_backend + selected_model on provider activation
setActiveProvider() was issuing two sequential PUTs — /api/settings/llm_backend
then /api/settings/selected_model. The settings handler hot-reloads the LLM
provider chain after each write, and config/llm.rs gives selected_model
precedence over provider defaults/overrides, so the first reload rebuilds the
chain with the new backend but the previous provider's model. If the second
request then fails, the instance stays stuck in that mixed state while the
success toast has already fired.
Route both writes through /api/settings/import instead: set_all_settings
commits the pair in one transaction and triggers a single reload, with
snapshot-based rollback of every key if the resulting chain fails to build.
Raised on the restart-notice removal PR (
|
||
|
firat.sertgoz
|
8898d3ea4f |
test(harness): add Phase 2 replay and gateway coverage (#2896)
* test(replay): add approval round-trip fixtures (Phase 2 of #2828) First fixture-driven Layer 1 (replay) coverage of the full v1 approval cycle: pause -> user resolution -> resume. Companion to the existing no_done_emitted_while_awaiting_approval test in e2e_response_order.rs, which covers the pause but not the resume. Three scenarios: - approval_yes: user approves -> tool runs once -> final LLM response - approval_no: user denies -> tool does NOT run -> agent surfaces a built-in rejection message (no follow-up LLM call, by design) - approval_always: allow-always on first call -> second call runs without re-prompting, exactly one ApprovalNeeded total Uses a test-only NeedsApprovalProbe tool with ApprovalRequirement::UnlessAutoApproved registered via TestRig::with_extra_tools, with auto_approve_tools(false) so the agent actually pauses for resolution. The deny-path discovery (no LLM follow-up on rejection) is documented in the test so future readers don't reintroduce the trailing text step. Updates tests/fixtures/llm_traces/README.md to list the new fixtures. Bumps approvals coverage in the harness-testing matrix from ~ to (closer to) full at Layer 1. * test(replay): expand approval coverage with 4 missing scenarios Adds the four approval scenarios that the original three-test set omitted, completing the state-space matrix across ApprovalRequirement variants, the master kill-switch config, and submission-routing edge cases. New tests (all in tests/e2e_approval_traces.rs): - always_requirement_ignores_allow_always_persistence ApprovalRequirement::Always is the unbypassable hard floor — even an 'allow-always' resolution must NOT skip the pause on subsequent calls of an Always-tool. Two pauses for two calls. - slash_approve_routes_as_approval_response '/approve' is parsed as Submission::ApprovalResponse even though bare 'yes' downgrades to UserInput when nothing is pending. Pins the divergent routing in submission.rs. - bare_yes_with_no_pending_approval_is_user_input Bare 'yes' with no pending approval must downgrade to UserInput and reach the LLM as a normal user message. Asserts the routing layer in agent_loop.rs performs the downgrade (parser is stateless). - config_auto_approve_bypasses_unless_auto_approved Agent-config auto_approve_tools=true is the master kill-switch — no ApprovalNeeded is ever emitted, even for UnlessAutoApproved tools. Also adds AlwaysApprovalProbe (mirrors NeedsApprovalProbe but returns ApprovalRequirement::Always) and three fixtures: - approval_always_floor.json - approval_slash.json - approval_bare_yes_no_pending.json README updated to list the new fixtures. Phase 2 of #2828. * test(replay): add auth-gate round-trip fixtures (Phase 2 of #2828) Five replay fixtures covering the engine v2 auth-gate state space: - auth_credential_provided: happy path (CredentialProvided -> resume) - auth_cancelled: user rejects (Cancelled -> resume) - auth_retry_invalid_then_valid: invalid credential, retry path - auth_external_callback: ExternalCallback submission path - auth_gate_request_id: AuthRequired populates request_id (v2 only) Probe tool: MockActivateTool (name "tool_activate") with scriptable output queue, installed via TestRegistry::replace_for_test to bypass PROTECTED_TOOL_NAMES. Planted minimal SKILL.md provides the credential spec needed by AuthManager's submit_auth_token path (otherwise the auth flow short-circuits with "Extension not installed"). Rig additions: - send_gate_auth_resolution(request_id, AuthGateResolution) - send_external_callback(request_id) - with_test_tool_override(tool) builder - TestChannel::channel_name / user_id accessors Serialization: all auth-gate tests share engine_v2_test_lock() (per-file static Mutex) because engine v2 uses a process-global OnceLock<RwLock<Option<EngineState>>>. Fixtures omit tools_used / all_tools_succeeded because engine v2 suppresses ToolStarted/ToolCompleted events when a tool output becomes a gate pause; verification uses the mock's internal execution counter instead. * test(router): cover auth fallback caller path (Phase 2 of #2828) * test(harness): add gateway-ops trace replay runner (#643, Phase 2 of #2828) Introduces Trace/TraceOperation/TraceExpectation types and TraceRunner that replays an ordered sequence of tool invocations against a libSQL test DB. The runner creates ActionRecords via the same save_action path gateway handlers use and matches outcomes against declared expectations. This is the inverse of the agentic TraceLlm harness: where TraceLlm replays an LLM stream and asserts the agent re-produces tool calls, TraceRunner replays caller-dispatched tool calls and asserts the Tool -> ActionRecord -> save_action pipeline matches expectations. Deliverables: - tests/support/trace_runner.rs: Trace, TraceOperation, TraceExpectation (Success { assertions } / Failure { error_contains }), TraceResult (with job_id for DB cross-checks), TraceFailure, TraceRunner with replay(). Assertion DSL supports eq / contains_text / fields (dot-path). - tests/e2e_gateway_trace_harness.rs: 7 integration tests covering echo roundtrip, idempotency, unknown-tool failure, mix assertions, forced mismatch detection, DB persistence via get_job_actions, and cross-run determinism. - tests/fixtures/gateway_traces/: 4 JSON fixtures + README documenting the wire format and the deferred settings_* / extension_* roadmap (blocked on #640 and network-stub work respectively). Pitfalls addressed: - Parent agent_jobs row is created via save_job before the first save_action; job_actions.job_id has a FK to agent_jobs(id) ON DELETE CASCADE that would otherwise fail. - Deterministic-field check in the determinism test excludes id / executed_at / duration (intentionally variable across replays). - ToolError has no NotFound variant; missing-tool lookups are reported via ExecutionFailed("tool not registered: {name}") so Failure expectations can substring-match on "not registered". * fix: address review findings (iteration 1) |
||
|
Octopus
|
c721bbc26c |
docs: update minimum Rust version to 1.92 in README (fixes #2898) (#2931)
Cargo.toml already enforces rust-version = "1.92" via the workspace manifest, but README.md still advertised "Rust 1.85+". Users running rustc 1.85 or 1.91 get a confusing build failure from wasmtime's MSRV requirement. Align the documented prerequisite with the enforced one. Fixes #2898 Co-authored-by: octo-patch <octo-patch@github.com> |
||
|
Illia Polosukhin
|
eb75a62a97 |
feat(debug-panel): expand Activity tab coverage with CodeAct + warnings (#2850)
* feat(debug-panel): expand Activity tab coverage with CodeAct + warnings The Activity tab was missing most event types: CodeAct runs showed only lossy chat summaries, WARN/ERROR logs only landed in server stdout, and tool entries hid their parameters on success. - Emit AppEvent::CodeExecuted (verbose-only) with raw code, stdout, and return value from the engine orchestrator so observers see what the model actually wrote. - Bridge WARN/ERROR tracing into AppEvent::Warning via spawn_warning_bridge, scoped by owner_id in multi-tenant mode to prevent cross-tenant log bleed. - Backfill params_summary on ActionExecuted/ActionFailed events from structured + scripting executors so the Activity tab shows tool args immediately (not just on failure) without waiting for tool_completed. - Wire debug-panel.js to render code_executed, warning, gate_required, gate_resolved, approval_needed, skill_activated, plan_update, thread_state_changed, child/mission_thread_spawned, onboarding_state, image_generated, suggestions, and the full sandbox-job event family. - Extract shared on(name, handler) wrapper to dedupe ~25 copies of the JSON-parse + reconnect-counter housekeeping and keep lastEventTime bookkeeping consistent across listeners. - Add i18n strings (en/ko/zh-CN) and CSS icon colors for the new activity types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(debug-panel): address review feedback on activity-trace PR - summarize_params generic fallback: skip sensitive-looking parameter keys (token/secret/password/api_key/auth/credential/bearer) so MCP and unknown-tool calls can't surface secret values into ActionExecuted events or debug-panel SSE. Adds two regression tests. - Cap CodeExecuted code/stdout at 8_000 chars (tail-last) before emission so a step that prints a large blob can't bloat persisted thread events. Matches the existing scripting OUTPUT_TRUNCATE_LEN. - await_thread_outcome: skip broadcasting verbose-only AppEvents when no debug subscriber is connected — mirrors the send_status gate and keeps CodeExecuted off the shared SSE broadcast buffer for normal browser tabs. - spawn_warning_bridge: same short-circuit on has_verbose_receivers. - debug-panel.js: introduce GATE_RESOLUTION_STATUS so `expired` (a failure path from router.rs) no longer renders as a green success badge; shared STATUS_TO_ACTIVITY map is kept for jobs/ plans/onboarding where `success` is the right default. - debug-panel.js: migrate the remaining legacy listeners to the shared on() wrapper so lastEventTime / totalEventsReceived bookkeeping stays consistent across every activity listener. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(debug-panel): address PR #2850 follow-up review on leak / tenant scoping - Warning bridge (`src/channels/web/mod.rs`): disable entirely in multi-tenant mode. The `tracing` layer captures log context at the global subscriber scope, not at request scope, so scoping the bridge to the gateway `owner_id` misroutes tenant A's WARN/ERROR log lines to the admin account (and prevents tenant A from ever seeing them). Per-request provenance would need threading through every `warn!` / `error!` call site — out of scope for this PR — so the safe move is to keep the bridge off until that lands. - `summarize_params` (`crates/ironclaw_engine/src/types/event.rs`): strip URL query strings / fragments / userinfo for `http` and `web_fetch`, and redact auth-bearing flag values (`-H`, `--header`, `-u`, `--user`, `--token`, `--api-key`, `--password`, `--auth`, `--bearer`) plus embedded URL query strings inside `shell` commands. Signed URLs, inline `Authorization: Bearer …` headers, and query- string API keys no longer reach `ToolCompleted.parameters` on the debug SSE stream. Six regression tests added. - `CodeExecuted` redaction (`src/bridge/router.rs`): apply the leak detector to `code` / `stdout` / `return_value` at the bridge boundary before SSE broadcast. The engine crate has no dependency on `ironclaw_safety`, so scrubbing lives here. Adds `SafetyLayer::leak_detector()` and `EffectBridgeAdapter::safety()` accessors. Handles both `Redact` and `Block`-action matches (scan_and_clean's `redacted_content` is `None` for Block-only matches, which would have passed bearer tokens / API keys through unchanged). Regression test covers string and nested-JSON cases. * fix(debug-panel): address PR #2850 Copilot follow-up review - `src/channels/web/log_layer.rs`: annotate `spawn_warning_bridge`'s `sse.broadcast_for_user` / `sse.broadcast` sites with `// projection-exempt: log source, WARN/ERROR tracing bridge → AppEvent::Warning` so the PROJECTION safety check (#9 in `scripts/pre-commit-safety.sh`) recognises the tracing `LogBroadcaster` as a typed source log. Added a comment block explaining why the source-log category isn't yet in `.claude/rules/gateway-events.md`'s table. - `crates/ironclaw_engine/src/executor/orchestrator.rs`: replace `tail_chars` (O(n) via `chars().count()`) with a local `tail_utf8_bytes` helper for the `CodeExecuted` emission path. Byte based so it stays O(1) + ≤3-byte UTF-8 boundary walk for arbitrarily large `code`/`stdout`. Also add `bounded_return_value` so a CodeAct snippet returning a 50 MB JSON value doesn't bloat persisted thread events — strings are tail-truncated; structured values that serialize past 8 KiB are dropped to `None` (rather than truncated into unparseable JSON). Seven regression tests cover ASCII / emoji boundary / null / small struct / oversized struct / large-string paths. `tail_chars` is kept unchanged for its existing callers, whose inputs are already bounded (`OUTPUT_TRUNCATE_LEN`, 500-char error slices). --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
1a44a9442e | chore(engine): bump Monty to v0.0.16 (#2784) | ||
|
firat.sertgoz
|
2d4b35daa9 | feat(missions): redesign missions overview surface (#2894) | ||
|
Pranav Raja
|
49f3e8d566 |
feat(credentials): path-based credential matching for per-endpoint auth (#2168)
* feat(credentials): path-based credential matching for per-endpoint auth Add `path_patterns` field to `CredentialMapping` to scope credentials to specific URL path prefixes on a host. When set, the request path must match a prefix at a segment boundary (`/` or `?`). When empty (default), credentials match all paths on the host — fully backwards compatible. Key changes: - `CredentialMapping.matches(host, path)` with segment-boundary enforcement - `path_matches_prefix()` rejects `..` traversal, normalizes trailing slashes - `host_matches_pattern()` deduplicated to single source in secrets/types.rs, case-insensitive per RFC 4343 - HTTP tool uses `find_for_url(host, path)` for path-aware credential lookup - Auth manager pre-flight check uses path-aware `find_for_url` - WASM tool/channel wrappers carry `path_patterns` through to injection time - `CredentialMappingSchema`, `SkillCredentialSpec` support `path_patterns` Tests cover segment-boundary attacks, path traversal rejection, case-insensitive host matching, path-scoped injection, and different credentials for different paths on the same host. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(credentials): address PR #2168 review feedback - Narrow `..` rejection in path_matches_prefix to per-segment so legitimate paths like /api/..config are no longer falsely blocked (path_matches_prefix was using path.contains("..")). - Path-scope credential injection in the channels WASM wrapper: ResolvedHostCredential now carries path_patterns and inject_host_credentials honors it, matching the tools-side wrapper. - Add path-aware CredentialInjector API (find_credentials_for_url / inject_for_url); deprecate the host-only variants and SharedCredentialRegistry::find_for_host with #[deprecated] attrs. - Tighten CredentialMappingSchema.path_patterns from Option<Vec<String>> to #[serde(default)] Vec<String>, matching sibling types. - Validate path_patterns in validate_credential_spec: require leading '/', reject empty, reject '..' as a segment. - Expand comment in http.rs documenting why LLM-header blocking is host-scoped (exfil defense) while injection is path-scoped (minimum privilege). Tests: +5 validation cases, +2 injector cases, +2 channel wrapper cases, +2 path_matches_prefix cases covering dot-dot-inside-segment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(credentials): address PR #2168 round-3 review - secrets/types: reject %2e / %2E in paths (percent-encoded traversal bypass for servers that decode before routing, e.g. IIS/Tomcat) - sandbox/proxy/policy: find_credential now honors path_patterns via CredentialMapping::matches, using request.path (regression test added) - wasm wrappers: extract shared extract_url_path_for_matching helper in secrets/types, with tracing::debug! on URL parse failure; removes duplicated 12-line block between tools/wasm and channels/wasm - ironclaw_skills/validation: factor validate_path_pattern out of validate_credential_spec and reject '?' and '#' in path_patterns (Url::path() strips them, so these silently never match) - tools/wasm/capabilities_schema: plumb the same validate_path_pattern through the WASM manifest loader — bad patterns log as warnings instead of silently failing to match - tools/builtin/http: unit tests for extract_path_from_params (valid, missing url, query+fragment stripping, bare host, malformed) - tests/skill_credential_injection: caller-level tests driving HttpTool::execute with path-scoped credentials — segment boundary and auth-gap-on-non-matching-path (per .claude/rules/testing.md "Test Through the Caller, Not Just the Helper") Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(credentials): address PR #2168 round-4 review - secrets/types: path_matches_prefix now decodes each segment and rejects only dot-segments (literal . or .., plus percent-encoded equivalents like %2e, %2e%2e, mixed case, .%2e, %2e.). Legitimate literal paths with embedded encoded dots — /files/foo%2ebar → foo.bar, /releases/v1%2e2 → v1.2 — are now allowed. Replaces the previous "reject any %2e substring" rule which over-rejected normal filenames. (Firat #3125964627) - secrets/types: add match_specificity(path_patterns, req_path) returning the length of the longest matching prefix (0 if unscoped). Exported pub(crate) for callers that need deterministic credential precedence. - credential_injector + both wasm wrappers: sort matching credentials by ascending path specificity, tie-broken alphabetically on secret_name, before the last-write-wins header merge. The most-specific mapping now wins any header conflict regardless of HashMap iteration order, which fixes nondeterministic winner selection on overlapping mappings. ResolvedHostCredential gains a secret_name field purely for stable tie-breaks (no secret material exposed). (Firat #3125963270) - bridge/auth_manager::check_http_auth: replace "return Ready on first resolved mapping" short-circuit with conjunctive evaluation — every non-optional matched mapping must resolve for Ready. Optional mappings are skipped. Missing required credentials are accumulated and returned as MissingCredentials so endpoints needing bearer + org-header surface the auth gate instead of failing at the wire with a raw 401. (Firat #3125963977) - tools/builtin/http::execute: stop clearing missing_credential on peer success and drop the !injected_any_credential guard. Track the first missing required credential for the 401/403 remediation UX; skip optional mappings. Matches the new auth_manager behavior. Regression tests: - secrets/types: path_matches_prefix_rejects_percent_encoded_dot_segments (adds .%2e and %2e. mixed-form cases), path_matches_prefix_allows_legit_ embedded_encoded_dot (foo%2ebar, v1%2e2), match_specificity_ranks_ longer_prefixes_higher. - tools/wasm/wrapper: test_inject_host_credentials_most_specific_path_wins verifies the sort is order-independent by constructing the same creds in both orders and asserting the specific one wins. - bridge/auth_manager: check_http_conjunctive_auth_any_missing_required_ raises_gate, check_http_conjunctive_auth_all_required_resolved_is_ready. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: cargo fmt after round-3/round-4 review fixes Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: untrack accidentally-committed local scratch files Follow-up to |
||
|
firat.sertgoz
|
1d8a46bbdf |
fix(bridge): surface latent WASM provider actions to the LLM (#2883) (#2891)
* fix(bridge): surface latent WASM provider actions to the LLM (#2883)
After
|
||
|
Pierre LE GUEN
|
0892f56af9 |
fix(wasm): remove stale 10M fuel limit from settings DB (#2851)
* fix(wasm): remove stale 10M fuel limit from settings DB Databases that persisted `wasm.default_fuel_limit = 10000000` before the code default was bumped to 500M (limits.rs, config/wasm.rs) still read the old value at startup because DB settings take priority over code defaults. This caused WASM tools like google_slides to fail with "Fuel exhausted: execution exceeded 10000000 fuel units" even though the code default is 500M. Add migration V25 (both PostgreSQL and libSQL) that deletes the stale setting row when its value is <= 10M, so the 500M code default takes effect. Users who intentionally set a custom limit above 10M are unaffected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: trigger fresh run with skip-regression-check label [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(wasm): extract JSONB scalar before cast, narrow to exact match (#2851) Address review feedback: - PostgreSQL: use (value#>>'{}')::BIGINT to extract JSONB scalar as text before casting, preventing runtime errors on JSONB columns - libSQL: use json_extract(value, '$') for equivalent JSON extraction - Narrow predicate from <= to = 10000000 to avoid deleting intentionally lowered custom fuel limits Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: serrrfirat <f@nuff.tech> |
||
|
firat.sertgoz
|
b5ba7496f0 |
fix(engine): enforce tool use for stop/pause/cancel commands (#2814)
* fix(engine): enforce tool use for stop/pause/cancel commands (#2808) The LLM was narrating about calling mission_pause/mission_list instead of actually executing them because neither the tool-intent nudge nor the execution obligation recognized stop/pause/cancel as action commands. - Add stop/pause/cancel/halt/disable to signals_tool_intent ACTION_VERBS so the nudge fires when the LLM says "I'll pause the mission" - Add stop/pause/cancel phrases to signals_execution_intent EXEC_PHRASES so the obligation system forces tool calls for "stop it", "pause the X" - Add bare imperative detection (startswith) for "stop", "stop pinging", "pause", "cancel" — avoids false positives like "I can't stop" - Add 5 regression tests covering true positives and false negatives Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review findings (iteration 1) - Add missing "please halt " to EXEC_PHRASES for consistency with please stop/pause/cancel - Strip trailing punctuation from bare commands so "Stop." and "cancel!" are detected - Add 2 regression tests covering both fixes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(engine): address gemini-code-assist review — halt/disable consistency (#2814) - Add "halt it/that/this/the" and "disable it/that/this/the" to EXEC_PHRASES for consistency with signals_tool_intent - Add "please disable " to polite execution phrases - Add "disable" to BARE_COMMANDS and IMPERATIVE_STARTS - Add regression test for halt/disable execution intent phrases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
009d3cd82f |
fix(web): use conversation-only chat sidebar (#2867)
* fix(web): use conversation-only chat sidebar * fix(ci): use unwrap_or_default() for clippy compliance Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Henry Park
|
d33fecb17c |
engine-v2: centralize action vs capability surface policy (#2827)
* Add canonical engine capability status enum * Add bridge tool surface assignment policy * fix(engine): tighten scoped surface assignment * fix(bridge): remove premature approval_gated field, surface ReadyScoped in capabilities - Remove approval_gated from SurfacePolicyInput (YAGNI until policy uses it) - Change ReadyScoped fallback from neither() to capabilities_only() so scoped subjects remain visible in background context - Update tests to match new behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): unblock section 2 policy PR * engine-v2: add capability projection and two-surface prompt baseline (#2826) * Add capability projection and two-surface prompt baseline * Reduce step-context args for clippy-clean two-surface stack * fix(engine): address two-surface review follow-ups * fix(engine): normalize alias-aware capability projection * fix(bridge): share extension fetch between projectors, preserve NeedsAuth in actions - Fetch list_capability_extensions once in EffectBridgeAdapter and pass to both ActionProjector and CapabilityProjector via prefetched_extensions - Keep NeedsAuth provider tools in available_actions so the LLM can trigger auth gates by attempting to call them - Add unit tests for NeedsAuth preservation and latent tool omission at the ActionProjector level where extension maps can be controlled Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: serrrfirat <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: serrrfirat <f@nuff.tech> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
c559a58810 |
feat(bridge): project 7 more engine events to AppEvents (#2844)
* feat(bridge): project 7 more engine events to AppEvents Second slice of #2654 bridge-coverage under #2792 Phase 1. Builds on #2797 (StepFailed, ChildCompleted, CodeExecutionFailed) by closing the remaining cheap `EventKind` drops: | `EventKind` | `AppEvent` | |---|---| | `LeaseGranted { lease_id, capability_name }` | new `LeaseGranted` | | `LeaseRevoked { lease_id, reason }` | new `LeaseRevoked` | | `LeaseExpired { lease_id }` | new `LeaseExpired` | | `SelfImprovementStarted` | new `SelfImprovement { phase: Started, .. }` | | `SelfImprovementComplete { prompt_updated, patterns_added }` | `SelfImprovement { phase: Complete, prompt_updated, patterns_added, .. }` | | `SelfImprovementFailed { error }` | `SelfImprovement { phase: Failed, error, .. }` | | `OrchestratorRollback { from, to, reason }` | new `OrchestratorRollback` | The three engine `SelfImprovement*` variants collapse into one wire event with a `SelfImprovementPhase` discriminator — consumers need one handler, variant-specific data is conveyed via optional phase-scoped fields. Following the types.md "Wire-stable enums" pattern — the phase enum is snake_case serde, not a stringly-typed `status` field. The lease events are security-visible: capability grants, revocations, and expiries should be auditable on the UI stream. `LeaseExpired` in particular closes a "tools start failing after TTL with no visible reason" gap. Also: - Adds `impl fmt::Display for LeaseId` in the engine alongside the existing `ThreadId` / `ProjectId` impls. The bridge code stringifies `LeaseId` for the wire; missing `Display` blocked the first compile. - Cleans up a stray doc-comment misplacement from #2797 where the `thread_event_to_app_events` docstring was attached to `code_execution_category_to_wire`. Regression tests mirror the #2797 pattern — one per representative arm (lease grant for the lease family, self-improvement complete for the richest phase, orchestrator rollback). The two remaining lease variants and two remaining self-improvement phases are covered by the existing `event_type_matches_serde_type_field` drift-catch test. Approval pair (`EventKind::ApprovalRequested` / `ApprovalReceived`) is still deferred — they need to land together with the gate-manager migration in Phase 1 PR 3 to avoid duplicate-emit with the direct `GateRequired` / `GateResolved` broadcasts. Refs: #2792, #2654 * refactor(bridge): typed SelfImprovementPhase + exhaustive match Addresses two Gemini review comments on #2844. **1. `SelfImprovementPhase` as a typed internally-tagged enum.** Previously the `AppEvent::SelfImprovement` variant carried three `Option<T>` fields (`prompt_updated`, `patterns_added`, `error`), only some of which were populated per phase. Per `.claude/rules/types.md` — and the reviewer's note — this is an `Option`-that-can-lie pattern the type system should rule out. Phase-specific data now lives on the variant: ```rust enum SelfImprovementPhase { Started, Complete { prompt_updated: bool, patterns_added: usize }, Failed { error: String }, } ``` Wire shape is preserved via `#[serde(tag = "phase")]` on the enum and `#[serde(flatten)]` on the `AppEvent::SelfImprovement.phase` field — JSON still looks like a flat object: `{"type": "self_improvement", "phase": "complete", "prompt_updated": true, ...}`. **2. Exhaustive `thread_event_to_app_events` match.** Dropped the `_ => vec![]` wildcard in favour of explicit arms for every `EventKind` variant. Deferred-bridge variants get `vec![]` with a comment naming the migration plan: - `ApprovalRequested` / `ApprovalReceived` → waiting on the gate manager migration in #2792 Phase 1 PR 3 to avoid duplicate-emit with the existing direct `GateRequired` / `GateResolved` broadcasts. - `Unknown` → forward-compat catch-all in the engine enum; nothing useful to project from a variant written by a newer binary during a rolling deploy. New engine variants now fail the bridge to compile, which is exactly what the state-convergence epic (#2792) needs — no more silent drops. Refs: #2792, #2844 review * fix(bridge): sanitize OrchestratorRollback.reason before SSE projection `EventKind::OrchestratorRollback.reason` originates from `format!("execution failed: {e}")` in `crates/ironclaw_engine/src/executor/loop_engine.rs:327`, where `e: EngineError`. Variants like `Store { reason }` and `Llm { reason }` render DB connection strings, file paths, and raw upstream HTTP bodies — all of which reached every authenticated SSE consumer verbatim through the new `AppEvent::OrchestratorRollback` projection. Route the reason through a new `user_facing_rollback_reason` classifier that maps the existing `FailureCategory` taxonomy to short operator-facing messages (`"LLM provider unavailable"`, `"execution failed"`, etc.). The raw text still lives in the `debug!` log for operator triage, matching the pattern already used for `AppEvent::Error`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(deny): ignore RUSTSEC-2026-0104 (rustls-webpki CRL panic) Same transitive pin as 0049/0098/0099 — rustls-webpki 0.102.8 is held by libsql 0.6.0 → rustls 0.22 → hyper-rustls 0.25. The advisory explicitly notes that applications not parsing CRLs are unaffected; we do not parse CRLs. [skip-regression-check] — deny.toml-only config change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(bridge): cover 4 new engine→AppEvent arms; sharpen rollback test Review follow-ups on PR #2844: - Add unit tests for `LeaseRevoked`, `LeaseExpired`, `SelfImprovementStarted`, and `SelfImprovementFailed` bridge arms — only `LeaseGranted` / `SelfImprovementComplete` / `OrchestratorRollback` had coverage before, leaving four new projections untested. - Rewrite `rollback_reason_drops_engine_error_detail` to drive the sanitiser with two unrelated leaky inputs and assert identical outputs (`execution failed`). The load-bearing check is input-independence; `!contains` probes remain as sentinel sniffs for the specific leak shapes. Avoids classifier-triggering tokens (no `upstream`, no `http 5xx`) so both inputs fall through to `Unknown`. - Reword the `ApprovalRequested` / `ApprovalReceived` comment: they are temporarily suppressed pending the gate-manager migration, not permanently dropped. The bridge will eventually map them here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
22ff4957c9 |
feat(safety): projection-exempt lint for gateway event sources (#2840)
* feat(safety): projection-exempt lint for gateway event sources Phase 1 of the gateway state-convergence epic (#2792): add check #9 to `scripts/pre-commit-safety.sh` that flags newly-added `sse.broadcast(` / `sse.broadcast_for_user(` calls without a `// projection-exempt: <reason>` annotation on the same line. The invariant is documented in the new `.claude/rules/gateway-events.md`: - Every `AppEvent` must project from a typed source log (engine `EventKind`, sandbox `JobEvent`, or a channel-lifecycle log). - A short transport-only allowlist (`Heartbeat`, `StreamChunk`) covers the ephemeral variants with no state backing them. - Direct emits are the root cause of the state-drift class — UI stream and replayable source end up with different stories. Four recent incidents (#2654, #2534, #2731, #2079) share this shape. The lint is diff-based, so pre-existing unannotated call sites aren't broken. Baseline annotation of the ~20 existing emit sites is the next PR under Phase 1 — this one establishes the gate. Suppressions require a named category (`bridge dispatcher`, `channel-lifecycle`, `sandbox JobEvent`, `transport-only, heartbeat`, or `migrate in #NNNN`). An unnamed `legacy` reason is rejected by review, not by the lint itself. Tested locally: - Fires on unannotated `sse.broadcast(...)` in a new file. - Suppressed by `// projection-exempt: transport-only, heartbeat`. - Does not match `Channel::broadcast` (different trait). - Does not match calls inside `#[cfg(test)] mod tests` blocks (via the shared `strip_test_mod_lines` filter). Refs: #2792, #2654 * refactor(safety): address review feedback on projection-exempt check Four review comments from Copilot and Gemini on #2840: 1. **Match rustfmt's method-chain wrapping.** The original regex only caught same-line `sse.broadcast(...)`. Long calls like `state\n .sse\n .broadcast_for_user(...)` — produced by rustfmt and already in-tree at `src/channels/web/features/extensions/mod.rs:645` — would bypass the check. New matcher adds a dangling-method alternation that catches `.broadcast_for_user(` at line start. Only the `_for_user` suffix (SseManager-unique) is matched in dangling form; bare `.broadcast(` can be `Channel::broadcast` trait, which is intentionally out of scope. 2. **Enforce the documented annotation format.** The check previously accepted any `// projection-exempt:` comment, including bare `// projection-exempt: legacy` that the rule doc explicitly forbids. Negative filter now requires `<category>, <detail>` — presence of a comma separating the category from the detail. 3. **Point at the real path in the warning.** Replace `bridge::thread_event_to_app_events` with `thread_event_to_app_events` in `src/bridge/router.rs` — the actual file location. 4. **Update suppression hint** to show the `<category>, <detail>` format rather than the generic `<reason>`. Verified against a 6-case fixture (same-line fire + suppress, dangling-chain fire + suppress, unnamed-category fire, `Channel::broadcast` silent). Refs: #2792, #2840 review * fix(safety): match header exclusion against grep -n prefixed output After `grep -nE '^\+'`, every line is prefixed with `N:`, so the `^\+\+\+` anchor for filtering diff header lines (`+++ b/file.rs`) never fires. The positive patterns already exclude header lines by shape, so today this is harmless — but the dead branch masks future defense-in-depth failures if the template is reused with a less specific positive match. Replace `^\+\+\+` with `:\+\+\+ ` in DISPATCH, CREDNAME, and PROJECTION checks so the exclusion works against the `grep -n` output shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(safety): regression for grep-n-prefixed header exclusion Covers PROJECTION / DISPATCH / CREDNAME pipelines: - diff header lines (`+++ b/path`) are filtered after `grep -n` - real broadcast/state/CredentialName lines are still flagged - `// projection-exempt: <category>, <detail>` exempts - bare `// projection-exempt: legacy` (no comma) is not exempt Locks in that `:\+\+\+ ` (matches the `grep -n` prefixed shape) behaves as intended, where the prior `^\+\+\+` anchor silently never fired. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(deny): ignore RUSTSEC-2026-0104 (rustls-webpki CRL panic) Same transitive pin as 0049/0098/0099 — rustls-webpki 0.102.8 is held by libsql 0.6.0 → rustls 0.22 → hyper-rustls 0.25. The advisory explicitly notes that applications not parsing CRLs are unaffected; we do not parse CRLs. [skip-regression-check] — deny.toml-only config change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(safety): portable grep boundary + broadened broadcast_for_user match Two PROJECTION bypass paths flagged in review: 1. `\b` is a GNU-grep extension (works in grep 3.x, not portable to BSD grep on macOS dev envs) — replace with `(^|[^[:alnum:]_])sse\.` so the check fires uniformly across `grep -E` implementations. 2. `broadcast_for_user(...)` on a non-`sse` receiver (e.g. `manager.broadcast_for_user(...)`) previously slipped through. The method is defined only on `SseManager` (`src/channels/web/platform/sse.rs:144`), so matching `\.broadcast_for_user\(` on any receiver is safe and makes the enforcement match the documented rule. Regression tests extended: chained-receiver, non-`sse` receiver, bare `sse.broadcast(`, and a portable-boundary negative case (identifier ending in `sse`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(gateway-events): align matcher description with broadened check Update the enforcement section to describe the two current PROJECTION matcher shapes after the review follow-up in the preceding commit: 1. Any-receiver `.broadcast_for_user(...)` — catches the non-`sse` receiver bypass and rustfmt wraps alike. 2. `<word-boundary>sse.broadcast(...)` with a portable boundary (`(^|[^[:alnum:]_])`), which is needed because `grep -E`'s `\b` is a GNU extension and not available on BSD grep. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(safety): tighten CREDNAME + projection-exempt lints, sync header Three follow-ups from the review: 1. CREDNAME portability — `\bCredentialName\b` used GNU-grep `\b`, which BSD grep does not recognise. Replace with the same `(^|[^[:alnum:]_])…([^[:alnum:]_]|$)` boundary used for PROJECTION and matches cleanly across GNU and BSD `grep -E`. 2. Empty-detail suppression bypass — `// projection-exempt: [^,]+,` accepted `// projection-exempt: foo,` (empty detail) as exempt even though `.claude/rules/gateway-events.md` requires a non-empty detail. Tighten to `[^,]+,[[:space:]]*[^[:space:]]` so a comma without a trailing token still fires the check. 3. Header suppression hint (`#24`) said `// projection-exempt: <reason>` — update to `<category>, <detail>` to match what the check actually accepts so contributors don't copy an unsupported format. Regression tests extended: `PROJECTION: empty detail after comma still flagged`, `PROJECTION: comma + whitespace-only detail still flagged`, `CREDNAME: CredentialNameExt (different type) is not flagged`. All 16 cases pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
lycheepuppy
|
41c73878eb |
fix(security): zip bomb denial of service in document extraction [MEDIUM] (#2093)
* fix(security): add decompressed size limits to ZIP-based document extraction The document extraction pipeline (DOCX, PPTX, XLSX) opened ZIP archives and read individual entries fully into memory with read_to_string() without any decompressed size limit. While a 10 MB limit (MAX_DOCUMENT_SIZE) was enforced on the compressed input, a zip bomb — a small compressed file that expands to an extremely large decompressed size — could pass the input check but decompress to gigabytes of XML, causing an OOM condition that crashes all active sessions. ZIP achieves compression ratios of 1000:1+ for repetitive XML data. A 10 MB compressed file could decompress to 10+ GB. The existing MAX_EXTRACTED_TEXT_LEN trim (100K chars) is applied after all entries are fully decompressed, so it cannot prevent the OOM. Changes: - Add bounded_read_zip_entry() helper that checks the declared uncompressed size of each entry against MAX_DECOMPRESSED_ENTRY (50 MB) and tracks cumulative size against MAX_DECOMPRESSED_TOTAL (100 MB) - Use take() as defense-in-depth against archives that lie about their entry sizes - Apply bounded reads to all three extractors: extract_pptx, extract_xlsx, and extract_office_xml (used by DOCX) - Add regression tests verifying bounded reads work correctly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): track actual decompressed bytes, not ZIP header metadata Address review feedback on #2093: - bounded_read_zip_entry now tracks actual bytes read (xml.len()) instead of trusting the ZIP header's declared uncompressed size - Fail closed when bounded reader hits the per-entry cap - Regression tests now exercise real rejection boundaries: actual byte accounting, cross-entry accumulation, and budget exhaustion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): use typed errors and pre-check cumulative budget in ZIP decompression Replace generic string errors with ExtractionError enum (TotalSizeLimitExceeded, EntryReadFailed) and add a pre-check that rejects entries whose header-declared size would exceed MAX_DECOMPRESSED_TOTAL before decompressing. The post-read check still uses actual bytes (xml.len()) so a lying header cannot bypass the cumulative limit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add per-entry truncation tests and configurable limits for zip bomb defense Refactors bounded_read_zip_entry into a configurable inner function (bounded_read_zip_entry_with_limits) so tests can exercise the critical defense paths without creating 50MB fixtures. Adds EntryTooLarge error variant to distinguish per-entry vs cumulative limit violations. New tests: - Per-entry truncation/fail-closed path (the actual zip bomb defense) - Per-entry pre-check rejection on declared header size - Cumulative total budget exhaustion across multiple entries - Caller-level: extract_office_xml rejects oversized DOCX entry - Caller-level: extract_pptx rejects oversized slide Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Wui <wui@Wui-Work-2.local> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |
||
|
Henry Park
|
9d651ea366 |
engine-v2: add canonical capability status vocabulary (#2825)
* Add canonical engine capability status enum * refactor(engine): add hash support for capability status |
||
|
Illia Polosukhin
|
417ee611df |
docs(plan): update engine v2 architecture to match verified reality (#2801)
* docs(plan): update engine v2 architecture plan to reflect verified reality The plan doc claimed several items as missing/pending that are already implemented. Update to match ground truth so future readers don't redo the verification pass. Changes: - Compaction (§4.3): marked DONE, pointer to orchestrator/default.py:240-310 - Tool reliability (§4.9): tracker exists; integration tracked in #2800 PR-B - Routines/Jobs (§6.7): routine_to_mission_alias already translates routine_* calls; create_job aliasing tracked in #2800 PR-C - Two-phase commit (§6.7): marked IMPLEMENTED via unified gate (policy.rs:126-169 + structured.rs:139-171); simulate/preview intentionally not added at policy layer - Acceptance testing (§6.7): pointer to with_engine_v2 harness; coverage expansion tracked in #2800 PR-D - Phase 7: split into 7a (engine-side, DONE) and 7b (host cleanup, blocked on default flip) - Status header + Implementation Progress table: updated to match current state; default-flip work consolidated under issue #2800 No code changes. Refs: #2800 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(plan): address review feedback on engine v2 architecture plan Apply accuracy fixes from PR #2801 review: - Compaction threshold: describe as configurable via `compaction_threshold` (defaults to 85%), matching `compact_if_needed` in the Python orchestrator rather than claiming a fixed 85%. - Token estimation: move ownership to the Python orchestrator (which runs the chars/token heuristic); Rust no longer claims to own this. - Compaction cross-reference: drop the stale "crate-structure block above includes executor/compaction.rs" note — compaction lives entirely in Python. - Reliability injection details (`ENGINE_V2_RELIABILITY_HINTS` kill switch, `EffectBridgeAdapter` write-backs, `build_step_context` reads) are labelled as proposed PR-B follow-up work rather than described as verified reality. - Denylist phrasing: make it clear that `build_software` remains the only hard-denylisted v1 tool *after* PR-C lands, not before. - Provenance rules: document accurately that `ToolOutput` provenance only injects `RequireApproval` on `Financial` effects; `WriteExternal` taint comes only from `LlmGenerated`, per policy.rs:126-169. - Engine-side cleanup: acknowledge that `Session` / `Routine` identifiers still appear in engine docs/comments; the invariant is no runtime dependency, not zero string occurrences. No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Guille
|
653801700e |
feat: add fork action to github tool (#2139)
feat: add fork_repo action to GitHub WASM tool Adds fork_repo action with full input validation, optional organization/name/default_branch_only params. CI failures are pre-existing (RUSTSEC-2026-0098 in rustls-webpki transitive dep, unrelated to this PR). |
||
|
jokemanfire
|
b1472a7dec |
fix(cli): use -m will not quit (#2150)
Now the guard will be take and cusom, So we should not skip msg_tx clone. |
||
|
Illia Polosukhin
|
e9bf77dcfc |
feat(bridge): project 3 dropped engine events to AppEvents (#2797)
* feat(bridge): project 3 dropped engine events to AppEvents Closes the first 3 of ~9 coverage gaps in `thread_event_to_app_events` (#2654) — the UI state convergence work tracked under #2792 (Phase 1). Bridges: - `EventKind::StepFailed` → `AppEvent::Error` (LLM / step failures were silently dropped; "Processing..." stuck with no explanation) - `EventKind::ChildCompleted` → new `AppEvent::ChildThreadCompleted` (symmetric to existing `ChildThreadSpawned`; tree views couldn't mark child branches finished) - `EventKind::CodeExecutionFailed` → new `AppEvent::CodeExecutionFailed` (CodeAct / Monty runtime failures never surfaced to the UI) Scope kept deliberately narrow: the 3 variants with no duplicate-emit risk. `ApprovalRequested` / `ApprovalReceived` are deferred to the PR that adds the `projection-exempt` lint (Phase 1 PR 2), where the existing direct emits from the gate manager can be audited in the same change. Regression tests mirror the existing `thread_event_to_app_events_preserves_call_id_for_action_events` pattern — one per new arm, asserting field mapping and `thread_id` propagation. Refs: #2792, #2654 * refactor(bridge): type CodeExecutionFailed.category as enum Addresses a types.md regression in the previous commit. `category` was stringified on the wire via the engine's `Display` impl, which violates the "Fixed small sets → enum" rule and risks silent drift if the engine enum adds a variant. - Define `CodeExecutionFailureCategory` in `ironclaw_common::event` as a parallel Copy enum with matching `#[serde(rename_all = "snake_case")]` — same wire format, compile-time variant safety. - Bridge the engine enum via an exhaustive match in `code_execution_category_to_wire`. Exhaustiveness is the point: adding a variant to the engine enum is now a compile error here, forcing the wire mirror to be kept in lockstep. - Re-export `CodeExecutionFailureCategory` from the crate root and update the bridge test to assert against the typed variant rather than a string literal. The `ironclaw_engine::CodeExecutionFailure` can't be imported directly into `ironclaw_common` (dependency direction), so the parallel enum is the cleanest option without a bigger crate restructure. Refs: #2792 |
||
|
Nick Pismenkov
|
5fbb67171d |
ci(canary): consolidate Live Canary to one daily 02:00 UTC slot (#2831)
Previously the hourly staggered schedule (five crons at :00/:15/:30/
:45/:50) kicked off a separate workflow run per cron, which produced
24 runs/day per lane group, four red dots per day when something
flaked, and four separate notifications.
Collapse to a single cron `0 2 * * *`. Every job's `if:` guard now
matches that one slot, so all lanes run as parallel jobs inside a
single workflow run:
- One run/day, one red dot on failure, one notification.
- All per-lane statuses visible inside the run; per-job results
still independent (one failing lane doesn't cancel siblings).
- If we want to temporarily dial up frequency for a specific lane
again, we add another cron here and update that lane's `if:`
guard to match.
02:00 UTC chosen as a low-traffic window globally.
|
||
|
Illia Polosukhin
|
bfca5e9331 |
[codex] Tighten auth flows and unify live canary coverage (#2367)
* ci: add live canary regression lanes
* test: tighten live zizmor canary prompt
* feat(auth): harden extension auth and unify canary lanes
* refactor(canary): unify auth live canary framework
* fix(mcp): share stdio runtime state across user views
* fix(ci): mark root crate unpublished
* fix(auth): address oauth canary review findings
* refactor: unify canary runners, restore post-merge user-isolation regressions
Addresses PR 2367 review feedback. Two workstreams.
Canary consolidation (addresses "5 top-level canary dirs" review nit):
- Collapse scripts/auth_browser_canary/ into scripts/auth_live_canary/
with a --mode {seeded,browser} flag. The two runners shared 93% of
their CLI, bootstrap, and stack orchestration.
- Delete scripts/auth_browser_canary/ (4 files, ~684 lines).
- Update run.sh dispatch so auth-live-seeded → --mode seeded and
auth-browser-consent → --mode browser. Lane names unchanged; workflow
YAML needs no edit.
- Fold browser-mode env vars into auth_live_canary/config.example.env
and merge ACCOUNTS.md references.
- Document the live-canary/ (shell) vs live_canary/ (Python package)
split inline so the naming isn't a trap.
Restore regressions dropped in the earlier origin/staging merge:
- ExtensionManager.pending_auth: re-key by (user_id, name) via a
PendingAuthKey struct instead of the bare extension name. Threaded
user_id through clear_pending_extension_auth + all insert/remove
sites. Without this, user A and user B collided on the same
extension's pending-auth state.
- McpSessionManager: re-add DEFAULT_MAX_SESSIONS + max_sessions field
+ with_limits() constructor + oldest-by-last_activity eviction in
get_or_create. Unbounded growth would have leaked one HashMap entry
per unique (user, server) forever.
- McpClient::for_user: re-add is_valid_mcp_user_id validation, bounded
UserClientCache (256-entry FIFO), and Result<Arc<Self>, ToolError>
return type. Cache means repeated tool calls from the same user skip
the initialize handshake.
Follow-up nits from the same review:
- MCP_MAX_SESSIONS env knob in app.rs so operators can raise the cap
without rebuilding (B4).
- Extract drop_pending_oauth_flows_for helper; two retain sites in
manager.rs now share one predicate (B5).
- Annotate the 5 cron schedules in .github/workflows/live-canary.yml
with which lanes each drives (B6).
Collateral: fix two stale crate::bridge::auth_manager::AuthManager
references in src/channels/web/server.rs left over from the earlier
module rename; without this, cargo test didn't compile.
Regression tests:
- test_session_manager_evicts_oldest_when_capacity_is_reached
- test_for_user_rejects_invalid_user_ids
- test_mcp_tool_wrapper_reuses_http_user_client_between_calls
All three assert on the specific class of bug the respective fix
prevents.
Verification:
- cargo check --no-default-features --features libsql: clean
- cargo clippy --no-default-features --features libsql --lib --tests:
zero warnings
- cargo fmt --check: clean
- cargo test tools::mcp -- --test-threads=1: 225 pass
- cargo test extensions::manager::tests: 109 pass
- cargo test --test mcp_multi_tenant_integration: both pass
- Both canary --mode {seeded,browser} --list-cases work
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix: resolve unbound variable error in live-canary dispatcher
In bash strict mode (set -u), the run_python_lane() function would fail
when case_args or passthrough_args arrays were empty due to unquoted array
expansion. Temporarily disable strict mode for these expansions to allow
empty arrays to expand to no arguments (rather than an empty string).
This fixes all three auth canary lanes:
- LANE=auth-live-seeded
- LANE=auth-browser-consent
- LANE=auth-smoke
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* ci: enable live-canary workflow on PRs
- Add pull_request trigger to detect canary runs on PR branches
- Auto-run auth-smoke on every PR to validate auth infrastructure
- Allow manual dispatch of other lanes (auth-full, etc) via workflow_dispatch on PRs
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* ci: enable live-canary on both main and staging PRs
Support pull_request triggers targeting both main and staging branches
so that canary tests run on PRs regardless of target branch.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* ci: enable all canary lanes to run on pull requests
Enable PR triggers for all non-self-hosted canary lanes:
- auth-full: add pull_request trigger
- auth-channels: add pull_request trigger
- deterministic-replay: add pull_request trigger
- public-smoke: add pull_request trigger
- persona-rotating: add pull_request trigger
- provider-matrix: add pull_request trigger
Excluded from PR triggers:
- auth-live-seeded, auth-browser-consent: require env secrets
- private-oauth: requires self-hosted runner
- release-public-full, upgrade-canary: manual-dispatch only
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
* fix: address PR #2367 Copilot review findings
- deny.toml: restore RUSTSEC-2026-0098/0099 ignores; cargo-deny still
needs them because libsql 0.6.0 pins rustls-webpki 0.102.8.
- scripts/live_canary/common.py: wait_for_port_line now uses select()
so the timeout is actually enforced (readline alone blocks forever
if the child never emits a newline).
- scripts/auth_canary/run_canary.py: ensure_tooling_present uses
shutil.which; prior check tested string truthiness and never caught
a missing cargo binary.
- scripts/live-canary/run.sh: run_python_lane quotes array expansions
properly to avoid word-splitting on args with spaces.
- Convert absolute /home/illia/ironclaw/... markdown links to
repo-relative paths in scripts/{auth_canary,auth_live_canary,
live-canary}/*.md and docs/internal/live-canary.md.
- src/channels/web/server.rs: fix stale crate::bridge::auth_manager
refs in test helper after the src/auth/extension.rs move.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(bridge): pass CredentialName as &str to setup instructions lookup
Staging landed CredentialName newtypes (#2611), so ToolReadiness::NeedsAuth
now carries a CredentialName. get_setup_instructions_or_default still takes
&str, so call .as_str() at the bridge boundary.
The method signatures in src/auth/extension.rs will be migrated in the
#2611 follow-up; this is the minimal fix to unblock the merge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(e2e): unblock two auth-matrix canary tests
Two distinct, pre-existing test bugs in tests/e2e/scenarios/test_v2_auth_oauth_matrix.py
that the newly-enabled live-canary PR workflow exposed:
1. test_wasm_channel_oauth_roundtrip: looked up the channel as
"gmail-channel" but the backend canonicalizes extension identities
by folding hyphens to underscores at ExtensionName construction
(.claude/rules/types.md). The /api/extensions list therefore returns
"gmail_channel"; switch the assertion and the setup URL accordingly.
2. test_wasm_tool_oauth_refresh_on_demand: OAuth refresh hits the mock
proxy at http://127.0.0.1:<port>, but validate_oauth_proxy_url
refuses loopback unless IRONCLAW_OAUTH_PROXY_ALLOW_LOOPBACK=1 is
set. The env var is gated to cfg(any(test, debug_assertions)) so
release binaries still reject it. Add it to the auth-matrix fixture
env.
Verified locally: both tests pass; three remaining browser-UI failures
(test_chat_first_gmail_installs_prompts_and_retries,
test_settings_first_gmail_auth_then_chat_runs,
test_settings_first_custom_mcp_auth_then_chat_runs) are a separate
frontend/onboarding flow issue — follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(e2e): resolve remaining auth-matrix canary failures
Follow-up to
|
||
|
jinxin
|
d4d5263ea6 |
fix: model provider config hardening (#2572)
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
e29429d727 |
fix(e2e): multi-tenant widget isolation + portfolio nudge recovery (#2790)
* fix(e2e): fix 5 test failures — multi-tenant widget isolation + portfolio nudge recovery
Widget customization: three tests expected multi-tenant behavior (CSS/widget/CSP
isolation) but ran against the single-tenant default server. Add a session-scoped
`multi_tenant_gateway_server` fixture with AGENT_MULTI_TENANT=true and its own
libSQL database, and rewire the three failing tests to use it.
Portfolio: the mock LLM's nudge response ("I found the information you
requested.") swallowed portfolio context when the engine sent a tool-intent
nudge. Add context-aware nudge recovery in match_response() that checks prior
user messages for portfolio/wallet keywords before falling through to the
generic nudge pattern. Also add word boundaries to the hello|hi|hey canned
pattern to prevent "hi" from matching inside "this".
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address review findings (iteration 1)
Forward cargo-llvm-cov env vars in multi_tenant_gateway_server fixture
so code coverage from the 3 rewired widget tests is captured in CI.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
||
|
Illia Polosukhin
|
4dea5dd5da |
fix: bug bash 4/16 triage — error boundary, TEE secrets, pairing, rehydration (#2753)
* 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 commits |
||
|
Zaki Manian
|
8d052a9eb5 |
fix(security): scope orchestrator credentials to job creator (#2068) (#2698)
* fix(security): remove cross-tenant credential fallbacks in orchestrator, WASM, and channels (#2068, #2069, #2100) Three credential isolation fixes that prevent cross-tenant secret leakage: - Orchestrator: get_credentials_handler now resolves the job creator's user_id from job_owner_cache (or DB fallback) instead of using a hardcoded global owner_id. Returns 403 when owner cannot be resolved. Removes the user_id field from OrchestratorState entirely. - WASM tools: resolve_host_credentials uses DefaultFallback::Denied instead of AdminOnly, preventing any user's WASM tool from falling back to "default" scope credentials. - Channel broadcast metadata: removes legacy migration fallback that read broadcast metadata from "default" scope. Channels re-persist metadata under the correct owner scope on next incoming message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): extract resolve_job_owner, bound cache, per-job credentials Address review feedback: - Extract resolve_job_owner() to DRY up cache-then-DB resolution - Bound job_owner_cache to 10K entries with batch eviction - Add register_job_owner() for pre-population at job creation - get_credentials_handler uses per-job owner instead of global state.user_id, preventing cross-tenant credential leakage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): filter empty user_id from cache, unify error codes Address follow-up review feedback: - Filter empty user_id before caching to prevent poisoned entries - Map secret decrypt failures to 403 (not 500) to avoid info leak distinguishing "secret missing for user" from "owner unknown" - register_job_owner is available for callers that have both the cache and user_id; DB is required for sandbox credential injection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: fix rustfmt line-length violation in orchestrator api Break long method chain in cache eviction across multiple lines to pass `cargo fmt --check`. https://claude.ai/code/session_01JRasj3ujmr1uzmfUeLbNFo * fix(review): address PR feedback — fix test, drop dead helper, bump log level - Update credentials_uses_job_creator_not_other_user to assert 403 FORBIDDEN. The prior assertion of 500 INTERNAL_SERVER_ERROR contradicted the same PR's change that mapped all secret-lookup failures to FORBIDDEN, so the test failed to even compile-as-regression. Also expand the comment to explain why uniform 403 is the correct wire response here. - Remove register_job_owner: the helper had zero call sites. The cache is self-warming because resolve_job_owner inserts on every DB fallback, so an explicit registration hook would only save one DB hit on the first SSE event of a job. Wiring it into ContainerJobManager::create_job is a larger refactor; file a follow-up if eager warming is worth the cost. - Update job_owner_cache doc comment to describe lazy population — the previous "populated when sandbox jobs are created" claim was aspirational. - Fix MAX_JOB_OWNER_CACHE_SIZE comment: HashMap eviction is not LRU/FIFO. Note IndexMap/lru::LruCache as upgrade options if recency matters. - Bump decrypt-failure log from debug to warn, add env_var for operability. Keeps 403 wire response (no existence-leak to the caller) but restores operator visibility for real crypto/keychain failures. - Annotate the job_event_handler unwrap_or_default with a silent-ok comment per the error-handling rule — SSE broadcast is best-effort and the empty user_id path is already handled below. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> |
||
|
Illia Polosukhin
|
22fa85b670 |
feat(engine): add short title field to v2 threads for sidebar labels (#2776)
* feat(engine): add short title field to v2 threads for sidebar labels `Thread.goal` is the execution prompt — a multi-paragraph meta-prompt for missions, or the full first user message for gateway chats. Reusing it as the sidebar label makes the conversation list expand to fit the longest prompt in view. Split the two concerns: - `Thread.title: Option<String>` (with `#[serde(default)]`) for the compact human label; legacy rows without the field rehydrate cleanly as None. - Threaded a `title` parameter through `ThreadManager::spawn_thread_with_history` and added a `spawn_thread_with_title` wrapper; title is applied before `save_thread` so the executor's in-memory copy observes it atomically. - Mission-spawned threads pass `Some(mission.name)`; gateway conversation threads pass `Thread::derive_title_from_message(content)` (first non-empty line, trimmed, char-safe truncated to 60 with an ellipsis). - `EngineThreadInfo` carries the new field; `chat_threads_handler` prefers it and falls back to a derived short label for pre-existing threads. - Belt-and-braces CSS truncation on `.thread-label` so the sidebar can never bleed across the page even if a title somehow slips through long. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(engine): streaming title truncation + docstring/test-name fix Addresses review feedback on PR #2776: - Drop the unreachable `trimmed.is_empty()` guard — the `find` predicate already guarantees the line is non-empty after trim. - Replace `trimmed.chars().count()` + re-iterate with a single streaming pass: take up to MAX_CHARS-1 chars, peek the rest, and append either the final char (no ellipsis, result is MAX_CHARS) or '…'. Avoids an O(n) scan on pathological single-line input. - Correct the docstring to say "leading and trailing whitespace" so it matches `trim()`, which is the right behavior for a sidebar label. - Rename `derive_title_trims_trailing_whitespace` to `derive_title_trims_whitespace` since the test input has whitespace on both ends. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(bridge): assert thread_to_info carries title and goal separately Addresses self-review finding on PR #2776: the `EngineThreadInfo` wire contract gained a `title` field but nothing in the Rust tree directly exercised the DTO populator after upstream dropped the sidebar engine-thread merging. Adds two small tests that build a `Thread` with and without a title and assert `thread_to_info` passes both `title` and `goal` through independently — so mission DTOs can render the short label without reading the multi-paragraph meta-prompt. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(bridge): propagate title through EngineThreadInfo and archive roundtrip Addresses two medium-severity review findings on PR #2776 (same class of bug — a new `Thread.title` field was added without propagation to satellite types, per `.claude/rules/review-discipline.md`): 1. `thread_to_info` now falls back to deriving a short label from `goal` when `title` is `None`. Without this, legacy engine threads persisted before the `title` field existed flow through to frontends (TUI, mission detail views) as `title = None`, and the frontend `threadTitle()` fallback chain in `history.js` renders a UUID prefix because `EngineThreadInfo` has no `turn_count`. 2. `ThreadArchiveSummary` now carries `title` (with `#[serde(default)]` for backwards compatibility, mirroring the `total_cost_usd` precedent at #2562). `compact_thread_summary` persists it, `thread_from_archive` reads it. `backfill_archived_threads` is a live consumer — without this, workspaces that only have archived summaries still rehydrate threads with no title. Tests: - `thread_to_info_derives_title_from_goal_when_absent` - `thread_to_info_derives_from_first_line_of_long_goal` - `archive_summary_preserves_title_through_round_trip` - `archive_summary_handles_legacy_json_without_title_field` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Robert Yan
|
c0b4e30cf6 | ci: release versioned docker image (#2795) | ||
|
Illia Polosukhin
|
8fffa8797c |
fix(tests): close staging test backlog — full suite green (#2744)
* fix(tests): close the staging test backlog — rust suite green, e2e 14→4 A pass over staging turned up 12 rust test failures and 14 playwright e2e failures + 1 fixture error. Most were wiring/invariant drift or stale test expectations around engine v2. This patch cleans up the ones with clear root causes. Rust (12 → 0): - `tools::builtin::skill_tools` (8 tests): ripped out hand-rolled ZIP byte blobs that were missing the EOCD record since the extractor switched to `zip::ZipArchive::new` in #2385. Tests now build through `zip::ZipWriter`, matching the production path. Drops the obsolete nested-path assertion whose assumption conflicts with intentional GitHub-archive root stripping. - `extensions::manager::test_telegram_token_colon_preserved_in_validation_url`: `src/pairing/approval.rs::propagate_approval_restores_runtime_state_when_on_start_fails` was mutating the `IRONCLAW_TEST_TELEGRAM_API_BASE_URL` runtime-env overlay without holding `ENV_MUTEX`. Now acquires `lock_env()` so concurrent readers see a stable value. - `bridge::router::handle_with_engine_persists_attachment_files_and_indexes_them`: two distinct `ENGINE_STATE_TEST_LOCK` statics (one in `test_support`, one in the sibling `tests` module) meant cross-module tests raced on the shared `ENGINE_STATE` `OnceLock`. Replaced the private duplicate with `use super::test_support::ENGINE_STATE_TEST_LOCK`. - `e2e_attachments::engine_v2_channel_attachments_persist_for_telegram_and_whatsapp`: attachment persistence resolves paths through the cached `bootstrap::ironclaw_base_dir()`, not the test's tempdir CWD. Added `bridge::override_engine_project_root_for_test` and wired the test to use it. - `telegram_auth_integration::test_group_message_emits_chat_type_metadata`: local fix — rebuild `channels-src/telegram` so the WASM picks up the April-17 `chat_type` emit from #2513. CI rebuilds the module per run, so no binary committed here. Playwright (14 failed + 1 error → 4 failed + 1 error): - `test_chat.py::test_gateway_attachment_flow_renders_thread_and_reaches_llm` and the unextractable variant: a legacy change listener on `#image-file-input` fired before the unified `handleAttachmentFiles` path, cleared `e.target.value`, and left the FileList empty by the time the unified handler ran. Removed the duplicate wiring in `crates/ironclaw_gateway/static/js/surfaces/chat.js`. - `test_chat.py::test_slash_autocomplete_shows_commands_and_skills`: `SLASH_COMMANDS` never merged installed skills. Added `refreshSlashSkillEntries()` that fetches `/api/skills` on menu open and re-runs the filter once the skills land. - `test_pending_user_messages.py::test_pending_message_survives_sse_reconnect`: the SSE open handler only reloads history when `disconnectMs > SSE_RELOAD_THRESHOLD_MS`; the test's instant reconnect skipped that. Ages `_sseDisconnectedAt` past threshold. - `test_pending_user_messages.py::test_welcome_card_hidden_when_pending`: `_create_new_thread` returned `currentThreadId` before the new-thread API round-trip set it, so callers got the pre-click id and keyed `_pendingUserMessages` on the wrong thread. Now waits for the id to change. - `TestV2EngineSkillInstallFlow` (7 → 2 failures): - Skill card template didn't render `usage_hint`, `has_requirements`, `has_scripts`, or `install_source_url`. Extended `renderSkillCard` in `surfaces/skills.js`. - The deny message `"Do not execute it; choose an alternative approach"` accidentally matched `user_signals_execution_intent`'s EXEC_PHRASES ("execute it"), re-arming `require_action_attempt` on resume and nudging the LLM into another tool call. Rephrased to `"Do not retry; choose a different approach"` in `src/bridge/router.rs`. Partial progress (still failing, needs deeper engine-v2 work): - `test_v2_engine_oauth_google::test_oauth_token_refresh_on_expiry`: added an `oauth:` block to the test's `google_drive` skill (which registers a refresh config via `credential_spec_to_oauth_refresh`) and aligned `GOOGLE_OAUTH_CLIENT_ID` with the mock proxy's expected `hosted-google-client-id`. Thread still hits the auth gate instead of refreshing — the pre-flight path isn't reaching `oauth_refresh_for_secret("google_drive_token")`; needs instrumentation on the engine-v2 gate pipeline. Net: rust suite green, playwright 4 failures left (2 skill-install approval-flow edge cases, 1 OAuth refresh, 1 REPL auth that flakes only under full-suite load) + 1 restart-fixture health-check timeout that flakes under 20-min suite pressure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): close the final 4 e2e failures + add copy-button coverage Follow-up to the earlier staging test pass. Drives the remaining playwright failures to green and adds the missing test for the per-message Copy button. New coverage: - `test_chat.py::test_message_copy_button_writes_raw_text`: clicking the per-message Copy button writes the raw text (user turn) or the raw markdown (assistant, via `data-raw`) to navigator.clipboard and flashes the button label to "Copied!" then back to "Copy". The existing `test_copy_from_chat_forces_plain_text` only covered the Cmd+C selection handler, so a regression to the button path was invisible. Fixes: - `TestV2EngineSkillInstallFlow::test_implicit_skill_activation_works_immediately_after_install`: the pika skill manifest uses the legacy `metadata.openclaw.requires` shape without a top-level `activation:` block, so `score_skill` scored 0 for every prompt and the skill never activated unless the user typed `/pikastream-video-meeting`. "Please use pikastream-video-meeting to prepare this call" should activate just like the slash form. `score_skill` now treats the skill name (and the hyphen/underscore-normalized form) as an implicit keyword, gated at ≥4 chars so short generic names don't false-match. `test_installed_skill_does_not_overfire_on_unrelated_prompt` still passes — a grocery-list prompt doesn't accidentally trigger pika. - `TestV2EngineSkillInstallFlow::test_duplicate_install_is_idempotent_and_keeps_single_card`: the test was waiting for an approval card on the second install, but `SkillInstallTool::requires_approval` short-circuits to `ApprovalRequirement::Never` when the skill is already loaded — asking the user to approve a guaranteed no-op is pure friction, and the test was asserting against that intentional behavior. Rewrote the test to skip the approval step and assert on the terminal message's idempotent "already installed / no install needed" wording, which matches the actual production output. - Mock LLM: the pattern branch in `match_tool_call` was re-emitting a matching tool call on every LLM round because "last user content" doesn't change across turns, so the engine looped until it hit the multi-result summary path. Added a guard that falls through to the text-response path when the matching tool_name is already present in `recent_tool_results` — mirroring real LLM behavior. - `test_v2_engine_oauth_google::test_oauth_token_refresh_on_expiry`: two compounding issues blocked the refresh path. (1) The mock `/oauth/refresh` handler validates `client_id == "hosted-google- client-id"`, but the fixture env set `test-google-client-id`. (2) Proxy URL points at `http://127.0.0.1:<port>` (the mock LLM) and the production SSRF guard blocks loopback by default; mock E2E tests opt in via `IRONCLAW_OAUTH_PROXY_ALLOW_LOOPBACK=1`. Also added an `oauth:` block to the test's `google_drive` skill so `credential_spec_to_oauth_refresh` registers a refresh config under `google_drive_token`. Finally, the refresh path needs a stored refresh token — paste-based auth (the earlier tests' fallback when no google-drive WASM binary is available) only persists the access token, so the test now skips in that configuration rather than asserting on a refresh that can't happen, matching the pattern already used by `test_oauth_redirect_flow`. Remaining after this PR: `test_repl_http_auth_prompt_accepts_token_and_retries` passes in isolation but flakes under full-suite load (the PTY REPL sibling test is already `@pytest.mark.skip` for the same reason); and `test_always_approve_survives_restart` which times out the `/api/health` probe under full-suite pressure. Both are PTY / fixture-startup concurrency issues, not product regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): close the last 2 e2e failures — full suite green (401 passed, 0 failed) Root-causes the two tests left open after the previous commit. Both were real bugs/config drift masquerading as flakiness. - `test_repl_http_auth_prompt_accepts_token_and_retries`: `CLI_MODE` defaults to `tui` (the ratatui full-screen UI), which reads stdin keystroke-by-keystroke and renders into a framebuffer. The PTY-driven tests in this file send whole lines via `os.write(master_fd, b"prompt\n")` and match for specific text in the raw stream — under the default TUI that line-based send never reaches the agent, so the auth card never fires and `_read_repl_until` times out with only cursor-position escape sequences captured. Pinning `CLI_MODE=repl` on the fixture routes the test back onto the plain line-based REPL surface it's written against. Confirmed passing 5/5 in isolation and under full-suite load. - `test_always_approve_survives_restart`: the fixture's ironclaw subprocess was dying at startup with `Channel webhook_server failed to start: Failed to bind to 127.0.0.1:8080: Address already in use (os error 98)` — the fixture picked a free `GATEWAY_PORT` but left `HTTP_HOST`/ `HTTP_PORT` unset, so the HTTP channel tried to claim the default port 8080 and collided with every other e2e server (and anything else on 8080). Every `/api/health` probe was hitting a dead process, which showed up as a 60 s timeout instead of a bind error because the subprocess's stderr was never drained — a full 64 KiB pipe buffer made the child block on its next write before it could even log the bind failure. Fix: - allocate a second free TCP port for `HTTP_PORT` (mirrors the sibling `v2_approval_server` fixture); - wire `stdout`/`stderr` through background drain tasks so `RUST_LOG=ironclaw=debug` output can't back-pressure the child into a startup hang; - surface the last 32 KiB of stderr in the timeout error so future regressions (panic, bind conflict) show up in the failure message instead of being silently swallowed. Full-suite e2e: 401 passed, 8 skipped, 0 failed, 0 errored (17:31). Rust unit + integration tests still green, clippy clean, fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): address PR #2744 review + reconcile with staging ## Review feedback - **Slash-skill cache spam (3× — Gemini + 2× Copilot):** the previous `refreshSlashSkillEntries()` re-fetched `/api/skills` on every keystroke in `filterSlashCommands`; the in-flight guard only suppressed concurrent duplicates. Added a 30 s TTL and an `invalidateSlashSkillCache()` hook that the install/remove flows in `surfaces/skills.js` call so the menu picks up install/remove changes immediately instead of waiting for the TTL. - **Wrong module path in comment (Copilot):** `src/bridge/router.rs` comment referenced `llm::reasoning::user_signals_execution_intent` but `reasoning` isn't `pub` — the helper is re-exported as `crate::llm::user_signals_execution_intent`. Updated the comment to use the canonical path and cross-reference the defining file. - **Misleading `#[tokio::test]` justification (Copilot):** prior comment said "single-threaded tokio and cannot deadlock" without pinning the runtime flavor. `#[tokio::test]` *does* default to the current-thread runtime in this crate, but spelling it out is safer against future defaults drifting. Pinned `#[tokio::test(flavor = "current_thread")]` explicitly and reworded the comment to name the runtime kind. - **Drain tasks cancelled but not awaited (Copilot):** the restart fixture in `test_v2_engine_approval_flow.py` cancelled the stdout/stderr drainers on `stop()` without awaiting them, causing "Task was destroyed but it is pending!" warnings and, on stop→start cycles, zombie readers. Now cancels *and* `asyncio.gather (..., return_exceptions=True)` awaits them. ## Merge reconciliation with `origin/staging` Staging merge introduced: - A strict MIME allowlist on `/api/chat/send` attachments (#2332). `test_gateway_attachment_unextractable_file_uses_placeholder` previously relied on `application/octet-stream` reaching `document_extraction` and triggering the "[Failed to extract …]" placeholder; the new gateway-side allowlist rejects that MIME outright at the HTTP layer, so the test never exercised the fallback path. Updated the test to upload a corrupt PDF (`%PDF-1.4` magic + garbage body) which passes MIME + header checks but fails extraction — the exact scenario the placeholder was designed for. - A conflict in `src/pairing/approval.rs` where staging added `#[ignore]` to the propagate-approval test (needs a pre-built telegram WASM binary) and this branch added `#[allow(clippy::await_holding_lock)]`. Merged both, plus pinned the explicit `current_thread` runtime flavor per review. ## Pre-existing failures left alone `test_portfolio.py::test_portfolio_chat_keyword_triggers_skill` and `test_portfolio_wallet_address_triggers_skill` both fail identically against plain `origin/staging` (verified via `git stash` + checkout of the staging versions of the test file and `crates/ironclaw_engine/orchestrator/default.py`). Root cause is unrelated to this PR — appears to be the mock LLM's portfolio response text tripping the engine's tool-intent nudge path before reaching the canned response the test asserts on. Out of scope here. ## Verification - `cargo fmt` - `cargo clippy --all --benches --tests --examples --all-features` — zero warnings - `cargo test --lib` — 5329 passed, 7 ignored, 0 failed - `pytest scenarios/test_chat.py scenarios/test_v2_engine_approval_flow.py scenarios/test_v2_engine_auth_flow.py::TestV2EngineSkillInstallFlow scenarios/test_v2_auth_oauth_matrix.py scenarios/test_pending_user_messages.py` — **58 passed, 1 skipped, 0 failed** Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(fmt): collapse override_engine_project_root call onto single line rustfmt on staging collapses this call; my earlier `cargo fmt` ran before the `project_root.clone()` edit landed so the local check missed it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR #2744 review: startup-timeout leak + attachment test isolation 1. `test_v2_engine_approval_flow.py` — `start()` re-raised `TimeoutError` from `wait_for_ready` without tearing the subprocess down. Because `await start()` runs before the fixture's `try/finally`, a startup timeout would leak the child process and its bound ports into the rest of the test run. Snapshot the stderr tail before teardown, `await stop()` (which kills the proc and cancels/awaits the drain tasks), then re-raise with the captured tail. 2. `tests/e2e_attachments.rs` — the `engine_v2_project_root()` helper derived from `bootstrap::ironclaw_base_dir()` is a process-global `LazyLock` that resolves to `$HOME/.ironclaw` on dev machines and CI runners. Passing its parent as the engine's project_root meant this test was writing real attachment files into `~/.ironclaw/attachments` every time it ran. Allocate a per-test `tempfile::TempDir` instead and point `override_engine_project_root_for_test` at it — now writes are fully contained. The `engine_v2_attachment_root_lock` mutex stays (still required to serialize mutations of the process-global engine state across concurrent 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> |
||
|
Illia Polosukhin
|
07972fc099 |
fix(auth): prevent OAuth URL parameter truncation (#2391) (#2746)
* fix(auth): switch OAuth URL construction to url crate to prevent char loss (#2391) Google OAuth was reportedly receiving `access_type=offlin` instead of `access_type=offline` when users ran `ironclaw tool auth google-calendar`, breaking the offline-token flow every Google WASM tool relies on (Calendar, Gmail, Drive, Docs, Sheets, Slides). The hand-rolled `format!` + `urlencoding::encode` loops in `auth::oauth::build_oauth_url` and `tools::mcp::auth::build_authorization_url` are replaced with `url::Url` + `query_pairs_mut()`, routing every query parameter through a single well-tested `application/x-www-form-urlencoded` serializer. The old concat path is kept as a defensive fallback for the (never-observed-in-practice) case where the authorization URL itself fails to parse. Regression coverage added at the call-site level per `.claude/rules/testing.md`: * `test_build_oauth_url_preserves_access_type_offline_exactly` — parses the returned URL and asserts `access_type == "offline"` exactly (not via `.contains()`, which would have passed on `offlin`). * `test_build_oauth_url_extra_params_preserve_all_chars_across_hash_orderings` — loops 16 iterations so random `HashMap` iteration order surfaces any bug sensitive to which param lands last. * `test_google_calendar_capabilities_produce_correct_oauth_url` — loads the shipped `google-calendar-tool.capabilities.json` shape, parses it via `CapabilitiesFile::from_json`, and drives the same `build_oauth_url` call site that `cli::tool::auth_tool_oauth` uses. * `test_build_authorization_url_extra_params_preserve_all_chars` — parallel regression for the MCP authorization-URL builder. The two pre-existing helper tests were also tightened to round-trip through `url::Url::parse` + `query_pairs()` rather than relying on substring assertions, so a 1-char truncation can no longer pass as a prefix match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(auth): address PR #2746 review feedback - Reject malformed authorization URLs with a specific error instead of concat-normalizing them (gemini-code-assist review). - Rebuild HashMap per iteration in order-probe tests so different iteration orders are actually exercised (Copilot review). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(auth): surface malformed OAuth descriptors at call sites (#2746) Address review feedback from @serrrfirat on PR #2746: two call sites of `build_pending_oauth_launch` were using `.ok()?` to silently drop `OAuthUrlError::MalformedConfig`, which regressed the fail-closed posture this PR introduced. Replaces `.ok()?` in both: - `AuthManager::start_skill_oauth_if_supported` - `ExtensionManager::start_secret_oauth_flow` with an explicit `match` that emits `tracing::error!` (carrying credential/extension/secret/user context) before falling back to the manual-token path. Operators now get a signal when an OAuth descriptor is misconfigured, rather than seeing the browser auth flow silently disappear. Signatures stay `Option<...>` — the existing `test_build_oauth_url_rejects_malformed_authorization_url` covers the helper-level regression; this change is call-site observability. [skip-regression-check] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
edbf0eaaa1 |
fix(engine): stop failed missions from respawning (#2736) (#2760)
* fix(engine): stop failed missions from respawning (#2736) * fix(engine): address henrypark133 review — allow failed mission resume (#2760) |
||
|
firat.sertgoz
|
0bb3f6ed84 |
fix(engine-v2): recover flattened tool calls (#2757)
* fix(engine-v2): recover flattened tool calls * fix: address review findings (iteration 1) * fix: address review findings (iteration 1) |
||
|
Illia Polosukhin
|
95dcf807e0 |
fix(gateway): serve Responses API under /api/v1/ prefix (#2201) (#2748)
* fix(gateway): serve Responses API under /api/v1/ prefix (#2201) The OpenAI Responses API was only reachable at `/v1/responses`, which broke the otherwise consistent `/api/...` prefix used by every other IronClaw HTTP surface. Callers expecting `/api/v1/responses` got a 404. This routes both paths through the same handlers: - `/api/v1/responses` + `/api/v1/responses/{id}` — canonical paths - `/v1/responses` + `/v1/responses/{id}` — retained as backward-compat aliases for clients already configured against the legacy path Also updates the web gateway CLAUDE.md route table, the USER_MANAGEMENT_API.md reference, and the module docstring for responses_api.rs so documentation points at the canonical prefix. Regression test: tests/responses_api_path_prefix.rs drives the full router via `start_server` and asserts that POST/GET on both the canonical and legacy paths reach the handler (400 from the handler for bad inputs, not 404 from the router) and that both paths enforce bearer auth (401 without a token). This follows the "Test Through the Caller, Not Just the Helper" rule so a future router edit that drops either path fails the test rather than silently regressing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(gateway): address PR #2748 review feedback - Extend both_paths_require_auth to cover GET /responses/{id} on both canonical and legacy paths. - Align USER_MANAGEMENT_API.md Responses API examples with the current handler behavior (only "default" model accepted). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: address PR #2748 reviewer nits - Change the "Go ahead with the transfer" Responses API request example to use "model": "default". The handler rejects any other value, so copying the old example verbatim would 400. - Expand the Error Format section to document that the Responses API returns an OpenAI-compatible JSON envelope ({"error": {...}}) rather than the plain-text body used by every other endpoint. Add 429 to the status-code table for Responses API rate limiting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address PR #2748 Copilot review nits on docs + test cleanup - Correct the documented Responses API 429 error type from `rate_limit_exceeded` to `rate_limit_error` to match what `create_response_handler` actually emits. - Clarify that the JSON error envelope covers handler-generated errors; missing/invalid bearer token (401) and auth-path 503 are returned by the shared gateway auth middleware as plain text. - Add a `ServerGuard` RAII helper in the Responses API path-prefix integration test that takes `state.shutdown_tx` on startup and sends `()` on drop, so each test tears its `axum::serve` task down instead of leaking it for the rest of the process. Update the six test callers to bind the guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |