fix(skills): give the provider cut one owner inside the parallel walker
The dashboard endpoint GET /api/skills/hub/search passes its user-supplied `source` straight into parallel_search_sources and never applied the merged provider cut, so ?source=nvidia returned a mixed set. That was the fourth caller of the walker; the cut was copy-pasted at three of them and missing at the fourth. parallel_search_sources already computes the normalized provider filter, so the cut now lives there — applied per source before results are counted and merged. Every caller (CLI search via unified_search, CLI browse, TUI-gateway browse, dashboard router) sees the same rule with no provider logic of its own, source_counts stop reporting rows that are then dropped, and the three duplicated call-site cuts are deleted. do_browse keeps its provider-specific "No skills found for provider" message. Also: - HermesIndexSource.search now treats a whitespace-only provider_filter as "no filter", matching GitHubSource.search (the two adapters previously disagreed on the same keyword argument; unreachable through the walker, which pre-normalizes). - The regression-test fixture seeds tap caches by github_provider_for label instead of case-sensitive repo literals, and serializes metas through _skill_meta_to_dict, so a DEFAULT_TAPS casing change can no longer silently unseed the fixture. Validation: 121 targeted tests green; disabling the walker cut fails the pre-existing test_unified_search_provider_filter_keeps_index_source with the expected clawhub leak; 4/4 regression cases still red on unpatched main.
This commit is contained in:
@@ -419,17 +419,15 @@ def do_browse(page: int = 1, page_size: int = 20, source: str = "all",
|
||||
page_size = max(1, min(page_size, 100))
|
||||
c = console or _console
|
||||
all_results, source_counts, timed_out = _fetch_browse_results(c, source)
|
||||
from tools.skills_hub_github import _provider_filter_of
|
||||
if not all_results:
|
||||
c.print("[dim]No skills found in the Skills Hub.[/]\n")
|
||||
return
|
||||
# Provider filter (nvidia/openai/...) narrows GitHub-tap skills by their per-tap
|
||||
# ``extra.provider`` label (the runtime index stores them all under source="github").
|
||||
from tools.skills_hub_github import _filter_results_by_provider, _provider_filter_of
|
||||
if _provider_filter_of(source):
|
||||
all_results = _filter_results_by_provider(all_results, source)
|
||||
if not all_results:
|
||||
# Provider narrowing happens inside parallel_search_sources; keep the
|
||||
# provider-specific empty message.
|
||||
if _provider_filter_of(source):
|
||||
c.print(f"[dim]No skills found for provider '{source}'.[/]\n")
|
||||
return
|
||||
else:
|
||||
c.print("[dim]No skills found in the Skills Hub.[/]\n")
|
||||
return
|
||||
deduped, page_items, page, total_pages, start = _rank_and_page(all_results, page, page_size)
|
||||
_render_browse_page(c, deduped, page_items, page, total_pages, start, source,
|
||||
source_counts, timed_out)
|
||||
@@ -437,7 +435,6 @@ def do_browse(page: int = 1, page_size: int = 20, source: str = "all",
|
||||
|
||||
def browse_skills(page: int = 1, page_size: int = 20, source: str = "all") -> dict:
|
||||
"""Paginated hub browse for programmatic callers (e.g. TUI gateway)."""
|
||||
from tools.skills_hub_github import _filter_results_by_provider, _provider_filter_of
|
||||
from tools.skills_hub_search import parallel_search_sources
|
||||
page_size = max(1, min(page_size, 100))
|
||||
# The shared parallel walker carries the index-aware source-skip logic — querying
|
||||
@@ -445,10 +442,6 @@ def browse_skills(page: int = 1, page_size: int = 20, source: str = "all") -> di
|
||||
all_results, _counts, _timed_out = parallel_search_sources(
|
||||
_sources(), query="", per_source_limits=_BROWSE_API_LIMITS,
|
||||
source_filter=source, overall_timeout=30)
|
||||
# Same provider rule as do_browse: sources that cannot filter per-tap (official,
|
||||
# url, ...) are only cut on the merged set.
|
||||
if _provider_filter_of(source):
|
||||
all_results = _filter_results_by_provider(all_results, source)
|
||||
if not all_results:
|
||||
return {"items": [], "page": 1, "total_pages": 1, "total": 0}
|
||||
deduped, page_items, page, total_pages, _start = _rank_and_page(all_results, page, page_size)
|
||||
|
||||
@@ -7,8 +7,8 @@ import pytest
|
||||
|
||||
from hermes_cli import skills_hub as cli_hub
|
||||
from hermes_cli.subcommands.skills import build_skills_parser
|
||||
from tools.skills_hub_github import GitHubAuth, GitHubSource, _tap_cache_key
|
||||
from tools.skills_hub_models import SkillMeta, _cache_metas
|
||||
from tools.skills_hub_github import GitHubAuth, GitHubSource, _tap_cache_key, github_provider_for
|
||||
from tools.skills_hub_models import SkillMeta, _cache_metas, _skill_meta_to_dict
|
||||
from tools.skills_hub_official import HermesIndexSource
|
||||
from tools.skills_hub_search import _hermes_index_cache_file, parallel_search_sources
|
||||
|
||||
@@ -30,17 +30,14 @@ def catalog(request, monkeypatch):
|
||||
if request.param == "index":
|
||||
path = _hermes_index_cache_file()
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(json.dumps({"skills": [vars(m) for m in others + wanted]}), encoding="utf-8")
|
||||
path.write_text(json.dumps({"skills": [_skill_meta_to_dict(m) for m in others + wanted]}), encoding="utf-8")
|
||||
source = HermesIndexSource(auth)
|
||||
else:
|
||||
source = GitHubSource(auth)
|
||||
for tap in source.taps:
|
||||
key = _tap_cache_key(tap["repo"], tap.get("path", ""), tap.get("bucket"))
|
||||
entries = []
|
||||
if tap["repo"] == "openai/skills":
|
||||
entries = others
|
||||
elif tap["repo"] == "NVIDIA/skills":
|
||||
entries = wanted
|
||||
label = github_provider_for(tap["repo"])
|
||||
entries = others if label == "OpenAI" else wanted if label == "NVIDIA" else []
|
||||
_cache_metas(key, entries)
|
||||
monkeypatch.setattr(cli_hub, "_sources", lambda: [source])
|
||||
return source, others, wanted
|
||||
|
||||
@@ -323,8 +323,8 @@ class HermesIndexSource(SkillSource):
|
||||
index order as tiebreaker — a raw break-at-limit slice buried the most relevant skills.
|
||||
Provider filters narrow the catalog before ranking and limiting."""
|
||||
skills = self._skills()
|
||||
if provider_filter:
|
||||
want = provider_filter.strip().lower()
|
||||
want = provider_filter.strip().lower()
|
||||
if want:
|
||||
skills = [s for s in skills if _entry_provider(s) == want]
|
||||
if not skills:
|
||||
return []
|
||||
|
||||
@@ -122,9 +122,9 @@ def _select_active_sources(sources: List[SkillSource], source_filter: str) -> Li
|
||||
|
||||
A provider filter (nvidia/openai/...) is not a source id — the data lives
|
||||
in the index/github source under ``extra.provider`` — so it selects like
|
||||
"all". Mixed-provider sources filter before limiting; the merged results
|
||||
are filtered again. "official" is always queried alongside an explicit
|
||||
source filter.
|
||||
"all". Mixed-provider sources narrow before their top-N cut; the walker
|
||||
cuts every source's results. "official" is always queried alongside an
|
||||
explicit source filter.
|
||||
"""
|
||||
effective = "all" if _provider_filter_of(source_filter) else source_filter
|
||||
index_available = effective == "all" and any(
|
||||
@@ -148,7 +148,9 @@ def parallel_search_sources(
|
||||
"""Search all sources in parallel with an overall timeout.
|
||||
|
||||
Returns ``(all_results, source_counts, timed_out_ids)``. *on_source_done*
|
||||
is an optional ``(source_id, count) -> None`` progress callback.
|
||||
is an optional ``(source_id, count) -> None`` progress callback. Under a
|
||||
provider filter every source's results are narrowed before they are
|
||||
counted and merged, so callers need no provider logic of their own.
|
||||
"""
|
||||
from concurrent.futures import as_completed
|
||||
|
||||
@@ -176,6 +178,11 @@ def parallel_search_sources(
|
||||
for fut in as_completed(futures, timeout=overall_timeout):
|
||||
try:
|
||||
sid, results = fut.result(timeout=0)
|
||||
if provider_filter:
|
||||
# One owner for the merged provider cut: sources that cannot
|
||||
# filter per-tap (official, url, ...) are narrowed here, so
|
||||
# every caller (CLI, TUI gateway, dashboard) sees one rule.
|
||||
results = _filter_results_by_provider(results, provider_filter)
|
||||
source_counts[sid] = len(results)
|
||||
all_results.extend(results)
|
||||
if on_source_done:
|
||||
@@ -195,9 +202,6 @@ def unified_search(query: str, sources: List[SkillSource],
|
||||
source_filter: str = "all", limit: int = 10) -> List[SkillMeta]:
|
||||
"""Search all sources (in parallel) and merge results."""
|
||||
all_results, _, _ = parallel_search_sources(sources, query=query, source_filter=source_filter, overall_timeout=30)
|
||||
# Provider filters target ``extra.provider`` on the merged set, not a source id.
|
||||
if _provider_filter_of(source_filter):
|
||||
all_results = _filter_results_by_provider(all_results, source_filter)
|
||||
deduped = _dedupe_by_trust(all_results)
|
||||
# Stable-sort by trust before truncating so the limit cut never drops a
|
||||
# builtin/official entry because a high-volume community source finished
|
||||
|
||||
Reference in New Issue
Block a user