Skip to content

fix(skills): prefer GitHub repo skill for owner/repo installs - #52499

Open
addelh wants to merge 2 commits into
NousResearch:mainfrom
addelh:fix/github-repo-skill-install-resolution
Open

fix(skills): prefer GitHub repo skill for owner/repo installs#52499
addelh wants to merge 2 commits into
NousResearch:mainfrom
addelh:fix/github-repo-skill-install-resolution

Conversation

@addelh

@addelh addelh commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes source resolution for bare GitHub repo skill installs such as:

hermes skills install mvanhorn/last30days-skill --force

When the user passes an explicit owner/repo identifier, Hermes should first inspect that GitHub repository and install its canonical runtime skill when it can be resolved unambiguously. Before this change, the resolver walked registries in order and could fetch a stale registry alias before the current GitHub repo, even though the user typed a GitHub-shaped identifier.

What changed

  • Add a bare-GitHub-repo resolver before the generic source loop.
  • For owner/repo identifiers, inspect the GitHub repo tree and resolve to:
    • the single non-root */SKILL.md if there is exactly one, or
    • a skill directory whose basename matches the repo slug, including common -skill / -skills suffix stripping.
  • Fall back to the existing source order when the GitHub repo is ambiguous or cannot be resolved.
  • Add regression coverage proving a ClawHub alias cannot shadow an explicit GitHub repo when the repo has a canonical skill at skills/<name>/SKILL.md.

Investigation details

This came from debugging mvanhorn/last30days-skill on a live Hermes install.

Current upstream Last30Days has scanner-ignore PRs merged:

  • mvanhorn/last30days-skill#526 — adds skills/last30days/.skillignore
  • mvanhorn/last30days-skill#656 — adds/expands repo-root .skillignore

Those PRs reduce dev/docs/test noise, but they explicitly do not make the skill installable under Hermes' current community-skill policy. PR #656's body still reports:

Verdict: DANGEROUS
Decision: BLOCKED

There was also a separate resolver problem: running the creator-documented install command fetched from ClawHubSource instead of the current GitHub runtime skill.

Before this patch, programmatic resolution of mvanhorn/last30days-skill produced:

matched_source ClawHubSource
bundle source/id/name/trust: clawhub last30days-skill last30days-skill community
file_count: 89
.skillignore present: false
skills/last30days/.skillignore present: false
sample files: README.md, docs/plans/..., scripts/lib/env.py, SKILL.md

After this patch, the same identifier resolves to the GitHub runtime skill:

matched_source GitHubSource
bundle: github mvanhorn/last30days-skill/skills/last30days last30days
files: 104
.skillignore present: true

Important caveat: this PR fixes the stale-source/alias-shadowing bug only. Last30Days direct install is still blocked by the scanner after resolving to the correct GitHub skill because required runtime files and SKILL.md still trigger dangerous findings under current policy:

scan dangerous findings 67 critical 12
force_allowed False
reason Blocked (community source + dangerous verdict, 67 findings). --force does not override a dangerous verdict.

So this is not claiming to make Last30Days install successfully by itself. It makes Hermes fetch the right package before the scanner makes its decision, which is necessary for the scanner-ignore work in skill repos to matter.

Tests

python -m pytest tests/hermes_cli/test_skills_hub_resolution.py tests/tools/test_skills_hub.py -q -o 'addopts='
# 145 passed

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Jun 25, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting a current resolver gap: on current main, hermes_cli/skills_hub.py:126-151 walks sources in order, while tools/skills_hub.py:638-640 makes a bare owner/repo impossible for GitHubSource to fetch directly.

Problems

  • hermes_cli/skills_hub.py:142 matches every two-segment identifier, not only unqualified GitHub repositories. This includes explicit source-qualified forms such as official/<skill>. tools/skills_hub.py:4066-4077 registers official as a source, but the new preflight would query GitHub for official/<skill> first, contradicting the docstring's claim that explicit registry identifiers are unchanged. It also creates avoidable GitHub API/rate-limit work and can change source selection if a matching GitHub repository exists.

Suggested changes

  • Exempt configured source prefixes from the bare-GitHub resolver and add a regression test proving official/<skill> bypasses _get_repo_tree and resolves through its intended source.

Automated hermes-sweeper review.

Comment thread hermes_cli/skills_hub.py
GitHub repo, while leaving short names and explicit registry identifiers
unchanged.
"""
if not _GITHUB_REPO_IDENTIFIER_RE.match(identifier):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also matches explicit source-qualified identifiers such as official/<skill> and clawhub/<skill>. Since the preflight runs before the normal router, those installs now probe GitHub first and can be hijacked by a matching GitHub repository. Exclude registered source prefixes here and add a regression test for official/<skill>.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 81c2cbf13. The bare owner/repo preflight now skips any identifier whose first segment matches a registered source ID, so explicit routes such as official/<skill> and clawhub/<skill> stay with the normal source router. Added a regression test that asserts official/<skill> never calls GitHub _get_repo_tree and resolves through the official source. Rebased onto current main; focused resolver tests pass (6 passed) and Ruff/diff checks are clean.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/install-update Installer, updater, packaging, wheels, doctor labels Jul 15, 2026
@addelh
addelh force-pushed the fix/github-repo-skill-install-resolution branch 2 times, most recently from 81c2cbf to 212dbf6 Compare August 16, 2026 17:12
@addelh

addelh commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main at 12eeadf8ac6eaadc624ac41d920a694d5861c918 and force-pushed head 212dbf6190e9c706f07d1c64529ea8cfbbc05473.

The rebase preserves upstream's junction-install fix from ed0a8a480 unchanged while retaining this PR's resolution contract:

  • registered source prefixes are detected dynamically through source_id() and bypass the GitHub preflight;
  • unqualified owner/repo requests still prefer the GitHub repository path.

Validation:

  • focused resolution tests: 3/3 passed in normal and hermetic runs;
  • upstream junction regression and existing skills CLI tests passed during independent review;
  • Ruff, git diff --check, and Windows-footgun checks passed;
  • independent strict review: APPROVE, no high/medium findings.

@addelh
addelh force-pushed the fix/github-repo-skill-install-resolution branch from 212dbf6 to 5967978 Compare August 17, 2026 06:17
@addelh

addelh commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (93ed11379) after the new Skills Hub local-edit protection and plugin security-scanning work. The rebase was conflict-free and range-diff confirmed both PR commits remain patch-equivalent; the dynamic registered-source guard and bare owner/repo resolution continue through the new drift and scan gates.

Validation on exact clean head 596797874:

  • focused resolution tests: 3/3 passed
  • broader Skills Hub suite: 100 passed, 1 skipped; two Windows-only failures reproduce identically on clean upstream main
  • Ruff, git diff --check, and static safety scans passed
  • independent strict review approved with no high/medium findings

The contributor branch was safely force-updated with an explicit fresh lease.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

3 participants