fix(extensions): restore chat-driven tool_install + fix double-invoke + auto-approve footgun (#3533) - #3559
Merged
Conversation
… + 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>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
size: XL
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where chat requests like “connect my telegram” stopped triggering chat-driven installation, and resolves several follow-on issues in the auth/approval gate + inline-await retry flow (double-invokes, stale pending gates, and auto-approve behavior).
Changes:
- Restores agent-callable
tool_install, updates prompts/docs accordingly, and adds coverage to prevent regression. - Adds
hiddento manifests/registry entries and filters hidden entries from default discovery surfaces (e.g.telegram_mtproto). - Prevents duplicate execution around gate retries by caching
resume_output, skips redundant OAuth callback re-entry when inline waiters were woken, cleans up matching pending-gate rows, and fixes seeded-default tool-permission “auto-approve” behavior.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/scenarios/test_v2_auth_oauth_matrix.py | Adds fixtures/tests for chat-first install flows (auto-approve + explicit approval). |
| tests/e2e/mock_llm.py | Extends mock LLM to drive gmail install → retry → auth-gate flow. |
| tests/e2e_advanced_traces.rs | Updates test fixture construction for new hidden field. |
| src/registry/manifest.rs | Adds hidden to manifest parsing and propagates to registry entries + unit test. |
| src/registry/installer.rs | Updates installer tests for new hidden field. |
| src/extensions/registry.rs | Threads hidden through builtin/registry entry creation and tests. |
| src/extensions/mod.rs | Adds hidden field to RegistryEntry for serialization/filtering. |
| src/extensions/manager.rs | Filters hidden registry entries out of default “available but not installed” list + test. |
| src/extensions/discovery.rs | Ensures discovered entries set hidden: false. |
| src/channels/web/features/oauth/mod.rs | Skips redundant external-callback re-entry when inline waiters were woken. |
| src/bridge/tool_permissions.rs | Treats DB values matching seeded defaults as implicit (fixes auto-approve footgun) + tests. |
| src/bridge/router.rs | Discards matching Authentication pending-gate rows when inline gates are resolved. |
| src/bridge/effect_adapter.rs | Carries precomputed action output into gates via resume_output. |
| src/bridge/CLAUDE.md | Updates bridge docs to reflect chat-driven tool_install behavior. |
| src/bridge/action_projector.rs | Removes special-case hiding of tool_install and updates tests. |
| scripts/live_canary/auth_registry.py | Adds new E2E approval-then-auth test to canary lane. |
| registry/tools/telegram_mtproto.json | Marks telegram_mtproto as "hidden": true. |
| crates/ironclaw_engine/src/executor/structured.rs | Short-circuits inline-gate retry with cached resume_output to avoid re-execution. |
| crates/ironclaw_engine/src/executor/scripting.rs | Adds resume_output to InlineGate and short-circuits retries when present. |
| crates/ironclaw_engine/src/executor/prompt.rs | Updates system prompt guidance to call tool_install(name=...) directly. |
| crates/ironclaw_engine/src/executor/orchestrator.rs | Avoids re-executing actions after approval when resume_output is present. |
| crates/ironclaw_engine/CLAUDE.md | Updates engine docs to reflect the restored tool_install contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+309
to
+315
| """Override a tool's permission state via the settings API. | ||
|
|
||
| The seeder writes the seeded default (AskEachTime for `tool_install`) to | ||
| DB at startup, which the auto-approve check in `effect_adapter` treats | ||
| as a user-explicit override and refuses to bypass even with | ||
| `AGENT_AUTO_APPROVE_TOOLS=true`. Test fixtures that need a tool to | ||
| auto-approve must explicitly set its permission to `always_allow`. |
Member
Author
There was a problem hiding this comment.
Fixed in a46144d. The docstring now reflects post-#3559 semantics: no DB row exists for seeded-default tools (the seeder was removed and a one-shot startup migration cleans up ghost rows), so AGENT_AUTO_APPROVE_TOOLS=true bypasses the code-level baseline without needing this helper. The helper is now used only for forced overrides (no-auto-approve fixtures, or pre-approving non-seeded tools).
Comment on lines
+1246
to
+1249
| # installed:" (pre-#3533, latent path with bridge-side auto-install | ||
| # implemented) or "is not callable in this execution context" | ||
| # (post-#3533, engine-side preflight rejection, tool_install | ||
| # re-enabled on the agent surface). |
Member
Author
Comment on lines
+39
to
+45
| Integrations that need user setup (`NeedsSetup`, `Inactive`, | ||
| `AvailableNotInstalled`) surface in the prompt under `Activatable | ||
| Integrations`, but the model cannot enable them itself — it tells the user | ||
| to install/activate them through the IronClaw UI. | ||
| Integrations`. The model installs them by calling `tool_install` directly; | ||
| the engine's auth preflight handles any credential prompt at execute time. | ||
| (Restored in issue #3533 / PR — `tool_install` was previously hidden from | ||
| the model surface, which left "connect my telegram" narrating manual UI | ||
| steps instead of running the actual install.) |
Member
Author
There was a problem hiding this comment.
Fixed in a46144d. The tool_install bullet in src/bridge/CLAUDE.md now reads "callable; agent-callable" instead of "non-agent surface", and notes that user consent is mediated by ApprovalRequirement::UnlessAutoApproved and the seeded AskEachTime permission rather than by hiding the tool. The two paragraphs are now consistent.
| `Activatable Integrations` and the model tells the user to install/activate | ||
| them through the IronClaw UI — the model cannot enable them itself. | ||
| `Activatable Integrations` and the model installs them by calling | ||
| `tool_install(name="<name>")` directly (issue #3533 / PR — the hidden gate |
Member
Author
There was a problem hiding this comment.
Fixed in a46144d. Both occurrences of the dangling issue #3533 / PR — now read issue #3533 / PR #3559.
Collaborator
Security/correctness review findingsReviewed CI snapshot: Findings
Notes
|
…unting, 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>
Member
Author
|
@serrrfirat Thanks — all three findings landed in a46144d.
For the duplicate-event concern in finding 2 — I traced the |
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>
Member
Author
|
Follow-up on finding 2 in cd20e29. While writing the The structured cached-output branch pushed Two regression tests landed:
Thanks for the catch on both — the doubled event would have been a quiet audit drift bug. |
This was referenced May 15, 2026
Merged
Closed
serrrfirat
added a commit
that referenced
this pull request
May 26, 2026
* fix(e2e): restore auth and approval coverage (#3430)
* test(e2e): avoid REPL auth retry race (#3437)
* feat: add pairing_approve tool for Slack binding via chat (#3396)
* feat: add pairing_approve tool for Slack binding via chat
Users can now paste their Slack pairing code in the IronClaw chat and
the LLM will call pairing_approve to bind their accounts. No need to
use the API directly.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: require approval before pairing + fix formatting
Address review comment: pairing_approve now requires UnlessAutoApproved
approval before executing, preventing accidental account binding.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test: add regression test for pairing_approve tool
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address review — Always approval, lock channel, add to protected list
1. Changed ApprovalRequirement to Always (not bypassable by auto-approve)
2. Locked channel to slack-relay constant (removed generic channel param)
3. Added pairing_approve to PROTECTED_TOOL_NAMES
4. Added tests: always-approval, protected-name, channel constant
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(web): isolate cross-tenant SSE/WS status events and thread access (#3390)
* fix(web): isolate cross-tenant SSE/WS status events and thread access
Plug a multi-tenant leak where unscoped `sse.broadcast(...)` calls
from `GatewayChannel::send_status`, sandbox `JobEvent` dispatch, the
WASM/Slack OAuth completion handlers, and any producer that lost
`metadata.user_id` along the way fan out to every connected
subscriber — exposing another tenant's tool calls, tool output,
onboarding state, and job lifecycle to anyone with an open SSE/WS.
Changes
- Extract `dispatch_status_event(sse, multi_tenant_mode, user_id, ev)`
from `Channel::send_status`. In multi-tenant mode an unscoped event
is dropped (with a WARN naming the producer to fix); single-tenant
keeps the global broadcast since there is one subscriber population.
- `IncomingMessage::new` now defaults `metadata` to `{"user_id": ...}`,
and `with_metadata` preserves the key so downstream `send_status`
consumers always have an owner to scope by.
- WASM/Slack OAuth completion broadcasts route through
`broadcast_for_user(&owner_id, ...)`. Sandbox `JobEvent` dispatch
in `main.rs` respects `multi_tenant_mode` for the empty-`user_id`
fallback.
- New pre-commit check #10 (`MULTITENANT`) flags unscoped
`sse.broadcast(...)` lines without a `// multi-tenant-safe: <reason>`
marker or a transport-only exemption. Marker regex accepts the marker
anywhere in a `//` comment so compound annotations on a single line
work.
Tests
- `src/channels/web/tests/status_event_isolation.rs` — 5 unit tests
covering both modes and the per-variant drop invariant.
- `src/channels/web/platform/sse.rs` — 2 quadrant tests for the
`subscribe_raw` filter (scoped/unscoped × matching/mismatched).
- `tests/thread_isolation_integration.rs` — 9 HTTP-level checks that
Bob cannot reach Alice's chat history (paginated and not), threads
list, engine v2 detail/steps/events, or Responses GET, plus an
unauthenticated-rejection guard.
- 8 new self-test cases for the `MULTITENANT` script check, including
the compound projection-exempt + multi-tenant-safe annotation case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(web): pin cross-tenant boundaries on jobs, files, routines
Audit of the protected route surface for the same bug shape #3390 fixed
(handler that takes a user-controlled id and reads without an ownership
predicate) found that the implementations were correct but four
boundaries had no integration test. Lock them in before they regress.
- Sandbox job persisted-events history (`/api/jobs/{id}/events`):
Bob → 404 on Alice's job; Alice → 200 on her own.
- Sandbox job workspace listing (`/api/jobs/{id}/files/list`):
Bob → 404 on Alice's job.
- Sandbox job file read (`/api/jobs/{id}/files/read`): Bob → 404 on
Alice's job; Alice → 200 on her own; Alice → 403/404 on
`?path=../outside.txt` (path-traversal pin against the
`canonicalize() + starts_with(base_canonical)` guard).
- Routine run history (`/api/routines/{id}/runs`): Bob → 404; Alice
→ 200 with at least one seeded run.
The OAuth-state and NEAR-nonce stores were also flagged in the audit
but neither is a real cross-tenant bug: both are pre-auth, single-use,
and the token IS the secret. Documenting here so a future audit
doesn't re-flag them.
Project-static (`/projects/{id}/...`) is left for a follow-up — it
relies on `ironclaw_base_dir()` which is a process-wide `LazyLock`,
making per-test override fragile in the integration runner.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): address PR #3390 review — forge-resistant metadata, OAuth toast routing
Addresses six review comments from gemini-code-assist, copilot, and
serrrfirat on PR #3390. False-positives and the perf nit on
`with_metadata` are explained in the reply thread, not changed in code.
- (HIGH, serrrfirat) `IncomingMessage::with_metadata` now ALWAYS sets
`metadata.user_id` from `self.user_id`, dropping any caller-supplied
value. A WASM channel emitting `{"user_id":"victim"}` via
`apply_emitted_metadata` can no longer reroute downstream
`ToolStarted` / `ToolResult` SSE events into another tenant's
stream. New unit tests pin the forgery-resistance invariant.
- (MEDIUM, copilot + serrrfirat) Slack relay OAuth callback now
broadcasts the completion toast to the resolved `oauth_user`
(the IronClaw user who initiated the flow) rather than
`state.owner_id`. In multi-tenant deployments those differ and the
previous routing delivered the toast to the wrong browser tab.
Extracted the lookup into `resolve_relay_oauth_user`; two unit
tests cover the secret-present and secret-missing cases.
- (MEDIUM, gemini) `dispatch_status_event` treats empty-string
`user_id` the same as `None` so producers that lost the field
along the way fail-closed instead of falling through to a global
broadcast in multi-tenant mode.
- (MEDIUM, gemini) `main.rs` sandbox JobEvent dispatch now reuses
`dispatch_status_event` instead of duplicating the drop / WARN /
broadcast policy. `dispatch_status_event` is bumped from
`pub(crate)` to `pub` so the binary crate can call it.
- (LOW, copilot) Pre-commit `MULTITENANT` self-tests gain three
cases (`state.sse.broadcast(`, `gw_state.sse.broadcast(`,
annotated receiver-prefixed) to lock the existing boundary regex
behaviour against future tightening.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(channels): preserve i64 metadata.user_id from Telegram in with_metadata
PR #3390's forge-resistance fix made `IncomingMessage::with_metadata`
*always* overwrite `metadata.user_id` with `self.user_id` as a String.
That broke the Telegram WASM channel: it persists Telegram's chat user
ID as `metadata.user_id: i64` and re-deserializes it into
`TelegramMessageMetadata { user_id: i64, ... }` in `on_respond` /
`on_status`. After the fix, `respond` blew up with
`invalid type: string "999", expected i64 at line 1 column 87`,
failing 3 Telegram integration tests in CI.
Narrow the carve-out: overwrite only when the existing `user_id` is a
String (or missing). Non-string values are channel-private and the
SSE routing layer reads via `as_str()` — non-strings already fail
closed in multi-tenant mode, so the forge threat (WASM emits
`{"user_id":"victim"}` as a string) is still mitigated, while
Telegram's i64 use case survives.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): redact WARN payload + harden dotdot traversal test (PR #3390)
Two follow-up fixes from the second pass of review on #3390:
1. `dispatch_status_event`'s WARN log used `?event`, which on
`AppEvent::Response` / `Thinking` / `ToolResult` carries
user-authored content into operator logs in a multi-tenant
deployment. Replace with `event_kind = event.event_type()`
(the wire-stable variant name) — enough to identify the
misbehaving producer without leaking tenant data. Picked up via
Copilot's review on `src/channels/web/mod.rs:666`.
2. `alice_job_file_read_rejects_dotdot_traversal` planted
`outside.txt` under `outer.path()` (the `start_server_with_db`
fixture's tempdir holding `test.db`) but probed
`?path=../outside.txt` relative to `alice_proj` — a separate
`tempfile::tempdir()` rooted at the OS temp directory. The two
paths were unrelated, so the test could pass even if `..`
traversal was permitted (probe just hit empty space). Build the
directory tree by hand instead: `parent/alice_proj/` with the
planted file at `parent/outside.txt`, so the probe deterministically
resolves to the planted bytes. Add a body-content assertion that
fails loudly if those bytes leak. Picked up via Copilot's review on
`tests/cross_tenant_resource_isolation.rs:355`.
Plus a `cargo fmt` fix for `src/channels/channel.rs:1202` that was
breaking the Formatting CI check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): address PR #3390 follow-ups — multi-tenant fallback WARN, exhaustive variant pin
- `resolve_relay_oauth_user`: take `multi_tenant_mode`; emit WARN when the
`relay:{ext}:oauth_user` secret is missing in multi-tenant mode so the
unrecoverable-initiator case surfaces in operator logs. Single-tenant
fallback stays silent (owner == only user).
- `dispatch_status_event`: doc note clarifying the function is `pub` only
for the sandbox JobEvent rx loop in `main.rs`; not part of a stable
public API.
- `_compile_time_appevent_variant_check`: exhaustive-match helper paired
with `unscoped_drop_holds_for_every_status_variant_in_multi_tenant`.
Adding a new `AppEvent` variant now fails the test build, prompting an
update to both the helper and the runtime leak-candidate list.
- `tests/thread_isolation_integration.rs`: honest scope note on the
engine-v2 thread tests — they pin handler shape (404/empty for
unknown id), not the cross-tenant ownership branch. Cross-tenant
engine-v2 coverage requires an `ENGINE_STATE` test fixture; tracked
as a follow-up in the comment block.
Tests: 8 unit (5 status_event_isolation + 3 resolve_relay_oauth_user)
and 9 thread_isolation_integration pass; clippy clean; pre-commit
safety scripts pass (regression suite 27 cases).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): unconditionally consume relay:{ext}:oauth_user secret in OAuth callback
The previous cleanup site lived inside the `if let Some(pairing_store)`
branch of the result block, which was unreachable on three failure
paths:
1. `pairing_store` is None (no identity pairing wired up)
2. The result block `?`-short-circuits before reaching the `if let`
(e.g. `set_setting` fails, `activate_stored_relay` fails, an inner
`relay_config()` / `list_connections()` errors)
3. The `if let` body itself errors before reaching the delete (e.g.
`list_connections` returns no matching team)
Leaving the secret behind lets a subsequent OAuth callback for the
same extension read a stale initiating user and misroute the
completion toast — Copilot review on PR #3390 (comment id 3211833864).
Move the delete to right after `resolve_relay_oauth_user` returns,
where it always runs once the value has been captured, regardless of
downstream failure mode. Updated the inner comment to document that
the secret is already gone by the time the pairing branch reads
`oauth_user`.
Regression test: `test_relay_oauth_callback_consumes_oauth_user_secret_on_failure_path`
seeds the secret, fires the callback against a fixture with no real
relay backend (so the result block deterministically errors), and
asserts the secret is gone afterward. Pre-fix this would have left
the secret behind on the failure path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): tighten Telegram pairing UX and OAuth-failure recovery (#3317, #3319, #3320) (#3381)
* fix(auth): tighten Telegram pairing UX and OAuth-failure recovery (#3317, #3319, #3320)
Three Bug Bash P1 issues from the same user journey: setup → use → fail.
The unifying root cause was per-channel auth tested in isolation; cross-channel
flows (Telegram → Gmail OAuth → resume) had no coverage and three small leaks
combined into a stuck conversation.
#3317 — Telegram pairing reply now names every IronClaw surface explicitly
(web settings, agent chat, terminal). The agent submission parser learns
`approve <channel> <code>`, dispatched through a new bridge handler that
mirrors `POST /api/pairing/{channel}/approve`.
#3319 — OAuth callback failures now log a category + correlation ID so a
user-reported "I saw 400" maps to one log line. Adds the
`OauthCallbackFailure` enum and `oauth_failure_correlation_id` helper.
#3320 — Two cleanup gaps fixed: (a) `/clear` now drains
`pending_oauth_flows` for the user (otherwise stale flows linger 5min and
mask new auth attempts); (b) OAuth provider-error and exchange-failure paths
now auto-cancel the engine pending auth gate via `clear_engine_pending_auth`,
so the conversation isn't blocked waiting for a resume that will never arrive.
Tests: 5 new submission-parser tests, 2 new bridge-handler tests, and one
new OAuth callback test verifying the pending-flow drain on provider error.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(auth): cross-channel pairing claim coverage + canary lane (#3317)
Adds the structural coverage that was missing when #3317 shipped:
- E2E (`tests/e2e/scenarios/test_telegram_pairing_chat_claim.py`):
three scenarios that drive the full Telegram pairing flow through
the gateway. Asserts the bot reply names every IronClaw surface
(web Settings, agent chat, terminal CLI), drives `approve telegram
CODE` through `/api/chat/send` and verifies the paired user
exchanges messages without re-prompting, and confirms invalid
codes get a clear rejection instead of an LLM-improvised reply.
- Rust integration (`tests/telegram_pairing_chat_claim_integration.rs`):
drives `Submission::PairingClaim` through a real `Agent` →
`bridge::handle_pairing_claim` → `PairingStore::approve` chain
using `TestRig` with engine v2 enabled. Covers the happy path
(mints a code, claims it via chat, asserts `Pairing approved`)
and the invalid-code rejection. The unit tests in `bridge/router`
cover only the no-extension-manager and invalid-channel branches —
this test exercises the wiring between submission parser, agent
loop dispatch, and bridge handler that #3317 specifically broke.
- Canary (`scripts/live_canary/auth_registry.py`): adds the two
user-visible scenarios to `AUTH_CHANNEL_TESTS` so the auth-channels
lane (scheduled every 6h) catches the regression class in CI
before any real user encounters it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: align pairing-claim and oauth-correlation comments with code (#3381)
Three Copilot review comments on PR #3381 flagged docstring/code drift in
already-merged PR #3317/#3319/#3320 changes. No behavior change — only
the doc strings move:
- `Submission::PairingClaim.code` and the inline `approve <channel> <code>`
parser comment claimed the user's casing was preserved, but the parser
builds the code from `lower` and the regression tests already lock in
the lowercased shape (`code == "abc12345"`). Update both comments to
describe the actual normalize-then-store contract.
- `oauth_failure_correlation_id` claimed the correlation appeared in the
user-facing error subtitle, but the failure path renders
`landing_html(label, false)` whose subtitle is fixed and never receives
the correlation. Mark the helper as logs-only and note that plumbing
the ID through the HTML is a follow-up.
[skip-regression-check] doc-only, behavior already covered by existing
pairing-claim parser tests in submission.rs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(telegram): bump channel registry to 0.2.11
The pairing-reply wording was updated in channels-src/telegram/, which
the version-check CI requires be matched by a registry version bump.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(telegram): fix import path in pairing chat claim e2e test
The scenarios/ folder is a Python package (has __init__.py), so a
flat `from test_telegram_e2e import …` fails with
ModuleNotFoundError during pytest collection. Switch to a relative
import that matches the package layout, and drop the unused
OWNER_USER_ID symbol while we're here.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): address Copilot review on /clear OAuth drain + correlation doc
Two follow-ups on PR #3381's Copilot pass:
1. `Agent::process_clear` (engine v1 path) now drains in-flight OAuth
flows for the clearing user, mirroring the engine-v2 cleanup added in
`bridge::router::clear_engine_conversation`. Without this, `/clear`
was a clean slate on v2 but v1 left ghost flows in
`extension_manager.pending_oauth_flows()` until the 5-minute
`OAUTH_FLOW_EXPIRY` ticked over — same regression class #3320 fixed
on v2.
2. `oauth_failure_correlation_id`'s docstring previously said "redacted
state fingerprint", but callers seed it with the raw `state` query
value (or `flow.extension_name` for post-resolution failures).
Updated the doc to describe the actual behaviour: an arbitrary seed
that is hashed before any hex output, with a pointer to
`redact_oauth_state_for_logs` for the log-safe fingerprint.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(e2e): make Telegram pairing chat-claim suite actually run
The scenario landed in PR #3317 was orphaned — never wired into any CI
lane and could not pass even when run by hand. Three structural
issues, all fixed here:
1. `install_telegram` now overlays the locally-built WASM (and matching
capabilities file) on top of the registry-downloaded artifact when
present. The pairing-reply wording lives inside the WASM binary, so
without this overlay the test was asserting source-tree text against
the previous release's bytes. The overlay is best-effort: when the
local WASM is absent (CI groups that don't build the channel), the
test that depends on it skips with a clear message and the canary
lane in `scripts/live_canary/auth_registry.py` still covers the
wording end-to-end against the deployed binary.
2. `Submission::PairingClaim` is handled out-of-band by the bridge
layer; the response is delivered via `WebChannel::respond` →
`AppEvent::Response` over SSE only — no `Turn` is persisted, so
polling `/api/chat/history` could never see it. Refactored
`test_chat_surface_approves_pairing_code` and
`test_chat_surface_rejects_invalid_pairing_code` onto a
`_send_and_collect_response` helper that opens the SSE stream first
(so the broadcast doesn't fan out to zero subscribers) and matches
on the `response` event for the test thread.
3. Wired the file into `e2e.yml`'s `extensions` group so the suite
actually runs on every PR.
Verified locally: all three scenarios pass, plus the existing 22
Telegram e2e tests still green with the install-overlay change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(bridge): bound and sanitize invalid-channel echo in pairing claim
Address Copilot review on PR #3381: `handle_pairing_claim`'s
invalid-channel branch was rendering the raw `channel` token (and the
underlying `IdentityError`, which itself echoes the offending input)
back to the user. Both routes are unbounded and could carry control
characters or markup since `channel` comes from chat input — a
hostile prompt could blow up the SSE / Telegram / TUI reply or smuggle
backticks/escape sequences through.
Cap the echo at 32 ASCII-alphanumeric (or `-`/`_`) characters and
replace the verbatim error with a fixed category description, so the
reply size and shape are bounded by what we render explicitly. Add a
regression test that drives a 200-char hostile blob (control chars +
backticks) through the handler and asserts the rendered reply stays
clean and short.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(bridge): include hyphens in invalid-channel error copy
Address Copilot review on PR #3381: the invalid-channel reply
listed "lowercase letters, digits, or underscores" as the valid
character set, but `ExtensionName::new` (and `web::features::pairing::
parse_channel`) intentionally accept hyphens too — they're folded to
underscores during canonicalization. A user typing `slack-relay` would
otherwise get an "invalid name" reply listing rules that contradict
the actual validator.
Updated the message to include hyphens with `telegram` and
`slack-relay` as concrete examples, and tightened the regression test
to assert against the user-controlled preview region between the
delimiter backticks rather than a global backtick count (which was
fragile to copy that includes example slugs in backticks).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): address PR #3381 review on credential-scoped gate cleanup and Telegram surface promise
Three reviewer findings, one commit:
- OAuth provider-error and exchange-failure paths used
`clear_engine_pending_auth(user, None)`, which discards every
Authentication gate for the user. A failed Gmail callback could
silently wipe an unrelated Slack/MCP gate waiting on a different
thread. New `clear_engine_pending_auth_for_credential(user, credential)`
helper in bridge::router scopes cleanup to the failed flow.
Provider-error path tracks `removed_secret_name` alongside
`removed_user_id` so the scoped variant is callable.
- Expired-flow branch in the OAuth callback handler had two bugs: it
never cleared the engine pending auth gate (so the conversation sat
blocked forever, same #3320 class the provider-error fix addresses),
and the broader `clear_auth_mode` it called would re-discard via
the unscoped helper anyway. Now calls the credential-scoped helper
and the legacy-v1-only `clear_session_auth_mode_for_thread`.
- Telegram pairing reply advertised `approve telegram CODE` as
usable "in any IronClaw chat (TUI / web / Telegram)", but an
unpaired Telegram DM is intercepted by the allowlist gate before
the agent parser sees the command — the user would just get
another pairing reply. Reply now lists only the surfaces that
actually work (web / TUI / CLI) and a comment explains why.
Regression coverage:
- `clear_engine_pending_auth_for_credential_only_clears_matching_credential`
locks in helper scoping (Gmail/Slack two-gate scenario).
- `oauth_callback_expired_flow_clears_credential_scoped_engine_gate`
drives the full callback through axum oneshot with engine state
seeded; asserts the matching gate clears and the unrelated gate
survives.
- E2E `test_telegram_dm_approve_command_is_intercepted_by_allowlist_gate`
exercises the Telegram webhook path (not /api/chat/send) to lock in
the channel-layer interception, wired into the auth canary lane.
- Existing E2E pairing-reply test gains an assertion that
"TUI / web / Telegram" is *not* in the reply.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(oauth): mirror failure-cleanup contract on provider-error and reconcile stale comment
Copilot review on PR #3381 caught two real issues in the credential-scoped cleanup
landed in d45cf1bd8:
- Provider-error branch (`?error=access_denied`) returned the error page
without broadcasting `OnboardingState::Failed` or clearing the legacy v1
session `pending_auth`. The exchange-failure and expiry branches do both.
Net effect: the auth card stayed spinning and the next user message was
intercepted as a token. Now mirrors the other failure paths — keep the
full `flow`, emit Failed SSE, clear v1 session, clear credential-scoped
engine gate, then return the error page.
- Post-exchange comment said "failed callbacks should leave the gate
visible for retry" — that was the pre-#3320 contract. Rewrote it to
describe the new shape: each failure mode clears its own gate at the
failure site; this section only handles legacy-v1 session cleanup that
runs regardless of outcome. Also explains why we use
`clear_session_auth_mode_for_thread` here instead of `clear_auth_mode`
(the latter would re-clear the engine gate on the *success* path and
break the `ExternalCallback` resume).
Regression: `test_oauth_callback_provider_error_broadcasts_onboarding_failed`
in the oauth tests module — drives an `?error=access_denied` callback with
a flow whose `sse_manager` is attached, asserts the receiver gets
`OnboardingState::Failed` with the provider's `error_description` as the
message body.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(common): describe paths and platform helpers in crate description (#3498)
Align the package `description` and the lib.rs crate-level doc with
the modules now exposed from `ironclaw_common` (paths, platform,
env_helpers, attachment), which #3387 lifted out of `src/`. The
previous wording predates that extraction and only mentioned "types
and utilities".
This is also the release-plumbing trigger for v0.28.1: release-plz
proposes a leaf bump on source-path changes, and once `ironclaw_common`
crosses to a new patch, the root `ironclaw` bump can be added on top
of the release-plz branch (same approach as commit 9e69f22d2 for
v0.28.0). See PR #3372 for the equivalent v0.28.0 trigger.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: release
* chore(release): bump ironclaw to 0.28.1
cargo-semver-checks did not detect API-breaking changes in the
ironclaw_common 0.4.1 -> 0.4.2 leaf bump, so release-plz did not
cascade a bump into the root ironclaw package. Add the root version
bump and CHANGELOG entry manually so this release-plz PR produces
an ironclaw-v0.28.1 tag and triggers cargo-dist.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(llm): hide provider-specific auth, model fetch, and embeddings config behind facades (#3416)
* refactor(llm): hide provider-specific auth, model fetch, and embeddings config behind facades
External callers were reaching into provider-specific modules of
`ironclaw_llm` (`gemini_oauth::CredentialManager`,
`github_copilot_auth::*`, `OpenAiCodexSessionManager`,
`codex_auth::*`, `BedrockConfig`, etc.). Closes those leaks behind a
small set of verb-based public surfaces while keeping per-provider
behaviour inside the LLM crate.
Changes:
1. Extract `oauth_helpers.rs` into a new `ironclaw_oauth` crate. The
loopback OAuth callback listener (port 9876, landing pages,
`OAUTH_CALLBACK_HOST` rules) is shared by every IronClaw OAuth flow
(NEAR AI session login, WASM tool auth, MCP) and never depended on
`ironclaw_llm`. `src/auth/oauth.rs` now `pub use ironclaw_oauth::*`
directly. `ironclaw_llm` no longer depends on `ironclaw_oauth` —
the helper had zero internal callers.
2. Add `ironclaw_llm::auth` facade (`start_login`, `validate_token`,
`default_headers`, `load_persisted_credentials`,
`default_credentials_path`) with backend-agnostic types
(`AuthPrompt`, `LoginRequest`, `AuthOutcome`, `PersistedCredentials`,
`OpenAiCodexLoginOptions`, `AuthBackend`, `CredentialSource`).
Privatize `gemini_oauth`, `github_copilot_auth`, `openai_codex_session`,
`codex_auth` (`pub(crate) mod`). Migrate the wizard, the
`ironclaw login --openai-codex` CLI subcommand, and the LLM config
loader to the facade. Wizard introduces a single `WizardAuthPrompt`
that handles device-code prompts + browser launch for all backends.
3. Add `ironclaw_llm::models::fetch_models_for(provider_id, &opts)`
facade. Privatize `fetch_anthropic_models`, `fetch_openai_models`,
`fetch_ollama_models`, `fetch_openai_compatible_models`,
`is_openai_chat_model`, `openai_model_priority`, `sort_openai_models`.
Wizard's per-backend match collapses to one call. Move classifier
unit tests into `crates/ironclaw_llm/src/models.rs`; rewrite the
two wizard fallback tests through the public API.
4. Decouple embeddings from `ironclaw_llm::BedrockConfig`. New
`crate::workspace::BedrockEmbeddingSetup { region, profile }` carries
only what `BedrockEmbeddings` actually needs. `EmbeddingsConfig::create_provider`
and `BedrockEmbeddings::new` take the new type; callers translate from
`LlmConfig.bedrock` at the boundary (`src/app.rs`, `src/cli/mod.rs`).
5. Add `ironclaw_llm::testing::nearai_test_config(model)` helper for
tests that need a minimal `LlmConfig` shape (no retries, no caching,
NEAR AI backend). Replaces two duplicated 30-line struct literals
in the gateway settings hot-reload tests.
Boundary cleanup is behaviour-preserving: 4,932 main-binary unit tests,
729 ironclaw_llm unit tests, 4 ironclaw_oauth tests, 3 architecture
boundary tests all pass; `cargo clippy --all --benches --tests
--examples --all-features` is clean.
Three `pub` methods on `gemini_oauth::CredentialManager` /
`GeminiOauthProvider` (`get_valid_access_token`, `last_response_meta`,
`count_tokens`) and the `GeminiResponseMeta` struct are now reachable
only crate-internally and have no callers; marked `#[allow(dead_code)]`
with a comment rather than deleted to keep this PR purely a boundary
move (delete in a follow-up if no caller emerges).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(llm): promote dedicated backends into the registry; absorb config validation, defaults, and per-provider overrides into ironclaw_llm
Continues the LLM boundary cleanup from 0addf3ac2. After that commit
provider-specific auth, model fetch, and embeddings config lived behind
facades inside `ironclaw_llm`, but four backend-specific knowledge
sources still leaked out:
1. Validation rules and default values for the dedicated-config
backends (Bedrock cross-region prefixes, OpenAI Codex endpoints
and client_id, Gemini OAuth credentials path defaults) lived
inline in `src/config/llm.rs::resolve`.
2. The dispatcher in `create_llm_provider` matched on backend strings
("nearai", "bedrock", ...) instead of a typed protocol value. The
same booleans (`is_nearai`, `is_bedrock`, `is_gemini_oauth`,
`is_openai_codex`) recurred across `src/config/llm.rs`,
`src/app.rs`, `src/cli/models.rs`, and the wizard.
3. The setup wizard had per-backend specialization in
`step_inference_provider` and `run_provider_setup` (manual menu
pushes for nearai/bedrock/codex/gemini_oauth, four dedicated
`setup_*` entry points dispatched on string compares).
4. `Settings` carried named `bedrock_region`, `bedrock_cross_region`,
`bedrock_profile` columns even though no other dedicated backend
had named columns and adding a new one would mean schema churn.
Layers A-D address each in turn:
* Layer A — `BedrockConfig::build`, `OpenAiCodexConfig::build`, and
`GeminiOauthConfig::build` own validation + defaults inside the
crate. `LlmConfigError` (`MissingRequired` / `InvalidValue`) carries
the failures across the boundary, with a `From` impl into the
binary's `ConfigError`. `src/config/llm.rs` calls the builders;
named-string defaults are gone from the binary. The orphaned
`tests/gemini_oauth_regression.rs` husk is deleted.
* Layer B — `ProviderProtocol` gains four new variants
(`Bedrock`, `OpenAiCodex`, `GeminiOauth`, `NearAi`) plus a
`has_dedicated_config()` predicate. The four dedicated-config
backends (with all aliases) become first-class registry entries in
`providers.json`, so `is_known()` / `model_env_var()` / the wizard /
the gateway handler iterate the registry uniformly. The
`is_nearai`/`is_bedrock`/`is_gemini_oauth`/`is_openai_codex` boolean
spaghetti collapses to protocol comparisons. `OpenAiCodex` and
`NearAi` carry explicit `#[serde(rename = "openai_codex" / "nearai",
alias = ...)]` so the wire-stable adapter strings the gateway and
frontend already use keep working. `LlmConfig::active_model_name()`
is now consumed by `cli/doctor.rs` instead of an inlined partial
dispatch.
* Layer C — `SetupHint` gains four credential-collection variants
(`AwsCredentials`, `OAuthDeviceCode`, `FileBasedCredentials`,
`SessionToken`). The wizard's `step_inference_provider` builds its
menu from a single `registry.selectable()` iteration with generic
env-detection (declared `api_key_env`, plus an Anthropic-specific
OAuth fallback). `run_provider_setup` dispatches on the SetupHint
variant; the remaining `def.id == "..."` checks live inside the
`ApiKey` arm only because Anthropic and GitHub Copilot present a
hybrid choice (API key OR OAuth) the simple `ApiKey` hint doesn't
capture. The synthetic bedrock + nearai entries in
`handlers/llm.rs::build_llm_providers` are deleted; a single
registry-driven loop covers both. ADAPTER_LABELS in
`static/js/surfaces/config.js` gains entries for the new protocols.
* Layer D — `LlmBuiltinOverride` gains a generic
`extras: HashMap<String, String>` bag with `extra(key)` /
`set_extra(key, value)` accessors. The bedrock resolver and wizard
read/write through this bag; `Settings::migrate_legacy_provider_fields()`
drains the named `bedrock_*` columns into `extras` on
`Settings::load_from()` so existing `settings.json` files migrate
losslessly. The named columns are kept (deprecated, marked with
`#[serde(skip_serializing_if = "Option::is_none")]`) for one
release; tracked for deletion in #3443.
`strip_admin_only_llm_keys` and `llm_setting_requires_reload` now
match dotted-path subkeys under `llm_builtin_overrides.*` so a
write to e.g. `llm_builtin_overrides.bedrock.extras.region`
triggers the right gating + chain reload.
Boundary cleanup is behaviour-preserving: 4,933 main-binary unit tests,
739 ironclaw_llm unit tests pass; `cargo clippy --all --benches --tests
--examples --all-features` is clean. New regression tests:
`crates/ironclaw_llm/src/config.rs` (6 builder tests),
`crates/ironclaw_llm/src/registry.rs::dedicated_config_backends_are_in_registry_and_selectable`,
and `src/setup/wizard.rs::legacy_bedrock_fields_migrate_into_extras_on_load`.
Three follow-ups tracked in #3443: delete the deprecated `bedrock_*`
named columns, move `BedrockEmbeddings` out of `src/workspace/` into
the LLM crate (last cargo-feature leak), and drive
`LlmConfig::active_model_name()` off `ProviderProtocol` instead of
backend strings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: add bug-bash regression-snapshot harness
Bug-bash fixtures pin specific open bugs to a deterministic snapshot.
When a bug is fixed, the snapshot diff is the reviewable proof; when
someone reintroduces the bug, the snapshot drifts and CI blocks the
merge.
This commit lands the harness plus the first recorded fixture for
issue #2541 (agent must call a tool, not answer from training data):
tests/e2e_bug_bash_snapshots.rs
`snapshot_summarization_uses_tools` replays the fixture, captures
`ReplayOutcome`, and asserts the YAML snapshot. Gated on
`feature = "libsql"`, same as other replay-snapshot tests.
tests/fixtures/llm_traces/bug_bash/summarization_uses_tools.json
Two-step recorded LLM trace (tool_call -> text) keyed off the
user prompt via `request_hint.last_user_message_contains`.
tests/fixtures/llm_traces/bug_bash/README.md
Coverage map for #2540-#2546 (one recorded, six TODO) plus the
`IRONCLAW_RECORD_TRACE` recording workflow.
tests/snapshots/replay__bug_bash_summarization_uses_tools.snap
Insta YAML snapshot pinning `tool_calls: [echo]`, 2 LLM calls,
and the event-kind histogram. Drift = regression.
* fix(settings): preserve pre-existing extras during legacy bedrock migration
`migrate_legacy_provider_fields` claimed to be idempotent and to drain
named `bedrock_*` columns into `llm_builtin_overrides["bedrock"].extras`
once on load. The previous implementation drained correctly but used
`HashMap::insert` unconditionally, which means a settings file
carrying BOTH a legacy `bedrock_region` column AND an already-populated
`extras["region"]` (manual hand-edit, or a future writer emitting both
shapes during a transition) would silently downgrade to the legacy
value.
Guard each `set_extra` call with `entry.extra(key).is_none()` so the
new-shape value always wins. Clarify the docstring to state this
explicitly.
Add three regression tests in `settings::tests`:
- `legacy_bedrock_migration_round_trips_through_save` — legacy JSON ->
load_from -> serialize -> reload, asserts the deprecated columns are
not re-emitted and extras survive the round trip.
- `legacy_bedrock_migration_preserves_existing_extras` — file with both
shapes; asserts the pre-existing extras value is kept and absent
extras are still backfilled from legacy fields.
- `legacy_bedrock_migration_is_idempotent_in_memory` — calling the
migration twice on the same Settings is a no-op (compares serialized
shape, since LlmBuiltinOverride does not derive PartialEq).
* fix(pr-3416): address PR review — migration on DB/TOML, admin-key gate, codex login, credential_kind/has_credentials
Addresses comments from gemini-code-assist, Copilot, and serrrfirat on PR #3416.
## Bugs
**Legacy bedrock fields not migrated on DB/TOML loads** (serrrfirat, High).
`Settings::load_from` (JSON) ran `migrate_legacy_provider_fields`, but
`from_db_map` and `load_toml` did not. Existing operators with
`bedrock_*` settings persisted in the DB or `config.toml` would silently
lose their AWS region/profile/cross-region after upgrade because the
resolver now reads only from `llm_builtin_overrides["bedrock"].extras`.
Both loaders now call the migration; added round-trip tests for each.
**Admin-only key write gate had narrower matching than read gate**
(Copilot, High). `strip_admin_only_llm_keys` matches both exact keys
and dotted subpaths under admin-only roots; `is_admin_only_setting_key`
in the web settings handler used `.contains(&key)` only. A non-admin
could write `llm_builtin_overrides.bedrock.extras.region` directly,
bypassing the gate. Promoted `is_admin_only_llm_key` to `pub(crate)`,
made the web write-side gate call it, added regression tests covering
dotted subpaths.
**`ironclaw login --openai-codex` dropped TOML/DB config** (Copilot,
High). The pre-refactor code resolved `Config::from_env` and used
`config.llm.openai_codex` so endpoint / client-id / session-path
overrides committed via TOML or DB stuck. The post-refactor code only
read env vars via `OpenAiCodexLoginOptions::from_env`. Added
`OpenAiCodexLoginOptions::from_resolved_config(&OpenAiCodexConfig)`;
the login command now prefers the resolved config when present and
falls back to env-only when `Config::from_env` itself fails (fresh
machine, no DB).
**Dedicated-auth backends marked configured without credentials**
(serrrfirat, Medium). `nearai` / `gemini_oauth` / `openai_codex` ship
`api_key_required: false` because they don't authenticate via a bearer
API key. The frontend `isProviderConfigured` treated that as "no
credentials needed" and rendered the Use button on a fresh install,
where clicking could trigger an interactive device-code OAuth from
inside a settings request.
Added `credential_kind` (wire-stable snake_case discriminator matching
`SetupHint::kind()`, e.g. `session_token`, `o_auth_device_code`,
`file_based_credentials`, `aws_credentials`) and `has_credentials`
(backend-authoritative; checks AWS env vars for Bedrock, codex session
file existence, file-based credential path expansion + existence) to
the web LLM providers payload. Frontend `isProviderConfigured` /
`providerMissingReason` now gate non-api-key kinds on `has_credentials`.
## Nits
**`fetch_models_for` doc overclaimed "Always returns something"**
(Copilot). The generic openai-compatible branch returns `vec![]` when
`base_url` is empty. Updated the docstring to call this out so callers
know to handle the empty case.
**`AuthError::Other` used for "validation not applicable"** (Gemini
bot). Added a dedicated `AuthError::TokenValidationNotSupported { backend }`
variant; `validate_token` now returns it for Gemini / OpenAiCodex
instead of stringly-formatted `Other`.
**Bug-bash regression-harness URLs pointed at `near/ironclaw`**
(Copilot, x2). The canonical tracker is `nearai/ironclaw`. Rewrote
all seven URLs in `tests/fixtures/llm_traces/bug_bash/README.md` and
the one in `tests/e2e_bug_bash_snapshots.rs`.
## Declined
The Gemini bot's MalformedConfig suggestion at
`crates/ironclaw_llm/src/models.rs:46` was not adopted: the call site is
the openai-compatible model-listing path, not a security-sensitive
request. The fetcher early-returns `vec![]` on empty `base_url` — no
URL parsing happens — and the docstring tightening above covers the
observable surprise. Promoting it to a typed error would change the
public-facing `fetch_models_for` signature for no behavioural gain.
## Tests
- `cargo fmt --check` clean
- `cargo clippy --all --benches --tests --examples --all-features` zero warnings
- `cargo test --lib` 4,941 / 4,941 pass
- `cargo test --features libsql --test e2e_bug_bash_snapshots` 1 / 1 pass
- New regression tests:
- `settings::tests::legacy_bedrock_fields_migrate_into_extras_on_db_load`
- `settings::tests::legacy_bedrock_fields_migrate_into_extras_on_toml_load`
- `channels::web::features::settings::tests::test_admin_only_setting_keys_cover_dotted_subpaths`
- `channels::web::handlers::llm::tests::test_llm_providers_expose_credential_kind_and_has_credentials`
- `channels::web::handlers::llm::tests::test_nearai_has_credentials_true_when_session_token_loaded`
* fix(pr-3416): tighten Bedrock/Codex has_credentials probes; collapse set_extra into one .into()
- `backend_has_credentials` for AWS now requires `AWS_PROFILE` OR
(`AWS_ACCESS_KEY_ID` AND `AWS_SECRET_ACCESS_KEY`). The lone
`AWS_ACCESS_KEY_ID` / `AWS_SESSION_TOKEN` arms previously flipped
has_credentials true even though the AWS SDK can't sign without the
secret key, so the UI was rendering Bedrock as configured on hosts
that would fail at first call.
- `backend_has_credentials` for OpenAI Codex now honours
`OPENAI_CODEX_SESSION_PATH` via `read_env` before falling back to
the default session path under `~/.ironclaw/`. Users with a custom
session location were seeing "not configured" despite a valid login.
- New regression tests `test_bedrock_partial_aws_env_reports_not_configured`
and `test_openai_codex_honours_session_path_env` drive the
`build_llm_providers` call site (not just the helper) so both gaps
stay closed.
- Tidied `LlmBuiltinOverride::set_extra` to convert the key once and
reuse it across the remove/insert branches; behaviour identical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(providers): default nearai model to "auto"
Switch the nearai registry entry's `default_model` from
`claude-sonnet-4-5` to `auto`, NEAR AI's server-side routing alias.
New installs without `NEARAI_MODEL` set now get auto-routed instead
of being pinned to a specific Anthropic model.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Make Skills E2E lifecycle deterministic (#3309)
* test(e2e): make skills lifecycle deterministic
* test(e2e): address skills review comments (#3309)
* test(e2e): unxfail two auth-matrix tests now that contracts match (#3589)
Both xfails in tests/e2e/scenarios/test_v2_auth_oauth_matrix.py are
stale and pass against current code:
- test_wasm_tool_first_chat_auth_attempt_emits_auth_url
Marked xfail in #3235 because the engine-v2 callable-only contract
(#2868) stopped emitting an auth gate on direct LLM-driven tool
calls. PR #3157 (auth-preflight + inline-await) restored the
behavior the test asserts: when the LLM emits a direct call to a
not-yet-authed extension, the bridge raises an Authentication gate
with auth_url populated (src/bridge/effect_adapter.rs:1356-1392).
Marker removed; test passes.
- test_settings_first_custom_mcp_auth_then_chat_runs
The xfail reason claimed post-auth tool-output propagation was
broken. Real cause: engine-v2 gates the first MCP tool call on
`approval` and the browser fixture has no auto-approve UI, so the
chat sat in pending_gate forever. Same shape as the bugs fixed in
#3235 for test_wasm_tool_oauth_refresh_on_demand and
test_mcp_same_server_multi_user_via_browser. Inserted
_wait_for_tool_call between _send_chat and _wait_for_response_contains
to drive approval through the API; test passes.
Verified locally: both tests pass back-to-back in 27s on a fresh
auth_matrix_server.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(extensions): restore chat-driven tool_install + fix double-invoke + auto-approve footgun (#3533) (#3559)
* 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>
* chore: release
* feat(engine): IRONCLAW_DISABLE_CODEACT flag to disable v2 CodeAct (#3665)
* add flag to disable codeact on engine v2
* fmt
* fix(engine): keep compact actions reachable when CodeAct is disabled
With IRONCLAW_DISABLE_CODEACT=true the structured-tools prompt told
the model to use the provider's tool_calls interface for every action,
but the bridge filtered the provider tool list down to
emits_full_schema_tool(). Most tools default to CompactToolInfo
(mission_create, gmail_send, notion_search, ...), so they appeared in
the prompt as "available" while being absent from the provider tool
list — i.e. unreachable. Addresses serrrfirat's review on PR #3665.
Fix coordinates both halves of the surface:
- src/bridge/llm_adapter.rs: in disabled-CodeAct mode, drop the
emits_full_schema_tool() filter and emit every action into the
provider tool list with its full schema.
- crates/ironclaw_engine/src/executor/prompt.rs: in disabled-CodeAct
mode, skip the "## Enabled Tools" section. The compact-form listing
with the tool_info(detail="schema") instruction is meaningless when
the provider already sends full schemas, and would just duplicate
the surface. "## Activatable Integrations" stays — the model still
needs to know what tool_install can target.
Test seam: build_codeact_system_prompt_inner now takes disable_codeact
as an explicit parameter, called once at the public entry points. This
lets prompt tests exercise both branches without process-global env
mutation.
Tests:
- executor::prompt::tests::disabled_codeact_omits_enabled_tools_section_and_keeps_activatable
- bridge::llm_adapter::tests::complete_emits_compact_actions_when_codeact_disabled
- existing complete_with_tools_only_emits_full_schema_provider_tools
now serialized via lock_env() so env mutation in the new test
can't leak across parallel runs.
cargo test -p ironclaw_engine --lib: 527 passed
cargo test --lib bridge::: 469 passed
cargo clippy -p ironclaw_engine --all-targets -- -D warnings: clean
cargo clippy --lib --tests -- -D warnings: clean
cargo fmt --check: clean
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Emil Bogomolov <emil.bogomolov@near.ai>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Fix markdown_to_mrkdwn to avoid converting emphasis inside generated <… (#3532)
* agent: Fix markdown_to_mrkdwn to avoid converting emphasis inside g…
* agent: Fix rustfmt/clippy CI failure by removing extra blank line b…
* agent: slack: fix markdown_to_mrkdwn replacement order to satisfy p…
* slack: protect generated links and sanitize sentinels in markdown_to_mrkdwn
Two issues raised on PR #3532 review:
1. Emphasis inside generated `<url|text>` was still rewritten because the
global `**`/`~~` → `*`/`~` substitution ran after link materialization.
Push the generated link span into the same protected arena used for
Slack-native `<...>` constructs so subsequent global replacements can't
reach inside it. Matches the PR's stated goal.
2. Untrusted input containing the private-use sentinel chars
(U+E000 / U+E001) could forge a protected-span reference and pull in
another span's content. Strip those chars from input up front.
Adds regression tests for both. Bumps registry/channels/slack.json
0.3.2 → 0.3.3 to satisfy the channel-source version-bump check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* slack: escape link labels, drop pipe/gt URLs, expand nested sentinels
Addresses two follow-up review concerns on PR #3532:
Copilot review: `<url|text>` was built by string concatenation, so
`|` or `>` inside the URL would corrupt the entity, and `<` / `>`
inside the label would open/close a Slack span and break the link.
The label now escapes `<` → `<` and `>` → `>` (Slack's documented
literal-character form); a URL containing `<`, `>`, or `|` falls back
to leaving the original markdown form intact (those chars are not
valid URL characters per RFC 3986 anyway).
Latent nested-sentinel bug introduced by the previous fix: a markdown
link whose label contained a Slack-native `<...>` span (e.g.
`[<@U1> hi](url)`) ended up with the inner sentinel buried inside the
arena entry for the outer link span. The final restore pass advances
past the outer sentinel without rescanning what it just emitted, so
the raw U+E000/U+E001 characters would leak into the output. URL and
label are now pre-expanded before the link span is pushed.
While here, factor the duplicated restore loop into
`expand_protected_spans`, reused by both the pre-link expansion and
the final restore, and lift the sentinel constants to file scope.
Adds three regression tests covering label-bracket escaping, the URL
pipe/gt fallback, and the nested-span case. Bumps
registry/channels/slack.json 0.3.3 → 0.3.4.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(gateway): add logs download button (#3588)
* feat(web): support externally-provided tools in Responses API (#3122)
* feat(web): support externally-provided tools in Responses API
Lets callers of `/v1/responses` (and `/api/v1/responses`) declare their
own `function`-typed tools and feed back results via
`function_call_output` items, matching the OpenAI Responses wire shape.
Since IronClaw's engine has no per-request tool surface, integration
happens at the prompt level: the catalog is rendered as
`<external-tools>` in the user message and the agent signals a call by
ending its response with a fenced ```` ```tool_call ```` block. When
that fence is recognised, the reply is split into a leading `Message`
plus a `function_call` `ResponseOutputItem`.
Validation rejects unsupported tool types (`web_search`, `file_search`,
`code_interpreter`) and tools missing `name` with 400, with two new
integration tests covering both paths.
* refactor(responses-api): switch external tools to engine v2 native path
Replace the prompt-level fence protocol from PR #3122 with engine v2
native tool calls: caller-supplied `tools[]` are surfaced as real
LLM-callable actions, the engine pauses with `ResumeKind::External`
when one is invoked, and the bridge router projects the pause to a
new `AppEvent::ExternalToolCall` carrying the OpenAI-shaped
`function_call` wire fields.
The integration is small because v2 already has the right primitives:
- `ResumeKind::External { callback_id }` and
`GateResolution::ExternalCallback { payload }` already existed for
OAuth-style callbacks.
- `agent_loop.rs:1480` already routes Responses API messages to
`handle_with_engine` when `ENGINE_V2=true`, so no v2 migration of
the endpoint itself is needed.
- `EffectBridgeAdapter::execute_action` is the single chokepoint
where caller tools can be detected before they reach the dispatch
pipeline.
Changes:
- New `src/bridge/external_tools.rs` (`ExternalToolCatalog`) — per-thread
registry of caller-supplied `ActionDef`s, plus the `ext_tool:`
callback-id helpers used to disambiguate external-tool pauses from
OAuth/pairing pauses (which also use `ResumeKind::External`).
- `EffectBridgeAdapter` consults the catalog: any name in it is
short-circuited to a `GatePaused { resume_kind: External {
callback_id: ext_tool:<call_id> } }` before any registry dispatch,
and `available_action_inventory` merges the catalog into the
LLM-visible action surface (internal beats external on collision).
- `Submission::ExternalCallback` gains an optional `payload` field;
`bridge::handle_external_callback` plumbs it into
`GateResolution::ExternalCallback { payload }`. Fallback predicate
`gate_resume_is_external` lets non-auth External pauses (i.e.
caller-tool resumes) resolve through the same handler.
- New `AppEvent::ExternalToolCall` projected by `notify_pending_gate`
when a paused gate carries an `ext_tool:` callback id; OAuth/
pairing flows keep flowing through the existing `GateRequired`
channel.
- `responses_api.rs` is gutted of the prompt rendering and fence
parsing (`render_external_tools_preamble`, `extract_trailing_tool_call`,
`parse_external_tool_call`, `ParsedToolCall`, `external_tool_names`
accumulator field, and the `TOOL_CALL_FENCE` constants). The handler
now: rejects `tools[]` when `ENGINE_V2=false`, registers caller
tools in the catalog under the resolved thread id, detects resume
requests (`previous_response_id` + `function_call_output` items in
`input`) and submits them as `Submission::ExternalCallback` with
the outputs as the resolution payload, and surfaces
`AppEvent::ExternalToolCall` as a `function_call` `ResponseOutputItem`
in both streaming (`output_item.added`+`done`) and non-streaming.
- All existing OAuth/pairing `ExternalCallback` constructors updated
to pass `payload: None` (no behaviour change).
- Fence-protocol unit tests removed; replaced with coverage for the
new `responses_tools_to_action_defs` converter and the accumulator's
`ExternalToolCall` arm.
Existing 9 integration tests in `tests/responses_api_path_prefix.rs`
still pass.
Note for reviewers:
- The accumulator-side text response no longer tries to split the
reply on a fenced `tool_call` block. The wire shape that callers
receive for caller-tool invocations is purely event-driven now.
- Internal vs external collision is handled silently by the dedup in
`available_action_inventory` (internal wins). A request-time
rejection for shadowing names is a follow-up — the current behavior
is safe (the LLM only sees the internal version) but could surprise
a caller who expects their tool to run.
* test(responses-api): cover ENGINE_V2-off and resume-without-pending-gate
Two new integration tests for behaviours added by the engine-native
external-tool refactor:
- `external_tools_rejected_when_engine_v2_disabled`: a request with
caller-supplied `tools[]` while `ENGINE_V2` is off must 400 with a
message naming the flag, not silently fall through.
- `resume_without_pending_gate_returns_400`: a request with
`function_call_output` items and a `previous_response_id` that
doesn't correspond to a live external-tool gate must 400, not start
a fresh turn against the (unrelated) thread.
Both tests drive the full router (`start_test_server` + bearer auth)
per `.claude/rules/testing.md` "Test Through the Caller".
* test(responses-api): integration tests + drop unsafe env mutation
Three groups of changes:
1. **Drop unsafe env-var mutation in tests.** `responses_api.rs` no
longer reads `ENGINE_V2` directly: it keys off the presence of the
live `ExternalToolCatalog` (initialized by `init_engine`) as the
"engine v2 is up" signal. The path-prefix test that exercises the
no-engine branch no longer needs `unsafe { std::env::remove_var }`
— the absence of `init_engine` in `TestGatewayBuilder` is what
makes the catalog absent, which is what makes the request reject.
2.…
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
* fix(e2e): restore auth and approval coverage (#3430)
* test(e2e): avoid REPL auth retry race (#3437)
* feat: add pairing_approve tool for Slack binding via chat (#3396)
* feat: add pairing_approve tool for Slack binding via chat
Users can now paste their Slack pairing code in the IronClaw chat and
the LLM will call pairing_approve to bind their accounts. No need to
use the API directly.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: require approval before pairing + fix formatting
Address review comment: pairing_approve now requires UnlessAutoApproved
approval before executing, preventing accidental account binding.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test: add regression test for pairing_approve tool
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address review — Always approval, lock channel, add to protected list
1. Changed ApprovalRequirement to Always (not bypassable by auto-approve)
2. Locked channel to slack-relay constant (removed generic channel param)
3. Added pairing_approve to PROTECTED_TOOL_NAMES
4. Added tests: always-approval, protected-name, channel constant
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(web): isolate cross-tenant SSE/WS status events and thread access (#3390)
* fix(web): isolate cross-tenant SSE/WS status events and thread access
Plug a multi-tenant leak where unscoped `sse.broadcast(...)` calls
from `GatewayChannel::send_status`, sandbox `JobEvent` dispatch, the
WASM/Slack OAuth completion handlers, and any producer that lost
`metadata.user_id` along the way fan out to every connected
subscriber — exposing another tenant's tool calls, tool output,
onboarding state, and job lifecycle to anyone with an open SSE/WS.
Changes
- Extract `dispatch_status_event(sse, multi_tenant_mode, user_id, ev)`
from `Channel::send_status`. In multi-tenant mode an unscoped event
is dropped (with a WARN naming the producer to fix); single-tenant
keeps the global broadcast since there is one subscriber population.
- `IncomingMessage::new` now defaults `metadata` to `{"user_id": ...}`,
and `with_metadata` preserves the key so downstream `send_status`
consumers always have an owner to scope by.
- WASM/Slack OAuth completion broadcasts route through
`broadcast_for_user(&owner_id, ...)`. Sandbox `JobEvent` dispatch
in `main.rs` respects `multi_tenant_mode` for the empty-`user_id`
fallback.
- New pre-commit check #10 (`MULTITENANT`) flags unscoped
`sse.broadcast(...)` lines without a `// multi-tenant-safe: <reason>`
marker or a transport-only exemption. Marker regex accepts the marker
anywhere in a `//` comment so compound annotations on a single line
work.
Tests
- `src/channels/web/tests/status_event_isolation.rs` — 5 unit tests
covering both modes and the per-variant drop invariant.
- `src/channels/web/platform/sse.rs` — 2 quadrant tests for the
`subscribe_raw` filter (scoped/unscoped × matching/mismatched).
- `tests/thread_isolation_integration.rs` — 9 HTTP-level checks that
Bob cannot reach Alice's chat history (paginated and not), threads
list, engine v2 detail/steps/events, or Responses GET, plus an
unauthenticated-rejection guard.
- 8 new self-test cases for the `MULTITENANT` script check, including
the compound projection-exempt + multi-tenant-safe annotation case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(web): pin cross-tenant boundaries on jobs, files, routines
Audit of the protected route surface for the same bug shape #3390 fixed
(handler that takes a user-controlled id and reads without an ownership
predicate) found that the implementations were correct but four
boundaries had no integration test. Lock them in before they regress.
- Sandbox job persisted-events history (`/api/jobs/{id}/events`):
Bob → 404 on Alice's job; Alice → 200 on her own.
- Sandbox job workspace listing (`/api/jobs/{id}/files/list`):
Bob → 404 on Alice's job.
- Sandbox job file read (`/api/jobs/{id}/files/read`): Bob → 404 on
Alice's job; Alice → 200 on her own; Alice → 403/404 on
`?path=../outside.txt` (path-traversal pin against the
`canonicalize() + starts_with(base_canonical)` guard).
- Routine run history (`/api/routines/{id}/runs`): Bob → 404; Alice
→ 200 with at least one seeded run.
The OAuth-state and NEAR-nonce stores were also flagged in the audit
but neither is a real cross-tenant bug: both are pre-auth, single-use,
and the token IS the secret. Documenting here so a future audit
doesn't re-flag them.
Project-static (`/projects/{id}/...`) is left for a follow-up — it
relies on `ironclaw_base_dir()` which is a process-wide `LazyLock`,
making per-test override fragile in the integration runner.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): address PR #3390 review — forge-resistant metadata, OAuth toast routing
Addresses six review comments from gemini-code-assist, copilot, and
serrrfirat on PR #3390. False-positives and the perf nit on
`with_metadata` are explained in the reply thread, not changed in code.
- (HIGH, serrrfirat) `IncomingMessage::with_metadata` now ALWAYS sets
`metadata.user_id` from `self.user_id`, dropping any caller-supplied
value. A WASM channel emitting `{"user_id":"victim"}` via
`apply_emitted_metadata` can no longer reroute downstream
`ToolStarted` / `ToolResult` SSE events into another tenant's
stream. New unit tests pin the forgery-resistance invariant.
- (MEDIUM, copilot + serrrfirat) Slack relay OAuth callback now
broadcasts the completion toast to the resolved `oauth_user`
(the IronClaw user who initiated the flow) rather than
`state.owner_id`. In multi-tenant deployments those differ and the
previous routing delivered the toast to the wrong browser tab.
Extracted the lookup into `resolve_relay_oauth_user`; two unit
tests cover the secret-present and secret-missing cases.
- (MEDIUM, gemini) `dispatch_status_event` treats empty-string
`user_id` the same as `None` so producers that lost the field
along the way fail-closed instead of falling through to a global
broadcast in multi-tenant mode.
- (MEDIUM, gemini) `main.rs` sandbox JobEvent dispatch now reuses
`dispatch_status_event` instead of duplicating the drop / WARN /
broadcast policy. `dispatch_status_event` is bumped from
`pub(crate)` to `pub` so the binary crate can call it.
- (LOW, copilot) Pre-commit `MULTITENANT` self-tests gain three
cases (`state.sse.broadcast(`, `gw_state.sse.broadcast(`,
annotated receiver-prefixed) to lock the existing boundary regex
behaviour against future tightening.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(channels): preserve i64 metadata.user_id from Telegram in with_metadata
PR #3390's forge-resistance fix made `IncomingMessage::with_metadata`
*always* overwrite `metadata.user_id` with `self.user_id` as a String.
That broke the Telegram WASM channel: it persists Telegram's chat user
ID as `metadata.user_id: i64` and re-deserializes it into
`TelegramMessageMetadata { user_id: i64, ... }` in `on_respond` /
`on_status`. After the fix, `respond` blew up with
`invalid type: string "999", expected i64 at line 1 column 87`,
failing 3 Telegram integration tests in CI.
Narrow the carve-out: overwrite only when the existing `user_id` is a
String (or missing). Non-string values are channel-private and the
SSE routing layer reads via `as_str()` — non-strings already fail
closed in multi-tenant mode, so the forge threat (WASM emits
`{"user_id":"victim"}` as a string) is still mitigated, while
Telegram's i64 use case survives.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): redact WARN payload + harden dotdot traversal test (PR #3390)
Two follow-up fixes from the second pass of review on #3390:
1. `dispatch_status_event`'s WARN log used `?event`, which on
`AppEvent::Response` / `Thinking` / `ToolResult` carries
user-authored content into operator logs in a multi-tenant
deployment. Replace with `event_kind = event.event_type()`
(the wire-stable variant name) — enough to identify the
misbehaving producer without leaking tenant data. Picked up via
Copilot's review on `src/channels/web/mod.rs:666`.
2. `alice_job_file_read_rejects_dotdot_traversal` planted
`outside.txt` under `outer.path()` (the `start_server_with_db`
fixture's tempdir holding `test.db`) but probed
`?path=../outside.txt` relative to `alice_proj` — a separate
`tempfile::tempdir()` rooted at the OS temp directory. The two
paths were unrelated, so the test could pass even if `..`
traversal was permitted (probe just hit empty space). Build the
directory tree by hand instead: `parent/alice_proj/` with the
planted file at `parent/outside.txt`, so the probe deterministically
resolves to the planted bytes. Add a body-content assertion that
fails loudly if those bytes leak. Picked up via Copilot's review on
`tests/cross_tenant_resource_isolation.rs:355`.
Plus a `cargo fmt` fix for `src/channels/channel.rs:1202` that was
breaking the Formatting CI check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): address PR #3390 follow-ups — multi-tenant fallback WARN, exhaustive variant pin
- `resolve_relay_oauth_user`: take `multi_tenant_mode`; emit WARN when the
`relay:{ext}:oauth_user` secret is missing in multi-tenant mode so the
unrecoverable-initiator case surfaces in operator logs. Single-tenant
fallback stays silent (owner == only user).
- `dispatch_status_event`: doc note clarifying the function is `pub` only
for the sandbox JobEvent rx loop in `main.rs`; not part of a stable
public API.
- `_compile_time_appevent_variant_check`: exhaustive-match helper paired
with `unscoped_drop_holds_for_every_status_variant_in_multi_tenant`.
Adding a new `AppEvent` variant now fails the test build, prompting an
update to both the helper and the runtime leak-candidate list.
- `tests/thread_isolation_integration.rs`: honest scope note on the
engine-v2 thread tests — they pin handler shape (404/empty for
unknown id), not the cross-tenant ownership branch. Cross-tenant
engine-v2 coverage requires an `ENGINE_STATE` test fixture; tracked
as a follow-up in the comment block.
Tests: 8 unit (5 status_event_isolation + 3 resolve_relay_oauth_user)
and 9 thread_isolation_integration pass; clippy clean; pre-commit
safety scripts pass (regression suite 27 cases).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(web): unconditionally consume relay:{ext}:oauth_user secret in OAuth callback
The previous cleanup site lived inside the `if let Some(pairing_store)`
branch of the result block, which was unreachable on three failure
paths:
1. `pairing_store` is None (no identity pairing wired up)
2. The result block `?`-short-circuits before reaching the `if let`
(e.g. `set_setting` fails, `activate_stored_relay` fails, an inner
`relay_config()` / `list_connections()` errors)
3. The `if let` body itself errors before reaching the delete (e.g.
`list_connections` returns no matching team)
Leaving the secret behind lets a subsequent OAuth callback for the
same extension read a stale initiating user and misroute the
completion toast — Copilot review on PR #3390 (comment id 3211833864).
Move the delete to right after `resolve_relay_oauth_user` returns,
where it always runs once the value has been captured, regardless of
downstream failure mode. Updated the inner comment to document that
the secret is already gone by the time the pairing branch reads
`oauth_user`.
Regression test: `test_relay_oauth_callback_consumes_oauth_user_secret_on_failure_path`
seeds the secret, fires the callback against a fixture with no real
relay backend (so the result block deterministically errors), and
asserts the secret is gone afterward. Pre-fix this would have left
the secret behind on the failure path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): tighten Telegram pairing UX and OAuth-failure recovery (#3317, #3319, #3320) (#3381)
* fix(auth): tighten Telegram pairing UX and OAuth-failure recovery (#3317, #3319, #3320)
Three Bug Bash P1 issues from the same user journey: setup → use → fail.
The unifying root cause was per-channel auth tested in isolation; cross-channel
flows (Telegram → Gmail OAuth → resume) had no coverage and three small leaks
combined into a stuck conversation.
#3317 — Telegram pairing reply now names every IronClaw surface explicitly
(web settings, agent chat, terminal). The agent submission parser learns
`approve <channel> <code>`, dispatched through a new bridge handler that
mirrors `POST /api/pairing/{channel}/approve`.
#3319 — OAuth callback failures now log a category + correlation ID so a
user-reported "I saw 400" maps to one log line. Adds the
`OauthCallbackFailure` enum and `oauth_failure_correlation_id` helper.
#3320 — Two cleanup gaps fixed: (a) `/clear` now drains
`pending_oauth_flows` for the user (otherwise stale flows linger 5min and
mask new auth attempts); (b) OAuth provider-error and exchange-failure paths
now auto-cancel the engine pending auth gate via `clear_engine_pending_auth`,
so the conversation isn't blocked waiting for a resume that will never arrive.
Tests: 5 new submission-parser tests, 2 new bridge-handler tests, and one
new OAuth callback test verifying the pending-flow drain on provider error.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(auth): cross-channel pairing claim coverage + canary lane (#3317)
Adds the structural coverage that was missing when #3317 shipped:
- E2E (`tests/e2e/scenarios/test_telegram_pairing_chat_claim.py`):
three scenarios that drive the full Telegram pairing flow through
the gateway. Asserts the bot reply names every IronClaw surface
(web Settings, agent chat, terminal CLI), drives `approve telegram
CODE` through `/api/chat/send` and verifies the paired user
exchanges messages without re-prompting, and confirms invalid
codes get a clear rejection instead of an LLM-improvised reply.
- Rust integration (`tests/telegram_pairing_chat_claim_integration.rs`):
drives `Submission::PairingClaim` through a real `Agent` →
`bridge::handle_pairing_claim` → `PairingStore::approve` chain
using `TestRig` with engine v2 enabled. Covers the happy path
(mints a code, claims it via chat, asserts `Pairing approved`)
and the invalid-code rejection. The unit tests in `bridge/router`
cover only the no-extension-manager and invalid-channel branches —
this test exercises the wiring between submission parser, agent
loop dispatch, and bridge handler that #3317 specifically broke.
- Canary (`scripts/live_canary/auth_registry.py`): adds the two
user-visible scenarios to `AUTH_CHANNEL_TESTS` so the auth-channels
lane (scheduled every 6h) catches the regression class in CI
before any real user encounters it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: align pairing-claim and oauth-correlation comments with code (#3381)
Three Copilot review comments on PR #3381 flagged docstring/code drift in
already-merged PR #3317/#3319/#3320 changes. No behavior change — only
the doc strings move:
- `Submission::PairingClaim.code` and the inline `approve <channel> <code>`
parser comment claimed the user's casing was preserved, but the parser
builds the code from `lower` and the regression tests already lock in
the lowercased shape (`code == "abc12345"`). Update both comments to
describe the actual normalize-then-store contract.
- `oauth_failure_correlation_id` claimed the correlation appeared in the
user-facing error subtitle, but the failure path renders
`landing_html(label, false)` whose subtitle is fixed and never receives
the correlation. Mark the helper as logs-only and note that plumbing
the ID through the HTML is a follow-up.
[skip-regression-check] doc-only, behavior already covered by existing
pairing-claim parser tests in submission.rs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(telegram): bump channel registry to 0.2.11
The pairing-reply wording was updated in channels-src/telegram/, which
the version-check CI requires be matched by a registry version bump.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(telegram): fix import path in pairing chat claim e2e test
The scenarios/ folder is a Python package (has __init__.py), so a
flat `from test_telegram_e2e import …` fails with
ModuleNotFoundError during pytest collection. Switch to a relative
import that matches the package layout, and drop the unused
OWNER_USER_ID symbol while we're here.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): address Copilot review on /clear OAuth drain + correlation doc
Two follow-ups on PR #3381's Copilot pass:
1. `Agent::process_clear` (engine v1 path) now drains in-flight OAuth
flows for the clearing user, mirroring the engine-v2 cleanup added in
`bridge::router::clear_engine_conversation`. Without this, `/clear`
was a clean slate on v2 but v1 left ghost flows in
`extension_manager.pending_oauth_flows()` until the 5-minute
`OAUTH_FLOW_EXPIRY` ticked over — same regression class #3320 fixed
on v2.
2. `oauth_failure_correlation_id`'s docstring previously said "redacted
state fingerprint", but callers seed it with the raw `state` query
value (or `flow.extension_name` for post-resolution failures).
Updated the doc to describe the actual behaviour: an arbitrary seed
that is hashed before any hex output, with a pointer to
`redact_oauth_state_for_logs` for the log-safe fingerprint.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(e2e): make Telegram pairing chat-claim suite actually run
The scenario landed in PR #3317 was orphaned — never wired into any CI
lane and could not pass even when run by hand. Three structural
issues, all fixed here:
1. `install_telegram` now overlays the locally-built WASM (and matching
capabilities file) on top of the registry-downloaded artifact when
present. The pairing-reply wording lives inside the WASM binary, so
without this overlay the test was asserting source-tree text against
the previous release's bytes. The overlay is best-effort: when the
local WASM is absent (CI groups that don't build the channel), the
test that depends on it skips with a clear message and the canary
lane in `scripts/live_canary/auth_registry.py` still covers the
wording end-to-end against the deployed binary.
2. `Submission::PairingClaim` is handled out-of-band by the bridge
layer; the response is delivered via `WebChannel::respond` →
`AppEvent::Response` over SSE only — no `Turn` is persisted, so
polling `/api/chat/history` could never see it. Refactored
`test_chat_surface_approves_pairing_code` and
`test_chat_surface_rejects_invalid_pairing_code` onto a
`_send_and_collect_response` helper that opens the SSE stream first
(so the broadcast doesn't fan out to zero subscribers) and matches
on the `response` event for the test thread.
3. Wired the file into `e2e.yml`'s `extensions` group so the suite
actually runs on every PR.
Verified locally: all three scenarios pass, plus the existing 22
Telegram e2e tests still green with the install-overlay change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(bridge): bound and sanitize invalid-channel echo in pairing claim
Address Copilot review on PR #3381: `handle_pairing_claim`'s
invalid-channel branch was rendering the raw `channel` token (and the
underlying `IdentityError`, which itself echoes the offending input)
back to the user. Both routes are unbounded and could carry control
characters or markup since `channel` comes from chat input — a
hostile prompt could blow up the SSE / Telegram / TUI reply or smuggle
backticks/escape sequences through.
Cap the echo at 32 ASCII-alphanumeric (or `-`/`_`) characters and
replace the verbatim error with a fixed category description, so the
reply size and shape are bounded by what we render explicitly. Add a
regression test that drives a 200-char hostile blob (control chars +
backticks) through the handler and asserts the rendered reply stays
clean and short.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(bridge): include hyphens in invalid-channel error copy
Address Copilot review on PR #3381: the invalid-channel reply
listed "lowercase letters, digits, or underscores" as the valid
character set, but `ExtensionName::new` (and `web::features::pairing::
parse_channel`) intentionally accept hyphens too — they're folded to
underscores during canonicalization. A user typing `slack-relay` would
otherwise get an "invalid name" reply listing rules that contradict
the actual validator.
Updated the message to include hyphens with `telegram` and
`slack-relay` as concrete examples, and tightened the regression test
to assert against the user-controlled preview region between the
delimiter backticks rather than a global backtick count (which was
fragile to copy that includes example slugs in backticks).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(auth): address PR #3381 review on credential-scoped gate cleanup and Telegram surface promise
Three reviewer findings, one commit:
- OAuth provider-error and exchange-failure paths used
`clear_engine_pending_auth(user, None)`, which discards every
Authentication gate for the user. A failed Gmail callback could
silently wipe an unrelated Slack/MCP gate waiting on a different
thread. New `clear_engine_pending_auth_for_credential(user, credential)`
helper in bridge::router scopes cleanup to the failed flow.
Provider-error path tracks `removed_secret_name` alongside
`removed_user_id` so the scoped variant is callable.
- Expired-flow branch in the OAuth callback handler had two bugs: it
never cleared the engine pending auth gate (so the conversation sat
blocked forever, same #3320 class the provider-error fix addresses),
and the broader `clear_auth_mode` it called would re-discard via
the unscoped helper anyway. Now calls the credential-scoped helper
and the legacy-v1-only `clear_session_auth_mode_for_thread`.
- Telegram pairing reply advertised `approve telegram CODE` as
usable "in any IronClaw chat (TUI / web / Telegram)", but an
unpaired Telegram DM is intercepted by the allowlist gate before
the agent parser sees the command — the user would just get
another pairing reply. Reply now lists only the surfaces that
actually work (web / TUI / CLI) and a comment explains why.
Regression coverage:
- `clear_engine_pending_auth_for_credential_only_clears_matching_credential`
locks in helper scoping (Gmail/Slack two-gate scenario).
- `oauth_callback_expired_flow_clears_credential_scoped_engine_gate`
drives the full callback through axum oneshot with engine state
seeded; asserts the matching gate clears and the unrelated gate
survives.
- E2E `test_telegram_dm_approve_command_is_intercepted_by_allowlist_gate`
exercises the Telegram webhook path (not /api/chat/send) to lock in
the channel-layer interception, wired into the auth canary lane.
- Existing E2E pairing-reply test gains an assertion that
"TUI / web / Telegram" is *not* in the reply.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(oauth): mirror failure-cleanup contract on provider-error and reconcile stale comment
Copilot review on PR #3381 caught two real issues in the credential-scoped cleanup
landed in d45cf1bd8:
- Provider-error branch (`?error=access_denied`) returned the error page
without broadcasting `OnboardingState::Failed` or clearing the legacy v1
session `pending_auth`. The exchange-failure and expiry branches do both.
Net effect: the auth card stayed spinning and the next user message was
intercepted as a token. Now mirrors the other failure paths — keep the
full `flow`, emit Failed SSE, clear v1 session, clear credential-scoped
engine gate, then return the error page.
- Post-exchange comment said "failed callbacks should leave the gate
visible for retry" — that was the pre-#3320 contract. Rewrote it to
describe the new shape: each failure mode clears its own gate at the
failure site; this section only handles legacy-v1 session cleanup that
runs regardless of outcome. Also explains why we use
`clear_session_auth_mode_for_thread` here instead of `clear_auth_mode`
(the latter would re-clear the engine gate on the *success* path and
break the `ExternalCallback` resume).
Regression: `test_oauth_callback_provider_error_broadcasts_onboarding_failed`
in the oauth tests module — drives an `?error=access_denied` callback with
a flow whose `sse_manager` is attached, asserts the receiver gets
`OnboardingState::Failed` with the provider's `error_description` as the
message body.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(common): describe paths and platform helpers in crate description (#3498)
Align the package `description` and the lib.rs crate-level doc with
the modules now exposed from `ironclaw_common` (paths, platform,
env_helpers, attachment), which #3387 lifted out of `src/`. The
previous wording predates that extraction and only mentioned "types
and utilities".
This is also the release-plumbing trigger for v0.28.1: release-plz
proposes a leaf bump on source-path changes, and once `ironclaw_common`
crosses to a new patch, the root `ironclaw` bump can be added on top
of the release-plz branch (same approach as commit 070cbede1 for
v0.28.0). See PR #3372 for the equivalent v0.28.0 trigger.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore: release
* chore(release): bump ironclaw to 0.28.1
cargo-semver-checks did not detect API-breaking changes in the
ironclaw_common 0.4.1 -> 0.4.2 leaf bump, so release-plz did not
cascade a bump into the root ironclaw package. Add the root version
bump and CHANGELOG entry manually so this release-plz PR produces
an ironclaw-v0.28.1 tag and triggers cargo-dist.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(llm): hide provider-specific auth, model fetch, and embeddings config behind facades (#3416)
* refactor(llm): hide provider-specific auth, model fetch, and embeddings config behind facades
External callers were reaching into provider-specific modules of
`ironclaw_llm` (`gemini_oauth::CredentialManager`,
`github_copilot_auth::*`, `OpenAiCodexSessionManager`,
`codex_auth::*`, `BedrockConfig`, etc.). Closes those leaks behind a
small set of verb-based public surfaces while keeping per-provider
behaviour inside the LLM crate.
Changes:
1. Extract `oauth_helpers.rs` into a new `ironclaw_oauth` crate. The
loopback OAuth callback listener (port 9876, landing pages,
`OAUTH_CALLBACK_HOST` rules) is shared by every IronClaw OAuth flow
(NEAR AI session login, WASM tool auth, MCP) and never depended on
`ironclaw_llm`. `src/auth/oauth.rs` now `pub use ironclaw_oauth::*`
directly. `ironclaw_llm` no longer depends on `ironclaw_oauth` —
the helper had zero internal callers.
2. Add `ironclaw_llm::auth` facade (`start_login`, `validate_token`,
`default_headers`, `load_persisted_credentials`,
`default_credentials_path`) with backend-agnostic types
(`AuthPrompt`, `LoginRequest`, `AuthOutcome`, `PersistedCredentials`,
`OpenAiCodexLoginOptions`, `AuthBackend`, `CredentialSource`).
Privatize `gemini_oauth`, `github_copilot_auth`, `openai_codex_session`,
`codex_auth` (`pub(crate) mod`). Migrate the wizard, the
`ironclaw login --openai-codex` CLI subcommand, and the LLM config
loader to the facade. Wizard introduces a single `WizardAuthPrompt`
that handles device-code prompts + browser launch for all backends.
3. Add `ironclaw_llm::models::fetch_models_for(provider_id, &opts)`
facade. Privatize `fetch_anthropic_models`, `fetch_openai_models`,
`fetch_ollama_models`, `fetch_openai_compatible_models`,
`is_openai_chat_model`, `openai_model_priority`, `sort_openai_models`.
Wizard's per-backend match collapses to one call. Move classifier
unit tests into `crates/ironclaw_llm/src/models.rs`; rewrite the
two wizard fallback tests through the public API.
4. Decouple embeddings from `ironclaw_llm::BedrockConfig`. New
`crate::workspace::BedrockEmbeddingSetup { region, profile }` carries
only what `BedrockEmbeddings` actually needs. `EmbeddingsConfig::create_provider`
and `BedrockEmbeddings::new` take the new type; callers translate from
`LlmConfig.bedrock` at the boundary (`src/app.rs`, `src/cli/mod.rs`).
5. Add `ironclaw_llm::testing::nearai_test_config(model)` helper for
tests that need a minimal `LlmConfig` shape (no retries, no caching,
NEAR AI backend). Replaces two duplicated 30-line struct literals
in the gateway settings hot-reload tests.
Boundary cleanup is behaviour-preserving: 4,932 main-binary unit tests,
729 ironclaw_llm unit tests, 4 ironclaw_oauth tests, 3 architecture
boundary tests all pass; `cargo clippy --all --benches --tests
--examples --all-features` is clean.
Three `pub` methods on `gemini_oauth::CredentialManager` /
`GeminiOauthProvider` (`get_valid_access_token`, `last_response_meta`,
`count_tokens`) and the `GeminiResponseMeta` struct are now reachable
only crate-internally and have no callers; marked `#[allow(dead_code)]`
with a comment rather than deleted to keep this PR purely a boundary
move (delete in a follow-up if no caller emerges).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(llm): promote dedicated backends into the registry; absorb config validation, defaults, and per-provider overrides into ironclaw_llm
Continues the LLM boundary cleanup from 0addf3ac2. After that commit
provider-specific auth, model fetch, and embeddings config lived behind
facades inside `ironclaw_llm`, but four backend-specific knowledge
sources still leaked out:
1. Validation rules and default values for the dedicated-config
backends (Bedrock cross-region prefixes, OpenAI Codex endpoints
and client_id, Gemini OAuth credentials path defaults) lived
inline in `src/config/llm.rs::resolve`.
2. The dispatcher in `create_llm_provider` matched on backend strings
("nearai", "bedrock", ...) instead of a typed protocol value. The
same booleans (`is_nearai`, `is_bedrock`, `is_gemini_oauth`,
`is_openai_codex`) recurred across `src/config/llm.rs`,
`src/app.rs`, `src/cli/models.rs`, and the wizard.
3. The setup wizard had per-backend specialization in
`step_inference_provider` and `run_provider_setup` (manual menu
pushes for nearai/bedrock/codex/gemini_oauth, four dedicated
`setup_*` entry points dispatched on string compares).
4. `Settings` carried named `bedrock_region`, `bedrock_cross_region`,
`bedrock_profile` columns even though no other dedicated backend
had named columns and adding a new one would mean schema churn.
Layers A-D address each in turn:
* Layer A — `BedrockConfig::build`, `OpenAiCodexConfig::build`, and
`GeminiOauthConfig::build` own validation + defaults inside the
crate. `LlmConfigError` (`MissingRequired` / `InvalidValue`) carries
the failures across the boundary, with a `From` impl into the
binary's `ConfigError`. `src/config/llm.rs` calls the builders;
named-string defaults are gone from the binary. The orphaned
`tests/gemini_oauth_regression.rs` husk is deleted.
* Layer B — `ProviderProtocol` gains four new variants
(`Bedrock`, `OpenAiCodex`, `GeminiOauth`, `NearAi`) plus a
`has_dedicated_config()` predicate. The four dedicated-config
backends (with all aliases) become first-class registry entries in
`providers.json`, so `is_known()` / `model_env_var()` / the wizard /
the gateway handler iterate the registry uniformly. The
`is_nearai`/`is_bedrock`/`is_gemini_oauth`/`is_openai_codex` boolean
spaghetti collapses to protocol comparisons. `OpenAiCodex` and
`NearAi` carry explicit `#[serde(rename = "openai_codex" / "nearai",
alias = ...)]` so the wire-stable adapter strings the gateway and
frontend already use keep working. `LlmConfig::active_model_name()`
is now consumed by `cli/doctor.rs` instead of an inlined partial
dispatch.
* Layer C — `SetupHint` gains four credential-collection variants
(`AwsCredentials`, `OAuthDeviceCode`, `FileBasedCredentials`,
`SessionToken`). The wizard's `step_inference_provider` builds its
menu from a single `registry.selectable()` iteration with generic
env-detection (declared `api_key_env`, plus an Anthropic-specific
OAuth fallback). `run_provider_setup` dispatches on the SetupHint
variant; the remaining `def.id == "..."` checks live inside the
`ApiKey` arm only because Anthropic and GitHub Copilot present a
hybrid choice (API key OR OAuth) the simple `ApiKey` hint doesn't
capture. The synthetic bedrock + nearai entries in
`handlers/llm.rs::build_llm_providers` are deleted; a single
registry-driven loop covers both. ADAPTER_LABELS in
`static/js/surfaces/config.js` gains entries for the new protocols.
* Layer D — `LlmBuiltinOverride` gains a generic
`extras: HashMap<String, String>` bag with `extra(key)` /
`set_extra(key, value)` accessors. The bedrock resolver and wizard
read/write through this bag; `Settings::migrate_legacy_provider_fields()`
drains the named `bedrock_*` columns into `extras` on
`Settings::load_from()` so existing `settings.json` files migrate
losslessly. The named columns are kept (deprecated, marked with
`#[serde(skip_serializing_if = "Option::is_none")]`) for one
release; tracked for deletion in #3443.
`strip_admin_only_llm_keys` and `llm_setting_requires_reload` now
match dotted-path subkeys under `llm_builtin_overrides.*` so a
write to e.g. `llm_builtin_overrides.bedrock.extras.region`
triggers the right gating + chain reload.
Boundary cleanup is behaviour-preserving: 4,933 main-binary unit tests,
739 ironclaw_llm unit tests pass; `cargo clippy --all --benches --tests
--examples --all-features` is clean. New regression tests:
`crates/ironclaw_llm/src/config.rs` (6 builder tests),
`crates/ironclaw_llm/src/registry.rs::dedicated_config_backends_are_in_registry_and_selectable`,
and `src/setup/wizard.rs::legacy_bedrock_fields_migrate_into_extras_on_load`.
Three follow-ups tracked in #3443: delete the deprecated `bedrock_*`
named columns, move `BedrockEmbeddings` out of `src/workspace/` into
the LLM crate (last cargo-feature leak), and drive
`LlmConfig::active_model_name()` off `ProviderProtocol` instead of
backend strings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test: add bug-bash regression-snapshot harness
Bug-bash fixtures pin specific open bugs to a deterministic snapshot.
When a bug is fixed, the snapshot diff is the reviewable proof; when
someone reintroduces the bug, the snapshot drifts and CI blocks the
merge.
This commit lands the harness plus the first recorded fixture for
issue #2541 (agent must call a tool, not answer from training data):
tests/e2e_bug_bash_snapshots.rs
`snapshot_summarization_uses_tools` replays the fixture, captures
`ReplayOutcome`, and asserts the YAML snapshot. Gated on
`feature = "libsql"`, same as other replay-snapshot tests.
tests/fixtures/llm_traces/bug_bash/summarization_uses_tools.json
Two-step recorded LLM trace (tool_call -> text) keyed off the
user prompt via `request_hint.last_user_message_contains`.
tests/fixtures/llm_traces/bug_bash/README.md
Coverage map for #2540-#2546 (one recorded, six TODO) plus the
`IRONCLAW_RECORD_TRACE` recording workflow.
tests/snapshots/replay__bug_bash_summarization_uses_tools.snap
Insta YAML snapshot pinning `tool_calls: [echo]`, 2 LLM calls,
and the event-kind histogram. Drift = regression.
* fix(settings): preserve pre-existing extras during legacy bedrock migration
`migrate_legacy_provider_fields` claimed to be idempotent and to drain
named `bedrock_*` columns into `llm_builtin_overrides["bedrock"].extras`
once on load. The previous implementation drained correctly but used
`HashMap::insert` unconditionally, which means a settings file
carrying BOTH a legacy `bedrock_region` column AND an already-populated
`extras["region"]` (manual hand-edit, or a future writer emitting both
shapes during a transition) would silently downgrade to the legacy
value.
Guard each `set_extra` call with `entry.extra(key).is_none()` so the
new-shape value always wins. Clarify the docstring to state this
explicitly.
Add three regression tests in `settings::tests`:
- `legacy_bedrock_migration_round_trips_through_save` — legacy JSON ->
load_from -> serialize -> reload, asserts the deprecated columns are
not re-emitted and extras survive the round trip.
- `legacy_bedrock_migration_preserves_existing_extras` — file with both
shapes; asserts the pre-existing extras value is kept and absent
extras are still backfilled from legacy fields.
- `legacy_bedrock_migration_is_idempotent_in_memory` — calling the
migration twice on the same Settings is a no-op (compares serialized
shape, since LlmBuiltinOverride does not derive PartialEq).
* fix(pr-3416): address PR review — migration on DB/TOML, admin-key gate, codex login, credential_kind/has_credentials
Addresses comments from gemini-code-assist, Copilot, and serrrfirat on PR #3416.
## Bugs
**Legacy bedrock fields not migrated on DB/TOML loads** (serrrfirat, High).
`Settings::load_from` (JSON) ran `migrate_legacy_provider_fields`, but
`from_db_map` and `load_toml` did not. Existing operators with
`bedrock_*` settings persisted in the DB or `config.toml` would silently
lose their AWS region/profile/cross-region after upgrade because the
resolver now reads only from `llm_builtin_overrides["bedrock"].extras`.
Both loaders now call the migration; added round-trip tests for each.
**Admin-only key write gate had narrower matching than read gate**
(Copilot, High). `strip_admin_only_llm_keys` matches both exact keys
and dotted subpaths under admin-only roots; `is_admin_only_setting_key`
in the web settings handler used `.contains(&key)` only. A non-admin
could write `llm_builtin_overrides.bedrock.extras.region` directly,
bypassing the gate. Promoted `is_admin_only_llm_key` to `pub(crate)`,
made the web write-side gate call it, added regression tests covering
dotted subpaths.
**`ironclaw login --openai-codex` dropped TOML/DB config** (Copilot,
High). The pre-refactor code resolved `Config::from_env` and used
`config.llm.openai_codex` so endpoint / client-id / session-path
overrides committed via TOML or DB stuck. The post-refactor code only
read env vars via `OpenAiCodexLoginOptions::from_env`. Added
`OpenAiCodexLoginOptions::from_resolved_config(&OpenAiCodexConfig)`;
the login command now prefers the resolved config when present and
falls back to env-only when `Config::from_env` itself fails (fresh
machine, no DB).
**Dedicated-auth backends marked configured without credentials**
(serrrfirat, Medium). `nearai` / `gemini_oauth` / `openai_codex` ship
`api_key_required: false` because they don't authenticate via a bearer
API key. The frontend `isProviderConfigured` treated that as "no
credentials needed" and rendered the Use button on a fresh install,
where clicking could trigger an interactive device-code OAuth from
inside a settings request.
Added `credential_kind` (wire-stable snake_case discriminator matching
`SetupHint::kind()`, e.g. `session_token`, `o_auth_device_code`,
`file_based_credentials`, `aws_credentials`) and `has_credentials`
(backend-authoritative; checks AWS env vars for Bedrock, codex session
file existence, file-based credential path expansion + existence) to
the web LLM providers payload. Frontend `isProviderConfigured` /
`providerMissingReason` now gate non-api-key kinds on `has_credentials`.
## Nits
**`fetch_models_for` doc overclaimed "Always returns something"**
(Copilot). The generic openai-compatible branch returns `vec![]` when
`base_url` is empty. Updated the docstring to call this out so callers
know to handle the empty case.
**`AuthError::Other` used for "validation not applicable"** (Gemini
bot). Added a dedicated `AuthError::TokenValidationNotSupported { backend }`
variant; `validate_token` now returns it for Gemini / OpenAiCodex
instead of stringly-formatted `Other`.
**Bug-bash regression-harness URLs pointed at `near/ironclaw`**
(Copilot, x2). The canonical tracker is `nearai/ironclaw`. Rewrote
all seven URLs in `tests/fixtures/llm_traces/bug_bash/README.md` and
the one in `tests/e2e_bug_bash_snapshots.rs`.
## Declined
The Gemini bot's MalformedConfig suggestion at
`crates/ironclaw_llm/src/models.rs:46` was not adopted: the call site is
the openai-compatible model-listing path, not a security-sensitive
request. The fetcher early-returns `vec![]` on empty `base_url` — no
URL parsing happens — and the docstring tightening above covers the
observable surprise. Promoting it to a typed error would change the
public-facing `fetch_models_for` signature for no behavioural gain.
## Tests
- `cargo fmt --check` clean
- `cargo clippy --all --benches --tests --examples --all-features` zero warnings
- `cargo test --lib` 4,941 / 4,941 pass
- `cargo test --features libsql --test e2e_bug_bash_snapshots` 1 / 1 pass
- New regression tests:
- `settings::tests::legacy_bedrock_fields_migrate_into_extras_on_db_load`
- `settings::tests::legacy_bedrock_fields_migrate_into_extras_on_toml_load`
- `channels::web::features::settings::tests::test_admin_only_setting_keys_cover_dotted_subpaths`
- `channels::web::handlers::llm::tests::test_llm_providers_expose_credential_kind_and_has_credentials`
- `channels::web::handlers::llm::tests::test_nearai_has_credentials_true_when_session_token_loaded`
* fix(pr-3416): tighten Bedrock/Codex has_credentials probes; collapse set_extra into one .into()
- `backend_has_credentials` for AWS now requires `AWS_PROFILE` OR
(`AWS_ACCESS_KEY_ID` AND `AWS_SECRET_ACCESS_KEY`). The lone
`AWS_ACCESS_KEY_ID` / `AWS_SESSION_TOKEN` arms previously flipped
has_credentials true even though the AWS SDK can't sign without the
secret key, so the UI was rendering Bedrock as configured on hosts
that would fail at first call.
- `backend_has_credentials` for OpenAI Codex now honours
`OPENAI_CODEX_SESSION_PATH` via `read_env` before falling back to
the default session path under `~/.ironclaw/`. Users with a custom
session location were seeing "not configured" despite a valid login.
- New regression tests `test_bedrock_partial_aws_env_reports_not_configured`
and `test_openai_codex_honours_session_path_env` drive the
`build_llm_providers` call site (not just the helper) so both gaps
stay closed.
- Tidied `LlmBuiltinOverride::set_extra` to convert the key once and
reuse it across the remove/insert branches; behaviour identical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(providers): default nearai model to "auto"
Switch the nearai registry entry's `default_model` from
`claude-sonnet-4-5` to `auto`, NEAR AI's server-side routing alias.
New installs without `NEARAI_MODEL` set now get auto-routed instead
of being pinned to a specific Anthropic model.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Make Skills E2E lifecycle deterministic (#3309)
* test(e2e): make skills lifecycle deterministic
* test(e2e): address skills review comments (#3309)
* test(e2e): unxfail two auth-matrix tests now that contracts match (#3589)
Both xfails in tests/e2e/scenarios/test_v2_auth_oauth_matrix.py are
stale and pass against current code:
- test_wasm_tool_first_chat_auth_attempt_emits_auth_url
Marked xfail in #3235 because the engine-v2 callable-only contract
(#2868) stopped emitting an auth gate on direct LLM-driven tool
calls. PR #3157 (auth-preflight + inline-await) restored the
behavior the test asserts: when the LLM emits a direct call to a
not-yet-authed extension, the bridge raises an Authentication gate
with auth_url populated (src/bridge/effect_adapter.rs:1356-1392).
Marker removed; test passes.
- test_settings_first_custom_mcp_auth_then_chat_runs
The xfail reason claimed post-auth tool-output propagation was
broken. Real cause: engine-v2 gates the first MCP tool call on
`approval` and the browser fixture has no auto-approve UI, so the
chat sat in pending_gate forever. Same shape as the bugs fixed in
#3235 for test_wasm_tool_oauth_refresh_on_demand and
test_mcp_same_server_multi_user_via_browser. Inserted
_wait_for_tool_call between _send_chat and _wait_for_response_contains
to drive approval through the API; test passes.
Verified locally: both tests pass back-to-back in 27s on a fresh
auth_matrix_server.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(extensions): restore chat-driven tool_install + fix double-invoke + auto-approve footgun (#3533) (#3559)
* 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>
* chore: release
* feat(engine): IRONCLAW_DISABLE_CODEACT flag to disable v2 CodeAct (#3665)
* add flag to disable codeact on engine v2
* fmt
* fix(engine): keep compact actions reachable when CodeAct is disabled
With IRONCLAW_DISABLE_CODEACT=true the structured-tools prompt told
the model to use the provider's tool_calls interface for every action,
but the bridge filtered the provider tool list down to
emits_full_schema_tool(). Most tools default to CompactToolInfo
(mission_create, gmail_send, notion_search, ...), so they appeared in
the prompt as "available" while being absent from the provider tool
list — i.e. unreachable. Addresses serrrfirat's review on PR #3665.
Fix coordinates both halves of the surface:
- src/bridge/llm_adapter.rs: in disabled-CodeAct mode, drop the
emits_full_schema_tool() filter and emit every action into the
provider tool list with its full schema.
- crates/ironclaw_engine/src/executor/prompt.rs: in disabled-CodeAct
mode, skip the "## Enabled Tools" section. The compact-form listing
with the tool_info(detail="schema") instruction is meaningless when
the provider already sends full schemas, and would just duplicate
the surface. "## Activatable Integrations" stays — the model still
needs to know what tool_install can target.
Test seam: build_codeact_system_prompt_inner now takes disable_codeact
as an explicit parameter, called once at the public entry points. This
lets prompt tests exercise both branches without process-global env
mutation.
Tests:
- executor::prompt::tests::disabled_codeact_omits_enabled_tools_section_and_keeps_activatable
- bridge::llm_adapter::tests::complete_emits_compact_actions_when_codeact_disabled
- existing complete_with_tools_only_emits_full_schema_provider_tools
now serialized via lock_env() so env mutation in the new test
can't leak across parallel runs.
cargo test -p ironclaw_engine --lib: 527 passed
cargo test --lib bridge::: 469 passed
cargo clippy -p ironclaw_engine --all-targets -- -D warnings: clean
cargo clippy --lib --tests -- -D warnings: clean
cargo fmt --check: clean
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Emil Bogomolov <emil.bogomolov@near.ai>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Fix markdown_to_mrkdwn to avoid converting emphasis inside generated <… (#3532)
* agent: Fix markdown_to_mrkdwn to avoid converting emphasis inside g…
* agent: Fix rustfmt/clippy CI failure by removing extra blank line b…
* agent: slack: fix markdown_to_mrkdwn replacement order to satisfy p…
* slack: protect generated links and sanitize sentinels in markdown_to_mrkdwn
Two issues raised on PR #3532 review:
1. Emphasis inside generated `<url|text>` was still rewritten because the
global `**`/`~~` → `*`/`~` substitution ran after link materialization.
Push the generated link span into the same protected arena used for
Slack-native `<...>` constructs so subsequent global replacements can't
reach inside it. Matches the PR's stated goal.
2. Untrusted input containing the private-use sentinel chars
(U+E000 / U+E001) could forge a protected-span reference and pull in
another span's content. Strip those chars from input up front.
Adds regression tests for both. Bumps registry/channels/slack.json
0.3.2 → 0.3.3 to satisfy the channel-source version-bump check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* slack: escape link labels, drop pipe/gt URLs, expand nested sentinels
Addresses two follow-up review concerns on PR #3532:
Copilot review: `<url|text>` was built by string concatenation, so
`|` or `>` inside the URL would corrupt the entity, and `<` / `>`
inside the label would open/close a Slack span and break the link.
The label now escapes `<` → `<` and `>` → `>` (Slack's documented
literal-character form); a URL containing `<`, `>`, or `|` falls back
to leaving the original markdown form intact (those chars are not
valid URL characters per RFC 3986 anyway).
Latent nested-sentinel bug introduced by the previous fix: a markdown
link whose label contained a Slack-native `<...>` span (e.g.
`[<@U1> hi](url)`) ended up with the inner sentinel buried inside the
arena entry for the outer link span. The final restore pass advances
past the outer sentinel without rescanning what it just emitted, so
the raw U+E000/U+E001 characters would leak into the output. URL and
label are now pre-expanded before the link span is pushed.
While here, factor the duplicated restore loop into
`expand_protected_spans`, reused by both the pre-link expansion and
the final restore, and lift the sentinel constants to file scope.
Adds three regression tests covering label-bracket escaping, the URL
pipe/gt fallback, and the nested-span case. Bumps
registry/channels/slack.json 0.3.3 → 0.3.4.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(gateway): add logs download button (#3588)
* feat(web): support externally-provided tools in Responses API (#3122)
* feat(web): support externally-provided tools in Responses API
Lets callers of `/v1/responses` (and `/api/v1/responses`) declare their
own `function`-typed tools and feed back results via
`function_call_output` items, matching the OpenAI Responses wire shape.
Since IronClaw's engine has no per-request tool surface, integration
happens at the prompt level: the catalog is rendered as
`<external-tools>` in the user message and the agent signals a call by
ending its response with a fenced ```` ```tool_call ```` block. When
that fence is recognised, the reply is split into a leading `Message`
plus a `function_call` `ResponseOutputItem`.
Validation rejects unsupported tool types (`web_search`, `file_search`,
`code_interpreter`) and tools missing `name` with 400, with two new
integration tests covering both paths.
* refactor(responses-api): switch external tools to engine v2 native path
Replace the prompt-level fence protocol from PR #3122 with engine v2
native tool calls: caller-supplied `tools[]` are surfaced as real
LLM-callable actions, the engine pauses with `ResumeKind::External`
when one is invoked, and the bridge router projects the pause to a
new `AppEvent::ExternalToolCall` carrying the OpenAI-shaped
`function_call` wire fields.
The integration is small because v2 already has the right primitives:
- `ResumeKind::External { callback_id }` and
`GateResolution::ExternalCallback { payload }` already existed for
OAuth-style callbacks.
- `agent_loop.rs:1480` already routes Responses API messages to
`handle_with_engine` when `ENGINE_V2=true`, so no v2 migration of
the endpoint itself is needed.
- `EffectBridgeAdapter::execute_action` is the single chokepoint
where caller tools can be detected before they reach the dispatch
pipeline.
Changes:
- New `src/bridge/external_tools.rs` (`ExternalToolCatalog`) — per-thread
registry of caller-supplied `ActionDef`s, plus the `ext_tool:`
callback-id helpers used to disambiguate external-tool pauses from
OAuth/pairing pauses (which also use `ResumeKind::External`).
- `EffectBridgeAdapter` consults the catalog: any name in it is
short-circuited to a `GatePaused { resume_kind: External {
callback_id: ext_tool:<call_id> } }` before any registry dispatch,
and `available_action_inventory` merges the catalog into the
LLM-visible action surface (internal beats external on collision).
- `Submission::ExternalCallback` gains an optional `payload` field;
`bridge::handle_external_callback` plumbs it into
`GateResolution::ExternalCallback { payload }`. Fallback predicate
`gate_resume_is_external` lets non-auth External pauses (i.e.
caller-tool resumes) resolve through the same handler.
- New `AppEvent::ExternalToolCall` projected by `notify_pending_gate`
when a paused gate carries an `ext_tool:` callback id; OAuth/
pairing flows keep flowing through the existing `GateRequired`
channel.
- `responses_api.rs` is gutted of the prompt rendering and fence
parsing (`render_external_tools_preamble`, `extract_trailing_tool_call`,
`parse_external_tool_call`, `ParsedToolCall`, `external_tool_names`
accumulator field, and the `TOOL_CALL_FENCE` constants). The handler
now: rejects `tools[]` when `ENGINE_V2=false`, registers caller
tools in the catalog under the resolved thread id, detects resume
requests (`previous_response_id` + `function_call_output` items in
`input`) and submits them as `Submission::ExternalCallback` with
the outputs as the resolution payload, and surfaces
`AppEvent::ExternalToolCall` as a `function_call` `ResponseOutputItem`
in both streaming (`output_item.added`+`done`) and non-streaming.
- All existing OAuth/pairing `ExternalCallback` constructors updated
to pass `payload: None` (no behaviour change).
- Fence-protocol unit tests removed; replaced with coverage for the
new `responses_tools_to_action_defs` converter and the accumulator's
`ExternalToolCall` arm.
Existing 9 integration tests in `tests/responses_api_path_prefix.rs`
still pass.
Note for reviewers:
- The accumulator-side text response no longer tries to split the
reply on a fenced `tool_call` block. The wire shape that callers
receive for caller-tool invocations is purely event-driven now.
- Internal vs external collision is handled silently by the dedup in
`available_action_inventory` (internal wins). A request-time
rejection for shadowing names is a follow-up — the current behavior
is safe (the LLM only sees the internal version) but could surprise
a caller who expects their tool to run.
* test(responses-api): cover ENGINE_V2-off and resume-without-pending-gate
Two new integration tests for behaviours added by the engine-native
external-tool refactor:
- `external_tools_rejected_when_engine_v2_disabled`: a request with
caller-supplied `tools[]` while `ENGINE_V2` is off must 400 with a
message naming the flag, not silently fall through.
- `resume_without_pending_gate_returns_400`: a request with
`function_call_output` items and a `previous_response_id` that
doesn't correspond to a live external-tool gate must 400, not start
a fresh turn against the (unrelated) thread.
Both tests drive the full router (`start_test_server` + bearer auth)
per `.claude/rules/testing.md` "Test Through the Caller".
* test(responses-api): integration tests + drop unsafe env mutation
Three groups of changes:
1. **Drop unsafe env-var mutation in tests.** `responses_api.rs` no
longer reads `ENGINE_V2` directly: it keys off the presence of the
live `ExternalToolCatalog` (initialized by `init_engine`) as the
"engine v2 is up" signal. The path-prefix test that exercises the
no-engine branch no longer needs `unsafe { std::env::remove_var }`
— the absence of `init_engine` in `TestGatewayBuilder` is what
makes the catalog absent, which is what makes the request reject.
2.…
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
… + auto-approve footgun (nearai#3533) (nearai#3559) * fix(extensions): restore chat-driven tool_install + fix double-invoke + auto-approve footgun (nearai#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** (nearai#2868). The unified `tool_activate` it was meant to be subsumed by was later removed in nearai#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-nearai#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 nearai#3559 review (permission bypass, lease accounting, hidden search filter) Five fixes from the nearai#3559 review (4× Copilot doc nits + 3× serrrfirat security/correctness findings): 1. **Permission bypass (High).** Pre-nearai#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 nearai#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 nearai#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(nearai#3559): caller-level regression coverage for review findings 1 & 2 Two follow-up regression tests for the nearai#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes issue #3533 — "connect my telegram" gave the user two options instead of installing, and even when the agent recovered to
tool_installthe tool ran twice and required pressing "Always" to clear the pending gate.tool_install(hidden in engine-v2: make available_actions callable-only for blocked providers #2868, orphaned when its replacementtool_activatewas removed in Mission auto-resume after auth/approval gate resolution (#3133 follow-up) #3166).hiddenflag onExtensionManifest, marked ontelegram_mtproto.tool_installinvocation by carryingresume_outputthroughInlineGateso the inline-await retry short-circuits with the cached output instead of re-executing.ExternalCallbackre-entry when the inline path already woke a parked waiter (eliminates the "thread already running" race + phantom second dispatch).pending_gateswhen the inline path delivers Approved.ToolPermissionSnapshot::resolve_permissionnow treats DB values matching the seeded default as implicit, soAGENT_AUTO_APPROVE_TOOLS=trueactually bypasses seededAskEachTimedefaults as documented.Test plan
cargo fmtcargo clippy --all --benches --tests --examples --all-features— zero warningscargo test --lib— 4980/4980 pass (host)cargo test -p ironclaw_engine --lib— 525/525 pass (engine)bridge::tool_permissions::tests::seeded_default_matching_db_value_is_implicitbridge::tool_permissions::tests::user_override_diverging_from_seeded_default_stays_explicitbridge::action_projector::tests::tool_install_is_callable_by_agentregistry::manifest::tests::test_parse_hidden_manifest_propagates_to_registry_entryextensions::manager::tests::list_filters_hidden_registry_entries_from_available_settests/e2e/scenarios/test_v2_auth_oauth_matrix.py::test_chat_first_gmail_installs_prompts_and_retries— was@pytest.mark.xfail, now passes (xfail removed)test_chat_install_approval_then_auth_card— explicit-approval sibling, single "Approve" click, no "Always" workaroundtest_settings_first_gmail_auth_then_chat_runs— still greenauth-fullcanary lane viascripts/live_canary/auth_registry.pyWhat's changed (high-level)
src/bridge/action_projector.rshidden_from_model_callable_surfacegate;tool_installis callable from the agent againsrc/bridge/effect_adapter.rsauth_gate_from_extension_resultpasses the action's already-computed output asresume_outputsrc/bridge/router.rsresolve_inline_gates_for_credentialdiscards matching Authentication rows frompending_gatessrc/bridge/tool_permissions.rssrc/channels/web/features/oauth/mod.rsExternalCallbackre-entry when inline-await already woke a waitercrates/ironclaw_engine/src/executor/{scripting,structured,orchestrator}.rsInlineGategainsresume_output; retry returns cached output instead of re-executingcrates/ironclaw_engine/src/executor/prompt.rstool_install(name=...)directly for activatable integrationssrc/registry/manifest.rs,src/extensions/*hiddenflag on manifests + registry entries; filter from default-discoveryregistry/tools/telegram_mtproto.json"hidden": truetests/e2e/scenarios/test_v2_auth_oauth_matrix.py,tests/e2e/mock_llm.pyscripts/live_canary/auth_registry.pyAUTH_FULL_TESTS🤖 Generated with Claude Code