feat(skills): SKILL.md skills UI — browse, create, install from URL (#681)#740
Merged
senamakel merged 20 commits intotinyhumansai:mainfrom Apr 22, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds end-to-end SKILL.md support: new frontend UI (create/install modals, detail drawer, resource tree/preview), a typed frontend Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Skills Page
participant CreateModal as CreateSkillModal
participant API as skillsApi
participant RPC as Core JSON‑RPC
participant FS as File System
User->>Frontend: Open "New skill"
Frontend->>CreateModal: Mount modal
User->>CreateModal: Submit form
CreateModal->>API: createSkill(params)
API->>RPC: callCoreRpc('openhuman.skills_create', params)
RPC->>FS: Scaffold skill dir + write SKILL.md
RPC-->>API: SkillSummary
API-->>CreateModal: Created skill
CreateModal->>Frontend: onCreated(skill)
Frontend->>Frontend: Add/refresh discoveredSkills
sequenceDiagram
actor User
participant Frontend as Skills Page
participant InstallDialog as InstallSkillDialog
participant API as skillsApi
participant RPC as Core JSON‑RPC
participant Network as HTTPS Host
participant FS as File System
User->>Frontend: Open "Install from URL"
User->>InstallDialog: Submit URL (+timeout)
InstallDialog->>API: installSkillFromUrl({url, timeout})
API->>RPC: callCoreRpc('openhuman.skills_install_from_url', params)
RPC->>Network: Fetch SKILL.md
Network-->>RPC: Content
RPC->>RPC: Parse & validate frontmatter
RPC->>FS: Atomic write SKILL.md, discover skill
RPC-->>API: { new_skills, stdout, stderr }
API-->>InstallDialog: Result
InstallDialog->>Frontend: onInstalled(result)
Frontend->>Frontend: Refresh & select installed skill
sequenceDiagram
actor User
participant Frontend as Skills Page
participant Drawer as SkillDetailDrawer
participant Tree as SkillResourceTree
participant Preview as SkillResourcePreview
participant API as skillsApi
participant RPC as Core JSON‑RPC
User->>Frontend: Open skill details
Frontend->>Drawer: Mount drawer
User->>Tree: Select resource path
Tree->>Drawer: onSelect(path)
Drawer->>Preview: Mount preview(skillId,path)
Preview->>API: readSkillResource({skillId,relativePath})
API->>RPC: callCoreRpc('openhuman.skills_read_resource', params)
RPC->>RPC: Validate path & read file
RPC-->>API: {content, bytes}
API-->>Preview: Content & size
Preview->>User: Render file + size
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
oxoxDev
force-pushed
the
feat/681-skill-detail-drawer
branch
from
6b8968f to
dc0f531
Compare
This was referenced Apr 22, 2026
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
app/src/components/skills/SkillResourceTree.tsx (1)
96-109: Expose selected state to assistive tech on resource buttons.Selection is currently visual only; add ARIA state so screen-reader users can detect which resource is active.
♿ Suggested tweak
<button type="button" + aria-pressed={isSelected} onClick={() => { log('click path=%s', path); onSelect(path); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/SkillResourceTree.tsx` around lines 96 - 109, The resource buttons only convey selection visually; update the button in SkillResourceTree (the button that calls onSelect(path) and uses isSelected/path) to expose the state to assistive tech by adding ARIA attributes — e.g., set aria-pressed={isSelected} (or aria-selected if you switch the role to option/treeitem) and/or aria-current={isSelected ? "true" : undefined} so screen readers can detect the active resource; keep the existing onClick, title, and styling logic unchanged.app/src/components/skills/__tests__/SkillDetailDrawer.test.tsx (1)
1-11: Docblock overstates coverage (backdrop click closes the drawer).I don’t see a backdrop-click assertion in this file. Either add that test or trim this bullet to keep test intent accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/__tests__/SkillDetailDrawer.test.tsx` around lines 1 - 11, The docblock at the top of SkillDetailDrawer.test.tsx incorrectly claims there is a "backdrop click closes the drawer" test; either remove that bullet or add the missing test: if removing, simply delete or edit that line in the header to reflect actual coverage; if adding, implement a test that renders the SkillDetailDrawer (passing a mock onClose), simulates a backdrop click (e.g., locate the backdrop element for SkillDetailDrawer and trigger a click via fireEvent or userEvent), and assert the mock onClose was called — reference the test file SkillDetailDrawer.test.tsx, the SkillDetailDrawer component, and the onClose/mock handler when adding the assertion.
🤖 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/CreateSkillModal.tsx`:
- Around line 58-76: The previewSlug function currently calls
name.normalize('NFKD') which decomposes accented letters (e.g., É -> E + ◌́) so
the ASCII filter keeps the base letter and yields different slugs than the Rust
backend; change previewSlug to avoid NFKD decomposition (remove the .normalize
call or use a form that does not decompose) so non-ASCII characters are dropped
byte-for-byte like the Rust slugifier, keep the ASCII-only test ((ch >= 'a' &&
ch <= 'z') || (ch >= '0' && ch <= '9')), preserve the hyphen/coalescing logic
and final trim (out.replace(/^-+|-+$/g, '')) so the frontend slug matches
src/openhuman/skills/ops.rs::slugify_skill_name().
In `@src/openhuman/skills/ops.rs`:
- Around line 1320-1345: The code currently creates target_dir (variable
target_dir) before writing SKILL_MD, which leaves an empty/partial directory
behind if std::fs::write or std::fs::rename fails and causes future installs to
fail due to target_dir.exists(); fix by staging the file write outside the final
install directory (e.g., write temp file in a system temp dir or in skills_root
with a unique name) and then atomically move/rename the staged file into
target_dir (or create a temp directory and rename the whole directory into
place) instead of creating target_dir first; additionally ensure cleanup on
error by removing any created temp files/dirs or removing target_dir if you must
create it early (handle errors from create_dir_all, write, and rename and
perform fs::remove_dir_all on target_dir or fs::remove_file on temp_file in the
same function where create_dir_all, std::fs::write, and std::fs::rename are
used).
- Around line 1405-1416: The blob/tree/raw branch handling assumes the ref is
exactly segments[3], which breaks for refs containing slashes; update the logic
in the normalized computation (the block that builds normalized from
parsed.path_segments() and the segments Vec) to locate the index of the marker
token ("blob", "tree" or "raw") with segments.iter().position(...) and then
treat everything after that index as a single tail (tail =
segments[index+1..].join("/")); keep owner = segments[0] and repo = segments[1]
and build the raw URL using that tail (e.g.
format!("https://raw.githubusercontent.com/{owner}/{repo}/{tail}")), and apply
the same approach for the "tree" / "raw" branches so refs with slashes are
preserved.
- Around line 925-947: Before creating a user-scope skill, ensure you reject
slug collisions that exist in the other scope: when params.scope ==
SkillScope::User (the block constructing scope_root), perform a cross-scope
lookup for the desired slug (the same check used later that does find(|s| s.name
== slug) or reuse the discover_skills()/list_skills() helper) to see if a
trusted project-scope skill in workspace_dir already owns that slug and return
an Err if so; similarly, when creating a project-scope skill, check the user
scope (home_dir.join(".openhuman").join("skills")) for the same slug and error
if found. Use the existing symbols params.scope, slug, scope_root,
workspace_dir, home_dir and the discovery function used elsewhere to implement
the cross-scope existence check before writing the new directory (also apply the
same fix around the other creation site referenced at lines ~1002-1006).
- Around line 977-985: create_skill_inner() currently passes &slug into
render_skill_md(), causing the slug to be written into the frontmatter name and
Markdown heading; change the call(s) to pass the original user-entered name (the
variable that holds the friendly title from the form, e.g. params.name or name)
into render_skill_md() instead of &slug so the human-readable title is
persisted; update every place render_skill_md(...) is called (including the
other occurrence around the second create path) to use the user-provided name
variable and ensure render_skill_md() continues to accept &str for the name
parameter.
---
Nitpick comments:
In `@app/src/components/skills/__tests__/SkillDetailDrawer.test.tsx`:
- Around line 1-11: The docblock at the top of SkillDetailDrawer.test.tsx
incorrectly claims there is a "backdrop click closes the drawer" test; either
remove that bullet or add the missing test: if removing, simply delete or edit
that line in the header to reflect actual coverage; if adding, implement a test
that renders the SkillDetailDrawer (passing a mock onClose), simulates a
backdrop click (e.g., locate the backdrop element for SkillDetailDrawer and
trigger a click via fireEvent or userEvent), and assert the mock onClose was
called — reference the test file SkillDetailDrawer.test.tsx, the
SkillDetailDrawer component, and the onClose/mock handler when adding the
assertion.
In `@app/src/components/skills/SkillResourceTree.tsx`:
- Around line 96-109: The resource buttons only convey selection visually;
update the button in SkillResourceTree (the button that calls onSelect(path) and
uses isSelected/path) to expose the state to assistive tech by adding ARIA
attributes — e.g., set aria-pressed={isSelected} (or aria-selected if you switch
the role to option/treeitem) and/or aria-current={isSelected ? "true" :
undefined} so screen readers can detect the active resource; keep the existing
onClick, title, and styling logic unchanged.
🪄 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: e87b65aa-3489-47ac-ad52-3a56856b912c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
app/src/components/skills/CreateSkillModal.tsxapp/src/components/skills/InstallSkillDialog.tsxapp/src/components/skills/SkillDetailDrawer.tsxapp/src/components/skills/SkillResourcePreview.tsxapp/src/components/skills/SkillResourceTree.tsxapp/src/components/skills/__tests__/CreateSkillModal.test.tsxapp/src/components/skills/__tests__/InstallSkillDialog.test.tsxapp/src/components/skills/__tests__/SkillDetailDrawer.test.tsxapp/src/components/skills/__tests__/SkillResourcePreview.test.tsxapp/src/pages/Skills.tsxapp/src/services/api/__tests__/skillsApi.test.tsapp/src/services/api/skillsApi.tssrc/core/all.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/ops.rssrc/openhuman/skills/schemas.rs
Comment on lines
+58
to
+76
| function previewSlug(name: string): string { | ||
| const lower = name.normalize('NFKD').toLowerCase(); | ||
| let out = ''; | ||
| let prevHyphen = false; | ||
| for (const ch of lower) { | ||
| // ASCII alnum pass-through | ||
| if ((ch >= 'a' && ch <= 'z') || (ch >= '0' && ch <= '9')) { | ||
| out += ch; | ||
| prevHyphen = false; | ||
| continue; | ||
| } | ||
| if ((ch === '-' || ch === '_' || /\s/.test(ch)) && !prevHyphen) { | ||
| out += '-'; | ||
| prevHyphen = true; | ||
| } | ||
| } | ||
| // Trim leading/trailing hyphens | ||
| return out.replace(/^-+|-+$/g, ''); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the slug preview byte-for-byte aligned with the Rust slugifier.
This preview normalizes with NFKD, so names like Éclair render as eclair here, while src/openhuman/skills/ops.rs::slugify_skill_name() drops non-ASCII characters and would create clair. That makes the modal promise a different install path than the backend will actually write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/skills/CreateSkillModal.tsx` around lines 58 - 76, The
previewSlug function currently calls name.normalize('NFKD') which decomposes
accented letters (e.g., É -> E + ◌́) so the ASCII filter keeps the base letter
and yields different slugs than the Rust backend; change previewSlug to avoid
NFKD decomposition (remove the .normalize call or use a form that does not
decompose) so non-ASCII characters are dropped byte-for-byte like the Rust
slugifier, keep the ASCII-only test ((ch >= 'a' && ch <= 'z') || (ch >= '0' &&
ch <= '9')), preserve the hyphen/coalescing logic and final trim
(out.replace(/^-+|-+$/g, '')) so the frontend slug matches
src/openhuman/skills/ops.rs::slugify_skill_name().
Comment on lines
+925
to
+947
| let scope_root = match params.scope { | ||
| SkillScope::User => { | ||
| let home = | ||
| home_dir.ok_or_else(|| "could not resolve user home directory".to_string())?; | ||
| home.join(".openhuman").join("skills") | ||
| } | ||
| SkillScope::Project => { | ||
| if !is_workspace_trusted(workspace_dir) { | ||
| return Err(format!( | ||
| "workspace {} is not trusted; create {}/.openhuman/trust to enable project-scope skills", | ||
| workspace_dir.display(), | ||
| workspace_dir.display(), | ||
| )); | ||
| } | ||
| workspace_dir.join(".openhuman").join("skills") | ||
| } | ||
| SkillScope::Legacy => { | ||
| return Err( | ||
| "cannot create skill in legacy scope; choose 'user' or 'project'".to_string(), | ||
| ); | ||
| } | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
Reject cross-scope slug collisions before creating a user-scope skill.
The existence check only looks inside the destination scope. If a trusted project-scope skill already owns the same slug, a user-scope create will still write a new directory, but re-discovery immediately shadows it and find(|s| s.name == slug) returns the existing project skill instead of the one that was just created. The caller gets the wrong Skill, and the new file is effectively invisible in that workspace.
Also applies to: 1002-1006
🤖 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 925 - 947, Before creating a
user-scope skill, ensure you reject slug collisions that exist in the other
scope: when params.scope == SkillScope::User (the block constructing
scope_root), perform a cross-scope lookup for the desired slug (the same check
used later that does find(|s| s.name == slug) or reuse the
discover_skills()/list_skills() helper) to see if a trusted project-scope skill
in workspace_dir already owns that slug and return an Err if so; similarly, when
creating a project-scope skill, check the user scope
(home_dir.join(".openhuman").join("skills")) for the same slug and error if
found. Use the existing symbols params.scope, slug, scope_root, workspace_dir,
home_dir and the discovery function used elsewhere to implement the cross-scope
existence check before writing the new directory (also apply the same fix around
the other creation site referenced at lines ~1002-1006).
Comment on lines
+977
to
+985
| let skill_md_path = skill_dir.join(SKILL_MD); | ||
| let skill_md = render_skill_md( | ||
| &slug, | ||
| description, | ||
| params.license.as_deref(), | ||
| params.author.as_deref(), | ||
| ¶ms.tags, | ||
| ¶ms.allowed_tools, | ||
| ); |
Contributor
There was a problem hiding this comment.
Persist the user-entered name, not the slug.
create_skill_inner() passes &slug into render_skill_md(), and render_skill_md() writes that value into frontmatter name: and the Markdown heading. A skill created from My Demo Skill therefore comes back as my-demo-skill in the catalog, which loses the human-readable title the form just collected.
Suggested fix
-fn render_skill_md(
- slug: &str,
+fn render_skill_md(
+ display_name: &str,
+ slug: &str,
description: &str,
license: Option<&str>,
author: Option<&str>,
tags: &[String],
allowed_tools: &[String],
) -> String {
let mut out = String::new();
out.push_str("---\n");
- out.push_str(&format!("name: {slug}\n"));
+ out.push_str(&format!("name: {}\n", yaml_scalar(display_name)));
out.push_str(&format!("description: {}\n", yaml_scalar(description)));
@@
}
out.push_str("---\n\n");
- out.push_str(&format!("# {slug}\n\n"));
+ out.push_str(&format!("# {}\n\n", display_name));
out.push_str(description); let skill_md = render_skill_md(
+ display_name,
&slug,
description,
params.license.as_deref(),
params.author.as_deref(),
¶ms.tags,Also applies to: 1045-1081
🤖 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 977 - 985, create_skill_inner()
currently passes &slug into render_skill_md(), causing the slug to be written
into the frontmatter name and Markdown heading; change the call(s) to pass the
original user-entered name (the variable that holds the friendly title from the
form, e.g. params.name or name) into render_skill_md() instead of &slug so the
human-readable title is persisted; update every place render_skill_md(...) is
called (including the other occurrence around the second create path) to use the
user-provided name variable and ensure render_skill_md() continues to accept
&str for the name parameter.
…ds (tinyhumansai#681) Introduces `read_skill_resource(skill_id, relative_path)` in the skills ops module. Used by the new `skills.read_resource` RPC (landed in a follow-up commit) to let the UI preview files bundled alongside a SKILL.md without having to shell out to the Node runtime. Guards rejecting each known attack surface have their own unit test: - empty skill_id / empty relative_path - unknown skill - absolute paths - `..` traversal escapes (checked after canonicalization against the skill root, reusing the pattern from the Node exec allowlist) - directory targets - symlinked leaves (reject via `symlink_metadata` before open) - files over the 128 KB cap - non-UTF-8 content (binary allowlist is text-only) Happy-path test covers a small text resource under the skill root. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ansai#681) Adds `skills` RPC namespace with `skills.list` and `skills.read_resource` handlers. `read_resource` delegates to `read_skill_resource` (previous commit) and surfaces path-traversal / size / encoding errors back to the caller verbatim so the UI can render the error string as-is. - `src/openhuman/skills/schemas.rs` (new): controller + schema definitions, plus unit tests for schema name stability, round-trip of the minimum `SkillSummary` fields, and controller list/schema length parity. - `src/openhuman/skills/mod.rs`: declare `pub mod schemas` and re-export `all_skills_controller_schemas`, `all_skills_registered_controllers`, and `skills_schemas`. - `src/core/all.rs`: register the controllers + schemas and add a namespace description so the RPC discovery endpoint surfaces it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…umansai#681) Thin typed wrapper around the `skills.list` and `skills.read_resource` RPCs added in the previous commit. The client normalises the backend response shape (bytes + UTF-8 content) and rethrows backend error strings verbatim so the preview pane can render them unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…inyhumansai#681) Presentational component that takes the `resources: PathBuf[]` from a loaded Skill and renders it as a grouped list (scripts, assets, references, etc. based on the first path segment). Selecting a leaf calls `onSelect(relativePath)` so the parent drawer can drive preview state. Stateless — no fetching, no effects. Styling follows the stone/coral design tokens used across the Skills page. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ansai#681) Presentational component that fetches a single bundled resource via `skillsApi.readSkillResource`. Backend caps payloads at 128 KB and either returns UTF-8 text or a plain error string, so the preview pane has three visual states: loading, error, success. - On error (e.g. "path escape", ">128KB", "non-UTF-8"), renders the backend message verbatim in a coral panel. - On success, renders a monospace pre block with the byte count in the footer. - `key={id:path}` on the mount site (in SkillDetailDrawer, next commit) drives a remount when the selected resource changes — so no setState-in-effect hack is needed to reset loading state. Vitest specs cover: loading state, success rendering with byte footer, error rendering for traversal / oversize / encoding strings, cancelled fetch guard on unmount. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…humansai#681) Slide-in right-hand drawer that displays frontmatter metadata (description, version, author, license, tags, allowed_tools) and hosts SkillResourceTree + SkillResourcePreview. Opened by clicking a skill card on the Skills page (wired in the next commit). - Focus management: on mount, focuses the close button via `window.requestAnimationFrame` and restores the previously focused element on unmount. - Esc + backdrop click dismiss. - Preview pane is conditionally rendered and keyed on `${skill.id}:${selectedResource}` so changing the selected resource remounts the previewer (avoids setState-in-effect pattern). Vitest specs cover: render with frontmatter, resource tree click opens preview, close button / Esc / backdrop dismiss paths, focus restoration, empty-resources case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…i#681) Wires the Skills page to the new drawer: clicking a skill card sets `selectedSkill` state, which mounts `SkillDetailDrawer`. Dismissing the drawer clears the state. Cards gain an explicit "View details" affordance for discoverability. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…oring (tinyhumansai#681) Adds `create_skill` in ops.rs plus the `skills.create` controller + handler in schemas.rs. Writes a minimal SKILL.md (with optional license/author/tags/ allowed-tools frontmatter) under the selected scope, scaffolds scripts/ references/assets subdirs, and re-discovers the skill to return the parsed SkillSummary. Legacy scope rejects; Project scope requires the trust marker; User scope is always allowed when a home directory is available. Hardened against path traversal the same way read_skill_resource is: canonicalize the scope root, canonicalize the target dir, reject unless the target starts with the root. Slug derivation is ASCII-only (collapse whitespace/ -/_ to a single hyphen, drop other chars, trim hyphens, enforce MAX_NAME_LEN). Tests (hermetic via create_skill_inner): - user-scope happy path (slug, metadata, SKILL.md on disk, subdirs) - slug collision rejection - invalid name (no alphanumerics) rejection - project-scope without trust marker rejection - project-scope with trust marker happy path - legacy-scope rejection - empty-description rejection - slugify edge cases Closes part of tinyhumansai#681 (backend scope for create flow).
…tinyhumansai#681) Introduces `install_skill_from_url(url, timeout_secs?)` — a JSON-RPC method that shells out to `npx --yes skills add <url>` under the managed Node runtime so the UI can install published SKILL.md packages directly. Security posture: - https scheme only (no http, file, ssh, git+https…) - Rejects `localhost`, `*.localhost`, `*.local`, RFC1918 private IPv4, loopback, link-local, multicast, broadcast, unspecified, 100.64/10 CGN, 0.0.0.0/8, and IPv6 loopback/unspecified/multicast, fc00::/7 ULA, fe80::/10 link-local. Explicitly covers 169.254.169.254 cloud metadata. - Trims + caps URL at 2048 chars; parses with the `url` crate. - IPv6 brackets stripped from `host_str()` before address parse. Process posture: - Reuses `NodeBootstrap` so the managed toolchain resolves first. - `env_clear()` + explicit PATH injection (bootstrap bin_dir first) + a narrow safe-env allow-list (HOME, TERM, LANG, LC_ALL, LC_CTYPE, USER, SHELL, TMPDIR). Matches the npm_exec pattern from tinyhumansai#723. - Default 60s wall-clock timeout, capped at 600s. - Captures stdout/stderr; returns both on success or failure. - Diff-based `new_skills`: snapshots discovered skills pre-install and reports slugs that appear post-install. Surface: - JSON-RPC: `openhuman.skills_install_from_url` params: { url: string, timeout_secs?: number } result: { url, stdout, stderr, new_skills[] } - Wired into `all_skills_controller_schemas()` and `all_skills_registered_controllers()`. Tests (5 new unit tests, all pass — 51/51 in skills::): - validate_install_url_accepts_public_https - validate_install_url_rejects_non_https_scheme (http, file, ftp, ssh, git+https, javascript) - validate_install_url_rejects_empty_and_oversized - validate_install_url_rejects_private_and_loopback (20 URLs inc. CGN, cloud metadata, IPv6 ULA/link-local/loopback/multicast) - validate_install_url_rejects_malformed (missing scheme, empty host, non-https scheme, unparseable bracketed host) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…inyhumansai#681) Adds typed frontend wrappers for the two new skill-authoring RPC methods: - `skillsApi.createSkill(input)` — scaffolds a new SKILL.md skill via `openhuman.skills_create`. Accepts camelCase `allowedTools` and rekeys it to the `allowed-tools` spelling the SKILL.md frontmatter convention expects, matching `SkillsCreateParams` in `src/openhuman/skills/schemas.rs`. Optional fields are only sent when explicitly provided so the Rust `#[serde(default)]` defaults apply cleanly. - `skillsApi.installSkillFromUrl(input)` — installs a published skill package via `openhuman.skills_install_from_url`. Accepts camelCase `timeoutSecs` and rekeys it to `timeout_secs`. Normalizes the response (snake_case `new_skills` -> camelCase `newSkills`, missing list -> []). Both wrappers reuse the existing `unwrapEnvelope` helper so they tolerate either a bare RPC payload or the `{ data: … }` envelope some transports emit. Adds `CreateSkillInput`, `InstallSkillFromUrlInput`, and `InstallSkillFromUrlResult` type exports for downstream modal components. Tests (vitest, 6 new specs, all pass): - createSkill forwards inputs and rekeys allowedTools - createSkill omits optional fields when absent - createSkill unwraps envelope responses - installSkillFromUrl forwards url and rekeys timeoutSecs - installSkillFromUrl omits timeout_secs + defaults newSkills to [] - installSkillFromUrl unwraps envelope responses Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…umansai#681) Adds a centered white modal that scaffolds a new SKILL.md skill via `skillsApi.createSkill`, matching the settings-modal design rules (520px desktop, 16px radius, backdrop+blur, Escape/click-out to close, focus capture). Form fields mirror the Rust `SkillsCreateParams` schema: - name (required) — display name, also slugified into the on-disk directory; a live slug preview surfaces what will hit disk - description (required) — short prose; written as the `description:` field in the generated YAML frontmatter - scope (user | project radio) — `legacy` is hidden because that layout is read-only and being phased out - license (optional) — free-form SPDX-style string - author (optional) - tags (optional, CSV) — normalised client-side; empty entries dropped - allowedTools (optional, CSV) — rekeyed to `allowed-tools` on the JSON-RPC wire by `skillsApi.createSkill` The slug preview mirrors `slugify_skill_name` on the Rust side (lowercase ASCII alnum + `-`, collapse repeats, trim edge hyphens) so the user sees what the Rust slugifier will produce; the Rust side stays authoritative when the skill is persisted. On success `onCreated(skill)` fires with the freshly-discovered `SkillSummary`, letting the parent grid insert the new row without a full refetch. On failure the Rust error string is surfaced verbatim in a coral-styled alert and the submit button re-enables. Vitest specs cover: required-field rendering, live slug preview, submit-disabled gating, Escape close, wire-format rekey of `allowedTools` → `'allowed-tools'`, `onCreated` dispatch, and error-banner recovery. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ai#681) Adds a centered white modal that installs a published skill package via `skillsApi.installSkillFromUrl`. The Rust side shells out to `npx --yes skills add <url>` under the managed Node toolchain, with an allow-list on the URL (https only, no private/loopback/link-local/ multicast/cloud-metadata hosts) and a wall-clock timeout (default 60s, max 600s). UI contract: - Single URL input plus optional timeout in seconds. - Client-side `isLikelyValidUrl` fails fast on non-https URLs so the user doesn't pay a round-trip for shape errors the Rust side would reject anyway; the Rust side remains authoritative. - Timeout field validates `1 <= n <= 600` client-side to mirror the server-side clamp range. - While the RPC is in flight we render a spinner with "Running `npx skills add`…" copy and disable close / backdrop dismiss so we don't orphan the subprocess. - On success we surface the list of `newSkills` (ids that appeared post-install) plus captured stdout/stderr panes inside collapsible <details> elements, then hand the full result back to the caller via `onInstalled` so the parent can refetch the skills list and auto-select the new row. - On failure the Rust error string is rendered verbatim in a coral alert and the submit button re-enables. Vitest specs cover: required-field rendering, URL shape gating (empty, malformed, http://, https://), timeout range validation, `timeoutSecs` forwarding on submit, success panel with newSkills rendering, blank timeout omitted from payload, and error-banner recovery. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…yhumansai#681) Adds a header row on the Skills page with two buttons: - **New skill** → opens `CreateSkillModal` - **Install from URL** → opens `InstallSkillDialog` Extracts the existing `listSkills` effect into a reusable `refreshDiscoveredSkills` helper so both new flows can reconcile their results against the freshly-discovered `SkillSummary` rows rather than relying on the optimistic payload from the RPC alone. Create flow: - Optimistically appends the returned `SkillSummary` to `discoveredSkills` (dedupe by id). - Auto-opens the detail drawer for the new skill so the user lands in context — matches the install flow's UX. - Follows up with `refreshDiscoveredSkills()` so version/author/ warnings picked up by the Rust discoverer end up in state too. Install flow: - Always refreshes the list (the install can add multiple skills if the package declares several). - Auto-opens the detail drawer for the first newly-installed skill when at least one id is reported back; otherwise leaves the grid in its refreshed state. Both buttons sit in a flush `max-w-lg` header above the existing search bar, styled consistent with `UnifiedSkillCard` CTAs — ocean primary for "New skill" (positive action), neutral stone for "Install from URL" (secondary). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…inyhumansai#681) Installer no longer shells out to the vercel-labs/skills CLI. It now fetches SKILL.md over HTTPS, validates YAML frontmatter, and writes into the user's skills dir. Size cap (1 MiB), timeout clamp (1-600s), GitHub blob->raw URL normalization, and path-traversal guards are covered by unit tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… fetch (tinyhumansai#681) Dialog subtitle, helper text, and in-flight indicator reflect the new direct SKILL.md fetch flow. Errors from the core are categorized into friendly titles (URL rejected, too large, timeout, parse failure, already installed, write failed) with the raw backend message tucked under a details disclosure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rization (tinyhumansai#681) Fixtures updated to raw GitHub SKILL.md URLs. New cases assert the categorization helper surfaces the right title for invalid SKILL.md, unsupported URL form, and unknown backend errors (raw text hidden under details). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…#681) Project scope (`<ws>/.openhuman/skills/`) is gated on a `<ws>/.openhuman/trust` marker that the workspace rarely has, so freshly-installed skills were invisible to `skills.list` until the user opted the workspace into trust. Route installs to `~/.openhuman/skills/<slug>` — the user-scope root that `discover_skills` always scans — so "Install from URL" surfaces the new skill immediately. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…yhumansai#681) `install_from_url` stopped shelling out to `npx --yes skills add <url>` when it was rewritten to fetch SKILL.md over HTTPS directly, but schema descriptions and SDK wrappers still described the old subprocess flow. Fix the module-level rustdoc, the JSON-RPC schema `description`/`comment` fields, and the TS client wrapper doc comments so the surface documents what actually runs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
oxoxDev
force-pushed
the
feat/681-skill-detail-drawer
branch
from
2e0b1a3 to
0fab789
Compare
…nyhumansai#681) Two fixes surfaced by CodeRabbit review on PR tinyhumansai#740: * `validate_install_url` only inspected literal-IP hosts, so a public-looking hostname like `evil.example.com` with an A record pointing at `127.0.0.1` / `169.254.x` / etc. would still be handed to `reqwest`. Resolve the host via `tokio::net::lookup_host` before the GET and reject if any returned address falls in loopback / private / link-local / multicast / unspecified ranges. Document the remaining DNS-rebinding gap (pinning to a `SocketAddr` + custom reqwest resolver is tracked separately). * If `std::fs::write` or `std::fs::rename` fails after `create_dir_all` succeeded, the empty/partial target directory used to survive and permanently block retries under the same slug. Wrap the write+rename in a rollback that removes the temp file + the just-created directory on failure (best-effort; cleanup errors are logged and the original write error is surfaced). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
app/src/components/skills/InstallSkillDialog.tsx (1)
43-59: Consider removing unusedonSelectSkillprop or marking it as reserved.The
onSelectSkillprop is declared but never invoked within the component. The docstring notes it's "for symmetry withCreateSkillModal" and "not invoked by the dialog itself." If this is intentional API surface for future use, consider adding a//@ts-expect-error`` or removing it until needed to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/skills/InstallSkillDialog.tsx` around lines 43 - 59, The onSelectSkill prop declared on the Props interface is never used inside the InstallSkillDialog component; either remove onSelectSkill from Props (and any call sites) to shrink the public API, or keep it intentionally but add a clear comment on the Props interface and above the prop (e.g., “// reserved for symmetry with CreateSkillModal — intentionally unused”) so reviewers won’t treat it as dead code; update the Props interface and the InstallSkillDialog component signature accordingly and ensure any references to onSelectSkill elsewhere are removed if you choose the former.app/src/services/api/skillsApi.ts (1)
84-92: NarrowCreateSkillInput.scopeto creatable scopes.
SkillScopeincludes'legacy', butskills_createonly documents'user'and'project'. Exposing the broader union here makes an unsupported value look valid and pushes the failure to runtime.Proposed fix
export type SkillScope = 'user' | 'project' | 'legacy'; +export type CreateSkillScope = 'user' | 'project'; @@ export interface CreateSkillInput { name: string; description: string; - scope?: SkillScope; + scope?: CreateSkillScope; license?: string; author?: string; tags?: string[]; allowedTools?: string[]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/api/skillsApi.ts` around lines 84 - 92, The CreateSkillInput.scope field currently uses the broad SkillScope (which includes 'legacy') but the skills_create endpoint only accepts 'user' or 'project'; change CreateSkillInput so scope is narrowed to only the creatable values (e.g., replace scope?: SkillScope with scope?: 'user' | 'project' or introduce a new type alias like CreatableSkillScope and use that) and update any references to CreateSkillInput in the codebase (e.g., functions that call skills_create) to use the narrowed type so invalid values are caught at compile time rather than runtime.src/openhuman/skills/schemas.rs (1)
163-193: Match the standardschemas.rsexport surface.This module exports
skills_schemas/all_skills_*, but the repo contract for domain schema modules isschemas,all_controller_schemas, andall_registered_controllers. Keeping the common surface avoids per-domain exceptions and makes these modules interchangeable.As per coding guidelines,
src/openhuman/**/schemas.rsrequiresschemas(function: &str) -> ControllerSchema,all_controller_schemas() -> Vec<ControllerSchema>, andall_registered_controllers() -> Vec<RegisteredController>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/schemas.rs` around lines 163 - 193, Rename the exported functions in this module to match the repo contract: change skills_schemas(function: &str) -> ControllerSchema to schemas(function: &str) -> ControllerSchema, change all_skills_controller_schemas() -> Vec<ControllerSchema> to all_controller_schemas(), and change all_skills_registered_controllers() -> Vec<RegisteredController> to all_registered_controllers(); update the vectors that currently call skills_schemas(...) and construct RegisteredController entries (which reference handlers handle_skills_list, handle_skills_read_resource, handle_skills_create, handle_skills_install_from_url) to call the renamed schemas(...) and ensure any external call sites or uses import the new symbol names so the module exports are schemas, all_controller_schemas, and all_registered_controllers as required by the coding guidelines.
🤖 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/services/api/skillsApi.ts`:
- Around line 225-229: The installSkillFromUrl function currently logs the raw
input.url with log('installSkillFromUrl: request url=%s', input.url); — replace
that raw log with a namespaced debug call and a redacted URL: parse input.url in
installSkillFromUrl, strip credentials and query string (or mask query values)
and log only the safe parts (hostname + pathname or a masked query) using a
debug namespace like debug('services:skillsApi') instead of the global log
function; ensure no JWTs, API keys, or full PII from input.url are emitted.
- Around line 101-107: The InstallSkillFromUrlInput.timeoutSecs must be
validated as a non-negative integer before sending: update the runtime check
where InstallSkillFromUrlInput is consumed (the request-sender / install-skill
call at the usage site) to verify Number.isInteger(timeoutSecs) and timeoutSecs
>= 0 (or undefined), and reject/throw a clear client-side error if not; do not
rely on server clamping. Ensure the check runs before serializing the payload so
fractional or negative values are blocked (or coerce intentionally only if you
add explicit truncation), and include this validation alongside the
InstallSkillFromUrlInput handling.
In `@src/openhuman/skills/schemas.rs`:
- Around line 457-470: The debug logs in the install_from_url RPC handler
currently log full URLs (wire.url and outcome.url), which can leak
tokens/credentials; update the tracing::debug! invocations in the
install_from_url flow (the handler that converts wire into
InstallSkillFromUrlParams and calls install_skill_from_url) to log a redacted
URL instead of the raw string—implement or call a small sanitizer (e.g.,
redact_query_or_credentials(url: &str) -> String) and pass its output to the
trace statements for both the initial wire.url and outcome.url so no query
params or userinfo are written to logs.
- Around line 431-432: The mutating handlers (skills.create and
skills.install_from_url) currently call resolve_workspace_dir() /
resolve_config() which silently fall back to a default workspace on errors;
change these code paths so that resolve_workspace_dir() and resolve_config()
errors are propagated (fail-closed) for write operations instead of returning a
fallback. Specifically, in the callers around create_skill(...) and the
install_from_url handler, detect and return the error from
resolve_workspace_dir()/resolve_config() (or convert it to a suitable API error)
before attempting create_skill or any disk writes, ensuring that
create_skill(...) and any disk-mutating logic only run when an authoritative
workspace/config was successfully resolved. Ensure read-only handlers may keep
the existing fallback behavior but all mutating functions require a successful,
non-fallback resolution.
---
Nitpick comments:
In `@app/src/components/skills/InstallSkillDialog.tsx`:
- Around line 43-59: The onSelectSkill prop declared on the Props interface is
never used inside the InstallSkillDialog component; either remove onSelectSkill
from Props (and any call sites) to shrink the public API, or keep it
intentionally but add a clear comment on the Props interface and above the prop
(e.g., “// reserved for symmetry with CreateSkillModal — intentionally unused”)
so reviewers won’t treat it as dead code; update the Props interface and the
InstallSkillDialog component signature accordingly and ensure any references to
onSelectSkill elsewhere are removed if you choose the former.
In `@app/src/services/api/skillsApi.ts`:
- Around line 84-92: The CreateSkillInput.scope field currently uses the broad
SkillScope (which includes 'legacy') but the skills_create endpoint only accepts
'user' or 'project'; change CreateSkillInput so scope is narrowed to only the
creatable values (e.g., replace scope?: SkillScope with scope?: 'user' |
'project' or introduce a new type alias like CreatableSkillScope and use that)
and update any references to CreateSkillInput in the codebase (e.g., functions
that call skills_create) to use the narrowed type so invalid values are caught
at compile time rather than runtime.
In `@src/openhuman/skills/schemas.rs`:
- Around line 163-193: Rename the exported functions in this module to match the
repo contract: change skills_schemas(function: &str) -> ControllerSchema to
schemas(function: &str) -> ControllerSchema, change
all_skills_controller_schemas() -> Vec<ControllerSchema> to
all_controller_schemas(), and change all_skills_registered_controllers() ->
Vec<RegisteredController> to all_registered_controllers(); update the vectors
that currently call skills_schemas(...) and construct RegisteredController
entries (which reference handlers handle_skills_list,
handle_skills_read_resource, handle_skills_create,
handle_skills_install_from_url) to call the renamed schemas(...) and ensure any
external call sites or uses import the new symbol names so the module exports
are schemas, all_controller_schemas, and all_registered_controllers as required
by the coding guidelines.
🪄 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: ccbf3593-9632-465f-a8b3-d32758493e38
📒 Files selected for processing (16)
app/src/components/skills/CreateSkillModal.tsxapp/src/components/skills/InstallSkillDialog.tsxapp/src/components/skills/SkillDetailDrawer.tsxapp/src/components/skills/SkillResourcePreview.tsxapp/src/components/skills/SkillResourceTree.tsxapp/src/components/skills/__tests__/CreateSkillModal.test.tsxapp/src/components/skills/__tests__/InstallSkillDialog.test.tsxapp/src/components/skills/__tests__/SkillDetailDrawer.test.tsxapp/src/components/skills/__tests__/SkillResourcePreview.test.tsxapp/src/pages/Skills.tsxapp/src/services/api/__tests__/skillsApi.test.tsapp/src/services/api/skillsApi.tssrc/core/all.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/ops.rssrc/openhuman/skills/schemas.rs
✅ Files skipped from review due to trivial changes (2)
- src/openhuman/skills/mod.rs
- app/src/components/skills/tests/SkillDetailDrawer.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/all.rs
- app/src/components/skills/tests/CreateSkillModal.test.tsx
- app/src/components/skills/tests/SkillResourcePreview.test.tsx
- app/src/pages/Skills.tsx
Comment on lines
+101
to
+107
| * `timeoutSecs` is optional — the Rust side defaults to 60s and caps at | ||
| * 600s. Values outside that range are clamped server-side. | ||
| */ | ||
| export interface InstallSkillFromUrlInput { | ||
| url: string; | ||
| timeoutSecs?: number; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Serde with serde_jsonaccept JSON numbers like -1 or 0.5 when deserializing into a Rustu64, or are those values rejected as invalid?
💡 Result:
No, Serde with serde_json rejects JSON numbers like -1 or 0.5 when deserializing into a Rust u64. These values are rejected as invalid.
Citations:
- 1: https://cseweb.ucsd.edu/classes/sp22/cse223B-a/tribbler/serde_json/value/struct.Number.html
- 2: https://docs.rs/serde_json/latest/serde_json/value/enum.Value.html
- 3: https://docs.serde.rs/serde_json/struct.Number.html
- 4: https://docs.rs/serde_json/1.0.8/serde_json/struct.Number.html
- 5: https://github.com/serde-rs/json/blob/master/src/de.rs
🏁 Script executed:
sed -n '230,240p' app/src/services/api/skillsApi.tsRepository: tinyhumansai/openhuman
Length of output: 460
🏁 Script executed:
grep -n "timeoutSecs\|timeout_secs" app/src/services/api/skillsApi.tsRepository: tinyhumansai/openhuman
Length of output: 263
Validate timeoutSecs as an integer before sending it.
The interface accepts any number, but the backend expects a Rust u64. Negative and fractional values will fail deserialization before the server-side clamp logic runs, contradicting the documentation claim that "Values outside that range are clamped server-side." Either validate on the client (reject non-integer, non-positive values) or update the docs to reflect what actually gets clamped.
Occurs at: lines 106 (interface definition) and 235 (usage).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/services/api/skillsApi.ts` around lines 101 - 107, The
InstallSkillFromUrlInput.timeoutSecs must be validated as a non-negative integer
before sending: update the runtime check where InstallSkillFromUrlInput is
consumed (the request-sender / install-skill call at the usage site) to verify
Number.isInteger(timeoutSecs) and timeoutSecs >= 0 (or undefined), and
reject/throw a clear client-side error if not; do not rely on server clamping.
Ensure the check runs before serializing the payload so fractional or negative
values are blocked (or coerce intentionally only if you add explicit
truncation), and include this validation alongside the InstallSkillFromUrlInput
handling.
Comment on lines
+225
to
+229
| installSkillFromUrl: async ( | ||
| input: InstallSkillFromUrlInput | ||
| ): Promise<InstallSkillFromUrlResult> => { | ||
| log('installSkillFromUrl: request url=%s', input.url); | ||
| const response = await callCoreRpc< |
Contributor
There was a problem hiding this comment.
Don't log raw install URLs from the UI.
A full URL can include signed query params or embedded credentials. Emit a redacted form instead of the raw user input.
As per coding guidelines, React/TypeScript code in app/src/ must use namespaced debug logs for development diagnostics and avoid logging secrets, raw JWTs, API keys, or full PII.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/services/api/skillsApi.ts` around lines 225 - 229, The
installSkillFromUrl function currently logs the raw input.url with
log('installSkillFromUrl: request url=%s', input.url); — replace that raw log
with a namespaced debug call and a redacted URL: parse input.url in
installSkillFromUrl, strip credentials and query string (or mask query values)
and log only the safe parts (hostname + pathname or a masked query) using a
debug namespace like debug('services:skillsApi') instead of the global log
function; ensure no JWTs, API keys, or full PII from input.url are emitted.
Comment on lines
+431
to
+432
| let workspace = resolve_workspace_dir().await; | ||
| match create_skill(workspace.as_path(), payload.into()) { |
Contributor
There was a problem hiding this comment.
Fail closed for write operations when config/workspace resolution fails.
skills.create and skills.install_from_url both write to disk, but resolve_workspace_dir() / resolve_config() silently downgrade config load failures to a fallback workspace. That can create/install a skill into the wrong workspace after a timeout/error, and in the worst case drops to ./.openhuman/workspace if the default root lookup also fails. The read-only handlers can keep the fallback, but the mutating ones should require an authoritative workspace/config.
Also applies to: 462-465, 490-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/schemas.rs` around lines 431 - 432, The mutating
handlers (skills.create and skills.install_from_url) currently call
resolve_workspace_dir() / resolve_config() which silently fall back to a default
workspace on errors; change these code paths so that resolve_workspace_dir() and
resolve_config() errors are propagated (fail-closed) for write operations
instead of returning a fallback. Specifically, in the callers around
create_skill(...) and the install_from_url handler, detect and return the error
from resolve_workspace_dir()/resolve_config() (or convert it to a suitable API
error) before attempting create_skill or any disk writes, ensuring that
create_skill(...) and any disk-mutating logic only run when an authoritative
workspace/config was successfully resolved. Ensure read-only handlers may keep
the existing fallback behavior but all mutating functions require a successful,
non-fallback resolution.
Comment on lines
+457
to
+470
| tracing::debug!( | ||
| url = %wire.url, | ||
| timeout_secs = ?wire.timeout_secs, | ||
| "[skills][rpc] install_from_url" | ||
| ); | ||
| let config = resolve_config().await; | ||
| let workspace = config.workspace_dir.clone(); | ||
| let payload: InstallSkillFromUrlParams = wire.into(); | ||
| match install_skill_from_url(workspace.as_path(), payload).await { | ||
| Ok(outcome) => { | ||
| tracing::debug!( | ||
| url = %outcome.url, | ||
| new_count = outcome.new_skills.len(), | ||
| "[skills][rpc] install_from_url: ok" |
Contributor
There was a problem hiding this comment.
Redact install URLs before logging them.
Both debug logs emit the full install URL. Remote install links can contain query tokens or embedded credentials, so this leaks secrets into local logs on a security-sensitive path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/schemas.rs` around lines 457 - 470, The debug logs in
the install_from_url RPC handler currently log full URLs (wire.url and
outcome.url), which can leak tokens/credentials; update the tracing::debug!
invocations in the install_from_url flow (the handler that converts wire into
InstallSkillFromUrlParams and calls install_skill_from_url) to log a redacted
URL instead of the raw string—implement or call a small sanitizer (e.g.,
redact_query_or_credentials(url: &str) -> String) and pass its output to the
trace statements for both the initial wire.url and outcome.url so no query
params or userinfo are written to logs.
…sai#681) CI's cargo fmt (stable) rewraps the long `.map(..).unwrap_or(false)` chain that passed local fmt. Apply the break so the pre-push hook and the upstream lint job agree. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 tasks
5 tasks
This was referenced Apr 24, 2026
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
2 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
skills.read_resource,skills.create, andskills.install_from_url— under theskillsnamespace, wired through the controller registry.Problem
Issue #681 asks for a skills UI so users can view, create, and install SKILL.md-format skills without leaving the app. Before this change the app had no skills panel and no authoring surface — skills could only be managed by editing files under
~/.openhuman/skills/manually. The install story in particular needed to work for "any normie" with a GitHub URL, not require a Node toolchain for first-time install.Solution
Core (Rust)
src/openhuman/skills/ops.rsgainedinstall_skill_from_url, which fetches a single SKILL.md over HTTPS using the existingreqwestclient, parses frontmatter withserde_yaml, derives a slug, and writes into the resolved skills directory. Helpers:normalize_install_url(GitHub blob→raw rewriting),derive_install_slug(filesystem-safe slug from frontmattername),parse_skill_md_str(frontmatter + body validation). Size capMAX_SKILL_MD_BYTES = 1 MiB; timeout clamp[1, 600]s; 12 unit tests.src/openhuman/skills/schemas.rsregisters the new RPCs in the controller registry — no domain branches incore/jsonrpc.rsorcore/cli.rs.skills.read_resourceandskills.createfollow the same pattern for browsing bundled resources and scaffolding.App (React/Tauri)
SkillsGrid→ click →SkillDetailDrawer(right-side panel) withSkillResourceTree(grouped by top-level directory) andSkillResourcePreview(size-gated text/image preview).CreateSkillModalscaffolds SKILL.md viaskills.create.InstallSkillDialogcallsskills.install_from_url. Errors from the core are funneled throughcategorizeInstallError(raw)which maps Rust error prefixes (invalid url:,unsupported url form:,fetch too large:,fetch timed out,invalid SKILL.md:,skill already installed,write failed:, …) to friendly titles + hints; unknown errors show a generic title with the raw message tucked under<details>.app/src/api/skillsApi.ts.Design tradeoffs
npx skills add. The CLI writes to./claude-code/skills//./cursor/skills/which do not match openhuman's discovery paths, and requiring Node on first install blocks non-technical users. Direct fetch covers the common "paste a GitHub URL" path with zero prerequisites.npm install+ fallback tonpx skills add) is intentionally deferred to a follow-up; this PR ships the SKILL.md path that issue Integrate skills.md #681 actually exercises.Submission Checklist
cargo testcases inskills/ops.rs(URL normalization, slug derivation, frontmatter parsing, path traversal). 8 Vitest cases inInstallSkillDialog.test.tsxcover the direct-fetch fixtures and error categorization branches. Tests forSkillDetailDrawer,SkillResourceTree,SkillResourcePreview,CreateSkillModalalso included.///on new public functions inops.rs(install_skill_from_url, helpers) and new types inschemas.rs. Component files carry brief JSDoc headers.Impact
https://URLs, single-file SKILL.md only, 1 MiB hard cap, timeout cap at 600s, path-traversal guard on the derived slug. No arbitrary code execution — SKILL.md is data, not executed on install.cargo check/ clippy pass (one pre-existingderivable_implswarning onSkillScopeunrelated to this PR).skills.*namespace; no existing RPC or UI route changed.Related
npm install+npx skills addfallback) for skills that need a Node runtime at install timeSummary by CodeRabbit
New Features
Tests