Split skills registry and runtime modules#3481
Merged
senamakel merged 34 commits intoJun 8, 2026
Merged
Conversation
Add two new RegistryKind variants (GithubSkillsDir, ClawHubApi) and implement fetch logic for each: - HermesHub: scans GitHub API contents/ endpoint for skill directories, constructs raw.githubusercontent.com download URLs for each SKILL.md - ClawHub: paginates the clawhub.ai REST API (up to 5 pages), maps slug/displayName/summary/stats to CatalogEntry with clawhub:// scheme Replace the non-existent awesome-openclaw and hermes-community default sources with real endpoints. ClawHub install is gracefully rejected with a message directing users to the OpenClaw CLI.
Two new built-in agents living in skill_registry/agent/: - skill_setup: discovery and installation specialist — browses registries, searches by keyword, installs/uninstalls skills - skill_executor: execution specialist — loads SKILL.md instructions, reads bundled resources, runs scripts via shell Both follow the agent_memory pattern (external module, registered in the central BUILTINS loader).
Rust E2E (`tests/skill_registry_e2e.rs`): exercises sources, browse, search, install (happy path + duplicate rejection + ClawHub rejection) against a real core router with isolated temp HOME. Registered in `scripts/test-rust-e2e.sh` ALL_E2E_SUITES. Playwright (`skills-registry.spec.ts`): adds RPC smoke tests for sources, browse, and search endpoints via callCoreRpc.
Add both skill agents to the orchestrator's subagents allowlist and add routing guidance in the decision tree so the orchestrator delegates skill install/browse/run requests to the correct specialist.
…tion New tools in skill_registry/tools.rs: - skill_registry_browse: browse all enabled registries - skill_registry_search: search catalog by keyword - skill_registry_install: install from catalog entry - skill_registry_sources: list configured sources Orchestrator changes: - Add all 4 tools to named tools list - Set omit_skills_catalog = false so installed skills render in prompt - Add render_installed_skills() to prompt.rs for session-start visibility
…Human label - Add SkillDetailDialog popup when clicking any skill tile (catalog or installed) — shows description, version, author, stars, tags, and source URL with install button for uninstalled skills - Fix duplicate badge chips: remove format badge from catalog tiles (source badge already identifies the registry), add friendly names to SourceBadge (Community, HermesHub, ClawHub) - Replace "OpenHuman" fallback format label with actual format name or neutral "Skill" — we don't own third-party skills - Change default format from "openhuman" to "agentskills" in index parser since skills follow the agentskills.io spec - Add i18n keys for detail dialog across all 14 locales
…ggles Backend: - Add HermesJsonCatalog registry kind for the aggregated Hermes catalog at hermes-agent.nousresearch.com/docs/api/skills.json (90k entries with full metadata: descriptions, tags, author, version) - Cap at 1000 entries per fetch to keep catalog manageable - Change default format from "openhuman" to "agentskills" Frontend: - Add source toggle buttons (Community, HermesHub, ClawHub) that filter the catalog by registry source — users can enable/disable each - Load sources on mount and default all enabled sources to active
Remove multi-source registry architecture (RegistrySource, RegistryKind, per-backend fetch functions). The Hermes JSON catalog at hermes-agent.nousresearch.com/docs/api/skills.json already aggregates all sources (built-in, ClawHub, skills.sh, LobeHub, browse.sh). - ops.rs: single fetch_catalog() from Hermes API with local caching - store.rs: remove custom sources persistence - schemas: remove add_source/remove_source RPCs, add categories RPC - tools.rs: simplify to match new CatalogEntry (no source_id param) - Frontend: sources are now string[], source toggles use entry.source - E2E tests: updated to match simplified API shape
…client-side filtering The 90k-entry Hermes catalog was being filtered client-side, causing search to return nothing for queries like "docker". Now search goes through the skill_registry_search RPC with 300ms debounce, results are capped at 60 entries, and Playwright + unit tests cover the flow.
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:
📝 WalkthroughWalkthroughMoves the skill registry to an aggregated Hermes catalog with caching, adds a skill_runtime execution/resolution surface, rewires RPCs/tools/agents, updates Skills Explorer to an RPC-driven source-aware catalog with a detail dialog, and adds unit, Playwright, and Rust E2E tests. ChangesSkill Registry Refactor + Skill Runtime Module + Skills Explorer UI
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
|
coderabbitai
Bot
added
feature
![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 (5)
tests/skill_registry_e2e.rs (1)
481-483: 💤 Low valueConsider either asserting on
stderror removing the binding.The
_install_stderrbinding is captured but not validated. Ifstderrpresence is sufficient, the_prefix is appropriate. However, if the intent is to later verifystderrcontent (e.g., confirm no unexpected warnings), consider adding an assertion or a comment clarifying the intent.💡 Option A: Remove if not needed
- let _install_stderr = install_result - .get("stderr") - .expect("install result must contain `stderr`"); + // stderr is present but not validated; install success is confirmed by stdout and new_skills. + assert!(install_result.get("stderr").is_some(), "install result must contain `stderr`");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/skill_registry_e2e.rs` around lines 481 - 483, The binding `_install_stderr` captures `install_result.get("stderr")` but isn't used; either remove the binding if merely checking presence isn't needed, or replace it with an assertion that validates stderr content (for example assert it is Some(...) or matches expected/no warnings) to make the test intent explicit—update the code referencing `_install_stderr` (the `install_result.get("stderr")` call) accordingly and add a short comment if you intend to keep the presence-only check.src/openhuman/skill_registry/tools.rs (1)
236-288: 💤 Low valueConsider adding explicit
is_concurrency_safefor consistency.
SkillRegistryUninstallTool(andSkillRegistryInstallTool) don't overrideis_concurrency_safe(), while the read-only tools explicitly returntrue. For write operations, the default (likelyfalse) is correct, but adding explicit implementations would improve clarity and consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/skill_registry/tools.rs` around lines 236 - 288, The Tool impl for SkillRegistryUninstallTool should explicitly implement is_concurrency_safe() to return false (and do the same for SkillRegistryInstallTool) to make write-operation concurrency semantics explicit; update the impl Tool for SkillRegistryUninstallTool to add a fn is_concurrency_safe(&self) -> bool { false } (and mirror this change in the SkillRegistryInstallTool impl) so the concurrency intent is clear and consistent with the read-only tools that return true.app/src/components/skills/SkillsExplorerTab.tsx (2)
328-331: ⚡ Quick winConsider adding Escape key to close the dialog.
Modal dialogs should close on Escape for keyboard accessibility. Add a
useEffectwith a keydown listener or handle it on the backdrop div.⌨️ Example implementation
useEffect(() => { const handleEscape = (e: KeyboardEvent) => { if (e.key === 'Escape') onClose(); }; document.addEventListener('keydown', handleEscape); return () => document.removeEventListener('keydown', handleEscape); }, [onClose]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/skills/SkillsExplorerTab.tsx` around lines 328 - 331, Add keyboard accessibility to SkillsExplorerTab by registering a keydown listener that calls the existing onClose when the Escape key is pressed: inside the SkillsExplorerTab component (ensure useEffect is imported), add a useEffect that attaches a document.addEventListener('keydown', handler) where handler checks e.key === 'Escape' and calls onClose(), and clean up the listener in the effect's return; include onClose in the dependency array so the handler always uses the latest callback.
893-899: 💤 Low valueFallback English string defeats i18n.
The
|| \Showing...`` fallback will show English even for non-English locales if the key returns empty. If the translation key is properly defined, this fallback is unnecessary. If it's missing, you'd want to catch it during development.Consider removing the fallback and ensuring the key exists in all locale files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/skills/SkillsExplorerTab.tsx` around lines 893 - 899, Remove the English fallback that defeats i18n: instead of using the expression that appends `|| \`Showing ...\``, return only the localized string from t('skills.explorer.showingOf') (use the existing variables catalogTotal and displayedCatalog for replacements). Update the call in SkillsExplorerTab.tsx so the component renders solely the translation (e.g., pass shown/total as replacement params to t) and delete the fallback branch; ensure the translation key exists in locales or add it during localization sync.app/src/components/skills/__tests__/WorkflowRunnerBody.test.tsx (1)
166-167: ⚡ Quick winAdd explicit behavior tests for the new runtime-preflight path.
The new mocks keep tests green, but there’s still no assertion that
resolveRuntimesis called with the inferred requirement, or that unavailable runtimes blockrunWorkflowand surface an error state. Adding those two cases will lock down the new branch.As per coding guidelines, app tests should prefer behavior-oriented coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/skills/__tests__/WorkflowRunnerBody.test.tsx` around lines 166 - 167, Add two behavior tests in WorkflowRunnerBody.test.tsx: (1) assert that hoisted.resolveRuntimes is called with the inferred requirement when the workflow is run (e.g., trigger the UI action that calls runWorkflow and verify hoisted.resolveRuntimes was invoked with the expected requirement object), and (2) simulate resolveRuntimes returning no matching runtimes (e.g., mockResolvedValue({ runtimes: [] })) and assert that runWorkflow is not called and the component surfaces an error state/message. Use the existing hoisted.resolveRuntimes and runWorkflow identifiers to locate and update tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/skills/SkillsExplorerTab.tsx`:
- Line 121: Update the onKeyDown handler in the SkillsExplorerTab component so
that when the Space key is detected (e.key === ' ' or 'Space'), you call
e.preventDefault() before invoking onClick(); keep the existing Enter behavior
intact. Locate the onKeyDown handler that currently checks e.key and the onClick
function reference and add the preventDefault call for the Space branch to stop
the page from scrolling.
- Line 218: The onKeyDown handler in SkillsExplorerTab (the inline handler that
checks e.key === 'Enter' || e.key === ' ') should call e.preventDefault() when
the Space key is used to activate the tile, matching SkillTile behavior; update
the handler so that when detecting the Space key it first calls
e.preventDefault() then invokes onClick(), ensuring default page scroll is
prevented on Space activation.
- Around line 397-399: Replace the hardcoded "License" label in
SkillsExplorerTab (the span rendering "License") to use the translator hook
useT() and the key t('skills.detail.license') (i.e., import/use useT() if not
already used in this component and call t('skills.detail.license') for the span
content); then add the new translation key "skills.detail.license" to en.ts and
mirror it in the other locale files with appropriate translations so all locales
cover this UI string.
In `@app/src/components/skills/WorkflowRunnerBody.tsx`:
- Around line 581-592: handleRun performs a runtime preflight using
inferRuntimeRequirement and workflowsApi.resolveRuntimes but handleRunJobNow
calls runWorkflow directly, allowing "Run now" to bypass the check; update
handleRunJobNow to perform the same preflight: call
inferRuntimeRequirement(selectedWorkflow), call workflowsApi.resolveRuntimes
with that requirement when present, throw the same error if any
resolved.runtimes are unavailable (mirroring the unavailable.map(...) join
logic), and only call runWorkflow after the preflight passes so both entry
points share the same runtime validation.
- Around line 586-590: The error message built from unavailable.map(...) in
WorkflowRunnerBody.tsx is hardcoded English; replace it with a localized string
via the component's useT() hook: add a translation key (e.g.
"runner.preflight.unavailable") that accepts placeholders for runtime name and
error, call const t = useT() (or reuse the existing t) in the component, and
build the thrown Error by mapping unavailable to
t('runner.preflight.unavailable', { runtime: runtime.runtime, error:
runtime.error ?? t('runner.preflight.unavailable_default') }) and joining those
localized pieces; ensure you add the new keys to the locale files so the UI uses
translations instead of raw English.
In `@src/openhuman/skill_runtime/tools.rs`:
- Around line 52-63: The execute method currently uses the stored Arc<Config>
(self.config) which can become stale; update execute to reload the config at the
start of the method (using the same pattern as the RPC handler, e.g., call
Config::load_or_init() or config_rpc::load_config_with_timeout()) and then pass
that fresh config into resolve_runtimes instead of self.config so runtime
paths/flags reflect current settings; keep all existing result handling
(serde_json::to_string, ToolResult::success) unchanged.
---
Nitpick comments:
In `@app/src/components/skills/__tests__/WorkflowRunnerBody.test.tsx`:
- Around line 166-167: Add two behavior tests in WorkflowRunnerBody.test.tsx:
(1) assert that hoisted.resolveRuntimes is called with the inferred requirement
when the workflow is run (e.g., trigger the UI action that calls runWorkflow and
verify hoisted.resolveRuntimes was invoked with the expected requirement
object), and (2) simulate resolveRuntimes returning no matching runtimes (e.g.,
mockResolvedValue({ runtimes: [] })) and assert that runWorkflow is not called
and the component surfaces an error state/message. Use the existing
hoisted.resolveRuntimes and runWorkflow identifiers to locate and update tests.
In `@app/src/components/skills/SkillsExplorerTab.tsx`:
- Around line 328-331: Add keyboard accessibility to SkillsExplorerTab by
registering a keydown listener that calls the existing onClose when the Escape
key is pressed: inside the SkillsExplorerTab component (ensure useEffect is
imported), add a useEffect that attaches a document.addEventListener('keydown',
handler) where handler checks e.key === 'Escape' and calls onClose(), and clean
up the listener in the effect's return; include onClose in the dependency array
so the handler always uses the latest callback.
- Around line 893-899: Remove the English fallback that defeats i18n: instead of
using the expression that appends `|| \`Showing ...\``, return only the
localized string from t('skills.explorer.showingOf') (use the existing variables
catalogTotal and displayedCatalog for replacements). Update the call in
SkillsExplorerTab.tsx so the component renders solely the translation (e.g.,
pass shown/total as replacement params to t) and delete the fallback branch;
ensure the translation key exists in locales or add it during localization sync.
In `@src/openhuman/skill_registry/tools.rs`:
- Around line 236-288: The Tool impl for SkillRegistryUninstallTool should
explicitly implement is_concurrency_safe() to return false (and do the same for
SkillRegistryInstallTool) to make write-operation concurrency semantics
explicit; update the impl Tool for SkillRegistryUninstallTool to add a fn
is_concurrency_safe(&self) -> bool { false } (and mirror this change in the
SkillRegistryInstallTool impl) so the concurrency intent is clear and consistent
with the read-only tools that return true.
In `@tests/skill_registry_e2e.rs`:
- Around line 481-483: The binding `_install_stderr` captures
`install_result.get("stderr")` but isn't used; either remove the binding if
merely checking presence isn't needed, or replace it with an assertion that
validates stderr content (for example assert it is Some(...) or matches
expected/no warnings) to make the test intent explicit—update the code
referencing `_install_stderr` (the `install_result.get("stderr")` call)
accordingly and add a short comment if you intend to keep the presence-only
check.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab02581f-4f63-4c34-9e06-19403da54cee
📒 Files selected for processing (70)
app/src/components/skills/SkillsExplorerTab.tsxapp/src/components/skills/UninstallSkillConfirmDialog.tsxapp/src/components/skills/WorkflowRunnerBody.tsxapp/src/components/skills/__tests__/SkillsExplorerTab.test.tsxapp/src/components/skills/__tests__/WorkflowRunnerBody.test.tsxapp/src/components/skills/preflightGate.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/services/api/__tests__/workflowsApi.test.tsapp/src/services/api/skillRegistryApi.test.tsapp/src/services/api/skillRegistryApi.tsapp/src/services/api/workflowsApi.test.tsapp/src/services/api/workflowsApi.tsapp/test/playwright/specs/skills-registry.spec.tsscripts/test-rust-e2e.shsrc/core/all.rssrc/core/jsonrpc.rssrc/openhuman/agent/tools/run_workflow.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/agent_registry/agents/orchestrator/agent.tomlsrc/openhuman/agent_registry/agents/orchestrator/prompt.mdsrc/openhuman/agent_registry/agents/orchestrator/prompt.rssrc/openhuman/mod.rssrc/openhuman/skill_registry/README.mdsrc/openhuman/skill_registry/agent/mod.rssrc/openhuman/skill_registry/agent/skill_setup/agent.tomlsrc/openhuman/skill_registry/agent/skill_setup/mod.rssrc/openhuman/skill_registry/agent/skill_setup/prompt.mdsrc/openhuman/skill_registry/agent/skill_setup/prompt.rssrc/openhuman/skill_registry/mod.rssrc/openhuman/skill_registry/ops.rssrc/openhuman/skill_registry/schemas/controller_schemas.rssrc/openhuman/skill_registry/schemas/handlers.rssrc/openhuman/skill_registry/schemas/wire_types.rssrc/openhuman/skill_registry/store.rssrc/openhuman/skill_registry/tools.rssrc/openhuman/skill_registry/types.rssrc/openhuman/skill_runtime/README.mdsrc/openhuman/skill_runtime/agent/mod.rssrc/openhuman/skill_runtime/agent/skill_executor/agent.tomlsrc/openhuman/skill_runtime/agent/skill_executor/mod.rssrc/openhuman/skill_runtime/agent/skill_executor/prompt.mdsrc/openhuman/skill_runtime/agent/skill_executor/prompt.rssrc/openhuman/skill_runtime/mod.rssrc/openhuman/skill_runtime/ops.rssrc/openhuman/skill_runtime/run_machinery.rssrc/openhuman/skill_runtime/schemas.rssrc/openhuman/skill_runtime/tools.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/ops.rssrc/openhuman/workflows/e2e_plumbing_tests.rssrc/openhuman/workflows/e2e_run_tests.rssrc/openhuman/workflows/ops_install.rssrc/openhuman/workflows/preflight.rssrc/openhuman/workflows/registry.rssrc/openhuman/workflows/schemas/handlers.rssrc/openhuman/workflows/schemas/mod.rstests/skill_registry_e2e.rs
💤 Files with no reviewable changes (1)
- src/openhuman/workflows/schemas/mod.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/openhuman/skill_runtime/tools.rs (2)
13-19: 💤 Low valueConsider removing the unused
_configparameter if not required by the registration site.The struct is a unit struct with no fields (line 13), and the constructor's
_configparameter is never used (indicated by the_prefix). If the registration/wiring code doesn't require this signature, consider simplifying:-pub fn new(_config: Arc<Config>) -> Self { +pub fn new() -> Self { Self }If the parameter is required by a registration pattern elsewhere, consider adding a doc comment explaining why it's accepted but unused.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/skill_runtime/tools.rs` around lines 13 - 19, The constructor for SkillRuntimeResolveRuntimesTool currently accepts an unused parameter _config: Arc<Config>; either remove that parameter from the new method signature (change fn new() -> Self) and callers that register/construct SkillRuntimeResolveRuntimesTool accordingly, or if the registration API requires the signature, keep the parameter but add a doc comment to the SkillRuntimeResolveRuntimesTool::new function explaining why Config is accepted and intentionally unused; update references to the new constructor name accordingly.
33-44: 💤 Low valueConsider documenting runtime aliases in the schema description.
The schema enum lists
["all", "node", "python"]butRuntimeRequirement::from_optional(context snippet 1) also accepts aliases like"nodejs","javascript", and"python3". LLM agents relying on the schema won't discover these alternatives. Consider adding them to the enum or documenting in the description:"runtime": { "type": "string", "enum": ["all", "node", "python"], - "description": "Runtime to resolve. Defaults to all." + "description": "Runtime to resolve. Defaults to all. Accepts aliases: nodejs, javascript (node); python3 (python)." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/skill_runtime/tools.rs` around lines 33 - 44, The JSON schema in parameters_schema() currently enumerates ["all","node","python"] but RuntimeRequirement::from_optional accepts aliases like "nodejs", "javascript", and "python3"; update parameters_schema() to either expand the "enum" to include those aliases or explicitly list them in the "description" (e.g., mention accepted aliases "nodejs", "javascript", "python3"), so LLM agents see all valid runtime names—refer to parameters_schema() and RuntimeRequirement::from_optional when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/lib/i18n/fr.ts`:
- Line 285: Replace the mixed-language toast message for the key
'skills.explorer.uninstallSuccess' with a fully French, consistent label: change
its value from "Skill désinstallée avec succès." to "Compétence désinstallée
avec succès." so it matches the surrounding "Compétence" terminology used
elsewhere (update the string value for 'skills.explorer.uninstallSuccess').
---
Nitpick comments:
In `@src/openhuman/skill_runtime/tools.rs`:
- Around line 13-19: The constructor for SkillRuntimeResolveRuntimesTool
currently accepts an unused parameter _config: Arc<Config>; either remove that
parameter from the new method signature (change fn new() -> Self) and callers
that register/construct SkillRuntimeResolveRuntimesTool accordingly, or if the
registration API requires the signature, keep the parameter but add a doc
comment to the SkillRuntimeResolveRuntimesTool::new function explaining why
Config is accepted and intentionally unused; update references to the new
constructor name accordingly.
- Around line 33-44: The JSON schema in parameters_schema() currently enumerates
["all","node","python"] but RuntimeRequirement::from_optional accepts aliases
like "nodejs", "javascript", and "python3"; update parameters_schema() to either
expand the "enum" to include those aliases or explicitly list them in the
"description" (e.g., mention accepted aliases "nodejs", "javascript",
"python3"), so LLM agents see all valid runtime names—refer to
parameters_schema() and RuntimeRequirement::from_optional when making the
change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f10b3e8-6317-472b-80f0-29a43b39511f
📒 Files selected for processing (18)
app/src/components/skills/SkillsExplorerTab.tsxapp/src/components/skills/WorkflowRunnerBody.tsxapp/src/components/skills/__tests__/WorkflowRunnerBody.test.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tssrc/openhuman/skill_runtime/tools.rs
✅ Files skipped from review due to trivial changes (4)
- app/src/lib/i18n/ko.ts
- app/src/lib/i18n/pl.ts
- app/src/lib/i18n/it.ts
- app/src/lib/i18n/ru.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/lib/i18n/en.ts
- app/src/components/skills/tests/WorkflowRunnerBody.test.tsx
- app/src/components/skills/SkillsExplorerTab.tsx
Replace mixed English/French "Skill désinstallée" with fully French "Compétence désinstallée" to match surrounding locale wording.
The job-level env sets CARGO_BUILD_JOBS=1 to serialize linker work and avoid Bus errors on hosted runners, but the step-level env overrides it back to 2. Remove the override so the linker runs single-threaded as intended. Closes tinyhumansai#3484
CARGO_BUILD_JOBS=1 alone is insufficient — the single coverage-instrumented binary still exceeds the 7GB RAM on ubuntu-22.04 runners. Adding swap prevents OOM kills during the link phase. Ref: tinyhumansai#3484
The PR CI workflow (pr-ci.yml) is the one that actually runs on PRs, not coverage.yml (workflow_dispatch only). Add 8GB swap and reduce CARGO_BUILD_JOBS to 1 to prevent rust-lld OOM during coverage linking. Ref: tinyhumansai#3484
CI containers don't have sudo installed — the process already runs as root. Add `options: --privileged` so the swapon syscall is permitted, and remove sudo prefix from all swap commands. Ref: tinyhumansai#3484
The CI container lacks sudo, so swap setup fails inside it. Switch the Rust Core Coverage job to run on the bare ubuntu-22.04 host (installing system deps via apt) so the pierotofy/set-swap-space action works. With 10GB swap (~17GB total), CARGO_BUILD_JOBS=2 is safe again. Ref: tinyhumansai#3484
Running on a bare host (for swap support) instead of the CI container means a cold Rust cache and apt-get overhead. The previous 30-min timeout cancelled the job at ~34 min. Bumping to 45 min gives enough headroom for cold-cache runs. Ref: tinyhumansai#3484
The bare-host approach worked (no OOM) but was too slow without the container's pre-built toolchain and deps (34 min, hit 30-min timeout). Restore the CI container image for its build optimizations. Use `--privileged` to allow `swapon` inside the container (runs as root, no sudo needed). With 10GB swap + container perf, CARGO_BUILD_JOBS=2 should complete well within 30 min. Ref: tinyhumansai#3484
The --privileged container shares the host's /swapfile mount. mkswap refuses to format a mounted swapfile. Add swapoff -a before fallocate to cleanly resize the existing host swap from ~4GB to 10GB. Ref: tinyhumansai#3484
The host's /swapfile is already mounted and can't be reformatted from inside the container even with --privileged. Create additional swap at /tmp/extra-swap instead, stacking on top of the existing ~4GB host swap for ~14GB total virtual memory. Ref: tinyhumansai#3484
swapon rejects swap files on overlay/tmpfs filesystems (Invalid argument). The /__w directory is bind-mounted from the host's real ext4 filesystem, so swapon works there. Ref: tinyhumansai#3484
…rror The 10GB swap file on /__w consumed too much disk, leaving insufficient space for coverage-instrumented build artifacts. rust-lld crashed with SIGBUS (bus error) when the disk filled up. Reducing swap to 4GB frees ~6GB for the build, and CARGO_BUILD_JOBS=1 ensures only one linker runs at a time to keep peak memory within the 7GB RAM + 4GB swap budget.
The bus error (SIGBUS) during coverage linking was caused by disk exhaustion, not memory exhaustion — the swap file consumed disk that the linker needed for its enormous coverage-instrumented binaries. Remove swap entirely and instead free ~15GB by removing pre-installed Android SDK, .NET, and hostedtoolcache from the runner. Keep CARGO_BUILD_JOBS=1 to limit peak memory within the 7GB RAM budget.
The previous rm -rf /opt/hostedtoolcache ran inside the container where that path doesn't exist. The host's toolcache is bind-mounted at /__t inside the container — deleting /__t/* actually frees ~8GB on the shared partition, preventing the SIGBUS during coverage-instrumented linking.
The catalog_mirrors_builtins test enforces that every built-in agent has a matching entry in RESOURCE_CATALOG. The new skill_setup and skill_executor agents were missing.
Cover previously-untested branches in four files targeted by the diff-cover gate on PR tinyhumansai#3481: skillRegistryApi.ts (was 63%): - search() with query-only, with source+category, and envelope shape - sources() / categories() happy path and envelope unwrapping - install() with missing new_skills → [] fallback - browse() with forceRefresh=true and default false workflowsApi.ts (was 93.8%): - resolveRuntimes('all') and resolveRuntimes() (default) using empty params object, covering the ternary on line 538 SkillsExplorerTab.tsx (was 42.8%): - SkillFormatBadge fallback label for unknown formats - SkillTile / CatalogTile keyboard activation (Enter key) - SkillDetailDialog open/close with all metadata fields - Detail dialog footer install button triggering handleRegistryInstall - Registry install success toast and error toast paths - Source toggle button rendering and single-source filter triggering search - Catalog count and installed count badges - Legacy scope badge display - Empty registry state and empty installed state - Retry button for both catalog and installed error states - Refresh button sending forceRefresh=true - Hermes-first sort order in installed view WorkflowRunnerBody.tsx (was 69.1%): - skillsError display when listWorkflows rejects - Recent runs DONE / DEGENERATE / FAILED status badge colors - Run row expand → tailing indicator + log content render - readRunLog error path surfaced in viewer - Expand/collapse toggle icons (▾ / ▸) - All-runs vs skill-scoped recent runs heading - renderField for boolean (checkbox), integer (number), and string - handleRemoveJob calling openhumanCronRemove + list refresh - loadJobHistory error path (graceful, no crash) - History run row with null output → historyNoOutput placeholder - handleSaveSchedule with missing required fields → scheduleError - buildCronJobName with non-empty inputs (via save schedule flow) - ensureRuntimeAvailability failure when python runtime unavailable - Preflight gate pill in run error display
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
1 participant
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
skill_runtimeand remote discovery/install intoskill_registry.skill_runtime; kept discover/install/uninstall tooling underskill_registry.Problem
Solution
src/openhuman/skill_registryas the catalog/cache/install/uninstall owner, backed by the Hermesskills.jsonAPI and local cache.src/openhuman/skill_runtimeas the run/cancel/log/recent-runs/runtime-resolution owner.skill_runtimeand leave the discovery/install agent inskill_registry.openhuman.skill_registry_*andopenhuman.skill_runtime_*directly.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/pr-ci.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge. Local focused coverage was added; full diff coverage is left to CI for the full merged lcov gate.## Related: N/A: no new matrix feature ID.skill_registryandskill_runtimeREADMEs instead.Closes #NNNin the## Relatedsection: N/A: no issue was provided for this branch.Impact
OPENHUMAN_SKILL_REGISTRY_REFRESH_ON_BOOT=0disables it.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/skill-registry-backendsd8c7c6b7c5d7531327553685a6b2d2262eddd817Validation Run
pnpm --filter openhuman-app format:checkvia pre-push hookpnpm typecheckpnpm debug unit src/services/api/skillsApi.test.ts src/services/api/skillRegistryApi.test.ts src/components/skills/__tests__/WorkflowRunnerBody.test.tsx src/components/skills/__tests__/SkillsExplorerTab.test.tsx;cargo test --manifest-path Cargo.toml --test skill_registry_e2e -- --nocapture;cargo test --manifest-path Cargo.toml skill_registry --lib;cargo test --manifest-path Cargo.toml skill_runtime --libcargo fmt --manifest-path Cargo.toml;cargo check --manifest-path app/src-tauri/Cargo.tomlvia pre-push hookOPENHUMAN_DEV_PORT=1423 pnpm dev:applaunched successfully; embedded core reachedhttp://127.0.0.1:7788/rpcand scheduled the registry boot refresh.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
workflows; existing run machinery was moved underskill_runtimeand re-exported through new controllers.OPENHUMAN_SKILL_INSTALL_ALLOW_LOCAL_HTTP=1; registry boot refresh is best-effort and does not block core load.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Internationalization
Tests & Docs