perf(skills): discover installed skills once per workflow lookup

[mysma-9403]

Summary

• get_workflow_with_profile — the resolution seam behind run_workflow / describe_workflow — walked the installed-skill roots twice per call: once inside load_workflows_with_profile, and again to resolve a display-name in the fallback branch.
• Extract discover_all (prune + one discovery walk) and definitions_from_discovered (builtins + parse each discovered skill); discover once and feed both the id lookup and the name fallback from that single list.
• Behavior-preserving refactor — the parsed list and resolution order are unchanged.

Problem

Resolving one skill by id parses the whole catalogue and, on the fallback path, re-walks the roots:

let workflows = load_workflows_with_profile(workspace_dir, profile_skills_root); // discover walk #1 + parse all
if let Some(exact) = workflows.iter().rev().find(|s| s.definition.id == id) { return Some(exact.clone()); }
// display-name fallback:
let slug = discover_workflows_with_profile(/* … */)   // discover walk #2 — from scratch
    .into_iter().find(|w| w.scope == WorkflowScope::Profile && w.name == id)…;

discover_workflows_with_profile already reads every skill's WORKFLOW.md/SKILL.md frontmatter; load_workflows_with_profile then reads each one again to parse its definition; and the name-fallback discovers a second full time. So a single run_workflow pays O(N_installed_skills) filesystem reads, with the fallback path re-walking the roots it just walked.

Solution

Split the work so discovery happens once and is reused:

• discover_all(workspace_dir, profile_skills_root) — prune (before discovery, unchanged ordering) + one discover_workflows_with_profile walk, returning the lightweight Workflow entries.
• definitions_from_discovered(&[Workflow]) — builtins prepended + parse each discovered skill's definition.
• load_workflows_with_profile now delegates to both (behavior identical).
• get_workflow_with_profile calls discover_all once, derives the parsed list from it, and — on the name-fallback — reuses that same discovery instead of walking the roots again.

This is a pure refactor: the parsed workflow list is byte-identical (same builtins, same discovery, same per-skill parse), the exact-id then display-name resolution order is unchanged, and the fallback reads the same discovery it did before — computed once rather than twice. It removes one full root walk per lookup (a third markdown read of every installed skill on the fallback path).

Submission Checklist

• Tests added or updated (happy path + at least one failure / edge case) — behavior-preserving refactor; covered by the existing resolution suite, which now exercises both extracted helpers: get_workflow_with_profile_resolution_matrix (id resolution + precedence), get_profile_workflow_resolves_distinct_display_name (the reused name fallback), profile_workflow_exact_id_overrides_builtin (builtin vs discovered), skill_md_only_install_resolves_by_dir_slug_not_frontmatter_name (slug vs frontmatter name), and prune_removes_legacy_bundled_only (prune-before-discover ordering). All 15 skills::registry tests pass.
• Diff coverage ≥ 80% — the two extracted helpers and the rewritten lookup are exercised by the resolution suite above (both are on the call path of every get_workflow* / load_workflows* test).
• Coverage matrix updated — N/A: behaviour-preserving performance refactor, no matrix row added/removed/renamed.
• All affected feature IDs from the matrix are listed under ## Related — N/A.
• No new external network dependencies introduced.
• Manual smoke checklist updated if this touches release-cut surfaces — N/A: internal resolution path, identical behavior.
• Linked issue closed via Closes #NNN — N/A: no tracking issue (self-identified hot-path redundancy).

Impact

• Runtime: desktop/core — every run_workflow / describe_workflow (and any get_workflow caller). Removes a redundant full discovery walk per lookup; the win scales with the number of installed skills.
• Compatibility: none — resolution results and precedence are unchanged.

Related

• Closes: N/A
• Follow-up PR(s)/TODOs: the primary lookup still parses every discovered skill's definition to match one id; resolving a single skill lazily (parse only the matched directory) is a larger change with a subtler correctness argument (workflow.toml ids can differ from the dir slug) and is intentionally out of scope here.

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: perf/skills-registry-single-discover

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6b821c5a-4b7b-4d37-a952-c8192634a9f8

📥 Commits

Reviewing files that changed from the base of the PR and between 2826259 and bee05ca.

📒 Files selected for processing (1)

• src/openhuman/skills/registry.rs

---

📝 Walkthrough

Walkthrough

Workflow discovery now runs through a shared helper. The registry reuses discovered workflows for definition loading and profile display-name resolution, removing repeated root scans.

Changes

Workflow discovery and lookup

Layer / File(s)	Summary
Centralized workflow discovery src/openhuman/skills/registry.rs	The registry discovers applicable workflows once, prunes legacy entries, and builds definitions from the shared results.
Profile lookup reuse src/openhuman/skills/registry.rs	Profile-aware lookup reuses discovered workflows for definition selection and display-name resolution. Scope filtering and slug fallback remain unchanged.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: senamakel

Poem

I hop through workflows, tidy and bright,
One discovery pass makes the path right.
Profiles find names without scanning anew,
Legacy trails vanish from view.
Squeak, says the rabbit, the registry grew!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: discovering installed skills once per workflow lookup to remove redundant discovery work.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Refactors the skills/workflow registry lookup path to avoid re-discovering installed skill roots multiple times during a single get_workflow_with_profile call (the core resolution seam behind run_workflow / describe_workflow). This reduces redundant filesystem walks while preserving the existing resolution order and outputs.

Changes:

• Extracts a shared discovery helper (discover_all) that prunes legacy bundled skills and performs a single discovery walk.
• Extracts definitions_from_discovered to parse WorkflowDefinitions from an already-discovered list, keeping built-ins prepended.
• Updates get_workflow_with_profile to reuse the single discovery result for both exact-id lookup and display-name fallback.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mysma-9403]

CI note — the two red Rust lanes are pre-existing on main, not from this PR

• Rust Feature-Gate Smoke (gates off) — its scoped cargo test --no-default-features --lib step fails on two tests in config/migration_helpers (migrate_hermes_apply_imports_markdown_entries, migrate_openclaw_apply_imports_markdown_entries_into_target_workspace), which panic with no EmbeddingHost installed — the host must call memory::embedding_host::set_embedding_host during startup wiring. Those tests are untouched by this PR. I reproduced them on a clean checkout of main with cargo test --no-default-features --lib config::migration_helpers::ops (2/8 fail) — it's fallout from the tinymemory v0.3.0 extraction, surfacing only in the gates-off test run (a plain cargo check --no-default-features on main compiles clean). Older-base PRs branched before it merged still pass this lane.
• Rust Core Coverage (cargo-llvm-cov) — the chronic pre-existing red on Rust PRs.

Rust Quality (fmt, clippy) passes, which is the signal that this PR itself compiles and lints clean under the product feature set. The change here is confined to its own module and unrelated to either red lane.