mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(skills): surface registry-installed skills in Installed tab + raise catalog browse timeout (#3954) (#3987)
This commit is contained in:
@@ -559,7 +559,9 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
|
||||
setSkillsLoading(true);
|
||||
setSkillsError(null);
|
||||
try {
|
||||
const result = await workflowsApi.listWorkflows();
|
||||
// Include `skills/`-root installs (registry installs land there) so they
|
||||
// appear in the Installed tab and flip the catalog Install button.
|
||||
const result = await workflowsApi.listWorkflows({ includeSkills: true });
|
||||
log('fetchSkills: count=%d', result.length);
|
||||
setSkills(result);
|
||||
} catch (err) {
|
||||
@@ -697,7 +699,11 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) {
|
||||
setInstallingId(entry.id);
|
||||
try {
|
||||
const result = await skillRegistryApi.install(entry.id);
|
||||
void fetchSkills();
|
||||
// Await the refetch so `installedKeys` is fresh before the button
|
||||
// re-renders — otherwise it briefly flips back to "Install" between
|
||||
// clearing the installing state and the list updating. `fetchSkills`
|
||||
// swallows its own errors, so this never throws into the catch below.
|
||||
await fetchSkills();
|
||||
onToast?.({
|
||||
type: 'success',
|
||||
title: t('skills.install.installComplete'),
|
||||
|
||||
@@ -460,7 +460,8 @@ export const WorkflowRunnerBody = ({ headerText, className }: SkillsRunnerBodyPr
|
||||
setSkillsLoading(true);
|
||||
setSkillsError(null);
|
||||
workflowsApi
|
||||
.listWorkflows()
|
||||
// Include `skills/`-root installs so registry-installed skills are runnable here.
|
||||
.listWorkflows({ includeSkills: true })
|
||||
.then(list => {
|
||||
if (cancelled) return;
|
||||
// Hide the codegraph-smoke skill — internal smoke-test only.
|
||||
@@ -1583,7 +1584,7 @@ export const WorkflowRunnerBody = ({ headerText, className }: SkillsRunnerBodyPr
|
||||
onCreated={() => {
|
||||
setEditOpen(false);
|
||||
void workflowsApi
|
||||
.listWorkflows()
|
||||
.listWorkflows({ includeSkills: true })
|
||||
.then(list => setSkills(list.filter(s => s.id !== 'codegraph-smoke')))
|
||||
.catch(() => {});
|
||||
void workflowsApi
|
||||
|
||||
@@ -281,6 +281,25 @@ describe('workflowsApi.listWorkflows', () => {
|
||||
expect(result[0].platforms).toEqual([]);
|
||||
expect(result[1].sourceFormat).toBe('legacy');
|
||||
});
|
||||
|
||||
it('omits params by default (automations-only view)', async () => {
|
||||
const { callCoreRpc } = await import('../../coreRpcClient');
|
||||
vi.mocked(callCoreRpc).mockResolvedValueOnce({ workflows: [] });
|
||||
await workflowsApi.listWorkflows();
|
||||
const call = vi.mocked(callCoreRpc).mock.calls[0][0];
|
||||
expect(call.method).toBe('openhuman.workflows_list');
|
||||
expect(call.params).toBeUndefined();
|
||||
});
|
||||
|
||||
it('passes include_skills when includeSkills is set', async () => {
|
||||
const { callCoreRpc } = await import('../../coreRpcClient');
|
||||
vi.mocked(callCoreRpc).mockResolvedValueOnce({ workflows: [] });
|
||||
await workflowsApi.listWorkflows({ includeSkills: true });
|
||||
expect(callCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.workflows_list',
|
||||
params: { include_skills: true },
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('workflowsApi.readWorkflowResource', () => {
|
||||
|
||||
@@ -62,6 +62,7 @@ describe('skillRegistryApi', () => {
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.skill_registry_search',
|
||||
params: { query: 'demo' },
|
||||
timeoutMs: 120_000,
|
||||
});
|
||||
expect(result[0].id).toBe('demo');
|
||||
});
|
||||
@@ -74,6 +75,7 @@ describe('skillRegistryApi', () => {
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.skill_registry_search',
|
||||
params: { query: 'q', source: 'ClawHub', category: 'devops' },
|
||||
timeoutMs: 120_000,
|
||||
});
|
||||
});
|
||||
|
||||
@@ -90,7 +92,10 @@ describe('skillRegistryApi', () => {
|
||||
|
||||
const result = await skillRegistryApi.sources();
|
||||
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_sources' });
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.skill_registry_sources',
|
||||
timeoutMs: 120_000,
|
||||
});
|
||||
expect(result).toEqual(['built-in', 'ClawHub']);
|
||||
});
|
||||
|
||||
@@ -107,7 +112,10 @@ describe('skillRegistryApi', () => {
|
||||
|
||||
const result = await skillRegistryApi.categories();
|
||||
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_categories' });
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.skill_registry_categories',
|
||||
timeoutMs: 120_000,
|
||||
});
|
||||
expect(result).toEqual(['productivity', 'devops']);
|
||||
});
|
||||
|
||||
@@ -140,6 +148,7 @@ describe('skillRegistryApi', () => {
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.skill_registry_browse',
|
||||
params: { force_refresh: true },
|
||||
timeoutMs: 120_000,
|
||||
});
|
||||
});
|
||||
|
||||
@@ -151,6 +160,7 @@ describe('skillRegistryApi', () => {
|
||||
expect(mockCallCoreRpc).toHaveBeenCalledWith({
|
||||
method: 'openhuman.skill_registry_browse',
|
||||
params: { force_refresh: false },
|
||||
timeoutMs: 120_000,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,6 +4,18 @@ import { callCoreRpc } from '../coreRpcClient';
|
||||
|
||||
const log = debug('skillRegistryApi');
|
||||
|
||||
/**
|
||||
* Catalog reads (`browse`/`search`/`sources`/`categories`) all funnel through
|
||||
* the backend's single-flight `browse_catalog`, whose COLD fetch downloads the
|
||||
* full ~90k-entry registry and can take ~80s. That comfortably exceeds the
|
||||
* default 30s `CORE_RPC_TIMEOUT_MS`, so a first load (or post-TTL revalidate)
|
||||
* would spuriously time out. Give these the longer per-call timeout the RPC
|
||||
* client supports for exactly such "slow-but-alive" calls; warm-cache reads
|
||||
* still return in milliseconds, so this only raises the ceiling for the rare
|
||||
* cold path. 120s = ~80s cold download + margin (well under the 10min clamp).
|
||||
*/
|
||||
const CATALOG_RPC_TIMEOUT_MS = 120_000;
|
||||
|
||||
export interface CatalogEntry {
|
||||
id: string;
|
||||
name: string;
|
||||
@@ -72,7 +84,11 @@ export const skillRegistryApi = {
|
||||
log('browse: forceRefresh=%s', forceRefresh);
|
||||
const response = await callCoreRpc<
|
||||
Envelope<{ entries: CatalogEntry[] }> | { entries: CatalogEntry[] }
|
||||
>({ method: 'openhuman.skill_registry_browse', params: { force_refresh: forceRefresh } });
|
||||
>({
|
||||
method: 'openhuman.skill_registry_browse',
|
||||
params: { force_refresh: forceRefresh },
|
||||
timeoutMs: CATALOG_RPC_TIMEOUT_MS,
|
||||
});
|
||||
const result = unwrap(response);
|
||||
log('browse: count=%d', result.entries.length);
|
||||
return result.entries;
|
||||
@@ -85,6 +101,7 @@ export const skillRegistryApi = {
|
||||
>({
|
||||
method: 'openhuman.skill_registry_search',
|
||||
params: { query, ...(source ? { source } : {}), ...(category ? { category } : {}) },
|
||||
timeoutMs: CATALOG_RPC_TIMEOUT_MS,
|
||||
});
|
||||
const result = unwrap(response);
|
||||
log('search: count=%d', result.entries.length);
|
||||
@@ -95,6 +112,7 @@ export const skillRegistryApi = {
|
||||
log('sources: request');
|
||||
const response = await callCoreRpc<Envelope<{ sources: string[] }> | { sources: string[] }>({
|
||||
method: 'openhuman.skill_registry_sources',
|
||||
timeoutMs: CATALOG_RPC_TIMEOUT_MS,
|
||||
});
|
||||
const result = unwrap(response);
|
||||
log('sources: count=%d', result.sources.length);
|
||||
@@ -105,7 +123,7 @@ export const skillRegistryApi = {
|
||||
log('categories: request');
|
||||
const response = await callCoreRpc<
|
||||
Envelope<{ categories: string[] }> | { categories: string[] }
|
||||
>({ method: 'openhuman.skill_registry_categories' });
|
||||
>({ method: 'openhuman.skill_registry_categories', timeoutMs: CATALOG_RPC_TIMEOUT_MS });
|
||||
const result = unwrap(response);
|
||||
log('categories: count=%d', result.categories.length);
|
||||
return result.categories;
|
||||
|
||||
@@ -243,12 +243,29 @@ function normalizeWorkflowSummary(raw: RawWorkflowSummary): WorkflowSummary {
|
||||
};
|
||||
}
|
||||
|
||||
/** Options for {@link workflowsApi.listWorkflows}. */
|
||||
export interface ListWorkflowsOptions {
|
||||
/**
|
||||
* When `true`, also include capability skills under the `skills/` roots
|
||||
* (registry installs land there), not just `workflows/`-root automations.
|
||||
*/
|
||||
includeSkills?: boolean;
|
||||
}
|
||||
|
||||
export const workflowsApi = {
|
||||
/** Enumerate SKILL.md / legacy skills visible in the active workspace. */
|
||||
listWorkflows: async (): Promise<WorkflowSummary[]> => {
|
||||
log('listWorkflows: request');
|
||||
/**
|
||||
* Enumerate SKILL.md / legacy skills visible in the active workspace.
|
||||
*
|
||||
* By default returns only `workflows/`-root automations (the Automations UI
|
||||
* view). Pass `{ includeSkills: true }` to also include capability skills
|
||||
* under the `skills/` roots — the Skills Explorer uses this so
|
||||
* registry-installed skills show up in its Installed tab.
|
||||
*/
|
||||
listWorkflows: async (opts?: ListWorkflowsOptions): Promise<WorkflowSummary[]> => {
|
||||
log('listWorkflows: request includeSkills=%s', opts?.includeSkills ?? false);
|
||||
const response = await callCoreRpc<Envelope<WorkflowsListResult> | WorkflowsListResult>({
|
||||
method: 'openhuman.workflows_list',
|
||||
params: opts?.includeSkills ? { include_skills: true } : undefined,
|
||||
});
|
||||
const result = unwrapEnvelope(response);
|
||||
const workflows = (result?.workflows ?? []).map(normalizeWorkflowSummary);
|
||||
|
||||
@@ -556,3 +556,60 @@ fn resolve_workflow_for_resource(
|
||||
(None, None) => Err(format!("skill '{skill_id}' not found")),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod include_skills_tests {
|
||||
use super::*;
|
||||
|
||||
/// Write a minimal `<file>`-named bundle under `root/slug/`.
|
||||
fn seed_bundle(root: &Path, slug: &str, file: &str) {
|
||||
let dir = root.join(slug);
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
std::fs::write(
|
||||
dir.join(file),
|
||||
format!("---\nname: {slug}\ndescription: {slug} desc\n---\n\n{slug} body\n"),
|
||||
)
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
/// `discover_automations` lists only `workflows/`-root automations, while
|
||||
/// `discover_workflows` additionally surfaces `skills/`-root installs. This
|
||||
/// is exactly the branch `handle_workflows_list` selects on `include_skills`
|
||||
/// so the Skills Explorer's Installed tab can show registry installs (#3954).
|
||||
#[test]
|
||||
fn automations_excludes_skill_roots_but_full_discover_includes_them() {
|
||||
let home = tempfile::TempDir::new().unwrap();
|
||||
let home_path = home.path();
|
||||
// A registry-style install lands under `~/.openhuman/skills/`.
|
||||
seed_bundle(
|
||||
&home_path.join(".openhuman").join("skills"),
|
||||
"installed-skill",
|
||||
"SKILL.md",
|
||||
);
|
||||
// A "New workflow" automation lands under `~/.openhuman/workflows/`.
|
||||
seed_bundle(
|
||||
&home_path.join(".openhuman").join("workflows"),
|
||||
"my-automation",
|
||||
"WORKFLOW.md",
|
||||
);
|
||||
|
||||
// Automations-only view (the default `workflows_list` path) hides the skill.
|
||||
let automations = discover_automations(Some(home_path), None, false);
|
||||
let auto_names: Vec<&str> = automations.iter().map(|w| w.name.as_str()).collect();
|
||||
assert_eq!(
|
||||
auto_names,
|
||||
vec!["my-automation"],
|
||||
"discover_automations must exclude `skills/`-root installs"
|
||||
);
|
||||
|
||||
// Full view (`include_skills=true`) surfaces both.
|
||||
let full = discover_workflows(Some(home_path), None, false);
|
||||
let mut full_names: Vec<&str> = full.iter().map(|w| w.name.as_str()).collect();
|
||||
full_names.sort_unstable();
|
||||
assert_eq!(
|
||||
full_names,
|
||||
vec!["installed-skill", "my-automation"],
|
||||
"discover_workflows must include `skills/`-root installs"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -189,7 +189,21 @@ pub fn load_workflows(workspace_dir: &Path) -> Vec<WorkflowDefinition> {
|
||||
let Some(dir) = skill_md.parent() else {
|
||||
continue;
|
||||
};
|
||||
if let Some(def) = load_workflow_definition(dir, &wf.name, &wf.description) {
|
||||
// Build the runnable id from the on-disk slug (`dir_name`) so it matches
|
||||
// the `WorkflowSummary.id` shown in lists, the id the orchestrator prompt
|
||||
// tells the agent to run, and the slug uninstall resolves against — all
|
||||
// of which key on `dir_name`. A SKILL.md-only install whose frontmatter
|
||||
// `name` differs from its install slug (e.g. `name: My Cool Workflow` in
|
||||
// `my-cool-workflow/`) would otherwise build `definition.id` from the
|
||||
// name and be unresolvable by `workflows_describe` / `workflows_run`
|
||||
// ("unknown skill"). Falls back to `name` for legacy `Workflow` values
|
||||
// that predate `dir_name`. (#3987 codex review.)
|
||||
let slug = if wf.dir_name.is_empty() {
|
||||
wf.name.as_str()
|
||||
} else {
|
||||
wf.dir_name.as_str()
|
||||
};
|
||||
if let Some(def) = load_workflow_definition(dir, slug, &wf.description) {
|
||||
workflows.push(def);
|
||||
}
|
||||
}
|
||||
@@ -341,6 +355,33 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skill_md_only_install_resolves_by_dir_slug_not_frontmatter_name() {
|
||||
// Regression (#3987 codex review): a SKILL.md-only install whose
|
||||
// frontmatter `name` differs from its install slug must resolve via the
|
||||
// dir slug — the id surfaced in the list summary / orchestrator prompt /
|
||||
// uninstall — not the frontmatter name. Before the fix, `definition.id`
|
||||
// was built from `wf.name` ("My Cool Workflow"), so `get_workflow`
|
||||
// (keyed on the slug) returned None → "unknown skill".
|
||||
let tmp = tempfile::TempDir::new().unwrap();
|
||||
let dir = tmp.path().join("skills").join("my-cool-workflow");
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
std::fs::write(
|
||||
dir.join("SKILL.md"),
|
||||
"---\nname: My Cool Workflow\ndescription: does cool things\n---\n\n# Body\n",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let resolved = get_workflow(tmp.path(), "my-cool-workflow")
|
||||
.expect("SKILL.md-only install must resolve by its dir slug");
|
||||
assert_eq!(resolved.definition.id, "my-cool-workflow");
|
||||
// And NOT by the frontmatter name.
|
||||
assert!(
|
||||
get_workflow(tmp.path(), "My Cool Workflow").is_none(),
|
||||
"frontmatter name must not be the runnable id"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prune_removes_legacy_bundled_only() {
|
||||
let tmp = tempfile::TempDir::new().unwrap();
|
||||
|
||||
@@ -86,7 +86,12 @@ pub fn workflows_schemas(function: &str) -> ControllerSchema {
|
||||
namespace: "workflows",
|
||||
function: "list",
|
||||
description: "List SKILL.md and legacy skills discovered in the user home and workspace.",
|
||||
inputs: vec![],
|
||||
inputs: vec![FieldSchema {
|
||||
name: "include_skills",
|
||||
ty: TypeSchema::Bool,
|
||||
comment: "When true, also include capability skills under the `skills/` roots (where registry installs land), not just `workflows/`-root automations. Defaults to false (automations-only view).",
|
||||
required: false,
|
||||
}],
|
||||
outputs: vec![FieldSchema {
|
||||
name: "skills",
|
||||
ty: TypeSchema::Array(Box::new(TypeSchema::Ref("WorkflowSummary"))),
|
||||
|
||||
@@ -12,8 +12,9 @@ use serde_json::{Map, Value};
|
||||
use crate::core::all::ControllerFuture;
|
||||
use crate::openhuman::skill_runtime::spawn_workflow_run_background;
|
||||
use crate::openhuman::workflows::ops::{
|
||||
create_workflow, discover_automations, install_workflow_from_url, is_workspace_trusted,
|
||||
read_workflow_resource, uninstall_workflow, CreateWorkflowParams, UninstallWorkflowParams,
|
||||
create_workflow, discover_automations, discover_workflows, install_workflow_from_url,
|
||||
is_workspace_trusted, read_workflow_resource, uninstall_workflow, CreateWorkflowParams,
|
||||
UninstallWorkflowParams,
|
||||
};
|
||||
use crate::openhuman::workflows::{registry, run_log};
|
||||
use crate::rpc::RpcOutcome;
|
||||
@@ -30,22 +31,31 @@ use super::wire_types::{
|
||||
|
||||
pub(super) fn handle_workflows_list(params: Map<String, Value>) -> ControllerFuture {
|
||||
Box::pin(async move {
|
||||
let _ = deserialize_params::<WorkflowsListParams>(params)?;
|
||||
tracing::debug!("[workflows][rpc] list automations");
|
||||
let params = deserialize_params::<WorkflowsListParams>(params)?;
|
||||
let include_skills = params.include_skills;
|
||||
tracing::debug!(include_skills, "[workflows][rpc] list automations");
|
||||
let workspace = resolve_workspace_dir().await;
|
||||
let trusted = is_workspace_trusted(&workspace);
|
||||
let home = dirs::home_dir();
|
||||
// Automations list shows only `workflows/`-root task templates — not the
|
||||
// capability skills under `skills/` roots, which the agent harness still
|
||||
// loads via `discover_workflows` / `load_workflow_metadata`.
|
||||
let automations = discover_automations(home.as_deref(), Some(workspace.as_path()), trusted);
|
||||
// Default: automations-only (`workflows/` roots) so capability skills
|
||||
// don't masquerade as task templates in the Automations UI. The Skills
|
||||
// Explorer passes `include_skills=true` to also surface `skills/`-root
|
||||
// installs (registry installs land there) in its Installed tab. Either
|
||||
// way the agent harness loads both via `discover_workflows` /
|
||||
// `load_workflow_metadata`.
|
||||
let listed = if include_skills {
|
||||
discover_workflows(home.as_deref(), Some(workspace.as_path()), trusted)
|
||||
} else {
|
||||
discover_automations(home.as_deref(), Some(workspace.as_path()), trusted)
|
||||
};
|
||||
tracing::debug!(
|
||||
count = automations.len(),
|
||||
count = listed.len(),
|
||||
include_skills,
|
||||
workspace = %workspace.display(),
|
||||
trusted,
|
||||
"[workflows][rpc] list result"
|
||||
);
|
||||
let summaries = automations.into_iter().map(WorkflowSummary::from).collect();
|
||||
let summaries = listed.into_iter().map(WorkflowSummary::from).collect();
|
||||
to_json(RpcOutcome::new(
|
||||
WorkflowsListResult {
|
||||
workflows: summaries,
|
||||
|
||||
@@ -15,8 +15,14 @@ use crate::openhuman::workflows::ops::{
|
||||
|
||||
#[derive(Debug, Deserialize, Default)]
|
||||
pub(super) struct WorkflowsListParams {
|
||||
// No params today. Kept as an empty struct so future filters (scope,
|
||||
// search, etc.) can slot in without breaking older clients.
|
||||
/// When `true`, also include capability skills (under the `skills/` roots)
|
||||
/// in the listing — not just `workflows/`-root automations. The Skills
|
||||
/// Explorer passes this so registry-installed skills (which land under
|
||||
/// `~/.openhuman/skills/`) appear in its Installed tab and flip the catalog
|
||||
/// Install button to Installed. Omitted (defaults `false`) by the
|
||||
/// Automations UI, which keeps the automations-only view.
|
||||
#[serde(default)]
|
||||
pub(super) include_skills: bool,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
|
||||
Reference in New Issue
Block a user