mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
09d3ebb3cd
* 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>