feat: replace bundled skills with remote catalog from llmspec.dev#180
Conversation
- Remove inline bundled_skills/ directory from package - Add remote_catalog.py: fetches skills index from https://www.llmspec.dev/skills/skills.json with 30-min TTL cache and offline fallback - Add downloader.py: downloads skill zips on-demand with zip-slip protection and 50MB bomb guard - Rewrite skill_catalog.py: wraps remote catalog with same public interface (SkillCatalog, SkillCatalogEntry, catalog singleton) - Rewrite skills_install_menu.py: TUI browser now downloads from remote - Recreate skills_completion.py: tab completion uses cached remote catalog - Update core_commands.py: /skills install uses remote catalog - Update installer.py: simplified to shared InstallResult type - Update skills_menu.py: restore install keybinding (i) for remote catalog - Register SkillsCompleter in prompt_toolkit_completion.py - Add .gitignore entry for bundled_skills/ - Remove bundled_skills artifacts from pyproject.toml wheel config - Add tests for remote catalog and downloader (fully offline/mocked) Skills are now downloaded on-demand from the llmspec.dev registry instead of being shipped inline with the package (~12MB removed). Source: https://www.llmspec.dev/skills/skills.json
📝 WalkthroughWalkthroughAdds a remote-backed skills catalog, safe ZIP downloader/installer, an interactive TUI installer, CLI integration and completions for a new Changes
Sequence DiagramsequenceDiagram
participant User as User / CLI
participant Menu as SkillsMenu
participant CatalogUI as SkillsInstallMenu
participant Catalog as SkillCatalog
participant Remote as Remote API
participant Downloader as Downloader
participant FS as File System
User->>Menu: /skills install (or press i)
Menu->>CatalogUI: run_skills_install_menu()
CatalogUI->>Catalog: request categories & entries
Catalog->>Remote: fetch_remote_catalog() (uses cache/TLL)
Remote-->>Catalog: RemoteCatalogData
Catalog-->>CatalogUI: entries grouped by category
CatalogUI->>User: display TUI (browse/select)
User->>CatalogUI: select skill + confirm
CatalogUI->>Downloader: download_and_install_skill(skill_id, url)
Downloader->>Remote: download ZIP
Remote-->>Downloader: ZIP file
Downloader->>Downloader: validate ZIP (zip-slip, size)
Downloader->>FS: extract & stage files
Downloader->>FS: move staged content to final skill dir
FS-->>Downloader: install complete / error
Downloader-->>CatalogUI: InstallResult(success,message,installed_path)
CatalogUI-->>Menu: return (indicate changes)
Menu->>User: refresh/redisplay skills list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🤖 Fix all issues with AI agents
In `@code_puppy/command_line/skills_completion.py`:
- Around line 40-46: The subcommand mapping in the SkillsCompletion class
(self.subcommands) is out of sync: it includes "tui" but omits valid handlers
"toggle", "refresh", and "help" which are implemented in handle_skills_command
in skills_menu.py; update the self.subcommands dictionary to match the actual
handlers (add entries for "toggle", "refresh", and "help" with appropriate
descriptions and remove or keep "tui" only if there is a corresponding handler),
and ensure whatever completer uses self.subcommands reads from this canonical
dict so completions align with handle_skills_command.
- Around line 108-118: The check uses document.text (variable `text`) which
includes characters after the cursor; replace `text.endswith(" ")` with the
already-computed `text_before_cursor.endswith(" ")` in this completion generator
(the block handling `subcommand == "install"`, where `parts` is inspected and
`self._get_skill_ids()` is used) and make the identical change for the other
occurrence mentioned (the later branch around the second check near the
`Completion` yields) so the logic correctly inspects only text before the
cursor.
In `@code_puppy/command_line/skills_install_menu.py`:
- Around line 540-543: The finally block in skills_install_menu.py only restores
the alternate screen and resets set_awaiting_user_input(False) but omits
flushing buffered terminal input and a short settle delay like SkillsMenu.run()
does; update the finally block to also call termios.tcflush(sys.stdin,
termios.TCIFLUSH) and time.sleep(0.1) (ensure termios and time are imported)
before clearing set_awaiting_user_input(False) so that stale keypresses don't
leak into the subsequent safe_input() prompt.
- Around line 40-43: is_skill_installed currently only checks the default
"~/.code_puppy/skills" path, which breaks custom directory support; update
is_skill_installed to iterate over get_skill_directories() and return True if
SKILL.md exists in any of those dirs, and then update the install-menu call site
that invokes download_and_install_skill (from the install menu flow) to pass an
appropriate target_dir: if the skill is already found in one of
get_skill_directories() use that directory as target_dir, otherwise use the
default or prompt the user for a target (leveraging add_skill_directory if
needed), so download_and_install_skill receives the correct installation
location.
In `@code_puppy/plugins/agent_skills/downloader.py`:
- Around line 361-369: The code currently deletes skill_dir via
_safe_rmtree(skill_dir) but then returns
InstallResult(installed_path=skill_dir), which points to a non-existent path;
change the return to omit or set installed_path to None (or an explicit empty
value) so consumers won't be given a deleted path. Locate the post-install
verification block that calls _safe_rmtree(skill_dir) and replace the
InstallResult(..., installed_path=skill_dir) with InstallResult(...,
installed_path=None) (or remove the installed_path argument if InstallResult
defaults to None) and keep the existing logger.warning call.
- Around line 54-86: _download_to_file currently writes the streamed response to
disk without size checks, risking disk exhaustion; modify _download_to_file to
track bytes written while iterating response.iter_bytes() against a configurable
cap (reuse _MAX_UNCOMPRESSED_BYTES or introduce _MAX_DOWNLOAD_BYTES), and abort
the download if the counter exceeds the cap by closing the response, deleting
the partial dest file, logging a clear warning, and returning False (or raising
a controlled exception) so _validate_zip_safety never runs on an oversized
payload; ensure the size check happens inside the loop that writes chunks and
that cleanup runs in the same exception/cleanup path.
In `@tests/test_skill_catalog.py`:
- Around line 14-26: The test mutates rc.fetch_remote_catalog in
_load_skill_catalog causing persistent cross-test contamination; change the
patching to use pytest's monkeypatch or save/restore the original: inside
_load_skill_catalog (or convert it to/accept a fixture), call
monkeypatch.setattr(rc, "fetch_remote_catalog", _no_fetch) or save original =
rc.fetch_remote_catalog and restore it after importlib.reload(module) (or
register a teardown) so rc.fetch_remote_catalog is restored; reference the
function _load_skill_catalog and the attribute rc.fetch_remote_catalog when
making the change.
🧹 Nitpick comments (9)
code_puppy/command_line/core_commands.py (1)
935-941: Return value ofrun_skills_install_menu()is discarded.The function returns a
boolindicating whether a skill was installed, but it's ignored here. This is fine for the command handler (which always returnsTrueto signal the command was consumed), but consider logging or emitting a message if installation occurred, for parity with howskills_menu.py(line 585-587) uses the return value to trackchanges_made.code_puppy/command_line/skills_install_menu.py (1)
46-60: Line 60 is unreachable dead code.The
forloop always returns whenunit == "GB"(the last iteration), so thereturnafter the loop is never reached. Not a bug, just dead code.code_puppy/command_line/skills_completion.py (1)
136-147: Subcommand matching is case-sensitive, unlike other completers in the codebase.Line 140 uses
subcommand.startswith(partial)which is case-sensitive. Other completers (e.g.,SlashCompleterline 438,AgentCompleterline 390) use.lower().startswith(...)for case-insensitive matching.♻️ Proposed fix
- if subcommand.startswith(partial): + if subcommand.lower().startswith(partial.lower()):code_puppy/plugins/agent_skills/remote_catalog.py (2)
78-81:_safe_booltreats string"false"asTrue.
bool("false")returnsTruein Python. If the remote catalog ever sends stringified booleans (e.g. from a misconfigured serializer),has_scripts/has_references/has_licensewould silently becomeTrue. Consider checking for falsy string values explicitly.Suggested defensive fix
def _safe_bool(value: Any, default: bool = False) -> bool: if value is None: return default + if isinstance(value, str): + return value.lower() in {"true", "1", "yes"} return bool(value)
110-120: Non-atomic cache write can produce corrupt reads under concurrency.
write_texttruncates then writes. A concurrent reader (e.g. another terminal session) can observe a partial/empty file. The standard fix is to write to a temporary file in the same directory and then atomically rename it.Suggested atomic write
+import os +import tempfile + def _write_cache(cache_path: Path, data: dict[str, Any]) -> bool: try: cache_path.parent.mkdir(parents=True, exist_ok=True) - # Stable formatting so diffs are readable when debugging. - cache_path.write_text( - json.dumps(data, indent=2, sort_keys=True) + "\n", encoding="utf-8" - ) + content = json.dumps(data, indent=2, sort_keys=True) + "\n" + fd, tmp_path = tempfile.mkstemp( + dir=cache_path.parent, suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(content) + os.replace(tmp_path, str(cache_path)) + except BaseException: + os.unlink(tmp_path) + raise return True except Exception as e: logger.warning(f"Failed to write cache {cache_path}: {e}") return Falsecode_puppy/plugins/agent_skills/downloader.py (1)
99-163: Zip safety checks are reasonable; note the header-trust limitation.
_validate_zip_safetyrelies oninfo.file_sizefrom zip headers, which a crafted zip can spoof. The docstring honestly acknowledges this ("somewhat"). For additional defense, consider tracking bytes written during extraction in_safe_extract_zipand aborting if cumulative output exceeds_MAX_UNCOMPRESSED_BYTES. This would catch deceptive headers at extraction time.Optional: track bytes during extraction
def _safe_extract_zip(zf: zipfile.ZipFile, extract_dir: Path) -> bool: """Safely extract zip contents into extract_dir.""" try: extract_dir.mkdir(parents=True, exist_ok=True) + total_written = 0 for info in zf.infolist(): parts = _zip_entry_parts(info.filename) @@ ... dest_path.parent.mkdir(parents=True, exist_ok=True) - with zf.open(info, "r") as src, dest_path.open("wb") as dst: - shutil.copyfileobj(src, dst) + with zf.open(info, "r") as src, dest_path.open("wb") as dst: + while True: + chunk = src.read(65536) + if not chunk: + break + total_written += len(chunk) + if total_written > _MAX_UNCOMPRESSED_BYTES: + logger.warning("Extraction exceeded size limit") + return False + dst.write(chunk)code_puppy/plugins/agent_skills/skill_catalog.py (3)
90-98: Comment on Line 95-96 contradicts the implementation.The comment says "Keep existing capitalization for words like 'Nextflow' if provided", but
part[:1].upper() + part[1:].lower()unconditionally forces title case. Input"NextFlow"would become"Nextflow", not preserve casing. Since skill IDs are likely always lowercase this is harmless, but the comment is misleading.Suggested comment fix
- # Keep existing capitalization for words like "Nextflow" if provided, - # otherwise just Title Case. - formatted.append(part[:1].upper() + part[1:].lower()) + # Title Case for non-acronym words. + formatted.append(part[:1].upper() + part[1:].lower())
248-253: Exporting a private-named function in__all__.
_format_display_namestarts with an underscore (private convention) but is explicitly exported in__all__. If it's part of the public API, consider renaming toformat_display_name. If it's intended as internal, remove it from__all__.
243-245: Module-level catalog import in skills_install_menu.py triggers network I/O when command is invoked.
catalog = SkillCatalog()at line 245 in skill_catalog.py callsfetch_remote_catalog()at import time. While skills_completion.py (used for/skillstab-completion) uses lazy import and avoids this cost, skills_install_menu.py imports catalog at module level (line 32), which triggers the network request when/skills installis invoked. The fetch has caching (30-minute TTL) and defensive error handling, so impact is limited to command startup latency on first run.For improved startup performance, consider lazy-loading the catalog in skills_install_menu.py:
# Instead of module-level: # from code_puppy.plugins.agent_skills.skill_catalog import catalog # Use a helper function: def get_catalog(): from code_puppy.plugins.agent_skills.skill_catalog import catalog return catalogThen replace
catalog.calls withget_catalog().in the file.
| if subcommand == "install": | ||
| # Case 1: exactly `install ` -> show all ids | ||
| if len(parts) == 1 and text.endswith(" "): | ||
| for skill_id in sorted(self._get_skill_ids()): | ||
| yield Completion( | ||
| skill_id, | ||
| start_position=0, | ||
| display=skill_id, | ||
| display_meta="Skill", | ||
| ) | ||
| return |
There was a problem hiding this comment.
text.endswith(" ") uses full document text, not text before cursor.
On line 110, text is document.text which includes characters after the cursor. If the cursor is positioned in the middle of the line (e.g. editing an earlier part), text.endswith(" ") may produce incorrect results. Use text_before_cursor instead, which was already computed on line 70.
🔧 Proposed fix
- if len(parts) == 1 and text.endswith(" "):
+ if len(parts) == 1 and text_before_cursor.endswith(" "):The same issue applies to line 137:
- if len(parts) == 1 and not text.endswith(" "):
+ if len(parts) == 1 and not text_before_cursor.endswith(" "):📝 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.
| if subcommand == "install": | |
| # Case 1: exactly `install ` -> show all ids | |
| if len(parts) == 1 and text.endswith(" "): | |
| for skill_id in sorted(self._get_skill_ids()): | |
| yield Completion( | |
| skill_id, | |
| start_position=0, | |
| display=skill_id, | |
| display_meta="Skill", | |
| ) | |
| return | |
| if subcommand == "install": | |
| # Case 1: exactly `install ` -> show all ids | |
| if len(parts) == 1 and text_before_cursor.endswith(" "): | |
| for skill_id in sorted(self._get_skill_ids()): | |
| yield Completion( | |
| skill_id, | |
| start_position=0, | |
| display=skill_id, | |
| display_meta="Skill", | |
| ) | |
| return |
🤖 Prompt for AI Agents
In `@code_puppy/command_line/skills_completion.py` around lines 108 - 118, The
check uses document.text (variable `text`) which includes characters after the
cursor; replace `text.endswith(" ")` with the already-computed
`text_before_cursor.endswith(" ")` in this completion generator (the block
handling `subcommand == "install"`, where `parts` is inspected and
`self._get_skill_ids()` is used) and make the identical change for the other
occurrence mentioned (the later branch around the second check near the
`Completion` yields) so the logic correctly inspects only text before the
cursor.
| def is_skill_installed(skill_id: str) -> bool: | ||
| """Return True if the skill is already installed locally.""" | ||
|
|
||
| return (Path.home() / ".code_puppy" / "skills" / skill_id / "SKILL.md").is_file() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, run the commands suggested in the review
rg -n "_DEFAULT_SKILLS_DIR" --type=py
rg -n "get_skill_directories" --type=py -C2Repository: mpfaffenberger/code_puppy
Length of output: 12238
🏁 Script executed:
#!/bin/bash
# Find the add_skill_directory function and related code
rg -n "add_skill_directory" --type=py -B2 -A5Repository: mpfaffenberger/code_puppy
Length of output: 5611
🏁 Script executed:
#!/bin/bash
# Look at the skills_install_menu.py file to understand context
wc -l code_puppy/command_line/skills_install_menu.pyRepository: mpfaffenberger/code_puppy
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Look at is_skill_installed and its context in skills_install_menu.py
sed -n '35,50p' code_puppy/command_line/skills_install_menu.pyRepository: mpfaffenberger/code_puppy
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Find all usages of is_skill_installed
rg -n "is_skill_installed" --type=py -B2 -A2Repository: mpfaffenberger/code_puppy
Length of output: 1831
🏁 Script executed:
#!/bin/bash
# Look at the config module to understand get_skill_directories
sed -n '13,45p' code_puppy/plugins/agent_skills/config.pyRepository: mpfaffenberger/code_puppy
Length of output: 1179
🏁 Script executed:
#!/bin/bash
# Check how downloader.py handles installations and if it respects custom dirs
sed -n '250,260p' code_puppy/plugins/agent_skills/downloader.pyRepository: mpfaffenberger/code_puppy
Length of output: 381
🏁 Script executed:
#!/bin/bash
# Look at discovery.py to see how it finds skills in all directories
sed -n '65,85p' code_puppy/plugins/agent_skills/discovery.pyRepository: mpfaffenberger/code_puppy
Length of output: 910
🏁 Script executed:
#!/bin/bash
# Check what discover_skills returns and if it's used in skills_install_menu
rg -n "discover_skills" code_puppy/command_line/skills_install_menu.py -B2 -A2Repository: mpfaffenberger/code_puppy
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check the SkillsInstallMenu class initialization to see where it gets its skill entries
sed -n '50,100p' code_puppy/command_line/skills_install_menu.pyRepository: mpfaffenberger/code_puppy
Length of output: 1280
🏁 Script executed:
#!/bin/bash
# Check imports and see where SkillsInstallMenu gets its data
head -30 code_puppy/command_line/skills_install_menu.pyRepository: mpfaffenberger/code_puppy
Length of output: 1237
🏁 Script executed:
#!/bin/bash
# Check how download_and_install_skill is called - what target_dir does it use?
rg -n "download_and_install_skill" code_puppy/command_line/skills_install_menu.py -B5 -A5Repository: mpfaffenberger/code_puppy
Length of output: 1475
🏁 Script executed:
#!/antml/bin/bash
# Look at the full implementation of download_and_install_skill
sed -n '200,280p' code_puppy/plugins/agent_skills/downloader.pyRepository: mpfaffenberger/code_puppy
Length of output: 2755
🏁 Script executed:
#!/bin/bash
# Verify: are there any other calls to download_and_install_skill that DO pass target_dir?
rg -n "download_and_install_skill" --type=py -B2 -A2Repository: mpfaffenberger/code_puppy
Length of output: 5224
🏁 Script executed:
#!/bin/bash
# Check if there's any test that validates is_skill_installed behavior with custom directories
rg -n "is_skill_installed" --type=pyRepository: mpfaffenberger/code_puppy
Length of output: 465
🏁 Script executed:
#!/bin/bash
# Look at the config module to see what add_skill_directory does
cat code_puppy/plugins/agent_skills/config.pyRepository: mpfaffenberger/code_puppy
Length of output: 5322
is_skill_installed only checks the default directory, inconsistent with custom skill directory support.
The function hardcodes ~/.code_puppy/skills/ but users can add custom directories via add_skill_directory(). However, this is part of a larger workflow issue: download_and_install_skill() accepts a target_dir parameter but is always called from the install menu without it (line 585), so installations always go to the default directory regardless of custom directories. This means:
- Skills in custom directories are discovered but marked "Not installed"
- If users attempt to install a discovered skill that already exists in a custom directory, it installs to the default location, creating duplicates
Either make download_and_install_skill() installation-aware of custom directories (by determining where the skill exists or prompting the user), or update is_skill_installed() to check all directories from get_skill_directories() for consistency.
🤖 Prompt for AI Agents
In `@code_puppy/command_line/skills_install_menu.py` around lines 40 - 43,
is_skill_installed currently only checks the default "~/.code_puppy/skills"
path, which breaks custom directory support; update is_skill_installed to
iterate over get_skill_directories() and return True if SKILL.md exists in any
of those dirs, and then update the install-menu call site that invokes
download_and_install_skill (from the install menu flow) to pass an appropriate
target_dir: if the skill is already found in one of get_skill_directories() use
that directory as target_dir, otherwise use the default or prompt the user for a
target (leveraging add_skill_directory if needed), so download_and_install_skill
receives the correct installation location.
| def _download_to_file(url: str, dest: Path) -> bool: | ||
| headers = { | ||
| "Accept": "application/zip, application/octet-stream, */*", | ||
| "User-Agent": "code-puppy/skill-downloader", | ||
| } | ||
|
|
||
| try: | ||
| dest.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with httpx.Client(timeout=30, headers=headers, follow_redirects=True) as client: | ||
| with client.stream("GET", url) as response: | ||
| response.raise_for_status() | ||
|
|
||
| with dest.open("wb") as f: | ||
| for chunk in response.iter_bytes(): | ||
| if chunk: | ||
| f.write(chunk) | ||
|
|
||
| logger.info(f"Downloaded skill zip to {dest}") | ||
| return True | ||
|
|
||
| except httpx.HTTPStatusError as e: | ||
| logger.warning( | ||
| "Skill download failed with HTTP status: " | ||
| f"{e.response.status_code} {e.response.reason_phrase}" | ||
| ) | ||
| return False | ||
| except (httpx.ConnectError, httpx.TimeoutException, httpx.NetworkError) as e: | ||
| logger.warning(f"Skill download network failure: {e}") | ||
| return False | ||
| except Exception as e: | ||
| logger.exception(f"Unexpected error downloading {url}: {e}") | ||
| return False |
There was a problem hiding this comment.
No download size limit — disk exhaustion risk.
_download_to_file streams the entire response to disk without tracking bytes written. A malicious or misconfigured server can serve an arbitrarily large payload, exhausting temp-dir disk space before _validate_zip_safety ever runs. Consider adding a byte counter and aborting when a threshold is exceeded (e.g., reuse _MAX_UNCOMPRESSED_BYTES or a dedicated download cap).
Suggested fix
+_MAX_DOWNLOAD_BYTES = 100 * 1024 * 1024 # 100 MB download cap
+
def _download_to_file(url: str, dest: Path) -> bool:
headers = {
"Accept": "application/zip, application/octet-stream, */*",
"User-Agent": "code-puppy/skill-downloader",
}
try:
dest.parent.mkdir(parents=True, exist_ok=True)
with httpx.Client(timeout=30, headers=headers, follow_redirects=True) as client:
with client.stream("GET", url) as response:
response.raise_for_status()
+ bytes_written = 0
with dest.open("wb") as f:
for chunk in response.iter_bytes():
if chunk:
+ bytes_written += len(chunk)
+ if bytes_written > _MAX_DOWNLOAD_BYTES:
+ logger.warning(
+ f"Download exceeded size limit "
+ f"({_MAX_DOWNLOAD_BYTES} bytes): {url}"
+ )
+ return False
f.write(chunk)🤖 Prompt for AI Agents
In `@code_puppy/plugins/agent_skills/downloader.py` around lines 54 - 86,
_download_to_file currently writes the streamed response to disk without size
checks, risking disk exhaustion; modify _download_to_file to track bytes written
while iterating response.iter_bytes() against a configurable cap (reuse
_MAX_UNCOMPRESSED_BYTES or introduce _MAX_DOWNLOAD_BYTES), and abort the
download if the counter exceeds the cap by closing the response, deleting the
partial dest file, logging a clear warning, and returning False (or raising a
controlled exception) so _validate_zip_safety never runs on an oversized
payload; ensure the size check happens inside the loop that writes chunks and
that cleanup runs in the same exception/cleanup path.
| # Post-install verification. | ||
| if not (skill_dir / "SKILL.md").is_file(): | ||
| logger.warning(f"Installed skill missing SKILL.md: {skill_dir}") | ||
| _safe_rmtree(skill_dir) | ||
| return InstallResult( | ||
| success=False, | ||
| message="Installed skill is missing SKILL.md", | ||
| installed_path=skill_dir, | ||
| ) |
There was a problem hiding this comment.
installed_path set on a just-deleted directory.
After Line 364 calls _safe_rmtree(skill_dir), Line 368 returns installed_path=skill_dir even though the directory no longer exists. Consumers checking result.installed_path.exists() would get False, which is confusing.
Suggested fix
if not (skill_dir / "SKILL.md").is_file():
logger.warning(f"Installed skill missing SKILL.md: {skill_dir}")
_safe_rmtree(skill_dir)
return InstallResult(
success=False,
message="Installed skill is missing SKILL.md",
- installed_path=skill_dir,
+ installed_path=None,
)📝 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.
| # Post-install verification. | |
| if not (skill_dir / "SKILL.md").is_file(): | |
| logger.warning(f"Installed skill missing SKILL.md: {skill_dir}") | |
| _safe_rmtree(skill_dir) | |
| return InstallResult( | |
| success=False, | |
| message="Installed skill is missing SKILL.md", | |
| installed_path=skill_dir, | |
| ) | |
| # Post-install verification. | |
| if not (skill_dir / "SKILL.md").is_file(): | |
| logger.warning(f"Installed skill missing SKILL.md: {skill_dir}") | |
| _safe_rmtree(skill_dir) | |
| return InstallResult( | |
| success=False, | |
| message="Installed skill is missing SKILL.md", | |
| installed_path=None, | |
| ) |
🤖 Prompt for AI Agents
In `@code_puppy/plugins/agent_skills/downloader.py` around lines 361 - 369, The
code currently deletes skill_dir via _safe_rmtree(skill_dir) but then returns
InstallResult(installed_path=skill_dir), which points to a non-existent path;
change the return to omit or set installed_path to None (or an explicit empty
value) so consumers won't be given a deleted path. Locate the post-install
verification block that calls _safe_rmtree(skill_dir) and replace the
InstallResult(..., installed_path=skill_dir) with InstallResult(...,
installed_path=None) (or remove the installed_path argument if InstallResult
defaults to None) and keep the existing logger.warning call.
| def _load_skill_catalog(): | ||
| """Import/reload skill_catalog without hitting the network. | ||
|
|
||
| skill_catalog creates a module-level singleton at import time, so we must | ||
| patch the remote fetcher before importing. | ||
| """ | ||
|
|
||
| def _no_fetch(*args, **kwargs): | ||
| return None | ||
|
|
||
| rc.fetch_remote_catalog = _no_fetch # type: ignore[assignment] | ||
| module = importlib.import_module("code_puppy.plugins.agent_skills.skill_catalog") | ||
| return importlib.reload(module) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if any other test files import from remote_catalog that could be affected
rg -n "remote_catalog" --type=py -g '*test*'Repository: mpfaffenberger/code_puppy
Length of output: 880
🏁 Script executed:
# Also check the skill_catalog module to understand the import pattern
fd "skill_catalog" --type=py | head -20Repository: mpfaffenberger/code_puppy
Length of output: 238
🏁 Script executed:
# Let's look at the actual import statements in the test file
cat -n tests/test_skill_catalog.py | head -30Repository: mpfaffenberger/code_puppy
Length of output: 1058
🏁 Script executed:
# Find and examine the skill_catalog module
fd "skill_catalog" -type f | grep -v testRepository: mpfaffenberger/code_puppy
Length of output: 239
🏁 Script executed:
# Check how skill_catalog imports fetch_remote_catalog
rg -n "fetch_remote_catalog\|from remote_catalog\|import remote_catalog" code_puppy/plugins/agent_skills/skill_catalog.pyRepository: mpfaffenberger/code_puppy
Length of output: 51
🏁 Script executed:
# Let's examine the skill_catalog module directly
cat code_puppy/plugins/agent_skills/skill_catalog.py | head -50Repository: mpfaffenberger/code_puppy
Length of output: 1146
🏁 Script executed:
# Find all test files to see if others might import from remote_catalog
fd "test_" -type f | head -20Repository: mpfaffenberger/code_puppy
Length of output: 239
🏁 Script executed:
# Check all test files for imports of remote_catalog
rg "from.*remote_catalog|import.*remote_catalog" --type pyRepository: mpfaffenberger/code_puppy
Length of output: 374
🏁 Script executed:
# Let's see the complete view of how tests set up and use _load_skill_catalog
cat -n tests/test_skill_catalog.py | sed -n '14,120p'Repository: mpfaffenberger/code_puppy
Length of output: 4280
🏁 Script executed:
# Check if there are any conftest.py files or other pytest fixtures that might import remote_catalog
find . -name conftest.py -type fRepository: mpfaffenberger/code_puppy
Length of output: 172
🏁 Script executed:
# Check the full test file to see all test functions and their structure
wc -l tests/test_skill_catalog.pyRepository: mpfaffenberger/code_puppy
Length of output: 100
🏁 Script executed:
# Check if there are any other files in the plugins/agent_skills directory
ls -la code_puppy/plugins/agent_skills/Repository: mpfaffenberger/code_puppy
Length of output: 843
🏁 Script executed:
# Check the main conftest.py file
cat tests/conftest.pyRepository: mpfaffenberger/code_puppy
Length of output: 5920
🏁 Script executed:
# Check if any conftest files import remote_catalog
rg "remote_catalog" tests/conftest.py tests/integration/conftest.py tests/mcp/conftest.py tests/plugins/conftest.pyRepository: mpfaffenberger/code_puppy
Length of output: 51
🏁 Script executed:
# Let's see how the skill_catalog module uses fetch_remote_catalog in detail
cat -n code_puppy/plugins/agent_skills/skill_catalog.py | sed -n '1,80p'Repository: mpfaffenberger/code_puppy
Length of output: 2162
🏁 Script executed:
# Check if any test files import skill_catalog or remote_catalog at module scope
rg "import.*skill_catalog|from.*skill_catalog" tests/Repository: mpfaffenberger/code_puppy
Length of output: 182
🏁 Script executed:
# Let's see what happens when skill_catalog is first imported - does it create a singleton?
cat -n code_puppy/plugins/agent_skills/skill_catalog.py | tail -50Repository: mpfaffenberger/code_puppy
Length of output: 1783
🏁 Script executed:
# Check if there's any module-level initialization in skill_catalog that uses fetch_remote_catalog
rg -n "catalog\s*=\|^catalog\s*=" code_puppy/plugins/agent_skills/skill_catalog.pyRepository: mpfaffenberger/code_puppy
Length of output: 51
🏁 Script executed:
# Check what SkillCatalog.__init__ does to understand how fetch_remote_catalog is used
cat -n code_puppy/plugins/agent_skills/skill_catalog.py | sed -n '145,180p'Repository: mpfaffenberger/code_puppy
Length of output: 1562
🏁 Script executed:
# Let's trace all the test functions to see the complete picture
rg -n "^def test_" tests/test_skill_catalog.pyRepository: mpfaffenberger/code_puppy
Length of output: 419
🏁 Script executed:
# Let's verify the complete __init__ method of SkillCatalog to see how it uses fetch_remote_catalog
cat -n code_puppy/plugins/agent_skills/skill_catalog.py | sed -n '120,150p'Repository: mpfaffenberger/code_puppy
Length of output: 1207
🏁 Script executed:
# Check if any test files are imported by conftest that might use skill_catalog
grep -r "skill_catalog\|remote_catalog" tests/conftest.py tests/plugins/conftest.pyRepository: mpfaffenberger/code_puppy
Length of output: 51
Direct module-attribute mutation is never restored — potential cross-test contamination.
rc.fetch_remote_catalog = _no_fetch (line 24) directly mutates the remote_catalog module attribute without cleanup. If other test modules later import fetch_remote_catalog from remote_catalog, they'll get the no-op. Consider using a monkeypatch fixture (or saving/restoring the original) to ensure proper test isolation.
🛡️ Suggested approach: convert to a fixture with monkeypatch
+@pytest.fixture(autouse=True)
+def _patch_fetch(monkeypatch):
+ """Prevent network calls during import-time singleton creation."""
+ monkeypatch.setattr(rc, "fetch_remote_catalog", lambda *a, **kw: None)
+
def _load_skill_catalog():
- """Import/reload skill_catalog without hitting the network.
-
- skill_catalog creates a module-level singleton at import time, so we must
- patch the remote fetcher before importing.
- """
-
- def _no_fetch(*args, **kwargs):
- return None
-
- rc.fetch_remote_catalog = _no_fetch # type: ignore[assignment]
+ """Import/reload skill_catalog (fetch already patched by fixture)."""
module = importlib.import_module("code_puppy.plugins.agent_skills.skill_catalog")
return importlib.reload(module)Note: This relies on the autouse fixture running before _load_skill_catalog() is called, which pytest guarantees for test functions. However, if _load_skill_catalog is called at module scope (it isn't), additional care would be needed.
🤖 Prompt for AI Agents
In `@tests/test_skill_catalog.py` around lines 14 - 26, The test mutates
rc.fetch_remote_catalog in _load_skill_catalog causing persistent cross-test
contamination; change the patching to use pytest's monkeypatch or save/restore
the original: inside _load_skill_catalog (or convert it to/accept a fixture),
call monkeypatch.setattr(rc, "fetch_remote_catalog", _no_fetch) or save original
= rc.fetch_remote_catalog and restore it after importlib.reload(module) (or
register a teardown) so rc.fetch_remote_catalog is restored; reference the
function _load_skill_catalog and the attribute rc.fetch_remote_catalog when
making the change.
… completer subcommands, fix repo-wide import sorting - Add docstrings to all 89 functions/methods/classes in new skill files - Fix skills_completion.py: remove non-existent 'tui' subcommand, add missing 'toggle', 'refresh', 'help' to match handle_skills_command - Fix 6 pre-existing import sort violations (ruff I001) across: config.py, registry.py, and 4 test files
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@code_puppy/plugins/agent_skills/skill_catalog.py`:
- Around line 247-249: The module-level instantiation catalog = SkillCatalog()
triggers network I/O at import time via fetch_remote_catalog(); change to lazy
initialization so the remote fetch happens only on first use — e.g., replace the
direct catalog variable with a lazily-constructed accessor (get_catalog() or a
LazySkillCatalog wrapper) or modify SkillCatalog so its constructor defers
fetch_remote_catalog() and exposes an explicit init/load method that runs the
network call on first method access; ensure references to catalog in other
modules call the accessor (or that LazySkillCatalog transparently proxies
methods) and keep fetch_remote_catalog, SkillCatalog, and the catalog symbol as
the touchpoints to locate and refactor.
🧹 Nitpick comments (7)
code_puppy/command_line/skills_install_menu.py (2)
46-60:_format_bytes: Line 60 is unreachable dead code.The
forloop always terminates via thereturnon Line 57/58 whenunit == "GB"(the last unit), so Line 60 can never execute. It's harmless but could be removed for clarity.Proposed cleanup
for unit in ("B", "KB", "MB", "GB"): if size < 1024.0 or unit == "GB": if unit == "B": return f"{int(size)} {unit}" return f"{size:.1f} {unit}" size /= 1024.0 - return f"{size:.1f} GB" + return f"{size:.1f} GB" # pragma: no cover – unreachable fallback
567-578: Manual alternate-screen ANSI escape management is fragile across terminals.The raw
\033[?1049h/\033[?1049lsequences work on most terminals but could misbehave on Windows or terminals without xterm alternate screen support. Sinceprompt_toolkitis already a dependency, consider using its built-in alternate screen support (e.g.,Application(full_screen=True)) which handles cross-platform differences, instead of manually writing escape sequences while settingfull_screen=False.tests/test_skill_downloader.py (1)
28-241: Consider adding tests for the security validation paths.The downloader has explicit defenses for zip-slip (path traversal entries), zip-bomb (uncompressed size cap), and
skill_nametraversal — but none of these are exercised by tests. Adding cases for these would strengthen confidence in the security guarantees.Example scenarios:
- A zip entry with
../../etc/passwdto verify zip-slip rejection- A zip with entries exceeding the 50MB uncompressed cap
- A
skill_namelike../evilto verify input validationcode_puppy/plugins/agent_skills/remote_catalog.py (1)
277-299: Verboselogger.infoon every cache hit may generate noise.Line 281 logs at
INFOlevel on every fresh-cache hit. Since this module runs on startup (via thecatalogsingleton), and potentially again during completions, theINFOmessage will appear frequently. Consider usingDEBUGfor the cache-hit path and reservingINFOfor actual network fetches.Proposed change
if not force_refresh and cache_fresh: - logger.info(f"Using fresh remote catalog cache: {_CACHE_PATH}") + logger.debug(f"Using fresh remote catalog cache: {_CACHE_PATH}") cached = _read_cache(_CACHE_PATH)code_puppy/plugins/agent_skills/skill_catalog.py (3)
93-97: Misleading comment: existing capitalization is not preserved.The comment says "Keep existing capitalization for words like 'Nextflow' if provided," but line 97 unconditionally lowercases everything after the first character (
part[1:].lower())."Nextflow"would become"Nextflow"only by coincidence of the title-case path. Consider removing the misleading comment or, if preservation is intended, changing the logic.Suggested comment fix
- # Keep existing capitalization for words like "Nextflow" if provided, - # otherwise just Title Case. - formatted.append(part[:1].upper() + part[1:].lower()) + # Title Case the word. + formatted.append(part[:1].upper() + part[1:].lower())
252-257: Private name_format_display_nameexported in__all__.By Python convention the leading underscore signals "internal." Exporting it in
__all__is contradictory. Either drop the underscore if it's genuinely public, or remove it from__all__if it's only meant for sibling modules and tests.
110-121: Inconsistent generic type syntax:List[str]vslist[str].The dataclass fields use
List[str]/Optional[Path]fromtyping(lines 115–116), while the class body uses barelist[...]/dict[...](lines 133–135, 151, 218). Pick one style consistently — sincefrom __future__ import annotationsis present, barelist/dictworks everywhere and thetypingimports can be dropped.Also applies to: 133-135
| # Singleton instance used by the rest of the codebase. | ||
| # NOTE: This must never crash import-time. | ||
| catalog = SkillCatalog() |
There was a problem hiding this comment.
Module-level singleton triggers network I/O at import time.
catalog = SkillCatalog() runs fetch_remote_catalog() the moment any module does from ... import catalog. This can stall application startup (or even hang if the network is slow and there's no timeout), makes unit tests that transitively import this module slower, and violates the file's own docstring goal: "This must never crash import-time."
Consider a lazy-initialization pattern so the fetch only happens on first access:
♻️ Proposed lazy-init pattern
-# Singleton instance used by the rest of the codebase.
-# NOTE: This must never crash import-time.
-catalog = SkillCatalog()
+# Lazy singleton – the network fetch is deferred until first attribute access.
+# NOTE: This must never crash import-time.
+
+class _LazyCatalog:
+ """Proxy that defers SkillCatalog construction to first use."""
+
+ def __init__(self) -> None:
+ self._instance: Optional[SkillCatalog] = None
+
+ def _ensure(self) -> SkillCatalog:
+ if self._instance is None:
+ self._instance = SkillCatalog()
+ return self._instance
+
+ def __getattr__(self, name: str):
+ return getattr(self._ensure(), name)
+
+
+catalog: SkillCatalog = _LazyCatalog() # type: ignore[assignment]🤖 Prompt for AI Agents
In `@code_puppy/plugins/agent_skills/skill_catalog.py` around lines 247 - 249, The
module-level instantiation catalog = SkillCatalog() triggers network I/O at
import time via fetch_remote_catalog(); change to lazy initialization so the
remote fetch happens only on first use — e.g., replace the direct catalog
variable with a lazily-constructed accessor (get_catalog() or a LazySkillCatalog
wrapper) or modify SkillCatalog so its constructor defers fetch_remote_catalog()
and exposes an explicit init/load method that runs the network call on first
method access; ensure references to catalog in other modules call the accessor
(or that LazySkillCatalog transparently proxies methods) and keep
fetch_remote_catalog, SkillCatalog, and the catalog symbol as the touchpoints to
locate and refactor.
2 participants
Skills are now downloaded on-demand from the llmspec.dev registry instead of being shipped inline with the package (~12MB removed).
Source: https://www.llmspec.dev/skills/skills.json
Summary by CodeRabbit
New Features
/skills installsubcommand and integrated install flow from the skills menu/skillssubcommands and remote skill IDsChores
Tests