From bee05cabf1505b590463197792b6938d86a310fb Mon Sep 17 00:00:00 2001 From: Maciej Myszkiewicz Date: Wed, 12 Aug 2026 14:41:22 +0200 Subject: [PATCH] perf(skills): discover installed skills once per workflow lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_workflow_with_profile` — the resolution seam behind `run_workflow` / `describe_workflow` — walked the skill roots twice per call: once inside `load_workflows_with_profile` (which discovers, then parses every skill's definition) and again for the display-name fallback, which re-ran `discover_workflows_with_profile` from scratch only to map a frontmatter name back to its slug. Extract two helpers — `discover_all` (prune + a single discovery walk) and `definitions_from_discovered` (builtins + parse each discovered skill) — and have `get_workflow_with_profile` discover once, feeding both the parsed- definition lookup and the name fallback from the same list. `load_workflows_with_profile` now delegates to the same helpers, so its behavior is unchanged. This is a behavior-preserving refactor: the parsed workflow list is identical (same builtins, same discovery, same per-skill parse), and the name fallback reads the same discovery it did before — just computed once instead of twice. The prune still runs before discovery in both paths. Removes one full root walk per lookup (a third markdown read of every installed skill on the fallback path). Covered by the existing resolution suite, which exercises both extracted helpers: `get_workflow_with_profile_resolution_matrix`, `get_profile_workflow_resolves_distinct_display_name` (the reused name fallback), `profile_workflow_exact_id_overrides_builtin`, `skill_md_only_install_resolves_by_dir_slug_not_frontmatter_name`, and `prune_removes_legacy_bundled_only` (prune-before-discover ordering). Claude-Session: https://claude.ai/code/session_01ACB4Ugi5pJMQqoCbZnVo6f --- src/openhuman/skills/registry.rs | 78 +++++++++++++++++++------------- 1 file changed, 47 insertions(+), 31 deletions(-) diff --git a/src/openhuman/skills/registry.rs b/src/openhuman/skills/registry.rs index 682e1758e1..d6d4f42ad1 100644 --- a/src/openhuman/skills/registry.rs +++ b/src/openhuman/skills/registry.rs @@ -13,7 +13,7 @@ use std::path::Path; use serde::{Deserialize, Serialize}; use crate::openhuman::agent::harness::definition::{AgentDefinition, PromptSource}; -use crate::openhuman::skills::WorkflowScope; +use crate::openhuman::skills::{Workflow, WorkflowScope}; /// One declared input — a parameter the skill needs, with a human description. /// `required` inputs must be supplied at run time; `kind` is an optional type @@ -172,10 +172,35 @@ pub fn load_workflows_with_profile( workspace_dir: &Path, profile_skills_root: Option<&Path>, ) -> Vec { - // Prune any legacy bundled skills an older build left behind so discover's - // legacy scan no longer surfaces them (idempotent). + definitions_from_discovered(&discover_all(workspace_dir, profile_skills_root)) +} + +/// Prune legacy bundled skills, then enumerate every installed skill across all +/// roots (deduped + scope-prioritised) via the same discovery the create/list +/// path uses. Returns the lightweight discovered entries **without** parsing +/// each one's definition — callers that need only a slug/name/scope (e.g. the +/// display-name fallback in [`get_workflow_with_profile`]) can reuse this list +/// instead of walking the roots again. +/// +/// The prune runs **before** discovery so its legacy scan no longer surfaces +/// the pruned skills (idempotent). +fn discover_all(workspace_dir: &Path, profile_skills_root: Option<&Path>) -> Vec { prune_legacy_default_workflows(workspace_dir); + let home = dirs::home_dir(); + let trusted = super::ops_discover::is_workspace_trusted(workspace_dir); + super::ops_discover::discover_workflows_with_profile( + home.as_deref(), + Some(workspace_dir), + profile_skills_root, + trusted, + ) +} +/// Parse a [`WorkflowDefinition`] for each already-discovered skill, prepended +/// by the built-in agents. Split out from [`load_workflows_with_profile`] so a +/// single [`discover_all`] walk can feed both the parsed list and a slug/name +/// lookup without re-discovering. +fn definitions_from_discovered(discovered: &[Workflow]) -> Vec { let mut workflows: Vec = Vec::new(); if let Ok(builtins) = crate::openhuman::agent::registry::agents::load_builtins() { @@ -188,16 +213,7 @@ pub fn load_workflows_with_profile( } } - // Enumerate across all roots (deduped + scope-prioritised) via the same - // discovery the create/list path uses, then load each one's definition. - let home = dirs::home_dir(); - let trusted = super::ops_discover::is_workspace_trusted(workspace_dir); - for wf in super::ops_discover::discover_workflows_with_profile( - home.as_deref(), - Some(workspace_dir), - profile_skills_root, - trusted, - ) { + for wf in discovered { let Some(skill_md) = wf.location.as_ref() else { continue; }; @@ -287,7 +303,13 @@ pub fn get_workflow_with_profile( id: &str, profile_skills_root: Option<&Path>, ) -> Option { - let workflows = load_workflows_with_profile(workspace_dir, profile_skills_root); + // Discover once; both the parsed-definition lookup and the display-name + // fallback below derive from this single walk. Previously each call + // discovered the roots twice — once inside `load_workflows_with_profile` + // and again for the name fallback. + let discovered = discover_all(workspace_dir, profile_skills_root); + let workflows = definitions_from_discovered(&discovered); + // Built-ins are prepended and discovered workflows follow them. Search in // reverse so the scope-resolved discovered entry (profile wins over global) // also wins over a built-in with the same runnable id. @@ -300,23 +322,17 @@ pub fn get_workflow_with_profile( // a private workflow admitted by the profile-local allow set can actually // be described and run. Keep the legacy profile-less lookup id-only: global // display names have never been runnable ids and may collide with builtins. - let home = dirs::home_dir(); - let trusted = super::ops_discover::is_workspace_trusted(workspace_dir); - let slug = super::ops_discover::discover_workflows_with_profile( - home.as_deref(), - Some(workspace_dir), - profile_skills_root, - trusted, - ) - .into_iter() - .find(|workflow| workflow.scope == WorkflowScope::Profile && workflow.name == id) - .map(|workflow| { - if workflow.dir_name.is_empty() { - workflow.name - } else { - workflow.dir_name - } - })?; + // Reuse the discovery above rather than walking the roots a second time. + let slug = discovered + .iter() + .find(|workflow| workflow.scope == WorkflowScope::Profile && workflow.name == id) + .map(|workflow| { + if workflow.dir_name.is_empty() { + workflow.name.clone() + } else { + workflow.dir_name.clone() + } + })?; workflows .into_iter()