Skip to content

Feat/issue 16 cli for skill discovery#84

Merged
rosspeili merged 16 commits into
ARPAHLS:mainfrom
rizzoMartin:feat/issue-16-cli_for_skill_discovery
May 22, 2026
Merged

Feat/issue 16 cli for skill discovery#84
rosspeili merged 16 commits into
ARPAHLS:mainfrom
rizzoMartin:feat/issue-16-cli_for_skill_discovery

Conversation

@rizzoMartin
Copy link
Copy Markdown
Contributor

Description

Implements the skillware list CLI command requested in #16.

Adds skillware/cli.py with a list subcommand that discovers all locally
installed skills by reusing SkillLoader's path resolution order
(SKILLWARE_SKILL_PATHskills/ under cwd and parents → bundled
site-packages/skills/). Output is rendered as a rich table showing ID,
version, category, issuer, description, and requirements.

The subparser structure is intentionally extensible to support follow-on
commands from #81 (skillware paths, skillware doctor, skillware config).

Type of Change (Matches Issue Templates)

  • Skill Proposal: New Skill (Contains manifest.yaml, skill.py, and instructions.md)
  • Bug Report Fix: Non-breaking change which fixes an execution error or framework bug
  • Doc Fix: Documentation Update
  • Framework Feature / RFC Updates: Core Framework Update (Changes to base_skill.py, loader.py, etc.)

Checklist (all PRs)

  • My code follows the Agent Code of Conduct.
  • I have run python -m flake8 . and pytest tests/ locally (or the subset relevant to this change).

New or updated skill (complete only if this PR adds or changes a skill under skills/)

Skip this section for framework-only, documentation-only, or other PRs that do not touch the skill registry.

Bundle & metadata

  • Skill lives at skills/<category>/<skill_name>/ (copied from templates/python_skill/ or equivalent).
  • manifest.yaml has name, version, description, valid parameters, and constitution.
  • manifest.yaml includes issuer with real name and email (not template placeholders).
  • Optional: issuer.github and issuer.org set when applicable.
  • requirements and env_vars are documented when the skill needs them.

Logic, cognition, and UI

  • skill.py is deterministic Python (no arbitrary LLM-generated code paths).
  • instructions.md explains when and how to use the skill.
  • card.json is present and its issuer matches manifest.yaml (name and email at minimum).

Tests & loader

  • test_skill.py covers execution and schema expectations.
  • SkillLoader.load_skill("<category>/<skill_name>") succeeds (or missing deps are documented).

Documentation & catalog

  • docs/skills/<skill_name>.md exists or is updated (ID, Issuer, usage).
  • docs/skills/README.md lists the skill with ID and Issuer.

Constitution & Safety (if adding or modifying a skill)

Related Issues

Fixes #16

@rosspeili
Copy link
Copy Markdown
Contributor

Hi @rizzoMartin, thanks again for picking this up, the overall approach is solid and the right call to reuse SkillLoader's path resolution order directly. A few things need attention before we can merge:

Incomplete files in the diff

Both skillware/cli.py and tests/test_cli.py appear to be truncated — cli.py ends mid-function right after the seen_ids dedup check, missing the rest of _discover_skills(), the cmd_list() function, and the argparse wiring. test_cli.py has an unfinished third test. Could you push the complete versions? It's possible something went wrong during the commit.

Missing [project.scripts] entry in pyproject.toml

For skillware list to work as a terminal command after pip install, we need:

[project.scripts]
skillware = "skillware.cli:main"

Without this the command doesn't exist as a shell entrypoint — it's referenced in your PR description but doesn't appear in the diff.

rich needs to be in requirements.txt as well

The PR adds rich to pyproject.toml dependencies, but requirements.txt also needs it so contributors who install via pip install -r requirements.txt don't hit a missing dependency when running the CLI. Both files should stay in sync — rich>=13.0 in both would be ideal.

A couple of correctness issues in the visible code

  • _get_skill_roots returns an override path even if it doesn't exist on disk — it should apply the same .exists() guard the normal path uses.
  • yaml.safe_load can return None on an empty/malformed manifest — data = yaml.safe_load(f) or {} is safer.
  • _discover_skills globs for manifest.yaml but doesn't check for skill.py. SkillLoader._is_skill_dir() already does this — worth using it here so the list only shows actually loadable skills.

Missing documentation updates

This is a new user-facing surface and the docs need to reflect it. Every other major surface in the framework has a dedicated page under docs/usage/ — the CLI deserves the same treatment. Specifically:

  • docs/usage/cli.md (new file) — Describe skillware list, its flags (category filter, custom path override), how SKILLWARE_SKILL_PATH affects output, and a sample terminal session. Follow the same structure as the other usage guides.
  • docs/usage/README.md — The "Finding skills on disk" section explains path resolution for load_skill(). skillware list is the human-readable version of that same lookup — add a line pointing to cli.md there.
  • README.md — No mention of the CLI anywhere, including the architecture diagram. Add a ## CLI section (or a bullet in Quick Start) showing skillware list as a verification step after install. The architecture tree under skillware/ should also include cli.py.
  • docs/TESTING.md — Consider adding skillware list as the first smoke-test step in the pre-commit checklist, before running pytest. It's the fastest way to confirm the install and path resolution are working.

Once the full files are up and these are addressed, happy to give it a proper pass. Thanks again for the effort on this, the path resolution mirroring and the extensible subparser design are exactly right.

You can add several commits to adress the missing points step by step instead of a huge push.

@rizzoMartin
Copy link
Copy Markdown
Contributor Author

rizzoMartin commented May 22, 2026

Hi @rosspeili, Thanks for the review, it is my firs time contributing in a real opensource project so thank you for the patience, I pushed all the requested changes in different commits as suggested and everything is ready to be reviewed.

I hope everything is clear and there is nothing truncated.

Regarding the [project.scripts] entry it was added in the previous PR

Thank you.

@rosspeili
Copy link
Copy Markdown
Contributor

Hi @rizzoMartin, great follow-up, and welcome to OS! Wish you a good start, and way to go! ARPA repos are fresh in OS as well, so you can find a lot of interesting projects to tackle, and a lot of good first issues if you're into AI, Agent Tools, Frameworks, and Security.

To the point, the complete implementation and entry point are exactly what was needed, and the overall structure is way more solid now. Two things need fixing before we can merge:

1. Missing encoding="utf-8" in cli.py

open(manifest_path) on line ~47 of skillware/cli.py has no encoding argument. Every file read in the codebase uses encoding="utf-8" explicitly, and this is enforced by our CONTRIBUTING style guide. On Windows this causes silent failures when manifest content contains non-ASCII characters (e.g. issuer names). Please change to:

with open(manifest_path, encoding="utf-8") as f:

2. capsys does not reliably capture rich.Console() output

test_cmd_list_filter_by_category uses capsys.readouterr() to assert on rich table output. rich.Console() does not always write through sys.stdout in a way that pytest's capsys intercepts — particularly in non-TTY environments like CI. The assertion assert "finance" not in captured.out could silently pass for the wrong reason.

Please pass Console(file=sys.stdout, force_terminal=False) to cmd_list via a parameter, or monkeypatch the console in the test to a StringIO buffer:

import io
buf = io.StringIO()
cmd_list(skills_root_override=tmp_path, category_filter="office", console=Console(file=buf))
assert "finance" not in buf.getvalue()

Also worth fixing while you're at it: docs/usage/cli.md is missing a newline at end of file, and pyproject.toml should pin "rich>=13.0" to match requirements.txt.

Everything else looks just great, the path resolution reuse, the dedup logic, the issuer fallback, and the graceful ImportError on missing rich are all well done. Hope this helps, and looking forward to the update. <3

@rosspeili
Copy link
Copy Markdown
Contributor

Hi @rizzoMartin, great iteration, the implementation is solid and almost everything from both rounds of review has been addressed cleanly. Three small things before we merge:

  1. docs/usage/cli.md is still missing a newline at end of file, this was flagged in the last review and wasn't added. Please append a blank line at the end of the file.

  2. print("No skills found.") should use console.print(...) instead — in cmd_list, the empty-result branch falls through to a bare print() call, bypassing the injected console parameter. This means test output using a StringIO-backed console misses this message, and it's inconsistent with how the rest of the function outputs. Please change to console.print("No skills found.").

  3. Minor cosmetic in README.md tree — the │ └── core/ line uses a plain ASCII pipe | instead of the box-drawing character used in the rest of the tree. Worth making consistent.

Everything else is well done — the path resolution mirroring, the dedup logic, the _is_skill_dir gate, the or {} guard, the encoding="utf-8", the Console(file=buf) test pattern, and the full docs suite. Once these three minor changes are in, we're good to merge. Thank you! <3

@rosspeili rosspeili merged commit 5e524a1 into ARPAHLS:main May 22, 2026
5 checks passed
@rosspeili
Copy link
Copy Markdown
Contributor

Perfect! All three are done, and the console initialization refactor actually makes cmd_list cleaner overall. This is ready to merge. Thank you for the patience on the iterations, @rizzoMartin. Great first contribution to an OS project. Welcome aboard! 🎉

@rizzoMartin
Copy link
Copy Markdown
Contributor Author

Thanks to you @rosspeili for your help and your feedback, I hope the project goes well and I will keep an eye on the issues to see if there is some more I can do to help

@rosspeili
Copy link
Copy Markdown
Contributor

The pleasure is mine, @rizzoMartin, and ofc you're more than welcome to pick up more issues. The CLI is your design, so you'll feel right at home with the sub-issues under #16. Or pick anything else that catches your eye across the board.

Out of curiosity, are you looking to build street cred in a specific area, or are there particular frameworks, libraries, or problem spaces you want to dig into? Happy to point you toward issues that would be a good fit.

To get a deeper feel for what Skillware is about, check out skillware.site and our latest post on why the old way of defining agent capabilities is broken:
skills.md is Dead — Why Your Agents Deserve Better

@rizzoMartin
Copy link
Copy Markdown
Contributor Author

Well to be honest, I work as a Data Engineer but beyond that I enjoy programming and discover new tools, I saw this repository in up-for-grabs.net and it seemed interestig for me.

I would like to focus a bit more in the Data Engineering field but i find difficult to find something to work on, so I'm just checking some AI projects just for curiosity.

I'll check the post and see if you can convince me to use skillware instead a skills.md 😉​

@rosspeili
Copy link
Copy Markdown
Contributor

Deal. 🤝 there are a few new skills under data engineering I will post as issues tomorrow. I will pong you to take a look and maybe suggest a better implementation. I have a rough idea of normalizing data for pre training.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat]: Add CLI tool for skill discovery and listing

2 participants