From f9e3cd38f565d690802fed02841c3187692d9786 Mon Sep 17 00:00:00 2001 From: Alan Wizemann Date: Thu, 7 May 2026 11:50:52 +0200 Subject: [PATCH] fix(skills): client-side filter for All-Sources hub search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #79 — Browse Hub clearly listed "honcho" but searching for "honcho" with the source picker on "All Sources" returned nothing. Root cause is on the Hermes side: `hermes skills search ` without a `--source` flag routes through the centralized `hermes-index` source and skips the external API sources (skills-sh, github, clawhub, lobehub, well-known, claude-marketplace). Browse aggregates those sources too, so any skill that lives only in the API tier shows up in browse but disappears in search. Same picker, same query, contradictory results. Rather than chase Hermes's index gaps, redefine "All Sources" search in Scarf to mean filter-what-you-see — the canonical type-to-filter UX users already expect on a list. Source-specific searches keep the CLI shell-out for full upstream search semantics on that registry. Implementation: - New `lastBrowseResults` cache populated on every successful `browseHub()`. Setter is `internal` so the test suite can seed without invoking the live CLI; out-of-module callers can still only read. - `searchHub()` now branches on `hubSource`. The "all" branch filters the cache via `localizedCaseInsensitiveContains` against name, description, and identifier, runs synchronously on the calling actor (UI invocations are already on MainActor) so the user sees the narrowed list without a render-tick gap. - If the cache is empty (search-before-browse), `browseHubThenFilter` performs one CLI fetch, populates the cache, then applies the filter — failure surfaces a "Search failed" banner instead of a silent empty state. - Source-specific search still shells out to `hermes skills search --source --limit 40`. Adds five regression tests covering name match, description match, case-insensitive folding, no-match message state, and the empty-query fallthrough to browse. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ViewModels/SkillsViewModel.swift | 107 +++++++++++++++++- .../SkillsViewModelHubFilterTests.swift | 98 ++++++++++++++++ 2 files changed, 201 insertions(+), 4 deletions(-) create mode 100644 scarf/Packages/ScarfCore/Tests/ScarfCoreTests/SkillsViewModelHubFilterTests.swift diff --git a/scarf/Packages/ScarfCore/Sources/ScarfCore/ViewModels/SkillsViewModel.swift b/scarf/Packages/ScarfCore/Sources/ScarfCore/ViewModels/SkillsViewModel.swift index 04f1196..a88f509 100644 --- a/scarf/Packages/ScarfCore/Sources/ScarfCore/ViewModels/SkillsViewModel.swift +++ b/scarf/Packages/ScarfCore/Sources/ScarfCore/ViewModels/SkillsViewModel.swift @@ -49,6 +49,18 @@ public final class SkillsViewModel { public var hubMessage: String? public var hubSource: String = "all" + /// Last successful `browseHub` payload, kept around so that the + /// "All Sources" search path can filter client-side (issue #79). + /// `hermes skills search` with no `--source` flag routes through + /// the centralized `hermes-index` source which can miss skills + /// that are visible in browse — we'd rather give the user the + /// canonical "type-to-filter" UX than chase Hermes's index gaps. + /// Source-specific searches still shell out to the CLI for full + /// upstream semantics. Setter is `internal` so the in-tree test + /// suite can seed the cache without invoking the live CLI; + /// out-of-module callers can still only read. + public internal(set) var lastBrowseResults: [HermesHubSkill] = [] + public let hubSources = ["all", "official", "skills-sh", "well-known", "github", "clawhub", "lobehub"] public var filteredCategories: [HermesSkillCategory] { @@ -260,14 +272,34 @@ public final class SkillsViewModel { browseHub() return } + let source = hubSource + let query = hubQuery + // Issue #79 — for "All Sources", filter the cached browse list + // client-side instead of shelling out. Hermes's all-source + // search routes through its centralized index which can miss + // skills (e.g. honcho) that browse surfaces from non-indexed + // registries. Specific-source searches keep the CLI path so + // power users still get full upstream search semantics. + if source == "all" { + if lastBrowseResults.isEmpty { + // No cache yet — kick off a browse, then filter on + // completion. The chained call lets the user type a + // query before ever clicking Browse. + browseHubThenFilter(query: query) + } else { + // Pure in-memory filter — runs synchronously on the + // calling actor (UI invocations are already on + // MainActor) so the user sees the narrowed list + // without a render-tick gap. + applyClientSideFilter(query: query, against: lastBrowseResults) + } + return + } isHubLoading = true let bin = context.paths.hermesBinary let xport = transport - let source = hubSource - let query = hubQuery Task.detached { [weak self] in - var args = ["skills", "search", query, "--limit", "40"] - if source != "all" { args += ["--source", source] } + let args = ["skills", "search", query, "--limit", "40", "--source", source] let result = Self.runHermes(executable: bin, args: args, transport: xport, timeout: 30) let parsed = HermesSkillsHubParser.parseHubList(result.output) await self?.finishBrowse( @@ -279,6 +311,66 @@ public final class SkillsViewModel { } } + /// Run a browse fetch and then immediately apply a client-side + /// filter. Used by `searchHub` when the user types into search + /// before any browse has cached results. + private func browseHubThenFilter(query: String) { + isHubLoading = true + let bin = context.paths.hermesBinary + let xport = transport + Task.detached { [weak self] in + let args = ["skills", "browse", "--size", "40"] + let result = Self.runHermes(executable: bin, args: args, transport: xport, timeout: 30) + let parsed = HermesSkillsHubParser.parseHubList(result.output) + await self?.finishBrowseThenFilter( + browseResults: parsed, + query: query, + exitCode: result.exitCode, + rawOutput: result.output + ) + } + } + + @MainActor + private func finishBrowseThenFilter( + browseResults: [HermesHubSkill], + query: String, + exitCode: Int32, + rawOutput: String + ) async { + if exitCode == 0 { + lastBrowseResults = browseResults + applyClientSideFilter(query: query, against: browseResults) + } else { + // Surface the underlying browse failure rather than a + // blank "no matches" state — the user typed a query, not + // a browse request, but the cache was empty so we tried. + isHubLoading = false + hubResults = [] + let detail = Self.firstSignificantLine(rawOutput) + hubMessage = detail.isEmpty + ? "Search failed (exit \(exitCode))" + : "Search failed: \(detail)" + } + } + + private func applyClientSideFilter(query: String, against pool: [HermesHubSkill]) { + let needle = query.trimmingCharacters(in: .whitespaces) + let matches: [HermesHubSkill] + if needle.isEmpty { + matches = pool + } else { + matches = pool.filter { skill in + skill.name.localizedCaseInsensitiveContains(needle) + || skill.description.localizedCaseInsensitiveContains(needle) + || skill.identifier.localizedCaseInsensitiveContains(needle) + } + } + isHubLoading = false + hubResults = matches + hubMessage = matches.isEmpty ? "No matches" : nil + } + public func installHubSkill(_ skill: HermesHubSkill) { isHubLoading = true hubMessage = "Installing \(skill.identifier)…" @@ -421,6 +513,13 @@ public final class SkillsViewModel { ) async { isHubLoading = false hubResults = results + // Cache the fresh browse payload so the "All Sources" search + // path can filter client-side (issue #79). Search results are + // not cached — they're already filtered by the user's query + // and would poison the filter pool. + if !isSearch && exitCode == 0 { + lastBrowseResults = results + } if results.isEmpty { if exitCode == 0 { hubMessage = isSearch ? "No matches" : "No results" diff --git a/scarf/Packages/ScarfCore/Tests/ScarfCoreTests/SkillsViewModelHubFilterTests.swift b/scarf/Packages/ScarfCore/Tests/ScarfCoreTests/SkillsViewModelHubFilterTests.swift new file mode 100644 index 0000000..1b0cd15 --- /dev/null +++ b/scarf/Packages/ScarfCore/Tests/ScarfCoreTests/SkillsViewModelHubFilterTests.swift @@ -0,0 +1,98 @@ +import Testing +import Foundation +@testable import ScarfCore + +/// Issue #79 regression. `searchHub()` with `hubSource == "all"` must +/// filter the cached browse list client-side (instead of shelling out +/// to `hermes skills search`, which routes through Hermes's +/// centralized index and can miss skills that browse aggregates from +/// non-indexed registries — `honcho` was the user-reported example). +/// +/// Source-specific searches keep the CLI path; that's not exercised +/// here because it requires a live `hermes` binary — the existing +/// HermesSkillsHubParser tests cover the parser side. +@Suite("SkillsViewModel hub filter") +@MainActor +struct SkillsViewModelHubFilterTests { + + private func makeViewModel() -> SkillsViewModel { + SkillsViewModel(context: .local) + } + + private let stubBrowse: [HermesHubSkill] = [ + HermesHubSkill( + identifier: "honcho", + name: "honcho", + description: "Memory provider for chat-scoped facts.", + source: "github" + ), + HermesHubSkill( + identifier: "1password", + name: "1password", + description: "Set up and use 1Password integration.", + source: "official" + ), + HermesHubSkill( + identifier: "spotify", + name: "spotify", + description: "Spotify skill — playback control via OAuth.", + source: "official" + ), + ] + + @Test func allSourcesFilterMatchesByName() { + let vm = makeViewModel() + vm.lastBrowseResults = stubBrowse + vm.hubSource = "all" + vm.hubQuery = "honcho" + vm.searchHub() + #expect(vm.hubResults.count == 1) + #expect(vm.hubResults.first?.identifier == "honcho") + #expect(vm.isHubLoading == false) + #expect(vm.hubMessage == nil) + } + + @Test func allSourcesFilterMatchesByDescription() { + let vm = makeViewModel() + vm.lastBrowseResults = stubBrowse + vm.hubSource = "all" + vm.hubQuery = "OAuth" + vm.searchHub() + #expect(vm.hubResults.count == 1) + #expect(vm.hubResults.first?.identifier == "spotify") + } + + @Test func allSourcesFilterIsCaseInsensitive() { + let vm = makeViewModel() + vm.lastBrowseResults = stubBrowse + vm.hubSource = "all" + vm.hubQuery = "HONCHO" + vm.searchHub() + #expect(vm.hubResults.count == 1) + #expect(vm.hubResults.first?.identifier == "honcho") + } + + @Test func allSourcesFilterEmptyMatchSetsMessage() { + let vm = makeViewModel() + vm.lastBrowseResults = stubBrowse + vm.hubSource = "all" + vm.hubQuery = "ringtone" + vm.searchHub() + #expect(vm.hubResults.isEmpty) + #expect(vm.hubMessage == "No matches") + } + + /// Empty query should fall through to `browseHub()`, which on + /// `.local` with no Hermes installed will set isHubLoading=true + /// and not block the test. We just assert the early-return guard + /// kicked in by checking the cache was untouched. + @Test func emptyQueryFallsThroughToBrowse() { + let vm = makeViewModel() + vm.lastBrowseResults = stubBrowse + vm.hubSource = "all" + vm.hubQuery = "" + let cacheBefore = vm.lastBrowseResults + vm.searchHub() + #expect(vm.lastBrowseResults == cacheBefore) + } +}