mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
33580d4d69
* fix(workspace): collapse reindex delete+insert into one transaction to close TOCTOU race Two concurrent reindexers for the same document could both DELETE the existing chunks, then both try to INSERT chunk_index 0, hitting the UNIQUE (document_id, chunk_index) constraint and failing with "database is locked" or constraint violation. The delete and inserts were separate libsql transactions with async points between them. Add `WorkspaceStore::replace_chunks(document_id, &[ChunkWrite])` that runs DELETE + N INSERTs inside a single BEGIN IMMEDIATE transaction (not the default DEFERRED — DEFERRED bypasses busy_timeout on the first write contention). The libsql impl, postgres impl, and the in-memory storage variant all go through the new method, and `Workspace::reindex_document` builds the `ChunkWrite` Vec (with embeddings) up front so nothing async happens between the delete and the insert loop. Regression test in `workspace::versioning_tests` spawns 4 concurrent writers against the same document on a multi-thread runtime and asserts last-writer-wins without UNIQUE collisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(auth): resolve display name + extension target from action when surfacing auth gates The engine's `ResumeKind::Authentication` only carries `credential_name` (e.g. `google_oauth_token`), which was being used as both the user-facing display string AND as the first argument to `submit_auth_token`. Two failure modes: 1. Display: users saw "google_oauth_token" in the auth-required prompt instead of the friendly extension name "google-drive-tool". 2. Routing: `submit_auth_token` expects an *extension* name and walks the extension's capabilities file to find the actual secret. Passing `google_oauth_token` directly fails closed with "Extension not installed: google_oauth_token", trapping the user in a re-auth loop on every paste. `bridge/router.rs` now resolves the actual extension via `tools.provider_extension_for_tool(action_name)` for both the gate display path and the `submit_auth_token` call. Built-in tools, HTTP, and skill credentials still fall back to the credential name (the existing behaviour for those callers). `extensions/manager.rs` fixes three more auth-readiness traps surfaced by the v2 Drive trace: - All capabilities lookups (`auth_wasm_tool`, channel activate, setup schema, configure, explicit secret query, upgrader) now go through `load_tool_capabilities` / `load_channel_capabilities` so a tool installed under the legacy hyphen filename (`google-drive-tool.capabilities.json`) is still resolved when looked up by canonical underscore name. The pre-v0.23 layout silently reported `no_auth_required` and bypassed the gate entirely. - `activate_wasm_tool` now uses `existing_extension_file_path` for both the `.wasm` and `.capabilities.json` lookups so the legacy hyphen filename is resolved here as well. Without this, the upstream `determine_installed_kind` happily reported the extension as installed via its own alias check, but `activate_wasm_tool` then failed with `NotInstalled` — the readiness probe fell back to "treat as ready" and the agent ended up calling a tool that couldn't activate, hit a 401/403, and looped trying to recover. - `configure()` post-activation OAuth cleanup now skips deletion when the caller is *also* providing a fresh credential in the same `secrets` map. The previous behaviour wrote the user's pasted token then immediately deleted it (along with `_scopes` / `_refresh_token` siblings), causing the resume to hit the wrapper with `token_exists=false` and re-fire the gate forever. Explicit Reconfigure (empty secrets map) still wipes the records to kick off a fresh OAuth dance. `config/mod.rs` test config now seeds a deterministic 32-byte master key so replay-mode tests that touch credentials get a working secrets store out of the box without each test having to build its own. Three regression tests in `extensions::manager::tests`: - `test_activate_wasm_tool_finds_legacy_hyphen_alias` - `test_auth_wasm_tool_finds_legacy_hyphen_alias` - `test_configure_preserves_oauth_token_when_caller_provides_it` - `test_configure_clears_oauth_token_for_reconfigure_flow` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(mcp): canonicalize MCP tool identifiers to snake_case at registration MCP tool names commonly contain dashes (e.g. Notion's `notion-search`), and so do user-supplied server names (`my-server`). The runtime converges on snake_case identifiers per `ToolRegistry::resolve_name`, and LLMs (Codex / GPT-5 in particular) silently normalize tool names to valid Python identifiers by converting dashes to underscores. The old code built the registry key as `format!("{server}_{tool}")` and preserved any dashes from the original tool name, so the registry got `notion_notion-search` while the LLM emitted `notion_notion_search` — direct lookup missed and the legacy alias fallback (which only goes underscores → dashes) couldn't reconstruct the mixed-separator form either, leaving every Notion MCP tool unreachable. Add `mcp_tool_id(server, tool)` in `tools/mcp/client.rs` that does `format!(...).replace('-', "_")`, re-export it from `tools/mcp/mod.rs`, and use it in: - `McpClient::create_tools` — the prefixed_name on every wrapped MCP tool now agrees with what the LLM will emit - `ExtensionManager::activate_mcp` — `tool_names` is now sourced from `tool_impls.iter().map(|t| t.name())` instead of being independently rebuilt from the raw McpTool list, eliminating drift between registered names and reported names - `ExtensionManager::latent_actions_for_mcp_server` — latent provider actions surfaced before activation use the same canonical form The original (possibly hyphenated) `t.name` is still preserved on the wrapper's inner `McpTool` and used verbatim when forwarding the `tools/call` request to the MCP server, so MCP protocol compatibility is unchanged — the canonicalization is internal-only. 5 regression tests in `tools::mcp::client::tests`: - `test_mcp_tool_id_canonicalizes_dashed_tool_name` - `test_mcp_tool_id_canonicalizes_dashed_server_name` - `test_mcp_tool_id_passthrough_for_already_canonical_names` - `test_create_tools_canonicalizes_dashed_mcp_tool_names` (drives `create_tools` end-to-end via MockTransport) - `test_create_tools_round_trips_through_registry_resolve_name` (caller-level test per `.claude/rules/testing.md` — registers the wrapped tools in a real `ToolRegistry` and asserts that `resolve_name("notion_notion_search")` returns the registered tool via the direct HashMap path, not via the legacy alias fallback) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(llm): flatten top-level schema unions for OpenAI + symmetric Python/Rust action_calls round-trip Two related bugs in the LLM ↔ engine boundary, both surfaced by the GitHub Copilot MCP and the v2 orchestrator: 1. **Top-level schema flatten.** OpenAI's tool API rejects schemas whose top level isn't `type: "object"` or that contain top-level `oneOf`/`anyOf`/`allOf`/`enum`/`not`, with HTTP 400: Invalid schema for function '<name>': schema must have type 'object' and not have 'oneOf'/'anyOf'/'allOf'/'enum'/'not' at the top level. The GitHub Copilot MCP's `github` tool uses top-level `oneOf` for action dispatch, so the agent was 400-ing the moment it tried to enumerate tools. `normalize_schema_strict` (already shared between the OpenAI Codex provider and `RigAdapter::convert_tools`) now short-circuits when it sees a forbidden top-level construct, replacing `parameters` with a permissive object envelope (`{type: "object", properties: {}, additionalProperties: true, required: []}`) and appending the original schema to the tool description as advisory text (truncated on a char boundary at 1500 bytes). The MCP server still validates the actual shape on its end, so the tool keeps working — we just lose API-level schema enforcement and the LLM has to read variant structure from the description. The function signature changes to take `&mut String` for the description (to append the hint). Both call sites (`openai_codex_provider::convert_tool_definition` and `rig_adapter::convert_tools`) pass an owned clone through. This is slightly lossy for Anthropic users on tools with top-level unions (Claude could have handled the union natively), but keeping a single normalizer for all rig-based providers is simpler than threading per-provider flags through the adapter, and the description hint preserves the variant info Claude needs. 2. **Python ↔ Rust `action_calls` field-name mismatch.** The Python orchestrator (`default.py`) appends assistant messages with `action_calls=calls` where each call is shaped `{name, call_id, params}` (the friendly Python names produced by `orchestrator.rs:handle_llm_complete`). The reverse parser `json_to_thread_messages` tried to deserialize via `serde_json::from_value::<Vec<ActionCall>>`, which expects the canonical Rust field names `{action_name, id, parameters}`. The deserialize fails, but `.ok()` swallows the error and the assistant message comes back with `action_calls = None`. Every subsequent tool result then looks orphaned to `sanitize_tool_messages` and gets rewritten as a user message, losing the model's ability to reason about prior tool calls. Introduce a private `PythonActionCall` interchange struct as the single source of truth for the field naming convention, with bidirectional `From` conversions. Both call sites (Rust → Python serialization + Python → Rust deserialization) now go through `action_calls_to_python_json` / `python_json_to_action_calls`, so any future field addition only needs to touch one struct definition. `ActionCall` itself is unchanged — adding `#[serde(rename = ...)]` would have invalidated every persisted Step record and ThreadEvent. 11 regression tests: - `rig_adapter::tests::test_normalize_schema_strict_*` (6 tests) covering pass-through, top-level oneOf flatten, anyOf/allOf/enum/not flatten, non-object replacement, nested-oneOf preservation, and char-boundary truncation - `rig_adapter::tests::test_convert_tools_handles_top_level_oneof_dispatcher` (caller-level test driving `convert_tools` end to end) - `openai_codex_provider::tests::test_convert_tool_definition_handles_top_level_oneof_dispatcher` (caller-level test driving the codex provider path) - `executor::orchestrator::tests::python_action_call_round_trips_through_serde` - `executor::orchestrator::tests::action_calls_to_python_json_uses_python_field_names` - `executor::orchestrator::tests::python_json_to_action_calls_parses_python_field_names` - `executor::orchestrator::tests::python_json_to_action_calls_rejects_canonical_field_names` (guards against silent shape drift) - `executor::orchestrator::tests::json_to_thread_messages_preserves_action_calls_from_python_orchestrator` (caller-level test feeding the literal JSON shape `default.py` produces, asserting the assistant message's `action_calls` survive the round-trip with matching call_ids) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(live): add Drive auth-gate round-trip live test + supporting harness pieces End-to-end smoke test for the post-flight auth gate path that the recent auth-postflight commits stitched together. Two phases: Phase A: delete the developer's real `google_oauth_token` (and the refresh token) from the test rig's libsql DB while keeping the `_scopes` companion record so `auth_wasm_tool`'s scope expansion check doesn't fire on the re-store. Send a Drive search prompt and assert the agent emits `StatusUpdate::AuthRequired` within one iteration. The expected path: 1. agent calls `google-drive-tool { action: "list_files" }` 2. wrapper's `resolve_host_credentials` reports `missing_required = ["google_oauth_token"]` 3. wrapper fails closed with the "requires credentials that are not configured" message 4. effect adapter's post-flight branch fires `auth::postflight::detect_post_call_auth_failure` 5. matcher hits the `requires credentials + not configured` pair (commit cd8b68de) 6. detector calls `ensure_extension_ready(.., ExplicitAuth)` → `EnsureReadyOutcome::NeedsAuth` 7. `EngineError::GatePaused { resume_kind: Authentication }` bubbles to the orchestrator 8. router stores it in `pending_gates` and emits `StatusUpdate::AuthRequired` Phase B: re-insert the captured token via `secrets_store()`, send the synthetic value as a follow-up message. The v2 router treats the next user message after an auth gate as `GateResolution::CredentialProvided`, which calls `submit_auth_token` (idempotent overwrite of what we just inserted) then `execute_pending_gate_action` → `execute_resolved_pending_action`, and the original Drive call replays. The test asserts the resume ran (additional tool activity + a follow-up response). Live-tier only (`#[ignore]`); skipped outside `IRONCLAW_LIVE_TEST=1`. The test deliberately does NOT commit a recorded trace fixture: any trace would inevitably capture the bearer token, real Drive file metadata, and HTTP headers — all PII that's hard to scrub safely. Hermetic regression coverage for the underlying alias-aware capabilities bug lives in `test_auth_wasm_tool_finds_legacy_hyphen_alias`. Supporting harness changes: - `LiveTestHarnessBuilder::with_no_trace_recording()` — opt-out flag for tests that touch real credentials. Live mode still runs against the real LLM but no fixture is committed; replay mode builds a stub harness so the test can detect the mode and skip gracefully without panicking on a missing fixture. - `LiveTestHarness::finish_turns(&[(user_input, responses)])` — multi-turn variant of `finish` for tests that span an auth-gate round-trip (prompt → AuthRequired → token → resume). The session log renders all turns in order so a reader can follow the full conversation, not just the first prompt. Status events are still rendered once at the top because the rig doesn't tag them with a turn boundary. - Session log formatter now renders `StatusUpdate::AuthRequired` and `StatusUpdate::AuthCompleted` so the gate is visible in the log. - `TestRig::secrets_store()` and `TestRig::owner_id()` accessors so live tests can manipulate credentials directly under the same scope the agent loop uses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(llm): log on python_json_to_action_calls deserialize failure Address PR #2209 review: the helper used `serde_json::from_value(...).ok()?` which is the exact `.ok()` swallow pattern the parent commit set out to fix. If a future Python orchestrator patch ever drifts the action_calls shape (extra required field, rename, partial migration), the helper would silently return None and every subsequent tool result would look orphaned to `sanitize_tool_messages` again — with no operator-visible signal at all. Replace with an explicit match that emits a `warn!` (with the parse error and the offending JSON value) on the failure path so the breadcrumb is visible the moment any drift happens. The `None` return is preserved so existing callers and the `python_json_to_action_calls_rejects_canonical_field_names` test still hold. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(test): generate random master key per Config::for_testing call Address PR #2209 review: the previous fix hardcoded `0123456789abcdef...0123456789abcdef` as the AES-256-GCM master key inside `pub fn for_testing`. The function is `pub` (gated only behind `#[cfg(feature = "libsql")]`, not `#[cfg(test)]`, because integration tests in `tests/*.rs` are separate crates compiled against the lib's non-test surface), which meant every developer building with libsql had a publicly-known master key sitting in their process — and the constant was now baked into Git history forever. Replace with `generate_test_master_key()`, a private helper that pulls 32 bytes from `rand::thread_rng()` and hex-encodes them. Each call returns a fresh key. Tests don't need cross-process determinism: each test creates its own temp DB and the secrets store is born fresh on every call anyway. `rand 0.8` is already a direct workspace dependency so no Cargo changes are needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(mcp): normalize all non-identifier characters in mcp_tool_id Address PR #2209 review: `mcp_tool_id` only handled `-` → `_`, but the MCP spec doesn't actually constrain tool names to OpenAI's `^[a-zA-Z0-9_-]{1,64}$` regex — a server could legally return `notion.search`, `notion:create_issue`, `files/read`, or names with spaces or non-ASCII characters. The same LLM normalization that bites on `-` will bite on `.` and `:` too, and `extract_server_name` only strips `.` from the host portion of a URL, leaving the tool portion of the prefixed name unprotected. Replace the single `.replace('-', "_")` with a `chars().map()` pass that sends every non-`[A-Za-z0-9_]` character to `_`. This handles dashes, dots, colons, slashes, spaces, and unicode in one shot — and since the chars iterator yields one Rust char per code point, multi-byte characters become a single `_` rather than splitting weirdly. New regression test `test_mcp_tool_id_normalizes_non_identifier_chars` covers dot, colon, slash, space, and multi-byte unicode inputs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(llm): make flatten_top_level hint keyword-aware Address PR #2209 review: the description hint appended by `flatten_top_level` was a one-size-fits-all "pick one variant and pass its fields as a flat object". That's correct for top-level `oneOf` and `anyOf`, but actively misleading for the other forbidden constructs: - `allOf` — the LLM should pass fields from ALL variants combined, not pick one - `enum` — the LLM should pass one of the listed literal values, not "fields" - `not` — the LLM should pass any object that does NOT match the constraint Extract `FORBIDDEN_TOP_LEVEL` to a module-level constant (now shared between `needs_top_level_flatten` and a new `detect_forbidden_top_level` helper) and add `schema_flatten_hint_intro(detected)` which branches on the actual keyword that triggered the flatten and returns a precise intro string. Falls back to a "free-form object" message when the schema wasn't an object at all (no recognized forbidden keyword, just the wrong top-level type). New regression test `test_normalize_schema_strict_hint_is_keyword_aware` asserts that each of the 5 keywords produces a hint containing the expected discriminating phrase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(workspace): serialize postgres replace_chunks via FOR UPDATE on parent doc Address PR #2209 review (Copilot, src/workspace/repository.rs:350): the libsql `replace_chunks` is fine because `BEGIN IMMEDIATE` acquires the writer lock at transaction start, but the postgres path used the default-isolation `BEGIN` which is not equivalent. Two concurrent reindexers running under separate snapshots can both DELETE (each sees its own pre-delete state, neither sees the other's), then race to INSERT chunk_index 0 and hit the `UNIQUE (document_id, chunk_index)` constraint. Add `SELECT 1 FROM memory_documents WHERE id = $1 FOR UPDATE` at the top of the transaction. The `FOR UPDATE` row lock is per-document, ties to the existing parent row (FK already in place from `memory_chunks.document_id`), and is released automatically on commit/rollback. Concurrent reindexers for the same doc now serialize on the parent row and last-writer-wins cleanly. Picked `FOR UPDATE` over `pg_advisory_xact_lock` because it's the row-locking primitive PG operators expect when reading the code, and it doesn't introduce a hash function dependency for the lock key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(llm): merge top-level union variants into flatten_top_level envelope Address PR #2209 review (gemini-code-assist, src/llm/rig_adapter.rs:296): after flattening a top-level oneOf/anyOf/allOf, the LLM was left with `properties: {}` and could only read variant fields from the description hint. That works but it's lossy — the LLM can't do schema-based reasoning about which fields exist, and the description hint is truncated to 1500 bytes so deeply-nested schemas are unreadable. Add `merge_top_level_variant_properties` which walks the union variants, collects every property they declare, and returns a single map. The flatten envelope now uses that map instead of empty `{}`, so the LLM sees structured field hints. `additionalProperties: true` and `required: []` are preserved, so strict-mode validation stays disabled and the LLM is free to mix fields across variants — the upstream MCP server enforces the actual constraints on its end. First-write wins on conflicting types: if two variants declare the same field with different schemas, the first variant's schema is kept. The full original schema still goes into the description hint, so the ambiguous case is recoverable from there. Two new regression tests: - `test_normalize_schema_strict_merges_variant_properties` exercises a GitHub-Copilot-shaped tool with two variants that share a discriminator and asserts every field from every variant survives. - `test_normalize_schema_strict_merge_first_write_wins_on_conflict` pins the documented conflict-resolution behaviour. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(router): extract resolve_extension_for_action helper for 3 dup sites Address PR #2209 review (henrypark133, src/bridge/router.rs:1606): the `provider_extension_for_tool + unwrap_or_else(credential_name)` pattern was implemented in three places — once via the `resolve_auth_gate_display_name` helper at line ~64, and twice inlined in `resolve_gate` (line ~1603) and `await_thread_outcome` (line ~2779). The inline sites couldn't use the helper because they'd already destructured `credential_name` from the `ResumeKind` match and needed the result for `submit_auth_token`, not just display. Extract the core into `async fn resolve_extension_for_action(tools, action_name, credential_fallback) -> String`. Make `resolve_auth_gate_display_name` a thin wrapper that handles the non-Authentication ResumeKind variants. Both inline sites now call the helper directly with the destructured `credential_name`. The two inline-site comment blocks that explained the rationale are collapsed into shorter "see helper for full rationale" pointers since the doc on `resolve_extension_for_action` carries the full explanation now. Three sites collapse to one implementation. The auth display + routing logic now has a single source of truth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(mcp): warn on post-normalization tool name collisions in create_tools Address PR #2209 review (serrrfirat, src/tools/mcp/client.rs:682): after the broader `mcp_tool_id` char normalization (commit 18d4ce48), two MCP tools whose names differ only by `-` vs `_` (e.g. `search-all` and `search_all`) collide on the same registry key. The second `ToolRegistry::register` call silently shadows the first with no signal at all — operators debugging an unreachable tool would have zero breadcrumb to discover the collision. Add collision detection in `McpClient::create_tools` itself, where we still have both the original tool name and the normalized id. Build a `HashMap<normalized_id, original_name>` while iterating, and emit a `tracing::warn!` when two distinct originals collide on the same id. The warn carries the normalized id, both colliding original names, and the server name, so an operator can immediately see which upstream tools to rename. Behaviour is unchanged — the second tool still wins on register, matching what the LLM would emit anyway since it normalizes both names to the same string. The collision detection is scoped to a single MCP server's tool list because cross-server collisions have different registry-key prefixes (`server_a_foo` vs `server_b_foo`) and can't actually shadow each other. This is the right level — `ToolRegistry::register` itself doesn't have access to the pre-normalization name and couldn't emit this signal even if we wanted it there. New regression test `test_create_tools_handles_post_normalization_collision` drives a MockTransport that lists `search-all` and `search_all`, asserts both wrappers are produced with the same `Tool::name()`, registers them in a real `ToolRegistry`, and asserts last-write-wins on shadow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(engine): drop entries from action_calls_to_python_json on failure instead of injecting null Address PR #2209 review (serrrfirat, crates/ironclaw_engine/src/executor/orchestrator.rs:1920): the previous helper used `unwrap_or_else(|_| Value::Null)` which silently corrupts the array when serialization fails. The Python orchestrator (`default.py`) accesses `c.get("name")` / `c.get("call_id")` / `c.get("params")` on each entry, so a `null` would crash with a Python `AttributeError` and lose the entire LLM step — and the fallback contradicts this PR's own stated goal of not silently swallowing errors. Replace with `filter_map` so a failed entry is dropped from the output rather than corrupting it. The warn log on the failure path is preserved (and now also includes `action_name` for easier correlation). Python's tool-result loop iterates `range(len(results))` against the same shortened call list so a missing entry is benign. Note: the failure path is essentially unreachable for the `PythonActionCall` shape (`String + String + Value` all infallible-to-serialize) but the contract should still be safe — the helper will be touched again when the Python interchange shape evolves and we don't want a future maintainer to discover this trap the hard way. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(engine): summarize action_calls in warn log to avoid leaking PII Address PR #2209 review (serrrfirat, crates/ironclaw_engine/src/executor/orchestrator.rs:1999): the `python_json_to_action_calls` warn log emitted `value = %value` which dumps the full action_calls JSON array on parse failure. Tool params can carry user PII (search queries, file names, email content, conversation text), and the warn fires precisely when the Python ↔ Rust shape drifts — exactly the moment operators will be grepping logs and shipping output to log aggregators (Datadog, CloudWatch, Sentry). Add `summarize_action_calls_for_log` which builds a structural-only summary: array length and the keys of the first entry. The keys themselves are static field names (`name`, `call_id`, `params`), not user data. The shape summary is enough to debug a drift (operator can see whether the shape is roughly right and which fields are missing) without exposing any of the actual parameter contents. Edge cases handled: - empty array → "empty array" - non-array value (Python passed wrong shape) → "non-array value of type <kind>" via a small `json_value_type_name` helper - entries that aren't objects → "<not an object>" rather than attempting to walk them Two regression tests: - `summarize_action_calls_for_log_does_not_leak_user_pii` builds an intentionally PII-laden value with salary spreadsheet queries, credentials, and "private message about layoffs" content, asserts none of the user-content strings appear in the summary, AND that even the upstream tool name doesn't leak (operator-level intent signal). - `summarize_action_calls_for_log_handles_edge_cases` pins the empty/string/object/null fallback paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * incorporate #2227: factory server_name normalization, registry bidirectional alias, WASM/channel loader normalization, legacy token fallback Merge the non-overlapping changes from PR #2227 (fix-tool-name-hyphen-normalization) so this PR supersedes it. Our PR already fixed the core issue (mcp_tool_id canonicalization + stricter non-identifier-char normalization), but #2227 adds valuable defense-in-depth and compatibility layers that we didn't cover: - `tools/mcp/factory.rs` — normalize `server.name` at the factory boundary (before any branch including the OAuth early-return). This ensures the secret name, session key, and tool prefix all use the same underscore-only form. Our PR normalized only at the tool-id level which left the server_name itself hyphenated in the session manager and token secret store. - `tools/mcp/client.rs` — normalize `new_with_name` so callers that pass a hyphenated server name get consistent behavior even when bypassing the factory. - `tools/mcp/config.rs` + `tools/mcp/auth.rs` — legacy token secret name fallback for pre-normalization tokens. When checking if an MCP server is authenticated, if the canonical (underscore) secret name doesn't exist, try the legacy (hyphenated) form. This prevents forcing re-auth on existing users who stored tokens under the old hyphenated server name before upgrade. - `tools/registry.rs` — bidirectional `resolve_key` helper that tries exact → hyphen→underscore → underscore→hyphen aliases in `get`, `has`, `unregister`, `resolve_name`, `get_resolved`, `provider_extension_for_tool`, and `tool_definitions_for_actions`. Defense-in-depth: even if a tool somehow ends up registered with a mixed-separator name (edge case, stale DB, manual insertion), the registry will still find it. 4 new regression tests cover both alias directions, get_resolved, and unregister via alias. - `tools/wasm/loader.rs` + `channels/wasm/loader.rs` — `load_from_dir` and `discover_*` functions normalize hyphenated filenames (file stem → replace('-', "_")). Dev tool install name changed from `{name}-tool` to `{name}_tool`. Conflict resolution: manager.rs (kept our version with comment), client.rs create_tools (kept our `mcp_tool_id` which is strictly better — handles ALL non-identifier chars, not just dashes), client.rs tests (kept our comprehensive MockTransport-based test suite, dropped #2227's simpler duplicate). All other hunks from #2227 applied cleanly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(manager): normalize server name prefix in starts_with tool-list filters Address PR #2209 review (henrypark133, Critical C1): the 3 `starts_with(&format!("{}_", name))` filters in `activate_mcp` (already-active fast path), `list()`, and `remove()` used the raw (possibly hyphenated) server name, while `mcp_tool_id` normalizes the tool registry keys to underscores-only. A hyphenated server name like `my-server` produced a prefix `my-server_` that matched zero tools (they're all `my_server_*`), returning empty tool lists and failing to unregister on extension removal. Fix: use `crate::tools::mcp::mcp_tool_id(name, "")` as the prefix. This produces `my_server_` from `my-server`, matching the registered keys exactly. All 3 sites now use the same normalization as tool registration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(llm): accept array type containing "object" in needs_top_level_flatten Address PR #2209 review (henrypark133, Critical C2): `needs_top_level_flatten` only matched `JsonValue::String("object")` for the type check. A top-level `"type": ["object", "null"]` (valid JSON Schema for a nullable object, produced by some upstream providers and `make_nullable`) triggered `bad_type = true` and the schema was flattened, silently discarding all its properties. Extend the check to also accept `JsonValue::Array` when any element is the string `"object"`. This prevents unnecessary flattening of schemas that are semantically object-typed but use the array form for nullability. New regression test: `test_normalize_schema_strict_does_not_flatten_nullable_object_type` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(mcp): update seen_ids on collision so 3rd collision reports against 2nd Address PR #2209 review (henrypark133, Nit N1): the collision detection in create_tools skipped the `seen_ids.insert` on the collision branch, so a 3rd colliding tool would report against the 1st original name instead of the 2nd (the actual shadow). Added the insert inside the warning branch so subsequent collisions report the correct chain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(wasm): fix CI type mismatch + add string-matching fallback for wrapped traps Address PR #2209 CI failure and Copilot review comments: 1. **CI fix (E0308):** `classify_trap_error` took `anyhow::Error` but wasmtime 43's `call_execute` returns `wasmtime::Error` (a distinct type in `wasmtime_internal_core`). Changed the signature to accept `wasmtime::Error` directly. This is also more correct — accepting the native error type preserves type information that a lossy `.into()` conversion would strip, making the structured `Trap` downcast more reliable. 2. **String-matching fallback (Copilot, wrapper.rs:1107):** The doc claimed "falls back to string matching" but the implementation only did the structured downcast. Added a string-matching fallback that checks the full Display chain for "all fuel consumed", "out of fuel", "OutOfFuel", and "unreachable" when the downcast fails. This covers the case where component-model glue or host wrappers bury the Trap inside layers that `downcast_ref` can't see through. New regression test `trap_classification_fuel_via_string_fallback` exercises this path using a plain `wasmtime::Error::msg` wrapper. 3. **Stale doc comment (Copilot, test_rig.rs:1282):** Updated `secrets_store()` doc to reflect that most test rigs now have a working secrets store because `Config::for_testing()` generates a random master key per call. `None` only occurs with a config override that explicitly disables secrets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review: tighten unreachable trap match, cap schema serialization, document legacy auth Address PR #2209 review (serrrfirat, 4 comments): 1. `wrapper.rs:1131` — tightened the string fallback from bare `contains("unreachable")` to `contains("unreachable code")` / `"UnreachableCodeReached"` / `"wasm trap: unreachable"`. The old match would false-positive on HTTP errors like "endpoint was unreachable" or "server unreachable: connection refused", replacing the real diagnostic chain with a generic message. 2. `rig_adapter.rs:326` — added `count_json_nodes` pre-check (cheap recursive walk, no alloc) before calling `serde_json::to_string`. A malicious MCP server with a many-MB schema would have triggered a proportional allocation even though we only keep 1500 bytes. Schemas over 5000 nodes skip serialization entirely and get a "(schema too large to inline)" placeholder instead. 3. `auth.rs:1202` — documented that the legacy token fallback intentionally uses bare `get_decrypted` (no refresh). The path is transitional: users re-auth once and get migrated to the canonical naming scheme. Wiring refresh through the legacy path adds complexity for a self-healing compat layer. 4. `rig_adapter.rs:171` — Anthropic lossiness was already documented in the `normalize_schema_strict` doc comment (lines 164-170). Reply-only; per-provider flag is a follow-up. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(llm): ensure array items is a JSON Schema object for OpenAI strict mode OpenAI rejects array-typed properties whose `items` field is missing, boolean (`true`), or any non-object value with: "array schema items is not an object" Schema generators like schemars produce `{"type": "array"}` (no items) or `{"type": "array", "items": true}` for `Vec<serde_json::Value>`, which is valid JSON Schema but violates OpenAI's strict-mode rules. The google_docs_tool's `requests: Vec<serde_json::Value>` field triggered this on every tool enumeration. In `normalize_schema_recursive`, detect array-typed properties and ensure `items` is a JSON Schema object before recursing. Missing or non-object `items` are replaced with `{}` (accept any item). Object `items` are left untouched and recursed into as before. Regression test covers all three cases: missing items, boolean items (`true`), and well-formed items (must not be clobbered). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(llm): add post-normalization validation to catch schema rules the normalizer misses The schema_validator module already knew the "array items must be an object" rule (Rule 8, line 218), had a test for it (test_array_missing_items_fails, line 315), and would have caught the google_docs_tool 400 — but it was only wired into CI tests against built-in tools, never applied to WASM/MCP tool schemas or to the output of normalize_schema_strict. The root cause pattern: we're playing whack-a-mole with OpenAI's undocumented strict-mode rules, adding fixes one at a time when a new tool exposes a schema shape the normalizer doesn't handle. Each time, the fix is a runtime 400 in production that takes a PR cycle to fix. The structural fix: run validate_strict_schema as a debug-level post-check after normalization. If the normalizer missed something, the diagnostic appears in local logs immediately (before the schema even reaches the LLM provider), giving developers a local breadcrumb instead of a runtime 400 from OpenAI. The schema still goes through (the tool remains usable), and the LLM provider surfaces the 400 if OpenAI actually rejects it — but now the cause is instantly visible in `RUST_LOG=ironclaw::llm=debug` output. This also means that any future normalizer rule we add gets automatic regression coverage: if the normalizer introduces a bug that violates a rule the validator knows about, the debug log fires on every tool call in dev mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(llm): normalize merged properties on flatten path + silence null action_calls warn Two runtime issues from the latest deploy: 1. The flatten path in `normalize_schema_strict` short-circuited with `return schema`, skipping BOTH the recursive normalizer AND the post-normalization validator. The merged properties copied from union variants were raw — a `Vec<serde_json::Value>` field like google_docs_tool's `requests` kept its bare `{"type": "array"}` without `items`, and OpenAI rejected it with "array schema items is not an object" on every tool-using call. Fix: the flatten path now normalizes each merged property individually via `normalize_schema_recursive(prop_schema)` before returning the envelope. The top-level envelope stays permissive (`additionalProperties: true`, `required: []`) so the LLM can mix fields across variants, but each property's internal schema gets the full treatment (array items, nested objects, etc.). The post-normalization validator also runs on both paths now (no early return before it). Regression test: `test_normalize_schema_strict_flatten_normalizes_merged_array_items` mimics the google_docs_tool shape (tagged enum with `oneOf`, one variant containing an items-less array) and asserts the merged `requests` property has `items` as an object after normalization. 2. `python_json_to_action_calls` warn log fired on every text-only assistant message with "invalid type: null, expected a sequence" because Python's `action_calls: null` (legitimate "no tool calls" signal) was passed to the parser. Added a `.filter(|v| !v.is_null())` before the parser call in `json_to_thread_messages` so null is treated the same as "key absent" — no parse attempt, no false alarm. The warn only fires for genuinely malformed data now. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(llm): replace node-counting DoS guard with size-capped serializer Address PR #2209 Copilot review (rig_adapter.rs:347 x2, :406): 1. `count_json_nodes` doc claimed "returns early once it exceeds the caller's budget" but always fully traversed. The approach also missed the case a reviewer flagged: few-node schemas with multi-MB string values would pass the node check but still allocate proportionally during `serde_json::to_string`. Replace both the node counter and the `to_string` + truncate pattern with `serialize_json_capped(value, max_bytes)`: a `serde_json::to_writer` call through a `CappedWriter` that silently discards bytes past the budget. This bounds the actual heap allocation to `max_bytes` regardless of schema shape — many-node deep recursion AND multi-MB string values are both capped. The writer returns `Ok(data.len())` after the cap so serde_json thinks all bytes were consumed and continues (minimal remaining work since the output is being discarded). The output is guaranteed valid UTF-8 because serde_json only emits ASCII structural characters and JSON-escaped unicode. The `count_json_nodes` function and `MAX_SCHEMA_NODES` constant are removed — the capped serializer subsumes them entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(engine): serialize bootstrap context action_calls through PythonActionCall The bootstrap context builder (`build_orchestrator_inputs`) serialized `m.action_calls` directly via the canonical `ActionCall` serde format (`{action_name, id, parameters}`), but the Python orchestrator passes these back verbatim in `working_messages` on the next `__llm_complete__` call, where `python_json_to_action_calls` expects the interchange format (`{name, call_id, params}`). The mismatch surfaced as "missing field \`name\`" on every thread resume after a gate pause (approval, auth), orphaning all subsequent tool results. This is the SECOND code path (after `handle_llm_complete`) that feeds action_calls into the Python working transcript. Both must use the same shape — `action_calls_to_python_json` is the single source of truth. Triggered by: user approves `tool_upgrade` → thread resumes → bootstrap rebuilds context from `internal_messages` (which stores canonical `ActionCall`s from the DB) → Python reads `{action_name, id, parameters}` → echoes them back on next LLM call → `python_json_to_action_calls` fails → assistant message loses tool_call linkage → all tool results orphaned. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(engine): add bootstrap-path round-trip test to guard against future action_calls serialization drift The gate-resume bug (08e47209) happened because `build_orchestrator_inputs` serialized `action_calls` with canonical `ActionCall` field names instead of the `PythonActionCall` interchange format. The existing round-trip test only covered the `__llm_complete__` path (within a single Python orchestrator run), not the bootstrap path (thread resume after gate pause). Anyone adding a THIRD serialization path in the future would have no test guardrail. Two new tests: 1. `bootstrap_context_action_calls_round_trip_through_python_interchange`: Builds a `ThreadMessage` with `action_calls` in canonical format (the shape stored in the DB), serializes through the EXACT pattern `build_orchestrator_inputs` uses, parses back through `json_to_thread_messages`, and asserts the calls survive. This is the test that would have caught the gate-resume bug on the first attempt. 2. `canonical_action_call_field_names_do_not_round_trip`: Negative test that verifies canonical names (`{action_name, id, parameters}`) are REJECTED by the parser. Documents the current contract: if this test ever passes, the `PythonActionCall` interchange type can be removed because the formats unified. Serves as a tripwire for anyone who adds `#[serde(rename)]` to `ActionCall` or changes the parser to accept both formats. Together these two tests cover every known serialization boundary into the Python transcript and make the failure mode instantly visible in `cargo test` rather than as a runtime warn log after a gate pause. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(llm): skip strict-mode post-validator on flatten path to eliminate false-positive noise The post-normalization validator (added in ce96c2a5 as a safety net) was firing on every flattened schema with "additionalProperties should be false" — a false positive because the flatten envelope deliberately uses `additionalProperties: true` so the LLM can mix variant fields. With 12 flattened tools loaded, this produced 12 debug log lines PER LLM CALL, drowning real signals. The flatten path's output is intentionally non-strict — running a strict-mode validator on it is semantically wrong. Move the validator behind the non-flatten branch and add an early `return schema` for the flatten path (after normalizing individual properties). The validator still catches issues on normal strict-mode schemas (the non-flatten path), which is where it has value. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: close 5 coverage gaps across schema normalization, action_calls, and MCP naming Systematic test audit of all 27 PR commits identified gaps where production bugs had no hermetic regression test or where important code paths had only helper-level (not caller-level) coverage. New tests: 1. **test_realistic_wasm_schema_survives_normalize_flatten_pipeline** (rig_adapter.rs) — End-to-end test using the google_docs_tool's actual schema shape: tagged enum with 4 variants, one containing `requests: Vec<Value>` (bare array, no items), one with a nested object (text_style). Drives through normalize_schema_strict AND convert_tools. Asserts oneOf flattened, all variant properties merged, array items is object, nested objects get strict-mode. This single test would have caught BOTH production bugs (flatten path short-circuit + array items unreachable on flatten path). 2. **test_normalize_schema_strict_fixes_deeply_nested_array_items** (rig_adapter.rs) — 3-level nesting: object → array → object → array → object → array. Verifies the recursive normalizer walks the full depth and fixes every array items at every level. 3. **json_to_thread_messages_handles_null_action_calls_gracefully** + **handles_absent_action_calls** + **handles_empty_action_calls_array** (orchestrator.rs) — Three edge cases for the Python ↔ Rust message round-trip: null (was a false alarm), absent (baseline), and empty array (valid, produces Some(vec![])). The null case would have caught the "invalid type: null, expected a sequence" false alarm before it hit production. 4. **latent_provider_actions_normalize_hyphenated_server_names** (manager.rs) — Registers an MCP server with hyphenated name (`my-mcp-server`) and two tools (one with dashes, one without). Asserts latent action_names use all-underscore form (`my_mcp_server_search_all`) and the old hyphenated form doesn't survive. Exercises the mcp_tool_id normalization at the ExtensionManager layer. 5. **test_serialize_json_capped_boundary_conditions** + **test_serialize_json_capped_large_string_values** (rig_adapter.rs) — Size-capped serializer edge cases: under cap (full output), exactly at cap, over cap (truncated), zero cap (empty), and the multi-MB-string-in-few-nodes case the old node counter missed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review: address all 6 review items — UTF-8 safety, FOR UPDATE row check, MCP config alias, legacy token request path, stale reindex guard 1. **serialize_json_capped UTF-8 safety** (Copilot, rig_adapter.rs:399): serde_json v1 emits raw UTF-8 for non-ASCII chars (e.g. CJK in property descriptions), so byte-capped truncation can cut mid-codepoint. `String::from_utf8` now falls back to `e.valid_up_to()` to trim to the last complete codepoint instead of dropping the entire hint on a UTF-8 error. 2. **FOR UPDATE row count check** (Copilot x2, repository.rs:377): `SELECT 1 ... FOR UPDATE` returns 0 rows if the document doesn't exist, silently acquiring no lock. Now checks the row count and returns a clear `ChunkingFailed` error when it's 0. 3. **MCP config lookup alias-aware** (serrrfirat, factory.rs:40): `get_mcp_server` now tries exact name → hyphen alias (underscores→hyphens) → underscore alias (hyphens→underscores). After factory normalizes `server.name` to underscores, `provider_extension_for_tool` returns `my_server`, but the persisted config is keyed as `my-server`. Without alias lookup, `activate_mcp("my_server")` failed with `ServerNotFound`. 4. **Legacy token request-time fallback** (serrrfirat, auth.rs:1208): `get_access_token` now falls back to the legacy (pre-normalization) secret name when the canonical name returns no token. Without this, `is_authenticated` reported true (it has its own fallback) but the actual MCP request sent no Authorization header — the server appeared ready but tool execution 401'd until re-auth. 5. **Stale reindex guard** (serrrfirat, workspace/mod.rs:2181): `reindex_document_with_metadata` now captures the content hash at read time and re-checks it after computing embeddings. If another writer updated the document content during the embedding window, the reindexer skips chunk replacement — the other writer's reindex call will produce correct chunks for the new content. This closes the content-vs-chunks skew where writer B wins the document UPDATE but writer A wins the later replace_chunks transaction. 6. **Log summary PII concern** (Copilot, orchestrator.rs:2020): false positive — the keys logged are JSON Schema property names from the PythonActionCall interchange dict, not user tool parameters. Reply-only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * review(workspace): narrow reindex concurrency check error handling Address Copilot review (workspace/mod.rs:2208): the optimistic concurrency check caught all `Err(_)` as "document deleted" which would silently swallow real DB errors (transient connection issues), leaving chunks stale with no signal. Now only catches `DocumentNotFound` for the deleted case; other errors propagate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tools): canonicalize paths in file_history to fix macOS symlink mismatch The file_history snapshot/restore tests were failing on macOS because `/var` is a symlink to `/private/var`. `snapshot()` stored the original path (`/var/folders/.../code.rs`), but `execute()` called `validate_path()` which canonicalizes to `/private/var/folders/...`. The path comparison in `restore_latest` mismatched, returning "No file history found" even though the snapshot existed. Fix: canonicalize paths consistently at both the storage boundary (`snapshot()`) and the lookup boundary (`latest_snapshot_for()`, `snapshots_for()`, `restore_latest()`). A shared `canonical()` helper handles the non-existent-file case (write_file's "new file" path) by canonicalizing the parent directory and joining the filename — the parent always exists even when the file itself doesn't yet. This was a pre-existing staging failure from PR #2025 that affected all macOS developers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): update e2e_live_personas to match refactored test harness APIs The PR changed `live_harness.rs` and `test_rig.rs` APIs without updating `e2e_live_personas.rs`, causing Clippy compilation failures across all feature sets. Also replaces `.expect()` in `rig_adapter.rs` with `from_utf8_unchecked` (sound per `valid_up_to` invariant) to fix the no-panics CI check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tools): deduplicate path canonicalization in file_history::snapshot Replace inline canonicalization logic in `snapshot()` with a call to the existing `Self::canonical()` helper to eliminate duplication. 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: Henry Park <henrypark133@gmail.com>