fix: use gateway owner_id for relay OAuth nonce storage by henrypark133 · Pull Request #2473 · nearai/ironclaw

henrypark133 · 2026-04-14T20:27:47Z

Summary

Relay OAuth nonce was stored under the DB user's UUID but the callback handler looked it up under state.owner_id ("default"), causing every Slack OAuth callback to fail with "Invalid or expired authorization"
Changed auth_channel_relay to use self.user_id (which holds config.owner_id) for nonce storage, matching the callback handler's lookup scope
Added tracing::warn! to the callback handler's get_decrypted error path for future diagnosability

Root cause: CrabShack hosted instances don't set IRONCLAW_OWNER_ID, so it defaults to "default". But DB-authenticated users have UUID IDs. The nonce was stored under the UUID but retrieved under "default".

Test plan

cargo test --lib channels::web::server::tests::test_relay_oauth_callback — 3/3 pass
cargo clippy --lib -p ironclaw — clean
cargo fmt — clean
Deploy to staging, retry Slack OAuth flow end-to-end

🤖 Generated with Claude Code

The relay OAuth nonce was stored under the authenticated user's ID (a DB user UUID) but the callback handler looked it up under state.owner_id (the gateway owner, typically "default"). This mismatch caused the nonce lookup to silently fail, returning "Invalid or expired authorization" on every Slack OAuth callback. Use self.user_id (which holds config.owner_id) in auth_channel_relay for nonce storage so both sides use the same scope. Also adds tracing to the callback handler's get_decrypted error path to make future auth failures diagnosable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes Slack channel-relay OAuth failures in hosted gateway deployments by aligning where the CSRF nonce is stored (during OAuth initiation) with where it is later retrieved (during the public OAuth callback).

Changes:

Store the relay OAuth nonce under the gateway owner scope (self.user_id / state.owner_id) instead of the DB-authenticated caller’s UUID.
Add a tracing::warn! log on nonce retrieval failure in the Slack relay OAuth callback handler.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`src/extensions/manager.rs`	Stores the relay OAuth CSRF nonce under the gateway owner ID so callback lookup succeeds.
`src/channels/web/server.rs`	Adds warning log context when nonce retrieval fails during the public OAuth callback.

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

Verifies that a nonce stored under a DB user UUID (different from the gateway owner_id) is not found by the callback handler, reproducing the bug that caused "Invalid or expired authorization" on hosted instances. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request improves error logging in the Slack relay OAuth callback and ensures that OAuth state secrets are managed using the gateway owner's ID instead of the caller's ID. The review feedback highlights an opportunity to include more context in the warning logs via a redacted state parameter and identifies a potential race condition where concurrent authentication attempts could overwrite the shared state nonce.

- Also delete legacy caller-scoped nonce on upgrade (Copilot) - Include redacted state param in tracing log (Gemini) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

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

Introduce typed identifiers for the backend-secret vs user-facing extension identity split that the Extension/Auth Invariants section of CLAUDE.md describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity- confusion bugs with the same shape: a stringly-typed value passed through multiple layers with each layer meaning a different thing. Newtypes make each of those a compile error. This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam (ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth, LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the remaining extension_name: String fields. Wire format is unchanged — both newtypes use #[serde(transparent)] so on- wire and on-disk representations stay plain strings and legacy persisted rows keep deserializing. Validation runs at explicit construction (::new / ::try_from / ::from_str), not at deserialize time. Also adds .claude/rules/types.md codifying the "no stringly-typed internals" rule. Regression coverage: 17 new unit tests in identity.rs; existing auth_manager, router, and gate tests (130+ cases) all pass unchanged.

* feat(common): add CredentialName and ExtensionName newtypes Introduce typed identifiers for the backend-secret vs user-facing extension identity split that the Extension/Auth Invariants section of CLAUDE.md describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity- confusion bugs with the same shape: a stringly-typed value passed through multiple layers with each layer meaning a different thing. Newtypes make each of those a compile error. This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam (ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth, LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the remaining extension_name: String fields. Wire format is unchanged — both newtypes use #[serde(transparent)] so on- wire and on-disk representations stay plain strings and legacy persisted rows keep deserializing. Validation runs at explicit construction (::new / ::try_from / ::from_str), not at deserialize time. Also adds .claude/rules/types.md codifying the "no stringly-typed internals" rule. Regression coverage: 17 new unit tests in identity.rs; existing auth_manager, router, and gate tests (130+ cases) all pass unchanged. * fix(common): address PR #2611 review feedback Four fixes from Copilot, Gemini, and Claude reviews: - **identity.rs docs**: drop reference to a non-existent `validate()` re-validation API. Document that instances represent "passed validation at some point in history" rather than "guaranteed valid right now" — by design. - **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token` gate path was using `CredentialName::from_trusted` to wrap a value read straight out of a tool's JSON output. Tool output is external/untrusted; use `CredentialName::new` (validating) with a cascade: external → tool name → `from_trusted(tool_name)` as final fallback. Closes a credential-name shape-injection vector. - **canonicalize()**: reorder checks cheapest-first against the trimmed slice so invalid inputs reject without allocating a canonicalized `String`. `replace('-', "_")` is deferred until after the structural checks pass; since `-`/`_` are both one byte, the earlier length check stays valid. - **Remove `Deref<Target = str>`** from identity newtypes, keep `AsRef<str>`. Auto-deref let `&cred_name` silently coerce to `&str`, which is exactly the implicit-conversion pattern these newtypes exist to prevent. Callers that had a `&CredentialName` where `&str` was expected now write `.as_str()` explicitly. Added a regression test for the accessor contract and updated the rule template in `.claude/rules/types.md` to document the decision. Declined one review item (Claude): the remaining `to_string()` calls inside `IdentityError` variants are on the exception path; the common invalid-input case no longer allocates twice after the canonicalize reorder, and errors must carry owned strings so they can escape the function. Regression coverage: 5035 lib tests + 18 identity tests (one new — `explicit_accessors_work`) pass. Zero clippy warnings.

Three rule additions + one enforcement hook covering the identity boundary that PR #2617 review uncovered: - src/channels/web/CLAUDE.md — extend "Unified Extension Onboarding" with explicit rules: * Setup/configure/activate routes MUST validate `{name}` via `ExtensionName::new` at handler entry (return 400 on failure). * Web DTOs and handlers MUST NOT reference `CredentialName` — credential identity is backend-only; the dispatcher/auth_manager resolves it from the ExtensionName server-side. * Auth-flow extension resolution happens in *one* place (`AuthManager::resolve_extension_name_for_auth_flow`). Wrappers are thin and delegate; they must not duplicate the precedence logic or re-derive from credential prefixes. The four recent identity bugs (#2561, #2473, #2512, #2574) were duplicate- resolution drift. - src/bridge/CLAUDE.md — new module spec documenting auth_manager.rs as the single authority for auth-flow extension resolution, with the resolver's four-step precedence order and the approved wrapper call sites. - scripts/pre-commit-safety.sh — new check #8 (CREDNAME): flags `CredentialName` references in newly-added production lines under `src/channels/web/**`. Test-mod code is excluded via the existing `strip_test_mod_lines` filter. Suppression via `// web-identity-exempt: <reason>` for the rare legitimate case of reading an already-typed value off a backend struct. Smoke-tested: * baseline (current branch) — no warnings * injected violation — fires with CREDNAME warning * injected violation + `// web-identity-exempt:` — suppressed The rules and the check live at the same level — humans read the rule, CI enforces it.

…icating Addresses two Copilot comments on PR #2617 that surfaced the same architectural issue: the no-auth-manager fallback in `pending_gate_extension_name` had grown a three-branch copy of the resolver's precedence that quietly skipped branch 3 (canonicalize action_name + check `ExtensionManager::extension_info`). Exactly the duplicate-resolution drift the "one resolver" rule in `src/bridge/CLAUDE.md` warns against — four prior identity bugs (#2561, #2473, #2512, #2574) were the same pattern. - Extracted `pub(crate) async fn resolve_auth_flow_extension_name` to `src/bridge/auth_manager.rs` as the single site of the four-branch precedence. Takes `Option<&ToolRegistry>` + `Option<&ExtensionManager>` so both the `AuthManager` method (which passes its own fields) and the web wrapper (which passes `state.tool_registry` / `state.extension_manager`) share identical logic. - `AuthManager::resolve_extension_name_for_auth_flow` is now a 1-block delegator. - `pending_gate_extension_name` in `web/server.rs` drops its inline fallback entirely and calls the shared free function from both branches. The bare-test-harness path now runs branch 3 (canonicalize + installed-extension check) that it previously missed. - Updated `src/bridge/CLAUDE.md` to document the free function as the single authority, the three approved wrappers as thin delegators, and the return type as `ExtensionName` (was stale `String` from the pre-c813caa9 era). Regression coverage: the existing `resolve_extension_name_for_auth_flow_prefers_installed_channel_name` test passes unchanged — it exercises branch 3 through the method, which now reaches it via the extracted free function.

…2617) * feat(common): add CredentialName and ExtensionName newtypes Introduce typed identifiers for the backend-secret vs user-facing extension identity split that the Extension/Auth Invariants section of CLAUDE.md describes. Four recent PRs (#2561, #2473, #2512, #2574) have been identity- confusion bugs with the same shape: a stringly-typed value passed through multiple layers with each layer meaning a different thing. Newtypes make each of those a compile error. This is PR 1 of 2. PR 1 lands the newtypes and migrates the core auth seam (ResumeKind::Authentication, MissingCredential, ToolReadiness::NeedsAuth, LatentActionExecution::NeedsAuth, extensions/naming.rs). PR 2 will migrate AppEvent.extension_name, OAuth/pending-flow stores, TUI events, and the remaining extension_name: String fields. Wire format is unchanged — both newtypes use #[serde(transparent)] so on- wire and on-disk representations stay plain strings and legacy persisted rows keep deserializing. Validation runs at explicit construction (::new / ::try_from / ::from_str), not at deserialize time. Also adds .claude/rules/types.md codifying the "no stringly-typed internals" rule. Regression coverage: 17 new unit tests in identity.rs; existing auth_manager, router, and gate tests (130+ cases) all pass unchanged. * fix(common): address PR #2611 review feedback Four fixes from Copilot, Gemini, and Claude reviews: - **identity.rs docs**: drop reference to a non-existent `validate()` re-validation API. Document that instances represent "passed validation at some point in history" rather than "guaranteed valid right now" — by design. - **effect_adapter.rs**: the `awaiting_authorization` / `awaiting_token` gate path was using `CredentialName::from_trusted` to wrap a value read straight out of a tool's JSON output. Tool output is external/untrusted; use `CredentialName::new` (validating) with a cascade: external → tool name → `from_trusted(tool_name)` as final fallback. Closes a credential-name shape-injection vector. - **canonicalize()**: reorder checks cheapest-first against the trimmed slice so invalid inputs reject without allocating a canonicalized `String`. `replace('-', "_")` is deferred until after the structural checks pass; since `-`/`_` are both one byte, the earlier length check stays valid. - **Remove `Deref<Target = str>`** from identity newtypes, keep `AsRef<str>`. Auto-deref let `&cred_name` silently coerce to `&str`, which is exactly the implicit-conversion pattern these newtypes exist to prevent. Callers that had a `&CredentialName` where `&str` was expected now write `.as_str()` explicitly. Added a regression test for the accessor contract and updated the rule template in `.claude/rules/types.md` to document the decision. Declined one review item (Claude): the remaining `to_string()` calls inside `IdentityError` variants are on the exception path; the common invalid-input case no longer allocates twice after the canonicalize reorder, and errors must carry owned strings so they can escape the function. Regression coverage: 5035 lib tests + 18 identity tests (one new — `explicit_accessors_work`) pass. Zero clippy warnings. * feat(common): apply ExtensionName newtype to fan-out sites (PR 2/2) Follow-up to #2611. Migrates the remaining stringly-typed extension_name and credential_name fields to use the ExtensionName and CredentialName newtypes introduced in ironclaw_common::identity. Fields now typed: - AppEvent::{OnboardingState, GateRequired, ExtensionStatus}.extension_name (serde transparent — wire format unchanged) - StatusUpdate::{AuthRequired, AuthCompleted}.extension_name - TuiEvent::{AuthRequired, AuthCompleted}.extension_name (adds ironclaw_common dep to ironclaw_tui) - PendingOAuthLaunchParams.extension_name - PendingOAuthFlow.extension_name - PendingAuth.extension_name, PendingAuthPrompt.extension_name - ParsedAuthData.extension_name, selected_auth_prompt tuple - emit_auth_required_status() and Session::enter_auth_mode() parameters - event_from_configure_result() parameter - resolve_extension_for_action() and resolve_auth_gate_display_name() return types - normalize_extension_name() return type PendingAuthPrompt::new is now infallible (accepts ExtensionName directly) since the identity validator carries the non-empty invariant the constructor used to re-check. The "blank extension name" rejection test moved out — that logic lives in ironclaw_common::identity tests. Test updates use `ExtensionName::new("...").unwrap()` at construction sites and `from_trusted(...)` where a trusted upstream string is being adapted. Every site is a compile-time audit of where the type was crossing a boundary untyped. Regression coverage: existing 5034 lib tests + 26 engine_v2_gate integration tests + 40 ironclaw_common tests all pass. Zero clippy warnings across all features. * fix(web): return ExtensionName from pending_gate_extension_name Addresses Claude's review comment on #2611: the function was doing `Some(credential_name.as_str().to_string())` in the fallback branch, defeating the newtype's purpose by re-stringifying the identity. Return `Option<ExtensionName>` instead. Plumbs through `PendingGateInfo. extension_name` (wire format unchanged — `#[serde(transparent)]`). The fallback path's cross-identity conversion (credential name → extension name) is now an explicit `ExtensionName::from_trusted` call, making the boundary crossing visible at the call site. Also fixes the `Deref<Target = str>` removal fallout that followed the rebase onto the updated PR 1: call sites that relied on auto-deref (`ext.contains(...)`, `auth_manager.submit_auth_token(&cred_name, ...)`) now explicitly call `.as_str()`. * fix(router,web): address PR #2617 review feedback Four Gemini review comments, all on the boundary between credential/ extension identifiers and user input. 1. [HIGH, security] extensions_setup_submit_handler was wrapping the URL path segment in ExtensionName::from_trusted, which skips the newtype's path-traversal / invalid-character validation. That path is user-controlled (`/api/extensions/{name}/setup`). Validate with ExtensionName::new at the handler entry and return 400 on failure; downstream uses switch to .as_str() or .clone() of the validated value, and the three in-handler from_trusted sites disappear. 2. Rename resolve_auth_gate_display_name -> resolve_auth_gate_extension_name. The function returns an identifier/slug, not a human-readable display name — the old name was a leftover from when the value was a String. 3. Return Option<ExtensionName> from the renamed function. Previously the non-Authentication gate branch fabricated an ExtensionName::from_trusted(pending.action_name), which was semantically wrong (an action name is not an extension identifier) and silently defeated the type's invariants. Now it returns None for Approval/External gates, and callers thread an Option through. send_pending_gate_status accepts Option<&ExtensionName> and only uses it on the Authentication arm, with a warn! log if upstream plumbing ever reaches the arm with None. The GateRequired SSE event's extension_name is now a clean .clone() of the Option. 4. Rename auth_display_name -> extension_name on send_pending_gate_status so the parameter name matches both its type and the StatusUpdate::AuthRequired.extension_name field it feeds. Regression: new test_extensions_setup_submit_rejects_path_traversal_name at the handler tier (per .claude/rules/testing.md "Test Through the Caller, Not Just the Helper") drives the handler with malformed path segments and asserts 400 before the value reaches extension lookup or any from_trusted wrap. 5035 lib tests pass, zero clippy warnings. * docs(identity): codify web-boundary rules + add static check Three rule additions + one enforcement hook covering the identity boundary that PR #2617 review uncovered: - src/channels/web/CLAUDE.md — extend "Unified Extension Onboarding" with explicit rules: * Setup/configure/activate routes MUST validate `{name}` via `ExtensionName::new` at handler entry (return 400 on failure). * Web DTOs and handlers MUST NOT reference `CredentialName` — credential identity is backend-only; the dispatcher/auth_manager resolves it from the ExtensionName server-side. * Auth-flow extension resolution happens in *one* place (`AuthManager::resolve_extension_name_for_auth_flow`). Wrappers are thin and delegate; they must not duplicate the precedence logic or re-derive from credential prefixes. The four recent identity bugs (#2561, #2473, #2512, #2574) were duplicate- resolution drift. - src/bridge/CLAUDE.md — new module spec documenting auth_manager.rs as the single authority for auth-flow extension resolution, with the resolver's four-step precedence order and the approved wrapper call sites. - scripts/pre-commit-safety.sh — new check #8 (CREDNAME): flags `CredentialName` references in newly-added production lines under `src/channels/web/**`. Test-mod code is excluded via the existing `strip_test_mod_lines` filter. Suppression via `// web-identity-exempt: <reason>` for the rare legitimate case of reading an already-typed value off a backend struct. Smoke-tested: * baseline (current branch) — no warnings * injected violation — fires with CREDNAME warning * injected violation + `// web-identity-exempt:` — suppressed The rules and the check live at the same level — humans read the rule, CI enforces it. * fix(auth): validate user-influenced names at the resolver boundary Addresses four Copilot review comments on PR #2617 that all pointed at the same seam: the canonical `AuthManager::resolve_extension_name_for_auth_flow` returned a raw `String` whose first branch (the LLM-supplied `name` parameter on `tool_install` / `tool_activate` / `tool_auth` actions) passed through without `ExtensionName` validation. Both call sites then wrapped the result in `ExtensionName::from_trusted`, promoting an unvalidated user-influenced value to a typed identity. - **Resolver now returns `ExtensionName`.** Branch 1 validates the user-controlled name via `ExtensionName::new` and falls through on failure; branches 2–4 use `from_trusted` because their sources (tool registry hint, canonicalizer, typed credential fallback) are already trusted upstream. This consolidates validation in the single "resolve once" site documented in `src/bridge/CLAUDE.md`. - **router.rs and server.rs drop their wraps.** `resolve_extension_for_action` (router) and `pending_gate_extension_name` (server) return the resolver's typed output directly. The tool-registry fallback in router.rs (no-auth-manager path) keeps its `from_trusted` wrap since it operates on the same trusted sources as branch 2. - **`restore_selected_auth_prompt` re-validates rehydrated prompts.** `PendingAuthPrompt` is `#[serde(transparent)]`, so deserialize does not re-check the inner `ExtensionName` string. A legacy-persisted invalid name would previously have been dropped by the old `PendingAuthPrompt::new(String, ...)` empty-string rejection; now `restore_selected_auth_prompt` re-runs `ExtensionName::new` and drops + warns on failure, upgrading the old non-empty-only check to the full identity invariant. New test `test_restore_selected_auth_prompt_rejects_invalid_legacy_row` forges three invalid rows (empty / uppercase / path-traversal) straight through serde and asserts each is dropped. - **Docstring on `PendingAuthPrompt` refreshed.** The old comment claimed `::new` "trims and validates extension_name is non-empty", which is no longer true — `::new` is infallible and the invariant lives in `ExtensionName` itself. The new comment documents the split: validation runs at `ExtensionName::new` construction and at restore-from-persistence, not inside `PendingAuthPrompt`. Regression: 5063 lib tests pass (+1 new). Clippy zero warnings. * fix(ci): adapt post-merge-from-staging sites to ExtensionName Staging shipped #2640 (repl unlock) and gateway refactor commits after my last merge. The CI build picked them up via auto-merge and hit three type mismatches my branch hadn't seen: - src/channels/repl.rs:908 — new test constructs `StatusUpdate::AuthRequired { extension_name: "google_oauth_token" .to_string(), ... }`. Typed field; now `ExtensionName::new(...).unwrap()`. - src/channels/web/server.rs:1405-1424 — staging added a no-auth-manager fallback chain to `pending_gate_extension_name` that returned raw `Some(String)` on three branches. Aligned with `AuthManager::resolve_extension_name_for_auth_flow`: branch 1 (user-influenced `tool_install`/`tool_activate`/`tool_auth` `name` param) validates via `ExtensionName::new` and falls through on failure; branches 2-3 (provider-extension hint, credential-name fallback) use `from_trusted` because they're sourced from typed upstream state. Mirrors the fix applied to the canonical resolver in c813caa. - src/channels/web/server.rs:3831 — test used `.as_deref()` on the function's Option<ExtensionName> return; switched to `.as_ref().map(|n| n.as_str())` matching the pattern from the adjacent test. No new logic — just adapting two staging landings to the typed surface PR #2617 introduces. The validation behaviour for the fallback path is already locked in by the identity-layer tests in `ironclaw_common::identity` (rejects_path_traversal, rejects_uppercase, etc.) and by the regression test added in c813caa (test_restore_selected_auth_prompt_rejects_invalid_legacy_row). [skip-regression-check] — type adaptation to unblock CI, no behaviour change needing its own regression test. Clippy with `-D warnings` clean, 5074 lib tests pass. * fix(auth): extract shared resolver; wrapper delegates instead of duplicating Addresses two Copilot comments on PR #2617 that surfaced the same architectural issue: the no-auth-manager fallback in `pending_gate_extension_name` had grown a three-branch copy of the resolver's precedence that quietly skipped branch 3 (canonicalize action_name + check `ExtensionManager::extension_info`). Exactly the duplicate-resolution drift the "one resolver" rule in `src/bridge/CLAUDE.md` warns against — four prior identity bugs (#2561, #2473, #2512, #2574) were the same pattern. - Extracted `pub(crate) async fn resolve_auth_flow_extension_name` to `src/bridge/auth_manager.rs` as the single site of the four-branch precedence. Takes `Option<&ToolRegistry>` + `Option<&ExtensionManager>` so both the `AuthManager` method (which passes its own fields) and the web wrapper (which passes `state.tool_registry` / `state.extension_manager`) share identical logic. - `AuthManager::resolve_extension_name_for_auth_flow` is now a 1-block delegator. - `pending_gate_extension_name` in `web/server.rs` drops its inline fallback entirely and calls the shared free function from both branches. The bare-test-harness path now runs branch 3 (canonicalize + installed-extension check) that it previously missed. - Updated `src/bridge/CLAUDE.md` to document the free function as the single authority, the three approved wrappers as thin delegators, and the return type as `ExtensionName` (was stale `String` from the pre-c813caa9 era). Regression coverage: the existing `resolve_extension_name_for_auth_flow_prefers_installed_channel_name` test passes unchanged — it exercises branch 3 through the method, which now reaches it via the extracted free function. * Merge remote-tracking branch 'origin/staging' into feat/identity-newtypes-pr2 Picks up #2644 (platform/ extraction) and #2645 (features/oauth/ move). Manual resolutions: - src/channels/web/server.rs: staging removed 720 lines of OAuth callback code (moved to features/oauth/mod.rs in #2645). My PR 2 ExtensionName changes to two of those functions (oauth_callback_handler, slack_relay_oauth_callback_handler) ported to the new location. - src/bridge/auth_manager.rs: extended the shared resolver's branch-1 action pattern to include 'tool-activate' and 'tool-auth' variants, matching staging's new pending_gate_extension_name_uses_install_parameters_for_hyphenated_activate_tool test expectation. Underscore + hyphen variants for all three actions. No new PR 2 logic — just aligning the type surface with two staging refactors. 5074 lib tests pass (+1 vs previous — the new staging hyphenated-tool test). Clippy -D warnings clean. * fix(web): address PR #2617 round-3 review feedback Two Copilot findings from the 2026-04-18 review: 1. `/api/extensions/{name}/{activate,remove,setup}` handlers accepted `Path<String>` and forwarded it to the extension manager without validating path-traversal, invalid characters, or case — only `extensions_setup_submit_handler` had the `ExtensionName::new` guard. Applied the same boundary validation to all three siblings. 2. `restore_pending_auth_mode` took `extension_name: &str` and re-wrapped it with `ExtensionName::from_trusted`, re-introducing an unvalidated string boundary even though every caller already held an `ExtensionName` (`pending_auth.extension_name`). Changed the helper to accept `&ExtensionName` so the identity stays typed end-to-end; `from_trusted` is no longer needed here. Regression: added `test_extensions_sibling_handlers_reject_path_traversal_name` covering activate / remove / setup-GET with the same malformed slugs the setup-submit test already locks in (path traversal, slash in segment, uppercase, space, trailing underscore). Drives the handlers through axum routing so the boundary is exercised end-to-end. * fix(ci): adapt replay_outcome to ExtensionName after staging merge Staging #2621 added `tests/support/replay_outcome.rs`, which destructures `StatusUpdate::{AuthRequired,AuthCompleted}.extension_name` into a `String` field of `EventSummary`. This PR made those `StatusUpdate` fields `ExtensionName`, so the post-merge build breaks in the replay snapshot gate and all-features clippy jobs. Convert to `String` at the destructure via `ExtensionName::into()` so the `EventSummary` shape (and the persisted `.snap` files) stay unchanged. The test-support / snapshot wire format is a legitimate String boundary per `.claude/rules/types.md`.

Address review feedback on #2347: 1. **codex P2 — AlwaysAllow silent commit on thread-delete race.** When `resolve_gate` hit `Approved { always: true }`, it persisted `AlwaysAllow` to DB *before* calling `execute_pending_gate_action`. The rollback branch only fires on `result.is_err()`, but the thread-missing path now returns `Ok(BridgeOutcome::Respond(...))` for graceful `expired` UX — so the preference would stick even though the tool never ran. Added a pre-flight `load_thread` check in the `Approved` arm that short-circuits with `emit_gate_expired_dismissal` before any auto-approve / DB write occurs. 2. **Rebase onto staging (`scope_thread_id: Option<ExternalThreadId>`).** The PR predated #2561/#2473's identity-type tightening. The inline `pending.scope_thread_id.clone().or_else(|| Some(pending.thread_id.to_string()))` no longer type-checks against `Option<String>` on the wire field. Replaced with `Some(pending.effective_wire_thread_id())` — matches the five other `GateResolved` emit sites and satisfies the "don't re-derive identity values" invariant in `src/bridge/CLAUDE.md`. 3. **Extracted `emit_gate_expired_dismissal`** so the expired-SSE path is shared between the pre-flight check and `execute_pending_gate_action`'s `Ok(None)` arm. Documented the pre-flight contract on the helper itself. 4. **Regression test** `resolve_gate_approved_with_missing_thread_emits_expired_and_skips_persist` drives `resolve_gate` at the caller level with `Approved { always: true }` and no thread in the store. Asserts the first SSE event is `expired` (not `approved_always`), satisfying `.claude/rules/testing.md` → "Test Through the Caller, Not Just the Helper". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(gate): handle orphaned approval gates when thread is deleted (#2323) When a thread is deleted while an approval gate is pending, the gate becomes orphaned -- unresolvable on backend and undismissable on frontend. This fixes three root causes: 1. execute_pending_gate_action now detects missing threads and emits a gate_resolved event with resolution "expired" instead of erroring, so the frontend can dismiss the stale card. 2. PendingGateStore gains discard_for_thread() for bulk cleanup of all gates tied to a specific thread, regardless of user. 3. Frontend handleGateResolved now handles "expired" resolution to remove stale approval cards and re-enable chat input. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(gate): split Ok(None)/Err(_) arms and wire discard_for_thread into production Address review feedback on #2347: 1. Split the conflated `Ok(None) | Err(_)` match arm in `execute_pending_gate_action`: `Ok(None)` means the thread was genuinely deleted (emit "expired" gate resolution), while `Err(_)` indicates a transient DB failure (propagate the error so the caller can retry instead of permanently discarding the gate). 2. Wire `discard_for_thread()` into the conversation clear path (`clear_engine_conversation`), replacing the per-user `discard()` call. This ensures all pending gates for a thread are cleaned up regardless of which user created them, preventing orphaned gates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(gate): pre-flight thread check before persisting AlwaysAllow Address review feedback on #2347: 1. **codex P2 — AlwaysAllow silent commit on thread-delete race.** When `resolve_gate` hit `Approved { always: true }`, it persisted `AlwaysAllow` to DB *before* calling `execute_pending_gate_action`. The rollback branch only fires on `result.is_err()`, but the thread-missing path now returns `Ok(BridgeOutcome::Respond(...))` for graceful `expired` UX — so the preference would stick even though the tool never ran. Added a pre-flight `load_thread` check in the `Approved` arm that short-circuits with `emit_gate_expired_dismissal` before any auto-approve / DB write occurs. 2. **Rebase onto staging (`scope_thread_id: Option<ExternalThreadId>`).** The PR predated #2561/#2473's identity-type tightening. The inline `pending.scope_thread_id.clone().or_else(|| Some(pending.thread_id.to_string()))` no longer type-checks against `Option<String>` on the wire field. Replaced with `Some(pending.effective_wire_thread_id())` — matches the five other `GateResolved` emit sites and satisfies the "don't re-derive identity values" invariant in `src/bridge/CLAUDE.md`. 3. **Extracted `emit_gate_expired_dismissal`** so the expired-SSE path is shared between the pre-flight check and `execute_pending_gate_action`'s `Ok(None)` arm. Documented the pre-flight contract on the helper itself. 4. **Regression test** `resolve_gate_approved_with_missing_thread_emits_expired_and_skips_persist` drives `resolve_gate` at the caller level with `Approved { always: true }` and no thread in the store. Asserts the first SSE event is `expired` (not `approved_always`), satisfying `.claude/rules/testing.md` → "Test Through the Caller, Not Just the Helper". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Illia Polosukhin <ilblackdragon@gmail.com>

Copilot AI review requested due to automatic review settings April 14, 2026 20:27

github-actions Bot added scope: channel/web Web gateway channel scope: extensions Extension management size: S 10-49 changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels Apr 14, 2026

Copilot started reviewing on behalf of henrypark133 April 14, 2026 20:28 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Comment thread src/extensions/manager.rs

github-actions Bot added size: M 50-199 changed lines and removed size: S 10-49 changed lines labels Apr 14, 2026

gemini-code-assist Bot reviewed Apr 14, 2026

View reviewed changes

Comment thread src/channels/web/server.rs

Comment thread src/extensions/manager.rs

fix: address PR review comments

578289d

- Also delete legacy caller-scoped nonce on upgrade (Copilot) - Include redacted state param in tracing log (Gemini) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 14, 2026 20:42

Copilot started reviewing on behalf of henrypark133 April 14, 2026 20:43 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Comment thread src/channels/web/server.rs

nickpismenkov approved these changes Apr 14, 2026

View reviewed changes

henrypark133 merged commit 8973d1b into staging Apr 14, 2026
19 checks passed

henrypark133 deleted the fix/relay-oauth-nonce-scope branch April 14, 2026 23:20

ilblackdragon mentioned this pull request Apr 18, 2026

feat(common): CredentialName + ExtensionName newtypes (PR 1/2) #2611

Merged

8 tasks

ironclaw-ci Bot mentioned this pull request Apr 18, 2026

chore: promote staging to staging-promote/72d31b72-24600795343 (2026-04-18 09:21 UTC) #2639

Merged

ilblackdragon mentioned this pull request Apr 20, 2026

fix(gate): handle orphaned approval gates when thread deleted #2347

Merged

4 tasks

ilblackdragon mentioned this pull request May 10, 2026

feat(reborn): add text-only loop driver host factory #3439

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use gateway owner_id for relay OAuth nonce storage#2473

fix: use gateway owner_id for relay OAuth nonce storage#2473
henrypark133 merged 3 commits intostagingfrom
fix/relay-oauth-nonce-scope

henrypark133 commented Apr 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

henrypark133 commented Apr 14, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants