Skip to content

fix(skills): name stale index entries instead of generic fetch failure - #106901

Closed
nikkoxgonzales wants to merge 1 commit into
NousResearch:mainfrom
nikkoxgonzales:fix/skills-stale-index-message
Closed

nikkoxgonzales wants to merge 1 commit into
NousResearch:mainfrom
nikkoxgonzales:fix/skills-stale-index-message

Conversation

@nikkoxgonzales

Copy link
Copy Markdown

What does this PR do?

_resolve_source_meta_and_bundle already distinguishes index-hit-without-files from unknown identifiers, but do_install printed the same generic 'Could not fetch' for both — sending users off to re-check spellings for what is actually a stale skills.sh entry. Split the message (stale branch names the index + suggests search), and add a staleness caveat to do_search results from skills.sh.

Related Issue

Fixes #3259
Supersedes #3261 (stale since July — re-applied onto the current _print_fetch_failure helper, with credit to Mibayy)

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/skills_hub.py: _print_fetch_failure takes meta/source and branches stale vs unknown; do_install passes them; do_search footer warns on skills.sh results
  • tests/hermes_cli/test_skills_hub.py: 3 tests (stale message, generic preserved, search caveat)

How to Test

  1. pytest tests/hermes_cli/test_skills_hub.py -q → 11 passed
  2. Install a skills.sh identifier whose files 404 → 'Stale index entry' naming skills-sh; unknown identifier → original generic message

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs (fix(skills): clear error message for stale skills.sh index entries #3261 stale/superseded, noted above)
  • My PR contains only changes related to this fix
  • I've added tests for my changes
  • I've tested on my platform: Windows 11
  • ruff check on both changed files passes

Documentation & Housekeeping

  • N/A (no docs/config/schema changes; CLI output text only)

_resolve_source_meta_and_bundle already distinguishes index-hit-without-
files from unknown identifiers, but do_install printed the same generic
'Could not fetch' for both, sending users off to re-check spellings for
what is actually a stale skills.sh entry. Split the message, and add a
staleness caveat to do_search results from skills.sh.

Fixes NousResearch#3259. Supersedes NousResearch#3261 (stale since July — re-applied onto the
current _print_fetch_failure helper).
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) comp/cli CLI entry point, hermes_cli/, setup wizard labels Sep 9, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Summary

Names stale skills.sh index entries explicitly at install time and adds a search-time caveat, instead of reporting a generic fetch failure. Small, focused UX fix with tests for both the stale and unknown-identifier paths.

Findings

Non-blocking (nit) — hermes_cli/skills_hub.py:29: src_id resolution calls getattr(source, "source_id", lambda: "the registry")(). If source is None this safely falls back, but if any source exposes source_id as a plain string attribute rather than a method, the trailing () raises TypeError. Consider guarding with callable(...):

sid = getattr(source, "source_id", None)
src_id = sid() if callable(sid) else (sid or "the registry")

Non-blocking (nit) — hermes_cli/skills_hub.py:11: any(r.source in (...) for r in results) iterates results after the table was already printed from it. This is fine if results is a list, but if it ever becomes a one-shot generator the note would silently never print. No action needed if the caller always passes a list — just flagging the assumption.

Tests

New tests cover stale-entry messaging (test_do_install_stale_index_names_the_problem), the unchanged generic path (test_do_install_unknown_identifier_stays_generic), and the search caveat (test_do_search_warns_about_skills_sh_staleness). Good coverage for the change size.

Verdict

Looks good. The two nits above are optional hardening; nothing blocking.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via #108906 — your commit is on main as 257a704d18d1 with authorship preserved. One follow-up on top (08bb58bd4a58): the stale-entry verdict is only emitted when no adapter was rate limited (a throttled fetch also yields meta-without-bundle), and the per-search caveat was dropped since the install error now names the condition. Thanks; #3259 is closed.

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: skills.sh index contains non-existent skills - install fails with "Could not fetch"

4 participants