feat(skills): uninstall for user-scope SKILL.md skills (#781)#833
Merged
graycyrus merged 14 commits intotinyhumansai:mainfrom Apr 24, 2026
Merged
feat(skills): uninstall for user-scope SKILL.md skills (#781)#833graycyrus merged 14 commits intotinyhumansai:mainfrom
graycyrus merged 14 commits intotinyhumansai:mainfrom
Conversation
…ild (tinyhumansai#781) `collapse_whitespace` lives in `threads::title`; the new tests in `threads::ops::tests` (added in tinyhumansai#797) call it without an explicit import, so `cargo test --lib` fails to compile on upstream/main. Add the missing `use` so any PR branching off main gets a green baseline. Drive-by; unrelated to the skills uninstall work in the rest of this branch but needed so the rest of the diff can be validated locally. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ards (tinyhumansai#781) Adds `uninstall_skill(params, home_dir_override)` plus `UninstallSkillParams` and `UninstallSkillOutcome` under `openhuman::skills::ops`. Only user-scope installs (`~/.openhuman/skills/<name>/`) are removable; project-scope and legacy `<ws>/skills/` trees stay read-only because the user manages them directly via git/editor. Defensive resolution: - reject names with `/`, `\`, or `..` before any fs call - cap name length at `MAX_NAME_LEN` (64) - canonicalize the skills root and the candidate, then `starts_with`-check so symlinks and `..` traversal cannot escape - refuse anything that is not a plain directory (symlinks rejected) - refuse dirs without a `SKILL.md` — we only remove things that look like skills we installed Five unit tests cover happy path, path-traversal rejects, empty-name reject, not-installed clean error, non-SKILL.md dir refusal, and symlink escape (unix only). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…inyhumansai#781) Expose the new core `uninstall_skill` through the standard controller registry so CLI and JSON-RPC adapters pick it up automatically — per the `Controller-only exposure` rule in CLAUDE.md, no edits to `core/cli.rs` or `core/jsonrpc.rs` needed. - Adds `skills_uninstall` to `all_skills_controller_schemas` and `all_skills_registered_controllers` - `skills_schemas("skills_uninstall")` spells out the `name` input + `name` / `removed_path` / `scope` outputs the UI consumes - `handle_skills_uninstall` deserializes the wire payload, calls `uninstall_skill(.., None)` (production `dirs::home_dir()` path), and surfaces backend errors verbatim so the dialog can show them inline The existing `controller_lists_match_lengths` schema test covers the 5-vs-5 length invariant now that we added one entry to each list. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t types (tinyhumansai#781) Thin typed wrapper around `openhuman.skills_uninstall` that mirrors the existing `installSkillFromUrl` / `createSkill` patterns: snake_case wire → camelCase result, envelope unwrap, debug-log entry + exit. Caller passes the slug; result carries `name`, `removedPath` (canonical on-disk path — useful to echo in success toasts), and `scope`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…KILL.md cards (tinyhumansai#781) Adds a destructive-action confirm dialog surfaced from a new ⋯-menu "Uninstall" item on user-scope, non-legacy discovered SKILL.md cards. Project and legacy scopes deliberately do not get the menu item — the backend RPC rejects them and we don't want to advertise a button that will only error. UI contract (see component rustdoc-style header): - Shows skill name, resolved on-disk path (SKILL.md stripped), and a plain destructive warning line - Cancel → onClose; Uninstall → skillsApi.uninstallSkill(name) - While the RPC is in flight, buttons disable, Esc no-ops, backdrop dismissal blocked so the caller sees the outcome - Success fires onUninstalled(result) then closes; page drops the row from local state + refetches to surface any previously-shadowed project-scope duplicate, and closes the detail drawer if it was showing the removed skill - Failure surfaces the raw backend error inline; the dialog stays open so the user can retry or cancel Vitest covers all five paths (render, cancel-no-RPC, confirm-happy, error-inline, in-flight button disable). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full skill-uninstall feature: frontend confirm dialog and Skills page integration, a new TypeScript API method, backend RPC/schema and ops to safely delete skill directories, and tests covering unit and end-to-end flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Skills Page / Dialog
participant API as skillsApi
participant RPC as Backend RPC
participant FS as Filesystem
User->>UI: Click "Uninstall" on a discovered skill
UI->>UI: Open UninstallSkillConfirmDialog (store focus)
User->>UI: Click "Uninstall" (Confirm)
UI->>UI: Set submitting, disable buttons
UI->>API: uninstallSkill(skill.id)
API->>RPC: JSON-RPC call skills.uninstall
RPC->>RPC: Validate name, canonicalize paths, enforce containment
RPC->>FS: Check SKILL.md exists
RPC->>FS: remove_dir_all(skill_dir)
FS-->>RPC: Success / Error
RPC-->>API: Return outcome {name, removed_path, scope} or error
API-->>UI: Resolve with UninstallSkillResult or throw
UI->>UI: onUninstalled(result) then onClose()
UI->>UI: Restore focus
UI->>UI: Trigger toast + remove from list + refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
senamakel
reviewed
Apr 24, 2026
senamakel
reviewed
Apr 24, 2026
senamakel
reviewed
Apr 24, 2026
senamakel
reviewed
Apr 24, 2026
senamakel
reviewed
Apr 24, 2026
senamakel
reviewed
Apr 24, 2026
Member
|
bro fix the build issues. then good to merge... @oxoxDev |
Member
|
same here... pls fix build and type issues.. |
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
Apr 24, 2026
… polish, schema cleanup - ops.rs: remove TOCTOU `candidate.exists()` pre-check; canonicalize directly and map NotFound → "skill '<name>' is not installed", other kinds → original "canonicalize <path> failed: <e>" message - ops.rs: add best-effort comment on the skill_md.exists() guard clarifying that remove_dir_all is the authoritative operation - UninstallSkillConfirmDialog.tsx: prefix caught error with "Couldn't uninstall skill: <msg>" for user-friendliness - Skills.tsx: add success toast via ToastContainer after uninstall (same pattern as Intelligence.tsx) - schemas.rs: eliminate SkillsUninstallParamsWire + From impl; deserialize directly into UninstallSkillParams (already derives Deserialize), matching the pattern for SkillsListParams
tinyhumansai#781) TypeScript narrowed `let resolveIt: Fn | null = null` to `never` after assignment inside the Promise executor closure, breaking `tsc --noEmit` on CI with `Type 'never' has no call signatures`. Replace with a deferred-style wrapper object whose optional `resolve` member keeps the callable type intact across the closure boundary. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx (1)
94-95: Use role/name queries for the confirm button instead ofdata-testid.Line 94, Line 126, Line 135, Line 164, and Line 168 couple assertions to an implementation attribute. Prefer accessible behavior queries (
getByRole) for better resilience.Proposed refactor
- fireEvent.click(screen.getByTestId('uninstall-skill-confirm')); + fireEvent.click(screen.getByRole('button', { name: /^Uninstall$/ })); - const confirm = screen.getByTestId('uninstall-skill-confirm') as HTMLButtonElement; + const confirm = screen.getByRole('button', { name: /^Uninstall$/ }) as HTMLButtonElement;As per coding guidelines, "Prefer testing behavior over implementation details in unit tests; use existing helpers from
app/src/test/before adding new harness code".Also applies to: 126-127, 135-136, 164-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx` around lines 94 - 95, Replace tests that target the confirm button via data-testid ('uninstall-skill-confirm') with accessible queries using getByRole/getByRole(..., { name: /.../ }) or the project's test helpers so the button is selected by role/name (e.g., role "button" and the visible confirmation label) instead of implementation attributes; update all occurrences in UninstallSkillConfirmDialog.test.tsx (the fireEvent.click and related assertions at the lines referenced) to use screen.getByRole('button', { name: /confirm/i }) or the appropriate localized label from your test helpers in app/src/test/ to make the tests resilient and accessible-friendly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx`:
- Around line 94-95: Replace tests that target the confirm button via
data-testid ('uninstall-skill-confirm') with accessible queries using
getByRole/getByRole(..., { name: /.../ }) or the project's test helpers so the
button is selected by role/name (e.g., role "button" and the visible
confirmation label) instead of implementation attributes; update all occurrences
in UninstallSkillConfirmDialog.test.tsx (the fireEvent.click and related
assertions at the lines referenced) to use screen.getByRole('button', { name:
/confirm/i }) or the appropriate localized label from your test helpers in
app/src/test/ to make the tests resilient and accessible-friendly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23145a76-52b8-44da-87a1-05626895b800
📒 Files selected for processing (1)
app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx
…nicalize (tinyhumansai#781) Addresses senamakel review comment at ops.rs:1833. The earlier pattern of `candidate.exists()` → `fs::canonicalize(&candidate)` opens a time-of-check / time-of-use window where an adversary could swap the directory for a symlink escaping `skills_root` between the two calls, bypassing the `starts_with` guard. Fold the existence check into `canonicalize` and branch on `io::ErrorKind::NotFound` so the friendly "skill '<name>' is not installed" message is preserved while other kinds keep the lower-level diagnostic. Also add a best-effort comment on the subsequent `skill_md.exists()` guard clarifying that `remove_dir_all` is the authoritative operation — a TOCTOU race there is benign. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…umansai#781) Addresses senamakel review comment at schemas.rs:166. The wire struct was structurally identical to UninstallSkillParams and only existed to satisfy a trivial From impl. UninstallSkillParams already derives Deserialize, so the RPC handler can deserialize directly — matching the SkillsListParams pattern in the same file and removing 12 lines of boilerplate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ansai#781) Addresses senamakel review comment at UninstallSkillConfirmDialog:84. Raw Rust error strings like "canonicalize ... failed: Os { code: 13, PermissionDenied, ... }" were being surfaced verbatim in the dialog. Prefix the caught message with "Couldn't uninstall skill: " so the technical detail is preserved (useful for bug reports) while the leading copy stays human-readable. The existing dialog-level "Could not uninstall" label is unchanged, so the vitest expectation at the error-path test is unaffected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ai#781) Addresses senamakel review comment at Skills.tsx:825. Previously the dialog closed silently on success, leaving no visible confirmation that a destructive action took effect. Wire the same ToastContainer / addToast / removeToast pattern already used by Intelligence.tsx and fire a "Skill uninstalled" toast from the onUninstalled callback so the feedback loop closes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/skills/ops.rs (1)
1755-1761: Add entry-point logging for diagnosabilityPer coding guidelines, functions should log at entry points with grep-friendly prefixes. Similar functions in this file (
read_skill_resource,create_skill_inner) usetracing::debug!at entry. Consider adding one here for consistency:pub fn uninstall_skill( params: UninstallSkillParams, home_dir_override: Option<&Path>, ) -> Result<UninstallSkillOutcome, String> { let trimmed = params.name.trim().to_string(); + tracing::debug!( + name = %trimmed, + "[skills] uninstall_skill: entry" + ); if trimmed.is_empty() {Also, consider switching
log::warn!/log::info!(lines 1764, 1812, 1845) totracing::warn!/tracing::info!for consistency with other ops functions.As per coding guidelines: "Add substantial development-oriented logging at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/ops.rs` around lines 1755 - 1761, Add entry-point diagnostic logging to the uninstall_skill function: at the start of uninstall_skill(params: UninstallSkillParams, home_dir_override: Option<&Path>) emit a tracing::debug! with a grep-friendly prefix (e.g., "ops::uninstall_skill: entering" plus the trimmed skill name and relevant params) to match other ops functions like read_skill_resource and create_skill_inner; while updating, replace any uses of log::warn! and log::info! inside uninstall_skill with tracing::warn! and tracing::info! for consistency, and ensure branch/error paths also use tracing macros with clear messages for diagnosability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/openhuman/skills/ops.rs`:
- Around line 1755-1761: Add entry-point diagnostic logging to the
uninstall_skill function: at the start of uninstall_skill(params:
UninstallSkillParams, home_dir_override: Option<&Path>) emit a tracing::debug!
with a grep-friendly prefix (e.g., "ops::uninstall_skill: entering" plus the
trimmed skill name and relevant params) to match other ops functions like
read_skill_resource and create_skill_inner; while updating, replace any uses of
log::warn! and log::info! inside uninstall_skill with tracing::warn! and
tracing::info! for consistency, and ensure branch/error paths also use tracing
macros with clear messages for diagnosability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcb51574-af2e-4151-857f-30599f8c5101
📒 Files selected for processing (4)
app/src/components/skills/UninstallSkillConfirmDialog.tsxapp/src/pages/Skills.tsxsrc/openhuman/skills/ops.rssrc/openhuman/skills/schemas.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/components/skills/UninstallSkillConfirmDialog.tsx
- app/src/pages/Skills.tsx
…stall # Conflicts: # app/src/pages/Skills.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/pages/Skills.tsx`:
- Around line 831-857: Add app-level E2E tests that exercise the uninstall flow
wired in UninstallSkillConfirmDialog: write a happy-path test that triggers the
dialog, confirms uninstall, asserts the success toast from ToastContainer
appears, verifies the removed skill is filtered out from discoveredSkills (i.e.,
the UI row disappears), that the detail drawer selection (setSelectedSkill) is
cleared when it matched the removed skill, and that refreshDiscoveredSkills is
called/reconciles side effects; also add a failure-path test that simulates an
uninstall RPC error and asserts an error toast and no removal from
discoveredSkills. Use selectors for the skill card, the uninstall button that
opens UninstallSkillConfirmDialog, and toast messages to locate UI elements and
mock the RPC responses for success and failure.
In `@src/openhuman/skills/ops.rs`:
- Around line 1822-1865: The code currently canonicalizes skills_root and
candidate which dereferences symlinks; instead, call std::fs::symlink_metadata
on the raw skills_root and on candidate (the path built from
home.join(".openhuman").join("skills") and skills_root.join(&trimmed)) before
any canonicalize, and if either meta.file_type().is_symlink() is true return an
Err refusing symlinked roots/aliases; only after both raw-path symlink checks
pass, proceed to std::fs::canonicalize to obtain canonical_root and
canonical_candidate and keep the existing starts_with and is_dir checks.
- Around line 1747-1751: The Uninstall flow keys off the frontmatter display
name; change UninstallSkillParams to carry the on-disk slug (e.g., add/rename a
field like `slug: String` or `disk_name: String`) and update the uninstall logic
that currently uses `UninstallSkillParams::name` (and the related uninstall
functions referenced around lines 1790-1844) to resolve and remove the directory
`~/.openhuman/skills/<slug>/` instead of the frontmatter `name`; update any call
sites and API decoding to accept/send the slug/ID (the same value exposed as
`SkillSummary.id`) so the uninstall targets the actual on-disk directory rather
than the display name.
- Around line 2864-2992: The tests exercise uninstall_skill(...) directly but
lack a JSON-RPC E2E that verifies controller registration, wire decoding,
response shape and error propagation for the RPC method
"openhuman.skills_uninstall"; add an integration test that starts the JSON-RPC
handler used in tests (registering the openhuman skills controller), invokes the
"openhuman.skills_uninstall" method with the same UninstallSkillParams payload
(both success and error cases such as missing SKILL.md and path traversal), and
asserts the RPC response matches the expected success structure (name, scope)
and that errors are forwarded verbatim (contain the same messages produced by
uninstall_skill), using the existing uninstall_skill test fixtures (temporary
home dirs) to create real on-disk conditions. Ensure the test references the RPC
method name "openhuman.skills_uninstall" and the uninstall_skill helper so it
validates the wire layer instead of calling the helper directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1ded306-59b8-4446-8130-220448fbb9cf
📒 Files selected for processing (2)
app/src/pages/Skills.tsxsrc/openhuman/skills/ops.rs
Comment on lines
+831
to
+857
| {uninstallCandidate && ( | ||
| <UninstallSkillConfirmDialog | ||
| skill={uninstallCandidate} | ||
| onClose={() => setUninstallCandidate(null)} | ||
| onUninstalled={result => { | ||
| console.debug('[skills][uninstall] complete', { | ||
| name: result.name, | ||
| removedPath: result.removedPath, | ||
| }); | ||
| addToast({ | ||
| type: 'success', | ||
| title: 'Skill uninstalled', | ||
| message: `"${result.name}" was removed successfully.`, | ||
| }); | ||
| // If the detail drawer was showing the skill we just removed, | ||
| // close it — the resource tree is now stale and any `read_resource` | ||
| // RPC would fail with a clean "not installed" error. | ||
| setSelectedSkill(prev => (prev && prev.id === result.name ? null : prev)); | ||
| // Drop it from local state so the card disappears without a | ||
| // round-trip; refresh to pick up any side effects (e.g. a | ||
| // previously-shadowed project-scope skill now surfaces). | ||
| setDiscoveredSkills(prev => prev.filter(s => s.id !== result.name)); | ||
| void refreshDiscoveredSkills(); | ||
| }} | ||
| /> | ||
| )} | ||
| <ToastContainer notifications={toasts} onRemove={removeToast} /> |
Contributor
There was a problem hiding this comment.
This uninstall page wiring still needs app E2E coverage.
The dialog tests do not exercise the behavior added here: removing the row from discoveredSkills, closing a selected drawer, showing the success toast, and reconciling after refreshDiscoveredSkills(). Please add an app E2E for the happy path and failure path before merge. As per coding guidelines, "When building a new feature, define E2E scenarios (core RPC + app) up front that cover happy paths, failure modes, auth/policy gates, and regressions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/pages/Skills.tsx` around lines 831 - 857, Add app-level E2E tests
that exercise the uninstall flow wired in UninstallSkillConfirmDialog: write a
happy-path test that triggers the dialog, confirms uninstall, asserts the
success toast from ToastContainer appears, verifies the removed skill is
filtered out from discoveredSkills (i.e., the UI row disappears), that the
detail drawer selection (setSelectedSkill) is cleared when it matched the
removed skill, and that refreshDiscoveredSkills is called/reconciles side
effects; also add a failure-path test that simulates an uninstall RPC error and
asserts an error toast and no removal from discoveredSkills. Use selectors for
the skill card, the uninstall button that opens UninstallSkillConfirmDialog, and
toast messages to locate UI elements and mock the RPC responses for success and
failure.
Contributor
Author
There was a problem hiding this comment.
Partially addressed — deferring full app E2E to a follow-up.
Landed in b160a8e: regression test in UninstallSkillConfirmDialog.test.tsx that exercises a divergent id vs name fixture and asserts skillsApi.uninstallSkill is called with the slug (id). This is the most load-bearing behavioural claim in the uninstall wiring, since the slug bug was the concrete defect the dialog test previously couldn't catch.
Deferred: Skills.tsx-level integration test that mounts the full page, clicks through the card → dialog → confirm, asserts discoveredSkills row filter, drawer selection clear, toast appearance, and refreshDiscoveredSkills call. The existing Skills.composio-catalog.test.tsx gives us the renderWithProviders harness pattern; a sibling Skills.uninstall.test.tsx would slot in the same way. Tracking as a follow-up so this PR ships with the slug fix + symlink preflight + RPC E2E rather than blocking on the page harness.
Contributor
There was a problem hiding this comment.
@oxoxDev, thanks for the clear breakdown. The slug regression test in UninstallSkillConfirmDialog.test.tsx (b160a8e) covers the most critical behavioral contract — uninstallSkill called with skill.id (slug) rather than skill.name — so the load-bearing case is protected.
The deferred Skills.uninstall.test.tsx is well-scoped: the renderWithProviders harness from Skills.composio-catalog.test.tsx is a clean pattern to follow, and the assertions are well-defined (row filter, drawer clear, toast appearance, refreshDiscoveredSkills call). Would you like me to open a follow-up GitHub issue to track that, so it doesn't get lost after merge?
✏️ Learnings added
Learnt from: oxoxDev
Repo: tinyhumansai/openhuman PR: 833
File: app/src/pages/Skills.tsx:831-857
Timestamp: 2026-04-24T11:37:55.524Z
Learning: In `app/src/pages/Skills.tsx` and `app/src/components/skills/UninstallSkillConfirmDialog.tsx` (PR `#833`), the page-level integration test for the uninstall flow (`Skills.uninstall.test.tsx`) was intentionally deferred to a follow-up. The planned test should: mount the full Skills page using the `renderWithProviders` harness (same pattern as `Skills.composio-catalog.test.tsx`), click through the skill card → uninstall overflow menu → UninstallSkillConfirmDialog → confirm, and assert: `discoveredSkills` row is removed from the grid, detail drawer selection is cleared when it matched the removed skill, a success toast appears via ToastContainer, and `refreshDiscoveredSkills` is invoked. The blocker was the page harness setup time; the slug regression (id vs name) was already covered in `UninstallSkillConfirmDialog.test.tsx` commit b160a8ef.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 283
File: app/test/e2e/specs/skill-execution-flow.spec.ts:67-114
Timestamp: 2026-04-03T06:28:58.632Z
Learning: In `app/test/e2e/specs/skill-execution-flow.spec.ts`, the model→agent→tool→conversation E2E path (model-triggered tool_calls) is intentionally deferred to a future PR. It is tracked via an `it.skip` placeholder at the end of the spec. The blocker is that the mock backend does not yet support returning structured `tool_calls` in chat completion responses. The current spec only covers the deterministic skill runtime RPC surface: `skills_start` → `skills_list_tools` → `skills_call_tool` → `skills_stop`.
Learnt from: CR
Repo: tinyhumansai/openhuman PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-23T06:30:00.454Z
Learning: Applies to app/src/**/*.{ts,tsx} : When building a new feature, define E2E scenarios (core RPC + app) up front that cover happy paths, failure modes, auth/policy gates, and regressions
Learnt from: CR
Repo: tinyhumansai/openhuman PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-23T06:30:00.454Z
Learning: Applies to **/*.{rs,ts,tsx} : Ship unit tests and coverage for behavior you are adding or changing before building additional features on top
Learnt from: CR
Repo: tinyhumansai/openhuman PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-23T06:30:00.454Z
Learning: Applies to app/test/e2e/specs/**/*.spec.ts : Assert both UI outcomes and backend/mock effects when relevant; add failure diagnostics (request logs, `dumpAccessibilityTree()`) for faster debugging
Learnt from: graycyrus
Repo: tinyhumansai/openhuman PR: 167
File: app/src/components/OnboardingOverlay.tsx:30-39
Timestamp: 2026-04-01T13:52:17.936Z
Learning: In React components, do not perform synchronous `setState` (or other state-updating calls) directly inside `useEffect` bodies. The lint rule `react-hooks/set-state-in-effect` (enforced in this codebase) disallows patterns such as resetting state flags at the top of an effect before returning. Refactor to avoid calling state setters in the effect; prefer deriving state from props/render, using refs, or restructuring logic so state updates occur outside `useEffect`.
…humansai#781) Store the directory slug on `Skill::dir_name` separately from the frontmatter-derived `Skill::name`, so a skill whose SKILL.md declares a name that disagrees with its folder can still be resolved by slug. The uninstall RPC already keys on the slug that reaches it; exposing slug in the discovery payload (via `SkillSummary.id`, in a follow-up commit) is what lets the UI stop sending the display name. Add a raw-path symlink preflight on both the skills root and the candidate directory, refused before `canonicalize` dereferences either. Previously canonicalising first meant a symlinked `~/.openhuman/skills` root, or an in-tree alias like `alias -> real-skill`, would silently resolve to a different directory than the literal slug the caller named, and the later `starts_with` / `is_dir` guards would compare against the resolved target. Tests: two new cases — rejecting a symlinked skills root, and rejecting an in-tree symlink alias (the real skill behind the alias must survive). Existing `uninstall_skill_rejects_symlink_escape` updated to accept the preflight's message alongside the defence-in-depth `starts_with` / `is_dir` errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… by slug (tinyhumansai#781) `SkillSummary.id` previously echoed `Skill.name`, which is the frontmatter display name. The uninstall RPC resolves `~/.openhuman/skills/<slug>/`, so when a skill's SKILL.md declared a name that diverged from its folder the dialog sent the display name and the backend returned "not installed". Expose the new `Skill::dir_name` slug through `SkillSummary.id` (falling back to `name` for any deserialised `Skill` payloads written before the field existed). Update the `openhuman.skills_uninstall` param docstring to spell out that `name` on the wire is the slug, not the display name, and swap the confirm dialog from `skill.name` to `skill.id`. Added a regression test in UninstallSkillConfirmDialog.test.tsx that exercises a fixture where id and name diverge and asserts the slug (id) reaches `skillsApi.uninstallSkill`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ai#781) CodeRabbit flagged that uninstall coverage stopped at the `uninstall_skill(...)` helper: controller registration, wire decoding, response shape, and verbatim error propagation were untested, so all four could break silently with the helper still green. Add a tokio E2E that spins up `build_core_http_router(false)` on an ephemeral port and exercises three paths against the real JSON-RPC surface: - success — seeded skill dir removed, response includes slug, user scope, and a `removed_path` that references the slug directory - not installed — error forwarded verbatim (contains "not installed") - path traversal — `name: "../etc"` error forwarded verbatim (contains "path separators" / "path escapes" / "not installed" depending on which guard fires first) Uses the existing `EnvVarGuard` + `tempdir()` pattern from the auth / memory e2e tests so HOME is isolated to the test's tempdir. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/json_rpc_e2e.rs (2)
2383-2387: Traversal assertion is too permissive.Allowing
"not installed"here can hide a regression in traversal rejection. Forname: "../etc", assert the traversal-specific error only.Patch
assert!( - traversal_msg.contains("path separators") - || traversal_msg.contains("path escapes") - || traversal_msg.contains("not installed"), + traversal_msg.contains("path separators") || traversal_msg.contains("path escapes"), "expected traversal rejection error, got: {traversal_err}" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/json_rpc_e2e.rs` around lines 2383 - 2387, The assertion that checks traversal_msg is too permissive—remove the `"not installed"` alternative so the test enforces a traversal-specific rejection for inputs like name: "../etc"; update the assert that currently references traversal_msg to only accept traversal-related substrings (e.g., contains "path separators" or "path escapes") and fail if the message is "not installed" or anything else, ensuring the traversal rejection is explicitly validated.
2281-2289: Add the missing-SKILL.md wire failure case you describe.The test comment says missing
SKILL.mdis validated, but the body only coversnot installedand traversal. Add an RPC call for a directory lackingSKILL.mdand assert that backend error is forwarded.As per coding guidelines: "Ship unit tests and coverage for behavior you are adding or changing before building additional features on top."
Also applies to: 2350-2389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/json_rpc_e2e.rs` around lines 2281 - 2289, The test is missing the wire-level failure case for a directory that exists but lacks SKILL.md; add an RPC invocation of "openhuman.skills_uninstall" using UninstallSkillParams that targets a created skill directory without SKILL.md (similar to the existing traversal and not-installed cases), then assert the JSON-RPC response forwards the backend error message for the missing SKILL.md case (the same string the uninstall_skill(...) helper would return) and uses the expected error shape/status; locate the existing assertions around UninstallSkillParams and reuse their request/response decoding helpers to add this extra assertion.app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx (1)
47-60: Add a Windows-path rendering case.Please add one assertion for
location: 'C:\\Users\\me\\.openhuman\\skills\\weather-helper\\SKILL.md'to lock the cross-platform suffix stripping behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx` around lines 47 - 60, Add a Windows-path rendering assertion to the UninstallSkillConfirmDialog test: create or reuse the fixture but set skill.location to "C:\\Users\\me\\.openhuman\\skills\\weather-helper\\SKILL.md", render <UninstallSkillConfirmDialog skill={fixture} ... />, and assert that the dialog shows the stripped path "C:\\Users\\me\\.openhuman\\skills\\weather-helper" (use a string or a RegExp that properly escapes backslashes) so the suffix stripping behavior is locked for Windows paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/skills/UninstallSkillConfirmDialog.tsx`:
- Around line 111-114: The code in UninstallSkillConfirmDialog uses
skill.location.replace(/\/SKILL\.md$/i, '') which only strips POSIX separators;
update the replacement to handle Windows backslashes as well (e.g., match either
'/' or '\' before SKILL.md) or normalize the path separators before replacing so
both "C:/.../SKILL.md" and "C:\...\SKILL.md" become the directory; locate the
skill.location.replace call in UninstallSkillConfirmDialog and change the regex
to handle [\/\\]SKILL\.md$ (case-insensitive) or normalize separators first and
then remove "/SKILL.md".
In `@tests/json_rpc_e2e.rs`:
- Around line 2296-2298: The test currently sets HOME and unsets
OPENHUMAN_WORKSPACE using EnvVarGuard::set_to_path("HOME", home) and
EnvVarGuard::unset("OPENHUMAN_WORKSPACE") but misses unsetting BACKEND_URL and
VITE_BACKEND_URL; update the setup to also call
EnvVarGuard::unset("BACKEND_URL") and EnvVarGuard::unset("VITE_BACKEND_URL") (or
the equivalent guard helpers) so this E2E test has the same environment
isolation as the rest of the file and cannot inherit shell/CI values.
---
Nitpick comments:
In `@app/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsx`:
- Around line 47-60: Add a Windows-path rendering assertion to the
UninstallSkillConfirmDialog test: create or reuse the fixture but set
skill.location to "C:\\Users\\me\\.openhuman\\skills\\weather-helper\\SKILL.md",
render <UninstallSkillConfirmDialog skill={fixture} ... />, and assert that the
dialog shows the stripped path
"C:\\Users\\me\\.openhuman\\skills\\weather-helper" (use a string or a RegExp
that properly escapes backslashes) so the suffix stripping behavior is locked
for Windows paths.
In `@tests/json_rpc_e2e.rs`:
- Around line 2383-2387: The assertion that checks traversal_msg is too
permissive—remove the `"not installed"` alternative so the test enforces a
traversal-specific rejection for inputs like name: "../etc"; update the assert
that currently references traversal_msg to only accept traversal-related
substrings (e.g., contains "path separators" or "path escapes") and fail if the
message is "not installed" or anything else, ensuring the traversal rejection is
explicitly validated.
- Around line 2281-2289: The test is missing the wire-level failure case for a
directory that exists but lacks SKILL.md; add an RPC invocation of
"openhuman.skills_uninstall" using UninstallSkillParams that targets a created
skill directory without SKILL.md (similar to the existing traversal and
not-installed cases), then assert the JSON-RPC response forwards the backend
error message for the missing SKILL.md case (the same string the
uninstall_skill(...) helper would return) and uses the expected error
shape/status; locate the existing assertions around UninstallSkillParams and
reuse their request/response decoding helpers to add this extra assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df289651-5966-4dfd-95de-d53ee99d8687
📒 Files selected for processing (6)
app/src/components/skills/UninstallSkillConfirmDialog.tsxapp/src/components/skills/__tests__/UninstallSkillConfirmDialog.test.tsxsrc/openhuman/skills/inject.rssrc/openhuman/skills/ops.rssrc/openhuman/skills/schemas.rstests/json_rpc_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/skills/schemas.rs
Comment on lines
+111
to
+114
| {skill.location && ( | ||
| <p className="mt-3 break-all rounded-lg bg-stone-50 px-3 py-2 font-mono text-[11px] text-stone-600"> | ||
| {skill.location.replace(/\/SKILL\.md$/i, '')} | ||
| </p> |
Contributor
There was a problem hiding this comment.
Handle Windows separators when stripping SKILL.md.
Current regex only removes /SKILL.md, so \SKILL.md remains visible on Windows paths.
Patch
- {skill.location.replace(/\/SKILL\.md$/i, '')}
+ {skill.location.replace(/[\\/]SKILL\.md$/i, '')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {skill.location && ( | |
| <p className="mt-3 break-all rounded-lg bg-stone-50 px-3 py-2 font-mono text-[11px] text-stone-600"> | |
| {skill.location.replace(/\/SKILL\.md$/i, '')} | |
| </p> | |
| {skill.location && ( | |
| <p className="mt-3 break-all rounded-lg bg-stone-50 px-3 py-2 font-mono text-[11px] text-stone-600"> | |
| {skill.location.replace(/[\\/]SKILL\.md$/i, '')} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/skills/UninstallSkillConfirmDialog.tsx` around lines 111 -
114, The code in UninstallSkillConfirmDialog uses
skill.location.replace(/\/SKILL\.md$/i, '') which only strips POSIX separators;
update the replacement to handle Windows backslashes as well (e.g., match either
'/' or '\' before SKILL.md) or normalize the path separators before replacing so
both "C:/.../SKILL.md" and "C:\...\SKILL.md" become the directory; locate the
skill.location.replace call in UninstallSkillConfirmDialog and change the regex
to handle [\/\\]SKILL\.md$ (case-insensitive) or normalize separators first and
then remove "/SKILL.md".
Comment on lines
+2296
to
+2298
| let _home_guard = EnvVarGuard::set_to_path("HOME", home); | ||
| let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE"); | ||
|
|
Contributor
There was a problem hiding this comment.
Match env isolation with the rest of this E2E file.
This test should also unset BACKEND_URL and VITE_BACKEND_URL; otherwise it can inherit shell config and become flaky in local/CI runs.
Patch
let _home_guard = EnvVarGuard::set_to_path("HOME", home);
let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE");
+ let _backend_url_guard = EnvVarGuard::unset("BACKEND_URL");
+ let _vite_backend_guard = EnvVarGuard::unset("VITE_BACKEND_URL");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let _home_guard = EnvVarGuard::set_to_path("HOME", home); | |
| let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE"); | |
| let _home_guard = EnvVarGuard::set_to_path("HOME", home); | |
| let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE"); | |
| let _backend_url_guard = EnvVarGuard::unset("BACKEND_URL"); | |
| let _vite_backend_guard = EnvVarGuard::unset("VITE_BACKEND_URL"); | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/json_rpc_e2e.rs` around lines 2296 - 2298, The test currently sets HOME
and unsets OPENHUMAN_WORKSPACE using EnvVarGuard::set_to_path("HOME", home) and
EnvVarGuard::unset("OPENHUMAN_WORKSPACE") but misses unsetting BACKEND_URL and
VITE_BACKEND_URL; update the setup to also call
EnvVarGuard::unset("BACKEND_URL") and EnvVarGuard::unset("VITE_BACKEND_URL") (or
the equivalent guard helpers) so this E2E test has the same environment
isolation as the rest of the file and cannot inherit shell/CI values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Close the uninstall gap on the #781 umbrella — after #740 landed SKILL.md browse/create/install and #807 wired agent inference, users still had no way to remove a skill from the UI. This PR adds
openhuman.skills_uninstall, a typedskillsApi.uninstallSkill, and a confirm-dialog action surfaced from the overflow menu on user-scope SKILL.md cards.Five micro-commits:
fix(threads/tests): import collapse_whitespace to unblock lib test build— drive-by;cargo test --libfails onupstream/mainbecause the new tests from [codex] Harden threads schema parse contract tests #797 callcollapse_whitespacewithout importing it. One-line fix so the rest of the diff can be validated locally.feat(skills/core): uninstall_skill fn with canonicalize + SKILL.md guards—openhuman::skills::ops::uninstall_skill+UninstallSkillParams/UninstallSkillOutcome+ 5 unit tests.feat(skills/rpc): register skills_uninstall controller + wire schema— follows theController-only exposurerule from CLAUDE.md; added toall_skills_registered_controllersandskills_schemas.feat(app/api): skillsApi.uninstallSkill wrapper + UninstallSkillResult types— mirrors the existinginstallSkillFromUrl/createSkillpatterns.feat(app/ui): UninstallSkillConfirmDialog + overflow menu action on SKILL.md cards— destructive-action confirm dialog; wired into Skills.tsx for user-scope non-legacy rows; vitest covers 5 paths.Problem
#740 landed browse / create / install from URL and #807 wired SKILL.md bodies into
Agent::turn, so installed skills both list in the UI and reach the LLM at inference time. But the only way to remove a broken, outdated, or one-off skill wasrm -rf ~/.openhuman/skills/<name>/from a terminal. Users reported this was a real friction point: with no reversible uninstall, installing a new skill from a URL is a one-way trip — bad enough that users will avoid installing anything they are not certain about, which defeats the point of the install flow.Solution
Backend (
src/openhuman/skills/ops.rs)New
uninstall_skill(params, home_dir_override) -> Result<UninstallSkillOutcome, String>:/,\, or..and any name exceedingMAX_NAME_LEN(64) before any filesystem call.skills_root = <home>/.openhuman/skillsand the candidate<root>/<name>. Assert the canonical candidate stillstarts_with(canonical_root)— blocks..traversal and symlink escape in one check.symlink_metadatarejects symlinks explicitly (belt + braces after canonicalisation) and refuses non-directory entries.SKILL.mdto exist in the candidate. We only remove things that look like skills we installed, not arbitrary directories the user might have dropped under~/.openhuman/skills/.fs::remove_dir_all.Only user-scope is supported. Project-scope (
<ws>/.openhuman/skills/) and legacy (<ws>/skills/) are read-only — the user owns those trees via git/editor/filesystem and exposing a destructive delete over RPC invites foot-guns from rogue tool calls.Five unit tests cover the happy path plus every guard (path-traversal names, empty name, skill not installed, dir without
SKILL.md, symlink-escape out of tree).RPC (
src/openhuman/skills/schemas.rs)Registered via the controller registry — no edits to
core/cli.rsorcore/jsonrpc.rs.skills_schemas("skills_uninstall")declares thenameinput and thename/removed_path/scopeoutputs;handle_skills_uninstalldeserialises viaSkillsUninstallParamsWire, delegates touninstall_skill(.., None), and surfaces backend errors verbatim so the dialog can show them inline.Frontend API (
app/src/services/api/skillsApi.ts)Thin typed wrapper mirroring
installSkillFromUrl/createSkill: snake_case wire → camelCase result, envelope unwrap, namespaceddebuglog entries.UI (
app/src/components/skills/UninstallSkillConfirmDialog.tsx,app/src/pages/Skills.tsx)skill.scope === 'user' && !skill.legacy. Project and legacy scopes stay read-only (and the RPC would reject them anyway).InstallSkillDialog/CreateSkillModal— see.claude/rules/15-settings-modal-system.md)./SKILL.mdstripped for readability), a destructive warning line, and Cancel / Uninstall buttons.onUninstalled(result)then close. The page drops the row from local state for instant feedback, refetches to surface any previously-shadowed project-scope duplicate, and closes the detail drawer if it was showing the removed skill.Submission Checklist
cargo test -p openhuman --lib skills::, all 74 skills tests green) and 5 new vitest cases (yarn test:unit UninstallSkillConfirmDialog, 5/5 green).moltbookvia URL, clicked ⋯ → Uninstall → confirmed, card disappeared, detail drawer closed,~/.openhuman/skills/moltbook/removed from disk.uninstall_skill,UninstallSkillParams,UninstallSkillOutcome, the schema arm, and the handler. Module-level comment onUninstallSkillConfirmDialog.tsxspells out the UI contract.Impact
openhuman_corelib). Single synchronous filesystem call gated by four validation checks. No new dependencies. No new threads or async context.starts_withcheck prevents.., symlink, and absolute-path escapes. Name validation happens before any filesystem access so a maliciousnamenever touchescanonicalize.SKILL.mdpresence check prevents the RPC from being used to nuke arbitrary user directories that happen to sit under~/.openhuman/skills/.remove_dir_all— the SKILL.md skills we install are small (<1 MiB SKILL.md plus optionalscripts/+references/+assets/).SkillSummarywire format unchanged; no existing RPCs changed. Frontend: discovered SKILL.md cards gain an overflow menu entry, everything else is identical.Related
uninstall/enable RPCs+InstalledSkillCard redesigncheckboxes for the uninstall half. Remaining feat(skills): complete #681 acceptance — package install, SKILL.md inference wiring, UI polish, docs #781 items stay in that issue (package install, enable/disable RPCs, Moltbook/Solana E2E, architecture docs).feat(skills): agentic loop wiring for SKILL.md bodies); branches straight offupstream/mainpost-feat(skills): SKILL.md skills UI — browse, create, install from URL (#681) #740 merge.skills.toggle_enabledfor disable-without-delete via a frontmatter flag.removedPathin a success toast (dialog currently closes silently on success).SkillDetailDraweritself so uninstall is reachable from the detail view too.Summary by CodeRabbit