feat(skills): discover skills from extra_skill_dirs (mirror extra_agent_dirs)#277
Conversation
…nt_dirs) Skills could only be loaded from the single global SKILLS_DIR (~/.aws/cli-agent-orchestrator/skills/), so the only way to make a project's skills available was to copy or symlink each one into that global directory. Agent profiles already support extra discovery directories via the `extra_agent_dirs` setting; this adds the symmetric `extra_skill_dirs`. - settings_service: add get_extra_skill_dirs / set_extra_skill_dirs - utils/skills: resolve load_skill_metadata/content and list_skills across the global store first, then extra_skill_dirs (first match wins, so a global skill is never shadowed; list_skills dedups by name) - api: add GET/POST /settings/skill-dirs (mirrors /settings/agent-dirs) - docs/settings.md: document extra_skill_dirs and the endpoints - tests: settings_service, utils/skills, api/settings Discovery parity only — resolution stays global/flat and default behavior is unchanged when extra_skill_dirs is empty. Project-scoped skill visibility can build on this later.
vprudnikoff
force-pushed
the
feat/extra-skill-dirs
branch
from
5c4969f to
f9f02e1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #277 +/- ##
=======================================
Coverage ? 87.34%
=======================================
Files ? 92
Lines ? 11082
Branches ? 0
=======================================
Hits ? 9680
Misses ? 1402
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@patricka3125 can you please help to review this PR? |
There was a problem hiding this comment.
Pull request overview
This PR adds project-friendly skill discovery by introducing an extra_skill_dirs setting (mirroring extra_agent_dirs) so CAO can resolve skills from additional user-configured directories without requiring per-skill symlinks into the global skill store.
Changes:
- Added
get_extra_skill_dirs/set_extra_skill_dirsto the settings service and corresponding API endpoints (GET/POST /settings/skill-dirs). - Updated skill resolution/listing to search the global skills store first, then each configured
extra_skill_dirsentry (deduping by name). - Expanded test coverage and added autouse fixtures to avoid reading a developer’s real
settings.jsonduring tests; updated docs to describe the new setting/endpoints.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/test_skills.py | Adds autouse fixture to default extra_skill_dirs to empty; adds new tests for extra-skill-dir resolution behavior. |
| test/services/test_settings_service.py | Adds settings service tests for get_extra_skill_dirs / set_extra_skill_dirs and independence from agent dirs. |
| test/cli/commands/test_skills.py | Adds autouse fixture to prevent real extra_skill_dirs from affecting CLI skills tests. |
| test/api/test_settings_api.py | Adds API tests for GET/POST /settings/skill-dirs. |
| src/cli_agent_orchestrator/utils/skills.py | Implements multi-root skill discovery and deduped listing across global + extra_skill_dirs. |
| src/cli_agent_orchestrator/services/settings_service.py | Persists extra_skill_dirs in settings.json (mirroring extra_agent_dirs). |
| src/cli_agent_orchestrator/api/main.py | Adds GET/POST /settings/skill-dirs endpoints. |
| docs/settings.md | Documents extra_skill_dirs and adds skill-dirs endpoints to the settings docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for directory in _skill_search_dirs(): | ||
| if not directory.exists(): | ||
| continue | ||
|
|
||
| try: | ||
| skills.append(load_skill_metadata(item.name)) | ||
| except Exception as exc: | ||
| logger.warning("Skipping invalid skill folder '%s': %s", item, exc) | ||
|
|
||
| return sorted(skills, key=lambda skill: skill.name) | ||
| for item in directory.iterdir(): | ||
| if not item.is_dir() or item.name in skills_by_name: | ||
| continue |
There was a problem hiding this comment.
Good catch. The non-directory child case was already handled (if not item.is_dir()), but you're right about two real gaps: (1) a configured extra_skill_dirs entry pointing at a file would hit NotADirectoryError on iterdir(), and (2) pointing at a broad project root logged a warning for every non-skill subdir. Fixed: the configured root is now guarded with is_dir(), and a subdirectory is only passed to _load_skill_folder when it actually contains SKILL.md — so unrelated folders are skipped silently, while a folder that has a SKILL.md but fails to load is still reported.
| """Update user-added extra skill directories.""" | ||
| from cli_agent_orchestrator.services.settings_service import ( | ||
| get_extra_skill_dirs, | ||
| set_extra_skill_dirs, | ||
| ) | ||
|
|
||
| result_extra: List[str] = [] | ||
| if body.extra_dirs is not None: | ||
| result_extra = set_extra_skill_dirs(body.extra_dirs) | ||
| return {"extra_dirs": result_extra or get_extra_skill_dirs()} |
There was a problem hiding this comment.
Agreed — POST /settings/skill-dirs now returns the same shape as its GET ({skills_dir, extra_dirs}), matching the agent-dirs endpoints.
| | `GET` | `/settings/agent-dirs` | Get current agent directories (merged with defaults) | | ||
| | `POST` | `/settings/agent-dirs` | Update agent directories | | ||
| | `GET` | `/settings/extra-agent-dirs` | Get extra custom directories | | ||
| | `POST` | `/settings/extra-agent-dirs` | Set extra custom directories | | ||
| | `GET` | `/settings/skill-dirs` | Get the global skill store path and extra skill directories | | ||
| | `POST` | `/settings/skill-dirs` | Set extra custom skill directories | |
There was a problem hiding this comment.
You're right — there is no /settings/extra-agent-dirs endpoint; extra agent dirs are set via the extra_dirs field on /settings/agent-dirs. Removed those two rows from the table.
…I/docs
Addresses review feedback on extra_skill_dirs:
- list_skills(): guard the configured root with is_dir() (a misconfigured
entry pointing at a file no longer raises NotADirectoryError), and only
treat a subdirectory as a skill when it contains SKILL.md, so pointing at a
broad project root no longer logs a warning per unrelated folder. A folder
that has a SKILL.md but fails to load is still reported.
- POST /settings/skill-dirs now returns the same shape as its GET
({skills_dir, extra_dirs}), matching the agent-dirs endpoints.
- docs/settings.md: drop the bogus /settings/extra-agent-dirs rows (no such
endpoint; extra agent dirs are set via extra_dirs on /settings/agent-dirs).
| for item in directory.iterdir(): | ||
| if not item.is_dir() or item.name in skills_by_name: | ||
| continue | ||
| # extra_skill_dirs may point at a broad project root, so only treat a | ||
| # subdirectory as a skill when it actually contains a SKILL.md; |
There was a problem hiding this comment.
Fixed in f2f7b7f. _resolve_skill_path() now returns the first <dir>/<skill> that loads as a valid skill ("first valid match wins"), so it agrees with list_skills() — an invalid earlier folder no longer shadows a valid later one. When nothing loads cleanly it falls back to the first folder that has a SKILL.md (so the real validation error is still surfaced), then to the canonical global path. Added tests for both the shadowing case and the invalid-only case.
| def get_extra_skill_dirs() -> List[str]: | ||
| """Get extra skill scan directories (user-added custom paths).""" | ||
| settings = _load() | ||
| dirs = settings.get("extra_skill_dirs", []) | ||
| return dirs if isinstance(dirs, list) else [] | ||
|
|
||
|
|
||
| def set_extra_skill_dirs(dirs: List[str]) -> List[str]: | ||
| """Set extra skill scan directories.""" | ||
| settings = _load() | ||
| extra_skill_dirs = [d for d in dirs if d.strip()] | ||
| settings["extra_skill_dirs"] = extra_skill_dirs | ||
| _save(settings) | ||
| return extra_skill_dirs |
There was a problem hiding this comment.
Fixed in f2f7b7f. get_extra_skill_dirs() and set_extra_skill_dirs() now filter to non-empty strings, so malformed persisted data (e.g. null/numbers in a hand-edited settings.json) can't reach Path(extra) in _skill_search_dirs() and raise TypeError. Added a test covering the filtering.
|
Thanks for the update @vprudnikoff . I re-reviewed this against One issue I think is worth fixing before merge:
That means the injected catalog/MCP skill list can tell the agent a skill is available, but the subsequent Suggested fix: make list/load share the same resolution rule. Either:
I’d prefer “first valid match wins” since it matches the current |
|
@vprudnikoff Just would like to follow up do you get a chance to address the feedbacks? we are keen to merge your PRs |
…st_skills _resolve_skill_path() stopped at the first <dir>/<skill>/SKILL.md before validation, so an invalid earlier folder (e.g. global) could shadow a valid later one (e.g. an extra dir). list_skills() skips invalid folders, so the injected catalog could advertise a skill that load_skill then failed to load. Resolution now returns the first folder that loads as a valid skill, falling back to the first folder with a SKILL.md (to surface the real validation error) or the canonical global path. Also harden get/set_extra_skill_dirs to drop non-string/empty entries so malformed settings.json can't raise TypeError in _skill_search_dirs().
|
@haofeif thanks for the follow-up — addressed both points in f2f7b7f.
Added focused tests for both (invalid-earlier-folder doesn't shadow a valid extra dir; invalid-only still surfaces the real validation error; non-string/empty entries are dropped). Full test suite is green. Ready for another look. |
| try: | ||
| _load_skill_folder(candidate) | ||
| except Exception: | ||
| continue | ||
| return candidate |
There was a problem hiding this comment.
Good call — fixed in 5946bde. Resolution now parses the matched SKILL.md exactly once: _resolve_skill() walks the search dirs and returns the loaded (metadata, content), and load_skill_metadata() / load_skill_content() just take the part they need instead of re-loading the resolved path. The "first valid match wins / surface the first validation error" semantics are unchanged (when no candidate loads cleanly, the first folder's error is re-raised; with no SKILL.md anywhere, the canonical global path raises FileNotFoundError).
|
@vprudnikoff thanks for the fix. this looks good to me. Can you please update the BTW, one question. Currently we can just run Either way, i am happy to approve this once the document is updated. Many thanks again |
Add an "Extra Skill Directories" section to docs/skills.md covering the scan-in-place model, how it differs from `cao skills add` (no copy/drift), the one-level scan, and the global-first "first valid match wins" resolution order; fix the Overview line that claimed skills live in a single directory. Also refactor skill resolution to parse the matched SKILL.md exactly once: _resolve_skill() returns the loaded (metadata, content) instead of a path that callers re-load, removing the double read/parse while keeping the "first valid match wins / surface first validation error" semantics.
|
@haofeif thanks! Updated On your question: it's less about disk space and more about avoiding a second copy. I also addressed the Copilot follow-up on the resolver (it now loads the matched SKILL.md once instead of re-reading it). Tests are green. |
Thank you for your contribution @vprudnikoff ! |
Summary
Skills can only be loaded from the single global skill store (
~/.aws/cli-agent-orchestrator/skills/), so the only way to make a project's skills available to the running CAO server is to copy or symlink each skill folder into that global directory. Agent profiles already solve the equivalent problem via theextra_agent_dirssetting — this PR adds the symmetricextra_skill_dirsso skills get the same first-class, project-friendly discovery.Motivation
extra_agent_dirslets you keep agent profiles in a project repo and register the directory once; CAO then discovers them live. Skills had no equivalent, so project-local skills required a per-skill symlink farm into the global store (brittle, andcao skills addcopies rather than links). Adding the mirror setting removes that asymmetry and the symlink workaround.What changed
settings_service—get_extra_skill_dirs/set_extra_skill_dirs(mirror of the existing*_extra_agent_dirspair).utils/skills—load_skill_metadata,load_skill_content, andlist_skillsnow resolve across the global store first, then eachextra_skill_dirsentry. First match wins, so a global skill is never shadowed by an extra directory;list_skillsdedups by name and skips invalid folders.GET/POST /settings/skill-dirs(mirror of/settings/agent-dirs).settings_service,utils/skills,api/settings, plus an autouse fixture in the skills-CLI tests so the existing "empty store" assertions don't read a developer's realextra_skill_dirs.Scope / compatibility
Discovery parity only. Resolution stays global/flat, and behavior is unchanged when
extra_skill_dirsis empty (the default). Directories are scanned live on each request, exactly likeextra_agent_dirs. Project-scoped visibility (per agent / per project) is intentionally out of scope and can build on this later.Testing
pytest— full suite green.black/isort— clean.