fix(v2): tool naming, auth gates, schema flatten, WASM traps, workspace race

[ilblackdragon]

Summary

Bug-fix bundle discovered while exercising the v2 engine end-to-end against real Notion + Google Drive MCP/WASM tools. Each fix has regression coverage and could ship independently. Supersedes #2227 (incorporated in a8530e8b).

1. Workspace replace_chunks TOCTOU race

Concurrent reindex of the same document hit UNIQUE (document_id, chunk_index) because delete_chunks + insert_chunk were separate transactions with async points between them. New WorkspaceStore::replace_chunks wraps both in one transaction: libsql uses BEGIN IMMEDIATE, postgres serializes via SELECT ... FOR UPDATE on the parent memory_documents row. Embeddings are pre-computed before opening the transaction so nothing async runs between delete and insert.

2. Auth gate display name + OAuth deletion guard + alias-aware activation

Three traps surfaced by the v2 Drive trace:

• Auth gate displayed google_oauth_token (credential name) instead of google-drive-tool (extension name) and passed it to submit_auth_token which expects an extension name, trapping users in a re-auth loop. Both paths now resolve via a shared resolve_extension_for_action helper.
• activate_wasm_tool and several capabilities-lookup sites used direct dir.join(name.wasm) instead of alias-aware helpers. A tool installed under the legacy hyphen filename silently failed to activate while the readiness probe reported it as ready.
• configure() post-activation cleanup unconditionally wiped the OAuth token the caller just provided. Now gated on whether the caller is providing a fresh credential in the same secrets map.
• starts_with prefix filters for MCP tool listing/removal used the raw (possibly hyphenated) server name while registry keys are normalized to underscores. Now uses mcp_tool_id(name, "") as the prefix at all 3 sites.

3. MCP tool name canonicalization

MCP tools with dashes in names (e.g. Notion's notion-search) were unreachable because the registry key preserved dashes while LLMs normalize to all-underscores. mcp_tool_id now replaces every non-[A-Za-z0-9_] character with _ (handles dashes, dots, colons, slashes, unicode). Collision detection in create_tools emits a warn! when two distinct originals collide on the same normalized id.

4. OpenAI tool schema top-level flatten + Python/Rust action_calls round-trip

• Schema flatten: OpenAI rejects top-level oneOf/anyOf/allOf/enum/not in tool schemas. normalize_schema_strict now detects this, merges all variant properties into a single flat object (so the LLM keeps structured field hints), and appends the original schema to the description as keyword-aware advisory text. Also handles nullable object types ("type": ["object", "null"]) without unnecessary flattening.
• Action_calls round-trip: The Python orchestrator stored {name, call_id, params} but json_to_thread_messages tried to deserialize as {action_name, id, parameters}. Introduced PythonActionCall interchange struct as single source of truth. The warn log on parse failure summarizes structure only (no PII from tool params).

5. WASM trap classification + fuel limit

• Extracted classify_trap_error helper that uses structured wasmtime::Trap downcast (version-proof) instead of string matching. Covers OutOfFuel, StackOverflow, UnreachableCodeReached, and falls back to full error chain for unrecognized traps.
• Default fuel limit raised from 10M to 100M instructions. 10M was too low for WASM tools making HTTP requests + parsing JSON responses — a single 30KB JSON round-trip can burn 20-50M instructions in serde_json.

6. Incorporated from #2227

• Factory-level server.name normalization before any branch (including OAuth early-return)
• new_with_name normalization for callers bypassing the factory
• Bidirectional resolve_key alias resolution in ToolRegistry (exact → hyphen→underscore → underscore→hyphen)
• Legacy token secret name fallback for pre-normalization tokens (prevents forced re-auth on upgrade)
• WASM tool + channel loader filename normalization

7. Live e2e Drive auth gate test

End-to-end smoke test driving prompt → AuthRequired → token paste → resume. #[ignore]d behind live tier, no committed trace fixture (PII risk). Supporting harness: with_no_trace_recording(), multi-turn finish_turns, TestRig::secrets_store() + owner_id() accessors.

8. Test config

Config::for_testing now generates a random 32-byte AES master key (generated fresh per call) so replay-mode tests that touch credentials have a working secrets store out of the box.

Test plan

• cargo fmt --all -- --check — clean
• cargo clippy --all --benches --tests --examples --all-features — zero warnings
• cargo test --workspace --lib — 5387 passed, 0 failed, 5 ignored
• All commits rebase cleanly onto latest origin/staging
• Manual: install Notion MCP → tool calls succeed via notion_notion_search
• Manual: install Google Drive WASM with no credentials → AuthRequired { extension_name: "google-drive-tool" } surfaces, token paste resumes
• Manual: enable GitHub Copilot MCP → no HTTP 400 schema error
• Manual: engine v2 tool flow → zero Rewriting orphaned tool_result log lines

Supersedes #2227.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes multiple v2 engine regressions found during live Notion + Google Drive MCP/WASM testing, spanning workspace chunking concurrency, auth-gate UX/routing, MCP tool naming, and LLM/provider boundary robustness.

Changes:

• Atomically replaces workspace chunks to avoid concurrent reindex races; adds regression test coverage.
• Fixes auth-gate display/routing and legacy hyphen/underscore alias handling across WASM tools/channels; expands tests and live harness support.
• Normalizes MCP tool ids for LLM-emitted names, flattens OpenAI-incompatible top-level JSON schemas, and fixes Python↔Rust action_calls round-trip.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/support/test_rig.rs	Exposes secrets store + owner id for live auth-gate tests.
tests/support/live_harness.rs	Adds multi-turn logging + opt-out trace recording for credentialed live tests.
tests/e2e_live.rs	Adds ignored live Drive auth-gate/refresh end-to-end tests.
src/workspace/repository.rs	Adds replace_chunks for transactional chunk replacement.
src/workspace/mod.rs	Switches write path to precompute embeddings then replace_chunks; adds concurrency regression test.
src/workspace/document.rs	Introduces ChunkWrite payload for chunk replacement.
src/tools/mcp/mod.rs	Re-exports mcp_tool_id for cross-module consistency.
src/tools/mcp/client.rs	Uses mcp_tool_id for registration names; adds tests for dashed tool names + registry round-trip.
src/llm/rig_adapter.rs	Adds top-level schema flattening for OpenAI tool API + tests; updates tool conversion to pass mutable description.
src/llm/openai_codex_provider.rs	Shares schema normalization/flattening with rig adapter; adds regression test.
src/llm/CLAUDE.md	Documents the updated schema normalization behavior.
src/extensions/manager.rs	Uses alias-aware lookup for wasm files/capabilities; fixes MCP activation naming; gates OAuth cleanup; adds regression tests.
src/db/postgres.rs	Implements WorkspaceStore::replace_chunks via repository.
src/db/mod.rs	Extends WorkspaceStore trait with replace_chunks.
src/db/libsql/workspace.rs	Implements replace_chunks with BEGIN IMMEDIATE to honor busy_timeout under contention.
src/config/mod.rs	Seeds deterministic secrets master key for test config.
src/bridge/router.rs	Fixes auth gate display/routing to use provider extension name where applicable.
crates/ironclaw_engine/src/executor/orchestrator.rs	Adds PythonActionCall interchange to preserve action/tool linkage across Python boundary; adds tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gemini-code-assist]

Code Review

This pull request introduces significant improvements across several modules, primarily focusing on enhancing the robustness and user experience of tool interactions and data management. Key changes include standardizing ActionCall interchange between Rust and Python orchestrators, refining authentication gate handling to display user-friendly names and correctly route tokens for WASM tools, and implementing atomic chunk replacement in the workspace store to prevent race conditions during concurrent reindexing. Additionally, the PR addresses issues with extension file lookup by supporting legacy hyphenated names and refines OAuth token management during configuration. The LLM tool schema normalization logic has been updated to handle OpenAI's strict requirements by flattening top-level union constructs and providing advisory hints in the description, and MCP tool names are now canonicalized to prevent registry key mismatches. New end-to-end tests for OAuth flows have been added, and the test harness now supports multi-turn conversations and optional trace recording. Feedback suggests improving the normalize_schema_strict function to retain more structured information for the LLM by merging properties from union variants, and addressing potential key collisions in mcp_tool_id by using structured keys instead of simple string concatenation.

[henrypark133]

Code Review Summary

Severity	Count
Critical	0
Warning	0
Nit	1

This is a very high-quality PR. All five fixes are well-motivated, clearly documented, and have thorough regression test coverage. The commit messages and PR description are exemplary — each bug is traced from root cause through fix with specific behavior-before-and-after analysis.

Highlights:

• fix(workspace): The BEGIN IMMEDIATE choice for libSQL is correct — DEFERRED transactions bypass busy_timeout on write contention, which is a non-obvious SQLite gotcha. Pre-computing embeddings before entering the transaction to eliminate async points inside it is a smart design decision.
• fix(auth): The configure() OAuth token deletion guard (!secrets.contains_key(&auth_cfg.secret_name)) cleanly separates the "submit token" path from the "reconfigure" path.
• fix(mcp): Canonicalizing at registration via mcp_tool_id while preserving the original dashed name on the inner McpTool for wire calls is the right layering.
• fix(llm): PythonActionCall interchange type captures the field-naming asymmetry in one place without touching ActionCall's serde attributes (which would invalidate persisted data). The caller-level json_to_thread_messages test is exactly the kind of regression test the codebase's testing rules call for.
• Schema flatten: Replacing the schema with a permissive envelope and moving the original to advisory description text is the right call. Truncation on char boundary is a nice detail.

Test Coverage Notes

• All five fixes have dedicated regression tests that exercise the actual bug path
• Workspace TOCTOU test uses 4 concurrent writers with multi-threaded tokio runtime
• MCP tool name test includes a caller-level round-trip through ToolRegistry::resolve_name
• Live tests properly #[ignore]d with with_no_trace_recording() to prevent PII leakage
• insert_chunk on the trait is still used by src/import/openclaw/memory.rs, so it's correctly retained alongside the new replace_chunks

[ilblackdragon]

Review fixes

Pushed 4 fixup commits addressing the Critical, High, and Medium issues from the review. Each fix is its own commit so the trail is reviewable in isolation.

#	Severity	Commit	What changed
1	🔴 Critical	`9ae48a7c`	`python_json_to_action_calls` now logs a `warn!` (with the parse error and the offending JSON value) on the failure path instead of swallowing via `.ok()?`. The whole point of the parent commit was to undo a `.ok()` swallow and the original fix re-introduced the same trap one layer deeper.
2	🟠 High	`5a6a3ac0`	`Config::for_testing` now generates a fresh random AES-256-GCM master key per call via `generate_test_master_key()` (32 random bytes from `rand::thread_rng()`, hex-encoded). The hardcoded `0123456789abcdef...` constant is gone — every developer building with `--features libsql` no longer has a publicly-known master key in their process. `#[cfg(test)]` wasn't an option because integration tests in `tests/*.rs` are separate crates compiled against the lib's non-test surface.
3	🟡 Medium	`18d4ce48`	`mcp_tool_id` now replaces every non-`[A-Za-z0-9_]` character with `_`, not just dashes. Handles dots, colons, slashes, spaces, and multi-byte unicode in one pass via `chars().map()`. New regression test `test_mcp_tool_id_normalizes_non_identifier_chars` exercises all five categories.
4	🟡 Medium	`264955e8`	`flatten_top_level` now picks a description hint that matches the actual keyword that triggered the flatten. `oneOf`/`anyOf` get "pick ONE variant", `allOf` gets "pass fields from ALL variants combined", `enum` gets "pass one of the listed values", `not` gets "any object that does NOT match". Extracted `FORBIDDEN_TOP_LEVEL` to a module constant and added `detect_forbidden_top_level` + `schema_flatten_hint_intro` helpers. New regression test `test_normalize_schema_strict_hint_is_keyword_aware`.

Issues I deliberately did NOT fix

🟠 High: Auth display name TOCTOU. Caching the resolved name on `PendingGate` would touch the persisted struct (`Serialize`/`Deserialize` via `GatePersistence`), require a backward-compat `#[serde(default)]` migration, and add a field to ~6 PendingGate construction sites. The TOCTOU window is small (it only matters if the tool is unregistered between the gate firing and the user pasting a token, in the same conversation), and even when it triggers the failure mode is benign — the second resolution falls back to `credential_name` which is the pre-fix behavior. I noted in my own review it was "lower priority; flag and forget" and I'm sticking to that judgement. Worth a follow-up issue if you want a paper trail.

🟡 Medium: Schema flatten truncation strategy. The 1500-byte byte-budget truncation is genuinely a hack — for a real GitHub Copilot `github` schema the LLM only sees the first ~15 variants. The right fix is a semantic summary ("this tool accepts one of N variants discriminated by `action`: ..., enumerate the discriminator values"), not just bumping the budget. That's significant work and out of scope for this PR — would prefer it as a follow-up issue with proper design rather than a band-aid here.

🟢 Low: `replace_chunks` comment wording and embedding-dimension validation symmetry between Postgres and libsql. Cosmetic / pre-existing risk, no functional impact, leaving alone.

Verification

Same gates as the original PR, all green:

• `cargo fmt --all -- --check` — clean
• `cargo clippy --all --benches --tests --examples --all-features` — zero warnings (one `manual_find` lint surfaced and was fixed in commit 4)
• `cargo test --workspace --lib` — 5371 passed, 0 failed, 5 ignored (was 5370; +2 new tests, -1 from a test count shift in one of the smaller binaries)

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

tests/support/test_rig.rs:866

• TestRigBuilder now always uses a temp skills_dir under the rig tempdir, and the previous skills_dir_override path selection was removed. That makes tests that intentionally exercise committed repo skills (via .with_skills_dir(...)) impossible without manually copying files into the tempdir, and currently breaks tests/e2e_live_personas.rs. Either reintroduce a with_skills_dir-style override, or update those tests to populate temp_dir/path().join("skills") with the desired SKILL.md content before build.

        // 2. Build Config.
        let has_config_override = config_override.is_some();
        let skills_dir = temp_dir.path().join("skills");
        let installed_skills_dir = temp_dir.path().join("installed_skills");
        let _ = std::fs::create_dir_all(&skills_dir);
        let _ = std::fs::create_dir_all(&installed_skills_dir);
        let mut config = if let Some(mut cfg) = config_override {
            // Override database to use temp libSQL, but preserve agent/llm settings.
            cfg.database.backend = ironclaw::config::DatabaseBackend::LibSql;
            cfg.database.libsql_path = Some(db_path);
            cfg.skills.local_dir = skills_dir;
            cfg.skills.installed_dir = installed_skills_dir;
            cfg
        } else {
            Config::for_testing(db_path, skills_dir, installed_skills_dir)
        };
        config.agent.max_tool_iterations = max_tool_iterations;
        config.safety.injection_check_enabled = injection_check;
        config.skills.enabled = enable_skills;
        if let Some(v) = auto_approve_tools {

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.