feat: Add find-skills bundled skill and 'skillet find' command (v0.2.0) - #5
Merged
Conversation
Add a new bundled skill that teaches users how to discover and install skills using 'skillet find' command. This skill serves as the entry point for users to explore the agent skills ecosystem.
- Add 'find' as the primary command for searching skills - Keep 'search' as an alias for backward compatibility - Bump version to 0.2.0 in pyproject.toml - Update uv.lock with new version
- Create src/skillet/bundled_skills/README.md to document build destination - Update .github/workflows/publish.yml to copy skills during CI build - Fix get_skills_dir() to find skills in package or repo root - Fix _seed_default_sources() to use absolute paths for bundled skills - Update tests to accommodate new sources.json format
- Add docs/DEVELOPMENT.md with development workflow - Update README.md Quick Start with 1-2-3 steps - Update README.md Common Commands to use 'find' as primary - Update docs/AUTHORING.md to reference find/search consistently - Link to development guide from README
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a shared utils module and commands package, rewires the Click CLI to delegate to command modules (new find/init/search), implements lock-aware source materialization with normalized origins, adds a CI step to bundle skills into packaging, updates documentation and tests, and bumps the package version to 0.2.0. ChangesCLI Refactoring & Commands Module
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as skillet find
participant FindCmd as _find_command
participant API as skills.sh API
participant Render as Result Formatter
User->>CLI: skillet find python
CLI->>FindCmd: delegate term="python"
FindCmd->>FindCmd: validate term length
FindCmd->>API: search_skills_api("python")
alt API Success
API-->>FindCmd: [skills]
FindCmd->>Render: iterate results
Render->>Render: extract name, source, installs, slug
Render-->>User: print skills with links & examples
else No Results
API-->>FindCmd: []
FindCmd-->>User: "No skills found" + tips
end
sequenceDiagram
participant User
participant CLI as skillet init
participant InitCmd as _init_command
participant Config as config module
participant Utils as utils (helpers)
participant Sources as sources (apply)
participant Lock as lock/mirrors
User->>CLI: skillet init [--skip-bundled]
CLI->>InitCmd: delegate directory, skip_config, skip_bundled
InitCmd->>Config: ensure config dir & load
alt not skip_bundled
InitCmd->>Utils: _seed_default_sources (bundled skills)
Utils-->>InitCmd: count seeded
end
InitCmd->>Utils: _ensure_project_skills_dir
InitCmd->>Utils: _github_token()
InitCmd->>Sources: apply_all_sources(sources.json)
Sources-->>InitCmd: errors, MaterializeSummary
InitCmd->>Utils: _record_applied_skills from summary
InitCmd->>Utils: _materialize_summary_lines
InitCmd-->>User: print materialized status
alt not skip_config
InitCmd->>Config: ensure_project_agents
InitCmd->>Utils: _emit_native_mirrors
InitCmd->>Lock: record via lock
InitCmd-->>User: print mirror mappings
end
InitCmd-->>User: "✓ Init complete!"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/test_cli.py (1)
51-54: ⚡ Quick winStrengthen the new
pathcontract assertion.Right now this only checks key presence. Since
pathis part of the new behavior, assert it is absolute and points to a real skill directory.Proposed test tightening
assert sources["git-os"]["kind"] == "local" assert sources["git-os"]["source"] == "git-os" assert "path" in sources["git-os"] + git_os_path = Path(sources["git-os"]["path"]) + assert git_os_path.is_absolute() + assert (git_os_path / "SKILL.md").is_file()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 51 - 54, The test currently only checks that "path" exists on sources["git-os"]; update the assertion to validate that sources["git-os"]["path"] is an absolute path and points to an existing skill directory by asserting os.path.isabs(sources["git-os"]["path"]) and that the path is a directory (e.g., os.path.isdir or Path(...).is_dir()), and, if applicable, assert the directory contains an expected marker file/name for a skill (for example a known file or subfolder) to ensure it's a real skill directory.src/skillet/cli.py (1)
217-237: ⚡ Quick winAdd a focused regression test for
init --skip-bundled.This introduces new init behavior, but there’s no explicit test in the provided changes validating that bundled sources are not seeded when the flag is used.
As per coding guidelines,
tests/**/*.py: Usepytestfor testing with configuration inpyproject.tomland test paths oftests/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/cli.py` around lines 217 - 237, Add a pytest regression test under tests/ that invokes the CLI init flow with the --skip-bundled flag and asserts that _seed_default_sources is not called (or that no bundled sources are written to the project config). Use Click's CliRunner to invoke the command entrypoint that calls init_cmd (or call init_cmd through its Click wrapper), set up a temporary project_dir (tmp_path) and verify after invocation that get_project_config_dir(project_dir) exists with project config but that _seed_default_sources returned/created nothing (or that the .skillet/config/sources.json file is absent or unchanged). Mock _seed_default_sources where appropriate to assert it was not invoked; place the test in tests/ following pytest conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/DEVELOPMENT.md`:
- Around line 124-136: The fenced code block containing the project tree
(starting with the line "```" before "agent-skillet/") lacks a language
specifier and triggers markdownlint MD040; update the opening fence from ``` to
```text (or ```plain) so it becomes ```text to silence the warning and preserve
the plain text tree layout in the block where the snippet showing
"agent-skillet/" and its children appears.
In `@pyproject.toml`:
- Around line 49-51: The package-data entry for "skillet" uses the incorrect
relative pattern "../skills/*/SKILL.md" which resolves outside the package and
won't be packaged; update the pattern under [tool.setuptools.package-data] for
the "skillet" package to "bundled_skills/*/SKILL.md" (paths are relative to the
package dir, e.g., src/skillet) so the SKILL.md files in
src/skillet/bundled_skills/* are included in the wheel.
In `@skills/find-skills/SKILL.md`:
- Around line 148-149: Replace the relative reference "docs/AUTHORING.md" in
SKILL.md with an absolute URL so links still work when the skill is mirrored
(e.g., change the string 'docs/AUTHORING.md' to
'https://github.com/508-dev/agent-skillet/blob/main/docs/AUTHORING.md'); update
the SKILL.md line that currently contains the relative path to use the absolute
GitHub URL.
- Around line 1-3: Add the required YAML frontmatter to the top of the SKILL.md
file so the skill is discoverable: insert a YAML block delimited by --- lines
containing at minimum the keys name (set to "Find Skills" or the canonical skill
name) and description (a short sentence describing the skill's purpose); ensure
the frontmatter appears before any markdown content so the skillet CLI (skillet
find/list) can index and display this skill.
In `@src/skillet/cli.py`:
- Line 1: Remove the unused import "importlib.resources" from the top of
src/skillet/cli.py (the import statement referencing importlib.resources) so
Ruff no longer raises F401; simply delete that import line or replace it with
only the actually used imports in the module.
---
Nitpick comments:
In `@src/skillet/cli.py`:
- Around line 217-237: Add a pytest regression test under tests/ that invokes
the CLI init flow with the --skip-bundled flag and asserts that
_seed_default_sources is not called (or that no bundled sources are written to
the project config). Use Click's CliRunner to invoke the command entrypoint that
calls init_cmd (or call init_cmd through its Click wrapper), set up a temporary
project_dir (tmp_path) and verify after invocation that
get_project_config_dir(project_dir) exists with project config but that
_seed_default_sources returned/created nothing (or that the
.skillet/config/sources.json file is absent or unchanged). Mock
_seed_default_sources where appropriate to assert it was not invoked; place the
test in tests/ following pytest conventions.
In `@tests/test_cli.py`:
- Around line 51-54: The test currently only checks that "path" exists on
sources["git-os"]; update the assertion to validate that
sources["git-os"]["path"] is an absolute path and points to an existing skill
directory by asserting os.path.isabs(sources["git-os"]["path"]) and that the
path is a directory (e.g., os.path.isdir or Path(...).is_dir()), and, if
applicable, assert the directory contains an expected marker file/name for a
skill (for example a known file or subfolder) to ensure it's a real skill
directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 303286ab-c9e5-4ef2-af91-51c7dee6fb0b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/publish.ymlAGENTS.mdREADME.mddocs/AUTHORING.mddocs/DEVELOPMENT.mdpyproject.tomlskills/find-skills/SKILL.mdsrc/skillet/bundled_skills/README.mdsrc/skillet/cli.pytests/test_cli.pytests/test_integration.py
- Add search_skills_api() function to query skills.sh API - Add SEARCH_API_BASE with env var override (SKILLS_API_URL) - Modify 'skillet find' to search skills.sh instead of local skills - Keep 'skillet search' for local skill search (legacy behavior) - Add format_installs() helper to format install counts (K/M) - Update documentation to reflect new behavior - API endpoint: https://skills.sh/api/search?q={query}&limit=10
- Add find-skills/SKILL.md - Add sprint/SKILL.md - These were created earlier but not committed - All tests pass (84/84)
- Clean state with all 84 tests passing - Ready for surgical refactoring following design patterns
Move shared utility functions from cli.py to utils.py: - get_skills_dir, get_project_skills_dir - _seed_default_sources, _emit_native_mirrors - _github_token, _print_sync_errors, _sync_footer - _ensure_project_skills_dir, _print_mirror_lines - _print_tracked_sources_count, _origin_from_source_entry - _record_applied_skills, _materialize_summary_lines - search_skills_api, format_installs
Create commands/ package with separate files for each command: - commands/__init__.py: package exports - commands/find.py: find command implementation (search skills.sh API) - commands/search.py: search command implementation (local search) - commands/init.py: init command implementation (initialize project)
Update cli.py to only have Click command definitions (entry points): - Import utility functions from utils.py - Import command implementations from commands/ package - Remove all implementation logic (now in utils.py and commands/) - cli.py reduced from 530 lines to 233 lines
Update tests to work with refactored cli.py: - Patch skillet.commands.init.ensure_project_agents instead of skillet.cli.ensure_project_agents - Tests now work with utils.py and commands/ package structure
Route imports through canonical modules, trim utils re-exports, and patch tests to use skillet.commands.init for ensure_project_agents. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/skillet/utils.py (1)
96-106: 💤 Low valueDev-tree fallback can match an unrelated
skills/directory in installed environments.
Path(skillet.__file__).parent.parent.parentlands insidesite-packages/for a typical pip install. If any sibling distribution there happens to ship a top-levelskills/directory containing aSKILL.mdsubdir (unlikely, but not impossible — and entirely possible in editable installs from monorepos), it will be returned as the bundled skills dir.Gate this branch on a dev-marker (e.g., presence of
pyproject.tomlor a.gitdirectory atpkg_path.parent.parent.parent) so it only triggers in development checkouts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/utils.py` around lines 96 - 106, The dev-tree fallback that computes pkg_path = Path(skillet.__file__).parent.parent.parent and returns skills_dir can accidentally match an unrelated installed `skills/` directory; restrict this branch to real development checkouts by verifying a dev-marker before using pkg_path: check for presence of a project marker (e.g., pkg_path.joinpath("pyproject.toml").exists() or pkg_path.joinpath(".git").is_dir()) and only iterate skills_dir if that marker is present; keep the existing import skillet and skills_dir logic but wrap it with the dev-marker guard so the code only returns the repo-root `skills/` during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.skillet/skills/sprint/SKILL.md:
- Around line 30-32: The markdown code fences in SKILL.md that show the example
branch pattern ("<type>/<ticket-id>-<short-description>"), the example commit
line ("feat(PROJ-123): Add user login"), and the git snippet currently have no
language identifiers; add explicit fence languages to silence MD040 by changing
the first fence to ```text for the pattern block, the single-line example fence
to ```text, and the multi-line git commands block to ```bash so the blocks read
e.g. ```text <type>/..., ```text feat(PROJ-123):..., and ```bash git checkout -b
... respectively.
In `@src/skillet/cli.py`:
- Around line 218-223: Update the CLI help string for the search command in
function search_cmd so it no longer claims to be an "alias for find"; change the
docstring from "Search local skills by name or description (alias for find)" to
a description that accurately reflects local behavior (e.g., "Search local
skills by name or description" or add "(local)"/ "(legacy)" if desired) so users
won't expect API-backed behavior; locate the docstring on the
`@main.command`("search") function that calls _search_command and modify it
accordingly.
- Around line 7-38: The file imports many symbols from skillet.utils that are no
longer used (get_skills_dir, _seed_default_sources, search_skills,
load_project_config, save_project_config, ensure_project_agents,
get_project_config_dir, PROJECT_CONFIG_VERSION) causing F401; remove those
unused names from the import list in src/skillet/cli.py and instead import only
the actually used symbols (e.g., _github_token, _print_sync_errors,
_materialize_summary_lines, _print_mirror_lines, _print_tracked_sources_count,
_record_applied_skills, _emit_native_mirrors, _sync_footer, apply_all_sources,
load_sources, sources_json_path, add_sources, apply_sources_and_emit,
is_managed, remove_source_entry, unrecord_skill, remove_skill,
get_skills_from_directory, run_config_wizard) and if any remaining symbols now
live in other modules (per the refactor) re-point those specific imports to
their new modules such as skillet.config.*, skillet.sources, or
skillet.installer.* rather than skillet.utils; keep the existing command imports
(_find_command, _search_command, _init_command) as-is.
In `@src/skillet/commands/__init__.py`:
- Around line 3-7: The __all__ list in this module exports names that don't
exist and causes AttributeError on wildcard imports; update __all__ to only
include the actual bindings or create explicit aliases. Either remove "find",
"search", "init" from __all__ so it contains only "_find_command",
"_search_command", "_init_command", or add aliases (e.g., assign find =
_find_command; search = _search_command; init = _init_command) and then include
those names in __all__ to expose them as intended.
In `@src/skillet/commands/find.py`:
- Around line 9-13: The CLI accepts a "directory" argument but _find_command
never uses it; update the command to remove the unused parameter by deleting the
`@click.argument`("directory", default=".") from find_cmd, change _find_command
signature to accept only term (def _find_command(term: str) -> None), and update
any call sites or tests that pass directory to call the single-argument
function; alternatively, if you prefer to keep the arg, explicitly document it
in the help text and add a TODO/reserved comment in _find_command referencing
"directory" so users know it's intentionally unused.
- Line 4: The file imports Path but never uses it causing an F401 lint failure;
remove the unused import by deleting the "Path" symbol from the import statement
in src/skillet/commands/find.py (the line "from pathlib import Path") so only
needed modules remain and CI lint (Ruff) passes.
In `@src/skillet/commands/init.py`:
- Around line 7-23: Replace the indirect imports from skillet.utils with direct
imports from the canonical module skillet.config.project for the config/project
helpers; specifically import ensure_project_agents, load_project_config,
save_project_config, get_project_config_dir, and PROJECT_CONFIG_VERSION from
skillet.config.project instead of skillet.utils, updating the import statement
in src/skillet/commands/init.py to reference those symbols from
skillet.config.project so you no longer depend on re-exports in skillet.utils.
- Around line 57-64: The save_project_config call is incorrectly outside the "if
not skip_config" block causing an unnecessary save when skip_config is true;
move the save_project_config(project_dir, proj_cfg) line so it is executed only
inside the same "if not skip_config:" block that calls
ensure_project_agents(project_dir) and proj_cfg =
load_project_config(project_dir), preserving the subsequent calls to
_emit_native_mirrors(project_dir) and _print_mirror_lines(written) behavior
unchanged.
In `@src/skillet/utils.py`:
- Around line 3-34: The file imports many symbols that are not used here
(importlib.resources, __version__, PROJECT_CONFIG_VERSION,
ensure_project_agents, get_project_config_dir, load_project_config,
save_project_config, run_config_wizard, remove_skill, is_managed,
unrecord_skill, get_skills_from_directory, search_skills, add_sources,
apply_sources_and_emit, apply_all_sources, remove_source_entry,
sources_json_path); remove these unused imports from src/skillet/utils.py and
update the consumers (e.g., cli.py and commands/init.py) to import the needed
symbols directly from their original modules (skillet.config.*,
skillet.installer.*, skillet.sources.*, skillet.operations.*) instead of from
skillet.utils; if you intentionally want to re-export them, add an explicit
__all__ and per-line “# noqa: F401” instead of leaving unused imports; re-run
Ruff and pytest to confirm the F401 violations are resolved.
- Around line 41-67: The function search_skills_api currently swallows all
errors; update it to catch and handle specific exceptions instead: replace the
broad "except Exception: pass" with handlers for httpx.RequestError and
httpx.TimeoutException (log the exception with context using the module logger
or print), ValueError/JSONDecodeError for response.json() errors, and a fallback
Exception that re-raises unexpected bugs; additionally, when
response.status_code != 200 log a warning including response.status_code and
response.text before returning [] so callers can see non-200 failures; refer to
the function name search_skills_api and the variables response, data, and
results when making these changes.
- Around line 120-139: The _seed_default_sources function currently writes
absolute paths (via entry.resolve()) into sources.json which breaks portability;
change _seed_default_sources to avoid storing the absolute "path" for bundled
skills and instead store only the bundled name in the "source" field (or set
kind="bundled") when calling upsert_source so entries can be resolved later via
get_skills_dir; additionally ensure the local resolver (_apply_local_source)
continues to fall back to get_skills_dir for entries whose kind is "bundled" or
that lack a "path" so apply_all_sources can locate bundled skills at runtime.
---
Nitpick comments:
In `@src/skillet/utils.py`:
- Around line 96-106: The dev-tree fallback that computes pkg_path =
Path(skillet.__file__).parent.parent.parent and returns skills_dir can
accidentally match an unrelated installed `skills/` directory; restrict this
branch to real development checkouts by verifying a dev-marker before using
pkg_path: check for presence of a project marker (e.g.,
pkg_path.joinpath("pyproject.toml").exists() or
pkg_path.joinpath(".git").is_dir()) and only iterate skills_dir if that marker
is present; keep the existing import skillet and skills_dir logic but wrap it
with the dev-marker guard so the code only returns the repo-root `skills/`
during development.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a70e461c-5ff9-452e-aed3-9a08d5fd2d11
📒 Files selected for processing (12)
.skillet/skills/find-skills/SKILL.md.skillet/skills/sprint/SKILL.mdREADME.mddocs/AUTHORING.mddocs/DEVELOPMENT.mdsrc/skillet/cli.pysrc/skillet/commands/__init__.pysrc/skillet/commands/find.pysrc/skillet/commands/init.pysrc/skillet/commands/search.pysrc/skillet/utils.pytests/test_cli.py
✅ Files skipped from review due to trivial changes (3)
- .skillet/skills/find-skills/SKILL.md
- tests/test_cli.py
- docs/AUTHORING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/skillet/commands/find.py (1)
40-40: 💤 Low valueHardcoded example skill could go stale.
wshobson/agents/python-design-patternsis a specific third-party path; if that skill is ever renamed or removed from skills.sh, the example becomes misleading. Consider using a placeholder like<owner>/<repo>/<skill-path>(already shown on line 39) or pulling the top result's slug fromskillsto print a live, always-valid example.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/commands/find.py` at line 40, Replace the hardcoded example string in the output (the click.echo call in the function in src/skillet/commands/find.py) with a non‑stale placeholder or a dynamic value: either use the existing generic placeholder "<owner>/<repo>/<skill-path>" (to match line 39) or fetch the top result from the skills list and print its slug so the example is always valid; update the click.echo invocation to output that placeholder or the computed top-slug instead of "wshobson/agents/python-design-patterns".src/skillet/utils.py (3)
30-33: 💤 Low valueUnify warning log formatting.
Lines 30 and 33 use f-strings while lines 38–43, 49, and 71–73 use
%-style deferred formatting. Pick one style for consistency; deferred formatting (logger.warning("...", arg)) is preferred so the message isn't built when the warning level is suppressed.♻️ Proposed change
- except httpx.TimeoutException as e: - logger.warning(f"{SEARCH_API_BASE} search timeout: {e} q={query}") - return [] - except httpx.RequestError as e: - logger.warning(f"{SEARCH_API_BASE} search request failed: {e} q={query}") - return [] + except httpx.TimeoutException as e: + logger.warning("skills.sh search timeout: %s q=%r", e, query) + return [] + except httpx.RequestError as e: + logger.warning("skills.sh search request failed: %s q=%r", e, query) + return []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/utils.py` around lines 30 - 33, The two logger.warning calls in src/skillet/utils.py that currently use f-strings (the ones logging SEARCH_API_BASE search timeout and httpx.RequestError with variables e and query) should be converted to use deferred %-style logging so the message formatting is skipped when the level is suppressed; update the calls that reference SEARCH_API_BASE, e, and query to use logger.warning with a format string and separate arguments (matching the existing style used elsewhere in this file) rather than f-strings.
88-130: 💤 Low valueThree-tier fallback in
get_skills_diris hard to follow; consider extracting helpers.The function repeats the same "iter
bundled, find a skill dir with SKILL.md, return it" pattern three times across packaged install, dev checkout, and CWD fallback. Inliningimport skillettwice and usingraise RuntimeErrorinside atrypurely as a control-flow jump (lines 111–112) makes the intent harder to read. A small helper like_first_valid_skills_dir(candidate: Path) -> Path | Nonewould flatten the logic and remove the inner-raisetrick. No behavior change required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/utils.py` around lines 88 - 130, The get_skills_dir function repeats the same "iterate directory and find a subdir containing SKILL.md" logic and re-imports skillet twice while using a RuntimeError for control-flow; extract a helper function (e.g. _first_valid_skills_dir(candidate: Path) -> Path | None) that returns the candidate if it contains at least one subdirectory with SKILL.md or None otherwise, then simplify get_skills_dir to: (1) import skillet once and check pkg_path / "bundled_skills" using the helper, (2) compute dev repo root without raising for control flow and check pkg_path / "skills" using the helper, and (3) check project_dir.resolve() / "skills" using the helper; keep behavior identical and raise RuntimeError("Cannot find bundled skills directory") only at the end if no helper call returns a Path.
145-148: 💤 Low value
parse_skill_fileparsed but only thenamefield is consumed.If the goal is just to read the front-matter
name, parsing the whole file is fine; but note that when the front-matter is missing or malformed,meta.get("name")falls back toentry.namealready, so theor {}guard againstparse_skill_filereturningNoneis the only safety net. Consider asserting/documentingparse_skill_file's return type, or simply useentry.nameif you don't need the SKILL.mdnameoverride here. (No bug — just dead-ish parsing work per skill at init.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/utils.py` around lines 145 - 148, The code parses the entire SKILL.md via parse_skill_file but only uses the name field, which makes the full parse unnecessary or fragile; either (A) ensure parse_skill_file always returns a dict (never None) so callers can safely do meta.get("name") — update parse_skill_file's return type/contract and remove the "or {}" sentinel in the caller, or (B) avoid the full parse here and only read a lightweight name override (or default to entry.name) by replacing the parse call with a simple safe-read that returns the front-matter name if present; locate the usage around the meta variable and the name assignment in the initialization loop and apply one of these fixes (referencing parse_skill_file and the meta/name assignment).tests/test_integration.py (1)
50-52: ⚡ Quick winAvoid hard-coding the bundled skill count.
This will fail every time another bundled skill is added, even if the copy/remove behavior still works. Derive the expected value from
get_skills_dir()or assert on the specific copied skills this test cares about.Proposed change
- assert ( - count == 4 - ) # Now we have 4 bundled skills (git-os, sprint, deploy-checklist, find-skills) + expected = sum( + 1 + for entry in get_skills_dir().iterdir() + if entry.is_dir() and (entry / "SKILL.md").is_file() + ) + assert count == expected🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_integration.py` around lines 50 - 52, The test currently hard-codes the bundled skill count (assert count == 4); replace that brittle assertion by deriving the expected number from the actual bundled skills directory: call get_skills_dir() (or use the existing helper that returns the bundled source dir), list its contents to compute expected_count, and assert count == expected_count; alternatively assert presence of the specific skill names the test relies on (e.g., "git-os", "sprint", "deploy-checklist", "find-skills") by checking they are in the copied list rather than asserting a fixed total. Ensure you reference the existing get_skills_dir() helper and the variable count from the test when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/DEVELOPMENT.md`:
- Around line 38-40: Update the documentation to show that "uv run skillet
search <query>" is an alias of "uv run skillet find <query>" (they are
equivalent) instead of labeling search as "local skills (legacy)"; change the
wording around the examples using the commands (the occurrences around the
current block and the later block referenced) to state that both commands
perform the same Skill.sh API search and that "search" is an alias of "find".
In `@src/skillet/cli.py`:
- Around line 209-214: The search command currently implements a local search
path by accepting a directory argument and calling _search_command; change it to
behave as an alias of the find command by removing the extra "directory"
argument and delegating to the same handler used by find (call
_find_command(term) or the public find handler) so both verbs use the identical
code path—update the search_cmd signature to only take term: str and replace the
call to _search_command with a call to _find_command(term) (or the find handler
function name used in this module) to ensure identical behavior and results.
In `@src/skillet/utils.py`:
- Around line 70-74: The final except block in the skills search routine (the
one that calls logger.exception("skills.sh search unexpected error processing
response q=%r", query)) violates the function's documented "return [] on error"
contract by re-raising the exception; change the behavior to return an empty
list instead of raising so callers like _find_command receive [] on failures,
i.e., replace the re-raise with returning [] (or alternatively remove the outer
try entirely if you prefer letting unexpected bugs propagate) and ensure the
logger.exception call remains to capture the error context.
---
Nitpick comments:
In `@src/skillet/commands/find.py`:
- Line 40: Replace the hardcoded example string in the output (the click.echo
call in the function in src/skillet/commands/find.py) with a non‑stale
placeholder or a dynamic value: either use the existing generic placeholder
"<owner>/<repo>/<skill-path>" (to match line 39) or fetch the top result from
the skills list and print its slug so the example is always valid; update the
click.echo invocation to output that placeholder or the computed top-slug
instead of "wshobson/agents/python-design-patterns".
In `@src/skillet/utils.py`:
- Around line 30-33: The two logger.warning calls in src/skillet/utils.py that
currently use f-strings (the ones logging SEARCH_API_BASE search timeout and
httpx.RequestError with variables e and query) should be converted to use
deferred %-style logging so the message formatting is skipped when the level is
suppressed; update the calls that reference SEARCH_API_BASE, e, and query to use
logger.warning with a format string and separate arguments (matching the
existing style used elsewhere in this file) rather than f-strings.
- Around line 88-130: The get_skills_dir function repeats the same "iterate
directory and find a subdir containing SKILL.md" logic and re-imports skillet
twice while using a RuntimeError for control-flow; extract a helper function
(e.g. _first_valid_skills_dir(candidate: Path) -> Path | None) that returns the
candidate if it contains at least one subdirectory with SKILL.md or None
otherwise, then simplify get_skills_dir to: (1) import skillet once and check
pkg_path / "bundled_skills" using the helper, (2) compute dev repo root without
raising for control flow and check pkg_path / "skills" using the helper, and (3)
check project_dir.resolve() / "skills" using the helper; keep behavior identical
and raise RuntimeError("Cannot find bundled skills directory") only at the end
if no helper call returns a Path.
- Around line 145-148: The code parses the entire SKILL.md via parse_skill_file
but only uses the name field, which makes the full parse unnecessary or fragile;
either (A) ensure parse_skill_file always returns a dict (never None) so callers
can safely do meta.get("name") — update parse_skill_file's return type/contract
and remove the "or {}" sentinel in the caller, or (B) avoid the full parse here
and only read a lightweight name override (or default to entry.name) by
replacing the parse call with a simple safe-read that returns the front-matter
name if present; locate the usage around the meta variable and the name
assignment in the initialization loop and apply one of these fixes (referencing
parse_skill_file and the meta/name assignment).
In `@tests/test_integration.py`:
- Around line 50-52: The test currently hard-codes the bundled skill count
(assert count == 4); replace that brittle assertion by deriving the expected
number from the actual bundled skills directory: call get_skills_dir() (or use
the existing helper that returns the bundled source dir), list its contents to
compute expected_count, and assert count == expected_count; alternatively assert
presence of the specific skill names the test relies on (e.g., "git-os",
"sprint", "deploy-checklist", "find-skills") by checking they are in the copied
list rather than asserting a fixed total. Ensure you reference the existing
get_skills_dir() helper and the variable count from the test when making this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c6c3976-f05b-4274-bf32-0e71c8668acd
📒 Files selected for processing (27)
.github/workflows/build_and_publish.ymldocs/DEVELOPMENT.mdpyproject.tomlskills/find-skills/SKILL.mdsrc/skillet/cli.pysrc/skillet/commands/__init__.pysrc/skillet/commands/find.pysrc/skillet/commands/init.pysrc/skillet/commands/search.pysrc/skillet/config/settings.pysrc/skillet/config/wizard.pysrc/skillet/installer/emitters.pysrc/skillet/operations/add_sources.pysrc/skillet/skills/parser.pysrc/skillet/skills/search.pysrc/skillet/sources/__init__.pysrc/skillet/sources/apply.pysrc/skillet/sources/github.pysrc/skillet/utils.pytests/test_cli.pytests/test_copier.pytests/test_emitters.pytests/test_integration.pytests/test_lock.pytests/test_settings_config.pytests/test_skills.pytests/test_sources.py
✅ Files skipped from review due to trivial changes (3)
- src/skillet/sources/init.py
- src/skillet/sources/github.py
- tests/test_lock.py
Comment on lines
+38
to
+40
| uv run skillet find <query> # Search skills.sh API (primary) | ||
| uv run skillet search <query> # Search local skills (legacy) | ||
| ``` |
There was a problem hiding this comment.
skillet search behavior is documented inconsistently with the new alias model.
These lines describe search as “local skills (legacy)”, but this PR defines search as an alias of find. Please align wording so both commands are documented as equivalent.
Suggested doc fix
-uv run skillet find <query> # Search skills.sh API (primary)
-uv run skillet search <query> # Search local skills (legacy)
+uv run skillet find <query> # Search skills.sh API (primary)
+uv run skillet search <query> # Alias of `find`
...
-# Test search command (searches local skills)
+# Test search command (alias of `find`)
uv run --project /path/to/agent-skillet skillet search gitAlso applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/DEVELOPMENT.md` around lines 38 - 40, Update the documentation to show
that "uv run skillet search <query>" is an alias of "uv run skillet find
<query>" (they are equivalent) instead of labeling search as "local skills
(legacy)"; change the wording around the examples using the commands (the
occurrences around the current block and the later block referenced) to state
that both commands perform the same Skill.sh API search and that "search" is an
alias of "find".
Member
There was a problem hiding this comment.
It's legacy because it searches local skills only?
There was a problem hiding this comment.
@michaelmwu — You're right, and my original comment was incorrect. skillet search searches local/installed skills only, while skillet find queries the remote skills.sh API. They are distinct commands with different behaviors, not aliases, so labeling search as "Search local skills (legacy)" in the docs is accurate. Sorry for the noise!
🐰 ears drooping in sheepish realization
ʕ •ᴥ• ʔ ～ my bad!
✏️ Learnings added
Learnt from: michaelmwu
Repo: 508-dev/agent-skillet PR: 5
File: docs/DEVELOPMENT.md:38-40
Timestamp: 2026-05-06T15:35:26.248Z
Learning: In the `agent-skillet` project, `skillet find` queries the remote skills.sh API, while `skillet search` searches only locally installed skills. They are NOT aliases — they are separate commands with different implementations (`src/skillet/commands/find.py` vs `src/skillet/commands/search.py`). The `search` command is considered "legacy" because it only operates on locally installed skills.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
209
to
+214
| @main.command("search") | ||
| @click.argument("term") | ||
| @click.argument("directory", default=".") | ||
| def search_cmd(term: str, directory: str) -> None: | ||
| """Search all skills by name or description.""" | ||
| project_dir = Path(directory).resolve() | ||
| project_skills = get_project_skills_dir(project_dir) | ||
|
|
||
| if not project_skills.exists(): | ||
| click.echo("No skills installed. Run 'skillet init' first.") | ||
| return | ||
|
|
||
| skills = get_skills_from_directory(project_skills) | ||
| results = search_skills(skills, term) | ||
|
|
||
| if not results: | ||
| click.echo(f"No skills found matching '{term}'") | ||
| return | ||
|
|
||
| click.echo(f"\nSearch results for '{term}':\n") | ||
| for skill in results: | ||
| click.echo(f" {skill['name']} (score: {skill['score']})") | ||
| if skill["description"]: | ||
| click.echo(f" {skill['description']}") | ||
| """Search local skills by name or description.""" | ||
| _search_command(term, directory) |
There was a problem hiding this comment.
search still does local search instead of behaving like the documented alias.
The PR objective says skillet search <query> should alias skillet find <query>, but this command still accepts a directory argument and calls _search_command, so users get a different code path and different results depending on which verb they type.
Proposed change
`@main.command`("search")
`@click.argument`("term")
-@click.argument("directory", default=".")
-def search_cmd(term: str, directory: str) -> None:
- """Search local skills by name or description."""
- _search_command(term, directory)
+def search_cmd(term: str) -> None:
+ """Alias for `find`."""
+ _find_command(term)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @main.command("search") | |
| @click.argument("term") | |
| @click.argument("directory", default=".") | |
| def search_cmd(term: str, directory: str) -> None: | |
| """Search all skills by name or description.""" | |
| project_dir = Path(directory).resolve() | |
| project_skills = get_project_skills_dir(project_dir) | |
| if not project_skills.exists(): | |
| click.echo("No skills installed. Run 'skillet init' first.") | |
| return | |
| skills = get_skills_from_directory(project_skills) | |
| results = search_skills(skills, term) | |
| if not results: | |
| click.echo(f"No skills found matching '{term}'") | |
| return | |
| click.echo(f"\nSearch results for '{term}':\n") | |
| for skill in results: | |
| click.echo(f" {skill['name']} (score: {skill['score']})") | |
| if skill["description"]: | |
| click.echo(f" {skill['description']}") | |
| """Search local skills by name or description.""" | |
| _search_command(term, directory) | |
| `@main.command`("search") | |
| `@click.argument`("term") | |
| def search_cmd(term: str) -> None: | |
| """Alias for `find`.""" | |
| _find_command(term) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/skillet/cli.py` around lines 209 - 214, The search command currently
implements a local search path by accepting a directory argument and calling
_search_command; change it to behave as an alias of the find command by removing
the extra "directory" argument and delegating to the same handler used by find
(call _find_command(term) or the public find handler) so both verbs use the
identical code path—update the search_cmd signature to only take term: str and
replace the call to _search_command with a call to _find_command(term) (or the
find handler function name used in this module) to ensure identical behavior and
results.
Add a UTF-8 block-letter banner to the CLI entry point that prints before each subcommand invocation. Suppress the banner when `-h` or `--help` is requested to keep help output clean. Behavior when no subcommand is provided remains unchanged.
Return an empty list instead of re-raising exceptions when processing skills.sh search API responses fails unexpectedly. This prevents crashes due to malformed or unexpected responses.
Member
|
This is a Codex generated review. Please validate. Findings:
Validation:
|
michaelmwu
reviewed
May 6, 2026
michaelmwu
left a comment
michaelmwu
left a comment
Member
There was a problem hiding this comment.
Visual inspection is okay but see AI code review
Enhance the GitHub source application logic to avoid redundant network calls when the local skill matches the lock origin.
Include ruff_cache/ in .gitignore to prevent caching files from being tracked in the repository.
…y file references
…matting Improve the search command by adding a more informative output format, including the number of matching skills and their respective scores. Introduce a function to determine terminal text width for better readability and include separators between results for clarity.
Member
|
This is a Codex generated review. Please validate. Follow-up finding after the latest PR update:
Validation:
|
…d a TODO file for future enhancements
Collaborator
Author
@michaelmwu |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/skillet/utils.py (1)
210-248: ⚡ Quick winAvoid re-deriving lock origins after apply.
apply_all_sources()already records the resolved origin in the source-specific code path, but_record_applied_skills()rebuilds it from rawsources.jsonand can overwrite that with a less precise value (local:skills/<name>, non-canonical GitHub spelling, etc.). That makes the lock describe the spec text rather than what was actually materialized.Prefer treating the apply-layer
record_skill()call as the source of truth, or at least canonicalize/reuse the already-recorded origin instead of rebuilding it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/skillet/utils.py` around lines 210 - 248, Currently _record_applied_skills rebuilds an origin from raw sources.json (via _origin_from_source_entry) and can overwrite the canonical origin recorded during apply_all_sources; change _record_applied_skills to prefer the already-recorded origin from the lock (e.g. lock.get("skills", {}).get(name, {}).get("origin")) and only fall back to _origin_from_source_entry when no origin exists, then pass that chosen origin into record_skill; ensure you still preserve mirrors extraction logic and do not call _origin_from_source_entry unconditionally so the apply-layer recorded origin remains the source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/skillet/sources/apply.py`:
- Around line 164-175: The early-return optimization in apply.py (the
conditional that checks dest.is_dir(), (dest / "SKILL.md").is_file(),
is_managed(project_dir, skill_name) and
_lock_github_matches_source(origin_locked, source_str)) is unsafe because it
only compares lock metadata and can skip re-materializing a corrupted/edited
skill on disk; remove this skip so managed GitHub sources are always re-fetched
during apply (i.e., delete the entire if block that returns None) or, if you
insist on keeping the optimization, replace the _lock_github_matches_source
check with a verification against an immutable artifact identity (e.g., resolved
commit SHA or archive digest) stored in the lock and use that (compare stored
SHA/digest to the resolved value before skipping); update
record_skill(project_dir, skill_name, origin=f"github:{canon}", mirrors=mirrors)
to run after re-fetching as before.
---
Nitpick comments:
In `@src/skillet/utils.py`:
- Around line 210-248: Currently _record_applied_skills rebuilds an origin from
raw sources.json (via _origin_from_source_entry) and can overwrite the canonical
origin recorded during apply_all_sources; change _record_applied_skills to
prefer the already-recorded origin from the lock (e.g. lock.get("skills",
{}).get(name, {}).get("origin")) and only fall back to _origin_from_source_entry
when no origin exists, then pass that chosen origin into record_skill; ensure
you still preserve mirrors extraction logic and do not call
_origin_from_source_entry unconditionally so the apply-layer recorded origin
remains the source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a29810ad-d863-479a-8ff9-13aeddd45b15
⛔ Files ignored due to path filters (1)
.skillet/skillet.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.gitignore.skillet/config/sources.jsonREADME.mdTODO.mddocs/AUTHORING.mdsrc/skillet/cli.pysrc/skillet/commands/search.pysrc/skillet/installer/copier.pysrc/skillet/sources/apply.pysrc/skillet/utils.pytests/test_cli.pytests/test_lock.py
✅ Files skipped from review due to trivial changes (3)
- TODO.md
- .gitignore
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/skillet/commands/search.py
- tests/test_cli.py
- src/skillet/cli.py
- tests/test_lock.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Features
find-skills- helps users discover skills in the ecosystemskillet find <query>(primary),skillet search <query>(alias)skillet init --skip-bundledto skip bundled skills installationInfrastructure
skills/at repo root contains actual skill filessrc/skillet/bundled_skills/during publishget_skills_dir()to properly locate bundled skills_seed_default_sources()to use absolute pathsDocumentation
docs/DEVELOPMENT.md- development workflow guideREADME.mdwith simplified Quick Start (1-2-3 steps)README.mdCommon Commands to usefindas primarydocs/AUTHORING.mdto referencefind/searchconsistentlysrc/skillet/bundled_skills/README.mdexplaining build destinationTesting
pathfieldBreaking Changes
findcommand (primary) andsearch(alias)pathfield for bundled skills (absolute paths)Testing
Checklist
Summary by CodeRabbit
New Features
Documentation
Improvements
Chores