[codex] Add user-scoped skills settings UI

[serrrfirat]

Summary

Adds a Skills section to the Settings UI where users can view system/workspace skills and manage their own skills.

Key changes:

• Add Settings > Skills route, UI grouping, add/edit/delete flows, and API client hooks.
• Add backend GET/PUT/DELETE skill content endpoints plus install/update/remove support with explicit confirmation headers.
• Scope self-service skill management to the authenticated user instead of requiring admin-only access.
• Keep system/workspace skills read-only and isolate multi-tenant user skill storage outside the shared scan root.
• Add regression coverage for cross-user skill isolation and update docs for the new Settings Skills surface.

Why

The prior implementation treated skill management as global because the web gateway exposed a single shared SkillRegistry. With the current identity structure, self-service skill management should operate against the signed-in user's skill mount while preserving shared system skills as read-only.

Validation

• cargo fmt
• cargo test --lib skills_
• cargo test --lib skill_info_
• cargo test -p ironclaw_skills test_update_skill
• cargo test -p ironclaw_skills test_validate_update_rejects_workspace_and_bundled_skills
• cargo test -p ironclaw_skills test_remove_flat_user_skill_rejected
• node --check for changed Settings JS modules
• python3 -m py_compile tests/e2e/scenarios/test_skills.py
• git diff --check

Full browser e2e was not run.

[gemini-code-assist]

Code Review

This pull request enables editing and deleting user-managed skills and introduces multi-tenant scoping for skill registries. The feedback highlights several performance and robustness concerns: constructing a fresh registry and scanning directories on every request in multi-tenant mode introduces significant overhead, which could be mitigated with caching; the use of a single global mutation lock creates a concurrency bottleneck across tenants; resolving user-scoped directories via .parent() is fragile and prone to permission issues; and there is an inconsistency in allowing users to edit but not delete user-placed skills.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[serrrfirat]

Skills settings screenshots from the current branch. Captured against the v2 static UI with mocked Skills API responses so the UI states are deterministic.

Skills tab: user-mounted skills plus read-only system skills

Add skill success

Edit skill editor

Delete skill success

The native confirmation dialog was also verified with message: Delete skill "release-note-helper"?

[henrypark133]

Code Review (multi-agent)

Intent: Add a user-scoped Skills settings UI with backend CRUD endpoints and scope skill management to the authenticated user's own skill mount.

Stats: 29 findings (29 raw, 29 after dedup) across 8 files. Reviewers run: security,bugs,performance,tests,conventions,local-patterns,maintainability,pattern-refactor. Reviewers failed: none. Body-only: 15

security

1. Medium bundle_path exposes absolute host filesystem path to API callers (src/channels/web/handlers/skills.rs:161-161, confidence 90) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:161
   In skill_info(), bundle_path is set to path.display().to_string() — a raw absolute host filesystem path. Serialized unconditionally into every GET /api/skills and POST /api/skills/search response. In multi-tenant mode this exposes internal storage layout including tenant id and user id of the skill
2. Medium source field leaks host filesystem path via Debug-format of SkillSource (src/channels/web/handlers/skills.rs:171-171, confidence 85) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:171
   format!("{:?}", skill.source) expands to User("/home/user/.ironclaw/..."). Included in SkillInfo.source returned to all authenticated callers.
3. Medium Skill search query has no length cap before catalog HTTP fetch (src/channels/web/handlers/skills.rs:209-209, confidence 80) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:209
   SkillSearchRequest.query has no max length validation before passed to catalog.search and to_lowercase(). Attacker can send arbitrarily long query for memory allocation. 14 MiB body limit applies to whole request, not specifically query.
4. Medium skills_install_handler skips safety scan on fetched remote content (src/channels/web/handlers/skills.rs:351-351, confidence 75) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:351
   skills_install_handler does NOT call validate_skill_content_safety on the final normalized content before writing to disk. Content fetched from remote URL or ClawHub catalog is parsed and installed without passing through the safety scanner. A malicious catalog entry or user-supplied URL pointing to
5. Medium SSRF: validation only on pre-DNS hostname; redirect chains not blocked (src/channels/web/handlers/skills.rs:307-312, confidence 65) (no diff position — body only) — anchor: src/tools/builtin/skill_tools.rs:1146
   validate_fetch_url checks URL's literal host but does NOT perform DNS lookup before HTTP request. DNS rebinding or hostname resolving to private address bypasses validation. fetch_skill_payload goes through reqwest which resolves DNS internally after validate_fetch_url returned.
6. Low Per-user scoped registry cache grows unbounded across unique user ids (src/channels/web/handlers/skill_registry_scope.rs:17-20, confidence 70) (no diff position — body only) — anchor: src/channels/web/handlers/skill_registry_scope.rs:17
   SCOPED_SKILL_REGISTRIES evicted only at cache_registry() time. Each distinct user_id gets its own cache entry. Attacker with many tokens or token-issuing endpoint fills the map.
7. Low backfill_local_dev_legacy_user_skills silently drops errors from read_dir entries (crates/ironclaw_reborn_composition/src/factory.rs:896-988, confidence 70) — anchor: crates/ironclaw_reborn_composition/src/factory.rs:955
   Each DirEntry error mapped to generic InvalidConfig with no path/OS error. Marker file still written after partial migration — partially migrated directory never retried (marker prevents re-run).

bugs

1. Medium Truncated search results reported as catalog_error, not a fetch failure (crates/ironclaw_reborn_composition/src/webui.rs:209-212, confidence 85) — anchor: crates/ironclaw_reborn_composition/src/webui.rs:209
   LocalSkillsProductFacade::search_skills surfaces result.truncated as catalog_error. catalog_error semantically is for remote fetch failures. Setting it on local pagination limit makes UI show error banner for normal state.
2. Medium SKILL_MUTATION_LOCKS uses strong Arc, growing unbounded in multi-tenant mode (src/channels/web/handlers/skills.rs:17-19, confidence 80) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:17
   Static SKILL_MUTATION_LOCKS stores Arc<tokio::sync::Mutex<()>> (strong refs). Every unique lock key retained forever. The crates/ironclaw_skills/src/management.rs uses Weak<...> for this — handler diverges.
3. Medium Workspace-source skills marked can_edit/can_delete=true when they should be read-only (crates/ironclaw_reborn_composition/src/webui.rs:315-315, confidence 70) — anchor: crates/ironclaw_reborn_composition/src/webui.rs:315
   can_manage = source_kind != RebornSkillSourceKind::System. Workspace skills get can_edit/can_delete=true but management mount view does not mount workspace skills path — only user/system. User clicking edit/delete on workspace skill will get 404. PR body says 'Keep system/workspace skills read-only'
4. Medium Backfill calls create_dir_all(scoped_root) after copy loop — code-order hazard (crates/ironclaw_reborn_composition/src/factory.rs:931-943, confidence 65) — anchor: crates/ironclaw_reborn_composition/src/factory.rs:931
   Loop calls copy_local_dev_legacy_skill_entry where destination.parent() is scoped_root. Currently works because copy helper calls create_dir_all(parent), but explicit create_dir_all(&scoped_root) at line 944 should come BEFORE the loop so logic is correct by construction.

performance

1. High SKILL_MUTATION_LOCKS map grows unbounded — one entry per user, never evicted (src/channels/web/handlers/skills.rs:17-131, confidence 90) (no diff position — body only) — anchor: src/channels/web/handlers/skill_registry_scope.rs:117
   Global SKILL_MUTATION_LOCKS HashMap accumulates one entry per unique user:{user_id} key. Inserted via or_insert_with but never removed. Multi-tenant deployment with many users leaks memory proportional to user population.
2. High Synchronous blocking filesystem I/O (std::fs::*) called from async startup path (crates/ironclaw_reborn_composition/src/factory.rs:894-992, confidence 85) — anchor: crates/ironclaw_reborn_composition/src/factory.rs:623
   backfill_local_dev_legacy_user_skills and copy_local_dev_legacy_skill_entry use std::fs::read_dir, symlink_metadata, create_dir_all, write, copy — all blocking — from within async fn build_local_dev. Stalls tokio worker thread.
3. Medium join_all fan-out: O(n) concurrent filesystem probes per skills_list request (src/channels/web/handlers/skills.rs:134-204, confidence 80) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:144
   skills_list_handler and skills_search_handler call join_all(skill_snapshot.into_iter().map(|skill| skill_info(skill, ...))). Each skill_info issues up to 3 FS ops. With n skills this is 3n concurrent I/O calls per request.
4. Medium Recursive directory copy with no depth limit — stack overflow on deep skill trees (crates/ironclaw_reborn_composition/src/factory.rs:953-992, confidence 72) — anchor: crates/ironclaw_reborn_composition/src/factory.rs:978
   copy_local_dev_legacy_skill_entry calls itself recursively with no max depth guard. Legacy skills/ is user-controlled. Deeply nested structure overflows stack.
5. Medium Cache stampede: concurrent requests for same user each build their own registry (src/channels/web/handlers/skill_registry_scope.rs:58-83, confidence 68) (no diff position — body only) — anchor: src/channels/web/handlers/skill_registry_scope.rs:79
   cached_scoped_registry releases mutex before scoped.discover_all().await then reacquires to write. Multiple simultaneous requests miss cache and each independently call discover_all().

tests

1. Medium skills_install_handler: no test for missing X-Confirm-Action header (src/channels/web/handlers/skills.rs:277-399, confidence 90) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:285
   skills_update_handler has the dedicated test; install handler's parallel guard at lines 285-294 has no equivalent test. Regression that removes guard goes undetected.
2. Medium skills_remove_handler: no test for missing X-Confirm-Action header (src/channels/web/handlers/skills.rs:401-446, confidence 90) (no diff position — body only) — anchor: src/channels/web/handlers/skills.rs:411
   Same guard pattern as install/update; no test asserting rejection when header absent.
3. Medium read_skill_content and update_skill have no tests in management::tests (crates/ironclaw_skills/src/management.rs:547-606, confidence 85) — anchor: crates/ironclaw_skills/src/management.rs:547
   New management public functions read_skill_content and update_skill have zero coverage. Validation paths (name validation, size, name-change rejection, NotFound guard) not exercised at management tier.
4. Medium LocalSkillsProductFacade: update_skill and remove_skill happy paths not covered by webui tests (crates/ironclaw_reborn_composition/src/webui.rs:179-287, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/webui.rs:252
   Webui covers install, list, read_skill_content (happy + cross-user isolation), and safety-rejection. Update happy path and remove happy path through LocalSkillsProductFacade not tested.
5. Low backfill_local_dev_legacy_user_skills: reborn-cli tenant leg untested (crates/ironclaw_reborn_composition/src/factory.rs:894-916, confidence 75) — anchor: crates/ironclaw_reborn_composition/src/factory.rs:903
   Iterates over both default and reborn-cli tenant IDs. Existing tests only assert tenants/default/.... Regression dropping reborn-cli iteration undetected.

conventions

1. High Skill mutation handlers bypass ToolDispatcher without dispatch-exempt annotation (src/channels/web/handlers/skills.rs:277-530, confidence 95) (no diff position — body only) — anchor: /Users/henry/Code/ironclaw/CLAUDE.md
   skills_install_handler, skills_remove_handler, skills_update_handler call directly into scoped_skill_registry (state.skill_registry) and perform side effects without routing through ToolDispatcher::dispatch(). CLAUDE.md 'Everything Goes Through Tools' MUST. No // dispatch-exempt: annotation.
2. Medium source field serialized via format!("{:?}") instead of stable enum representation (src/channels/web/handlers/skills.rs:171-171, confidence 90) (no diff position — body only) — anchor: /Users/henry/Code/ironclaw/.claude/rules/types.md
   SkillInfo.source set with format!("{:?}", skill.source) — Rust debug repr is not stable wire contract.
3. Medium bundle_path exposes absolute host filesystem paths over channel boundary (src/channels/web/handlers/skills.rs:161-161, confidence 88) (no diff position — body only) — anchor: /Users/henry/Code/ironclaw/.claude/rules/error-handling.md
   bundle_path computed as path.display().to_string() included verbatim in SkillInfo wire response. Emits /home/henry/.ironclaw/skills/foo to browser. error-handling.md prohibits exposing absolute paths.
4. Medium RebornSkillInfo.trust and RebornSkillInfo.source are stringly-typed wire fields for fixed enum sets (crates/ironclaw_product_workflow/src/reborn_services/types.rs:385-411, confidence 85) — anchor: /Users/henry/Code/ironclaw/.claude/rules/types.md
   RebornSkillInfo carries trust: String and source: String. Both carry values from typed enums (SkillTrust, SkillSource). types.md requires enums for fixed small sets.

local-patterns

1. Nit UnsupportedSkillsProductFacade uses Default instead of new_static() like siblings (crates/ironclaw_product_workflow/src/reborn_services.rs:184-188, confidence 75) — anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:258
   Sibling UnsupportedAutomationProductFacade and UnsupportedLifecycleProductFacade expose pub fn new_static(). New UnsupportedSkillsProductFacade derives Default and uses ::default().

maintainability

1. Medium Six skill methods copy-paste default bodies on RebornServicesApi already on SkillsProductFacade (crates/ironclaw_product_workflow/src/reborn_services.rs:412-465, confidence 75) — anchor: crates/ironclaw_product_workflow/src/reborn_services.rs:412
   Six skill methods have identical service_unavailable default bodies on both SkillsProductFacade (128-181) and RebornServicesApi (412-465). RebornServices initializes skills_facade to UnsupportedSkillsProductFacade — RebornServicesApi defaults can never fire in practice.
2. Low Every skill operation has paired _for_scope variant where one can subsume the other (crates/ironclaw_reborn_composition/src/lifecycle.rs:82-189, confidence 65) — anchor: crates/ironclaw_reborn_composition/src/lifecycle.rs:82
   Five operations in two forms each: list/list_for_scope etc. Unscoped forms reduce to foo_for_scope(self.owner_scope()?). Pure mechanical duplication.

pattern-refactor

1. Low Parallel method/method_for_scope pairs collapse to single scoped API (crates/ironclaw_reborn_composition/src/lifecycle.rs:82-189, confidence 60) — anchor: crates/ironclaw_reborn_composition/src/lifecycle.rs:82
   Doubled method surface adds no behavior. Single scoped signature per operation removes the redundant layer.

Prior review activity

• 4 prior inline comments detected on this PR (from prior automated reviews/bots). No findings here were dropped as duplicates; flag if any match a previous author-declined comment.

[henrypark133]

High — Synchronous blocking filesystem I/O (std::fs::*) called from async startup path.

backfill_local_dev_legacy_user_skills and copy_local_dev_legacy_skill_entry use std::fs::read_dir, symlink_metadata, create_dir_all, write, copy — all blocking — from within async fn build_local_dev. Stalls tokio worker thread.

Fix: Wrap backfill in tokio::task::spawn_blocking or convert to tokio::fs::*.

[henrypark133]

Medium — Truncated search results reported as catalog_error, not a fetch failure.

LocalSkillsProductFacade::search_skills surfaces result.truncated as catalog_error. catalog_error semantically is for remote fetch failures. Setting it on local pagination limit makes UI show error banner for normal state.

Fix: Remove the catalog_error mapping for truncated; either drop it or add a separate truncated: bool field.

[henrypark133]

Medium — read_skill_content and update_skill have no tests in management::tests.

New management public functions read_skill_content and update_skill have zero coverage. Validation paths (name validation, size, name-change rejection, NotFound guard) not exercised at management tier.

Fix: Add NotFound/InvalidInput error branch tests.

[henrypark133]

Medium — RebornSkillInfo.trust and RebornSkillInfo.source are stringly-typed wire fields for fixed enum sets.

RebornSkillInfo carries trust: String and source: String. Both carry values from typed enums (SkillTrust, SkillSource). types.md requires enums for fixed small sets.

Fix: Replace trust with RebornSkillTrustLevel enum and source with RebornSkillSourceKind or RebornSkillSourceDetail.

[henrypark133]

Medium — LocalSkillsProductFacade: update_skill and remove_skill happy paths not covered by webui tests.

Webui covers install, list, read_skill_content (happy + cross-user isolation), and safety-rejection. Update happy path and remove happy path through LocalSkillsProductFacade not tested.

Fix: Add update/remove happy path tests through the facade.

[henrypark133]

Low — backfill_local_dev_legacy_user_skills: reborn-cli tenant leg untested.

Iterates over both default and reborn-cli tenant IDs. Existing tests only assert tenants/default/.... Regression dropping reborn-cli iteration undetected.

Fix: Add test covering skill landing under both tenants/default and tenants/reborn-cli.

[henrypark133]

Low — backfill_local_dev_legacy_user_skills silently drops errors from read_dir entries.

Each DirEntry error mapped to generic InvalidConfig with no path/OS error. Marker file still written after partial migration — partially migrated directory never retried (marker prevents re-run).

Fix: Only write marker if no copy errors occurred; otherwise return the error.

[henrypark133]

Low — Every skill operation has paired _for_scope variant where one can subsume the other.

Five operations in two forms each: list/list_for_scope etc. Unscoped forms reduce to foo_for_scope(self.owner_scope()?). Pure mechanical duplication.

Fix: Delete unscoped variants. Callers call owner_scope() explicitly before dispatching.

[henrypark133]

Low — Parallel method/method_for_scope pairs collapse to single scoped API.

Doubled method surface adds no behavior. Single scoped signature per operation removes the redundant layer.

Fix: Consolidate to _for_scope variants. Caller invokes port.owner_scope()? before dispatch.

[henrypark133]

Nit — UnsupportedSkillsProductFacade uses Default instead of new_static() like siblings.

Sibling UnsupportedAutomationProductFacade and UnsupportedLifecycleProductFacade expose pub fn new_static(). New UnsupportedSkillsProductFacade derives Default and uses ::default().

Fix: Add pub fn new_static() and update call site.