mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
34eeeaf05a
* fix(extensions): restore chat-driven tool_install + fix double-invoke + auto-approve footgun (#3533) "Connect my telegram" was giving the user two options and not actually installing anything because three layered issues had accumulated since engine v2: 1. **`tool_install` was hidden from the agent** (#2868). The unified `tool_activate` it was meant to be subsumed by was later removed in #3166, but the hidden-from-callable-surface gate stayed. Restored by dropping `hidden_from_model_callable_surface` from `bridge::action_projector`. User consent is mediated by the tool's own `ApprovalRequirement::UnlessAutoApproved` and the seeded `AskEachTime` permission. 2. **Two competing Telegram registry entries** (`telegram` channel and `telegram_mtproto` tool) both surfaced in the agent prompt's `Activatable Integrations` section. The LLM correctly enumerated them as "Option 1" and "Option 2" instead of installing the canonical bot channel. Added a `hidden: bool` field to `ExtensionManifest` / `RegistryEntry`, set `telegram_mtproto` to `hidden: true`, and filter hidden entries out of the "available-but-not-installed" appendix in `ExtensionManager::list`. Hidden entries remain installable by explicit name. 3. **Updated the agent prompt** so `Activatable Integrations` instructs the model to call `tool_install(name="<name>")` directly rather than describing manual UI steps. Fixes the double-`tool_install` invocation that surfaced once the agent could install from chat: - **`InlineGate` discarded cached output.** The bridge raised an Authentication gate after `tool_install` succeeded, and the inline-await retry re-executed the action (re-downloading the WASM bundle) instead of returning the already-computed output. Added `resume_output: Option<serde_json::Value>` to `InlineGate`; on approval, return the cached output if present. Mirror fix in the orchestrator's `execute_single_action_with_inline_retry` (reading `result_json["resume_output"]`) and the structured-batch retry path. - **`effect_adapter::auth_gate_from_extension_result`** now passes `Some(output_value.clone())` as the gate's `resume_output` so the retry has cached state to short-circuit on. - **OAuth callback double-fired.** `oauth_callback_handler` now skips the `ExternalCallback` re-entry when the inline-await path already woke a parked waiter — eliminates the "thread already running" race. - **`resolve_inline_gates_for_credential`** now also discards matching Authentication rows from `pending_gates` so the row doesn't linger in `HistoryResponse.pending_gate` after inline resolution. Fixes the auto-approve footgun: - **`ToolPermissionSnapshot::resolve_permission`** now collapses DB values that match the seeded default to `explicit = None`. Before this, the boot-time `seed_tool_permissions` write of `tool_install -> AskEachTime` was indistinguishable from a user-explicit override, causing `effect_adapter::enforce_tool_permission`'s `is_explicit_ask` check to refuse `AGENT_AUTO_APPROVE_TOOLS=true`. Real overrides (`AlwaysAllow`, `Disabled`) still surface as `Some(...)`. Tests - Unit: 4980/4980 pass (host) + 525/525 pass (engine). - Unit: new tests in `bridge::tool_permissions::tests` lock seeded-vs- explicit collapse; new test in `bridge::action_projector::tests` asserts `tool_install` is callable; new manifest hidden-flag tests in `registry::manifest::tests` and `extensions::manager::tests`. - E2E: removed `@pytest.mark.xfail` on `test_chat_first_gmail_installs_prompts_and_retries` (now passes end-to-end via the chat-driven install path). Added `test_chat_install_approval_then_auth_card` driving the explicit-approval variant with a single Approve click (no Always workaround needed) — wired into the `auth-full` canary lane. - Mock LLM: extended the gmail-install-then-retry pattern to recognize both the legacy "Extension not installed:" and the post-#3533 "is not callable in this execution context" error strings, and to retry `gmail(action="list_messages")` after a successful `tool_install`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(permissions): address #3559 review (permission bypass, lease accounting, hidden search filter) Five fixes from the #3559 review (4× Copilot doc nits + 3× serrrfirat security/correctness findings): 1. **Permission bypass (High).** Pre-#3559's `resolve_permission` collapsed any DB row whose value matched the seeded default to `explicit = None`, so a user who deliberately set `tool_install = AskEachTime` had their explicit choice silently dropped and `AGENT_AUTO_APPROVE_TOOLS=true` bypassed the gate. Provenance is now handled at write time: `seed_tool_permissions` is gone and a one-shot, sentinel-gated migration (`cleanup_ghost_seeded_tool_permissions`) deletes existing ghost-seeded rows at startup. With no ghost rows, the resolver treats every DB row as user-explicit and honors it. 2. **Lease/event accounting on `resume_output` replay (Medium).** Inline-gate handlers in `structured.rs`, `scripting.rs` (`resolve_tool_future` + `drive_inline_gate` retry loop), and `orchestrator.rs` refunded the lease use the action just consumed, then returned the cached `resume_output` on approval without re-consuming — netting successful side-effecting actions to zero lease uses. Skip the refund when the gate carries cached output. 3. **Hidden registry filter on `tool_search` (Medium).** `RegistryCatalog::search` did not filter `hidden: true` entries, so `telegram_mtproto` could resurface through the search path and reintroduce the "two Telegram options" outcome that #3533 fixes for the default-list path. Added the filter and a regression test. 4-7. Copilot doc nits: outdated `_set_tool_permission` docstring; misleading "bridge-side auto-install implemented" comment in `mock_llm.py`; `tool_install` described as "non-agent surface" in `src/bridge/CLAUDE.md` while a paragraph below says the model calls it directly; dangling `issue #3533 / PR —` placeholders in both CLAUDE.md docs. Regression tests: - `bridge::tool_permissions::user_explicit_value_matching_seeded_default_stays_explicit` — the original Copilot/serrrfirat bug case. - `app::cleanup_ghost_seeded_tool_permissions_removes_seed_matching_rows` — idempotent migration + sentinel. - `extensions::registry::test_search_skips_hidden_entries` — hidden entries excluded from search but still installable by exact name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(#3559): caller-level regression coverage for review findings 1 & 2 Two follow-up regression tests for the #3559 security review, plus a real bug surfaced by the first one. 1. `executor::structured::resume_output_replay_consumes_exactly_one_lease_use` exercises the post-execution Authentication gate inline-retry path with `max_uses=1` and asserts: - Cached output is returned as a successful `ActionResult`. - Exactly one `ActionExecuted` event is emitted. - The lease budget is exhausted after one execution (refund-skip keeps the consumption from being undone). Writing this test surfaced a real bug: the structured cached-output branch pushed `ActionExecuted` into `emitted_events`, and the caller's `classify_exec_result` emitted ANOTHER terminal `ActionExecuted` for the same Ok result — double-emit for one action. Tier 1 (`scripting::drive_inline_gate`) and Tier 1 alt (`orchestrator::execute_action_with_inline_gate`) emit themselves because their callers don't run an Ok-branch classifier; structured was the outlier. Dropped the redundant push; the classifier emits the single canonical event. 2. `bridge::effect_adapter::explicit_ask_each_time_for_seeded_default_tool_still_gates` drives `execute_action` end-to-end (the side-effecting caller) with a tool whose `name()` matches a seeded-`AskEachTime` baseline (`tool_install`) and an explicit `AskEachTime` user override. The resolver collapse-to-implicit bug would have shown up here — not just in the helper-level test that already exists in `bridge::tool_permissions::tests`. Per `.claude/rules/testing.md` "Test Through the Caller, Not Just the Helper". Added `SeededAskEachTimeTestTool` as a `tool_install`-named test fixture with `requires_approval: UnlessAutoApproved` to mirror the real tool's contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>