fix: owner_id was stored as string when recovered from settings on restart by gagdiez · Pull Request #2561 · nearai/ironclaw

gagdiez · 2026-04-17T01:24:07Z

Summary

Telegram channel stopped working on staging
The issue is that we correctly store the owner-id (after pairing) as u64, but then recover it as a string, and that makes the channel unable to start:

00: 45:07.702 ERROR Failed to start channel telegram: Channel telegram failed to start: Channel telegram callback failed: Failed to parse config: invalid type: string "176326495", expected 164 at line 1 column 47

This PR fixes this by correctly casting the stored value as u64 using an already existing function
Regression test added

Change Type

Validation

cargo fmt --all -- --check
cargo clippy --all --benches --tests --examples --all-features -- -D warnings
cargo build
Relevant tests pass:
cargo test --features integration if database-backed or integration behavior changed
Manual testing: I discovered this because my telegram stopped working, this fixes it
If a coding agent was used and supports it, review-pr or pr-shepherd --fix was run before requesting review

Security Impact

None

Database Impact

None

Blast Radius

Fixes telegram channel

Review track: A (small fix)

…start Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request modifies the register_channel function in src/channels/wasm/setup.rs to attempt parsing the owner_id as an integer before defaulting to a string representation in the configuration updates. The test suite has been updated to reflect this change by asserting the owner_id as a numeric value. I have no feedback to provide.

Copilot

Pull request overview

Fixes a WASM channel startup failure caused by injecting owner_id into channel runtime config as a JSON string instead of a JSON number (notably impacting Telegram after restart when owner_id is recovered from settings).

Changes:

Coerce owner_id into a numeric serde_json::Value when the resolved owner ID is parseable as an integer; otherwise keep it as a string.
Update the affected test assertion to expect a numeric JSON value for owner_id.

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

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gagdiez · 2026-04-17T01:38:35Z

fixes an elusive bug introduced in #2515

cc @henrypark133

henrypark133 · 2026-04-17T03:52:53Z

Thank you so much for catching/fixing this! Really appreciate it

henrypark133

Review: telegram owner_id recovery fix looks sound

I verified the checked-out change in src/channels/wasm/setup.rs:227: runtime config now goes through build_runtime_config_updates(...), so a recovered numeric owner_id stays a JSON number instead of being stringified on restart. The added regression test covers the settings-recovery case that broke Telegram startup.

Residual risk: I did not rerun the full WASM/channel suite in this pass; review here is based on code inspection plus the focused regression coverage in the diff.

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`.

…variants - Expand UserRole to {Owner, Admin, Regular} - UserId carries role; methods is_owner()/is_admin()/is_regular() - Remove From<String>/From<&str> impls (enforces types.md rule) - Validated construction via new(); from_trusted() for DB-sourced values Addresses bug pattern from #2561, #2620, #2349 where owner_id silently round-tripped as String.

…variants (#2677) * refactor(ownership): collapse OwnerId+Identity into UserId with role variants - Expand UserRole to {Owner, Admin, Regular} - UserId carries role; methods is_owner()/is_admin()/is_regular() - Remove From<String>/From<&str> impls (enforces types.md rule) - Validated construction via new(); from_trusted() for DB-sourced values Addresses bug pattern from #2561, #2620, #2349 where owner_id silently round-tripped as String. * refactor(ownership): address review feedback — id-only equality, persist owner role, doc fixes - UserId PartialEq/Eq/Hash now compare only `id`, not `role`. Role is metadata that travels with the identity; two UserIds with the same id but different roles must be interchangeable as HashMap/HashSet keys and cache lookup targets. Added a regression test that builds a HashSet keyed on UserId and asserts cross-role `.contains()` membership, plus a hash-equality check. - CLI pairing path now persists the "owner" role string (via UserRole::Owner.as_db_role()) instead of the hardcoded "admin", so a reload through UserRole::from_db_role stays Owner rather than being silently downgraded to Admin. - Update the feature/pairing approve handler to mirror the refactor: build UserId via from_trusted + UserRole::from_db_role(&user.role) instead of the removed OwnerId::from. - AdminScope doc comment now reflects that Owner also passes is_admin(). - AdminUser extractor error message now reads "Admin privileges required (admin or owner)" so the forbidden response matches the actual gate. --------- Co-authored-by: Henry Park <henrypark133@gmail.com>

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>

…start (nearai#2561) * fix: owner_id was stored as string when recovered from settings on restart Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add regression test for owner_id type on restart Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: use existing function --------- Co-authored-by: Guillermo Alejandro Gallardo Diez <gagdiez@iR2.local> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…i#2611) * 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 (nearai#2561, nearai#2473, nearai#2512, nearai#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 nearai#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.

…earai#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 (nearai#2561, nearai#2473, nearai#2512, nearai#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 nearai#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 nearai#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 nearai#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 nearai#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 nearai#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 (nearai#2561, nearai#2473, nearai#2512, nearai#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 nearai#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 nearai#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 nearai#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 nearai#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 (nearai#2561, nearai#2473, nearai#2512, nearai#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 nearai#2644 (platform/ extraction) and nearai#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 nearai#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 nearai#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 nearai#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`.

…variants (nearai#2677) * refactor(ownership): collapse OwnerId+Identity into UserId with role variants - Expand UserRole to {Owner, Admin, Regular} - UserId carries role; methods is_owner()/is_admin()/is_regular() - Remove From<String>/From<&str> impls (enforces types.md rule) - Validated construction via new(); from_trusted() for DB-sourced values Addresses bug pattern from nearai#2561, nearai#2620, nearai#2349 where owner_id silently round-tripped as String. * refactor(ownership): address review feedback — id-only equality, persist owner role, doc fixes - UserId PartialEq/Eq/Hash now compare only `id`, not `role`. Role is metadata that travels with the identity; two UserIds with the same id but different roles must be interchangeable as HashMap/HashSet keys and cache lookup targets. Added a regression test that builds a HashSet keyed on UserId and asserts cross-role `.contains()` membership, plus a hash-equality check. - CLI pairing path now persists the "owner" role string (via UserRole::Owner.as_db_role()) instead of the hardcoded "admin", so a reload through UserRole::from_db_role stays Owner rather than being silently downgraded to Admin. - Update the feature/pairing approve handler to mirror the refactor: build UserId via from_trusted + UserRole::from_db_role(&user.role) instead of the removed OwnerId::from. - AdminScope doc comment now reflects that Owner also passes is_admin(). - AdminUser extractor error message now reads "Admin privileges required (admin or owner)" so the forbidden response matches the actual gate. --------- Co-authored-by: Henry Park <henrypark133@gmail.com>

…#2347) * fix(gate): handle orphaned approval gates when thread is deleted (nearai#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 nearai#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 nearai#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 nearai#2561/nearai#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>

fix: owner_id was stored as string when recovered from settings on re…

c07bb62

…start Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gagdiez requested review from Copilot and serrrfirat April 17, 2026 01:24

github-actions Bot added scope: channel/wasm WASM channel runtime size: S 10-49 changed lines labels Apr 17, 2026

gagdiez requested a review from henrypark133 April 17, 2026 01:24

github-actions Bot added risk: medium Business logic, config, or moderate-risk modules contributor: regular 2-5 merged PRs labels Apr 17, 2026

Copilot started reviewing on behalf of gagdiez April 17, 2026 01:24 View session

gemini-code-assist Bot reviewed Apr 17, 2026

View reviewed changes

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Comment thread src/channels/wasm/setup.rs Outdated

Comment thread src/channels/wasm/setup.rs Outdated

Guillermo Alejandro Gallardo Diez and others added 2 commits April 17, 2026 03:31

test: add regression test for owner_id type on restart

3f31606

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

chore: use existing function

aab213a

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

henrypark133 approved these changes Apr 17, 2026

View reviewed changes

henrypark133 merged commit fe5cdce into nearai:staging Apr 17, 2026
14 checks passed

github-actions Bot mentioned this pull request Apr 17, 2026

chore: promote staging to staging-promote/3ac8e5f7-24538841293 (2026-04-17 04:57 UTC) #2565

Merged

gagdiez deleted the fix/telegram-owner-id-type branch April 17, 2026 09:03

ilblackdragon mentioned this pull request Apr 18, 2026

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

Merged

7 of 8 tasks

ilblackdragon mentioned this pull request Apr 19, 2026

refactor(ownership): collapse OwnerId+Identity into UserId with role variants #2677

Merged

5 of 6 tasks

ilblackdragon mentioned this pull request Apr 20, 2026

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

Merged

3 of 4 tasks

ilblackdragon mentioned this pull request May 10, 2026

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

Merged

zmanian mentioned this pull request May 17, 2026

engine v2: expose channel-supplied thread/response ids to tools #3669

Closed

10 tasks done

zetyquickly mentioned this pull request Jun 2, 2026

engine v2: newtype client_thread_id / client_response_id on ThreadExecutionContext (Z2 follow-up from #3669) #4355

Closed

This was referenced Jun 18, 2026

fix(reborn): NEAR AI MCP ready state projection #4990

Merged

feat(admin): persist Engine V2 LLM usage #4989

Merged

claude Bot mentioned this pull request Jul 9, 2026

fix(webui-v2): render chat errors inline #5822

Merged

Navigation Menu

fix: owner_id was stored as string when recovered from settings on restart - #2561

fix: owner_id was stored as string when recovered from settings on restart#2561
henrypark133 merged 3 commits into
nearai:stagingfrom
gagdiez:fix/telegram-owner-id-type

gagdiez commented Apr 17, 2026 •

edited

Loading

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

gagdiez commented Apr 17, 2026

Uh oh!

henrypark133 commented Apr 17, 2026

Uh oh!

henrypark133 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gagdiez commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type

Validation

Security Impact

Database Impact

Blast Radius

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

gagdiez commented Apr 17, 2026

Uh oh!

henrypark133 commented Apr 17, 2026

Uh oh!

henrypark133 left a comment

Choose a reason for hiding this comment

Review: telegram owner_id recovery fix looks sound

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gagdiez commented Apr 17, 2026 •

edited

Loading