mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-02 23:56:24 +08:00
staging
1325 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Illia Polosukhin
|
5fe3be8dce |
refactor(gateway): extract features/chat/ — ironclaw#2599 stage 4c (#2680)
Biggest single-slice migration in the epic: ten chat handlers + every chat-private helper + all chat helper-tests leave `server.rs` for a new `src/channels/web/features/chat/` module. Routes carried over end-to-end (no behavior change): - POST /api/chat/send, /api/chat/approval, /api/chat/gate/resolve - POST /api/chat/auth-token, /api/chat/auth-cancel (legacy v1 shims) - GET /api/chat/ws, /api/chat/events - GET /api/chat/history, /api/chat/threads - POST /api/chat/thread/new Chat-private helpers that moved along with the handlers: - `is_local_origin` (CSRF-gate for the WS upgrade) - `pending_gate_extension_name` → routes through the canonical `AuthManager::resolve_auth_flow_extension_name` (the identity invariant called out in `src/channels/web/CLAUDE.md` + check #8 in `scripts/pre-commit-safety.sh`); the wrapper is preserved byte-identical so the "one resolver" rule holds after the move. - In-progress reconciliation chain: `reconcile_in_progress_with_turns`, `in_progress_matches_turn`, `in_progress_from_metadata`, `is_stale_in_progress`, `completed_turn_is_newer_than_in_progress`, `in_progress_from_thread`, `summary_live_state`. - `turn_info_from_in_memory_turn`, `thread_state_label`, `turn_state_label`, `IN_PROGRESS_STALE_AFTER_MINUTES`. - `HistoryQuery`, `ChatEventsQuery` request DTOs and `extract_last_event_id` helper. - `engine_pending_gate_info` / `history_pending_gate_info` gate-info hydrators. Tests: 15 helper-level tests (5 reconcile, 2 in-memory-turn-info, 2 summary-live-state, 1 thread-state-label, 5 is-local-origin) move with the helpers into `features/chat/mod.rs::tests`. 8 caller-level tests (chat_history × 3, chat_approval, chat_auth_token × 2, chat_auth_cancel, chat_gate_resolve) stay in `server.rs::tests` for now because they rely on shared `GatewayState` builders (`test_gateway_state`, `test_gateway_state_with_store_and_session_manager`, `test_gateway_state_with_dependencies`) that construct state for multiple slices — promoting those builders to `src/channels/web/test_helpers.rs` is a follow-up. Also dropped 3 `test_build_turns_from_db_messages_*` tests in `server.rs` that were redundant with the 14 already in `util.rs::tests`. Cleanup of dead code: `src/channels/web/handlers/chat.rs` deleted entirely. The file held live `chat_events_handler` + `extract_last_event_id` + `ChatEventsQuery` (absorbed into `features/chat/`), plus three zombie duplicate handler definitions (`chat_ws_handler`, `chat_threads_handler`, `chat_new_thread_handler`) that predated the `server.rs` canonicals but were never deleted — one of them (`chat_ws_handler`) used a weaker `is_local_origin` heuristic that skipped IPv6-literal parsing, so accidentally wiring through it would have been a silent security degradation. The router.rs imports and `handlers/mod.rs` declaration are updated accordingly. Router updates: nine `server::` imports swapped for `features::chat::`, plus the `handlers::chat::chat_events_handler` import removed (now `features::chat::chat_events_handler`). The migration docstring above the feature-handler imports lists chat as extracted alongside logs / oauth / pairing / status. Quality gate: fmt clean, clippy clean on `--all --tests --examples --all-features`, 424 `channels::web` tests pass, boundary checker reports no back-edges with the existing empty allowlist. Net shape: `server.rs` shrinks from ~5,770 → ~3,620 lines (-2,150 lines). `handlers/chat.rs` goes from 432 → 0. Explicit non-scope (noted in the migration plan): - Four handlers (`chat_send`, `chat_ws`, `chat_threads`, `chat_new_thread`) gate side effects and currently have no caller-level test. Adding them is genuine new coverage, not regression preservation — a follow-up PR. The helper-level tests that DO cover things (`is_local_origin`, `reconcile_*`, `turn_info_from_in_memory_turn`, `summary_live_state`, `thread_state_label`) move with their helpers. - The pre-existing `engine_v2` / `engine_v2_enabled` duplicate in `GatewayStatusResponse` (flagged on PRs #2665 and earlier) still needs a coordinated frontend fix and isn't touched here. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Will.hou
|
90a6dadb43 |
fix(cli): prevent UTF-8 panic in MCP tool description truncation (fixes #1947) (#2008)
`&tool.description[..57]` panics when byte 57 is inside a multi-byte character (CJK = 3 bytes, emoji = 4 bytes). Replace with `floor_char_boundary()` which walks back to the nearest valid boundary. Also fixes the same pattern in `config/channels.rs` where `&scope[..32]` could panic on non-ASCII OAuth scopes. Adds 4 regression tests: ASCII, CJK, emoji, and mixed-boundary truncation. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-authored-by: willamhou <willamhou@ceresman.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Happy <yesreply@happy.engineering> |
||
|
Reid
|
c854a214f9 |
fix(cli): suppress non-CLI listeners under --cli-only (#1840) (#1869)
- Gate webhook, WASM, Signal, HTTP, gateway, tunnel, and orchestrator startup behind --cli-only - Restore the expected --cli-only contract so no non-CLI listeners bind or expose services - Prevent unintended network exposure from fallback listeners and managed tunnels - Stop registering job_prompt when no orchestrator is running to consume prompts - Fix sandbox readiness reporting under --cli-only so it reports disabled, not unavailable - Centralize the guard through non_cli_channels_enabled() for consistent startup behavior - Add regression coverage for unguarded network startup paths in async_main - Document --cli-only listener suppression in NETWORK_SECURITY.md |
||
|
Nick Stebbings
|
14333e4a0a |
fix(wasm): run leak scan on pre-injection headers in channel callbacks (#1377)
* fix(wasm): run leak scan on pre-injection headers in channel callbacks
The WASM channel host's http_request handler was scanning request headers
AFTER inject_credentials() replaced placeholder values (e.g. {SLACK_BOT_TOKEN})
with real secrets. This caused the leak detector to flag host-injected
credentials as potential leaks, blocking legitimate WASM channel callbacks.
Run the leak scan on the original WASM-provided headers (before any
credential injection) so host-injected tokens never appear in the scan.
WASM never sees the real values, so scanning the pre-injection state is
correct. Matches the existing pattern in src/tools/wasm/wrapper.rs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test: regression test for pre-injection leak scan ordering
Proves that scanning post-injection headers triggers a false positive
on host-injected xoxb- tokens, confirming the fix must scan WASM-provided
headers before credential injection.
* fix: address review feedback — eliminate double-parse, fix comment, migrate import
- Eliminate double-parse of headers_json: parse once, scan raw headers,
then inject credentials (matches tools wrapper pattern)
- Fix misleading comment: URL has template substitution but not yet
host credential injection (was "before ANY credential injection")
- Migrate import to ironclaw_safety::LeakDetector per CLAUDE.md
- Remove unnecessary block scope around leak scan
- Remove raw_url_for_scan alias (just use &url directly)
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>
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
|
||
|
Illia Polosukhin
|
a524bf8aee |
refactor(gateway): stage 4b slices + empty allowlist — ironclaw#2599 (#2665)
* refactor(gateway): stage 4b slices + empty allowlist — ironclaw#2599
Collapses the last pre-existing back-edges tracked by the boundary
checker and moves the first three small feature slices out of
server.rs.
Stage 4b slices:
- features/logs/ — /api/logs/events, /api/logs/level (GET/PUT)
- features/pairing/ — /api/pairing/{channel} (GET),
/api/pairing/{channel}/approve (POST)
- features/status/ — /api/gateway/status (+ GatewayStatusResponse,
ModelUsageEntry, each now owned by the slice)
Platform extensions (co-located with existing platform modules so
every caller — handlers/, features/, and the still-shrinking
server.rs — can reach them without a back-edge):
- platform/legacy_auth.rs:
- handle_legacy_auth_token_submission
- handle_legacy_auth_cancel
- clear_auth_mode, clear_auth_mode_for_thread
Consumers: server.rs chat HTTP shims + platform/ws.rs.
- platform/engine_dispatch.rs:
- dispatch_engine_submission
- dispatch_engine_external_callback
- dispatch_onboarding_ready_followup (now takes &ExtensionName)
Consumers: server.rs chat + extensions_setup_submit + features/pairing.
- platform/static_files.rs gains the workspace-backed layout/widget
readers (read_layout_config, load_resolved_widgets,
read_widget_manifest, LAYOUT_PATH, WIDGETS_DIR, MAX_WIDGET_* caps).
handlers/frontend.rs imports them back from platform.
ExtensionName adoption:
- Deletes sanitize_extension_name and its 5 unit tests from
server.rs. The defensive "never fails, returns 'unknown'" helper is
replaced with ironclaw_common::ExtensionName validation at the one
untrusted boundary we still expose (pairing_approve_handler's URL
path). Invalid names now return 400 at the handler — the old
behavior sanitized injection-shaped input into a safe-but-nonsense
string that would never have matched a real extension anyway, so
this is strictly better telemetry with no loss of reachable
behavior. Registry-sourced names (derive_onboarding in
handlers/extensions.rs) drop the sanitize call entirely; the
comment notes a follow-up to type Extension.name as ExtensionName
directly.
- Other shared helpers that needed to leave server.rs as collateral:
images_to_attachments moves to web/util.rs alongside the other
pure message-building helpers.
ws.rs cleanup:
- Switches GatewayState / PerUserRateLimiter / RateLimiter /
ActiveConfigSnapshot imports from the server.rs re-export path to
crate::channels::web::platform::state directly, removing the last
state-type allowlist entries.
Allowlist:
- scripts/check_gateway_boundaries.py: ALLOWLIST is now empty. All
eight pre-existing entries (widget helpers, seven ws.rs shim
symbols) are gone — every relocation landed in platform/. The
allowlist mechanism stays in place for future narrowly-scoped
exceptions.
Diff is roughly −700 lines net from server.rs (now ~5,700 down
from ~6,300), spread across three new feature-slice files and two
new platform modules. No behavior change — this is a pure
relocation + one type-boundary upgrade.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(gateway): lock in pairing boundary tests + tighten layout helper visibility — PR #2665
Two valid findings from the PR #2665 review:
- `read_layout_config` was carried over from `handlers/frontend.rs` as
`pub`, but every caller lives inside `src/channels/web/`. Tightened
to `pub(crate)` to match the rest of the workspace/widget helpers in
the same module (Copilot).
- Added regression coverage for the new 400 boundary in
`features/pairing/` — `parse_channel` now has 8 unit tests pinning
that it accepts the lowercase / snake_case shapes pairing uses and
lowercases mixed-case URL paths, and that it rejects empty, path
traversal, invalid chars, consecutive underscores, edge
underscores, and oversized input with `StatusCode::BAD_REQUEST`.
This locks in the stricter contract the PR introduced so a future
edit can't accidentally regress to silent canonicalization (Copilot).
The third review note (Gemini: `engine_v2` + `engine_v2_enabled`
redundancy in `GatewayStatusResponse`) is a pre-existing wire-contract
shape — `crates/ironclaw_gateway/static/app.js:8120` reads
`engine_v2` and `app.js:8130` reads `engine_v2_enabled`, so dropping
either field without a coordinated frontend change would regress the
browser UI. Out of scope for this PR's pure relocation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(gateway): parse_channel preserves hyphens for slack-relay — PR #2665 review
Copilot's round-2 review caught a real regression I introduced in this
PR: `parse_channel` returned `ExtensionName::new(...)` directly, and
`ExtensionName`'s canonical form folds `-` into `_`. The pairing store
(via `crate::pairing::normalize_channel_name` in `src/pairing/mod.rs`)
only lowercases — it does *not* fold hyphens — so the live WASM channel
`slack-relay` (see `src/channels/wasm/setup.rs` and
`crate::channels::relay::DEFAULT_RELAY_NAME`) stores hyphenated rows
that a folded `slack_relay` query would silently miss. Empty pairing
lists and failed approvals for every `slack-relay` code.
Fix: keep `ExtensionName::new` at the boundary for its rejection
semantics (path traversal, invalid chars, oversize, edge/consecutive
underscores) but discard the typed value. `parse_channel` now returns
the pre-fold lowercased `String`, which flows directly into
`pairing_store.list_pending` / `approve` and
`ext_mgr.complete_pairing_approval`. The two AppEvent / dispatch call
sites that need a typed `ExtensionName` wrap via
`ExtensionName::from_trusted` — same escape hatch staging's
`pairing_approve_handler` was using before this PR moved it. The
module docstring spells out why the discard-and-keep-the-raw-string
dance exists.
Regression test added (`parse_channel_preserves_hyphens_for_slack_relay`)
pinning both `slack-relay` and `SLACK-RELAY` round-trip through
`parse_channel` as `slack-relay`. Existing tests updated for the new
`Result<String, _>` return type. 9 tests pass.
Also fixed a stale comment in `platform/router.rs` (Copilot): the
"feature handlers still inline in server.rs pending migration" note
predated the logs/oauth/pairing/status slices being extracted. Rewrote
to describe the current split. And dropped the forward-looking
`derive_onboarding` comment about typing `Extension.name` as
`ExtensionName` — this PR just demonstrated that such a naive swap
would break `slack-relay` and related hyphenated channels, so the
follow-up is larger than "type the 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>
|
||
|
standardtoaster
|
81aec813e1 |
fix(gateway): v2 engine tool_calls persistence + e2e test coverage (#2452)
* test(e2e): add v2 engine tool execution lifecycle tests The v2 engine had zero e2e coverage for the tool call -> result -> response path. This gap was flagged in the #2193 audit and is the same code path that breaks in QA bug #2402 (infinite loop after tool operations). New test file: test_v2_engine_tool_lifecycle.py - Single tool call (echo, time) completes through v2 - Text-only message completes through v2 - Parallel tool calls (2 tools in one response) - Multi-step chain (echo -> result -> time -> result -> completion) - Multi-turn tool usage across conversation turns Mock LLM additions: - "parallel echo and time" trigger for multi-call responses - "multi step echo then time" trigger for sequential chains Also documents that v2 engine does not populate the tool_calls array in chat history (tool names show as "unknown"). This is a separate gap from execution correctness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(gateway): persist v2 engine tool_calls to chat history The v2 engine executed tools correctly but never wrote a `role="tool_calls"` message to the v1 conversation DB. This meant the chat history API returned `tool_calls: []` for all v2 threads, breaking the web UI's tool call display. Fix: after thread completion, extract ActionExecuted/ActionFailed events from the v2 event log and write them as a tool_calls DB row before the assistant response. The v1 history API now shows tool names, results, and errors for v2 engine threads. Steps are evicted from the in-memory store after join_thread, so this reads from the append-only event log instead. E2E test updated to assert tool_calls are populated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use thread internal_messages for tool_calls persistence The events approach used params_summary (input parameters) where result_preview (output) was expected. Thread internal_messages carry the actual tool output in ActionResult messages. Also fixes stale test file docstring that said tool_calls were not populated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: log conversation ID resolution failures instead of swallowing The v1 write_v1_response silently drops errors via .ok(). Don't replicate that -- log a warning so failed tool_calls persistence is diagnosable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review feedback on v2 tool_calls persistence - Drop redundant .chain(thread.messages.iter()) — ActionResult messages only exist in internal_messages - Change tracing::warn! to debug! for fire-and-forget persistence failures (warn corrupts TUI per CLAUDE.md) - Add tool_calls assertions to parallel, multi-step, and multi-turn tests — all 6 tests now verify the core persistence feature - Add result_preview content assertion to echo test for tighter coverage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: cargo fmt + add V24 migration checksum Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: move persist_v2_tool_calls to Completed arm + add unit tests Move persist_v2_tool_calls into the ThreadOutcome::Completed match arm so it only fires for final outcomes. Previously it ran for all outcomes including GatePaused, which caused duplicate/orphaned tool_calls rows when a gate resumed. Also fixes the Completed { response: None } gap where tool_calls were never persisted for threads that completed with tool output but no final text. Add two libsql-backed unit tests for persist_v2_tool_calls verifying correct extraction from internal_messages and skip behavior for text-only threads. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(review): address PR #2452 review follow-ups Three polish items from the PR #2452 review (https://github.com/nearai/ironclaw/pull/2452#pullrequestreview-4135957005), flagged under the Engine v2 review-follow-up tracker issue #2669. 1. **Restore `warn!` for `persist_v2_tool_calls` failures** — commit `ff372e11` changed them to `debug!` citing CLAUDE.md's "background tasks must not use info/warn" rule. That rule is about REPL/TUI corruption; `router.rs` is an HTTP handler path, not a background task. Silent `debug!` hid a user-visible bug (chat history missing `tool_calls` array) unless someone set `RUST_LOG=debug`. All four failure sites (load thread, serialize, resolve conv id, DB write) now emit at `warn!` and include the `thread_id` field for correlation. 2. **Regression test: `persist_v2_tool_calls` must only be called from the `Completed` arm** — commit `652315e8` fixed the original bug where the call was shared across all `ThreadOutcome` variants, causing partial tool executions on `GatePaused` to orphan DB rows that duplicated on resume. The existing unit tests call the function directly, so they cover the write path but not the gating. A future refactor could silently move the call back out of the `Completed` arm and nothing would fail. The new `persist_v2_tool_calls_only_called_from_completed_arm` test parses the source of `router.rs`, asserts exactly one call site, and asserts that site sits between the `Completed` and `GatePaused` match arms. 3. **Multi-byte UTF-8 truncation test** — the 500-byte preview truncation uses `char_indices()` + `len_utf8()` to avoid slicing mid-char. Behavior was correct but unexercised. New test constructs an ActionResult with 400 × 3-byte CJK chars (1200 bytes) and pins (a) no panic, (b) valid UTF-8 (via JSON round-trip), (c) body length < 500+max_char_width, (d) body contains only complete 3-byte chars. Verified: `cargo fmt`, `cargo clippy --no-default-features --features libsql --tests -- -D warnings` (0 warnings), `cargo test -p ironclaw --lib --features libsql` (5125 passed, +3 new). 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> |
||
|
firat.sertgoz
|
08693aa3cc |
feat(skills): activation feedback pipeline + install idempotence (#2530)
* feat(events): SkillActivated carries activation feedback notes
Add an optional `feedback: Vec<String>` field to the SkillActivated
event so the engine and selector can surface human-readable activation
notes (chain-load reasons, marker exclusions, scoring summaries) to the
UI. Wire the field through the StatusUpdate, the SSE bridge, and the
gateway's activity timeline; serialize-skip empty vectors so the wire
format stays backwards compatible.
* fix(skills): skill_install never prompts when skill is already loaded
When the LLM force-activates a persona via `/ceo-setup` it sometimes
follows up with a redundant `skill_install("ceo-setup")` call. The
`execute` path was already idempotent (returns `already_installed`
without touching the catalog), but `requires_approval` still gated
the call behind a confirmation prompt — pure friction on a guaranteed
no-op.
Mirror the idempotent shortcut in `requires_approval`: when a skill
with the requested name is already loaded (bundled, user, workspace,
or previously installed), return `ApprovalRequirement::Never`. The
shortcut wins even when `install_dependencies=true` because the
top-level execute is still a no-op (companions get reconciled by their
own activation paths). Regression test covers all three cases.
* fix(skills): preserve approval for dependency installs
* fix(events): include feedback in AppEvent::SkillActivated all-variants list
The variant-enumeration constructor in event.rs:501 was missed when
the new `feedback` field was added to AppEvent::SkillActivated, breaking
the build with E0063. All three Clippy CI jobs failed on this.
Regression: covered by `cargo build --all-features`, which fails to
compile if any variant in this list is constructed with missing fields.
* feat(skills): wire up v1 feedback producer for SkillActivated
The `SkillActivated` event carried an empty `feedback` field because
nothing populated it. This adds the producer end of the pipeline.
**Selector:**
- `prefilter_skills` now returns `SelectionOutcome { selected, notes }`.
- `try_select` returns a reason enum (`Selected`, `BudgetFull`,
`CandidateLimit`, `MarkerSatisfied`, `AlreadySelected`) so callers
can render distinct notes instead of opaque "skipped".
- Notes generated for:
- `<companion>: chain-loaded from <parent>`
- `<companion>: chain-load skipped (budget full)`
- `<companion>: chain-load skipped (max active skills reached)`
- `<companion>: chain-load skipped (setup already complete)`
- `<skill>: skipped (skill context budget exhausted)` for parents
that scored but didn't fit.
**Agent loop:**
- `select_active_skills` returns the notes alongside selected skills
and prepends a `<skill>: force-activated via /mention` note for each
explicit mention.
**Dispatcher:**
- Emits `StatusUpdate::SkillActivated { skill_names, feedback }` via
`channels.send_status` whenever something activated or notes exist
(so "nothing loaded because budget exhausted" surfaces too).
- Silent when nothing activated and no notes — no UI noise.
**Stale comment:**
- Router's v2-bridge comment no longer claims v1 callers populate
feedback "directly on `StatusUpdate`"; the v1 dispatcher now emits
its own event, and v2 remains empty until the Python orchestrator
is updated.
Regression: existing selector test `test_chain_load_respects_budget`,
`test_chain_load_skips_companion_with_satisfied_marker`, and
`test_chain_load_is_non_transitive` now also assert that the
corresponding note is in `outcome.notes`. The 42 selector tests and
503 agent-module tests all pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Illia Polosukhin
|
43d6fc16b0 |
feat(engine-v2): Phase 4 cost tracking + Phase 6 mission lifecycle acceptance (#2660)
* feat(engine-v2): Phase 4 cost tracking + Phase 6 mission lifecycle acceptance Closes two gaps blocking v2 engine becoming the default: **Phase 4 — token + cost accounting** - Delete orphaned `crates/ironclaw_engine/src/executor/compaction.rs` (176 lines). The Python orchestrator (`default.py::compact_if_needed`) has owned compaction policy since #1557; the Rust module had no callers anywhere in the workspace. - Wire `cost_usd` in `LlmBridgeAdapter` by calling `LlmProvider::calculate_cost()` at both the no-tools and with-tools response paths. The engine's `Thread::total_cost_usd` accumulator and `max_budget_usd` gates were already plumbed — only the adapter was hardcoding 0.0. - Persist `total_cost_usd` through `ThreadArchiveSummary` round-trip in `store_adapter.rs`. Previously, rehydrating an archived thread silently dropped the cost to 0.0. `#[serde(default)]` keeps existing archive files deserializing cleanly. **Phase 6 — mission lifecycle acceptance** Three new integration tests in `bridge/effect_adapter.rs` driving `execute_action()` end-to-end (per `.claude/rules/testing.md` "Test Through the Caller"): - `mission_full_lifecycle_via_execute_action` — create → list → complete → list, asserting the `Completed` status surfaces through `mission_list` after `mission_complete`. - `mission_fire_returns_thread_id_for_manual_cadence_via_execute_action` — fresh manual mission fires successfully and returns a UUID thread_id rather than `not_fired`. - `mission_list_returns_all_user_missions_via_execute_action` — all three created missions appear in `mission_list` output. **Regression tests for cost wiring** Three new tests in `bridge/llm_adapter.rs`: - `complete_no_tools_populates_cost_usd_through_adapter` - `complete_with_tools_populates_cost_usd_through_adapter` - `complete_routes_subcalls_through_cheap_provider_for_cost` — pins that `depth > 0` is priced with the cheap provider, not the primary. Coordinated with in-flight work: skipped paths owned by #2504 (auth E2E), #2631 (paused-lease resume), #2570 (mission re-fire), #2549 (mission_get), #2452 (tool_calls persistence), #2621 (replay snapshot). Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5079 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(engine-v2): surface engine capability actions to LLM via available_actions Fixes the gap called out in the PR body: `EffectBridgeAdapter::available_actions` was only enumerating v1 `ToolRegistry` tools + latent OAuth actions, so engine-native capabilities like `missions` never appeared in the LLM's tools list even when a thread held an active lease for them. The LLM was therefore unable to call `mission_create` / `mission_list` / etc. via structured tool calls; the only ways to drive missions were CodeAct Python calls (which relied on the same `known_actions` set and hit the same gap) or `/routine` slash commands falling through to v1. Wire `CapabilityRegistry` into the adapter and iterate active leases to surface every leased, engine-registered capability action. Respects lease grant scope — a lease granting only `mission_list` does not leak `mission_create`. Skips the `"tools"` capability since that lease is already reconciled from the v1 path. Router wires the shared `Arc<CapabilityRegistry>` to both the adapter and `ThreadManager` at setup. Three new regression tests: - `available_actions_surfaces_leased_mission_capability` - `available_actions_respects_partial_lease_grant` - `available_actions_omits_capability_without_lease` Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5082 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(engine-v2): close review gaps — archive round-trip, v1/engine merge, defensive filters Addresses gaps raised in PR #2660 review: - **Consolidate `use ironclaw_engine::{...}`** into a single grouped import in `effect_adapter.rs` (was split across two statements). - **Apply `is_v1_only_tool` / `is_v1_auth_tool` filters** to the engine capability path in `available_actions`. Defensive guardrail: a future engine capability that registers an action under a v1-denylisted name (`create_job`, `tool_auth`, ...) must not bypass the v2-isolation filters by virtue of coming through a different capability registry. - **`ThreadArchiveSummary` serialization round-trip tests** in `store_adapter.rs`: - `archive_summary_preserves_total_cost_usd_through_round_trip` — pins the regression the PR fixed (cost silently zeroed on rehydration). - `archive_summary_handles_legacy_json_without_total_cost_usd_field` — pins `#[serde(default)]` back-compat for archive files written before this PR. - **`available_actions` combined advertising tests** in `effect_adapter.rs`: - `available_actions_merges_v1_tools_with_engine_capabilities` — v1 tool + mission capability both surface on one call. - `available_actions_filters_v1_denylisted_names_from_engine_capabilities` — pins the new defensive filter. - **`cost_usd_from` subscription-billed-provider test** in `llm_adapter.rs`: - `complete_with_subscription_billed_provider_yields_zero_cost` — zero `cost_per_token` round-trips to exactly `0.0`, no NaN/Inf. Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5087 passed, +5 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine-v2): price cache tokens correctly in LlmBridgeAdapter Addresses PR #2660 review (gemini-code-assist + Copilot, L23/L115/L189): `cost_usd_from` only priced `input_tokens + output_tokens`, ignoring `cache_read_input_tokens` and `cache_creation_input_tokens`. For providers with prompt caching (Anthropic, OpenAI), this undercounted input cost and silently neutered the `max_budget_usd` gate. Extend the helper to mirror the canonical formula in `src/agent/cost_guard.rs::CostGuard::record_llm_call`: uncached_input = input_tokens - (cache_read + cache_write) cache_read_cost = input_rate * cache_read / cache_read_discount() cache_write_cost = input_rate * cache_write * cache_write_multiplier() cost = input_rate * uncached_input + cache_read_cost + cache_write_cost + output_rate * output_tokens All `LlmProvider` implementations already supply `cache_read_discount()` (default 1, Anthropic 10, OpenAI 2) and `cache_write_multiplier()` (default 1, Anthropic 1.25 for 5m / 2.0 for 1h) through the decorator chain, so no trait surgery is required. Regression test: `complete_prices_cache_tokens_with_discount_and_multiplier` uses Anthropic Sonnet 5m-TTL rates, exercises a 10k-input / 2k-read / 1k-write / 500-output response, and pins the correct total ($0.03285) against the old naive $0.0375 that would have undercounted ~14%. Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (435 passed), `cargo test -p ironclaw --lib` (5136 passed). 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
|
4104e87b65 |
fix(secrets): TOCTOU-safe auto-generate, lazy keychain probe, fail-loud on stale DB (#2653)
* fix(secrets): prefer keychain over env, TOCTOU-safe generate, testable resolve Follow-up to PR #2648 addressing the four items raised in its review: 1. **Probe order swap** — `SecretsConfig::resolve()` now probes the OS keychain first, then `SECRETS_MASTER_KEY` env var, then auto-generate. Keychain storage is OS-encrypted and the stronger substrate; env var remains the CI/Docker escape hatch when no keychain exists. When both are present with different keys, keychain wins. 2. **TOCTOU safety** — before writing a newly-generated key to `.env`, re-read the file to detect a concurrent writer's key. If present, use it instead of overwriting. Closes the common-case P1-wrote-while- P2-mid-generate race; a residual microsecond window remains between the re-check and the write (a full fix would need a file lock). 3. **Zeroization analysis** — intermediate hex `String`s flow through `SecretString` but aren't zeroized. Documented that this is acceptable because the durable leak surface is `~/.ironclaw/.env` in plaintext, not heap fragments. 4. **Dead branch** — removed the unreachable `KeySource::None` arm in `auto_setup_security`'s message match (replaced with `unreachable!()`), and added a test that asserts keychain wins when both sources are present. Refactor: `resolve_inner` now takes an injected keychain probe result and an `allow_keychain_persist` flag so tests drive every branch deterministically without touching the real OS keychain (previously hung on macOS dev machines waiting for Keychain Access dialogs). Also adds `crate::config::clear_injected_var` (test-only) so tests that exercise the `inject_single_var` path can clean up the overlay and avoid cross-test contamination. Tests: - `keychain_wins_over_env_when_both_present` — probe-order invariant - `env_var_wins_when_keychain_empty` — CI fallback still works - `resolve_persists_generated_key_when_nothing_available` — regression test for #1820, now deterministic - `toctou_picks_up_concurrent_writer` — new TOCTOU regression - `short_env_key_is_rejected` — AES-256 length invariant - `read_secrets_master_key_*` — helper unit tests All run in parallel without mutex contention. * fix(secrets): revert probe-order flip; fail loudly on stale DB with fresh key Addresses PR #2653 review feedback. - Revert the probe-order change: env-first, keychain-second, auto-generate third. The previous flip diverged from every other master-key reader (SetupWizard::step_security, SetupWizard::init_secrets_context, crate::secrets::resolve_master_key, cli import), creating a correctness hazard where onboarding could encrypt a row with one key and a later startup read it with another. Also restores "explicit env var wins" and avoids the unnecessary macOS Keychain Access dialog that an eager probe triggered even when SECRETS_MASTER_KEY was set. - Make the keychain probe lazy in resolve_with_env_path: only call keychain::get_master_key() when SECRETS_MASTER_KEY is unset. - Fix read_secrets_master_key TOCTOU parser: `split_once('=')?` bailed out of the whole scan on the first non-KEY=VALUE line (blank lines, comments), defeating the re-check on any real .env. Now continues past non-assignment lines. Raised by gemini-code-assist, Copilot, and @serrrfirat (3 dupes). - New safety gate: if resolve falls through to auto_generate_and_persist, mark SecretsConfig.generated = true. AppBuilder::init_secrets now calls SecretsStore::any_exist() and errors out when a fresh key meets a populated secrets table — those rows were encrypted with a different key and silently continuing would shadow unrecoverable data. Default trait impl returns false; Postgres, libSQL, and in-memory backends override with real probes. Tests: - env_wins_over_keychain_when_both_present (replaces keychain-wins) - keychain_wins_when_env_unset (new keychain-fallback coverage) - generated_flag_tracks_auto_generate_path (flag-propagation invariant) - read_secrets_master_key_skips_blank_and_comment_lines (TOCTOU regression) - any_exist_reflects_global_store_state (safety-gate backing query) * fix(secrets): address safety-gate review findings - TOCTOU-reuse branch now returns `generated = false`. When P2's `auto_generate_and_persist` picks up the key P1 concurrently wrote to `.env`, P2's key matches whatever rows P1 has encrypted — treating it as a fresh generate would cause `init_secrets` to spuriously abort the moment P1 wrote its first row. - Extract the safety gate into `crate::secrets::verify_generated_key_safe` with a dedicated `GeneratedKeySafetyError` (two variants: `StoreAlreadyPopulated`, `ProbeFailed`). `init_secrets` now calls it and `?`-propagates. Fail-closed on probe error: the previous warn-and-continue defeated the purpose of the gate when the DB was transiently broken. - `RecordingSecretsStore` mock now delegates `any_exist` to its inner store, matching its delegation pattern for every other method. - Refresh `auto_generate_and_persist` doc comment — keychain-first is conditional on `allow_keychain_persist`. Tests: - `toctou_picks_up_concurrent_writer` now asserts `!cfg.generated`. - `generated_flag_tracks_auto_generate_path` defensively clears the injected-var overlay between branches so leaked state from a prior test can't flip the branch under test. - New `verify_generated_key_safe_*` tests cover: non-generated key + populated store (must pass), generated key + empty store (first- install happy path), generated key + populated store (must fail with `StoreAlreadyPopulated` and mention the remediation env var), probe error (must fail-closed with `ProbeFailed`; `generated = false` must short-circuit before touching the probe). * fix(secrets): roll back persistence when safety gate rejects fresh key Addresses Copilot review finding on PR #2653. `auto_generate_and_persist` writes the fresh key to keychain or `~/.ironclaw/.env` *before* `init_secrets` runs the safety gate. Without rollback, a failed gate left the key persisted, so the next restart would read it back as `source = Env/Keychain, generated = false`, skip the gate, and silently accept a key that cannot decrypt the existing rows — exactly the data-shadowing the gate exists to prevent. `crate::secrets::rollback_generated_key_persistence(source, env_path)` now undoes the persistence on gate failure (best-effort; failures are logged and swallowed since the gate's abort is the primary user signal). Supporting `bootstrap::remove_bootstrap_var_to(path, key)` strips a single line from `.env` while preserving the rest. `init_secrets` wires both together: on gate failure, roll back when `generated = true`, then propagate the original gate error. Tests: - `rollback_removes_generated_env_key` — `.env` path, preserves siblings. - `rollback_tolerates_missing_env_file` — idempotent (gate re-fires). - `rollback_with_source_none_is_a_noop` — defensive against the never- actually-produced `generated=true + source=None` pair. |
||
|
Illia Polosukhin
|
0af0267125 |
feat(engine-v2): per-project sandbox (Phases 1–7) (#2211)
* feat(engine-v2): mount-backend abstraction for per-project sandbox (Phase 1) Adds the engine-side `MountBackend` trait + minimal `WorkspaceMounts` registry and a host-side bridge interceptor that routes sandbox-eligible tool calls (`file_read`, `file_write`, `list_dir`, `apply_patch`, `shell`) through a backend when their path argument starts with `/project/`. Default behavior is unchanged: until `EffectBridgeAdapter::set_workspace_mounts(Some(...))` is called (Phase 6), the interception path is dormant. This is the first phase of the per-project sandbox plan (`docs/plans/2026-04-10-engine-v2-sandbox.md`) and a deliberately small subset of the unified Workspace VFS proposed in nearai/ironclaw#1894 — just enough abstraction so the sandbox can be a `MountBackend` rather than a special case in the bridge. When #1894's full mount table lands, the sandbox backend slots in unchanged. Engine crate (`crates/ironclaw_engine/src/workspace/`): - `mount.rs` — `MountBackend` trait, `MountError` (NotFound / InvalidPath / PermissionDenied / Io / Tool / Backend / Unsupported), `DirEntry`, `EntryKind`, `ShellOutput` - `filesystem.rs` — `FilesystemBackend`: passthrough host-fs implementation with two-layer path validation (lexical reject of absolute / `..`, then symlink-escape canonicalization). `read`/`write`/`list` fully implemented; `patch`/`shell` return `Unsupported` so the bridge falls through to the host tool until Phase 5 - `registry.rs` — `WorkspaceMounts` per-project registry with lazy `ProjectMountFactory`, longest-prefix-match resolution, cached and invalidatable Bridge (`src/bridge/sandbox/`): - `intercept.rs` — `maybe_intercept` and `SANDBOX_TOOL_NAMES`. Returns `Handled(json)` on a successful backend dispatch, `FellThrough` for non-sandbox tools, host paths, missing path params, or `Unsupported` backend ops - `effect_adapter.rs` — `workspace_mounts` field + `set_workspace_mounts` setter; interception block in `execute_action_internal` right before `execute_tool_with_safety`, gated on the optional mount table Tests (31 new): - 17 engine workspace unit tests covering trait error mapping, path safety (lexical + symlink), longest-prefix routing, and lazy factory caching - 9 bridge sandbox unit tests including `intercept_actually_dispatches_into_backend` (counting backend) which proves the interceptor reaches the backend - 5 integration tests in `tests/engine_v2_sandbox_integration.rs` driving `EffectBridgeAdapter::execute_action()` end-to-end per the "Test Through the Caller" rule (`.claude/rules/testing.md`), including a host-path-falls-through test that asserts the sandbox tempdir was not touched, and a `..`-escape test that verifies no `/etc/passwd` content leaks even after safety-layer redaction Drive-by: feature-gate two pre-existing dead-code helpers in `crates/ironclaw_skills/src/parser.rs` on `#[cfg(feature = "registry")]` to match their only call site, fixing a pre-existing clippy warning that blocked the workspace's `-D warnings` policy when `ironclaw_skills` is built with `default-features = false` (as the engine crate does). Verification: - `cargo fmt --check` clean - `cargo clippy --all --benches --tests --examples --all-features` zero warnings - 31 / 31 new tests passing; no existing tests broken Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(engine-v2): per-project sandbox — Phases 2–7 + live Docker e2e test Completes the per-project sandbox plan (docs/plans/2026-04-10-engine-v2-sandbox.md Phases 2–7), building on Phase 1's mount-backend abstraction (#2211). Phase 2 — Project workspace folder: - `Project.workspace_path: Option<PathBuf>` field + `with_workspace_path()` - Host-side `project_workspace_path()`, `ensure_project_workspace_dir()` (creates `~/.ironclaw/projects/<id>/` mode 0700, idempotent) - `FilesystemMountFactory` taking a `ProjectPathResolver` closure (decoupled from `Store`); wired into `EffectBridgeAdapter` via `set_workspace_mounts()` Phase 3 — Standalone daemon binary: - `src/bin/sandbox_daemon.rs` — NDJSON over stdin/stdout, health/shutdown/execute_tool - Constructs ReadFileTool/WriteFileTool/ListDirTool/ApplyPatchTool/ShellTool with `base_dir=/project` (override via `IRONCLAW_SANDBOX_BASE_DIR`) Phase 4 — Dockerfile.sandbox: - Multi-stage build: rust-slim builder (+ python3 for pyo3) compiles sandbox_daemon; debian-slim runtime with tini PID 1, common build tools, `/project` mount target Phase 5 — ProjectSandboxManager + ContainerizedFilesystemBackend: - protocol.rs: Request/Response/RpcError matching daemon wire format - transport.rs: `SandboxTransport` trait (seam for testing without Docker) - containerized_backend.rs: `ContainerizedFilesystemBackend` impls `MountBackend`, translates relative→`/project/<rel>`, maps tool-error→MountError - docker_transport.rs: real bollard exec session, serialized Mutex, lazy reconnect - lifecycle.rs: deterministic `ironclaw-sandbox-<pid>` naming, ensure_running/stop/remove - manager.rs: `ProjectSandboxManager` per-project transport cache Phase 6 — Router gating on ENGINE_V2_SANDBOX: - `engine_v2_sandbox_enabled()` helper (truthy: 1/true/yes/on) - Router selects `ContainerizedMountFactory` when enabled + Docker reachable; falls back to `FilesystemMountFactory` with warning otherwise Live e2e bugs caught and fixed: - Shell without explicit `workdir` defaulted to host (not sandbox); fixed by defaulting to `/project/` in `extract_path_param` - `ContainerizedFilesystemBackend::shell` parsed `stdout`/`stderr` but host ShellTool returns merged `output` field; fixed with fallback key lookup - SANDBOX_TOOL_NAMES only had v2 names (`file_read`/`file_write`) but host registry uses v1 names (`read_file`/`write_file`); added both aliases Tests (62 sandbox-related, all green): - 27 bridge sandbox unit tests (intercept, workspace_path, factory, protocol, lifecycle, containerized_backend with ScriptedTransport mock) - 7 containerized-backend tests (including 2 regression tests for the shell bugs) - 5 engine v2 sandbox integration tests (EffectBridgeAdapter end-to-end) - 5 daemon binary smoke tests (real subprocess + NDJSON I/O) - 17 engine workspace unit tests - 1 live Docker e2e test: agent clones nearai/ironclaw into sandbox, renames to megaclaw via sed, verifies with grep — 70s, $0.09, recorded trace committed Verification: - `cargo fmt --check` clean - `cargo clippy --all --benches --tests --examples --all-features` zero warnings - All 62 sandbox tests passing; no existing tests broken Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: replace .expect() with Result in DockerTransport::ensure_session CI's no-panics checker flagged the .expect("just inserted") in production code. Replace with .ok_or_else() returning MountError::Backend. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: multi-tenant project paths + unify sandbox env var with v1 Two issues addressed: 1. Project workspace paths now namespace by user_id: `~/.ironclaw/projects/<user_id>/<project_id>/` instead of `~/.ironclaw/projects/<project_id>/`. Prevents filesystem collisions in multi-tenant deployments where two users could theoretically have the same project UUID. 2. Sandbox enablement now reads `SANDBOX_ENABLED` (same env var as v1 sandbox) in addition to `ENGINE_V2_SANDBOX`. Either being truthy enables the per-project sandbox. This means a single flag governs sandbox behavior regardless of engine version, while the v2-specific override remains available for transitional setups. Tests: 30 bridge sandbox unit tests passing (added multi-tenant path tests + env var combination tests). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review — TOCTOU race, shell env passthrough, canonicalize guard Three issues flagged by the code review bot on #2211: 1. TOCTOU race in WorkspaceMounts::resolve (HIGH): Added double-checked locking — re-check the cache after acquiring the write lock so two threads racing on the same project's first access don't both call factory.build(). The second thread finds the insert from the first. 2. Shell intercept ignores env parameter (MEDIUM): The shell arm in maybe_intercept was passing HashMap::new() instead of forwarding the tool call's env map. Fixed to parse parameters["env"] and pass it through to backend.shell(). 3. Canonicalization fails when root doesn't exist (MEDIUM): When self.root hasn't been created yet (first write to a new project), canonicalize_under_root would walk up to a real ancestor and the starts_with check against the non-existent root would always fail. Now skips canonicalization entirely when root doesn't exist — lexical safety is already guaranteed by safe_join. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review round 2 — apply_patch schema, content validation, dir perms, docs - Fix apply_patch schema mismatch: MountBackend::patch now takes (old_string, new_string, replace_all) matching ApplyPatchTool's actual contract. Previously sent {patch: diff} which would fail with invalid_params in the containerized daemon. - Validate file_write content param: return error instead of silently writing empty string when content is missing. - Log stderr frames from sandbox daemon at debug! instead of silently discarding them in docker_transport StreamReader. - Tighten permissions on intermediate directories created by ensure_project_workspace_dir (projects/, <user_id>/) to 0o700, not just the leaf. - Fix stale module doc in sandbox/mod.rs (referenced "Phase 5 will add" but all phases shipped). - Fix doc path mismatch: workspace path is <user_id>/<project_id>/, not <project_id>/ (workspace_path.rs, CLAUDE.md, design plan). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review round 3 — symlink safety, visibility, debug logging - Close TOCTOU window in canonicalize_under_root: re-canonicalize and verify containment when the reassembled path exists on disk - Fix list_dir_recursive: use symlink_metadata (lstat) so symlinks are detected instead of followed; validate directories against root before recursive traversal - Tighten is_mountable_path to /project/, /memory/, /home/ prefixes instead of any absolute path (defense-in-depth) - Narrow sandbox module visibility to pub(crate) and remove unused pub use re-exports - Remove concrete types (FilesystemBackend, DirEntry, EntryKind, ShellOutput) from engine crate top-level re-exports; access via ironclaw_engine::workspace:: module path - Add debug! tracing to sandbox intercept routing decisions - Add read_file/write_file v1 aliases to daemon SUPPORTED_TOOLS health response - Remove developer-local path from sandbox mod.rs doc comment - Merge staging to fix CI (user_timezone field on ThreadExecutionContext) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review round 4 — safety validation, network isolation, binary writes - Add pre-intercept safety param validation so sandbox-dispatched calls go through the same checks as host-dispatched calls (#1) - Set network_mode: "none" on sandbox containers to prevent outbound network access (#3) - Reject binary content in containerized write instead of silently corrupting via from_utf8_lossy (#5) - Cap list_dir depth to 10 to prevent unbounded traversal (#8) - Change container creation log from info! to debug! to avoid breaking REPL/TUI output (#10) - Make is_truthy case-insensitive so SANDBOX_ENABLED=True works (#11) - Return error instead of unwrap_or_default for missing container ID (#12) - Propagate set_permissions errors instead of silently ignoring (#13) - Return error for missing daemon output key instead of defaulting to empty object (#14) - Add env mutex guard in sandbox_live_e2e test (#15) - Fix rustfmt formatting for let-chain in canonicalize_under_root Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review round 5 — path traversal, error types, tests Security fixes: - Sanitize user_id in workspace path to prevent directory traversal via malicious user IDs containing `..` or `/` - Add Component::ParentDir check in ContainerizedFilesystemBackend::container_path matching the defense-in-depth approach of FilesystemBackend::safe_join Correctness: - Use MountError::Tool instead of MountError::InvalidPath for missing tool parameters (content, old_string, new_string) — fixes confusing LLM-visible error messages - Fix clippy sort_by_key suggestion in registry.rs Cleanup: - Remove spurious Notify import and dead _notify_link function New tests: - ContainerizedFilesystemBackend path traversal rejection (read + write) - container_path unit tests for safe and unsafe paths - Adversarial user_id test in workspace_path - Daemon-side path traversal test in sandbox_daemon_smoke Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review round 6 — param normalization, error types, edge cases - Normalize sandbox params via prepare_tool_params() before validation, matching the host execution path (fixes inconsistent validation) - Return ToolError::InvalidParameters instead of EngineError::Effect for sandbox param validation failures (consistent error surface) - ensure_dir checks path.is_dir() not path.exists() (rejects files) - Empty user_id returns "_anonymous" sentinel instead of empty hex string that would drop the tenant namespace via PathBuf::join("") - Restore ENGINE_V2_SANDBOX env var after sandbox live E2E test - Tighten is_mountable_path to /project/ only (no mounts for /memory/ or /home/ yet) - Add v1 tool name aliases (read_file, write_file) to SUPPORTED_TOOLS Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: unify sandbox env var — remove ENGINE_V2_SANDBOX, use SANDBOX_ENABLED only Single env var controls sandboxing for both engine versions. The transitional ENGINE_V2_SANDBOX override is removed from code, tests, docs, and Dockerfile. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: double-checked locking in transport_for, explicit stdin close in smoke test - ProjectSandboxManager::transport_for no longer holds the mutex across the Docker ensure_running await. Uses double-checked locking so concurrent projects initialize in parallel. - sandbox_daemon_smoke: explicitly take() stdin before wait_with_output so EOF is sent even without a shutdown request. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review — network mode, error types, race, protocol dedup - Change sandbox container network_mode from "none" to default bridge so git clone / cargo build / pip install work inside the container - Fix binary content rejection to use MountError::Tool instead of MountError::InvalidPath (semantic mismatch) - Fix list depth: use actual depth value instead of depth.max(1) - Fix orphan container race in transport_for by holding lock across container creation instead of double-checked locking - Deduplicate protocol types: daemon now imports from shared bridge::sandbox::protocol instead of defining its own copies - Make bridge::sandbox pub (narrow exposure: only protocol and workspace_path sub-modules are pub) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: update plan doc — sandbox uses bridge networking, not network_mode=none Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
cheese
|
3d51423b3a |
fix(llm): default missing OpenAI image detail to auto (#1940)
Co-authored-by: Edward Ji <26658037+edwardji@users.noreply.github.com> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> |
||
|
Illia Polosukhin
|
77c3821f33 |
feat(common): apply ExtensionName newtype to fan-out sites (PR 2/2) (#2617)
* feat(common): add CredentialName and ExtensionName newtypes Introduce typed identifiers for the backend-secret vs user-facing extension identity split that the Extension/Auth Invariants section of CLAUDE.md describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity- confusion bugs with the same shape: a stringly-typed value passed through multiple layers with each layer meaning a different thing. Newtypes make each of those a compile error. This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam (ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth, LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the remaining extension_name: String fields. Wire format is unchanged — both newtypes use #[serde(transparent)] so on- wire and on-disk representations stay plain strings and legacy persisted rows keep deserializing. Validation runs at explicit construction (::new / ::try_from / ::from_str), not at deserialize time. Also adds .claude/rules/types.md codifying the "no stringly-typed internals" rule. Regression coverage: 17 new unit tests in identity.rs; existing auth_manager, router, and gate tests (130+ cases) all pass unchanged. * fix(common): address PR #2611 review feedback Four fixes from Copilot, Gemini, and Claude reviews: - **identity.rs docs**: drop reference to a non-existent `validate()` re-validation API. Document that instances represent "passed validation at some point in history" rather than "guaranteed valid right now" — by design. - **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token` gate path was using `CredentialName::from_trusted` to wrap a value read straight out of a tool's JSON output. Tool output is external/untrusted; use `CredentialName::new` (validating) with a cascade: external → tool name → `from_trusted(tool_name)` as final fallback. Closes a credential-name shape-injection vector. - **canonicalize()**: reorder checks cheapest-first against the trimmed slice so invalid inputs reject without allocating a canonicalized `String`. `replace('-', "_")` is deferred until after the structural checks pass; since `-`/`_` are both one byte, the earlier length check stays valid. - **Remove `Deref<Target = str>`** from identity newtypes, keep `AsRef<str>`. Auto-deref let `&cred_name` silently coerce to `&str`, which is exactly the implicit-conversion pattern these newtypes exist to prevent. Callers that had a `&CredentialName` where `&str` was expected now write `.as_str()` explicitly. Added a regression test for the accessor contract and updated the rule template in `.claude/rules/types.md` to document the decision. Declined one review item (Claude): the remaining `to_string()` calls inside `IdentityError` variants are on the exception path; the common invalid-input case no longer allocates twice after the canonicalize reorder, and errors must carry owned strings so they can escape the function. Regression coverage: 5035 lib tests + 18 identity tests (one new — `explicit_accessors_work`) pass. Zero clippy warnings. * feat(common): apply ExtensionName newtype to fan-out sites (PR 2/2) Follow-up to #2611. Migrates the remaining stringly-typed extension_name and credential_name fields to use the ExtensionName and CredentialName newtypes introduced in ironclaw_common::identity. Fields now typed: - AppEvent::{OnboardingState, GateRequired, ExtensionStatus}.extension_name (serde transparent — wire format unchanged) - StatusUpdate::{AuthRequired, AuthCompleted}.extension_name - TuiEvent::{AuthRequired, AuthCompleted}.extension_name (adds ironclaw_common dep to ironclaw_tui) - PendingOAuthLaunchParams.extension_name - PendingOAuthFlow.extension_name - PendingAuth.extension_name, PendingAuthPrompt.extension_name - ParsedAuthData.extension_name, selected_auth_prompt tuple - emit_auth_required_status() and Session::enter_auth_mode() parameters - event_from_configure_result() parameter - resolve_extension_for_action() and resolve_auth_gate_display_name() return types - normalize_extension_name() return type PendingAuthPrompt::new is now infallible (accepts ExtensionName directly) since the identity validator carries the non-empty invariant the constructor used to re-check. The "blank extension name" rejection test moved out — that logic lives in ironclaw_common::identity tests. Test updates use `ExtensionName::new("...").unwrap()` at construction sites and `from_trusted(...)` where a trusted upstream string is being adapted. Every site is a compile-time audit of where the type was crossing a boundary untyped. Regression coverage: existing 5034 lib tests + 26 engine_v2_gate integration tests + 40 ironclaw_common tests all pass. Zero clippy warnings across all features. * fix(web): return ExtensionName from pending_gate_extension_name Addresses Claude's review comment on #2611: the function was doing `Some(credential_name.as_str().to_string())` in the fallback branch, defeating the newtype's purpose by re-stringifying the identity. Return `Option<ExtensionName>` instead. Plumbs through `PendingGateInfo. extension_name` (wire format unchanged — `#[serde(transparent)]`). The fallback path's cross-identity conversion (credential name → extension name) is now an explicit `ExtensionName::from_trusted` call, making the boundary crossing visible at the call site. Also fixes the `Deref<Target = str>` removal fallout that followed the rebase onto the updated PR 1: call sites that relied on auto-deref (`ext.contains(...)`, `auth_manager.submit_auth_token(&cred_name, ...)`) now explicitly call `.as_str()`. * fix(router,web): address PR #2617 review feedback Four Gemini review comments, all on the boundary between credential/ extension identifiers and user input. 1. [HIGH, security] extensions_setup_submit_handler was wrapping the URL path segment in ExtensionName::from_trusted, which skips the newtype's path-traversal / invalid-character validation. That path is user-controlled (`/api/extensions/{name}/setup`). Validate with ExtensionName::new at the handler entry and return 400 on failure; downstream uses switch to .as_str() or .clone() of the validated value, and the three in-handler from_trusted sites disappear. 2. Rename resolve_auth_gate_display_name -> resolve_auth_gate_extension_name. The function returns an identifier/slug, not a human-readable display name — the old name was a leftover from when the value was a String. 3. Return Option<ExtensionName> from the renamed function. Previously the non-Authentication gate branch fabricated an ExtensionName::from_trusted(pending.action_name), which was semantically wrong (an action name is not an extension identifier) and silently defeated the type's invariants. Now it returns None for Approval/External gates, and callers thread an Option through. send_pending_gate_status accepts Option<&ExtensionName> and only uses it on the Authentication arm, with a warn! log if upstream plumbing ever reaches the arm with None. The GateRequired SSE event's extension_name is now a clean .clone() of the Option. 4. Rename auth_display_name -> extension_name on send_pending_gate_status so the parameter name matches both its type and the StatusUpdate::AuthRequired.extension_name field it feeds. Regression: new test_extensions_setup_submit_rejects_path_traversal_name at the handler tier (per .claude/rules/testing.md "Test Through the Caller, Not Just the Helper") drives the handler with malformed path segments and asserts 400 before the value reaches extension lookup or any from_trusted wrap. 5035 lib tests pass, zero clippy warnings. * docs(identity): codify web-boundary rules + add static check Three rule additions + one enforcement hook covering the identity boundary that PR #2617 review uncovered: - src/channels/web/CLAUDE.md — extend "Unified Extension Onboarding" with explicit rules: * Setup/configure/activate routes MUST validate `{name}` via `ExtensionName::new` at handler entry (return 400 on failure). * Web DTOs and handlers MUST NOT reference `CredentialName` — credential identity is backend-only; the dispatcher/auth_manager resolves it from the ExtensionName server-side. * Auth-flow extension resolution happens in *one* place (`AuthManager::resolve_extension_name_for_auth_flow`). Wrappers are thin and delegate; they must not duplicate the precedence logic or re-derive from credential prefixes. The four recent identity bugs (#2561, #2473, #2512, #2574) were duplicate- resolution drift. - src/bridge/CLAUDE.md — new module spec documenting auth_manager.rs as the single authority for auth-flow extension resolution, with the resolver's four-step precedence order and the approved wrapper call sites. - scripts/pre-commit-safety.sh — new check #8 (CREDNAME): flags `CredentialName` references in newly-added production lines under `src/channels/web/**`. Test-mod code is excluded via the existing `strip_test_mod_lines` filter. Suppression via `// web-identity-exempt: <reason>` for the rare legitimate case of reading an already-typed value off a backend struct. Smoke-tested: * baseline (current branch) — no warnings * injected violation — fires with CREDNAME warning * injected violation + `// web-identity-exempt:` — suppressed The rules and the check live at the same level — humans read the rule, CI enforces it. * fix(auth): validate user-influenced names at the resolver boundary Addresses four Copilot review comments on PR #2617 that all pointed at the same seam: the canonical `AuthManager::resolve_extension_name_for_auth_flow` returned a raw `String` whose first branch (the LLM-supplied `name` parameter on `tool_install` / `tool_activate` / `tool_auth` actions) passed through without `ExtensionName` validation. Both call sites then wrapped the result in `ExtensionName::from_trusted`, promoting an unvalidated user-influenced value to a typed identity. - **Resolver now returns `ExtensionName`.** Branch 1 validates the user-controlled name via `ExtensionName::new` and falls through on failure; branches 2–4 use `from_trusted` because their sources (tool registry hint, canonicalizer, typed credential fallback) are already trusted upstream. This consolidates validation in the single "resolve once" site documented in `src/bridge/CLAUDE.md`. - **router.rs and server.rs drop their wraps.** `resolve_extension_for_action` (router) and `pending_gate_extension_name` (server) return the resolver's typed output directly. The tool-registry fallback in router.rs (no-auth-manager path) keeps its `from_trusted` wrap since it operates on the same trusted sources as branch 2. - **`restore_selected_auth_prompt` re-validates rehydrated prompts.** `PendingAuthPrompt` is `#[serde(transparent)]`, so deserialize does not re-check the inner `ExtensionName` string. A legacy-persisted invalid name would previously have been dropped by the old `PendingAuthPrompt::new(String, ...)` empty-string rejection; now `restore_selected_auth_prompt` re-runs `ExtensionName::new` and drops + warns on failure, upgrading the old non-empty-only check to the full identity invariant. New test `test_restore_selected_auth_prompt_rejects_invalid_legacy_row` forges three invalid rows (empty / uppercase / path-traversal) straight through serde and asserts each is dropped. - **Docstring on `PendingAuthPrompt` refreshed.** The old comment claimed `::new` "trims and validates extension_name is non-empty", which is no longer true — `::new` is infallible and the invariant lives in `ExtensionName` itself. The new comment documents the split: validation runs at `ExtensionName::new` construction and at restore-from-persistence, not inside `PendingAuthPrompt`. Regression: 5063 lib tests pass (+1 new). Clippy zero warnings. * fix(ci): adapt post-merge-from-staging sites to ExtensionName Staging shipped #2640 (repl unlock) and gateway refactor commits after my last merge. The CI build picked them up via auto-merge and hit three type mismatches my branch hadn't seen: - src/channels/repl.rs:908 — new test constructs `StatusUpdate::AuthRequired { extension_name: "google_oauth_token" .to_string(), ... }`. Typed field; now `ExtensionName::new(...).unwrap()`. - src/channels/web/server.rs:1405-1424 — staging added a no-auth-manager fallback chain to `pending_gate_extension_name` that returned raw `Some(String)` on three branches. Aligned with `AuthManager::resolve_extension_name_for_auth_flow`: branch 1 (user-influenced `tool_install`/`tool_activate`/`tool_auth` `name` param) validates via `ExtensionName::new` and falls through on failure; branches 2-3 (provider-extension hint, credential-name fallback) use `from_trusted` because they're sourced from typed upstream state. Mirrors the fix applied to the canonical resolver in |
||
|
Illia Polosukhin
|
7fb41555a9 |
ci(gateway): enforce platform/feature boundaries — ironclaw#2599 stage 5 (#2647)
* refactor(gateway): relocate auth / sse / ws into platform/ — ironclaw#2599 stage 3 Third increment of the ironclaw#2599 platform/feature split (follow-up to #2628 and #2643). Moves the three transport / framing modules into the platform/ subtree so the platform layer now contains the full set of cross-cutting infrastructure (state, router, static_files, auth, sse, ws). Changes: - src/channels/web/auth.rs -> src/channels/web/platform/auth.rs - src/channels/web/sse.rs -> src/channels/web/platform/sse.rs - src/channels/web/ws.rs -> src/channels/web/platform/ws.rs - platform/mod.rs declares the three new submodules. - channels/web/mod.rs adds backward-compat re-exports (`pub use platform::{auth, sse, ws};`) so every existing `crate::channels::web::{auth,sse,ws}::...` call site - roughly 40 files across handlers, tests, integration tests, and sibling modules - continues to resolve without edits. Follow-up PRs will migrate call sites to the canonical `platform::` path incrementally. - platform/mod.rs doc comment now describes the platform layer as having auth / SSE / WS (no longer "in later stages of #2599"). - CLAUDE.md file map points at the new paths and notes the re-exports. Pure move + re-export. No behavior change. Module contents are byte-identical to pre-move. Verified: cargo fmt --all; cargo clippy --all --benches --tests --examples --all-features clean; python3 scripts/check_no_panics.py clean; cargo check --all-features --all-targets clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(gateway): extract OAuth / relay callbacks into features/oauth/ — ironclaw#2599 stage 4a Fourth increment of the ironclaw#2599 platform/feature split. Opens the `features/` subtree with the OAuth feature slice — the first vertical slice to move out of server.rs into its own module under the ironclaw#2599 target layout. Slice contents: - `features/oauth/mod.rs` owns the three public gateway routes that receive OAuth-style callbacks: * `oauth_callback_handler` — generic OAuth callback for installable extensions (CSRF lookup, token exchange, storage, optional auto-activation). * `relay_events_handler` — HMAC-signed webhook from channel-relay. * `slack_relay_oauth_callback_handler` — Slack-specific relay completion flow. - Slice-private helpers `oauth_error_page` and `redact_oauth_state_for_logs` move with the slice (they have no other callers). Wiring: - `platform/router.rs` imports the three handlers from `features::oauth` instead of `server`; no route-table change. - `channels/web/mod.rs` registers `pub(crate) mod features;`. - `server.rs` loses the three handlers and their helpers, plus the imports they owned (`Sha256`, `Digest`, `HeaderMap`, `DEFAULT_RELAY_NAME`, `extension_name_candidates`, `SecretConsumeResult`). The test module re-imports the ones it still uses for the integration-level OAuth callback tests. Pure move. No behavior change. Each handler body is byte-identical to its pre-move counterpart. Every test in `server.rs` that exercises the OAuth callbacks (`test_oauth_callback_missing_params`, etc.) continues to pass against the re-imported handlers. Stats: server.rs 6973 → 6248 lines (−725); new `features/oauth/mod.rs` is 775 lines; new `features/mod.rs` 14 lines. The +30 delta is comment headers documenting the slice boundary. Verified: `cargo fmt --all`; `cargo clippy --all --benches --tests --examples --all-features` clean; `python3 scripts/check_no_panics.py` clean; `cargo test --lib` 5069 passed (one more than stage 3 — the new `css_handler_returns_base_in_multi_tenant_mode` test from staging lands green), same 2 pre-existing failures carried over (fixture and test-infra issues unrelated to gateway layout). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(gateway): enforce platform/feature boundaries — ironclaw#2599 stage 5 Adds `scripts/check_gateway_boundaries.py` and wires it into the `code_style` CI workflow as a required check. The script enforces the ironclaw#2599 layering rule: every file under `src/channels/web/platform/` except `router.rs` must not import from `handlers/` or `features/`. How it works: - Walks `src/channels/web/platform/*.rs`, skipping `router.rs` (the intentional composition point) and test modules. - Strips line comments, block comments, and string / raw-string / char literals so references inside docstrings and explanatory text don't trigger false positives. - Matches six forbidden import shapes: `crate::channels::web::{handlers,features}::`, `super::{handlers,features}::`, `super::super::{handlers,features}::`. - Prints diagnostics with file:line and the matched pattern for every violation; exits non-zero on any. - Carries unit tests behind a `test` subcommand (`python3 scripts/check_gateway_boundaries.py test`) that the CI job runs alongside the check itself. Simultaneous fix: one pre-existing back-edge that the check surfaced was the OIDC `check_email_domain()` helper living in `handlers/auth.rs` but called from `platform/auth.rs`. The helper is platform-level (it gates JWT validation before any handler runs), so it moves into `platform::auth` along with its five unit tests; the handler call site in `handlers::auth::handle_callback` now imports from the new home. No behavior change. The second pre-existing back-edge is the frontend bundle assembly path: `platform/static_files::build_frontend_html` calls `read_layout_config` and `load_resolved_widgets`, both still in `handlers/frontend.rs`. Migrating them requires also moving `read_widget_manifest` and the widget-size constants, which touches `load_widget_manifests` (used by `/api/frontend/widgets` and the engine-v2 widget endpoint). That's a separate focused PR — tracked via a narrow allowlist entry in the script with a follow-up comment. The allowlist is explicitly documented as "must not grow without reviewer sign-off". CLAUDE.md's "Platform vs. feature layering" section now names the script as the enforcement point. Verified: `python3 scripts/check_gateway_boundaries.py test` — 9 tests pass; `python3 scripts/check_gateway_boundaries.py` — clean; `cargo fmt --all`; `cargo clippy --all --benches --tests --examples --all-features` clean; `python3 scripts/check_no_panics.py` clean; `cargo test --lib channels::web` — 425 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(gateway): close boundary-checker bypasses — PR #2647 review Four issues raised on PR #2647's review are addressed: - Grouped `use crate::channels::web::{ handlers::... }` imports escape the per-line scan because the forbidden segment lands on a continuation line. Adds a multiline GROUPED_FORBIDDEN_PATTERN that matches across newlines and reports the line where `handlers::`, `features::`, or `server::` actually appears. - `use crate::channels::web::server::...` routes through the `server.rs` compatibility shim and still creates a platform → feature back-edge. Adds `server::` (and its `super::` variants) to FORBIDDEN_PATTERNS. Existing pre-existing shim usage in `platform/ws.rs` is captured as a tracked allowlist entry — the allowlist shrinks as individual types migrate out of `server.rs`. - `#[cfg(test)] mod ...` and `mod tests { ... }` bodies are now actually blanked before pattern matching, matching the docstring's stated exemption. Caller-level regression tests in platform files can import handler/feature modules without tripping the check. - `gateway-boundaries` is no longer gated solely on `has_code`. A new `has_boundary_check` output on the `changes` job fires when the checker script or this workflow itself changes, so PRs that only edit `scripts/check_gateway_boundaries.py` or `.github/workflows/code_style.yml` still run the guardrail. Also picks up a small perf nit: `text.splitlines()` is now computed once outside the loop instead of per-violation. Regression tests cover each case (grouped crate-web import, grouped super import, server-shim back-edge, cfg(test)/mod tests skip, and a sanity check that the test-module skip doesn't blanket-ignore the rest of the file). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(gateway): brace-aware grouped scan + narrower ws.rs allowlist — PR #2647 Copilot review Two issues raised by Copilot on the round-1 fixes: - `GROUPED_FORBIDDEN_PATTERN` used `[^{}]*?` and so could not match grouped imports that contain *nested* braces — e.g. `use crate::channels::web::{ platform::{state::GatewayState}, handlers::auth::login_handler };` produced zero violations even though the forbidden segment is plainly inside the web::{...} group. Replaced the regex with a depth-tracking walk: find each `crate::channels::web::{` / `super::{` / `super::super::{` header, find the matching `}` by counting braces (`{` / `}` only; string and comment contents are already blanked), then scan the body for `(handlers|features|server)::`. Report line numbers off absolute offsets so the reported line is where the forbidden segment lives, not where the header's `{` is. - `ws.rs`'s allowlist entry whitelisted the whole `crate::channels::web::server::` prefix, which would let any *new* accidental server-shim import in ws.rs silently pass. Narrowed to seven per-symbol entries covering the current pre-existing uses (GatewayState, PerUserRateLimiter, RateLimiter, ActiveConfigSnapshot, images_to_attachments, and the two handle_legacy_auth_* helpers). Future accidental shim imports fail the check and require explicit reviewer sign-off to add. Added `test_detects_nested_brace_grouped_import` as the regression test for the brace-aware scanner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>staging-tested |
||
|
Illia Polosukhin
|
c4927ba6e1 |
fix(ci): unblock staging Docker Build and echo tool E2E test (#2661)
Two independent staging CI regressions: 1. Docker Build was failing because `cargo install wasm-tools@1.246.1` re-resolved to the newest compatible `constant_time_eq@0.4.3`, which requires rustc >= 1.95, while the chef stage is pinned to rust:1.92. Add `--locked` so cargo uses the Cargo.lock shipped with each crate. 2. `test_builtin_echo_tool` started failing after PR #2555 intentionally aligned the in-memory history path with DB semantics: tool previews now surface in `result` with `result_preview` left empty. The test only inspected `result_preview`, so it timed out. Accept the preview from either field in `_wait_for_turn`. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
ff119531d4 |
test(replay): promote engine traces to insta-backed snapshot gate (#2621)
* test(replay): promote engine replay traces to insta-backed snapshot gate Adds a ReplayOutcome snapshot type, a replay-gate CI workflow, and a developer script wrapper for cargo-insta. Replaces unreviewable 3,000-line JSON diffs on engine changes with a YAML snapshot of the observable run shape (tool sequence, final state, retrospective analyzer issues). Why: engine v2 live-fixture traces had grown past reviewability. A single prompt-wording change could move the whole fixture, and reviewers had no way to see which behaviour actually changed. Splitting the fixture into a "replay driver" (JSON stays in tests/fixtures/) and a "regression snapshot" (YAML in tests/snapshots/) gives reviewers a narrow, stable diff to approve, while keeping the full recorded context for deterministic replay. Changes: - `tests/support/replay_outcome.rs` — ReplayOutcome + assert_replay_snapshot! macro; snapshots include retrospective analyzer output (TraceIssue severity/category) via a new `ironclaw::bridge::engine_retrospectives_for_test()` helper that runs `build_trace()` over engine threads - `tests/e2e_engine_v2.rs` — three POC snapshot tests (single_tool_echo, tool_error_recovery, zizmor_scan_v2) - `tests/e2e_bug_bash_snapshots.rs` + `tests/fixtures/llm_traces/bug_bash/` — bug-regression fixture template, mapped to open issues in the README - `.github/workflows/replay-gate.yml` — cargo insta test --check on engine/agent/LLM/tools/bridge path changes; rejects committed .snap.new - `scripts/replay-snap.sh` — review/accept/test/record wrappers around cargo-insta and IRONCLAW_RECORD_TRACE - `scripts/trace-coverage.sh` — reports EventKind variants with snapshot coverage; `--strict` mode for future CI promotion - `tests/e2e_live.rs` — `#[ignore]` swapped for `cfg_attr(not(feature="replay"), ignore)` so the replay CI job can run the scenarios without `-- --ignored` - `Cargo.toml` — new `replay = ["libsql"]` feature; insta gains the `yaml` feature - `tests/fixtures/llm_traces/README.md` — documents the two-role driver/snapshot split Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(replay): address PR #2621 review + swap cargo-insta installer Review fixes: - Replay gate was missing the bug-bash snapshot suite. Adds `tests/e2e_bug_bash_snapshots.rs` to the workflow paths trigger and the `cargo insta test --check` invocation so bug-regression snapshots are actually gated. (copilot-pull-request-reviewer) - `cargo install cargo-insta --locked` added ~40s of cold-cache compile to the gate. Swapped for `taiki-e/install-action@v2`, which downloads a precompiled binary in a few seconds. Also updated `scripts/replay-snap.sh` to *fail closed* when cargo-insta is missing instead of silently auto-installing it. (gemini-code-assist) - `engine_retrospectives_for_test` was `pub` and re-exported under the default-enabled `libsql` feature, contradicting its "not part of any public API" doc. Split the re-export, kept `reset_engine_state` as a plain `pub use`, and hid `engine_retrospectives_for_test` behind `#[doc(hidden)]` — it still needs to cross the crate boundary for integration tests (which live in a separate crate, so `#[cfg(test)]` doesn't reach them), but no longer appears in published docs. (copilot-pull-request-reviewer) - Added an explicit "caller must serialize" note on `engine_retrospectives_for_test` explaining the `ENGINE_STATE` singleton and pointing new callers at `engine_v2_test_lock()` / `reset_engine_state()`. Matches what the existing snapshot tests already do. (gemini-code-assist) Doc corrections: - `snapshot_zizmor_scan_v2` doc claimed the snapshot pinned `ApprovalNeeded` events and response wording — it doesn't. Rewrote to describe what the snapshot actually asserts (tool order, step count, retrospective issues, final state). (copilot-pull-request-reviewer) - `llm_call_count` was documented as "bucketed" but passed through verbatim. Updated the field doc to reflect the raw value. Bucketing wasn't needed because fixtures are deterministic. (copilot-pull-request-reviewer) - `src/bridge/router.rs` doc referenced a non-existent `ReplayOutcome.trace_issues` field — the struct uses `engine_threads`. Fixed the reference. (copilot-pull-request-reviewer) - `scripts/trace-coverage.sh` header claimed CI runs it with `--strict`; the workflow runs it in advisory mode. Rewrote the header to match, with a pointer for when to promote to strict. (copilot-pull-request-reviewer) No-change replies (rationale commented in the code): - `event_kind_name` uses an exhaustive `match` on `EventKind` rather than `Debug` or a `strum` derive. The compile-time exhaustiveness check is the point — adding a new engine event should force a conscious decision about how the snapshot represents it, not a silent fallthrough. Added a comment making that intent explicit. - `trace-coverage.sh` awk parser of `event.rs` is fragile — agreed, but the script is advisory and its failure mode is false negatives (uncovered variants simply aren't gated). Documented the tradeoff and the rewrite-in-Rust escape hatch in the script header. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(replay-gate): prime cache on staging, restrict PR runs to read-only The second run on PR #2621 missed the cache ("No cache found" in the rust-cache restore step) even though the workflow is wired correctly. Root cause: the repo sits close to GitHub's 10 GB per-repo cache quota (~59 entries, many >500 MB), and the LRU policy evicts PR-scoped caches before they get reused. Fix: - Add `push: [staging, main]` so the gate runs (and saves a ~1.2 GB cache under the `replay-gate` key) on every merge to the branches PRs actually target. Subsequent PRs restore from that base-branch cache — GitHub Actions permits cross-ref restore when the restoring ref's base matches the saved ref. - Set `save-if: ${{ github.event_name == 'push' }}` so PR runs only *read* the cache. Without this gate, each PR push would save its own copy and crowd out the primed base-branch cache, putting us right back in the eviction loop. Expected effect: cold-cache 9m → warm ~2-3m once staging has a run with the new workflow. Base-branch prime run still pays 9m (no regression). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(replay): drop bug-bash fixture scaffolding Replay fixtures can't reproduce the Phase 3 target bugs because the fixture *is* the LLM's output — handwriting a trace where the LLM emits a tool call doesn't test whether the real LLM would have emitted that call, only that the harness dispatches a scripted one. What `summarization_uses_tools.json` actually pinned was the happy path, not the #2541 bug. Of the 7 open bug-bash issues, only #2544 ("plans and delegates but never executes") is catchable by replay, and only via a live-recorded fixture. The other six are LLM-behavior or infra-timing bugs outside replay's reach. Rather than ship regression theater, tear out the scaffolding. Removed: - tests/e2e_bug_bash_snapshots.rs - tests/fixtures/llm_traces/bug_bash/ - tests/snapshots/replay__bug_bash_summarization_uses_tools.snap Unwired: - Replay-gate workflow paths + test list no longer mention bug_bash - scripts/replay-snap.sh test command drops the extra --test flag Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: switch to cargo-nextest with per-test timeouts Nextest runs each integration test in its own process and runs test binaries in parallel, which is a big unlock for this repo: - Engine v2 tests share a process-global `ENGINE_STATE` singleton (OnceLock), which the current test lock serialises inside a single test binary. Nextest's process-per-test model gives each test a clean state automatically, so the 16 engine_v2 tests stop running one-by-one. - Cross-binary parallelism: `cargo test --test A --test B` runs binaries in sequence; nextest runs them concurrently. Measured locally: the replay-gate test set (3 binaries, 21 tests) went from ~30s sequential to **2.7s parallel**. Adds `.config/nextest.toml` with: - `slow-timeout = 60s / terminate-after 3` in the default profile so a hung test fails fast instead of blocking the workflow-level 25- minute cap. - A `ci` profile with `fail-fast = false` (one flake shouldn't mask other failures), `failure-output = immediate-final`, `success-output = never` for readable Actions logs. - Per-test 300s override for the handful of genuinely slow scenarios (zizmor scan, e2e_thread_scheduling). Workflows updated: - `replay-gate.yml`: installs cargo-nextest via taiki-e/install-action alongside cargo-insta (one step), runs `cargo insta test --test-runner nextest` with `NEXTEST_PROFILE=ci`. - `test.yml`: all five `cargo test` invocations swapped for `cargo nextest run --profile ci`. Nextest doesn't execute doctests, so every nextest step is paired with a `cargo test --doc` follow-up to preserve coverage. Local dev is unchanged — `cargo test` still works; nextest is only required in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci: re-trigger replay-gate workflow after nextest migration Previous push only modified workflow files and `.config/nextest.toml`; GitHub skipped the `pull_request` workflow events for that sync, so the nextest migration didn't actually get exercised in CI. Empty commit forces re-evaluation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(replay): note nextest wiring in the fixtures README Also forces a CI re-run: the previous empty commit had no matching paths, so the `pull_request.paths` filters skipped every workflow including replay-gate. Touching a file under `tests/fixtures/llm_traces/**` re-matches the filter and runs the nextest-based gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(test): defer test.yml nextest migration Staging restructured test.yml significantly while this PR was open (matrix-config dynamic matrix, `changes` code-detection job, composite install-cargo-component action, save-if restricted to base-branch pushes). The merge into staging had heavy conflicts for every nextest-swap hunk. Rather than force a re-layering of the new staging structure on top of the nextest migration in this PR, revert test.yml to staging's current version. This PR now scopes the nextest change to just the replay-gate workflow (where it cleanly demonstrates the value) plus the shared `.config/nextest.toml` profile. Migrating the rest of test.yml to nextest is a follow-up that can rebase on the new structure without the heavy conflict surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Henry Park <henrypark133@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
11f006987d |
fix(engine): FINAL-await support + runaway loop protection (#2531)
* fix(engine): make FINAL/FINAL_VAR awaitable in CodeAct scripts LLMs frequently emit `await FINAL(answer)` by analogy with async tool calls. The handler returned `MontyObject::None` synchronously, so the subsequent `await` raised `TypeError: 'NoneType' object can't be awaited` and the answer was dropped on the floor. Route FINAL and FINAL_VAR through the existing `pending_futures` mechanism with a trivially-resolving task. `final_answer` is set the moment the call arrives, so both `FINAL(x)` (coroutine discarded) and `await FINAL(x)` (resolves to None) succeed. Two regression tests in `scripting::tests` pin both forms. * fix(engine): normalize typographic punctuation before skill activation iOS, macOS, and most rich-text inputs autocorrect `I'm` (ASCII U+0027) to `I'm` (curly U+2019). Skill activation patterns like `(?i)I'm a (CEO|...)` are authored with ASCII punctuation, so the curly form silently failed to match — `ceo-setup` and any other skill with apostrophes in its regex never activated for autocorrected input. Add `normalize_punctuation()` in `default.py` that folds 8 typographic quote variants and the en/em dashes to ASCII before scoring runs. Implemented with chained `.replace()` because Monty does not expose `str.maketrans`/`.translate()`. Runs once per turn on the goal text; user content is untouched everywhere else. Two regression tests under `executor::orchestrator::tests` drive `normalize_punctuation` and `select_skills` end-to-end with the exact ceo-setup pattern and a curly-apostrophe goal. * docs(codeact): note closure capture quirk and Rust regex limits Two recurring CodeAct failures got their own paragraphs in the preamble: - Function closures defined in one ```repl``` block do not reliably capture names (modules, top-level vars) defined in earlier blocks, producing spurious `NameError`. Authors must keep the helper, its imports, and its call sites in the same block. - The embedded `re` is the Rust `regex` crate, not CPython: positional- only flag args, no lookaround, no backreferences. Lead with string methods (`in`, `startswith`, `splitlines`) and only reach for `re` when truly needed. Pure docs change. * fix(engine): adapt FINAL-await tests to staging CodeExecutionResult API Replace `had_error` field references with `failure.is_none()` to match the staging struct where `had_error: bool` was replaced by `failure: Option<CodeExecutionFailure>`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(engine): address gemini-code-assist review — normalize before extraction, seed explicit skills (#2531) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(skills): remove useless .into_iter() to satisfy clippy (#2531) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(clippy): collapse nested if blocks into match guards (#2531) Fixes collapsible_match lints in ironclaw_tui::render and ironclaw_engine::runtime::mission triggered by Rust 1.95. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(clippy): resolve collapsible_match and unnecessary_sort_by lints (#2531) Fix 5 collapsible_match lints (responses_api, cli/tool, rig_adapter, setup/prompts) and 2 unnecessary_sort_by lints (glob_tool, grep_tool) triggered by Rust 1.95 clippy. Also fix rustfmt formatting for match guards in render.rs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(clippy): group wasm startup channel registration context --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
ce88b6eac7 |
test(e2e): harden tab_button selector against strict-mode duplicates (#2656)
Closes #2626.
`tests/e2e/helpers.py` used `.tab-bar button[data-tab="{tab}"]` to locate
every main-nav tab button. Commit
|
||
|
firat.sertgoz
|
f0be3f8885 |
fix(engine): allow completed event-driven missions to re-fire (#2570)
* fix(engine): allow completed event-driven missions to re-fire on new events The /expected command returned "no self-improvement missions configured" after the learning mission's first thread completed, because: 1. fire_on_system_event() only processed Active missions, skipping Completed ones. Event-driven missions should re-fire since each event is a fresh investigation. 2. fire_mission() rejected all terminal missions via is_terminal(). Now Completed event-driven missions pass through. 3. threads_today counter never reset daily, permanently exhausting the daily budget after enough fires. 4. ensure_learning_missions was only called for the owner at init, so non-owner users never got learning missions. Fixes: add is_event_driven() helper on Mission, allow Completed event-driven missions in all three event-fire methods, add daily reset for threads_today when last_fire_at is on a previous UTC day, and call ensure_learning_missions in handle_expected before firing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: make daily reset persist best-effort (iteration 1) The daily threads_today reset used `?` to propagate store errors, which would abort fire_mission on a transient store failure. Changed to log-and-continue so the in-memory reset still allows the mission to fire. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve CI failures - reduce WASM startup registration arguments via a context struct for clippy - restore post-install auth gate naming and stabilize config tests - make telegram approval fixture optional in tests - add safety annotations for existing staged slice/expect patterns --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
fcb295bad4 |
fix(secrets): auto-generate master key via .env on every startup (#1820) (#2648)
* fix(secrets): auto-generate master key via .env fallback on every startup (#1820) The "secrets store is not available" failure happened when a user reached `require_secrets_store` (web settings → save API key) but `SecretsConfig::resolve()` had returned `KeySource::None` because neither `SECRETS_MASTER_KEY` nor an OS keychain entry existed. This is the normal state on headless Linux, inside a container without secret-service, or after a partial onboarding that wrote `ONBOARD_COMPLETED=true` but didn't persist a key. The wizard's quick-mode `auto_setup_security()` already handled this chain correctly — env var → keychain read → keychain write → generate + persist to `~/.ironclaw/.env` as `SECRETS_MASTER_KEY=…` — but that code only ran during onboarding. If the user's gate slipped past `check_onboard_needed()`, nothing re-ran it. Move the chain into `SecretsConfig::resolve()` so it runs on every startup: - `resolve()` generates and persists a key via `upsert_bootstrap_vars_to()` (the same writer the wizard uses) when keychain is unavailable, and injects it into the process env overlay so the current run sees it. - `auto_setup_security()` collapses to a thin caller: `SecretsConfig::resolve()` → build `SecretsCrypto` → mirror source/hex into settings → print status. This removes the duplicate chain and keeps one persistence format (`.env`) and one `KeySource` (`Env`) for the keychain-unavailable path. Regression test `resolve_persists_generated_key_when_keychain_empty` drives `resolve_with_env_path()` with a tempfile and asserts the `.env` carries the generated key. Skips gracefully when the host keychain already holds a key (developer machines) so we don't wipe a real key. Supersedes #2312, which added a second persistence format (`~/.ironclaw/master.key`) + `KeySource::File` variant + `Keystore` trait alongside the existing `.env` path, with a known divergence-on-recovery gap between the two stores. Option B here reuses the single existing path instead. * ci: re-trigger regression check with skip-regression-check label |
||
|
Illia Polosukhin
|
695e6fa13e |
refactor(gateway): extract OAuth / relay callbacks into features/oauth/ — ironclaw#2599 stage 4a (#2645)
Fourth increment of the ironclaw#2599 platform/feature split. Opens
the `features/` subtree with the OAuth feature slice — the first
vertical slice to move out of server.rs into its own module under
the ironclaw#2599 target layout.
Slice contents:
- `features/oauth/mod.rs` owns the three public gateway routes
that receive OAuth-style callbacks:
* `oauth_callback_handler` — generic OAuth callback for
installable extensions (CSRF lookup, token exchange, storage,
optional auto-activation).
* `relay_events_handler` — HMAC-signed webhook from channel-relay.
* `slack_relay_oauth_callback_handler` — Slack-specific relay
completion flow.
- Slice-private helpers `oauth_error_page` and
`redact_oauth_state_for_logs` move with the slice (they have no
other callers).
Wiring:
- `platform/router.rs` imports the three handlers from
`features::oauth` instead of `server`; no route-table change.
- `channels/web/mod.rs` registers `pub(crate) mod features;`.
- `server.rs` loses the three handlers and their helpers, plus the
imports they owned (`Sha256`, `Digest`, `HeaderMap`,
`DEFAULT_RELAY_NAME`, `extension_name_candidates`,
`SecretConsumeResult`). The test module re-imports the ones it
still uses for the integration-level OAuth callback tests.
Pure move. No behavior change. Each handler body is byte-identical
to its pre-move counterpart. Every test in `server.rs` that exercises
the OAuth callbacks (`test_oauth_callback_missing_params`, etc.)
continues to pass against the re-imported handlers.
Stats: server.rs 6973 → 6248 lines (−725); new `features/oauth/mod.rs`
is 775 lines; new `features/mod.rs` 14 lines. The +30 delta is
comment headers documenting the slice boundary.
Verified: `cargo fmt --all`;
`cargo clippy --all --benches --tests --examples --all-features`
clean; `python3 scripts/check_no_panics.py` clean;
`cargo test --lib` 5069 passed (one more than stage 3 — the new
`css_handler_returns_base_in_multi_tenant_mode` test from staging
lands green), same 2 pre-existing failures carried over (fixture
and test-infra issues unrelated to gateway layout).
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Illia Polosukhin
|
9ea65a5c0c |
refactor(gateway): relocate auth / sse / ws into platform/ — ironclaw#2599 stage 3 (#2644)
Third increment of the ironclaw#2599 platform/feature split (follow-up to #2628 and #2643). Moves the three transport / framing modules into the platform/ subtree so the platform layer now contains the full set of cross-cutting infrastructure (state, router, static_files, auth, sse, ws). Changes: - src/channels/web/auth.rs -> src/channels/web/platform/auth.rs - src/channels/web/sse.rs -> src/channels/web/platform/sse.rs - src/channels/web/ws.rs -> src/channels/web/platform/ws.rs - platform/mod.rs declares the three new submodules. - channels/web/mod.rs adds backward-compat re-exports (`pub use platform::{auth, sse, ws};`) so every existing `crate::channels::web::{auth,sse,ws}::...` call site - roughly 40 files across handlers, tests, integration tests, and sibling modules - continues to resolve without edits. Follow-up PRs will migrate call sites to the canonical `platform::` path incrementally. - platform/mod.rs doc comment now describes the platform layer as having auth / SSE / WS (no longer "in later stages of #2599"). - CLAUDE.md file map points at the new paths and notes the re-exports. Pure move + re-export. No behavior change. Module contents are byte-identical to pre-move. Verified: cargo fmt --all; cargo clippy --all --benches --tests --examples --all-features clean; python3 scripts/check_no_panics.py clean; cargo check --all-features --all-targets clean. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Henry Park
|
2917f1b4f7 |
fix: restore only active WASM channels at startup (#2562)
* fix: restore only active wasm channels at startup * fix: address PR review follow-ups * Merge origin/staging; fix CredentialName newtype + unused import Merge 120 commits from staging. Fix two compilation issues from the merge: test used raw String where CredentialName newtype is now required (staging PR #2611), and unused HashSet import in setup tests after prior redundant-filter removal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR review — owner_id type, naming, whitespace trim - Use build_runtime_config_updates() in register_channel so boot-time owner_id injection matches the post-approval path (numeric when parseable as i64, string otherwise). - Rename persisted_active_channels_raw to persisted_active_channels since load_persisted_active_channels already normalizes. - Trim whitespace from persisted channel names before validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: derive settings_persistence_available from DB, not workspace adapter ExtensionManager::settings_store() falls back to the raw DB when no workspace adapter is set. Check components.db.is_some() so the persistence flag aligns with the actual settings store availability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: use ExtensionName for persisted WASM channel name validation Replace hand-rolled validation in normalize_persisted_wasm_channel_names with ExtensionName::new(), which provides the canonical validation (trim, hyphen→underscore, path traversal, lowercase-only, no consecutive underscores). This aligns startup channel name validation with the rest of the extension system. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
962aaed221 |
refactor(gateway): extract start_server + route composition into platform/router.rs — ironclaw#2599 stage 2 (#2643)
* refactor(gateway): extract start_server and route composition into platform/router.rs — ironclaw#2599 stage 2 Second increment of the ironclaw#2599 platform/feature split. Moves `start_server()` and the Axum route composition out of `server.rs` into a dedicated `platform/router.rs`, so the platform-vs-features dependency direction is visible: the router depends on handler modules (both `handlers/*` and the still-inline handlers in `server.rs`), never the reverse. Changes: - New `src/channels/web/platform/router.rs` owns `start_server()`, the four routers (`public`, `protected`, `statics`, `projects`), and the cross-cutting layer stack (CORS, 10 MB body limit, panic catch, `X-Content-Type-Options`, `X-Frame-Options`, CSP). - Feature handlers still inline in `server.rs` are now `pub(crate)` so the router can register them without leaking them outside the crate. Two private structs that are directly referenced by pub(crate) handlers (`HistoryQuery`, `GatewayStatusResponse`) were also raised to `pub(crate)` so the handler signatures type-check from the router module. - `server.rs` keeps the feature handlers that haven't migrated yet (OAuth callbacks, chat, extensions, pairing, logs, gateway status) and adds `pub use platform::router::start_server` so external call sites — `src/channels/web/mod.rs`, `tests/multi_tenant_integration.rs` — keep working. The trimmed imports drop `Router`, `DefaultBodyLimit`, `CorsLayer`, `tokio::sync::mpsc`, etc., since they're no longer used in the remaining body. - `mod.rs` now calls `platform::router::start_server` directly; the `server::start_server` shim exists only for external code paths that still reach for it. - `CLAUDE.md` file map now lists `platform/router.rs` and clarifies that `server.rs` is feature-handler-only pending migration. No behavior change. Route table, middleware stack, CORS policy, body limits, panic handling, security headers, and CSP are byte-identical to origin/staging. Stats: server.rs 7463 → 6973 lines (−490); new `platform/router.rs` is 537 lines. `cargo clippy --all --benches --tests --examples --all-features` is clean. Unit test run: 5068 passed; the same 2 pre-existing failures carried over from stage 1 (`pairing::approval::tests::propagate_approval_restores_runtime_state_when_on_start_fails` needs a telegram WASM fixture; `extensions::manager::tests::test_telegram_token_colon_preserved_in_validation_url` has a test-infra URL override — neither references any symbol this PR touches). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(gateway): address PR #2643 review — proper error handling in router CORS + doc reconciliation Three fixes for review comments on #2643: 1. **router.rs CORS origin parsing (comment #3104968733, CI `no-panics`):** Replaced `.expect("valid origin")` with `.map_err(|e| ChannelError::StartupFailed { .. })` so a malformed bound address fails the bootstrap with a semantically specific error instead of panicking. Also switched from `format!("http://{}:{}", addr.ip(), addr.port())` to `format!("http://{addr}")` — `SocketAddr`'s `Display` brackets IPv6 addresses correctly (`[::1]:8080` rather than the ambiguous `::1:8080`), so the CORS origin stays valid on v6 binds. Fixes the `No panics in production code` CI check failure and the `Code Style` composite check that depends on it. 2. **platform/mod.rs docstring (comment #3104970223):** The previous "feature handlers depend on platform, not the other way around" wording conflicted with `router` importing feature handlers. Rewrote to name `router` as the single, intentional exception to the no-back-edges rule, and explicitly list the platform submodules the rule still applies to (`state`, `static_files`, future `auth`/`sse`/`ws`). 3. **CLAUDE.md layering section (comment #3104970232):** Same contradiction — rewrote to reconcile: router is the coupling point; every other platform submodule must stay handler-agnostic; the forthcoming CI check (ironclaw#2599 stage 5) enforces forbidden imports between `platform/{state,static_files,auth,sse,ws}.rs` and `handlers/*` / `features/*` while explicitly allowing `platform/router.rs` to reference both sides. Verified: `python3 scripts/check_no_panics.py` clean; `cargo clippy --all --benches --tests --examples --all-features` clean; `cargo test --lib channels::web::server::tests::test_` passes 30 server-module tests including CORS / CSP / start_server smoke coverage. 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
|
e25568437f |
fix(repl): unlock stdin after auth prompt (#2640)
* fix(repl): unlock stdin after auth prompt * test(repl): drive auth-required unlock through input polling loop Address Claude bot review on PR #2640: per `.claude/rules/testing.md` ("Test Through the Caller, Not Just the Helper"), a test that only asserts the `stdin_locked` flag flipped is insufficient regression coverage — the side effect that matters is the input thread's polling loop in `start()` actually resuming. The new test mirrors that polling pattern in a worker thread and asserts it observes the unlock after `send_status(AuthRequired)`, so a future regression that skipped the store would fail the test rather than pass it silently. https://claude.ai/code/session_01VgFn7fywjFgKbFQtwkrrNd --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
firat.sertgoz
|
1b99d0c325 |
test(e2e): fix Slack fixture boot path (#2638)
* test(e2e): fix Slack fixture boot path Fixes #2623 * test(e2e): tighten slack fixture teardown - Wrap tmpdirs and process lifecycle in an outer try/finally so reserved sockets always close, including when TemporaryDirectory construction fails before yield. - Drop redundant `reset_fake_slack` calls at the start of tests now that the `active_slack` fixture already resets between tests. Keeps the intentional mid-test reset in the malformed-payload resilience case. Review follow-ups on #2638. No behavior change for passing tests. --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> |
||
|
Illia Polosukhin
|
5058a1cf0c |
fix(ci): three staging regressions — skill chain-load, duplicate Jobs tab, onboarding E2E (#2637)
Scheduled batched CI on staging was red across three unrelated paths. All three are fixed in-place; the existing tests become the regression coverage. 1. `tests/support/test_rig.rs`: rebuild the skill registry against the test's `with_skills_dir()` tempdir and actually run `discover_all()`. `AppBuilder::init_database()` reloads `config` from DB/TOML/env at the top of `build_all()`, which clobbered `config.skills.local_dir` back to the default (`~/.ironclaw/skills/`). Any registry `build_all()` constructed therefore pointed at the user's real skills dir, not the tempdir the test had laid down — so `loaded_skill_names()` came back empty and the v1 chain-load assertion panicked. Write the tempdir paths back onto `components.config.skills.*` so `AgentDeps::skills_config` agrees with the registry. `skill_chain_load_lifecycle::v1_chain_load_pulls_in_required_companions` now passes. 2. `crates/ironclaw_gateway/static/index.html`: drop the duplicate right-side `status-logs-btn` Jobs button added in #2353. The main tab-bar already has `<button data-tab="jobs">Jobs</button>`, and the duplicate had no `data-v1-only`/`data-v2-only` marker, so both rendered simultaneously. That broke `test_connection.py` (Playwright strict-mode rejected `.tab-bar button[data-tab="jobs"]` resolving to two elements) and also left both buttons visually `active` when the Jobs tab was open. 3. `tests/e2e/scenarios/test_extensions.py`: align `test_onboarding_failed_sse_shows_error_toast_and_reloads_extensions` with every other auth-card test in the file — resolve the real thread id via `_active_thread_id(page)` before calling `_show_auth_card`. `showAuthCard` short-circuits on `isCurrentThread(data.thread_id)`, and the synthetic `"thread-fail"` id fails that check once `currentThreadId` is populated after `go_to_extensions(page)`. The auth card was never rendered, so the follow-up `wait_for` for `.auth-card` hit its 5s timeout. Verified: `cargo test --features libsql --test skill_chain_load_lifecycle` and `--test skill_setup_marker_lifecycle` pass; `cargo clippy --tests --features libsql` is clean. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Henry Park
|
82a0b7598a |
Fix gateway tool output visibility and timing (#2555)
* Fix gateway tool output visibility * Address PR review follow-ups * fix(web): truncate live tool activity previews * fix(engine): preserve failed tool durations in v2 gateway events * fix(engine): default missing ActionFailed durations * style: format scripting executor * fix(web): keep history tool results aligned with preview * fix(web): restore persisted tool result parsing * fix(web): align in-memory turn result/preview with DB path Live in-memory turns have only the full tool result, not a separately persisted short preview. Populate `ToolCallInfo.result` from the live value and leave `result_preview` empty so both paths surface the same field semantics to the UI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: re-trigger CI GitHub Actions dropped the Code Style workflow on the prior push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): collapse nested match arm in live_harness Rust 1.95's stricter clippy::collapsible_match warning trips on the inner `if` inside the ToolResult arm. Fold the preview check into the arm's guard to match the same predicate-in-guard style as the arm above. Fixes the Clippy (all-features) CI failure inherited from staging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
9fee70906e |
feat(common): CredentialName + ExtensionName newtypes (PR 1/2) (#2611)
* feat(common): add CredentialName and ExtensionName newtypes Introduce typed identifiers for the backend-secret vs user-facing extension identity split that the Extension/Auth Invariants section of CLAUDE.md describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity- confusion bugs with the same shape: a stringly-typed value passed through multiple layers with each layer meaning a different thing. Newtypes make each of those a compile error. This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam (ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth, LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the remaining extension_name: String fields. Wire format is unchanged — both newtypes use #[serde(transparent)] so on- wire and on-disk representations stay plain strings and legacy persisted rows keep deserializing. Validation runs at explicit construction (::new / ::try_from / ::from_str), not at deserialize time. Also adds .claude/rules/types.md codifying the "no stringly-typed internals" rule. Regression coverage: 17 new unit tests in identity.rs; existing auth_manager, router, and gate tests (130+ cases) all pass unchanged. * fix(common): address PR #2611 review feedback Four fixes from Copilot, Gemini, and Claude reviews: - **identity.rs docs**: drop reference to a non-existent `validate()` re-validation API. Document that instances represent "passed validation at some point in history" rather than "guaranteed valid right now" — by design. - **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token` gate path was using `CredentialName::from_trusted` to wrap a value read straight out of a tool's JSON output. Tool output is external/untrusted; use `CredentialName::new` (validating) with a cascade: external → tool name → `from_trusted(tool_name)` as final fallback. Closes a credential-name shape-injection vector. - **canonicalize()**: reorder checks cheapest-first against the trimmed slice so invalid inputs reject without allocating a canonicalized `String`. `replace('-', "_")` is deferred until after the structural checks pass; since `-`/`_` are both one byte, the earlier length check stays valid. - **Remove `Deref<Target = str>`** from identity newtypes, keep `AsRef<str>`. Auto-deref let `&cred_name` silently coerce to `&str`, which is exactly the implicit-conversion pattern these newtypes exist to prevent. Callers that had a `&CredentialName` where `&str` was expected now write `.as_str()` explicitly. Added a regression test for the accessor contract and updated the rule template in `.claude/rules/types.md` to document the decision. Declined one review item (Claude): the remaining `to_string()` calls inside `IdentityError` variants are on the exception path; the common invalid-input case no longer allocates twice after the canonicalize reorder, and errors must carry owned strings so they can escape the function. Regression coverage: 5035 lib tests + 18 identity tests (one new — `explicit_accessors_work`) pass. Zero clippy warnings. |
||
|
Illia Polosukhin
|
72d31b7210 |
refactor(gateway): extract platform layer (state + static_files) — ironclaw#2599 stage 1 (#2628)
First increment of the ironclaw#2599 gateway platform/feature split. Moves
the shared gateway state and the unauthenticated static-serving surface out
of the 8.5k-line `server.rs` monolith into a new `platform/` subtree:
- `platform/state.rs` — `GatewayState`, `RateLimiter`, `PerUserRateLimiter`,
`WorkspacePool` (+ `WorkspaceResolver` impl), `FrontendHtmlCache`,
`FrontendCacheKey`, `ActiveConfigSnapshot`, `PromptQueue`,
`RoutineEngineSlot`, `rate_limit_key_from_headers`.
- `platform/static_files.rs` — CSP directive set + `BASE_CSP_HEADER`
(single source of truth for both the global header layer and the
per-response nonce variant), `build_frontend_html` + cache-key plumbing,
unauthenticated static handlers (`/`, `/style.css`, `/app.js`,
`/theme.css`, `/favicon.ico`, `/i18n/*`, `/admin*`, `/api/health`), and
the authenticated `/projects/{id}/...` file-serving routes with the
ownership check.
`server.rs` keeps `start_server()`, the route table, and the feature
handlers that have not yet moved (OAuth callbacks, chat, extensions,
pairing, logs, gateway status). It re-exports the relocated state types
under their old paths so the 30+ external call sites that reach for
`crate::channels::web::server::GatewayState` continue to resolve without
churn — follow-up PRs update them incrementally.
`CLAUDE.md` file map is updated to document `platform/` vs the
transitional `handlers/` folder and explains the layering rule:
features depend on platform, never the reverse.
No behavior change — route table, CSP policy, cache semantics, and
multi-tenant guard rails are byte-identical. `cargo clippy --all
--benches --tests --examples --all-features` is clean; the relocated
tests (`test_base_csp_header_matches_build_csp_none`,
`test_stamp_nonce_into_html_*`, `workspace_pool_resolve_seeds_new_user_workspace`,
`test_build_frontend_html_returns_none_in_multi_tenant_mode`, etc.) pass
against the re-exported surface.
Scope is intentionally narrow to avoid colliding with the open
XL PRs that touch `server.rs` (#2548 workspace entities, #2555 tool
output, #2532 engine v2 sidebar) — this PR does not modify anything
inside the feature-handler blocks those PRs edit.
Stats: server.rs 8534 → 7463 lines (−1071); new files 1210 lines.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
firat.sertgoz
|
c74f9555da |
ci: speed up CI feedback loop (#2566)
* ci: speed up feedback loop — concurrency, dynamic matrix, path skip, faster staging - Add cancel-in-progress concurrency groups to 6 workflows (test, code_style, e2e, regression-test-check, pr-label-classify, pr-label-scope) so pushes to the same branch cancel stale CI runs instead of queuing behind them. - Collapse test/clippy matrix on PRs from 3 configs to 1 (all-features). Full 3-config matrix still runs on staging promotion and push-to-main. Cuts PR compilation from ~3x to ~1x. - Reduce staging-ci poll interval from 60 minutes to 10 minutes, cutting worst-case promotion latency by 6x. - Add path-based skip to test.yml and code_style.yml: a lightweight changes-detection job checks if any code files changed (src/, crates/, Cargo.*, etc.). Docs-only PRs skip all Rust compilation while the rollup job still passes for branch protection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: collapse nested ifs in trace_contains_tool_call match arms Clippy 1.95 added/tightened `clippy::collapsible_match`. The two nested `if`s in this helper are equivalent to additional match-arm guards, which is what the lint suggests. No behavior change. Inherited from #2268's merge into staging; would have failed `Clippy (all-features)` on every PR until fixed. * test: rustfmt struct destructure in collapsed match arm * ci: drop --benches from clippy invocations `--benches` pulls in `criterion` (heavy dep) but only covers 2 bench files in `crates/ironclaw_safety/`. Lints rarely differ in bench code, and `bench-compile` in test.yml already provides the type-check signal. Cold-cache impact: ~30s+ saved per Linux/Windows leg (criterion + plotters + ciborium chain). Warm-cache: marginal but non-zero. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: ilblackdragon@gmail.com <ilblackdragon@gmail.com> |
||
|
Illia Polosukhin
|
7f5b02d7f0 |
feat(docs): animated architecture overview video for contributors (#2365)
* feat(docs): animated architecture overview video for contributors Adds a Remotion-based animated video (82s, 12 scenes at 30fps) that visualizes the IronClaw architecture for new contributors. Covers engine v2 primitives, CodeAct execution, thread state machine, skills pipeline, tool dispatch, channel routing, trait implementations, and LLM decorator chain. - docs/architecture-video/ — Remotion project with 12 animated scenes - scripts/render-architecture-video.sh — render script - .claude/skills/architecture-video/ — Claude Code skill to update the video when architecture changes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(docs): address PR review feedback and fix cargo-deny CI - Resolve relative output paths in render script before cd - Use npm ci when package-lock.json exists for reproducible builds - Fix file paths in TraitsScene, ChannelImplsScene, CodeActScene - Label Channel trait code as simplified in ChannelsRoutingScene - Fix TypeScript version (5.9.3 → 5.7.3) and update lockfile - Add dom/esnext to tsconfig lib for React 19 compatibility - Fix license to MIT OR Apache-2.0 to match repo - Fix TOTAL_DURATION to count only scenes with transitions - Fix cargo-deny: add publish = false, ignore RUSTSEC-2026-0097 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(docs): address remaining PR review feedback - Remove `publish = false` from Cargo.toml (unrelated build policy change, should be a separate PR if desired) - Add `--` before output path in render script to prevent argument injection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(docs): address second round of PR review feedback - Add npm command check to render script (was only checking node/npx) - Memoize highlight() tokenization in CodeBlock — Remotion re-renders every frame and code is static per instance, so useMemo avoids repeated work - Rewrite README to describe the IronClaw architecture video project instead of the default Remotion template 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
|
96aa31bb42 | fix(telegram): update channel test for String owner_id + bump registry (#2620) | ||
|
Henry Park
|
12fb3b1437 |
Fix gateway thread retention and stale in-progress state (#2517)
* fix(gateway): persist in-progress chat state * Fix gateway thread retention and stale in-progress state * Use stable message IDs for gateway in-progress state * Fix gateway live state review follow-ups * Fix follow-up PR review comments * Fix clippy warning in skills catalog * Fix in-progress review follow-ups * Fix all-features clippy in TUI renderer * Fix legacy in-progress reconciliation * Fix remaining clippy warnings * Fix gateway review follow-ups --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
firat.sertgoz
|
ab276eb94d |
fix(gateway): time-gate SSE reconnect history reload (#2404) (#2415)
* fix(gateway): time-gate SSE reconnect history reload to prevent tab-switch flicker (#2404) Every SSE reconnection unconditionally called loadHistory(), which clears the entire chat DOM and re-renders all messages — losing scroll position and causing visible flicker on every browser tab switch. Now tracks when the SSE connection was lost and only reloads history if disconnected for more than 10 seconds. Brief reconnects (tab visibility change, transient network blip) preserve the existing DOM and rely on the "Done without response" safety net for missed events. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review findings (iteration 1) Set _sseDisconnectedAt before server restart in E2E test to prevent flaky timeout when the restart completes in <10s. 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
|
e3df3ec4ae |
feat(skills): setup-marker lifecycle, chain-loading, and live GitHub workflow test (#2268)
* chore: gitignore live test fixture containing recorded credentials
The github_dev_workflow live test records HTTP exchanges including
the github_token Bearer header. GitHub push protection correctly
blocks this. The fixture is only useful locally for replay; the
test skips gracefully without it.
* test: add live test for github developer workflow
Adds tests/e2e_github_dev_workflow.rs — a multi-turn live/replay test that
drives the developer-assistant + github-workflow skills end-to-end against
a synthetic nearai/ironclaw repository:
1. Setup — installs the wf-* mission set (excluding
wf-staging-review per the implement-but-don't-
auto-merge autonomy contract)
2. Issue opened — synthetic github.issue.opened webhook payload
3. Maintainer LGTM — pr.comment.created from a maintainer
4. PR review — non-maintainer review comment
5. CI failure — failing check_run
6. Approval — maintainer approval; asserts NO merge call ever
fires across the whole session
7. Digest — status report referencing the issue/PR
Webhook payloads are injected via TestRig::send_message with a
[GITHUB WEBHOOK] frame that matches what a real webhook→channel
adapter would emit. The mission OnSystemEvent firing path is covered
separately by mission.rs unit tests; this test exercises skill
behavior given the right inputs.
Adds two helpers to tests/support/live_harness.rs:
- trace_contains_tool_call(name, needle)
- assert_trace_contains_tool_call(name, needle, ctx)
Both scan ToolStarted.detail and ToolResult.preview for case-insensitive
substring matches, so behavior tests can assert *what the agent
actually called* without scraping the recorded trace JSON.
Drive-by cleanups from the extension-lifecycle merge:
- thread_ops.rs: drop orphaned RecordingStatusChannel + helper that
came from a dropped extension-lifecycle test variant
- bridge/router.rs: clippy needless_borrow on PendingGate args
- skills/mod.rs: SkillManifest no longer has metadata field; add
requires: GatingRequirements::default() to test fixture
- cargo fmt fallout in recording.rs / live_mission.rs / trace_llm.rs
The test is #[ignore]-tagged (live tier) and skips gracefully in replay
mode until tests/fixtures/llm_traces/live/github_dev_workflow_full_loop.json
is recorded with IRONCLAW_LIVE_TEST=1. Compile coverage is automatic
via the existing test matrix; live execution follows the same pattern
as e2e_live_personas.rs (manual recording + commit fixture).
cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --test e2e_github_dev_workflow -- -D warnings: clean
cargo test --features libsql --test e2e_github_dev_workflow -- --ignored: passes (skips, fixture missing)
* test(harness): add pre-seed secrets + diagnostic activity dump
Three additions to make the github_dev_workflow live test runnable:
1. **TestRigBuilder::with_secret(name, value)** — pre-seed credentials
in the SecretsStore before the agent starts. The kernel pre-flight
auth gate fires when a skill with a credential spec activates (e.g.
the github skill needs github_token); without a stored credential
the agent gets stuck in 'Authentication required' mode and can't
make progress. Tests inject a fake/dummy value so the gate is
satisfied — the test isn't actually hitting the credentialed API.
Implementation: AppComponents.secrets_store is captured during
build_all() and any pre-seeded (name, value) pairs are written via
secrets_store.create() with user_id = config.owner_id. Already-exists
errors are silenced so the helper is idempotent on seeded DBs.
2. **LiveTestHarnessBuilder::with_secret** — forwards to
TestRigBuilder::with_secret. Plumbed through both build_live and
build_replay so the same fixture works in both modes.
3. **dump_activity helper in e2e_github_dev_workflow.rs** — formats
captured StatusUpdate stream (skill activations + every tool
started/completed/result) to stderr. Used as a pre-assertion
diagnostic so failing live runs surface the agent's actual tool
sequence instead of an opaque panic on a workspace check.
Test relaxations from running this against the real LLM:
- verify_setup_landed accepts either developer-assistant OR
github-workflow as the active skill (the deterministic selector
picks based on keyword scoring + token budget; both routes are
valid since github-workflow owns the mission templates)
- final required-skills check drops developer-assistant in favor of
github-workflow + github (the orchestrator persona is optional)
- setup turn now pre-seeds github_token via with_secret
cargo check --features libsql --tests: clean
* test: rewrite github_dev_workflow as fully real live integration
Pivots the test from synthetic webhook simulation to a real end-to-end
integration test against the real nearai/ironclaw repo. Per project
owner: 'fully real live tests doing useful work on github repo... test
everything like it's live while recording all interactions to debug
what doesn't work and improve that'.
## Why the rewrite
The previous synthetic-event version injected fake GitHub payloads as
channel messages. With a real github_token in scope, the agent
attempted to fetch the fake issue 99001, got a 404, and helpfully
created 3 real issues + 3 real comments on nearai/ironclaw to
"reconcile" the discrepancy. The synthetic approach didn't surface
realistic failure modes anyway (auth gates, payload format mismatches,
rate limits), so we go all-in on real artifacts.
## New flow (2 turns + real artifact lifecycle)
1. Setup turn — agent installs the wf-* mission set for nearai/ironclaw
2. Test (NOT the agent) creates a real issue via direct REST API with
the title "[live-test {timestamp}] Add /metrics Prometheus endpoint"
and a real feature-request body.
3. Triage turn — test asks agent to triage issue #N. Agent reads via
github skill, generates a plan, posts a real comment back.
4. Verification — test polls api.github.com/issues/N/comments and
asserts at least one new comment exists since baseline. Comment
bodies are logged to stderr for human review (the most useful
debug output for iterating on skill quality).
5. Cleanup — std::panic::catch_unwind wraps the body so cleanup runs
regardless of pass/fail. Closes the issue with a final "live test
complete" comment. If cleanup itself fails, the issue URL is
printed for manual recovery.
## Test infrastructure additions
- TestRig.get_secret(name) — read decrypted secrets back from the
rig's SecretsStore. Required so the test can read the github_token
the harness pre-seeded via with_secrets(["github_token"]).
- TestRig captures secrets_store + owner_id from AppComponents during
build (needed for get_secret).
- github_api submodule inside the test file — direct REST helpers for
create_issue, list_issue_comments, post_issue_comment, close_issue.
Uses reqwest directly so the test has guaranteed GitHub access
regardless of skill selection / tool gating.
## Recording
- LLM trace fixture: tests/fixtures/llm_traces/live/github_dev_workflow_full_loop.json (65K)
- Session log: github_dev_workflow_full_loop.log (5.9K)
- Both committed so future runs can replay deterministically without
hitting real GitHub.
## What's NOT covered yet
Dropped from the previous version (can be added back as follow-ups):
- PR creation flow (agent opens a real PR with a real branch + real
code change)
- CI failure simulation (would need a real failing CI run)
- Mission OnSystemEvent firing via real webhooks (needs an HTTP
server registered as a GitHub webhook)
- Maintainer approval flow
This first version validates the most valuable slice: setup → react
to real issue → produce real comment → cleanup. If the agent's
comment quality is good, we expand from here.
cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --test e2e_github_dev_workflow -- -D warnings: clean
Live recording: passed in 85.9s
- Created issue #2185
- Agent posted 2 comments (full plan + follow-up)
- Closed issue #2185
* feat(skills): one-time setup-marker exclusion + rename persona skills to *-setup
The persona orchestrator skills (developer-assistant, ceo-assistant,
trader-assistant, content-creator-assistant) are pure first-time
onboarding flows — their entire body is Steps 1-N of workspace setup,
mission registration, and calibration memory writes. After those steps
run successfully, there is nothing left for the skill to do, but the
deterministic selector kept evaluating them on every conversation
turn, burning ~3000 tokens of activation budget for work already
completed and risking partial re-runs of setup steps.
This commit makes setup skills opt-in to one-time activation:
## Mechanism: setup_marker exclusion
New optional field on ActivationCriteria:
activation:
setup_marker: commitments/.developer-setup-complete
Before scoring, the selector caller (Agent::select_active_skills)
collects every distinct setup_marker referenced by loaded skills,
checks the workspace for each via Workspace::exists(), and passes
the set of satisfied markers into prefilter_skills. Any skill whose
marker is in the satisfied set is excluded from scoring entirely
(returns None from the filter map, skipping the score_skill call).
The selector check is opt-in: skills without a setup_marker are
unaffected. Reactive operational skills (commitment-triage,
decision-capture, github, github-workflow, etc.) keep activating
on every matching message as before.
Tests:
- 4 unit tests in crates/ironclaw_skills/src/selector.rs covering
marker present/absent, marker mismatch, and skill-without-marker
unaffected paths
- All 152 ironclaw_skills tests pass
- Live e2e_github_dev_workflow run on real nearai/ironclaw passes
(issue #2186 created, comment posted, closed) in 88s
## Rename: *-assistant → *-setup
Per project owner: 'rename persona skills to -setup skills to make
it explicit they are called once'. The -assistant suffix obscured
the lifecycle — these are not always-on assistants, they are
one-time onboarding wizards.
Renamed directories (via git mv) and updated SKILL.md `name:`
fields:
- skills/ceo-assistant → skills/ceo-setup
- skills/content-creator-assistant → skills/content-creator-setup
- skills/developer-assistant → skills/developer-setup
- skills/trader-assistant → skills/trader-setup
All four now declare `setup_marker: commitments/.<name>-setup-complete`
and have a new final 'Step N: Mark setup complete' instructing the
agent to write the marker via memory_write after confirming setup
with the user. Different personas have different markers so they
remain independently triggerable in separate workspaces.
Cross-references updated:
- tests/e2e_live_personas.rs (4 persona test invocations)
- tests/e2e_github_dev_workflow.rs (doc comments)
- tests/e2e/LIVE_TOOL_FAILURES.md (1 reference)
- crates/ironclaw_skills/src/types.rs (doc comment example)
## Bump: SKILLS_MAX_CONTEXT_TOKENS default 4000 → 6000
The previous default was so tight that a setup skill (3000 tokens)
plus its companion github-workflow (2000) plus github (2000) would
overflow at 7000. Reactive operational skills like
commitment-triage, decision-capture, tech-debt-tracker often got
budget-evicted. With setup skills now excluded after onboarding,
the freed budget plus the bump to 6000 lets the most useful
combinations fit comfortably (e.g. github-workflow + github +
product-prioritization is now active in the live recording, where
previously product-prioritization would have been evicted).
## Plumbing changes
- ActivationCriteria gains pub setup_marker: Option<String>
(#[serde(default)], so existing skills are unaffected)
- prefilter_skills signature gains
&satisfied_setup_markers: &HashSet<String> (caller passes empty
set to disable filtering — used by all existing tests via the
prefilter_no_markers wrapper)
- Agent::select_active_skills is now async — it needs to
Workspace::exists() each marker. dispatcher.rs caller updated
to .await. Snapshots the skill list under the read lock then
drops the guard before any await to avoid holding a poisonable
RwLock across an await point.
cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
cargo test -p ironclaw_skills: 152 passed
Live e2e_github_dev_workflow run: passes (88s)
* feat(skills): chain-load companions + v2 marker exclusion + commitment-setup marker
Three orthogonal follow-ups to the skill lifecycle work.
## 1. Chain-loading via requires.skills (v1 Rust + v2 Python)
When a parent skill is selected by the scorer, its requires.skills
companions are now automatically loaded, bypassing the score filter.
Persona/bundle skills like developer-setup can finally work as
designed: the orchestrator declares which operational skills it
delegates to, and selecting the orchestrator pulls them all in.
- **v1 Rust** (crates/ironclaw_skills/src/selector.rs): extracted
skill_token_cost() and try_select() helpers used by both the
scored-selection loop and the new chain-loading pass. Companions
consume the same budget and respect max_candidates. Non-transitive
(depth 1 only) to keep behavior predictable.
- **v2 Python** (crates/ironclaw_engine/orchestrator/default.py):
select_skills() gains an inline chain-loading pass that mirrors
the Rust logic. Uses a name-indexed lookup built from the skill
list passed in by handle_list_skills. No closure-over-outer-var
tricks that Monty would reject — the inner try-add is inlined.
7 chain-load unit tests in selector.rs covering: pulls in
companions, skipped when parent not selected, respects budget,
skips companion with satisfied marker, non-transitive (depth 2
not pulled), missing companion silent, dedup across parents.
## 2. v2 setup_marker exclusion
The v2 engine's Python orchestrator handles skill selection via
handle_list_skills (Rust) -> select_skills (Python). Since
handle_list_skills already has the full project doc list in scope,
we filter there: any skill whose metadata.activation.setup_marker
is in the set of existing doc titles gets excluded before the
Python orchestrator ever sees it. Zero extra store calls — we
reuse the existing list_memory_docs_with_shared result to build
an O(1) title set.
This is the v2 parity of the v1 satisfied_setup_markers parameter
threaded through prefilter_skills. Both paths now implement the
same rule: a one-time setup skill whose marker file has been
written has finished its job and should not keep burning
activation budget.
## 3. commitment-setup gets a setup_marker
commitment-setup writes commitments/README.md as its first step,
so the marker is automatically set after a successful first run.
Added:
activation:
setup_marker: commitments/README.md
To re-trigger (e.g. migrate to a new schema), delete README.md
first. project-setup was NOT given a marker — it's per-repo,
invoked repeatedly, not a singleton (each call creates a new
projects/<owner>-<repo>/project.md).
## 4. Lifecycle integration test
tests/skill_setup_marker_lifecycle.rs drives a real agent turn
through the v1 selector pipeline (Agent::select_active_skills ->
Workspace::exists -> prefilter_skills) to verify that a setup
skill:
Phase 1: activates on the first matching message (marker absent)
Phase 2: marker file is written via workspace.write()
Phase 3: is excluded on the second matching message
The test asserts on the captured LLM system prompt content (via
rig.captured_llm_requests) rather than on StatusUpdate events so
it's agnostic to v1/v2 path differences in how skill activations
are announced. The skill's body contains a distinctive marker
string (LIFECYCLE-TEST-SKILL-BODY-MARKER-Z7Q) — if the skill was
selected, that string appears in the system prompt; if excluded,
it doesn't.
Cover matrix after this commit:
- v1 selector: 35 unit tests + 4 setup-marker tests + 7 chain-load tests
- v2 handle_list_skills marker exclusion: 1 integration test (lifecycle)
plus structural verification via cargo check (the filter uses the
existing list_memory_docs API, no new store calls to test)
- v2 Python select_skills chain-load: covered by the v1 unit tests
through shared semantic contract (both paths mirror the same
algorithm); a direct Python-level test would require spinning up
the Monty interpreter which is out of scope for this session.
Verification:
cargo test -p ironclaw_skills --lib: 159 passed
cargo test -p ironclaw_engine: 304 passed
cargo test --features libsql --test skill_setup_marker_lifecycle: 1 passed
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
* feat(skills): carry requires through v1→v2 migration + chain-load test
V2SkillMetadata was missing the `requires` field entirely, so the
v1→v2 skill migration silently dropped `requires.skills` and the
chain-loading code I added to the v2 Python orchestrator in the
previous commit was effectively dead code — it always read an empty
companion list.
This was caught while writing an end-to-end chain-load test: the v1
test (through the Rust selector) passes, the v2 test (through the
Python orchestrator) was failing in a way that only made sense if
the companion metadata never reached Python. Inspection confirmed
`V2SkillMetadata` had no `requires` field, only `activation`.
## Fix
1. `V2SkillMetadata` gains `pub requires: GatingRequirements` with
`#[serde(default)]` for backwards compatibility (legacy
MemoryDocs in existing databases deserialize with an empty
`requires`).
2. `src/bridge/skill_migration.rs::v1_skill_to_memory_doc` now
copies `skill.manifest.requires.clone()` into the new field.
3. Four other explicit `V2SkillMetadata { ... }` literal
constructions updated with `requires: Default::default()`:
- `crates/ironclaw_engine/src/memory/skill_tracker.rs` (test helper)
- `crates/ironclaw_engine/src/runtime/mission.rs` (test helper)
- `crates/ironclaw_skills/src/v2.rs` (serde roundtrip test)
- `tests/engine_v2_skill_codeact.rs` (test fixture)
## New test: tests/skill_chain_load_lifecycle.rs
End-to-end lifecycle test for chain-loading. Writes three skills to
a tempdir:
- `parent-setup-test` — scored by a distinctive keyword, declares
two companions via `requires.skills`
- `companion-one-test` / `companion-two-test` — zero-scoring on
their own (keywords deliberately don't match)
Each skill body carries a distinctive marker string
(`CHAIN-LOAD-PARENT-BODY-J4V`, `CHAIN-LOAD-COMPANION-ONE-K5W`,
`CHAIN-LOAD-COMPANION-TWO-L6X`) that the test greps for in the
captured LLM system prompt via `rig.captured_llm_requests()`. If a
marker is present, the skill was injected into the prompt; if
absent, it wasn't.
Two test variants:
- **v1** (default rig, Rust selector path): **PASSES**. Proves the
chain-loading pass in `prefilter_skills` correctly pulls in both
companions despite their zero individual scores.
- **v2** (with_engine_v2, Python orchestrator path):
**`#[ignore]`d** with a detailed explanation. The v2 engine runs
a Python orchestrator that makes multiple LLM calls per user
message, but the default TestRig uses a single-turn TraceLlm that
exhausts after the first call — observing skill injection through
the v2 path needs a multi-turn TraceLlm harness or a dedicated v2
skill test rig. The structural wiring for v2 chain-loading
(V2SkillMetadata.requires + skill_migration copy + Python
select_skills chain-load pass) compiles and passes the 304-test
engine suite, so this is a test-harness gap, not a code gap.
When the multi-turn harness exists, flipping `#[ignore]` on the v2
test will exercise the full path.
Verification:
cargo test -p ironclaw_skills --lib: 159 passed
cargo test -p ironclaw_engine --lib: 304 passed
cargo test --features libsql --test skill_chain_load_lifecycle
-- --test-threads=1: 1 passed, 1 ignored
cargo test --features libsql --test skill_setup_marker_lifecycle
-- --test-threads=1: 1 passed
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
Also includes an updated fixture recording from the last live
`e2e_github_dev_workflow` run (issue #2204, agent posted 2 comments,
cleanup closed it). No functional difference; committed for
completeness since the fixture was modified on disk by the live run
and the test is hermetic in replay mode.
* fix: adapt thread_ops test to staging's test helper API
Use make_test_agent_with_status_channel instead of removed
make_thread_ops_test_agent, StdMutex instead of TokioMutex,
and fix String comparison direction.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* style: cargo fmt
* fix: remove dead try_add function and stale comments in Python orchestrator
Addresses PR #2268 review feedback: the try_add closure was defined but
never called since the logic was inlined for Monty compatibility.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: reconcile test harness after staging merge
Restore our branch's test helpers (SessionTurn, finish_turns_strict,
with_skills_dir, loaded_skill_names, active_skill_names, etc.) that
staging removed, while incorporating staging's new features
(record_trace, with_no_trace_recording, secrets_store/owner_id
accessors). Bridge the API gap with finish_turns_simple for tests
using staging's (String, Vec<String>) tuple convention.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address PR #2268 review feedback
- live_harness: replace panic with graceful TestMode::Skipped when
record_trace=false in replay mode; update e2e_live callers to check
mode() != Live instead of == Replay
- test_rig: match SecretError::NotFound explicitly in get_secret(),
return None silently instead of logging expected misses
- test_rig: replace brittle "already exists" string matching in
pre-seed loop with get_decrypted existence check before create
- default.py: align max_context_tokens fallback from 1000 to 2000
to match Rust ActivationCriteria default (both parent and companion)
- e2e_builtin_tool_coverage: fix routine_create_list using hardcoded
"test-user" instead of rig.owner_id() (broke when .with_skills()
changed channel user to config owner_id)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address PR #2268 review feedback (round 2)
1. Fix memory_write `path:` → `target:` in all 4 setup skill completion
markers (developer, ceo, content-creator, trader). The `memory_write`
tool reads `target`, not `path`, so markers were never written to the
correct location.
2. Add setup_marker validation in enforce_limits(): max 256 chars, reject
`..` path traversal. Prevents untrusted skills from abusing markers.
3. Fix v2 Python skill budget: default 4000 → 6000 to match v1 Rust
config. Also port the approx_tokens > declared * 2 sanity check from
Rust to prevent budget bypass via low max_context_tokens declarations.
4. Reorder developer-setup companion skills to put github/github-workflow
first (critical for setup) and fix misleading budget comment in config.
5. Move AssertUnwindSafe cleanup guard in e2e GitHub test to wrap
everything after create_issue, preventing orphaned issues on panic.
6. Scope workspace in select_active_skills to the requesting user_id so
multi-user channels check the correct user's setup marker state.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: remove duplicate skills_dir field from LiveTestHarnessBuilder
Both sides of the merge added the same field, resulting in a duplicate
declaration that failed compilation in test targets.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address CI failures and Copilot review feedback
1. Fix formatting (cargo fmt).
2. Filter existing_titles to non-Skill docs in v2 orchestrator so setup
markers don't collide with skill doc titles of the same name.
3. Fix stale doc comment in types.rs (commitments/README.md →
commitments/.developer-setup-complete).
4. Fix misleading comment on v2 requires field — the full
GatingRequirements struct is preserved, not just the companion list.
5. Match SecretError::NotFound explicitly in test_rig pre-seed loop
instead of catching all errors — other errors (DB, crypto) now
surface instead of triggering a blind create.
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
|
b93098d48f |
ci: save rust-cache only on main/staging pushes (#2609)
Swatinem/rust-cache was saving the multi-GB target/ directory at the end of every CI job — including PR jobs that will never be read from again by a different PR. The post-cache step was adding 5+ minutes of wall clock to each Clippy and Tests matrix leg, on top of the actual compile time. This change: - Adds save-if gating to every rust-cache step in code_style.yml and test.yml so the cache is only written when github.ref is refs/heads/main or refs/heads/staging. PR jobs still restore, they just skip the upload. - Adds push triggers on main and staging to code_style.yml so those pushes actually exercise each matrix leg and refresh the cache entry that PRs will restore from next. Without this, the cache would never be refreshed after the one-time manual warm-up. - Gates no-panics on pull_request events since it diffs against github.event.pull_request.base.sha, which is empty on push events. Expected impact on PR wall clock: ~5 minutes saved per Clippy / Tests matrix leg, no change to cache hit rate for subsequent PRs. |
||
|
Illia Polosukhin
|
af47b27a13 |
ci: share rust-cache across clippy matrix legs (#2610)
* ci: share rust-cache across clippy matrix legs The three clippy matrix entries (all-features, default, libsql-only) each had a distinct cache key, so each leg maintained its own multi-GB target/ in the GitHub Actions cache. GHA caps repo cache at 10 GB and evicts LRU, so these three near-duplicates crowd out other useful entries and forced each leg to re-warm after eviction. Switch to shared-key: clippy for the Linux matrix and shared-key: clippy-windows for the Windows matrix. Whichever leg finishes (and saves) first wins the slot; the other two restore from that cache on the next run. Because --all-features builds a superset of the artifacts needed by default / libsql-only, a shared cache is incrementally useful for every leg even when the saver wasn't all-features. This is independent of and complementary to #2609 (save-if gating); together they reduce total cache churn per clippy job to near-zero wall clock on PR runs after the first main/staging push refreshes the slot. * ci(clippy): run only all-features on push Adds if: github.event_name == 'pull_request' || matrix.name == 'all-features' to the Linux clippy matrix so push events (main/staging) run only the all-features leg. This pins the saver for the new shared-key slot: --all-features builds a superset of the artifacts needed by default and libsql-only, so when those PR legs cache-restore they always start from the richest possible baseline rather than whichever leg happened to win a three-way race. PRs still run all three legs, so lint coverage is unchanged on the path that matters (before merge). Push events are only exercised after a PR has already passed, so the redundant two legs were just warming a cache anyway. |
||
|
firat.sertgoz
|
158f5f7d7f |
fix(mcp): validate server names with strict allowlist (fixes #1882) (#2400)
* fix(mcp): validate server names with strict allowlist (fixes #1882) MCP server names are interpolated into secret keys, tool name prefixes, and provider tags. Without validation, shell metacharacters (;|&`$), path separators (/\), dots, and other special characters in server names could enable injection attacks. This adds an allowlist-based check in McpServerConfig::validate() that only permits [a-zA-Z0-9_-]. Dots are excluded because LLM providers require tool names to match ^[a-zA-Z0-9_-]+$ and server names are used as tool name prefixes. To avoid breaking users with legacy names (e.g. "My Server"), load_mcp_servers_from() and load_mcp_servers_from_db() now skip invalid entries with a tracing::warn instead of failing the entire config load. Supersedes #1941 and incorporates its review feedback (removing dots from the allowlist, graceful degradation on invalid names). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(mcp): preserve schema_version when filtering invalid servers McpServersFile::default() sets schema_version to 0 (u32::default), but configs loaded from JSON get schema_version 1 via serde default. The server-filtering logic in load_mcp_servers_from() and load_mcp_servers_from_db() was using McpServersFile::default(), silently downgrading schema_version from 1 to 0 on every load-filter-save cycle (e.g. via bootstrap_nearai_mcp_server). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(mcp): use Vec::retain for server filtering Replace manual for-loop with `retain` as suggested in review — simpler and avoids constructing a new McpServersFile struct. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve CI failures — clippy too_many_arguments + restore merge-lost install-param extraction - Add #[allow(clippy::too_many_arguments)] to register_startup_channels - Restore tool_install/tool_activate/tool_auth parameter extraction block in pending_gate_extension_name that was lost during staging merge 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: Zaki <zaki@iqlusion.io> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> |
||
|
Illia Polosukhin
|
6a28a4c861 |
fix(test): case-insensitive tool_search description assertion (#2608)
* fix(test): case-insensitive assertion in tool_search description The e2e assertion at tests/e2e_builtin_tool_coverage.rs:1230 checked for a lowercase "use the `message` tool ..." substring, but #2515 capitalized the first word in src/tools/builtin/extension_tools.rs:110. The local unit test in that file was updated; this e2e test was missed, breaking the Run Tests job on main and blocking release-plz PR #2606. Normalize to lowercase before substring match so a future copy-edit doesn't silently break CI again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update tests/e2e_builtin_tool_coverage.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> |
||
|
firat.sertgoz
|
06527e4f22 |
fix(channels): unify hot-activation owner_id type and capabilities fallback (#2471)
* Fix WASM channel owner_id fallback * ci: ignore rand advisory * ci: satisfy cargo-deny path dependency versions * fix(telegram): handle null/string owner_id and propagate to WASM config The bundled Telegram capabilities.json ships `"owner_id": null`. The previous code only called `Value::as_i64()`, which returns `None` for `Null`, so the fallback silently produced no owner — the fix never actually worked for Telegram. Changes: - Handle `Null`, `String`, and `Number` variants in `owner_actor_id_for_channel()` so the real production payload works. - Propagate the *resolved* owner_id into the WASM runtime config map regardless of whether it came from runtime config or capabilities fallback (previously only the runtime-config path injected it). - Add `tracing::debug!` for non-scalar owner_id values to aid debugging. - Add tests: null config, missing capabilities file, empty string, non-scalar value, and caller-level register_channel tests that verify config injection and null-owner-id handling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(channels): unify owner_id type and add capabilities fallback to hot-activation Address two follow-up items from PR #2349 review: 1. Type consistency: boot path injected owner_id as Value::String, but hot-activation path (build_wasm_channel_runtime_config_updates) used Value::Number. Changed the function to accept Option<&str> and inject as Value::String, matching the boot path. 2. Capabilities fallback: hot-activation paths (complete_loaded_wasm_channel_activation and refresh_active_channel) only checked runtime HashMap and settings store. Now they also consult capabilities.json via the extracted owner_id_from_capabilities() helper, matching the boot path's behavior. Also updates the telegram WASM module to accept both string and number JSON for owner_id via a custom deserializer, since all other channels already use Option<String>. 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: Illia Polosukhin <ilblackdragon@gmail.com> |
||
|
firat.sertgoz
|
0ab2f134ff |
Prompt for local profile during quick onboarding (#2389)
* feat(setup): prompt for local profile on first run * refactor: encapsulate DB config backup/restore into Settings helpers Extract backup_database_config() and restore_database_config() on Settings to replace inline field-by-field save/restore in the setup wizard. Cleaner interface, consistent with project encapsulation standards. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(setup): address henrypark133 + zmanian review — reorder flow, remove backup/restore (#2389) - Remove `DatabaseConfigBackup` struct and backup/restore methods; reorder quick-mode flow so profile selection runs before `auto_setup_database()`, letting the existing clone→try_load→merge_from pattern preserve wizard-chosen DB settings naturally (henrypark133). - Add comment explaining the cfg-gated `loaded` variable shadowing in `try_load_existing_settings` (zmanian #1). - Change catch-all `_ =>` to explicit `1 => ... _ => unreachable!()` in profile match arm (zmanian #3). - Add caller-level test verifying profile application preserves DB config through the merge_from cycle (henrypark133 testing feedback). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Alex Rozgo
|
dcca2187fa | fix(telegram): emit chat_type metadata for group-safe prompt behavior (#2513) | ||
|
firat.sertgoz
|
645c2eb14e |
feat(gateway): show commit hash in version for non-tagged builds (#2486)
* feat(gateway): show git commit hash in version display for non-tagged builds
When the binary is not built from an exact git tag, the user info popover
now shows the short commit hash next to the version (e.g. "IronClaw v0.25.0
(
|
||
|
firat.sertgoz
|
275c3c2198 |
fix(e2e): resolve 12 E2E test failures across routines and features groups (#2503)
* fix(e2e): resolve 12 E2E test failures across routines and features groups
Three root causes fixed:
1. Read-only thread regression (9 routines failures):
|
||
|
Illia Polosukhin
|
afb0dcf136 |
refactor(events): add OnboardingStateDto::pairing_required constructor (#2607)
Three call sites (bridge::router, channels::web::server, extensions::manager) hand-constructed AppEvent::OnboardingState with state=PairingRequired and identical 8-field payloads. Any new field on OnboardingState required touching all three sites and they could silently disagree. Collapse emission through a single associated function in ironclaw_common so auth_url/setup_url/state are invariant and the three sites only supply the fields that genuinely vary (request_id, thread_id, message, instructions, onboarding). |
||
|
Nige
|
a619c4720d |
fix(ownership): fail closed on WASM default-scope fallback (#2465)
* fix(wasm): fail closed on default-scope fallback * fix(wasm): implement settings-store has_settings mock |
||
|
Nige
|
ce98cf2cd2 |
fix(status): report active WASM channels accurately (#2420)
* fix(cli): report active wasm channels in status * Update src/cli/status.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> |
||
|
Nige
|
786d526d4a | fix(llm): normalize NEAR AI tool schemas (#2463) | ||
|
firat.sertgoz
|
db33ea3b92 |
fix(security): redact credentials in HTTP tool + recording interceptor (#2529)
* fix(security): redact credentials in HTTP exchange recorder and tool Two leak surfaces fixed in tandem so a recorded fixture can never again ship a live token: - `RecordingHttpInterceptor::after_response` now scrubs every recorded exchange in place: a curated set of credential-bearing headers (Authorization, Cookie, x-api-key, etc., case-insensitive) and query parameters (access_token, api_key, password, etc.) get replaced with `[REDACTED]`. Non-sensitive fields are untouched so replay matching still works. New unit test pins the behaviour against headers, query params, and Set-Cookie. - `HttpTool` snapshots caller-supplied headers BEFORE credential injection and feeds the snapshot to the interceptor — injected Authorization/API-key values never reach the recorder even if the recorder's redaction list missed something. Also dedupes credential mappings so the same secret declared by both a WASM tool's capabilities and a skill's `credentials` block doesn't append two Authorization headers (which GitHub rejects with 401). Adds a redacted preview log (first/last 4 chars only) to aid triage. * fix(security): address zmanian review — body/userinfo redaction, dedupe fix (#2529) - Redact sensitive JSON body keys (password, secret, private_key, etc.) before persisting HTTP exchanges in recorded traces - Redact URL userinfo (user:pass@host) to prevent credential leakage - Switch dedupe key from JSON serialization to structured HashSet with CredentialLocation implementing Eq+Hash (avoids non-canonical key issue) - Fix Unicode safety in secret preview (chars().count() vs byte len) - Expand SENSITIVE_QUERY_PARAMS with auth, jwt, session - Fix clippy ptr_arg warning on redact_headers - Add regression tests for body scrubbing, userinfo redaction, and URL preservation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): address review feedback — dedupe key, unicode safety, body+userinfo redaction (#2529) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): address ilblackdragon review — exact body-key match, replay URL redaction, caller tests (#2529) - Body-key redaction now uses exact match (not substring) to avoid over-redacting non-sensitive fields like token_count, input_tokens, session_id, auth_method. - Replay URL comparison redacts the incoming URL before matching against stored (already-redacted) URLs, preventing false mismatch warnings. - Added caller-level test (SpyInterceptor) proving HttpTool passes caller_headers (pre-injection snapshot) to the interceptor, so injected Authorization headers never reach the recorder. - Added regression tests for credential-mapping dedup: duplicates removed, different locations preserved. - Added trace warning on unparseable URL in redact_url and code comment linking redact_json_value to ironclaw_safety leak-detector. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): address ilblackdragon review round 2 — dedup caller test, form-urlencoded redaction, userinfo cleanup (#2529) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): snapshot URL pre-injection and redact response bodies Close the remaining credential-leak paths into recorded traces on top of the round-2 form-urlencoded / userinfo / dedup work. - Snapshot `parsed_url` before the credential-injection loop mutates it, and hand the snapshot to the HTTP interceptor instead of the post-injection URL. `CredentialLocation::QueryParam`/`UrlPath` mappings with parameter names outside the recorder's fixed `SENSITIVE_QUERY_PARAMS` allowlist (e.g. `signature`, `auth_v2`) previously shipped raw into fixture files despite downstream redaction; the snapshot makes this structurally impossible. - Call `redact_body` in `redact_exchange_response` so OAuth token endpoint replies like `{"access_token":"..."}` don't ship into committed fixtures. Request bodies were already scrubbed; this closes the corresponding response gap and inherits the form-urlencoded path added in the round-2 commit. - Correct the `LeakDetector` references in `recording.rs` comments. `LeakDetector::scan` runs on response bodies upstream in `HttpTool::execute`, but only as a hard-block filter — values below its block threshold still flow through to the recorder, so this allowlist is the last line before the fixture file. - Add caller-level regression tests: one driving `HttpTool::execute` with a `QueryParam` mapping under a non-allowlisted name to prove the URL snapshot works, and one asserting response bodies containing `access_token`/`refresh_token` are redacted before persistence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> |
||
|
Illia Polosukhin
|
5fa60f66b1 |
feat: discover tool source in working directory during install (#2396)
* feat: discover tool source code in working directory during install When `tool_install` can't find a tool in the registry, it now searches common directories relative to the current working directory before returning "not found": - tools-src/<name>/ - tool-src/<name>/ - <name>/ (direct subdirectory) Matches both hyphenated and underscored name variants, and strips/adds `_tool`/`-tool` suffixes. A directory is only considered a match if it contains a Cargo.toml. This lets users say "install portfolio tool" when tool source is at tool-src/portfolio/ without needing the explicit path. * style: apply cargo fmt formatting * fix(extensions): address PR #2396 review feedback - Restrict local tool source discovery to WASM kinds only; skip non-WASM kind hints (McpServer, ChannelRelay, AcpAgent) that can't be built from a local Cargo source. - Refactor candidate name generation to use HashSet, avoiding weird combos like `my_portfolio-tool` and `*_tool-tool` from the old suffix logic. - Update NotFound error message to mention all 3 search patterns (tools-src/, tool-src/, direct subdir). - Include source path in InstallResult.message so the user/LLM can verify provenance when a tool is installed from a local directory instead of the verified registry (confused-deputy mitigation). - Change local-discovery log from info! to debug! per CLAUDE.md REPL/TUI logging rule. - Extract install_from_local_source() helper and add caller-level tests per testing.md ("Test Through the Caller, Not Just the Helper") to cover kind defaulting, target_dir routing, and message annotation. * fix(extensions): resolve wasm artifact via Cargo.toml crate name Address follow-up review feedback on PR #2396: 1. Suffix-stripping name mismatch (HIGH): when `find_local_tool_source` matched a directory via suffix add/strip (e.g. input `portfolio_tool` -> dir `portfolio/`), `install_from_local_source` passed `None` for `crate_name`, so artifact lookup searched for `<name>.wasm` instead of the real `<crate>.wasm` and every suffix-matched install failed. Parse `Cargo.toml` from the discovered source and pass `[package].name` as `crate_name`. 2. Non-deterministic candidate ordering (MEDIUM): the `HashSet` of name variants gave non-deterministic iteration, so directory matches within one search dir could vary across runs. Replace with a priority-ordered `Vec` + `retain` dedup: canonical underscore form first, hyphen next, suffix-adjusted variants last. Adds a caller-level regression test for the name-mismatch bug and a determinism test covering the underscore-vs-hyphen ordering. * style: apply cargo fmt * fix(extensions): tighten local tool source discovery (PR #2396 review) - find_local_tool_source_in: require Cargo.toml to be a regular file (is_file) rather than merely existing, so a directory named Cargo.toml cannot falsely qualify a candidate source directory. - install_from_local_source: reject non-UTF-8 source paths with a clear InstallFailed error instead of silently lossy-converting them into a build-dir path that will not resolve. Addresses Copilot review comments on nearai/ironclaw#2396. * fix(extensions): drop dead -tool strip branch in local source discovery `underscore_name` is built via `name.replace('-', "_")`, so the `underscore_name.strip_suffix("-tool")` fallback can never match — it was unreachable code. The single `_tool` strip already covers both `name_tool` and `name-tool` inputs because hyphens are normalized first. Added `find_local_tool_source_strips_hyphen_tool_suffix` to lock in that the hyphenated suffix input still resolves to the unsuffixed dir. Addresses Copilot review comment on nearai/ironclaw#2396. |