Unify gateway onboarding, auth gates, and pairing flows - #2515
Merged
Merged
Conversation
…ing state The Telegram channel setup flow via the gateway was broken end-to-end. Four interconnected bugs prevented pairing/ownership from completing: 1. pairing_approve_handler only wrote to channel_identities DB — the running WasmChannel's owner_actor_id was never updated, so the owner was never recognized and broadcast metadata was never stored. 2. refresh_active_channel() re-ran on_start() but never called ensure_polling(), leaving polling in a stale state on repeated tool_activate calls and causing Telegram 409 conflicts. 3. activate_wasm_channel() had a TOCTOU race on active_channel_names that allowed duplicate polling loops, and hot_add() didn't await old polling task termination. 4. onboarding_state was always None in extension API responses and PairingRequired SSE was never emitted, so the frontend could never render the pairing card. Changes: - approve_pairing (DB trait + both backends) now returns external_id - WasmChannel.owner_actor_id wrapped in RwLock with set_owner_actor_id() - ExtensionManager.complete_pairing_approval() orchestrates: persist owner_id → update running channel → restart polling - pairing_approve_handler calls complete_pairing_approval and emits PairingCompleted SSE (scoped to approving user) - refresh_active_channel() calls ensure_polling() and syncs owner - Per-channel activation mutex prevents TOCTOU race - hot_add() drops write lock before awaiting shutdown - Extension list handlers populate onboarding_state when Pairing - derive_onboarding() helper in handlers/extensions.rs - Regression tests for derive_onboarding and resolve_message_scope Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the v2 engine hits a gate-paused state (approval needed, auth required), the web gateway was sending BOTH an interactive card (via send_status → SSE) AND a redundant text message (via AppEvent::Response). Users saw a duplicate prompt. Root cause: v2 bridge functions returned Ok(Some(text)) for gate-paused outcomes, which mapped via from_legacy to HandleOutcome::Respond — sending both the card and the text. The v1 path correctly used HandleOutcome::Pending. Fix: - Gate-paused paths in router.rs now return Ok(None) instead of text - New bridge_to_outcome() checks has_any_pending_gate() after each v2 bridge call — if a gate exists, returns Pending (suppresses text + Done) - New from_bridge() maps None → NoResponse (not Shutdown) for v2 paths - Removed pending_gate_prompt_message() — the function that generated the duplicate text - notify_pending_gate() no longer emits GateRequired SSE directly (redundant with send_pending_gate_status per-channel routing) - Updated 3 tests to assert None return + StatusUpdate delivery Each channel renders the approval/auth card natively via send_status: web → SSE card, TUI → widget, relay → buttons. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- bridge_to_outcome: only return Pending when handler returned None (preserves legitimate text responses for ambiguous gate messages) - process_emitted_messages: clone owner_actor_id out of read lock before awaiting resolve_message_scope_with_pairing - Normalize channel_name to lowercase in complete_pairing_approval and pairing_approve_handler for consistent webhook/store lookups - cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…iring extraction
- Replace Option<String> bridge handler returns with typed BridgeOutcome
enum (Respond/NoResponse/Pending), eliminating post-hoc has_any_pending_gate
query and the None→NoResponse mapping that swallowed v2 shutdown signals
- Add ExternalId newtype for approve_pairing return (was bare String)
- Fix noop PairingStore::approve to return NotFound instead of Ok("")
- Extract pairing approval orchestration to src/pairing/approval.rs
- Clone RwLock<owner_actor_id> before awaiting in respond()
- Downgrade warn! to debug! in pairing handlers (TUI logging rule)
- Gate TELEGRAM_TEST_API_BASE_ENV const behind cfg(test/debug_assertions)
- Remove hardcoded Telegram auth instructions; use capabilities prompt
- Fix unused mut receiver in test
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… generic pairing The Telegram-specific verification challenge (/start CODE deep link flow) blocked the generic pairing flow from ever running — configure() returned early with activated:false when the challenge was pending, so the channel never started polling and users couldn't generate pairing codes. Removed ~1200 lines: - TelegramBindingResult, TelegramBindingData, TelegramOwnerBindingState, TelegramVerificationMeta, PendingTelegramVerificationChallenge types - configure_telegram_binding, resolve_telegram_binding, issue_telegram_verification_challenge, notify_telegram_owner_verified and all Telegram API response types (getUpdates polling loop, etc.) - ConfigureResult.verification field + VerificationChallenge re-export - All verification-related test fixtures and 6 test functions - Dead RecordingChannel test helper, unused set_channel_owner_id method - Gated send_telegram_text_message + helpers behind cfg(test) Replaced with: - validate_telegram_token() — lightweight getMe call for token validation + bot_username extraction (persisted for mention detection) - All channels now follow: credentials → validate → activate → pairing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing mode After a channel activates with no owner binding, broadcast a per-user PairingRequired SSE event so the web UI shows the pairing card without requiring a manual refresh. Also populate pairing_required, onboarding_state, and onboarding fields on ConfigureResult so callers know the channel needs pairing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a tool triggers an auth gate (awaiting_token), the dispatcher
already sends an AuthRequired card and puts the thread in auth mode.
The thread_ops handler was then calling complete_turn(&instructions)
which overwrote auth mode back to Idle AND persisted the auth prompt
("Enter your Telegram Bot API token...") as the turn response — rendering
a redundant text bubble alongside the auth card.
Fix: skip complete_turn and persist_assistant_response for AuthPending.
The turn is paused (not complete), and the auth card is the only
user-facing signal. Tool calls are still persisted for history.
Also removes the now-unused `instructions` field from
AgenticLoopResult::AuthPending — the instructions were already sent
via the AuthRequired status event before AuthPending is returned.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After the web UI submits a token via /api/chat/auth-token or approves
pairing via /api/pairing/{channel}/approve, the agent's turn was stuck
at Pending forever — these HTTP handlers configured the extension
directly but never signaled the agent loop to resume.
Fix: inject a follow-up message through msg_tx (the agent's message
channel) after successful auth/pairing. This uses the same pattern as
the OAuth callback handler — the LLM picks up the injected message,
sees the activation/pairing result, and produces a natural response.
The response goes through the full agent pipeline (hooks, safety,
history persistence, Done event).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the user dismisses the auth card, the frontend calls /api/chat/auth-cancel which clears auth mode. But the original agent turn was still paused at Pending with no Done event. The UI stayed stuck at "Processing..." forever. Fix: inject a cancellation message through msg_tx so the LLM can acknowledge the cancellation and the turn completes naturally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The injected follow-up message after pairing approval had no thread_id, causing the gateway to fail with "missing a routing target." The response from the LLM was produced but couldn't be delivered. Fix: add optional thread_id to PairingApproveRequest. The frontend passes currentThreadId so the agent responds in the same conversation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers: - Auth-token/cancel handlers don't 500 - Pairing approve accepts optional thread_id field - Backward compatibility: approve without thread_id works - PairingRequired SSE shows pairing card - PairingCompleted SSE dismisses pairing card - Frontend sends currentThreadId in pairing approve request body Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The AuthPending handler was not calling complete_turn() (to avoid persisting redundant auth instructions as the response), but this also skipped the ThreadState::Processing → Idle transition. The thread stayed stuck in Processing forever, so the follow-up message injected through msg_tx after auth/pairing was silently rejected. Fix: explicitly set thread.state = Idle in both AuthPending arms without calling complete_turn(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Telegram verification challenge flow was removed — channels now go straight to activation and use the generic pairing flow. The conditional verification retry in setup_telegram() was dead code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve single conflict in src/extensions/manager.rs: keep staging's inject_wasm_channel_secret_config_updates() (Feishu #2443) alongside this branch's removal of dead Telegram verification code. Also add V24 migration checksum to checksums.lock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Gate TELEGRAM_TEST_API_BASE_ENV and telegram_api_base_url() behind cfg(any(test, debug_assertions)) to prevent production env var override (serrrfirat HIGH — ship blocker) - Sanitize validate_telegram_token() error messages to avoid leaking bot tokens via reqwest Display (Copilot) - Log failed msg_tx sends instead of silently dropping (ilblackdragon) - Forward thread_id in PairingCompleted SSE event (Copilot) - Fix stale doc comment on persist_numeric_owner_id (Copilot) - Hoist duplicate parse::<i64>() in propagate_approval (ilblackdragon) - Delete dead _removed_telegram_verification_test (ilblackdragon) - Fix always-passing E2E thread_id assertion (Copilot) - Add V24 migration checksum to checksums.lock Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The doc said "silently succeeds" but the implementation returns NotFound when no database is configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…in spawned tasks Two hardening fixes from PR review deferrals: 1. Extension names from HTTP request bodies were interpolated directly into format strings that become IncomingMessage content fed to the agent loop. Add sanitize_extension_name() that strips non-alphanumeric chars and apply it at the two prompt injection points in chat_auth_token_handler and chat_auth_cancel_handler. 2. start_polling() and start_websocket_runtime() captured owner_actor_id as an owned Option<String> at spawn time. After pairing approval, WebSocket channels kept using the stale pre-approval value. Change to pass Arc<RwLock<Option<String>>> so spawned tasks read the current owner on each tick/event. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts: - wrapper.rs: keep cfg(test) for TELEGRAM_TEST_API_BASE_ENV (staging narrowed from debug_assertions; manager.rs has its own gate) - wrapper.rs: keep both set_owner_actor_id and new test helpers from staging; adapt owner_actor_id_for_test to async (reads Arc<RwLock>) - setup.rs: add .await to owner_actor_id_for_test() calls Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ening, WS parity Structural changes: - Replace Thread::complete_turn/fail_turn/interrupt with single conclude_turn(TurnOutcome) that makes it impossible to forget the turn state. Fixes AuthPending arms leaving Turn stuck at Processing. - Add TurnOutcome::CompletedSilently for auth-card-only turns. Security: - Sanitize channel name in pairing_approve_handler (missed injection site) - Fix bot token leak in validate_telegram_token — log safe fields (is_timeout, is_connect, status) instead of reqwest error display which includes the URL containing the token - Consume stale fallback auth gate before replaying message to prevent duplicate agentic runs on repeated OAuth callbacks - Sanitize channel_name in derive_onboarding user-visible strings - Add #[must_use] to BridgeOutcome enum WS/REST parity: - Add thread_id to WsClientMessage::AuthToken and AuthCancel - WS AuthToken handler now injects follow-up message via msg_tx (matching REST chat_auth_token_handler behavior) - WS AuthCancel handler now clears engine pending auth and injects cancellation message (matching REST chat_auth_cancel_handler) Cleanup: - Deduplicate build_runtime_config_updates (manager.rs imports from approval.rs instead of maintaining its own copy) - Downgrade info! to debug! for auto-generated secret log - Downgrade warn! to debug! for OAuth fallback diagnostic - Upgrade debug! to warn! for on_start failure in propagate_approval - Rename misleading e2e test to match what it actually tests - Add mixed-character truncation test for sanitize_extension_name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…jection New e2e tests: - test_auth_cancel_injects_follow_up_message_via_sse: verifies the msg_tx injection path actually delivers messages end-to-end (SSE response event appears after auth-cancel) - test_sanitize_extension_name_in_auth_cancel: verifies injection characters in extension_name are stripped before reaching the agent loop - test_pairing_approve_sanitizes_channel_name: verifies channel path param is sanitized in pairing approve handler - test_ws_auth_token_accepts_thread_id: verifies WS auth_token messages accept the new thread_id field - test_ws_auth_cancel_accepts_thread_id: verifies WS auth_cancel messages accept thread_id and connection stays alive Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When result.activated was false, the chat_auth_token_handler skipped the msg_tx injection. This left the paused turn (Pending with Done suppressed) permanently stuck — the UI showed "Running tool_install..." forever. Now both REST and WS handlers always: 1. Clear auth mode 2. Broadcast AuthCompleted (with success=true/false) 3. Inject a follow-up message via msg_tx The message content varies based on activation status so the LLM can respond appropriately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eiver The previous fix (drop write lock before shutdown) removed the channel from the map before calling shutdown(). This dropped the last strong Arc reference in the channel manager, killing the forwarding task's receiver. The router holds its own Arc to the inner WasmChannel, so propagate_approval's ensure_polling() could still send via message_tx — but the receiver was dead, causing "channel closed" errors. Revert to the staging pattern: read-lock to clone the Arc, drop the lock, shutdown the clone, then write-lock to insert the replacement. The old entry stays in the map (keeping the forwarding task alive) until the insert atomically replaces it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot review: the set_setting result for bot_username was silently dropped with `let _ =`. Now logs at debug level if the DB write fails, giving visibility into mention detection degradation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a WASM channel is loaded at boot without credentials (fresh DB),
on_start fails (e.g., Telegram deleteWebhook returns 404 with unresolved
{TELEGRAM_BOT_TOKEN}). Previously, message_tx was set BEFORE on_start,
so the sender survived but the receiver (rx) was dropped on error return.
Later, refresh_active_channel restarted polling which cloned the orphaned
sender — every send failed with "channel closed".
Fixes:
- Move message_tx creation AFTER on_start succeeds in Channel::start()
- Add WasmChannel::ensure_message_channel() that creates (tx, rx) if
message_tx is None or closed, returning the stream for forwarding
- refresh_active_channel calls ensure_message_channel() after on_start
succeeds and wires up a forwarding task if needed
Also:
- Revert hot_add to match staging exactly (no behavior change needed)
- Remove temporary debug logging (message_tx state before dispatch)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Update AuthPending doc to reflect TurnOutcome::CompletedSilently (was "turn NOT completed", now accurately describes conclude_turn) - Move `import websockets` inside try block so ImportError is caught by the except handler when the package isn't installed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Staging added set_channel_owner_id(), configure_telegram_binding(), resolve_telegram_binding(), and notify_telegram_owner_verified() in manager.rs. These are all part of the old Telegram verification flow that this PR intentionally removes. Take our side for both conflict regions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rs, tighten tests - propagate_approval: propagate on_start() error as ActivationFailed instead of swallowing it (zmanian review #1) - router.rs: move test-only HashMap import into mod tests (zmanian #2) - chat.rs: remove duplicate clear_auth_mode (Copilot review #1) - e2e: strengthen auth-token assertion to check status 200 + success field, remove overlapping test_auth_cancel_returns_success (Copilot #2/#3) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…warn log - ensure_message_channel: single write lock for atomic check-and-create (fixes TOCTOU race where concurrent callers could orphan a forwarding task) - chat_auth_token_handler: add missing clear_engine_pending_auth() call (REST/WS parity — WS and REST cancel already had it, REST token did not) - pairing_approve_handler: debug! → warn! for complete_pairing_approval failure (operationally significant — channel won't route until restart) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…prove propagation, skip double Telegram getMe (#2432) - Sanitize result.message before interpolation into synthetic agent input to prevent prompt injection via crafted validation errors (server.rs + ws.rs) - Surface complete_pairing_approval() failure to frontend with success=false SSE event and ActionResponse::fail instead of silently succeeding - Return ActionResponse::ok when auth_url is present even if activated=false so OAuth flows can progress through the frontend popup - Skip generic validation_endpoint check for Telegram (validate_telegram_token already calls getMe and extracts bot_username — avoids double API round-trip) - Sanitize generic validation_endpoint error messages to avoid leaking sensitive URL paths (e.g. bot tokens) via reqwest::Error Display Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: llm
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 58 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+168
to
+170
| import json | ||
|
|
||
| body = json.loads(captured["body"]) |
There was a problem hiding this comment.
Redundant import json inside test_pairing_approve_sends_thread_id; the module already imports json at the top. Removing the inner import avoids unnecessary shadowing and keeps the test file consistent.
…y-onboarding # Conflicts: # src/channels/web/server.rs # src/tools/builtin/glob_tool.rs # src/tools/builtin/grep_tool.rs
github-actions
Bot
added
risk: medium
nickpismenkov
approved these changes
Apr 16, 2026
14 tasks
Merged
5 tasks
ilblackdragon
added a commit
that referenced
this pull request
Apr 18, 2026
The e2e assertion at tests/e2e_builtin_tool_coverage.rs:1230 checked for a lowercase "use the `message` tool ..." substring, but #2515 capitalized the first word in src/tools/builtin/extension_tools.rs:110. The local unit test in that file was updated; this e2e test was missed, breaking CI on main and blocking the release-plz PR (#2606). Normalize to lowercase before substring match so a future copy-edit doesn't silently break CI again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Apr 18, 2026
The e2e assertion at tests/e2e_builtin_tool_coverage.rs:1230 checked for a lowercase "use the `message` tool ..." substring, but #2515 capitalized the first word in src/tools/builtin/extension_tools.rs:110. The local unit test in that file was updated; this e2e test was missed, breaking the Run Tests job on main and blocking release-plz PR #2606. Normalize to lowercase before substring match so a future copy-edit doesn't silently break CI again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Apr 18, 2026
* fix(test): case-insensitive assertion in tool_search description The e2e assertion at tests/e2e_builtin_tool_coverage.rs:1230 checked for a lowercase "use the `message` tool ..." substring, but #2515 capitalized the first word in src/tools/builtin/extension_tools.rs:110. The local unit test in that file was updated; this e2e test was missed, breaking the Run Tests job on main and blocking release-plz PR #2606. Normalize to lowercase before substring match so a future copy-edit doesn't silently break CI again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update tests/e2e_builtin_tool_coverage.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This was referenced Apr 18, 2026
ilblackdragon
added a commit
that referenced
this pull request
Apr 21, 2026
…ydration (#2753) * fix: bug bash 4/16 triage — error boundary, TEE secrets, pairing, rehydration Addresses six bug-bash tickets that cluster into five focused fixes. Grouped into one commit because the changes are all small, independent, and share the same release window — split per-file reviewability is preserved by the touched-surface list below and each change carries a regression test. - #2540 — Orchestrator VM timeout is now configurable via `IRONCLAW_ORCHESTRATOR_MAX_DURATION_SECS` (30..=3600s, default 300s). Timeout, memory-limit, and Python-traceback errors map to user-safe messages instead of leaking the Monty interpreter's internal trace. - #1994, #2546 — New `LlmError::BadGateway { provider, status, retry_after }` variant. Upstream 502/503/504 from `nearai_chat` now map here (body logged at debug, never carried on the error) and are retried by `RetryProvider` + counted transient by the circuit breaker. Root cause of #2546's raw-traceback leak was the response body being wrapped into `RequestFailed.reason` and nested three layers deep on the way out; that path is gone. - #1537 — `AppBuilder::init_secrets` always installs a secrets store: persistent when the master key + DB handles resolve, ephemeral in-memory otherwise. This mirrors the ExtensionManager fallback so `WasmToolLoader` and `setup_wasm_channels` get a store on hosted TEE deployments where `SECRETS_MASTER_KEY` is absent, restoring the fail-closed credential-injection path instead of silently dropping into unauthenticated HTTP. - #1839 — Slack `chat.postMessage` returns HTTP 200 on scope/token failures with `{"ok": false, "error": ...}` in the body. Response parsing was extracted into a testable `slack_post_message_result` helper that now surfaces the failure, and `send_pairing_reply` errors are logged with scope guidance (`chat:write`, `im:write`) instead of being swallowed by `let _ = ...`. - #1993 — Chat rehydration's `reconcile_in_progress_with_turns` now requires BOTH a final response AND all recorded tool calls having `has_result && !has_error` before dropping the in-progress flag. Previously a 502 mid-turn would persist the agent's "Done!" claim while the tool call errored, and reopen showed fabricated success. The deeper fix (engine-v2 side-effect gate for the forward path at #2544 / #2541) is a follow-up. Touched surfaces: - channels-src/slack/src/lib.rs - crates/ironclaw_engine/src/executor/orchestrator.rs - src/app.rs - src/channels/web/features/chat/mod.rs - src/llm/{error,nearai_chat,retry,circuit_breaker}.rs Regression tests: - `orchestrator::tests::failure_reason_*` (4 cases covering timeout, memory limit, traceback strip, pass-through) - `llm::retry::tests::test_is_retryable_classification` (BadGateway arm) - `app::tests::ephemeral_secrets_store_is_constructible_and_usable` - `slack::tests::slack_post_message_result_{accepts,rejects,empty}` - `chat::tests::test_reconcile_retains_in_progress_when_tool_call_failed` Out of scope / deferred: - #2544, #2541 — engine-v2 hard side-effect gate (documented as aspirational in `.claude/rules/tool-evidence.md`; design belongs in its own PR). - #2437 — closed upstream, no code change; see #2437 (comment) - #2543 — likely fixed by #2515, needs retest on staging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tee): surface persistent-store failures and probe in doctor Follow-up to the #1537 ephemeral-store fallback. The fallback alone doesn't tell an operator *why* the persistent store is missing on a hosted TEE — that was #1537's real ergonomic pain. Three diagnostic improvements: 1. `install_ephemeral_secrets_store` now takes a `reason` tag and logs at `warn!` with the specific path (no master key / crypto failure / no DB handles / feature-flag mismatch / unexpected create_secrets_store None). Previously the install was silent at `debug!`, so operators had no signal the fallback had fired. 2. `ironclaw doctor`'s `check_secrets` now runs the same `SecretsConfig::resolve` path `AppBuilder::init_secrets` uses, then calls `create_secrets_store` to probe that the backing store is actually reachable. The old check only read `settings.secrets_master_key_source`, which misses the exact hosted-TEE failure mode: master key resolves to `Env`/`Keychain` but the DB handle isn't wired, so the store factory returns None and runtime silently falls back to ephemeral. 3. `src/db/CLAUDE.md` note claiming `LibSqlSecretsStore` is "not plumbed through the main startup path" was stale — the factory dispatches on `DatabaseHandles` (init_secrets path) and `DatabaseBackend` (CLI helper) and both wire libSQL. Note updated to reflect the actual wiring plus the #1537 ephemeral-fallback contract. The two existing `check_secrets` unit tests asserted the old settings- only behavior; rewritten as "does-not-panic" checks because the new function reads real env and the outcome is test-host dependent (matches the shape of `check_docker_daemon_does_not_panic`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm,app): address PR #2753 review comments Three fixes from Copilot + Gemini review on PR #2753: 1. **BadGateway retry_after no longer forces 60s sleeps.** Copilot flagged that `retry_after_header` was always `Some(parse_retry_after(...))`, and `parse_retry_after` returns a 60s default when the header is absent. That meant 502/503/504 responses without a Retry-After header would sleep ~60s between attempts instead of using exponential backoff (1s → 2s → 4s). Now the header is parsed only when present; absent header → `None` → `RetryProvider` falls through to `retry_backoff_delay`. Existing 429 rate-limit behavior is preserved (60s fallback kept explicit at the 429 call site). 2. **HTTP 500 is now mapped to BadGateway.** Gemini (security-medium) pointed out that upstream application errors frequently return 500 with a Python traceback in the body, and my prior change only mapped 502–504. 500 was falling through to `RequestFailed { reason: "HTTP 500: <body>" }` — exactly the leak #2546 describes. Match broadened to `500..=599`; the `status` field still records the specific code for operators. Matches the intent documented in `.claude/rules/error-handling.md` ("raw HTTP 5xx → temporarily unavailable"). 3. **Ephemeral secrets store now fails loud.** Copilot observed that `build_ephemeral_secrets_store` returning `None` + the fallback install silently dropping it left `self.secrets_store = None` possible, which would blow up much later in `init_extensions` with a less-actionable "secrets store not initialized" error. Changed to return `Result`; `install_ephemeral_secrets_store` propagates via `?` so startup aborts at the real root cause. Regression tests: - `llm::retry::tests::bad_gateway_without_retry_after_does_not_match_some_arm` (fix 1 — guards against the `Some(_)` match arm catching a None value) - `llm::retry::tests::test_is_retryable_classification` gains a `BadGateway { status: 500, .. }` case (fix 2) - `app::tests::ephemeral_secrets_store_is_constructible_and_usable` already exercised `.expect(...)` on the builder — now validates the `Result` contract (fix 3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway): typed orchestrator failure + preserve debug detail Addresses the remaining PR #2753 review feedback (Copilot + serrrfirat): - Introduce OrchestratorFailure / OrchestratorFailureKind typed enum in the engine's error module. Replaces the format!()-built `reason` that fed EngineError::Effect. Parse, start, resume, and NameLookup panic paths all route through the typed classifier — user-safe message via Display, raw detail preserved in `debug_detail`. - EngineError gains an Orchestrator(OrchestratorFailure) variant and a debug_detail() accessor. ThreadOutcome::Failed carries the detail through to the channel edge. - bridge/router.rs: new `gateway_debug_errors_enabled()` helper reads IRONCLAW_DEBUG_ERRORS and appends the preserved detail to the reply when on. Off by default — low-level detail still goes to tracing::debug. - Tighten the orchestrator timeout substring match from the bare "duration" to "timed out" / "timeout" / "duration limit" / "max_duration" / "maximum duration" so unrelated runtime errors no longer get misclassified as time-budget exhaustion. - doctor's check_secrets is now read-only: uses crate::secrets:: resolve_master_key (env + keychain only) instead of the auto- persisting SecretsConfig::resolve. Missing key reports as Skip without mutating ~/.ironclaw/.env. - Chat reload: turn_tool_calls_succeeded keys off the *trailing* tool call rather than every tool call in turn history, so a turn that errored once and recovered via a later successful retry no longer stays pinned to Processing forever. Regression tests: - failure_reason_does_not_treat_bare_duration_as_timeout - failure_reason_strips_python_traceback asserts debug_detail retains raw trace - test_reconcile_allows_recovery_from_earlier_tool_error Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(gateway): surface engine debug detail to Debug Inspector + logs Replaces the IRONCLAW_DEBUG_ERRORS env-var gate with unconditional visibility in the two places it actually belongs: the gateway's Debug Inspector panel and debug text logs. The chat reply stays sanitized. - Drop gateway_debug_errors_enabled() and the env-var-gated append in bridge_outcome_for_failed_thread. The flag was only there because the only delivery path was the chat reply, which can't carry raw detail. - Extend AppEvent::Error with an optional debug_detail field. Serialized onto the SSE `error` event so any listener (Debug Inspector, future tooling) sees it. - On ThreadOutcome::Failed, broadcast AppEvent::Error with {sanitized message, raw debug_detail, thread_id} so the inspector picks it up even though the chat reply is sanitized. - debug-panel.js renders debug_detail underneath the sanitized message on the Activity tab so operators can triage without tailing logs. - tracing::warn! on the failure path now includes debug_detail, which flows through log_layer into the gateway's log event stream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR #2753 follow-up review fixes Addresses four Copilot comments on commits 3c08e0c / 042c2ee: - orchestrator.rs: OrchestratorFailureKind::Other no longer renders the raw err_msg in Display. Channel-edge surfaces that bypass `user_facing_thread_failure` (runtime/mission.rs builds `format!("Mission failed: {error}")` directly) would have leaked tracebacks / internal file paths via unclassified Monty errors. User-facing text is now a generic "internal orchestrator failure"; the raw message is preserved on OrchestratorFailure::debug_detail as before. Dropped the now-unused `message` field on Other. - bridge/router.rs: the failure-path `warn!` now logs only `debug_detail_bytes`, not the full detail. Full raw text is emitted at `debug!` level so higher-severity logs don't carry multi-KB tracebacks. Operators still see the complete detail in the Debug Inspector (via AppEvent::Error.debug_detail) or with `RUST_LOG=ironclaw::bridge::router=debug`. - cli/doctor.rs: source_label had an unreachable KeySource::None arm. Since the key-present guard above already returned Skip, `source` is only ever Env or Keychain here — folded the match into the existing env-wins branch. Regression test renamed: `failure_reason_hides_unknown_raw_message_from_user_text` now asserts `Other`'s Display does not leak `NameError` while debug_detail still preserves it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR #2753 follow-up round 2 Addresses serrrfirat's review on commit 82d0641 — four issues that remained after the previous fix landed: - router.rs: a failed engine v2 thread on the web flow used to broadcast both AppEvent::Error on SSE and BridgeOutcome::Respond. GatewayChannel::respond then re-broadcast the same sanitized text as a response frame, so the browser rendered the same failure twice. The helper now takes sse_will_deliver_to_user and returns NoResponse when the originating channel is the gateway, so the SSE error card is the single user-visible surface. Non-gateway channels (telegram, relay, cli) still get Respond(sanitized) for primary delivery. - AppEvent::Error: debug_detail travelled on the default scoped SSE error event, where every authenticated consumer (chat UI, devtools, custom clients) sees it. Raw Monty tracebacks / upstream HTTP bodies must not cross that boundary. The field is removed from the wire payload; detail stays server-side via the existing tracing::debug! edge. The Debug Inspector now renders only the sanitized message. - doctor.rs: check_secrets probed the runtime via db::create_secrets_store, which opens a fresh backend and runs migrations — side-effectful, and not the same path that failed on hosted-TEE in #1537. The probe now uses connect_without_migrations + secrets::create_secrets_store(crypto, &handles), exercising the exact DatabaseHandles→Option<Arc<SecretsStore>> dispatch that AppBuilder::init_secrets runs. No migrations fire. - orchestrator.rs: the OrchestratorFailureKind::TimeLimit classifier caught any err_msg containing "timeout"/"timed out", so upstream LLM/network timeouts (Request timed out, Connection timed out) got mapped to TimeLimit and the user-facing message advised raising IRONCLAW_ORCHESTRATOR_MAX_DURATION_SECS — wrong remediation. The predicate set is narrowed to unmistakable Monty wall-clock markers (duration limit / max_duration / maximum duration / execution duration exceeded / orchestrator timed out). Upstream timeouts now fall through to Other. Regression tests: - failed_thread_outcome_is_no_response_when_sse_will_deliver locks in the single-surface contract for the gateway web flow. - failure_reason_does_not_treat_upstream_timeout_as_time_limit asserts four upstream-timeout shapes classify as Other (not TimeLimit) and their user message does NOT advise the budget knob. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR #2753 follow-up round 3 Addresses Copilot review comments on 8489a97 plus four review-derived nits surfaced during triage: - Update stale rehydration comment in `reconcile_in_progress_with_turns` to describe trailing-tool-call semantics (earlier failed attempts are allowed if a later retry succeeded) rather than the old "every recorded tool call completed successfully" wording. - Update stale rustdoc on `check_secrets` to describe the read-only `resolve_master_key()` probe instead of the dropped `SecretsConfig:: resolve` path that used to auto-generate keys. - Export `GATEWAY_CHANNEL_NAME` from `channels::web` and reference it from both the `Channel::name()` impl and `bridge::router`, eliminating the duplicated string literal. - Split `parse_retry_after` into two helpers. The existing `Option<&HeaderValue> -> Duration` stays for rate-limit callers (60s default on missing). New `parse_retry_after_value(&HeaderValue) -> Duration` is for 5xx paths that want to distinguish "absent" from "unparseable" so missing headers fall through to exponential backoff. - Strengthen doctor secrets tests: add `check_secrets_reports_env_source_when_env_key_is_set` which, under ENV_MUTEX, sets SECRETS_MASTER_KEY and asserts the rendered message surfaces the env source label plus the settings-vs-runtime drift warning — pinning the exact #1537 hosted-TEE axis the prior "doesn't panic" test couldn't detect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(doctor): allow await_holding_lock on env-guarded test `check_secrets_reports_env_source_when_env_key_is_set` holds the global `ENV_MUTEX` from `config::helpers::lock_env()` (a `std::sync::Mutex`) across `check_secrets(..).await`, which the `clippy::await_holding_lock` lint flags. The env vars the guard protects (`SECRETS_MASTER_KEY`) must stay pinned through the await because `check_secrets` reads them internally — dropping the guard early would let a concurrent test race on the env var. Mirrors the existing pattern in `bridge::auth_manager` (six existing sites). Local `cargo clippy --lib` missed this; CI runs with `--tests`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
* fix(channels): wire up pairing approval, polling restart, and onboarding state
The Telegram channel setup flow via the gateway was broken end-to-end.
Four interconnected bugs prevented pairing/ownership from completing:
1. pairing_approve_handler only wrote to channel_identities DB — the
running WasmChannel's owner_actor_id was never updated, so the
owner was never recognized and broadcast metadata was never stored.
2. refresh_active_channel() re-ran on_start() but never called
ensure_polling(), leaving polling in a stale state on repeated
tool_activate calls and causing Telegram 409 conflicts.
3. activate_wasm_channel() had a TOCTOU race on active_channel_names
that allowed duplicate polling loops, and hot_add() didn't await
old polling task termination.
4. onboarding_state was always None in extension API responses and
PairingRequired SSE was never emitted, so the frontend could
never render the pairing card.
Changes:
- approve_pairing (DB trait + both backends) now returns external_id
- WasmChannel.owner_actor_id wrapped in RwLock with set_owner_actor_id()
- ExtensionManager.complete_pairing_approval() orchestrates: persist
owner_id → update running channel → restart polling
- pairing_approve_handler calls complete_pairing_approval and emits
PairingCompleted SSE (scoped to approving user)
- refresh_active_channel() calls ensure_polling() and syncs owner
- Per-channel activation mutex prevents TOCTOU race
- hot_add() drops write lock before awaiting shutdown
- Extension list handlers populate onboarding_state when Pairing
- derive_onboarding() helper in handlers/extensions.rs
- Regression tests for derive_onboarding and resolve_message_scope
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(bridge): eliminate dual card + text emission for gate-paused flows
When the v2 engine hits a gate-paused state (approval needed, auth
required), the web gateway was sending BOTH an interactive card (via
send_status → SSE) AND a redundant text message (via AppEvent::Response).
Users saw a duplicate prompt.
Root cause: v2 bridge functions returned Ok(Some(text)) for gate-paused
outcomes, which mapped via from_legacy to HandleOutcome::Respond — sending
both the card and the text. The v1 path correctly used HandleOutcome::Pending.
Fix:
- Gate-paused paths in router.rs now return Ok(None) instead of text
- New bridge_to_outcome() checks has_any_pending_gate() after each v2
bridge call — if a gate exists, returns Pending (suppresses text + Done)
- New from_bridge() maps None → NoResponse (not Shutdown) for v2 paths
- Removed pending_gate_prompt_message() — the function that generated
the duplicate text
- notify_pending_gate() no longer emits GateRequired SSE directly
(redundant with send_pending_gate_status per-channel routing)
- Updated 3 tests to assert None return + StatusUpdate delivery
Each channel renders the approval/auth card natively via send_status:
web → SSE card, TUI → widget, relay → buttons.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address PR review comments
- bridge_to_outcome: only return Pending when handler returned None
(preserves legitimate text responses for ambiguous gate messages)
- process_emitted_messages: clone owner_actor_id out of read lock
before awaiting resolve_message_scope_with_pairing
- Normalize channel_name to lowercase in complete_pairing_approval
and pairing_approve_handler for consistent webhook/store lookups
- cargo fmt
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address self-review — BridgeOutcome enum, ExternalId newtype, pairing extraction
- Replace Option<String> bridge handler returns with typed BridgeOutcome
enum (Respond/NoResponse/Pending), eliminating post-hoc has_any_pending_gate
query and the None→NoResponse mapping that swallowed v2 shutdown signals
- Add ExternalId newtype for approve_pairing return (was bare String)
- Fix noop PairingStore::approve to return NotFound instead of Ok("")
- Extract pairing approval orchestration to src/pairing/approval.rs
- Clone RwLock<owner_actor_id> before awaiting in respond()
- Downgrade warn! to debug! in pairing handlers (TUI logging rule)
- Gate TELEGRAM_TEST_API_BASE_ENV const behind cfg(test/debug_assertions)
- Remove hardcoded Telegram auth instructions; use capabilities prompt
- Fix unused mut receiver in test
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(channels): remove dead Telegram verification flow, consolidate to generic pairing
The Telegram-specific verification challenge (/start CODE deep link flow)
blocked the generic pairing flow from ever running — configure() returned
early with activated:false when the challenge was pending, so the channel
never started polling and users couldn't generate pairing codes.
Removed ~1200 lines:
- TelegramBindingResult, TelegramBindingData, TelegramOwnerBindingState,
TelegramVerificationMeta, PendingTelegramVerificationChallenge types
- configure_telegram_binding, resolve_telegram_binding,
issue_telegram_verification_challenge, notify_telegram_owner_verified
and all Telegram API response types (getUpdates polling loop, etc.)
- ConfigureResult.verification field + VerificationChallenge re-export
- All verification-related test fixtures and 6 test functions
- Dead RecordingChannel test helper, unused set_channel_owner_id method
- Gated send_telegram_text_message + helpers behind cfg(test)
Replaced with:
- validate_telegram_token() — lightweight getMe call for token validation
+ bot_username extraction (persisted for mention detection)
- All channels now follow: credentials → validate → activate → pairing
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(channels): broadcast PairingRequired SSE after activation in pairing mode
After a channel activates with no owner binding, broadcast a per-user
PairingRequired SSE event so the web UI shows the pairing card without
requiring a manual refresh. Also populate pairing_required, onboarding_state,
and onboarding fields on ConfigureResult so callers know the channel
needs pairing.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(agent): don't persist auth instructions as turn response
When a tool triggers an auth gate (awaiting_token), the dispatcher
already sends an AuthRequired card and puts the thread in auth mode.
The thread_ops handler was then calling complete_turn(&instructions)
which overwrote auth mode back to Idle AND persisted the auth prompt
("Enter your Telegram Bot API token...") as the turn response — rendering
a redundant text bubble alongside the auth card.
Fix: skip complete_turn and persist_assistant_response for AuthPending.
The turn is paused (not complete), and the auth card is the only
user-facing signal. Tool calls are still persisted for history.
Also removes the now-unused `instructions` field from
AgenticLoopResult::AuthPending — the instructions were already sent
via the AuthRequired status event before AuthPending is returned.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(channels): resume agent turn after auth + pairing completion
After the web UI submits a token via /api/chat/auth-token or approves
pairing via /api/pairing/{channel}/approve, the agent's turn was stuck
at Pending forever — these HTTP handlers configured the extension
directly but never signaled the agent loop to resume.
Fix: inject a follow-up message through msg_tx (the agent's message
channel) after successful auth/pairing. This uses the same pattern as
the OAuth callback handler — the LLM picks up the injected message,
sees the activation/pairing result, and produces a natural response.
The response goes through the full agent pipeline (hooks, safety,
history persistence, Done event).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(channels): also resume agent turn on auth cancel
When the user dismisses the auth card, the frontend calls
/api/chat/auth-cancel which clears auth mode. But the original agent
turn was still paused at Pending with no Done event. The UI stayed
stuck at "Processing..." forever.
Fix: inject a cancellation message through msg_tx so the LLM can
acknowledge the cancellation and the turn completes naturally.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(channels): pass thread_id in pairing approve for proper routing
The injected follow-up message after pairing approval had no thread_id,
causing the gateway to fail with "missing a routing target." The
response from the LLM was produced but couldn't be delivered.
Fix: add optional thread_id to PairingApproveRequest. The frontend
passes currentThreadId so the agent responds in the same conversation.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(e2e): add Playwright tests for channel pairing flow
Covers:
- Auth-token/cancel handlers don't 500
- Pairing approve accepts optional thread_id field
- Backward compatibility: approve without thread_id works
- PairingRequired SSE shows pairing card
- PairingCompleted SSE dismisses pairing card
- Frontend sends currentThreadId in pairing approve request body
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(agent): transition thread to Idle on AuthPending
The AuthPending handler was not calling complete_turn() (to avoid
persisting redundant auth instructions as the response), but this
also skipped the ThreadState::Processing → Idle transition. The
thread stayed stuck in Processing forever, so the follow-up message
injected through msg_tx after auth/pairing was silently rejected.
Fix: explicitly set thread.state = Idle in both AuthPending arms
without calling complete_turn().
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(e2e): remove dead verification challenge branch from telegram e2e
The Telegram verification challenge flow was removed — channels now
go straight to activation and use the generic pairing flow. The
conditional verification retry in setup_telegram() was dead code.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address PR review comments
- Gate TELEGRAM_TEST_API_BASE_ENV and telegram_api_base_url() behind
cfg(any(test, debug_assertions)) to prevent production env var override
(serrrfirat HIGH — ship blocker)
- Sanitize validate_telegram_token() error messages to avoid leaking bot
tokens via reqwest Display (Copilot)
- Log failed msg_tx sends instead of silently dropping (ilblackdragon)
- Forward thread_id in PairingCompleted SSE event (Copilot)
- Fix stale doc comment on persist_numeric_owner_id (Copilot)
- Hoist duplicate parse::<i64>() in propagate_approval (ilblackdragon)
- Delete dead _removed_telegram_verification_test (ilblackdragon)
- Fix always-passing E2E thread_id assertion (Copilot)
- Add V24 migration checksum to checksums.lock
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: update PairingStore::approve doc for noop mode
The doc said "silently succeeds" but the implementation returns
NotFound when no database is configured.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: sanitize extension names in agent prompts + live owner_actor_id in spawned tasks
Two hardening fixes from PR review deferrals:
1. Extension names from HTTP request bodies were interpolated directly into
format strings that become IncomingMessage content fed to the agent loop.
Add sanitize_extension_name() that strips non-alphanumeric chars and apply
it at the two prompt injection points in chat_auth_token_handler and
chat_auth_cancel_handler.
2. start_polling() and start_websocket_runtime() captured owner_actor_id as
an owned Option<String> at spawn time. After pairing approval, WebSocket
channels kept using the stale pre-approval value. Change to pass
Arc<RwLock<Option<String>>> so spawned tasks read the current owner on
each tick/event.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address PR review findings — TurnOutcome refactor, security hardening, WS parity
Structural changes:
- Replace Thread::complete_turn/fail_turn/interrupt with single
conclude_turn(TurnOutcome) that makes it impossible to forget the
turn state. Fixes AuthPending arms leaving Turn stuck at Processing.
- Add TurnOutcome::CompletedSilently for auth-card-only turns.
Security:
- Sanitize channel name in pairing_approve_handler (missed injection site)
- Fix bot token leak in validate_telegram_token — log safe fields
(is_timeout, is_connect, status) instead of reqwest error display
which includes the URL containing the token
- Consume stale fallback auth gate before replaying message to prevent
duplicate agentic runs on repeated OAuth callbacks
- Sanitize channel_name in derive_onboarding user-visible strings
- Add #[must_use] to BridgeOutcome enum
WS/REST parity:
- Add thread_id to WsClientMessage::AuthToken and AuthCancel
- WS AuthToken handler now injects follow-up message via msg_tx
(matching REST chat_auth_token_handler behavior)
- WS AuthCancel handler now clears engine pending auth and injects
cancellation message (matching REST chat_auth_cancel_handler)
Cleanup:
- Deduplicate build_runtime_config_updates (manager.rs imports from
approval.rs instead of maintaining its own copy)
- Downgrade info! to debug! for auto-generated secret log
- Downgrade warn! to debug! for OAuth fallback diagnostic
- Upgrade debug! to warn! for on_start failure in propagate_approval
- Rename misleading e2e test to match what it actually tests
- Add mixed-character truncation test for sanitize_extension_name
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(e2e): add critical coverage for auth flow security and msg_tx injection
New e2e tests:
- test_auth_cancel_injects_follow_up_message_via_sse: verifies the msg_tx
injection path actually delivers messages end-to-end (SSE response event
appears after auth-cancel)
- test_sanitize_extension_name_in_auth_cancel: verifies injection characters
in extension_name are stripped before reaching the agent loop
- test_pairing_approve_sanitizes_channel_name: verifies channel path param
is sanitized in pairing approve handler
- test_ws_auth_token_accepts_thread_id: verifies WS auth_token messages
accept the new thread_id field
- test_ws_auth_cancel_accepts_thread_id: verifies WS auth_cancel messages
accept thread_id and connection stays alive
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: always inject follow-up message after auth token submission
When result.activated was false, the chat_auth_token_handler skipped
the msg_tx injection. This left the paused turn (Pending with Done
suppressed) permanently stuck — the UI showed "Running tool_install..."
forever.
Now both REST and WS handlers always:
1. Clear auth mode
2. Broadcast AuthCompleted (with success=true/false)
3. Inject a follow-up message via msg_tx
The message content varies based on activation status so the LLM
can respond appropriately.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: revert hot_add to clone-then-shutdown to preserve message_tx receiver
The previous fix (drop write lock before shutdown) removed the channel
from the map before calling shutdown(). This dropped the last strong
Arc reference in the channel manager, killing the forwarding task's
receiver. The router holds its own Arc to the inner WasmChannel, so
propagate_approval's ensure_polling() could still send via message_tx
— but the receiver was dead, causing "channel closed" errors.
Revert to the staging pattern: read-lock to clone the Arc, drop the
lock, shutdown the clone, then write-lock to insert the replacement.
The old entry stays in the map (keeping the forwarding task alive)
until the insert atomically replaces it.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: log bot_username set_setting failure instead of silently dropping
Copilot review: the set_setting result for bot_username was silently
dropped with `let _ =`. Now logs at debug level if the DB write fails,
giving visibility into mention detection degradation.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: repair message_tx when Channel::start() fails at boot
When a WASM channel is loaded at boot without credentials (fresh DB),
on_start fails (e.g., Telegram deleteWebhook returns 404 with unresolved
{TELEGRAM_BOT_TOKEN}). Previously, message_tx was set BEFORE on_start,
so the sender survived but the receiver (rx) was dropped on error return.
Later, refresh_active_channel restarted polling which cloned the orphaned
sender — every send failed with "channel closed".
Fixes:
- Move message_tx creation AFTER on_start succeeds in Channel::start()
- Add WasmChannel::ensure_message_channel() that creates (tx, rx) if
message_tx is None or closed, returning the stream for forwarding
- refresh_active_channel calls ensure_message_channel() after on_start
succeeds and wires up a forwarding task if needed
Also:
- Revert hot_add to match staging exactly (no behavior change needed)
- Remove temporary debug logging (message_tx state before dispatch)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address remaining review comments — stale doc, websockets import
- Update AuthPending doc to reflect TurnOutcome::CompletedSilently
(was "turn NOT completed", now accurately describes conclude_turn)
- Move `import websockets` inside try block so ImportError is caught
by the except handler when the package isn't installed
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address review comments — propagate on_start error, dedupe helpers, tighten tests
- propagate_approval: propagate on_start() error as ActivationFailed
instead of swallowing it (zmanian review #1)
- router.rs: move test-only HashMap import into mod tests (zmanian #2)
- chat.rs: remove duplicate clear_auth_mode (Copilot review #1)
- e2e: strengthen auth-token assertion to check status 200 + success
field, remove overlapping test_auth_cancel_returns_success (Copilot #2/#3)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: address serrrfirat review — TOCTOU race, missing v2 auth clear, warn log
- ensure_message_channel: single write lock for atomic check-and-create
(fixes TOCTOU race where concurrent callers could orphan a forwarding task)
- chat_auth_token_handler: add missing clear_engine_pending_auth() call
(REST/WS parity — WS and REST cancel already had it, REST token did not)
- pairing_approve_handler: debug! → warn! for complete_pairing_approval
failure (operationally significant — channel won't route until restart)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(web,extensions): address review — sanitize agent messages, fix approve propagation, skip double Telegram getMe (nearai#2432)
- Sanitize result.message before interpolation into synthetic agent input
to prevent prompt injection via crafted validation errors (server.rs + ws.rs)
- Surface complete_pairing_approval() failure to frontend with success=false
SSE event and ActionResponse::fail instead of silently succeeding
- Return ActionResponse::ok when auth_url is present even if activated=false
so OAuth flows can progress through the frontend popup
- Skip generic validation_endpoint check for Telegram (validate_telegram_token
already calls getMe and extracts bot_username — avoids double API round-trip)
- Sanitize generic validation_endpoint error messages to avoid leaking
sensitive URL paths (e.g. bot tokens) via reqwest::Error Display
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Unify gateway onboarding and pairing flows
* Fix gateway message metadata scoping
* Clean up web gateway warnings
* Fix auth and onboarding regression fallout
* Fix gate resolution and pairing rollback trust boundaries
* Guard legacy agent loop from v2 submissions
* Fix PR review follow-ups for onboarding flow
* Fix CI clippy failure in pairing tests
* Fix onboarding review follow-ups
* Fix clippy warning in skills catalog
* Tighten pairing flow e2e assertions
* Fix onboarding auth review follow-ups
* Fix auth routing and tui clippy lint
* Fix pairing gate handoff in onboarding flow
* Fix clippy guard in mission event scan
* Fix merged clippy regressions
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…#2608) * fix(test): case-insensitive assertion in tool_search description The e2e assertion at tests/e2e_builtin_tool_coverage.rs:1230 checked for a lowercase "use the `message` tool ..." substring, but nearai#2515 capitalized the first word in src/tools/builtin/extension_tools.rs:110. The local unit test in that file was updated; this e2e test was missed, breaking the Run Tests job on main and blocking release-plz PR nearai#2606. Normalize to lowercase before substring match so a future copy-edit doesn't silently break CI again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update tests/e2e_builtin_tool_coverage.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…ydration (nearai#2753) * fix: bug bash 4/16 triage — error boundary, TEE secrets, pairing, rehydration Addresses six bug-bash tickets that cluster into five focused fixes. Grouped into one commit because the changes are all small, independent, and share the same release window — split per-file reviewability is preserved by the touched-surface list below and each change carries a regression test. - nearai#2540 — Orchestrator VM timeout is now configurable via `IRONCLAW_ORCHESTRATOR_MAX_DURATION_SECS` (30..=3600s, default 300s). Timeout, memory-limit, and Python-traceback errors map to user-safe messages instead of leaking the Monty interpreter's internal trace. - nearai#1994, nearai#2546 — New `LlmError::BadGateway { provider, status, retry_after }` variant. Upstream 502/503/504 from `nearai_chat` now map here (body logged at debug, never carried on the error) and are retried by `RetryProvider` + counted transient by the circuit breaker. Root cause of nearai#2546's raw-traceback leak was the response body being wrapped into `RequestFailed.reason` and nested three layers deep on the way out; that path is gone. - nearai#1537 — `AppBuilder::init_secrets` always installs a secrets store: persistent when the master key + DB handles resolve, ephemeral in-memory otherwise. This mirrors the ExtensionManager fallback so `WasmToolLoader` and `setup_wasm_channels` get a store on hosted TEE deployments where `SECRETS_MASTER_KEY` is absent, restoring the fail-closed credential-injection path instead of silently dropping into unauthenticated HTTP. - nearai#1839 — Slack `chat.postMessage` returns HTTP 200 on scope/token failures with `{"ok": false, "error": ...}` in the body. Response parsing was extracted into a testable `slack_post_message_result` helper that now surfaces the failure, and `send_pairing_reply` errors are logged with scope guidance (`chat:write`, `im:write`) instead of being swallowed by `let _ = ...`. - nearai#1993 — Chat rehydration's `reconcile_in_progress_with_turns` now requires BOTH a final response AND all recorded tool calls having `has_result && !has_error` before dropping the in-progress flag. Previously a 502 mid-turn would persist the agent's "Done!" claim while the tool call errored, and reopen showed fabricated success. The deeper fix (engine-v2 side-effect gate for the forward path at nearai#2544 / nearai#2541) is a follow-up. Touched surfaces: - channels-src/slack/src/lib.rs - crates/ironclaw_engine/src/executor/orchestrator.rs - src/app.rs - src/channels/web/features/chat/mod.rs - src/llm/{error,nearai_chat,retry,circuit_breaker}.rs Regression tests: - `orchestrator::tests::failure_reason_*` (4 cases covering timeout, memory limit, traceback strip, pass-through) - `llm::retry::tests::test_is_retryable_classification` (BadGateway arm) - `app::tests::ephemeral_secrets_store_is_constructible_and_usable` - `slack::tests::slack_post_message_result_{accepts,rejects,empty}` - `chat::tests::test_reconcile_retains_in_progress_when_tool_call_failed` Out of scope / deferred: - nearai#2544, nearai#2541 — engine-v2 hard side-effect gate (documented as aspirational in `.claude/rules/tool-evidence.md`; design belongs in its own PR). - nearai#2437 — closed upstream, no code change; see nearai#2437 (comment) - nearai#2543 — likely fixed by nearai#2515, needs retest on staging. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tee): surface persistent-store failures and probe in doctor Follow-up to the nearai#1537 ephemeral-store fallback. The fallback alone doesn't tell an operator *why* the persistent store is missing on a hosted TEE — that was nearai#1537's real ergonomic pain. Three diagnostic improvements: 1. `install_ephemeral_secrets_store` now takes a `reason` tag and logs at `warn!` with the specific path (no master key / crypto failure / no DB handles / feature-flag mismatch / unexpected create_secrets_store None). Previously the install was silent at `debug!`, so operators had no signal the fallback had fired. 2. `ironclaw doctor`'s `check_secrets` now runs the same `SecretsConfig::resolve` path `AppBuilder::init_secrets` uses, then calls `create_secrets_store` to probe that the backing store is actually reachable. The old check only read `settings.secrets_master_key_source`, which misses the exact hosted-TEE failure mode: master key resolves to `Env`/`Keychain` but the DB handle isn't wired, so the store factory returns None and runtime silently falls back to ephemeral. 3. `src/db/CLAUDE.md` note claiming `LibSqlSecretsStore` is "not plumbed through the main startup path" was stale — the factory dispatches on `DatabaseHandles` (init_secrets path) and `DatabaseBackend` (CLI helper) and both wire libSQL. Note updated to reflect the actual wiring plus the nearai#1537 ephemeral-fallback contract. The two existing `check_secrets` unit tests asserted the old settings- only behavior; rewritten as "does-not-panic" checks because the new function reads real env and the outcome is test-host dependent (matches the shape of `check_docker_daemon_does_not_panic`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm,app): address PR nearai#2753 review comments Three fixes from Copilot + Gemini review on PR nearai#2753: 1. **BadGateway retry_after no longer forces 60s sleeps.** Copilot flagged that `retry_after_header` was always `Some(parse_retry_after(...))`, and `parse_retry_after` returns a 60s default when the header is absent. That meant 502/503/504 responses without a Retry-After header would sleep ~60s between attempts instead of using exponential backoff (1s → 2s → 4s). Now the header is parsed only when present; absent header → `None` → `RetryProvider` falls through to `retry_backoff_delay`. Existing 429 rate-limit behavior is preserved (60s fallback kept explicit at the 429 call site). 2. **HTTP 500 is now mapped to BadGateway.** Gemini (security-medium) pointed out that upstream application errors frequently return 500 with a Python traceback in the body, and my prior change only mapped 502–504. 500 was falling through to `RequestFailed { reason: "HTTP 500: <body>" }` — exactly the leak nearai#2546 describes. Match broadened to `500..=599`; the `status` field still records the specific code for operators. Matches the intent documented in `.claude/rules/error-handling.md` ("raw HTTP 5xx → temporarily unavailable"). 3. **Ephemeral secrets store now fails loud.** Copilot observed that `build_ephemeral_secrets_store` returning `None` + the fallback install silently dropping it left `self.secrets_store = None` possible, which would blow up much later in `init_extensions` with a less-actionable "secrets store not initialized" error. Changed to return `Result`; `install_ephemeral_secrets_store` propagates via `?` so startup aborts at the real root cause. Regression tests: - `llm::retry::tests::bad_gateway_without_retry_after_does_not_match_some_arm` (fix 1 — guards against the `Some(_)` match arm catching a None value) - `llm::retry::tests::test_is_retryable_classification` gains a `BadGateway { status: 500, .. }` case (fix 2) - `app::tests::ephemeral_secrets_store_is_constructible_and_usable` already exercised `.expect(...)` on the builder — now validates the `Result` contract (fix 3) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway): typed orchestrator failure + preserve debug detail Addresses the remaining PR nearai#2753 review feedback (Copilot + serrrfirat): - Introduce OrchestratorFailure / OrchestratorFailureKind typed enum in the engine's error module. Replaces the format!()-built `reason` that fed EngineError::Effect. Parse, start, resume, and NameLookup panic paths all route through the typed classifier — user-safe message via Display, raw detail preserved in `debug_detail`. - EngineError gains an Orchestrator(OrchestratorFailure) variant and a debug_detail() accessor. ThreadOutcome::Failed carries the detail through to the channel edge. - bridge/router.rs: new `gateway_debug_errors_enabled()` helper reads IRONCLAW_DEBUG_ERRORS and appends the preserved detail to the reply when on. Off by default — low-level detail still goes to tracing::debug. - Tighten the orchestrator timeout substring match from the bare "duration" to "timed out" / "timeout" / "duration limit" / "max_duration" / "maximum duration" so unrelated runtime errors no longer get misclassified as time-budget exhaustion. - doctor's check_secrets is now read-only: uses crate::secrets:: resolve_master_key (env + keychain only) instead of the auto- persisting SecretsConfig::resolve. Missing key reports as Skip without mutating ~/.ironclaw/.env. - Chat reload: turn_tool_calls_succeeded keys off the *trailing* tool call rather than every tool call in turn history, so a turn that errored once and recovered via a later successful retry no longer stays pinned to Processing forever. Regression tests: - failure_reason_does_not_treat_bare_duration_as_timeout - failure_reason_strips_python_traceback asserts debug_detail retains raw trace - test_reconcile_allows_recovery_from_earlier_tool_error Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(gateway): surface engine debug detail to Debug Inspector + logs Replaces the IRONCLAW_DEBUG_ERRORS env-var gate with unconditional visibility in the two places it actually belongs: the gateway's Debug Inspector panel and debug text logs. The chat reply stays sanitized. - Drop gateway_debug_errors_enabled() and the env-var-gated append in bridge_outcome_for_failed_thread. The flag was only there because the only delivery path was the chat reply, which can't carry raw detail. - Extend AppEvent::Error with an optional debug_detail field. Serialized onto the SSE `error` event so any listener (Debug Inspector, future tooling) sees it. - On ThreadOutcome::Failed, broadcast AppEvent::Error with {sanitized message, raw debug_detail, thread_id} so the inspector picks it up even though the chat reply is sanitized. - debug-panel.js renders debug_detail underneath the sanitized message on the Activity tab so operators can triage without tailing logs. - tracing::warn! on the failure path now includes debug_detail, which flows through log_layer into the gateway's log event stream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR nearai#2753 follow-up review fixes Addresses four Copilot comments on commits 3c08e0c / 042c2ee: - orchestrator.rs: OrchestratorFailureKind::Other no longer renders the raw err_msg in Display. Channel-edge surfaces that bypass `user_facing_thread_failure` (runtime/mission.rs builds `format!("Mission failed: {error}")` directly) would have leaked tracebacks / internal file paths via unclassified Monty errors. User-facing text is now a generic "internal orchestrator failure"; the raw message is preserved on OrchestratorFailure::debug_detail as before. Dropped the now-unused `message` field on Other. - bridge/router.rs: the failure-path `warn!` now logs only `debug_detail_bytes`, not the full detail. Full raw text is emitted at `debug!` level so higher-severity logs don't carry multi-KB tracebacks. Operators still see the complete detail in the Debug Inspector (via AppEvent::Error.debug_detail) or with `RUST_LOG=ironclaw::bridge::router=debug`. - cli/doctor.rs: source_label had an unreachable KeySource::None arm. Since the key-present guard above already returned Skip, `source` is only ever Env or Keychain here — folded the match into the existing env-wins branch. Regression test renamed: `failure_reason_hides_unknown_raw_message_from_user_text` now asserts `Other`'s Display does not leak `NameError` while debug_detail still preserves it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR nearai#2753 follow-up round 2 Addresses serrrfirat's review on commit 82d0641 — four issues that remained after the previous fix landed: - router.rs: a failed engine v2 thread on the web flow used to broadcast both AppEvent::Error on SSE and BridgeOutcome::Respond. GatewayChannel::respond then re-broadcast the same sanitized text as a response frame, so the browser rendered the same failure twice. The helper now takes sse_will_deliver_to_user and returns NoResponse when the originating channel is the gateway, so the SSE error card is the single user-visible surface. Non-gateway channels (telegram, relay, cli) still get Respond(sanitized) for primary delivery. - AppEvent::Error: debug_detail travelled on the default scoped SSE error event, where every authenticated consumer (chat UI, devtools, custom clients) sees it. Raw Monty tracebacks / upstream HTTP bodies must not cross that boundary. The field is removed from the wire payload; detail stays server-side via the existing tracing::debug! edge. The Debug Inspector now renders only the sanitized message. - doctor.rs: check_secrets probed the runtime via db::create_secrets_store, which opens a fresh backend and runs migrations — side-effectful, and not the same path that failed on hosted-TEE in nearai#1537. The probe now uses connect_without_migrations + secrets::create_secrets_store(crypto, &handles), exercising the exact DatabaseHandles→Option<Arc<SecretsStore>> dispatch that AppBuilder::init_secrets runs. No migrations fire. - orchestrator.rs: the OrchestratorFailureKind::TimeLimit classifier caught any err_msg containing "timeout"/"timed out", so upstream LLM/network timeouts (Request timed out, Connection timed out) got mapped to TimeLimit and the user-facing message advised raising IRONCLAW_ORCHESTRATOR_MAX_DURATION_SECS — wrong remediation. The predicate set is narrowed to unmistakable Monty wall-clock markers (duration limit / max_duration / maximum duration / execution duration exceeded / orchestrator timed out). Upstream timeouts now fall through to Other. Regression tests: - failed_thread_outcome_is_no_response_when_sse_will_deliver locks in the single-surface contract for the gateway web flow. - failure_reason_does_not_treat_upstream_timeout_as_time_limit asserts four upstream-timeout shapes classify as Other (not TimeLimit) and their user message does NOT advise the budget knob. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine,gateway,doctor): PR nearai#2753 follow-up round 3 Addresses Copilot review comments on 8489a97 plus four review-derived nits surfaced during triage: - Update stale rehydration comment in `reconcile_in_progress_with_turns` to describe trailing-tool-call semantics (earlier failed attempts are allowed if a later retry succeeded) rather than the old "every recorded tool call completed successfully" wording. - Update stale rustdoc on `check_secrets` to describe the read-only `resolve_master_key()` probe instead of the dropped `SecretsConfig:: resolve` path that used to auto-generate keys. - Export `GATEWAY_CHANNEL_NAME` from `channels::web` and reference it from both the `Channel::name()` impl and `bridge::router`, eliminating the duplicated string literal. - Split `parse_retry_after` into two helpers. The existing `Option<&HeaderValue> -> Duration` stays for rate-limit callers (60s default on missing). New `parse_retry_after_value(&HeaderValue) -> Duration` is for 5xx paths that want to distinguish "absent" from "unparseable" so missing headers fall through to exponential backoff. - Strengthen doctor secrets tests: add `check_secrets_reports_env_source_when_env_key_is_set` which, under ENV_MUTEX, sets SECRETS_MASTER_KEY and asserts the rendered message surfaces the env source label plus the settings-vs-runtime drift warning — pinning the exact nearai#1537 hosted-TEE axis the prior "doesn't panic" test couldn't detect. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(doctor): allow await_holding_lock on env-guarded test `check_secrets_reports_env_source_when_env_key_is_set` holds the global `ENV_MUTEX` from `config::helpers::lock_env()` (a `std::sync::Mutex`) across `check_secrets(..).await`, which the `clippy::await_holding_lock` lint flags. The env vars the guard protects (`SECRETS_MASTER_KEY`) must stay pinned through the await because `check_secrets` reads them internally — dropping the guard early would let a concurrent test race on the env var. Mirrors the existing pattern in `bridge::auth_manager` (six existing sites). Local `cargo clippy --lib` missed this; CI runs with `--tests`. 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
This PR consolidates the web gateway onboarding/auth/pairing flow around one normalized onboarding path and fixes the trust-boundary regressions that fell out of the earlier refactor.
The main goals are:
ENGINE_V2is still off by defaultThis PR supersedes #2432.
Problem
Before this change, onboarding/auth behavior was split across several overlapping paths:
That caused multiple user-facing regressions:
telegram_bot_tokeninstead oftelegramtool_installcould report “installed” while the UI had already auto-activated the extensionWhat changed
Unified onboarding flow
onboarding_statepath for extension/channel onboarding in the web gateway./api/extensions/{name}/setupflow as Settings.extension_namehandling so UI routing uses extension identity while backend secret persistence still uses the real credential key.Auth and gate handling
Submission::GateAuthResolutionand exactrequest_id-scoped bridge handling.AppEvent::GateRequiredemission where needed so/v1/responsesdoes not hang on auth-gated requests.request_idwhileENGINE_V2is still disabled by default.Setup and post-auth transitions
request_id,thread_id) so the backend can advance/discard the exact pending auth gate.auth_required, notfailed.Pairing and Telegram runtime fixes
Tool install / activation behavior
tool_installnow follows the real readiness path instead of returning a raw install result while hidden post-install logic separately activates/auth-gates the extension.Web/gateway boundary cleanup
IncomingMessageconstruction helpers so browser-originated and browser-injected messages consistently carryuser_idandthread_idmetadata.Status event missing user_id in metadata; broadcasting globallyregression.GateAuthResolution,ExternalCallback) fail before mutating session thread state whenENGINE_V2=false.Docs
CLAUDE.mdandsrc/channels/web/CLAUDE.mdwith the current onboarding/auth invariants and the temporary v1 compatibility boundary.Risk / compatibility notes
ENGINE_V2is still off by default, so this PR intentionally preserves a narrow legacy compatibility path for web auth prompts that do not carryrequest_id.server.rs,router.rs, and extension/pairing code. This branch improves behavior substantially, but a future backend onboarding coordinator would still be a worthwhile cleanup.Testing
Targeted Rust checks run during this series included:
cargo fmt --allcargo clippy --lib -- -D warningsAuth / onboarding / gate tests:
cargo test test_parser_json_gate_auth_resolution --libcargo test test_chat_gate_resolve_handler_credential_submission_uses_structured_gate_resolution --libcargo test test_chat_auth_token_handler_preserves_user_scoped_metadata --libcargo test test_chat_auth_cancel_handler_clears_requested_thread_auth_mode --libcargo test classify_configure_result_treats_oauth_continuation_as_auth_required --libcargo test event_from_configure_result_emits_auth_required_for_oauth_continuation --libcargo test insert_and_notify_pending_gate_sends_status_no_text --libcargo test accumulator_gate_required_marks_failed --libcargo test discard_engine_pending_auth_request_discards_only_matching_auth_gate --libcargo test discard_engine_pending_auth_request_matches_scope_thread_id --libcargo test v2_only_structured_submissions_do_not_switch_threads_when_engine_v2_disabled --libPairing / runtime rollback tests:
cargo test propagate_approval_restores_runtime_state_when_on_start_fails --libcargo test test_upsert_and_approve_pairing --lib --no-default-features --features libsqlcargo test test_pairing_approve_claims_code_for_authenticated_user --lib --features libsqlcargo test test_pairing_approve_does_not_inject_followup_agent_turn --lib --features libsqlcargo test test_pairing_approve_does_not_inject_followup_agent_turn_without_thread --lib --features libsqlcargo test test_pairing_approve_injects_ready_followup_for_active_thread --lib --features libsqlWeb / setup / responses / metadata tests:
cargo test test_extension_setup_request_deserialize_with_fields --libcargo test test_extensions_setup_submit_returns_failure_when_not_activated --libcargo test test_chat_approval_handler_preserves_user_scoped_metadata --libcargo test test_handle_client_approval_approve --libcargo test test_handle_client_message_sends_to_agent --libE2E tests run during the series:
tests/e2e/.venv/bin/pytest -q tests/e2e/scenarios/test_channel_pairing_flow.pytests/e2e/.venv/bin/pytest -q tests/e2e/scenarios/test_telegram_hot_activation.pytests/e2e/.venv/bin/pytest -q tests/e2e/scenarios/test_v2_kernel_auth_gateway_flow.pytests/e2e/.venv/bin/pytest -q tests/e2e/scenarios/test_auth_no_duplicate_response.pytests/e2e/.venv/bin/pytest -q tests/e2e/scenarios/test_v2_auth_oauth_matrix.py -k test_wasm_tool_first_chat_auth_attempt_emits_auth_urlFollow-up work
Not part of this PR, but still worth doing: