From eb468cfefcd752f04cedd4005f1c55f9cb2b37de Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 28 Sep 2026 14:36:42 +0300 Subject: [PATCH 1/2] feat(catalog): add real popularity ranking (Spec 110) --- ROADMAP.md | 1 + cmd/mcpproxy/catalog_cmd.go | 20 + docs/registries.md | 34 + internal/httpapi/catalog_test.go | 10 +- internal/registries/catalog.go | 270 ++++++- .../registries/catalog_popularity_test.go | 274 +++++++ internal/registries/catalog_test.go | 10 +- internal/registries/popularity.go | 75 ++ internal/registries/popularity_github.go | 712 ++++++++++++++++++ internal/registries/popularity_github_test.go | 535 +++++++++++++ internal/registries/popularity_key.go | 65 ++ internal/registries/popularity_key_test.go | 48 ++ .../popularity_review_fixes_test.go | 169 +++++ internal/registries/popularity_store.go | 115 +++ internal/registries/popularity_store_test.go | 173 +++++ internal/registries/rank_test.go | 46 ++ internal/registries/search.go | 10 + internal/registries/testhooks.go | 24 + internal/registries/types.go | 8 + internal/runtime/runtime.go | 64 +- scripts/test-api-e2e.sh | 6 + specs/110-catalog-popularity/plan.md | 103 +++ specs/110-catalog-popularity/spec.md | 95 +++ specs/110-catalog-popularity/tasks.md | 41 + 24 files changed, 2856 insertions(+), 52 deletions(-) create mode 100644 internal/registries/catalog_popularity_test.go create mode 100644 internal/registries/popularity.go create mode 100644 internal/registries/popularity_github.go create mode 100644 internal/registries/popularity_github_test.go create mode 100644 internal/registries/popularity_key.go create mode 100644 internal/registries/popularity_key_test.go create mode 100644 internal/registries/popularity_review_fixes_test.go create mode 100644 internal/registries/popularity_store.go create mode 100644 internal/registries/popularity_store_test.go create mode 100644 specs/110-catalog-popularity/plan.md create mode 100644 specs/110-catalog-popularity/spec.md create mode 100644 specs/110-catalog-popularity/tasks.md diff --git a/ROADMAP.md b/ROADMAP.md index ddf9d1930..c385a305a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1037,3 +1037,4 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | | [108-profiles-v3](./specs/108-profiles-v3/) | `in-flight` | 23/153 (15%) | | [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `in-flight` | 3/180 (2%) | +| [110-catalog-popularity](./specs/110-catalog-popularity/) | `in-flight` | 19/23 (83%) | diff --git a/cmd/mcpproxy/catalog_cmd.go b/cmd/mcpproxy/catalog_cmd.go index 4f0d30721..4d84d11db 100644 --- a/cmd/mcpproxy/catalog_cmd.go +++ b/cmd/mcpproxy/catalog_cmd.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "strings" + "sync" "github.com/spf13/cobra" @@ -14,6 +15,23 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" ) +// installMemoryPopularityProviderOnce guards installMemoryPopularityProvider +// so a CLI process that ends up calling it more than once (e.g. 'catalog +// show' after an in-process 'catalog search' fallback) installs a single +// provider rather than leaking one per call. +var installMemoryPopularityProviderOnce sync.Once + +// installMemoryPopularityProvider installs a memory-only (no bbolt store) +// popularity provider for the CLI's in-process paths (Spec 110 FR-010): the +// in-process 'catalog search' fallback (no daemon running) and 'catalog +// show'. The core daemon (internal/runtime) installs its own bbolt-backed +// provider instead — this one is never wired there. +func installMemoryPopularityProvider() { + installMemoryPopularityProviderOnce.Do(func() { + registries.SetPopularityProvider(registries.NewGitHubStarsProvider(registries.PopularityOptions{})) + }) +} + // printCatalogDeprecationNotice prints the FR-066 deprecation note to // stderr: 'registry search'/'registry add' remain as aliases (scripts must // not break), but point at their 'catalog' equivalent. @@ -124,6 +142,7 @@ func newCatalogShowCmd() *cobra.Command { ctx, cancel := registryContext() defer cancel() registries.SetRegistriesFromConfig(cfg) + installMemoryPopularityProvider() reg := registries.FindRegistry(source) if reg == nil { return outputError(clioutput.NewStructuredError(clioutput.ErrCodeServerNotFound, fmt.Sprintf("catalog source %q not found", source)). @@ -237,6 +256,7 @@ func catalogSearch(ctx context.Context, cfg *config.Config, q, source, tag strin // returns — which is also what makes it independently testable against a // fixture list (registries.SetRegistriesForTest). registries.SetRegistriesFromConfig(cfg) + installMemoryPopularityProvider() return catalogSearchInProcess(ctx, cfg, q, source, tag, limit) } diff --git a/docs/registries.md b/docs/registries.md index 3a15f1e46..7586dd5ba 100644 --- a/docs/registries.md +++ b/docs/registries.md @@ -219,6 +219,40 @@ Because every add surface (MCP, REST, CLI) funnels through the same keystone, a packages-only server is added as stdio and a remotes-only server as http identically across all surfaces. +## Catalog popularity signal + +The catalog's `Popular` landing section (and the popularity tiebreak in +search ranking) is backed by two real, source-native signals — no synthetic +scoring: + +- **GitHub stars** for any hit whose `source_code_url` resolves to a + `github.com//` URL (the official registry and reference + sources both carry one for almost every entry). Fetched from + `GET https://api.github.com/repos/{owner}/{repo}`, cached (with ETag + revalidation) for 24h, capped at 4 concurrent requests and a rolling + budget of 50 requests/hour without a token or 4,000/hour with one. +- **Docker Hub pull counts** (`pull_count` from the `docker-mcp-catalog` + registry's listing) map to `Installs`. Docker's own `star_count` is never + used — it lives on a different scale from GitHub stars (single digits vs. + tens of thousands) and the two are never summed or converted into one + another. + +Popularity fetching never blocks a search beyond a short bounded wait +(800ms by default): a cold miss is queued for a background fetch and simply +shows no stars on the current page, arriving on the next one. GitHub is +never listed in a search's `unavailable[]` — it is not a catalog source. + +**`MCPPROXY_GITHUB_TOKEN`** (not the generic `GITHUB_TOKEN`, which is +deliberately never read) raises the GitHub rate-limit budget from 50 to +4,000 requests/hour. Set it if your catalog has many distinct repositories +and stars are taking a while to fill in. + +**`MCPPROXY_CATALOG_POPULARITY=false`** (or `0`/`off`) disables all outbound +GitHub requests — useful for air-gapped or offline setups. Docker's +source-native install counts are unaffected either way, since they come from +the registry listing the Docker source already fetches, not from a separate +popularity call. + ## Adding a discovered server See [registry-add.md](features/registry-add.md). New servers are quarantined by diff --git a/internal/httpapi/catalog_test.go b/internal/httpapi/catalog_test.go index 1be5873e5..9ec1f596b 100644 --- a/internal/httpapi/catalog_test.go +++ b/internal/httpapi/catalog_test.go @@ -253,6 +253,14 @@ func TestCatalogSearch_AddedScopedForNonAdminUserContext(t *testing.T) { // TestCatalogSearch_EmptyQueryReturnsEmptyResults pins contracts/rest-api.md#catalog: // "Empty q → results: [], sections: {...}". Before this fix, Results was set // unconditionally to the ranked hit list even when Sections was populated. +// +// Spec 110 (T014): sections.popular is asserted EMPTY here, not populated — +// this fixture carries no popularity signal at all (no source_code_url, no +// Docker pull_count), and Popular now only ever shows hits with a known +// signal (FR-005/SC-002). Before Spec 110, Popular was just the ranked pool +// re-sorted by a popularity score that was always 0 for everyone, so it +// looked "populated" while actually carrying no real signal — the exact bug +// this spec fixes. func TestCatalogSearch_EmptyQueryReturnsEmptyResults(t *testing.T) { withCatalogFixtureRegistry(t) ctrl := &scopeController{cfg: scopeFixtureConfig(false), servers: catalogFixtureServers(), withManagement: true} @@ -270,7 +278,7 @@ func TestCatalogSearch_EmptyQueryReturnsEmptyResults(t *testing.T) { require.True(t, ok, "expected sections to be populated for an empty q") popular, ok := sections["popular"].([]interface{}) require.True(t, ok) - assert.NotEmpty(t, popular, "expected the fixture's entries in sections.popular") + assert.Empty(t, popular, "Spec 110 FR-005/SC-002: no popularity signal in this fixture -> sections.popular must be empty") } // TestCatalogSearch_SourceFilterAppliesBeforeTruncation is the regression for diff --git a/internal/registries/catalog.go b/internal/registries/catalog.go index 52abfdba4..c147cbd0f 100644 --- a/internal/registries/catalog.go +++ b/internal/registries/catalog.go @@ -110,6 +110,15 @@ type SearchOptions struct { // could silently drop a narrower source's real matches that simply // didn't survive the pre-filter truncation. Source string + + // PopularityWait bounds how long SearchAll waits (Spec 110 FR-007) for a + // background popularity fetch to land before returning. Zero — the Go + // zero value, i.e. left unset — means the default (800ms), matching + // every other *Timeout-shaped field in this struct (see SourceTimeout). + // A NEGATIVE value disables the wait entirely: SearchAll still enqueues + // the misses for background fetching, it just never blocks for them. + // The wait never outlives ctx. + PopularityWait time.Duration } const ( @@ -117,6 +126,11 @@ const ( catalogSectionCap = 12 defaultCatalogLimit = 10 maxCatalogLimit = 50 + + // defaultPopularityWait is SearchOptions.PopularityWait's zero-value + // default (Spec 110 FR-007): long enough for a warm cached/in-flight + // GitHub round trip, short enough to never meaningfully slow a search. + defaultPopularityWait = 800 * time.Millisecond ) // SearchAll fans SearchServers out to every enabled registry in parallel @@ -137,6 +151,21 @@ func SearchAll(ctx context.Context, q, tag string, limit int, opts SearchOptions limit = maxCatalogLimit } + empty := strings.TrimSpace(q) == "" + + // Spec 110 FR-005 (zcode review round 1, finding 2): an empty q fans out + // each source at the per-source MAXIMUM, not the caller's `limit`, so the + // section pool (Official/Popular) is as wide as each source will give — + // otherwise a `limit` of 10 would starve Popular of anything beyond the + // first 10 official hits before popularity ever gets a say. The final + // `results` list is still truncated to `limit` below. A non-empty q keeps + // fetching exactly `limit` per source (unchanged), since it has no + // sections to populate. + fetchLimit := limit + if empty { + fetchLimit = maxCatalogLimit + } + sources := ListRegistries() type sourceOutcome struct { @@ -154,7 +183,7 @@ func SearchAll(ctx context.Context, q, tag string, limit int, opts SearchOptions sctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() - entries, err := SearchServers(sctx, reg.ID, tag, q, limit, nil) + entries, err := SearchServers(sctx, reg.ID, tag, q, fetchLimit, nil) if err != nil { reason := err.Error() if sctx.Err() != nil { @@ -174,6 +203,12 @@ func SearchAll(ctx context.Context, q, tag string, limit int, opts SearchOptions wg.Wait() seen := make(map[string]bool) + // all is built and kept in MERGE order (registry list order, then each + // source's native order) until buildSections has taken its snapshot + // below — Spec 110 FR-005 (zcode review finding 1): Official must come + // from a copy taken BEFORE any Rank sort, or it silently degrades back + // into popularity order on an all-official default install (the exact + // bug this spec fixes). var all []CatalogHit var unavailable []SourceError for _, outcome := range outcomes { @@ -195,22 +230,22 @@ func SearchAll(ctx context.Context, q, tag string, limit int, opts SearchOptions all = filterHitsBySource(all, opts.Source) } - sort.SliceStable(all, func(i, j int) bool { return Rank(all[i], all[j], q) }) - sort.SliceStable(unavailable, func(i, j int) bool { return unavailable[i].Source < unavailable[j].Source }) + // Spec 110 FR-002/007: resolve popularity (cache hits immediately, misses + // queued for a bounded background wait) BEFORE ranking, so both the + // Rank tiebreak (US2) and the Popular section see up-to-date signal. This + // mutates each hit's Popularity in place but does not reorder `all`. + resolvePopularity(ctx, all, q, popularityWait(opts.PopularityWait)) - // Sections are built from the FULL ranked list, before truncation to - // `limit` — review round 4 F-F: limit defaults to 10 while - // catalogSectionCap is 12, so building sections AFTER truncating to - // `limit` meant Popular could never reach its own cap at the documented - // default, and any genuinely popular hit ranked just outside the top - // `limit` (e.g. a non-official source that lost the official-first - // sort) was silently excluded from Popular regardless of how popular it - // actually was. var sections *CatalogSections - if strings.TrimSpace(q) == "" { - sections = buildSections(all) + if empty { + // Built from `all` in its current MERGE order, before the Rank sort + // below (FR-005). + sections = buildSections(all, q) } + sort.SliceStable(all, func(i, j int) bool { return Rank(all[i], all[j], q) }) + sort.SliceStable(unavailable, func(i, j int) bool { return unavailable[i].Source < unavailable[j].Source }) + if len(all) > limit { all = all[:limit] } @@ -218,6 +253,64 @@ func SearchAll(ctx context.Context, q, tag string, limit int, opts SearchOptions return all, sections, unavailable } +// popularityWait resolves SearchOptions.PopularityWait's documented +// convention: zero (unset) means the 800ms default; negative disables the +// wait entirely (fetches are still enqueued, SearchAll just never blocks on +// them). +func popularityWait(configured time.Duration) time.Duration { + switch { + case configured < 0: + return 0 + case configured == 0: + return defaultPopularityWait + default: + return configured + } +} + +// resolvePopularity is Spec 110's FR-007 hook: it asks the installed +// PopularityProvider (if any) to resolve every hit's GitHub repo key, waits +// up to `wait` for the background fetch to land, and re-applies whatever is +// now cached. A nil provider (no popularity wiring — e.g. most tests) is a +// fast no-op. +func resolvePopularity(ctx context.Context, hits []CatalogHit, q string, wait time.Duration) { + provider := getPopularityProvider() + if provider == nil { + return + } + + // FR-009(e): enqueue misses in the order SearchAll ranks them, so the + // most relevant ones are fetched first. This priority copy is throwaway — + // it never replaces `hits`' own order, which Official's merge-order + // requirement (FR-005) depends on. + priority := append([]CatalogHit(nil), hits...) + sort.SliceStable(priority, func(i, j int) bool { return Rank(priority[i], priority[j], q) }) + + seen := make(map[string]bool, len(priority)) + keys := make([]string, 0, len(priority)) + for _, h := range priority { + key, ok := GitHubRepoKey(h.Entry.SourceCodeURL) + if !ok || seen[key] { + continue + } + seen[key] = true + // Only Stale/Absent need a fetch (FR-008): a Fresh positive or a + // still-fresh Negative (confirmed 404/451, or an error entry backing + // off within its own shorter TTL) is left alone. + if _, state := provider.Lookup(key); state == LookupStale || state == LookupAbsent { + keys = append(keys, key) + } + } + + provider.Resolve(ctx, keys, wait) + + // Re-apply lookups: any key that landed during the bounded wait now shows + // its stars; everything else is unchanged. + for i := range hits { + applyCachedStars(&hits[i]) + } +} + // filterHitsBySource keeps only the hits from one catalog source, applied // before ranking/truncation (see SearchOptions.Source). func filterHitsBySource(hits []CatalogHit, source string) []CatalogHit { @@ -243,7 +336,7 @@ func BuildCatalogHit(reg *RegistryEntry, entry ServerEntry) CatalogHit { if title == "" { title = entry.ID } - return CatalogHit{ + hit := CatalogHit{ Entry: entry, Source: reg.ID, Title: title, @@ -251,6 +344,62 @@ func BuildCatalogHit(reg *RegistryEntry, entry ServerEntry) CatalogHit { Verified: official, Official: official, } + // FR-001/FR-002: copy the source-native signal (e.g. Docker pull_count) + // first, then layer in GitHub stars from the provider's cache only — no + // network call, so this stays synchronous. + if entry.Popularity != nil { + p := *entry.Popularity + hit.Popularity = &p + } + applyCachedStars(&hit) + return hit +} + +// applyCachedStars fills in hit.Popularity.Stars from the installed +// PopularityProvider's cache ONLY (Spec 110 FR-002): no I/O, so both +// BuildCatalogHit and SearchAll's post-Resolve re-apply can call this freely. +// A nil provider, an entry with no GitHub-shaped SourceCodeURL, or a +// non-displayable lookup state (Absent/Negative) leave the hit unchanged. +func applyCachedStars(hit *CatalogHit) { + key, ok := GitHubRepoKey(hit.Entry.SourceCodeURL) + if !ok { + return + } + provider := getPopularityProvider() + if provider == nil { + return + } + stars, state := provider.Lookup(key) + if state != LookupFresh && state != LookupStale { + // No displayable stars now. The hit may still carry stars from an + // earlier apply (BuildCatalogHit saw them Stale, then the refresh + // during Resolve's wait came back 404/451), so fall back to the + // source-native value instead of leaving the old count in place. + resetToSourceNativeStars(hit) + return + } + if hit.Popularity == nil { + hit.Popularity = &Popularity{} + } + s := stars + hit.Popularity.Stars = &s +} + +// resetToSourceNativeStars drops provider-supplied stars from hit, keeping +// only what the source itself reported (entry.Popularity). +func resetToSourceNativeStars(hit *CatalogHit) { + if hit.Popularity == nil { + return + } + var native *int + if hit.Entry.Popularity != nil && hit.Entry.Popularity.Stars != nil { + v := *hit.Entry.Popularity.Stars + native = &v + } + hit.Popularity.Stars = native + if hit.Popularity.Stars == nil && hit.Popularity.Installs == nil { + hit.Popularity = nil + } } // derivePublisher extracts a display publisher from an official-protocol @@ -280,8 +429,8 @@ func Rank(a, b CatalogHit, q string) bool { if a.Verified != b.Verified { return a.Verified } - if ap, bp := popularityScore(a.Popularity), popularityScore(b.Popularity); ap != bp { - return ap > bp + if !popularityEqual(a.Popularity, b.Popularity) { + return morePopular(a.Popularity, b.Popularity) } if ar, br := relevanceScore(a, q), relevanceScore(b, q); ar != br { return ar > br @@ -303,18 +452,40 @@ func catalogTitle(h CatalogHit) string { return h.Entry.ID } -func popularityScore(p *Popularity) int { +// popularityKey extracts the (stars, installs) tuple a Popularity compares +// on, with a nil pointer or nil field reading as 0 (Spec 110 FR-004). +func popularityKey(p *Popularity) (stars, installs int) { if p == nil { - return 0 + return 0, 0 } - score := 0 if p.Stars != nil { - score += *p.Stars + stars = *p.Stars } if p.Installs != nil { - score += *p.Installs + installs = *p.Installs } - return score + return stars, installs +} + +// morePopular reports whether a ranks strictly ABOVE b: stars desc, then +// installs desc (FR-004). Stars and installs are never summed or converted +// into one another — a lexicographic tuple compare, not a score. +func morePopular(a, b *Popularity) bool { + as, ai := popularityKey(a) + bs, bi := popularityKey(b) + if as != bs { + return as > bs + } + return ai > bi +} + +// popularityEqual reports whether a and b compare equal under morePopular's +// ordering (both keys tie), the signal Rank and buildSections use to decide +// whether to fall through to the next sort key. +func popularityEqual(a, b *Popularity) bool { + as, ai := popularityKey(a) + bs, bi := popularityKey(b) + return as == bs && ai == bi } // relevanceScore is a simple, deterministic token-match count of q against @@ -344,23 +515,58 @@ func relevanceScore(h CatalogHit, q string) int { return score } -// buildSections splits the already-ranked merged hits into the empty-query -// landing sections (FR-060): official-source hits (in rank order) and the -// top-popularity hits across every source, each capped at 12. -func buildSections(ranked []CatalogHit) *CatalogSections { +// buildSections splits the merged, de-duplicated, source-filtered pool +// (BEFORE limit truncation and BEFORE any Rank sort) into the empty-query +// landing sections (Spec 110 FR-005, amending Spec 109 FR-060): +// +// - Official: official-source hits in `pool`'s own MERGE (source-native) +// order — registry-list order, then each source's native order. Pool +// MUST NOT have been Rank-sorted yet: on an all-official default install +// every hit ties on Official/Verified, so Rank order IS popularity order, +// and building Official from a ranked pool silently reintroduces the bug +// this spec fixes. Capped at 12. +// - Popular: hits with a known signal (stars>0 ∨ installs>0), sorted by +// popularity (FR-004) then Rank as a tiebreak, at most one per GitHub +// repo key (a monorepo's shared star count keeps only the first by Rank — +// spec.md edge cases), capped at 12. +func buildSections(pool []CatalogHit, q string) *CatalogSections { sections := &CatalogSections{Official: []CatalogHit{}, Popular: []CatalogHit{}} - for _, h := range ranked { + + for _, h := range pool { if h.Official && len(sections.Official) < catalogSectionCap { sections.Official = append(sections.Official, h) } } - byPopularity := append([]CatalogHit(nil), ranked...) - sort.SliceStable(byPopularity, func(i, j int) bool { - return popularityScore(byPopularity[i].Popularity) > popularityScore(byPopularity[j].Popularity) + candidates := make([]CatalogHit, 0, len(pool)) + for _, h := range pool { + if stars, installs := popularityKey(h.Popularity); stars > 0 || installs > 0 { + candidates = append(candidates, h) + } + } + sort.SliceStable(candidates, func(i, j int) bool { + if !popularityEqual(candidates[i].Popularity, candidates[j].Popularity) { + return morePopular(candidates[i].Popularity, candidates[j].Popularity) + } + return Rank(candidates[i], candidates[j], q) }) - for i := 0; i < len(byPopularity) && i < catalogSectionCap; i++ { - sections.Popular = append(sections.Popular, byPopularity[i]) + + seenRepo := make(map[string]bool, len(candidates)) + for _, h := range candidates { + if len(sections.Popular) >= catalogSectionCap { + break + } + dedupKey, hasRepo := GitHubRepoKey(h.Entry.SourceCodeURL) + if !hasRepo { + // No GitHub repo to dedup against (e.g. a Docker-only pull-count + // hit) — (source, id) already made this unique within `pool`. + dedupKey = "no-repo:" + h.Source + "\x00" + h.Entry.ID + } + if seenRepo[dedupKey] { + continue + } + seenRepo[dedupKey] = true + sections.Popular = append(sections.Popular, h) } return sections } diff --git a/internal/registries/catalog_popularity_test.go b/internal/registries/catalog_popularity_test.go new file mode 100644 index 000000000..c3c7839d2 --- /dev/null +++ b/internal/registries/catalog_popularity_test.go @@ -0,0 +1,274 @@ +package registries + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" +) + +// stubPopularityProvider is a deterministic, non-networked PopularityProvider +// for tests that need known star counts without any HTTP stub or timing +// dependency (T010/T012 — SC-001/SC-002). Resolve is a no-op: everything the +// test cares about is pre-seeded and already "fresh". +type stubPopularityProvider struct { + mu sync.Mutex + stars map[string]int +} + +func (s *stubPopularityProvider) Lookup(key string) (int, LookupState) { + s.mu.Lock() + defer s.mu.Unlock() + if v, ok := s.stars[key]; ok { + return v, LookupFresh + } + return 0, LookupAbsent +} + +func (s *stubPopularityProvider) Resolve(context.Context, []string, time.Duration) {} + +var _ PopularityProvider = (*stubPopularityProvider)(nil) + +func idsOf(hits []CatalogHit) []string { + out := make([]string, len(hits)) + for i, h := range hits { + out[i] = h.Entry.ID + } + return out +} + +// --- T010: BuildCatalogHit --------------------------------------------------- + +func TestBuildCatalogHit_CopiesPopularityAndAddsCacheOnlyStars(t *testing.T) { + reg := RegistryEntry{ID: "docker", Name: "Docker"} + installs := 42 + entry := ServerEntry{ + ID: "tool", Name: "Tool", + SourceCodeURL: "https://github.com/org/tool", + Popularity: &Popularity{Installs: &installs}, + } + + restore := SetPopularityProviderForTest(nil) + hit := BuildCatalogHit(®, entry) + restore() + if hit.Popularity == nil || hit.Popularity.Installs == nil || *hit.Popularity.Installs != 42 { + t.Fatalf("expected Installs to carry through with no provider installed, got %+v", hit.Popularity) + } + if hit.Popularity.Stars != nil { + t.Fatalf("expected no Stars with no provider installed, got %+v", hit.Popularity) + } + + stub := &stubPopularityProvider{stars: map[string]int{"org/tool": 321}} + restore2 := SetPopularityProviderForTest(stub) + hit2 := BuildCatalogHit(®, entry) + restore2() + if hit2.Popularity == nil || hit2.Popularity.Stars == nil || *hit2.Popularity.Stars != 321 { + t.Fatalf("expected Stars=321 from the cache-only lookup, got %+v", hit2.Popularity) + } + if hit2.Popularity.Installs == nil || *hit2.Popularity.Installs != 42 { + t.Fatalf("expected Installs to still carry through alongside Stars, got %+v", hit2.Popularity) + } +} + +// --- T012 / SC-001 ----------------------------------------------------------- + +// TestSearchAll_SC001_PopularDiffersFromOfficial pins SC-001: 14 official +// hits in source order (repo-14 has by far the most stars but sits outside +// Official's first-12 cap), plus one non-official Docker hit with pulls. +// Popular must differ from Official, Popular[0] must be the most-starred +// hit, and Popular must contain a hit Official does not. +func TestSearchAll_SC001_PopularDiffersFromOfficial(t *testing.T) { + entries := make([]string, 0, 14) + for i := 1; i <= 14; i++ { + entries = append(entries, fmt.Sprintf( + `{"id":"repo-%d","name":"Repo %d","source_code_url":"https://github.com/org/repo-%d"}`, i, i, i)) + } + officialSrv := jsonServer(t, "["+strings.Join(entries, ",")+"]") + dockerSrv := jsonServer(t, `{"results":[{"name":"popular-tool","pull_count":5000000,"short_description":"d"}]}`) + + withTestRegistries(t, []RegistryEntry{ + {ID: "official", Name: "Official", ServersURL: officialSrv.URL, Provenance: "official"}, + {ID: "docker", Name: "Docker", ServersURL: dockerSrv.URL, Protocol: protocolDocker}, + }) + + stub := &stubPopularityProvider{stars: map[string]int{ + "org/repo-14": 99999, // outside Official's first 12; the standout count + "org/repo-1": 500, + "org/repo-2": 10, + }} + restore := SetPopularityProviderForTest(stub) + defer restore() + + _, sections, unavailable := SearchAll(context.Background(), "", "", 10, SearchOptions{}) + if len(unavailable) != 0 { + t.Fatalf("expected no unavailable sources, got %+v", unavailable) + } + if sections == nil { + t.Fatal("expected sections for an empty query") + } + + if len(sections.Official) != 12 { + t.Fatalf("expected Official capped at 12, got %d: %+v", len(sections.Official), idsOf(sections.Official)) + } + for i, h := range sections.Official { + want := fmt.Sprintf("repo-%d", i+1) + if h.Entry.ID != want { + t.Fatalf("expected Official in SOURCE order — Official[%d]=%s, got %s (full: %+v)", i, want, h.Entry.ID, idsOf(sections.Official)) + } + } + + if len(sections.Popular) == 0 { + t.Fatal("expected a non-empty Popular section") + } + if sections.Popular[0].Entry.ID != "repo-14" { + t.Fatalf("expected Popular[0] to be the most-starred hit (repo-14), got %s", sections.Popular[0].Entry.ID) + } + + officialIDs := make(map[string]bool, len(sections.Official)) + for _, h := range sections.Official { + officialIDs[h.Entry.ID] = true + } + foundOutsideOfficial := false + foundDocker := false + for _, h := range sections.Popular { + if !officialIDs[h.Entry.ID] { + foundOutsideOfficial = true + } + if h.Source == "docker" { + foundDocker = true + } + } + if !foundOutsideOfficial { + t.Fatalf("expected Popular to contain a hit not in Official, got %+v", idsOf(sections.Popular)) + } + if !foundDocker { + t.Fatalf("expected the Docker install-count hit to blend into Popular, got %+v", idsOf(sections.Popular)) + } + if strings.Join(idsOf(sections.Official), ",") == strings.Join(idsOf(sections.Popular), ",") { + t.Fatal("expected Popular to differ from Official") + } +} + +// TestSearchAll_SC002_NoSignalMeansEmptyPopular pins SC-002: with no +// popularity known for any hit, Popular is empty (never padded with +// zero-popularity hits) and Official is unaffected. +func TestSearchAll_SC002_NoSignalMeansEmptyPopular(t *testing.T) { + officialSrv := jsonServer(t, `[{"id":"a","name":"A"},{"id":"b","name":"B"}]`) + withTestRegistries(t, []RegistryEntry{ + {ID: "official", Name: "Official", ServersURL: officialSrv.URL, Provenance: "official"}, + }) + + restore := SetPopularityProviderForTest(&stubPopularityProvider{stars: map[string]int{}}) + defer restore() + + _, sections, _ := SearchAll(context.Background(), "", "", 10, SearchOptions{}) + if sections == nil { + t.Fatal("expected sections for an empty query") + } + if len(sections.Popular) != 0 { + t.Fatalf("expected an empty Popular section with no signal, got %+v", idsOf(sections.Popular)) + } + if len(sections.Official) != 2 { + t.Fatalf("expected Official unchanged (2 hits), got %+v", idsOf(sections.Official)) + } +} + +// TestSearchAll_NoProviderInstalledMeansEmptyPopular is SC-002's "GitHub +// unreachable / no popularity wiring at all" variant: a nil provider (the +// package default, and what most tests run with) must behave the same as an +// installed-but-empty one. +func TestSearchAll_NoProviderInstalledMeansEmptyPopular(t *testing.T) { + officialSrv := jsonServer(t, `[{"id":"a","name":"A","source_code_url":"https://github.com/org/a"}]`) + withTestRegistries(t, []RegistryEntry{ + {ID: "official", Name: "Official", ServersURL: officialSrv.URL, Provenance: "official"}, + }) + + restore := SetPopularityProviderForTest(nil) + defer restore() + + _, sections, _ := SearchAll(context.Background(), "", "", 10, SearchOptions{}) + if sections == nil || len(sections.Popular) != 0 { + t.Fatalf("expected an empty Popular section with no provider installed, got %+v", sections) + } +} + +// --- T013 / SC-003 ------------------------------------------------------------ + +// TestSearchAll_SC003_ReturnsWithinPopularityWaitBudget pins SC-003: SearchAll +// returns within PopularityWait (+ generous slack for CI jitter) even when +// the GitHub fetch is slow, unavailable[] stays untouched (popularity is +// never a catalog source), and a second call after the stub answers carries +// the stars. The spec's illustrative "sleeps 5s" is shortened here (a fast, +// deterministic stand-in for "slower than the wait") so the suite stays +// fast; the property under test — SearchAll's return time is bounded by the +// wait, not by the upstream's latency — is unaffected by the absolute +// duration chosen. +func TestSearchAll_SC003_ReturnsWithinPopularityWaitBudget(t *testing.T) { + officialSrv := jsonServer(t, `[{"id":"a","name":"A","source_code_url":"https://github.com/org/repo"}]`) + withTestRegistries(t, []RegistryEntry{ + {ID: "official", Name: "Official", ServersURL: officialSrv.URL, Provenance: "official"}, + }) + + block := make(chan struct{}) + var once sync.Once + closeBlock := func() { once.Do(func() { close(block) }) } + slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-block: + case <-r.Context().Done(): + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 777}`)) + })) + defer func() { closeBlock(); slow.Close() }() + + restoreBase := SetGitHubAPIBaseForTest(slow.URL) + defer restoreBase() + provider := NewGitHubStarsProvider(PopularityOptions{}) + defer provider.Close() + defer SetPopularityProviderForTest(provider)() + + wait := 80 * time.Millisecond + start := time.Now() + hits, _, unavailable := SearchAll(context.Background(), "", "", 10, SearchOptions{PopularityWait: wait}) + elapsed := time.Since(start) + + if len(unavailable) != 0 { + t.Fatalf("expected unavailable[] untouched by a slow popularity fetch, got %+v", unavailable) + } + + slack := 400 * time.Millisecond // generous: CI jitter, not the property under test + if elapsed > wait+slack { + t.Fatalf("expected SearchAll to return within wait(%s)+slack(%s), took %s", wait, slack, elapsed) + } + for _, h := range hits { + if h.Popularity != nil && h.Popularity.Stars != nil { + t.Fatalf("expected no stars yet on the first call (stub still blocked), got %+v", *h.Popularity.Stars) + } + } + + closeBlock() // let the slow stub answer + + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if _, state := provider.Lookup("org/repo"); state == LookupFresh { + break + } + time.Sleep(10 * time.Millisecond) + } + + hits2, _, _ := SearchAll(context.Background(), "", "", 10, SearchOptions{PopularityWait: wait}) + found := false + for _, h := range hits2 { + if h.Popularity != nil && h.Popularity.Stars != nil && *h.Popularity.Stars == 777 { + found = true + } + } + if !found { + t.Fatalf("expected the second SearchAll call to carry the resolved stars, got %+v", hits2) + } +} diff --git a/internal/registries/catalog_test.go b/internal/registries/catalog_test.go index 783c5d316..503ee14a0 100644 --- a/internal/registries/catalog_test.go +++ b/internal/registries/catalog_test.go @@ -160,7 +160,7 @@ func TestSearchAll_PopularSectionNotLimitedByPageSize(t *testing.T) { entriesFor := func(prefix string) string { var entries []string for i := 0; i < 10; i++ { - entries = append(entries, fmt.Sprintf(`{"id":"%s%02d","name":"Server %s%02d"}`, prefix, i, prefix, i)) + entries = append(entries, fmt.Sprintf(`{"id":"%s%02d","name":"Server %s%02d","source_code_url":"https://github.com/org/%s%02d"}`, prefix, i, prefix, i, prefix, i)) } return "[" + strings.Join(entries, ",") + "]" } @@ -170,6 +170,14 @@ func TestSearchAll_PopularSectionNotLimitedByPageSize(t *testing.T) { {ID: "src1", Name: "Src1", ServersURL: src1.URL}, {ID: "src2", Name: "Src2", ServersURL: src2.URL}, }) + stars := make(map[string]int, 20) + for _, prefix := range []string{"a", "b"} { + for i := 0; i < 10; i++ { + stars[fmt.Sprintf("org/%s%02d", prefix, i)] = i + 1 + } + } + restore := SetPopularityProviderForTest(&stubPopularityProvider{stars: stars}) + defer restore() hits, sections, _ := SearchAll(context.Background(), "", "", 10, SearchOptions{}) if len(hits) != 10 { diff --git a/internal/registries/popularity.go b/internal/registries/popularity.go new file mode 100644 index 000000000..6475c816d --- /dev/null +++ b/internal/registries/popularity.go @@ -0,0 +1,75 @@ +// Spec 110 (catalog popularity signal): a PopularityProvider resolves GitHub +// star counts for repo keys (GitHubRepoKey), backed by a cached, +// rate-limited fetcher (githubStarsProvider is the production +// implementation — see popularity_github.go). It is installed process-wide +// via SetPopularityProvider (FR-010) so BuildCatalogHit and SearchAll can +// reach it without threading a dependency through every call site. +package registries + +import ( + "context" + "sync" + "time" +) + +// LookupState is PopularityProvider.Lookup's classification of a cached +// entry (Spec 110 FR-008, zcode review round 1 finding 7): +// +// - LookupFresh: a positive signal (stars) within its TTL. Display it; +// no fetch needed. +// - LookupStale: a positive signal past its TTL. Still display it +// (stale-while-revalidate), but it needs a refresh. +// - LookupNegative: no signal to display, and still within its TTL — +// either a confirmed 404/451, or a transient-error entry backing off +// within its own (shorter) TTL. Not re-fetched yet either way. +// - LookupAbsent: no signal, and free to (re)fetch now — never seen, or +// past a negative/error entry's TTL. +type LookupState int + +const ( + LookupAbsent LookupState = iota + LookupFresh + LookupStale + LookupNegative +) + +// PopularityProvider is the seam BuildCatalogHit and SearchAll use to read +// and refresh a GitHub repo's star count. +type PopularityProvider interface { + // Lookup returns the cached star count for key without any I/O. See + // LookupState for what each state means; only Fresh and Stale carry a + // meaningful `stars` value. + Lookup(key string) (stars int, state LookupState) + + // Resolve enqueues any of keys that are Stale or Absent for a background + // fetch (de-duplicated against what's already queued/in-flight) and + // waits up to wait — bounded by ctx — for them to land. Fetching + // continues after Resolve returns even if the wait/ctx expired first. + // Resolve returns immediately, without enqueueing anything, when the + // provider is breaker-paused, its rolling budget is exhausted, or none of + // the requested keys were admitted (e.g. all already fresh, or the queue + // was full) — see popularity_github.go. + Resolve(ctx context.Context, keys []string, wait time.Duration) +} + +var ( + popularityProviderMu sync.RWMutex + popularityProviderVal PopularityProvider +) + +// SetPopularityProvider installs the process-wide popularity provider +// (FR-010). nil disables all popularity lookups: BuildCatalogHit adds no +// stars and SearchAll enqueues no fetches — Docker's source-native Installs +// signal is unaffected either way (it never goes through this seam). +func SetPopularityProvider(p PopularityProvider) { + popularityProviderMu.Lock() + defer popularityProviderMu.Unlock() + popularityProviderVal = p +} + +// getPopularityProvider returns the currently installed provider, or nil. +func getPopularityProvider() PopularityProvider { + popularityProviderMu.RLock() + defer popularityProviderMu.RUnlock() + return popularityProviderVal +} diff --git a/internal/registries/popularity_github.go b/internal/registries/popularity_github.go new file mode 100644 index 000000000..0ad691b67 --- /dev/null +++ b/internal/registries/popularity_github.go @@ -0,0 +1,712 @@ +package registries + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "os" + "strconv" + "strings" + "sync" + "sync/atomic" + "time" + + "go.etcd.io/bbolt" + "go.uber.org/zap" +) + +const ( + githubAPIBaseURLDefault = "https://api.github.com" + + githubMaxConcurrentFetches = 4 // FR-009(a) + githubQueueCap = 256 // FR-009(b) + githubMaxCacheKeys = 5000 // FR-008 + githubRequestTimeout = 10 * time.Second + githubMaxBodyBytes = 1 << 20 // 1 MiB (FR-006) + + githubFreshTTL = 24 * time.Hour // FR-008: 200/304 and 404/451 + githubErrorTTL = 1 * time.Hour // FR-008: any other error + githubBreakerPause = 60 * time.Second + + githubRateLimitUnauth = 50 // FR-009(c): requests/hour without a token + githubRateLimitAuth = 4000 // FR-009(c): requests/hour with a token + + githubLowRemainingThreshold = 5 // FR-009(d) +) + +var ( + errGitHubBodyTooLarge = errors.New("github repo response exceeds the body cap") + errGitHubNoStargazers = errors.New("github repo response has no stargazers_count") +) + +// githubAPIBaseOverride lets SetGitHubAPIBaseForTest point new providers at +// an httptest.Server; production providers always read the compile-time +// default (FR-006: "overridable only by a test hook"). +var githubAPIBaseOverride atomic.Pointer[string] + +func currentGitHubAPIBase() string { + if p := githubAPIBaseOverride.Load(); p != nil { + return *p + } + return githubAPIBaseURLDefault +} + +// starsEntry is the cached record for one GitHub repo key (plan.md data +// model). Persisted as JSON in popularityBucketName. +type starsEntry struct { + Stars int `json:"stars"` + ETag string `json:"etag,omitempty"` + FetchedAt time.Time `json:"fetched_at"` + Status int `json:"status"` // 200/304 ok, 404/451 negative, else the last error status (0 = transport error) +} + +func (e *starsEntry) ok() bool { + return e.Status == http.StatusOK || e.Status == http.StatusNotModified +} + +func (e *starsEntry) negative() bool { + return e.Status == http.StatusNotFound || e.Status == http.StatusUnavailableForLegalReasons +} + +// ttl is how long e stays fresh (FR-008): 24h for a confirmed answer +// (positive OR negative), 1h for anything else (transport error, 5xx, 403, +// 429 — the breaker, not this TTL, is what actually throttles those). +func (e *starsEntry) ttl() time.Duration { + if e.ok() || e.negative() { + return githubFreshTTL + } + return githubErrorTTL +} + +// PopularityOptions configures NewGitHubStarsProvider. +type PopularityOptions struct { + // DB is the bbolt database to persist the cache to. nil means memory + // only (the CLI in-process fallback — plan.md wiring table). + DB *bbolt.DB + // Logger receives best-effort diagnostics (store errors, etc). A nil + // Logger falls back to zap.NewNop(). + Logger *zap.Logger +} + +// rollingBudget is a simple sliding-window request counter (FR-009c): at +// most `limit` `allow` calls may return true within any trailing `window`. +type rollingBudget struct { + mu sync.Mutex + limit int + window time.Duration + hits []time.Time +} + +func newRollingBudget(limit int) *rollingBudget { + return &rollingBudget{limit: limit, window: time.Hour} +} + +func (b *rollingBudget) prune(now time.Time) { + cutoff := now.Add(-b.window) + kept := b.hits[:0] + for _, t := range b.hits { + if t.After(cutoff) { + kept = append(kept, t) + } + } + b.hits = kept +} + +// hasCapacity reports whether a request could be spent right now, without +// consuming any budget (a peek, used by Resolve's fast bail-out). +func (b *rollingBudget) hasCapacity(now time.Time) bool { + b.mu.Lock() + defer b.mu.Unlock() + b.prune(now) + return len(b.hits) < b.limit +} + +// allow is the authoritative, budget-consuming check made by a worker right +// before it actually issues a request. +func (b *rollingBudget) allow(now time.Time) bool { + b.mu.Lock() + defer b.mu.Unlock() + b.prune(now) + if len(b.hits) >= b.limit { + return false + } + b.hits = append(b.hits, now) + return true +} + +// githubStarsProvider is the production PopularityProvider (plan.md data +// model): a cached, rate-limited GitHub stargazers_count fetcher behind a +// fixed 4-worker pool. +type githubStarsProvider struct { + mu sync.Mutex + entries map[string]*starsEntry + store *popularityStore // nil = memory only + + queue chan string + queued map[string]chan struct{} // in-flight+queued dedup; closed on completion + + logger *zap.Logger + token string + disabled bool // FR-011 kill switch, read once at construction + baseURL string + + now func() time.Time // injected clock (tests) + + budget *rollingBudget + pausedMu sync.Mutex + pausedTil time.Time + + // baseCtx is the provider's OWN context for background fetches — never + // the caller's Resolve ctx (zcode review finding 3). Cancelled by Close. + baseCtx context.Context + cancel context.CancelFunc + wg sync.WaitGroup + + closeOnce sync.Once + // closed is set by Close. A closed provider may still be the installed + // process-wide one (a search racing shutdown, or a test process that + // outlives a runtime), so Resolve must stop admitting keys that no + // worker will ever drain. + closed atomic.Bool +} + +var _ PopularityProvider = (*githubStarsProvider)(nil) + +// popularityFetchesEnabled reads the FR-011 kill switch. Read once, inside +// NewGitHubStarsProvider (zcode review finding 9: "the switch is read inside +// the provider constructor, which then returns a no-op provider, so the +// runtime and CLI call sites both honour it"). +func popularityFetchesEnabled() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv("MCPPROXY_CATALOG_POPULARITY"))) { + case "false", "0", "off": + return false + default: + return true + } +} + +// NewGitHubStarsProvider constructs the production PopularityProvider. When +// the FR-011 kill switch is set, the returned provider still answers Lookup +// from whatever is already cached (e.g. a bbolt store from before the switch +// was flipped) but starts no worker goroutines and Resolve is a no-op — a +// caller never needs to branch on the switch itself. +func NewGitHubStarsProvider(opts PopularityOptions) *githubStarsProvider { + logger := opts.Logger + if logger == nil { + logger = zap.NewNop() + } + + token := os.Getenv("MCPPROXY_GITHUB_TOKEN") // FR-011: never the generic GITHUB_TOKEN + limit := githubRateLimitUnauth + if token != "" { + limit = githubRateLimitAuth + } + + var store *popularityStore + if opts.DB != nil { + s, err := newPopularityStore(opts.DB) + if err != nil { + logger.Warn("catalog popularity: failed to open bbolt bucket; continuing memory-only", zap.Error(err)) + } else { + store = s + } + } + + baseCtx, cancel := context.WithCancel(context.Background()) + p := &githubStarsProvider{ + entries: make(map[string]*starsEntry), + store: store, + queue: make(chan string, githubQueueCap), + queued: make(map[string]chan struct{}), + logger: logger, + token: token, + disabled: !popularityFetchesEnabled(), + baseURL: currentGitHubAPIBase(), + now: time.Now, + budget: newRollingBudget(limit), + baseCtx: baseCtx, + cancel: cancel, + } + if store != nil { + p.entries = store.all() + p.mu.Lock() + for len(p.entries) > githubMaxCacheKeys { + p.evictIfNeededLocked() + } + p.mu.Unlock() + } + if !p.disabled { + p.startWorkers() + } + return p +} + +// Close stops all background fetch workers and returns immediately if +// already closed. It satisfies io.Closer so callers (internal/runtime) can +// hold the provider as an io.Closer without naming this unexported type. +func (p *githubStarsProvider) Close() error { + p.closeOnce.Do(func() { + p.closed.Store(true) + p.cancel() + p.wg.Wait() + // Wake anyone still waiting on a key the stopped workers never took. + p.mu.Lock() + pending := p.queued + p.queued = make(map[string]chan struct{}) + p.mu.Unlock() + for _, ch := range pending { + close(ch) + } + }) + return nil +} + +func (p *githubStarsProvider) startWorkers() { + for i := 0; i < githubMaxConcurrentFetches; i++ { + p.wg.Add(1) + go p.worker() + } +} + +func (p *githubStarsProvider) worker() { + defer p.wg.Done() + for { + select { + case <-p.baseCtx.Done(): + return + case key, ok := <-p.queue: + if !ok { + return + } + p.fetchAndStore(key) + } + } +} + +// fresh reports whether e is still within its TTL as of p.now(). +func (p *githubStarsProvider) fresh(e *starsEntry) bool { + return p.now().Sub(e.FetchedAt) < e.ttl() +} + +// getEntryLocked returns key's entry, lazily loading it from the store on a +// memory miss (plan.md: "Entries are loaded into memory lazily on the first +// Lookup miss"). Caller must hold p.mu. +func (p *githubStarsProvider) getEntryLocked(key string) *starsEntry { + if e, ok := p.entries[key]; ok { + return e + } + if p.store != nil { + if e, ok := p.store.get(key); ok { + p.entries[key] = e + return e + } + } + return nil +} + +// Lookup implements PopularityProvider. +func (p *githubStarsProvider) Lookup(key string) (int, LookupState) { + p.mu.Lock() + e := p.getEntryLocked(key) + p.mu.Unlock() + if e == nil { + return 0, LookupAbsent + } + return p.classify(e) +} + +// classify maps an entry to its LookupState (FR-008, zcode review finding +// 7). Whether stars are displayable depends only on whether there IS a +// positive count (e.Stars > 0) and whether it's still fresh — this covers a +// successful fetch, and also a since-erroring refresh that still carries a +// last-known-good count forward (FR-008's "keep the last-known stars"). +func (p *githubStarsProvider) classify(e *starsEntry) (int, LookupState) { + fresh := p.fresh(e) + if e.Stars > 0 { + if fresh { + return e.Stars, LookupFresh + } + return e.Stars, LookupStale + } + // No signal to show right now: either a confirmed 404/451, or an entry + // that has never successfully resolved (including one currently backing + // off after an error, within its own shorter TTL). + if fresh { + return 0, LookupNegative + } + return 0, LookupAbsent +} + +// Resolve implements PopularityProvider. +func (p *githubStarsProvider) Resolve(ctx context.Context, keys []string, wait time.Duration) { + if p.disabled || p.closed.Load() || len(keys) == 0 { + return + } + + // zcode review finding 5: bail out immediately, without enqueueing + // anything, when we already know no fetch will be attempted right now. + now := p.now() + if p.pausedNow(now) || !p.budget.hasCapacity(now) { + return + } + + var waiters []<-chan struct{} + for _, key := range keys { + if ch := p.enqueue(key); ch != nil { + waiters = append(waiters, ch) + } + } + if len(waiters) == 0 || wait <= 0 { + return + } + + waitCtx, cancel := context.WithTimeout(ctx, wait) + defer cancel() + for _, ch := range waiters { + select { + case <-ch: + case <-waitCtx.Done(): + return + } + } +} + +// enqueue admits key to the fetch queue, returning a channel closed once the +// fetch (or a no-op drop) completes — or nil if key needs no fetch (already +// fresh), is already queued/in-flight (its existing completion channel is +// returned instead, so callers share it rather than double-fetching), or the +// queue is full (FR-009b: overflow is simply dropped — no dedup entry is +// created, so nothing waits on it and the next search can request it again). +func (p *githubStarsProvider) enqueue(key string) <-chan struct{} { + p.mu.Lock() + defer p.mu.Unlock() + + if e := p.getEntryLocked(key); e != nil && p.fresh(e) { + return nil + } + if ch, ok := p.queued[key]; ok { + return ch + } + select { + case p.queue <- key: + ch := make(chan struct{}) + p.queued[key] = ch + return ch + default: + return nil + } +} + +// complete removes key from the in-flight/queued dedup set and closes its +// completion channel, waking any Resolve callers waiting on it. +func (p *githubStarsProvider) complete(key string) { + p.mu.Lock() + ch, ok := p.queued[key] + delete(p.queued, key) + p.mu.Unlock() + if ok { + close(ch) + } +} + +func (p *githubStarsProvider) pausedNow(now time.Time) bool { + p.pausedMu.Lock() + defer p.pausedMu.Unlock() + return now.Before(p.pausedTil) +} + +// pauseUntil extends the breaker pause to at least until, never shortening +// an existing longer pause. +func (p *githubStarsProvider) pauseUntil(until time.Time) { + p.pausedMu.Lock() + defer p.pausedMu.Unlock() + if until.After(p.pausedTil) { + p.pausedTil = until + } +} + +// fetchAndStore fetches one key (or drops it, if paused/over budget) and +// always completes it. It runs on p.baseCtx — a context owned by the +// provider, detached from whatever Resolve call originally enqueued the key +// (zcode review finding 3), so the fetch outlives that call. +func (p *githubStarsProvider) fetchAndStore(key string) { + defer p.complete(key) + + // After Close, a worker can still pick a buffered key (select chooses + // randomly among ready cases). Drop it before it spends budget or writes + // a shutdown-induced "transport error" entry to the store. + if p.baseCtx.Err() != nil { + return + } + + now := p.now() + if p.pausedNow(now) { + return // breaker engaged: retried on a later Resolve call + } + if !p.budget.allow(now) { + return // rolling budget exhausted: retried on a later Resolve call + } + + owner, repo, ok := splitRepoKey(key) + if !ok { + return + } + + p.mu.Lock() + prev := p.getEntryLocked(key) + p.mu.Unlock() + + ctx, cancel := context.WithTimeout(p.baseCtx, githubRequestTimeout) + defer cancel() + + status, stars, etag, headers, fetchErr := p.doRequest(ctx, owner, repo, prev) + p.applyResult(key, prev, status, stars, etag, headers, fetchErr) +} + +// rateLimitHeaders is the subset of GitHub's response headers the breaker +// (FR-009d) reads. +type rateLimitHeaders struct { + remaining string + reset string + retryAfter string +} + +// doRequest issues a single, non-retrying GET against p.baseURL (FR-006: +// "There are no retries; the breaker is the only backoff" — this +// deliberately does NOT use registryGet, which pins to a *RegistryEntry and +// retries 429/5xx three times, burning budget against a rate-limited API). +func (p *githubStarsProvider) doRequest(ctx context.Context, owner, repo string, prev *starsEntry) (status, stars int, etag string, headers rateLimitHeaders, err error) { + reqURL := p.baseURL + "/repos/" + url.PathEscape(owner) + "/" + url.PathEscape(repo) + + u, parseErr := url.Parse(reqURL) + if parseErr != nil { + return 0, 0, "", headers, parseErr + } + if blockErr := hostLiteralBlocked(u.Host, registryAllowPrivateFetch.Load()); blockErr != nil { + return 0, 0, "", headers, blockErr + } + if guardErr := guardRegistryTargetHost(ctx, reqURL); guardErr != nil { + return 0, 0, "", headers, guardErr + } + + req, reqErr := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, http.NoBody) + if reqErr != nil { + return 0, 0, "", headers, reqErr + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + req.Header.Set("User-Agent", registryUserAgent()) + if prev != nil && prev.ETag != "" { + req.Header.Set("If-None-Match", prev.ETag) + } + if p.token != "" { + req.Header.Set("Authorization", "Bearer "+p.token) + } + + resp, doErr := sharedRegistryClient().Do(req) + if doErr != nil { + return 0, 0, "", headers, doErr + } + defer resp.Body.Close() + + body, readErr := io.ReadAll(io.LimitReader(resp.Body, githubMaxBodyBytes+1)) + if readErr != nil { + return 0, 0, "", headers, readErr + } + + headers = rateLimitHeaders{ + remaining: resp.Header.Get("X-RateLimit-Remaining"), + reset: resp.Header.Get("X-RateLimit-Reset"), + retryAfter: resp.Header.Get("Retry-After"), + } + + switch resp.StatusCode { + case http.StatusOK: + // A 200 that is oversized, not JSON, or lacks stargazers_count (a + // captive portal, a proxy error page, a truncated body) is a failed + // fetch, not an answer: FR-008 keeps the last-known stars and ETag. + if int64(len(body)) > githubMaxBodyBytes { + return 0, 0, "", headers, errGitHubBodyTooLarge + } + var payload struct { + StargazersCount *int `json:"stargazers_count"` + } + if err := json.Unmarshal(body, &payload); err != nil { + return 0, 0, "", headers, fmt.Errorf("decode github repo response: %w", err) + } + if payload.StargazersCount == nil || *payload.StargazersCount < 0 { + return 0, 0, "", headers, errGitHubNoStargazers + } + return resp.StatusCode, *payload.StargazersCount, resp.Header.Get("ETag"), headers, nil + case http.StatusNotModified: + s := 0 + if prev != nil { + s = prev.Stars + } + etag := resp.Header.Get("ETag") + if etag == "" && prev != nil { + etag = prev.ETag + } + return resp.StatusCode, s, etag, headers, nil + default: + return resp.StatusCode, 0, "", headers, nil + } +} + +// applyResult stores the outcome of one fetch attempt (FR-008: a failed +// refresh keeps the last-known stars/etag and only advances FetchedAt; only +// a definitive 404/451 clears the stars) and, on an actual HTTP response +// (fetchErr == nil), updates the breaker from its rate-limit headers. +func (p *githubStarsProvider) applyResult(key string, prev *starsEntry, status, stars int, etag string, headers rateLimitHeaders, fetchErr error) { + // A request cut short by Close is not evidence about the repo; recording + // it would mark the key as an error (skipped for 1h) after a restart. + if fetchErr != nil && p.baseCtx.Err() != nil { + return + } + + now := p.now() + entry := &starsEntry{FetchedAt: now, Status: status} + + switch { + case fetchErr != nil: + entry.Status = 0 + if prev != nil { + entry.Stars, entry.ETag = prev.Stars, prev.ETag + } + case status == http.StatusOK: + entry.Stars = stars + entry.ETag = etag + case status == http.StatusNotModified: + entry.Stars = stars + entry.ETag = etag + case status == http.StatusNotFound || status == http.StatusUnavailableForLegalReasons: + // Definitive negative: Stars stays 0 regardless of what prev had — + // the repo really is gone/unavailable now. + default: + // Any other status (5xx, 403, 429, …): not definitive — keep the + // last-known stars/etag, just advance the (short, 1h) revalidate + // time via FetchedAt above. + if prev != nil { + entry.Stars, entry.ETag = prev.Stars, prev.ETag + } + } + + p.mu.Lock() + p.entries[key] = entry + p.evictIfNeededLocked() + p.mu.Unlock() + + if p.store != nil { + if err := p.store.put(key, entry); err != nil { + p.logger.Warn("catalog popularity: failed to persist entry", zap.String("key", key), zap.Error(err)) + } + } + + if fetchErr == nil { + p.applyBreaker(status, headers) + } +} + +// evictIfNeededLocked drops the single oldest-FetchedAt entry once the cache +// exceeds its cap (FR-008). Caller must hold p.mu. Only ever one entry over +// cap at a time, since insertion happens one key at a time. +func (p *githubStarsProvider) evictIfNeededLocked() { + if len(p.entries) <= githubMaxCacheKeys { + return + } + oldestKey := "" + var oldestTime time.Time + first := true + for k, e := range p.entries { + if first || e.FetchedAt.Before(oldestTime) { + oldestKey, oldestTime, first = k, e.FetchedAt, false + } + } + if oldestKey == "" { + return + } + delete(p.entries, oldestKey) + if p.store != nil { + if err := p.store.delete(oldestKey); err != nil { + p.logger.Warn("catalog popularity: failed to evict entry", zap.String("key", oldestKey), zap.Error(err)) + } + } +} + +// applyBreaker updates the pause window from one response's rate-limit +// headers (FR-009d): a 403/429 (Retry-After, else the reset time, else a 60s +// default) or ANY response reporting X-RateLimit-Remaining <= 5 (the reset +// time, else 60s). +func (p *githubStarsProvider) applyBreaker(status int, headers rateLimitHeaders) { + now := p.now() + + if status == http.StatusForbidden || status == http.StatusTooManyRequests { + if until, ok := parseRetryAfter(headers.retryAfter, now); ok { + p.pauseUntil(until) + return + } + if until, ok := parseUnixSeconds(headers.reset); ok { + p.pauseUntil(until) + return + } + p.pauseUntil(now.Add(githubBreakerPause)) + return + } + + if remaining, ok := parseNonNegativeInt(headers.remaining); ok && remaining <= githubLowRemainingThreshold { + if until, ok := parseUnixSeconds(headers.reset); ok { + p.pauseUntil(until) + return + } + p.pauseUntil(now.Add(githubBreakerPause)) + } +} + +func splitRepoKey(key string) (owner, repo string, ok bool) { + i := strings.IndexByte(key, '/') + if i <= 0 || i == len(key)-1 { + return "", "", false + } + return key[:i], key[i+1:], true +} + +func parseNonNegativeInt(v string) (int, bool) { + if v == "" { + return 0, false + } + n, err := strconv.Atoi(v) + if err != nil || n < 0 { + return 0, false + } + return n, true +} + +func parseUnixSeconds(v string) (time.Time, bool) { + if v == "" { + return time.Time{}, false + } + sec, err := strconv.ParseInt(v, 10, 64) + if err != nil { + return time.Time{}, false + } + return time.Unix(sec, 0), true +} + +// parseRetryAfter supports both Retry-After forms (RFC 9110): an integer +// number of seconds, or an HTTP-date. +func parseRetryAfter(v string, now time.Time) (time.Time, bool) { + if v == "" { + return time.Time{}, false + } + if secs, err := strconv.Atoi(v); err == nil { + return now.Add(time.Duration(secs) * time.Second), true + } + if t, err := http.ParseTime(v); err == nil { + return t, true + } + return time.Time{}, false +} diff --git a/internal/registries/popularity_github_test.go b/internal/registries/popularity_github_test.go new file mode 100644 index 000000000..4d723b012 --- /dev/null +++ b/internal/registries/popularity_github_test.go @@ -0,0 +1,535 @@ +package registries + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "sync" + "sync/atomic" + "testing" + "time" +) + +// --- T005: fetch behavior --------------------------------------------------- + +func TestGitHubStarsProvider_Fetch200StoresStarsAndSendsExactHeaders(t *testing.T) { + var gotHeaders http.Header + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotHeaders = r.Header.Clone() + w.Header().Set("ETag", `"v1"`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 123}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + + stars, state := p.Lookup("o/r") + if state != LookupFresh || stars != 123 { + t.Fatalf("expected Fresh/123, got state=%d stars=%d", state, stars) + } + if got := gotHeaders.Get("Accept"); got != "application/vnd.github+json" { + t.Errorf("Accept header = %q", got) + } + if got := gotHeaders.Get("X-GitHub-Api-Version"); got != "2022-11-28" { + t.Errorf("X-GitHub-Api-Version header = %q", got) + } + if got := gotHeaders.Get("User-Agent"); got == "" { + t.Error("expected a non-empty User-Agent") + } + if got := gotHeaders.Get("Authorization"); got != "" { + t.Errorf("expected no Authorization header without MCPPROXY_GITHUB_TOKEN, got %q", got) + } + if got := gotHeaders.Get("If-None-Match"); got != "" { + t.Errorf("expected no If-None-Match on a first fetch, got %q", got) + } +} + +func TestGitHubStarsProvider_BearerTokenSentOnlyWhenConfigured(t *testing.T) { + var gotAuth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 1}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + t.Setenv("MCPPROXY_GITHUB_TOKEN", "secret-token") + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + if gotAuth != "Bearer secret-token" { + t.Fatalf("expected 'Bearer secret-token', got %q", gotAuth) + } + if p.budget.limit != githubRateLimitAuth { + t.Fatalf("expected the authenticated rolling budget (%d) once a token is set, got %d", githubRateLimitAuth, p.budget.limit) + } +} + +func TestGitHubStarsProvider_Fetch304RevalidatesKeepsStars(t *testing.T) { + var reqCount int32 + var gotIfNoneMatch string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&reqCount, 1) + if n == 1 { + w.Header().Set("ETag", `"v1"`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 50}`)) + return + } + gotIfNoneMatch = r.Header.Get("If-None-Match") + w.Header().Set("ETag", `"v1"`) + w.WriteHeader(http.StatusNotModified) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + fixedNow := time.Now() + p.now = func() time.Time { return fixedNow } + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + if stars, state := p.Lookup("o/r"); state != LookupFresh || stars != 50 { + t.Fatalf("after first fetch: expected Fresh/50, got state=%d stars=%d", state, stars) + } + + // Advance the injected clock past the 24h positive TTL so the entry + // reads Stale and gets re-enqueued. + fixedNow = fixedNow.Add(25 * time.Hour) + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + if gotIfNoneMatch != `"v1"` { + t.Fatalf("expected If-None-Match %q on the revalidation request, got %q", `"v1"`, gotIfNoneMatch) + } + stars, state := p.Lookup("o/r") + if state != LookupFresh || stars != 50 { + t.Fatalf("after 304: expected stars preserved (Fresh/50), got state=%d stars=%d", state, stars) + } + if got := atomic.LoadInt32(&reqCount); got != 2 { + t.Fatalf("expected exactly 2 requests, got %d", got) + } +} + +func TestGitHubStarsProvider_Fetch404IsNegative(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/gone"}, time.Second) + stars, state := p.Lookup("o/gone") + if state != LookupNegative || stars != 0 { + t.Fatalf("expected Negative/0 for a 404, got state=%d stars=%d", state, stars) + } +} + +func TestGitHubStarsProvider_Fetch500KeepsLastKnownStars(t *testing.T) { + var reqCount int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + n := atomic.AddInt32(&reqCount, 1) + if n == 1 { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 30}`)) + return + } + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + fixedNow := time.Now() + p.now = func() time.Time { return fixedNow } + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + if stars, state := p.Lookup("o/r"); state != LookupFresh || stars != 30 { + t.Fatalf("after first fetch: expected Fresh/30, got state=%d stars=%d", state, stars) + } + + fixedNow = fixedNow.Add(25 * time.Hour) // past the 24h positive TTL -> re-enqueued + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + + stars, state := p.Lookup("o/r") + if stars != 30 { + t.Fatalf("expected the last-known stars (30) preserved through a 500, got %d", stars) + } + // Freshly time-stamped by the failed refresh, within its own 1h error + // TTL: still reads Fresh (it HAS a positive count). + if state != LookupFresh { + t.Fatalf("expected Fresh (last-known stars, revalidate time advanced), got state=%d", state) + } + if got := atomic.LoadInt32(&reqCount); got != 2 { + t.Fatalf("expected exactly 2 requests, got %d", got) + } +} + +func TestGitHubStarsProvider_BodyCappedAt1MiB(t *testing.T) { + padding := make([]byte, githubMaxBodyBytes+1000) + for i := range padding { + padding[i] = 'x' + } + // stargazers_count sits well past the 1 MiB cap, so it must never be read + // if the cap is actually enforced. + payload := append([]byte(`{"padding":"`), padding...) + payload = append(payload, []byte(`","stargazers_count": 999999}`)...) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write(payload) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r"}, 2*time.Second) + stars, _ := p.Lookup("o/r") + if stars == 999999 { + t.Fatal("expected the 1 MiB body cap to prevent stargazers_count from being read") + } +} + +func TestGitHubStarsProvider_FollowsSameHostRedirect(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/o/old", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/repos/o/new", http.StatusMovedPermanently) + }) + mux.HandleFunc("/repos/o/new", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 42}`)) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/old"}, time.Second) + stars, state := p.Lookup("o/old") + if state != LookupFresh || stars != 42 { + t.Fatalf("expected the same-host redirect to be followed, got state=%d stars=%d", state, stars) + } +} + +// --- T006: TTL/stale/requeue, dedup, overflow, concurrency, budget, breaker - + +func TestGitHubStarsProvider_EnqueueDedupsSameKey(t *testing.T) { + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") // no workers; drive enqueue directly + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + ch1 := p.enqueue("o/r") + ch2 := p.enqueue("o/r") + if ch1 == nil || ch2 == nil { + t.Fatal("expected both enqueue calls to be admitted") + } + if ch1 != ch2 { + t.Fatal("expected the second enqueue of an already-queued key to share its completion channel") + } +} + +func TestGitHubStarsProvider_QueueOverflowDropsWithoutLeavingDedupEntry(t *testing.T) { + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") // no workers; queue never drains + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + for i := 0; i < githubQueueCap; i++ { + p.queue <- fmt.Sprintf("filler/%d", i) + } + + ch := p.enqueue("overflow/key") + if ch != nil { + t.Fatal("expected enqueue to return nil once the queue is full") + } + p.mu.Lock() + _, tracked := p.queued["overflow/key"] + p.mu.Unlock() + if tracked { + t.Fatal("expected the dropped key to leave no dedup entry behind (so it can be requested again)") + } +} + +func TestGitHubStarsProvider_ConcurrencyCappedAtFour(t *testing.T) { + var active int32 + var mu sync.Mutex + var maxActive int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + n := atomic.AddInt32(&active, 1) + mu.Lock() + if n > maxActive { + maxActive = n + } + mu.Unlock() + time.Sleep(80 * time.Millisecond) + atomic.AddInt32(&active, -1) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 1}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + keys := make([]string, 0, 12) + for i := 0; i < 12; i++ { + keys = append(keys, fmt.Sprintf("owner/repo-%d", i)) + } + p.Resolve(context.Background(), keys, 3*time.Second) + + mu.Lock() + got := maxActive + mu.Unlock() + if got > githubMaxConcurrentFetches { + t.Fatalf("observed %d concurrent fetches, want <= %d", got, githubMaxConcurrentFetches) + } + if got < 2 { + t.Fatalf("expected some real concurrency (>=2 at once), got max=%d", got) + } + for _, k := range keys { + if _, state := p.Lookup(k); state != LookupFresh { + t.Errorf("expected %s to have been fetched (Fresh), got state=%d", k, state) + } + } +} + +func TestRollingBudget_EnforcesLimitPerWindow(t *testing.T) { + now := time.Unix(1_000_000, 0) + b := newRollingBudget(2) + if !b.allow(now) { + t.Fatal("expected the 1st request to be allowed") + } + if !b.allow(now) { + t.Fatal("expected the 2nd request to be allowed") + } + if b.allow(now) { + t.Fatal("expected the 3rd request within the same window to be blocked") + } + later := now.Add(61 * time.Minute) + if !b.allow(later) { + t.Fatal("expected a request to be allowed again once the window has rolled past") + } +} + +func TestGitHubStarsProvider_DefaultBudgetIsUnauthenticated(t *testing.T) { + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + if p.budget.limit != githubRateLimitUnauth { + t.Fatalf("expected the unauthenticated budget (%d), got %d", githubRateLimitUnauth, p.budget.limit) + } +} + +// TestGitHubStarsProvider_BreakerPausesOn403 pins SC-004: a 403 carrying +// X-RateLimit-Remaining: 0 and a future reset pauses ALL further fetches +// until that reset, while stale cached values keep being served. +func TestGitHubStarsProvider_BreakerPausesOn403(t *testing.T) { + var reqCount int32 + resetAt := time.Now().Add(2 * time.Hour) // far enough that test latency can't cross it + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + n := atomic.AddInt32(&reqCount, 1) + if n == 1 { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", strconv.FormatInt(resetAt.Unix(), 10)) + w.WriteHeader(http.StatusForbidden) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 1}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r1"}, 500*time.Millisecond) + if got := atomic.LoadInt32(&reqCount); got != 1 { + t.Fatalf("expected exactly 1 request before the breaker engaged, got %d", got) + } + + // Seed a stale cached value to assert it is still served during the pause. + p.mu.Lock() + p.entries["o/stale"] = &starsEntry{Stars: 9, Status: http.StatusOK, FetchedAt: time.Now().Add(-48 * time.Hour)} + p.mu.Unlock() + + p.Resolve(context.Background(), []string{"o/r2"}, 200*time.Millisecond) + if got := atomic.LoadInt32(&reqCount); got != 1 { + t.Fatalf("expected the breaker to block further requests before the reset, got %d total requests", got) + } + + stars, state := p.Lookup("o/stale") + if state != LookupStale || stars != 9 { + t.Fatalf("expected the stale cached value to still be served during the pause, got stars=%d state=%d", stars, state) + } +} + +// --- T008: Resolve wait bounds ---------------------------------------------- + +func TestGitHubStarsProvider_ResolveReturnsAroundWait(t *testing.T) { + block := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + <-block + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 5}`)) + })) + defer func() { + close(block) + srv.Close() + }() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + start := time.Now() + p.Resolve(context.Background(), []string{"o/r"}, 100*time.Millisecond) + elapsed := time.Since(start) + if elapsed < 100*time.Millisecond { + t.Fatalf("expected Resolve to wait at least 100ms, returned after %s", elapsed) + } + if elapsed > 800*time.Millisecond { + t.Fatalf("expected Resolve to return promptly after its wait, took %s", elapsed) + } + if _, state := p.Lookup("o/r"); state != LookupAbsent { + t.Fatalf("expected no result yet (stub still blocked), got state=%d", state) + } +} + +func TestGitHubStarsProvider_ResolveNeverOutlivesCtx(t *testing.T) { + block := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + <-block + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 5}`)) + })) + defer func() { + close(block) + srv.Close() + }() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) + defer cancel() + start := time.Now() + p.Resolve(ctx, []string{"o/r"}, 5*time.Second) // wait far longer than ctx + if elapsed := time.Since(start); elapsed > 800*time.Millisecond { + t.Fatalf("expected Resolve to return once ctx expired, took %s", elapsed) + } +} + +func TestGitHubStarsProvider_ResolveWaitZeroNeverBlocks(t *testing.T) { + block := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + <-block + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 5}`)) + })) + defer func() { + close(block) + srv.Close() + }() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + start := time.Now() + p.Resolve(context.Background(), []string{"o/r"}, 0) + if elapsed := time.Since(start); elapsed > 50*time.Millisecond { + t.Fatalf("expected wait=0 to return immediately without blocking, took %s", elapsed) + } +} + +func TestGitHubStarsProvider_FetchContinuesAfterResolveReturns(t *testing.T) { + block := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + <-block + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 5}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r"}, 50*time.Millisecond) // returns before the stub answers + if _, state := p.Lookup("o/r"); state != LookupAbsent { + t.Fatalf("expected no result yet, got state=%d", state) + } + + close(block) // let the stub answer now + + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if _, state := p.Lookup("o/r"); state == LookupFresh { + return + } + time.Sleep(10 * time.Millisecond) + } + t.Fatal("expected the background fetch to complete and land in the cache after Resolve returned") +} + +// --- T009: kill switch ------------------------------------------------------- + +func TestPopularityFetchesEnabled(t *testing.T) { + cases := map[string]bool{ + "": true, + "true": true, + "false": false, + "0": false, + "off": false, + "OFF": false, + "False": false, + } + for v, want := range cases { + t.Run(fmt.Sprintf("env=%q", v), func(t *testing.T) { + t.Setenv("MCPPROXY_CATALOG_POPULARITY", v) + if got := popularityFetchesEnabled(); got != want { + t.Errorf("popularityFetchesEnabled() = %v, want %v", got, want) + } + }) + } +} + +func TestGitHubStarsProvider_KillSwitchNeverFetches(t *testing.T) { + var reqCount int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&reqCount, 1) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"stargazers_count": 5}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r"}, 200*time.Millisecond) + time.Sleep(50 * time.Millisecond) + if got := atomic.LoadInt32(&reqCount); got != 0 { + t.Fatalf("expected the kill switch to prevent any outbound request, got %d", got) + } +} diff --git a/internal/registries/popularity_key.go b/internal/registries/popularity_key.go new file mode 100644 index 000000000..032d581d6 --- /dev/null +++ b/internal/registries/popularity_key.go @@ -0,0 +1,65 @@ +package registries + +import ( + "net/url" + "regexp" + "strings" +) + +// githubOwnerPattern/githubRepoPattern are GitHub's own name rules (FR-003). +var ( + githubOwnerPattern = regexp.MustCompile(`^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$`) + githubRepoPattern = regexp.MustCompile(`^[A-Za-z0-9._-]{1,100}$`) +) + +// GitHubRepoKey normalizes a source-code URL into a lower-cased "owner/repo" +// key when it identifies a github.com repository whose owner and repo both +// pass GitHub's name rules (Spec 110 FR-003). It handles every edge-case +// form spec.md calls out: a `git+` prefix, a `.git` suffix, a monorepo +// subpath (only the first two path segments are taken), a `www.` prefix, a +// trailing slash, and mixed case. Anything else — a non-GitHub host, a gist, +// a malformed URL, or an owner/repo that fails the name rules (including the +// literal repo names "." and "..") — returns ok=false and never causes a +// fetch. +func GitHubRepoKey(sourceCodeURL string) (key string, ok bool) { + raw := strings.TrimSpace(sourceCodeURL) + if raw == "" { + return "", false + } + raw = strings.TrimPrefix(raw, "git+") + + u, err := url.Parse(raw) + if err != nil { + return "", false + } + if u.Scheme != "http" && u.Scheme != "https" { + return "", false + } + + host := strings.ToLower(u.Hostname()) + host = strings.TrimPrefix(host, "www.") + if host != "github.com" { + return "", false + } + + path := strings.Trim(u.Path, "/") + if path == "" { + return "", false + } + parts := strings.SplitN(path, "/", 3) // owner, repo, rest (monorepo subpath discarded) + if len(parts) < 2 || parts[0] == "" || parts[1] == "" { + return "", false + } + + owner := parts[0] + repo := strings.TrimSuffix(parts[1], ".git") + + if repo == "." || repo == ".." { + return "", false + } + if !githubOwnerPattern.MatchString(owner) || !githubRepoPattern.MatchString(repo) { + return "", false + } + + return strings.ToLower(owner + "/" + repo), true +} diff --git a/internal/registries/popularity_key_test.go b/internal/registries/popularity_key_test.go new file mode 100644 index 000000000..c6aa88a68 --- /dev/null +++ b/internal/registries/popularity_key_test.go @@ -0,0 +1,48 @@ +package registries + +import "testing" + +// TestGitHubRepoKey covers every edge-case form spec.md calls out (FR-003). +func TestGitHubRepoKey(t *testing.T) { + tests := []struct { + name string + url string + wantKey string + wantOK bool + }{ + {"plain https", "https://github.com/o/r", "o/r", true}, + {"git suffix", "https://github.com/o/r.git", "o/r", true}, + {"monorepo subpath", "https://github.com/o/r/tree/main/src/x", "o/r", true}, + {"git+https prefix", "git+https://github.com/o/r", "o/r", true}, + {"git+https with .git", "git+https://github.com/o/r.git", "o/r", true}, + {"http scheme", "http://github.com/o/r", "o/r", true}, + {"www prefix", "https://www.github.com/o/r", "o/r", true}, + {"trailing slash", "https://github.com/o/r/", "o/r", true}, + {"mixed case", "https://GitHub.com/Owner/Repo", "owner/repo", true}, + {"reference monorepo", "https://github.com/modelcontextprotocol/servers/tree/main/src/fetch", "modelcontextprotocol/servers", true}, + + {"empty", "", "", false}, + {"non-github host", "https://gitlab.com/o/r", "", false}, + {"gist host", "https://gist.github.com/o/r", "", false}, + {"owner only, no repo", "https://github.com/o", "", false}, + {"malformed url", "://not a url", "", false}, + {"repo is dot", "https://github.com/o/.", "", false}, + {"repo is dotdot", "https://github.com/o/..", "", false}, + {"owner fails name rule (leading dash)", "https://github.com/-o/r", "", false}, + {"repo fails name rule (space)", "https://github.com/o/r r", "", false}, + {"ssh-style (no scheme)", "git@github.com:o/r.git", "", false}, + {"non-http scheme", "ftp://github.com/o/r", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + key, ok := GitHubRepoKey(tt.url) + if ok != tt.wantOK { + t.Fatalf("GitHubRepoKey(%q) ok = %v, want %v (key=%q)", tt.url, ok, tt.wantOK, key) + } + if ok && key != tt.wantKey { + t.Fatalf("GitHubRepoKey(%q) = %q, want %q", tt.url, key, tt.wantKey) + } + }) + } +} diff --git a/internal/registries/popularity_review_fixes_test.go b/internal/registries/popularity_review_fixes_test.go new file mode 100644 index 000000000..1b1ec3159 --- /dev/null +++ b/internal/registries/popularity_review_fixes_test.go @@ -0,0 +1,169 @@ +package registries + +import ( + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" +) + +// A 200 that is not a GitHub repo payload (captive portal, proxy error page) +// is a failed fetch: FR-008 keeps the last-known stars and ETag, and the +// bogus ETag must never be sent back as If-None-Match. +func TestGitHubStarsProvider_Malformed200KeepsLastKnownStars(t *testing.T) { + var reqCount int32 + var lastIfNoneMatch atomic.Value + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastIfNoneMatch.Store(r.Header.Get("If-None-Match")) + if atomic.AddInt32(&reqCount, 1) == 1 { + w.Header().Set("ETag", `"good"`) + _, _ = w.Write([]byte(`{"stargazers_count": 500}`)) + return + } + w.Header().Set("ETag", `"portal"`) + _, _ = w.Write([]byte(`Sign in to the Wi-Fi`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + fixedNow := time.Now() + p.now = func() time.Time { return fixedNow } + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + fixedNow = fixedNow.Add(25 * time.Hour) + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + + if stars, _ := p.Lookup("o/r"); stars != 500 { + t.Fatalf("a non-JSON 200 cleared the stars: got %d, want the last-known 500", stars) + } + p.mu.Lock() + etag := p.entries["o/r"].ETag + p.mu.Unlock() + if etag != `"good"` { + t.Fatalf("stored ETag = %q, want the last good one", etag) + } +} + +// A 200 without stargazers_count must not be read as "0 stars". +func TestGitHubStarsProvider_200WithoutStargazersIsAFailedFetch(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"message":"ok"}`)) + })) + defer srv.Close() + + defer SetGitHubAPIBaseForTest(srv.URL)() + p := NewGitHubStarsProvider(PopularityOptions{}) + defer p.Close() + + p.Resolve(context.Background(), []string{"o/r"}, time.Second) + p.mu.Lock() + e := p.entries["o/r"] + p.mu.Unlock() + if e == nil || e.Status != 0 || e.ttl() != githubErrorTTL { + t.Fatalf("expected an error entry with the 1h error TTL, got %+v", e) + } +} + +// After Close, a key a worker still picks off the buffer, or a request cut +// short by the cancellation, must not spend budget or persist a +// shutdown-induced error entry. +func TestGitHubStarsProvider_CloseDoesNotRecordShutdownErrors(t *testing.T) { + db := openTempPopularityDB(t) + defer SetGitHubAPIBaseForTest("http://127.0.0.1:1")() + p := NewGitHubStarsProvider(PopularityOptions{DB: db}) + _ = p.Close() + + p.fetchAndStore("o/r") + p.applyResult("o/s", nil, 0, 0, "", rateLimitHeaders{}, context.Canceled) + + if n := len(p.store.all()); n != 0 { + t.Fatalf("store has %d entries after shutdown, want 0", n) + } + if !p.budget.hasCapacity(time.Now()) || len(p.budget.hits) != 0 { + t.Fatalf("a post-Close key spent budget: %d hits", len(p.budget.hits)) + } +} + +// Resolve on a closed provider returns at once instead of queueing keys no +// worker will drain and waiting out the full wait. +func TestGitHubStarsProvider_ResolveAfterCloseReturnsImmediately(t *testing.T) { + defer SetGitHubAPIBaseForTest("http://127.0.0.1:1")() + p := NewGitHubStarsProvider(PopularityOptions{}) + _ = p.Close() + + start := time.Now() + p.Resolve(context.Background(), []string{"o/r"}, 2*time.Second) + if d := time.Since(start); d > 500*time.Millisecond { + t.Fatalf("Resolve on a closed provider took %s", d) + } + p.mu.Lock() + n := len(p.queued) + p.mu.Unlock() + if n != 0 { + t.Fatalf("closed provider admitted %d keys", n) + } +} + +// Close wakes waiters on keys the stopped workers never took. +func TestGitHubStarsProvider_CloseWakesPendingWaiters(t *testing.T) { + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") // no workers: keys stay queued + p := NewGitHubStarsProvider(PopularityOptions{}) + p.disabled = false // admit keys, but nothing drains them + ch := p.enqueue("o/r") + if ch == nil { + t.Fatal("expected the key to be admitted") + } + _ = p.Close() + select { + case <-ch: + case <-time.After(time.Second): + t.Fatal("waiter on a never-fetched key was not woken by Close") + } +} + +type stateStubProvider struct { + stars int + state LookupState +} + +func (s *stateStubProvider) Lookup(string) (int, LookupState) { return s.stars, s.state } +func (s *stateStubProvider) Resolve(context.Context, []string, time.Duration) {} + +// A repo confirmed gone during Resolve's wait must lose the stars an earlier +// (Stale) apply put on the hit, keeping only source-native popularity. +func TestApplyCachedStars_NegativeClearsEarlierStars(t *testing.T) { + stub := &stateStubProvider{stars: 900, state: LookupStale} + defer SetPopularityProviderForTest(stub)() + + installs := 42 + withInstalls := CatalogHit{Entry: ServerEntry{ + SourceCodeURL: "https://github.com/o/r", + Popularity: &Popularity{Installs: &installs}, + }} + bare := CatalogHit{Entry: ServerEntry{SourceCodeURL: "https://github.com/o/r"}} + for _, h := range []*CatalogHit{&withInstalls, &bare} { + if h.Entry.Popularity != nil { + p := *h.Entry.Popularity + h.Popularity = &p + } + applyCachedStars(h) + if h.Popularity == nil || h.Popularity.Stars == nil || *h.Popularity.Stars != 900 { + t.Fatalf("setup: expected stale stars applied, got %+v", h.Popularity) + } + } + + stub.state, stub.stars = LookupNegative, 0 + applyCachedStars(&withInstalls) + applyCachedStars(&bare) + + if withInstalls.Popularity == nil || withInstalls.Popularity.Stars != nil || *withInstalls.Popularity.Installs != 42 { + t.Fatalf("expected stars cleared and installs kept, got %+v", withInstalls.Popularity) + } + if bare.Popularity != nil { + t.Fatalf("expected Popularity dropped when no signal remains, got %+v", bare.Popularity) + } +} diff --git a/internal/registries/popularity_store.go b/internal/registries/popularity_store.go new file mode 100644 index 000000000..12e4efe52 --- /dev/null +++ b/internal/registries/popularity_store.go @@ -0,0 +1,115 @@ +package registries + +import ( + "encoding/json" + "fmt" + + "go.etcd.io/bbolt" +) + +// popularityBucketName is the dedicated bbolt bucket for the popularity +// cache (Spec 110 FR-008, plan.md R4: cache.Manager does not fit — fixed 2h +// TTL, deletes expired records, and Spec 105 authorization frames that are +// irrelevant here). +const popularityBucketName = "catalog_popularity" + +// popularityStore persists starsEntry records to popularityBucketName. A nil +// *popularityStore (never constructed) means memory-only, which is what the +// CLI in-process fallback uses (plan.md wiring table). +type popularityStore struct { + db *bbolt.DB +} + +// newPopularityStore opens (creating if needed) the popularity bucket on an +// already-open bbolt database. +func newPopularityStore(db *bbolt.DB) (*popularityStore, error) { + if db == nil { + return nil, fmt.Errorf("catalog popularity store: nil db") + } + if err := db.Update(func(tx *bbolt.Tx) error { + _, err := tx.CreateBucketIfNotExists([]byte(popularityBucketName)) + return err + }); err != nil { + return nil, fmt.Errorf("catalog popularity store: create bucket: %w", err) + } + return &popularityStore{db: db}, nil +} + +// all returns every decodable persisted entry. The provider loads the whole +// bucket once at construction so the FR-008 key cap applies to what is on +// disk, not just to what a lazy lookup happened to pull into memory — with +// lazy loading, a restart reset the in-memory count to zero and the bucket +// could grow past the cap forever. +func (s *popularityStore) all() map[string]*starsEntry { + out := make(map[string]*starsEntry) + _ = s.db.View(func(tx *bbolt.Tx) error { + b := tx.Bucket([]byte(popularityBucketName)) + if b == nil { + return nil + } + return b.ForEach(func(k, v []byte) error { + var entry starsEntry + if err := json.Unmarshal(v, &entry); err != nil { + return nil //nolint:nilerr // a corrupt record is skipped, never fatal + } + out[string(k)] = &entry + return nil + }) + }) + return out +} + +// get returns the persisted entry for key, or ok=false if there is none (or +// it fails to decode — treated the same as absent rather than as an error, +// since a corrupt single record must never break the whole cache). +func (s *popularityStore) get(key string) (*starsEntry, bool) { + var entry starsEntry + found := false + _ = s.db.View(func(tx *bbolt.Tx) error { + b := tx.Bucket([]byte(popularityBucketName)) + if b == nil { + return nil + } + v := b.Get([]byte(key)) + if v == nil { + return nil + } + if err := json.Unmarshal(v, &entry); err != nil { + return nil //nolint:nilerr // a corrupt record is treated as absent, not fatal + } + found = true + return nil + }) + if !found { + return nil, false + } + return &entry, true +} + +// put persists entry under key, creating the bucket if a prior open somehow +// missed it (defensive; newPopularityStore already creates it). +func (s *popularityStore) put(key string, entry *starsEntry) error { + data, err := json.Marshal(entry) + if err != nil { + return err + } + return s.db.Update(func(tx *bbolt.Tx) error { + b, err := tx.CreateBucketIfNotExists([]byte(popularityBucketName)) + if err != nil { + return err + } + return b.Put([]byte(key), data) + }) +} + +// delete removes key (used by cap eviction — FR-008: "evicting the oldest +// fetched_at first"). +func (s *popularityStore) delete(key string) error { + return s.db.Update(func(tx *bbolt.Tx) error { + b := tx.Bucket([]byte(popularityBucketName)) + if b == nil { + return nil + } + return b.Delete([]byte(key)) + }) +} diff --git a/internal/registries/popularity_store_test.go b/internal/registries/popularity_store_test.go new file mode 100644 index 000000000..ca93fb462 --- /dev/null +++ b/internal/registries/popularity_store_test.go @@ -0,0 +1,173 @@ +package registries + +import ( + "encoding/json" + "path/filepath" + "testing" + "time" + + "go.etcd.io/bbolt" +) + +func openTempPopularityDB(t *testing.T) *bbolt.DB { + t.Helper() + path := filepath.Join(t.TempDir(), "popularity.db") + db, err := bbolt.Open(path, 0o600, &bbolt.Options{Timeout: time.Second}) + if err != nil { + t.Fatalf("bbolt.Open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + return db +} + +// TestPopularityStore_RoundTrip pins the basic put/get round trip (T007). +func TestPopularityStore_RoundTrip(t *testing.T) { + db := openTempPopularityDB(t) + store, err := newPopularityStore(db) + if err != nil { + t.Fatalf("newPopularityStore: %v", err) + } + + entry := &starsEntry{Stars: 42, ETag: `"abc"`, FetchedAt: time.Now().UTC().Truncate(time.Second), Status: 200} + if err := store.put("o/r", entry); err != nil { + t.Fatalf("put: %v", err) + } + + got, ok := store.get("o/r") + if !ok { + t.Fatal("expected the entry to round-trip") + } + if got.Stars != entry.Stars || got.ETag != entry.ETag || got.Status != entry.Status || !got.FetchedAt.Equal(entry.FetchedAt) { + t.Fatalf("round-tripped entry mismatch: got %+v, want %+v", got, entry) + } + + if _, ok := store.get("missing/key"); ok { + t.Fatal("expected a missing key to report ok=false") + } +} + +// TestPopularityStore_Delete pins that delete removes a persisted entry. +func TestPopularityStore_Delete(t *testing.T) { + db := openTempPopularityDB(t) + store, err := newPopularityStore(db) + if err != nil { + t.Fatalf("newPopularityStore: %v", err) + } + _ = store.put("o/r", &starsEntry{Stars: 1, FetchedAt: time.Now()}) + if err := store.delete("o/r"); err != nil { + t.Fatalf("delete: %v", err) + } + if _, ok := store.get("o/r"); ok { + t.Fatal("expected the entry to be gone after delete") + } +} + +// TestGitHubStarsProvider_LazyLoadFromStore pins that a provider restart +// (fresh githubStarsProvider over the SAME bbolt db) sees a previously +// fetched entry via Lookup with NO fetch — the lazy-load-on-miss path +// (plan.md data model) survives a restart. +func TestGitHubStarsProvider_LazyLoadFromStore(t *testing.T) { + db := openTempPopularityDB(t) + store, err := newPopularityStore(db) + if err != nil { + t.Fatalf("newPopularityStore: %v", err) + } + fetchedAt := time.Now() + if err := store.put("o/r", &starsEntry{Stars: 77, Status: 200, FetchedAt: fetchedAt}); err != nil { + t.Fatalf("seed put: %v", err) + } + + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") // no workers; this test is Lookup-only + provider := NewGitHubStarsProvider(PopularityOptions{DB: db}) + defer provider.Close() + + stars, state := provider.Lookup("o/r") + if state != LookupFresh { + t.Fatalf("expected LookupFresh from the pre-seeded store, got state=%d stars=%d", state, stars) + } + if stars != 77 { + t.Fatalf("expected stars=77 from the pre-seeded store, got %d", stars) + } +} + +// TestGitHubStarsProvider_CapEviction pins FR-008's 5000-key cap: inserting +// one more than the cap evicts the single oldest-FetchedAt entry (from both +// memory and the store). +func TestGitHubStarsProvider_CapEviction(t *testing.T) { + db := openTempPopularityDB(t) + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") + provider := NewGitHubStarsProvider(PopularityOptions{DB: db}) + defer provider.Close() + + base := time.Now().Add(-time.Hour) + provider.mu.Lock() + for i := 0; i < githubMaxCacheKeys; i++ { + key := keyForIndex(i) + provider.entries[key] = &starsEntry{Stars: 1, Status: 200, FetchedAt: base.Add(time.Duration(i) * time.Second)} + } + provider.mu.Unlock() + + // The oldest key (index 0) should be evicted once we go one over cap. + provider.applyResult("new/key", nil, 200, 5, "", rateLimitHeaders{}, nil) + + provider.mu.Lock() + count := len(provider.entries) + _, oldestStillPresent := provider.entries[keyForIndex(0)] + provider.mu.Unlock() + + if count != githubMaxCacheKeys { + t.Fatalf("expected the entry count to stay capped at %d, got %d", githubMaxCacheKeys, count) + } + if oldestStillPresent { + t.Fatal("expected the oldest entry to have been evicted") + } +} + +func keyForIndex(i int) string { + return "owner/repo-" + string(rune('a'+(i%26))) + string(rune('a'+((i/26)%26))) + string(rune('a'+((i/676)%26))) +} + +// TestGitHubStarsProvider_CapHoldsAcrossRestart pins that the FR-008 key cap +// bounds the bbolt bucket, not just the in-memory map: a bucket already over +// the cap (written by an earlier process) is trimmed to the cap, oldest +// FetchedAt first, when the next provider opens it. With lazy loading the +// in-memory count restarted at zero and the bucket grew without bound. +func TestGitHubStarsProvider_CapHoldsAcrossRestart(t *testing.T) { + db := openTempPopularityDB(t) + if _, err := newPopularityStore(db); err != nil { + t.Fatalf("newPopularityStore: %v", err) + } + const over = 3 + base := time.Now().Add(-time.Hour) + if err := db.Update(func(tx *bbolt.Tx) error { + b := tx.Bucket([]byte(popularityBucketName)) + for i := 0; i < githubMaxCacheKeys+over; i++ { + v, err := json.Marshal(&starsEntry{Stars: 1, Status: 200, FetchedAt: base.Add(time.Duration(i) * time.Second)}) + if err != nil { + return err + } + if err := b.Put([]byte(keyForIndex(i)), v); err != nil { + return err + } + } + return nil + }); err != nil { + t.Fatalf("seed: %v", err) + } + + t.Setenv("MCPPROXY_CATALOG_POPULARITY", "false") + provider := NewGitHubStarsProvider(PopularityOptions{DB: db}) + defer provider.Close() + + if got := len(provider.store.all()); got != githubMaxCacheKeys { + t.Fatalf("bucket holds %d keys after reopen, want the cap %d", got, githubMaxCacheKeys) + } + for i := 0; i < over; i++ { + if _, state := provider.Lookup(keyForIndex(i)); state != LookupAbsent { + t.Errorf("oldest key %d should have been evicted, got state %d", i, state) + } + } + if _, state := provider.Lookup(keyForIndex(githubMaxCacheKeys + over - 1)); state != LookupFresh { + t.Errorf("newest key should survive, got state %d", state) + } +} diff --git a/internal/registries/rank_test.go b/internal/registries/rank_test.go index 38fe00ef9..5095b3209 100644 --- a/internal/registries/rank_test.go +++ b/internal/registries/rank_test.go @@ -70,6 +70,52 @@ func TestRank_MissingPopularityIsZero(t *testing.T) { } } +// TestRank_StarsBeatInstalls pins FR-004: stars is the primary popularity +// key — a hit with fewer stars never loses to one with vastly more installs. +func TestRank_StarsBeatInstalls(t *testing.T) { + starred := CatalogHit{Popularity: &Popularity{Stars: intPtr(10)}, Entry: ServerEntry{ID: "b"}} + installed := CatalogHit{Popularity: &Popularity{Installs: intPtr(999999)}, Entry: ServerEntry{ID: "a"}} + if !Rank(starred, installed, "") { + t.Error("expected stars to outrank a much larger installs count") + } + if Rank(installed, starred, "") { + t.Error("expected stars to outrank installs (reverse check)") + } +} + +// TestRank_InstallsBreakStarsTie pins FR-004's secondary key: when stars tie +// (including both nil/0), installs decides. +func TestRank_InstallsBreakStarsTie(t *testing.T) { + moreInstalls := CatalogHit{Popularity: &Popularity{Stars: intPtr(5), Installs: intPtr(200)}, Entry: ServerEntry{ID: "b"}} + fewerInstalls := CatalogHit{Popularity: &Popularity{Stars: intPtr(5), Installs: intPtr(10)}, Entry: ServerEntry{ID: "a"}} + if !Rank(moreInstalls, fewerInstalls, "") { + t.Error("expected higher installs to break an equal-stars tie") + } + + // Both nil Popularity.Stars (0) but different installs: still ties on + // stars(0) then breaks on installs. + onlyInstalls := CatalogHit{Popularity: &Popularity{Installs: intPtr(50)}, Entry: ServerEntry{ID: "d"}} + noSignal := CatalogHit{Entry: ServerEntry{ID: "c"}} + if !Rank(onlyInstalls, noSignal, "") { + t.Error("expected a hit with installs>0 to outrank one with no signal at all") + } +} + +// TestRank_StarsAndInstallsNeverSummed pins FR-004's "never converted into +// one another": a hit with fewer stars AND fewer installs than another must +// never win by having their sum happen to exceed it — i.e. this is a +// lexicographic tuple compare, not a score. +func TestRank_StarsAndInstallsNeverSummed(t *testing.T) { + a := CatalogHit{Popularity: &Popularity{Stars: intPtr(3), Installs: intPtr(1000000)}, Entry: ServerEntry{ID: "b"}} + b := CatalogHit{Popularity: &Popularity{Stars: intPtr(4), Installs: intPtr(1)}, Entry: ServerEntry{ID: "a"}} + // b has more stars (4 > 3) despite far fewer installs — b must win. + if !Rank(b, a, "") { + t.Error("expected the higher-stars hit to win regardless of the other's much larger installs") + } +} + func intPop(stars int) *Popularity { return &Popularity{Stars: &stars} } + +func intPtr(n int) *int { return &n } diff --git a/internal/registries/search.go b/internal/registries/search.go index 189e5f964..7206f539b 100644 --- a/internal/registries/search.go +++ b/internal/registries/search.go @@ -406,6 +406,16 @@ func parseDocker(rawData interface{}) []ServerEntry { server.UpdatedAt = lastUpdated } + // Spec 110 FR-001: pull_count -> Installs (comes free with the + // listing this parser already fetches). Docker's own star_count + // is deliberately NEVER mapped to Stars — it lives on a wildly + // different scale from GitHub stars (single digits vs tens of + // thousands) and mixing them would be meaningless. + if pullCount, ok := itemMap["pull_count"].(float64); ok && pullCount >= 0 { + installs := int(pullCount) + server.Popularity = &Popularity{Installs: &installs} + } + servers = append(servers, server) } } diff --git a/internal/registries/testhooks.go b/internal/registries/testhooks.go index d8a6139c5..cc6f35916 100644 --- a/internal/registries/testhooks.go +++ b/internal/registries/testhooks.go @@ -32,3 +32,27 @@ func AllowPrivateRegistryFetchForTest() (restore func()) { registryAllowPrivateFetch.Store(prev) } } + +// SetPopularityProviderForTest installs p as the process-wide popularity +// provider (Spec 110 FR-010) and returns a restore func reinstalling +// whatever was previously installed (typically nil). +func SetPopularityProviderForTest(p PopularityProvider) (restore func()) { + prev := getPopularityProvider() + SetPopularityProvider(p) + return func() { SetPopularityProvider(prev) } +} + +// SetGitHubAPIBaseForTest overrides the GitHub API base URL new +// githubStarsProvider instances read at construction (default +// githubAPIBaseURLDefault), so a test can point NewGitHubStarsProvider at an +// httptest.Server. Combine with AllowPrivateRegistryFetchForTest, since the +// SSRF guard otherwise blocks a loopback target. Returns a restore func. +func SetGitHubAPIBaseForTest(base string) (restore func()) { + prev := currentGitHubAPIBase() + b := base + githubAPIBaseOverride.Store(&b) + return func() { + p := prev + githubAPIBaseOverride.Store(&p) + } +} diff --git a/internal/registries/types.go b/internal/registries/types.go index 25ac92208..99d3edc94 100644 --- a/internal/registries/types.go +++ b/internal/registries/types.go @@ -46,6 +46,14 @@ type ServerEntry struct { // ${VAR} / $VAR placeholders (see DetectRequiredInputs). Empty for most // servers in this spec — no rich per-registry schema yet (decision O1). RequiredInputs []RequiredInput `json:"required_inputs,omitempty"` + + // Popularity carries a source-NATIVE popularity signal discovered while + // parsing this entry (Spec 110 FR-001) — currently only parseDocker's + // pull_count → Installs. Deliberately `json:"-"`: it must never appear in + // ServerEntry's own JSON (GET /registries/{id}/servers, search_servers), + // which stays unchanged. BuildCatalogHit copies it into CatalogHit and + // layers in GitHub stars from the popularity provider's cache. + Popularity *Popularity `json:"-"` } // RequiredInput declares a single env var / key a server needs before it will diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index fd6d02164..389b51fe4 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "math" "net" "os" @@ -139,6 +140,13 @@ type Runtime struct { indexManager *index.Manager upstreamManager *upstream.Manager cacheManager *cache.Manager + // popularityProvider is the Spec 110 catalog popularity signal's + // bbolt-backed GitHub-stars provider, installed process-wide via + // registries.SetPopularityProvider unconditionally at startup — the + // FR-011 kill switch is read INSIDE the provider constructor, so this is + // never nil, it just starts no workers when disabled. io.Closer so this + // file never needs to name the unexported provider type. + popularityProvider io.Closer // promptsRefresh debounces upstream prompts/list_changed notifications into a // single RefreshPrompts fan-out (F13). Nil until lifecycle registration. promptsRefresh *promptsRefreshDebouncer @@ -299,6 +307,19 @@ func New(cfg *config.Config, cfgPath string, logger *zap.Logger) (*Runtime, erro return nil, fmt.Errorf("failed to initialize cache manager: %w", err) } + // Spec 110 (catalog popularity signal): a bbolt-backed GitHub-stars + // provider, installed process-wide so BuildCatalogHit/SearchAll (catalog + // search, CLI, MCP search_servers with registry omitted) can show real + // popularity. FR-011's kill switch (MCPPROXY_CATALOG_POPULARITY=false) is + // read inside the constructor, so this is unconditional — a disabled + // provider still answers Lookup from whatever is already cached, it just + // starts no fetch workers. + popularityProvider := registries.NewGitHubStarsProvider(registries.PopularityOptions{ + DB: storageManager.GetDB(), + Logger: logger, + }) + registries.SetPopularityProvider(popularityProvider) + truncator := truncate.NewTruncator(cfg.ToolResponseLimit) // Initialize tokenizer (defaults to enabled with cl100k_base) @@ -396,24 +417,25 @@ func New(cfg *config.Config, cfgPath string, logger *zap.Logger) (*Runtime, erro rt := &Runtime{ cfg: cfg, // Boot: memory and disk agree by definition. - desiredCfg: cfg, - cfgPath: cfgPath, - logger: logger, - configSvc: configSvc, - storageManager: storageManager, - indexManager: indexManager, - upstreamManager: upstreamManager, - cacheManager: cacheManager, - sigCache: toolsig.NewCache(), - secretResolver: secretResolver, - tokenizer: tokenizer, - refreshManager: refreshManager, - activityService: activityService, - supervisor: supervisorInstance, - prechurnStore: prechurnStore, - previousShutdown: previousShutdown, - appCtx: appCtx, - appCancel: appCancel, + desiredCfg: cfg, + cfgPath: cfgPath, + logger: logger, + configSvc: configSvc, + storageManager: storageManager, + indexManager: indexManager, + upstreamManager: upstreamManager, + cacheManager: cacheManager, + popularityProvider: popularityProvider, + sigCache: toolsig.NewCache(), + secretResolver: secretResolver, + tokenizer: tokenizer, + refreshManager: refreshManager, + activityService: activityService, + supervisor: supervisorInstance, + prechurnStore: prechurnStore, + previousShutdown: previousShutdown, + appCtx: appCtx, + appCancel: appCancel, status: Status{ Phase: PhaseInitializing, Message: "Runtime is initializing...", @@ -892,6 +914,12 @@ func (r *Runtime) Close() error { r.cacheManager.Close() } + // Spec 110: stop the popularity provider's background fetch workers. + // Safe even if it was never installed (nil) or already closed. + if r.popularityProvider != nil { + _ = r.popularityProvider.Close() + } + // Spec 080 (FR-010, review round 4): the ActivityService owns BBolt // writers — activity records, retention pruning, usage-snapshot flushes, // async sensitive-data detection. The appCancel at the top of Close diff --git a/scripts/test-api-e2e.sh b/scripts/test-api-e2e.sh index 1cba91d66..4e273573a 100755 --- a/scripts/test-api-e2e.sh +++ b/scripts/test-api-e2e.sh @@ -717,6 +717,12 @@ if [ "$LISTEN_PORT" != "8081" ]; then echo "Updated listen port to :${LISTEN_PORT}" fi +# Spec 110 (catalog popularity signal, FR-011 kill switch): E2E must not +# depend on api.github.com (SC-005) — no outbound GitHub fetch, no rate-limit +# flakiness, no network dependency for a suite that otherwise runs fully +# offline against the local server under test. +export MCPPROXY_CATALOG_POPULARITY=false + # Start server in background $MCPPROXY_BINARY serve --config="$CONFIG_FILE" --log-level=info > "/tmp/mcpproxy_e2e.log" 2>&1 & MCPPROXY_PID=$! diff --git a/specs/110-catalog-popularity/plan.md b/specs/110-catalog-popularity/plan.md new file mode 100644 index 000000000..47860b381 --- /dev/null +++ b/specs/110-catalog-popularity/plan.md @@ -0,0 +1,103 @@ +# Implementation Plan: Real Popularity Signal for the Catalog + +**Branch**: `110-catalog-popularity` (stacked on `109-j-catalog-add-server`, PR #1383) | **Spec**: [spec.md](spec.md) + +## Summary + +This plan fills in `Popularity` from two real sources. Docker Hub `pull_count` comes free with the listing the Docker parser already fetches. GitHub `stargazers_count` is fetched through a cached, rate-limited provider that stays off the search's critical path. It also redefines the two landing sections so they differ: Official keeps source order, and Popular is ranked by the signal, drawn from the pool before truncation, and de-duplicated by repo. + +## Technical Context + +Go 1.26, backend-only for PR A. It uses existing dependencies only: `go.etcd.io/bbolt`, `zap`, and `net/http` via `sharedRegistryClient`. **No new dependencies.** Frontend (Vue) and Swift changes are limited to one display helper each (PR B). + +## Research & Decisions + +| # | Decision | Rationale | Rejected alternatives | +|---|---|---|---| +| R1 | **GitHub stars** as the primary signal | `SourceCodeURL` is already filled in for the official-registry and reference sources. MCP directories use it widely. It needs one unauthenticated REST call per repo. | npm downloads (only for npm-packaged stdio servers, and scoped packages cannot use the bulk endpoint). PyPI (pypistats is rate-limited and returns counts on a different scale). | +| R2 | **Docker `pull_count` → `Installs`**, never `Stars` | It costs nothing (already in the payload, verified live). Docker `star_count` is 9–58 while GitHub has ~80k, so mixing them would be meaningless. | Mapping Docker stars → `Stars` (different scale). | +| R3 | **Lexicographic `(stars, installs)`** comparison | The two signals cannot be compared directly. Any conversion factor would be made up, and a review would rightly reject it. A lexicographic order is pure and easy to explain. | A sum (the current code — pulls in the millions would swamp stars). Log-normalized blending (arbitrary weights). | +| R4 | **Own bbolt bucket, not `cache.Manager`** | `cache.Manager` has a fixed 2h TTL (not 6h), deletes expired records (so it cannot serve stale), and adds Spec 105 read_cache authorization frames that are irrelevant here. It would also inflate `read_cache` stats. A small dedicated bucket gets a 24h TTL, serve-stale and ETag storage. | Reusing `cache.Manager` (2h churn × 60/h budget means it never converges). A JSON file in the data dir (another persistence path to maintain, while bbolt is already open). | +| R5 | **Cache-first, with a bounded wait (800 ms) and background completion** | Warm lookups cost 0 ms. A cold landing still gets most of its stars on the first view, and the rest arrive by the next view. The 5 s per-source budget is untouched. | Fully synchronous (adds GitHub latency to every search). Fully async with no wait (the first landing always has an empty Popular). A startup prewarm job (another lifecycle component — deferred; R5 covers most of the value). | +| R6 | **Rolling budget of 50/h unauthenticated + header-driven circuit breaker** | 60/h is per IP, so 10 are left for the user's own unauthenticated `gh`/browser use. `X-RateLimit-Remaining`/`Reset` are authoritative (verified live: `x-ratelimit-limit: 60`, `x-ratelimit-reset`). | A token bucket (more code, and the headers already give the exact state). | +| R7 | **`MCPPROXY_GITHUB_TOKEN`, not `GITHUB_TOKEN`** | Least surprise: a developer's `GITHUB_TOKEN` is often a broad-scope CI or PAT token, so it has to be opted in explicitly. It is only ever sent to the constant host. | Reading `GITHUB_TOKEN`. A config field (5 wiring points; deferred until someone asks). | +| R8 | **Official section = source-native order** | Every default source is official (so all default hits are official), which means rank order would already be popularity order and the sections would be identical by construction. Source order keeps each registry's own curation (the reference list is hand-ordered). | Alphabetical (arbitrary, starts at "a…"). Excluding Popular items from Official (hides official servers). | +| R9 | **Popular: one hit per repo** | The 7 reference servers share `modelcontextprotocol/servers` and would otherwise take 7 of the 12 slots. | Counting no stars for monorepo subpaths (drops genuinely popular servers). | + +## Data Model + +```go +// types.go +type ServerEntry struct { …; Popularity *Popularity `json:"-"` } // FR-001: source-native, never marshalled + +// popularity.go +type PopularityProvider interface { + // Lookup returns the cached stars for a repo key without I/O. stale=true + // means past TTL (still usable); ok=false means never fetched / negative. + Lookup(key string) (stars int, state LookupState) // Fresh | Stale | Negative | Absent + // Resolve enqueues misses/stale keys (in priority order) and waits up to + // wait (bounded by ctx) for them; fetching continues after it returns. + Resolve(ctx context.Context, keys []string, wait time.Duration) +} + +// popularity_github.go +type githubStarsProvider struct { + mu sync.Mutex + entries map[string]*starsEntry // memory front + store starsStore // nil = memory only; bbolt impl + queue chan string // cap 256 + queued map[string]chan struct{} // dedup + completion signal + budget rollingBudget // 50/h or 4000/h + pausedTil time.Time // breaker + token string + baseURL string // const; test hook overrides + now func() time.Time // test clock +} +type starsEntry struct { + Stars int `json:"stars"` + ETag string `json:"etag,omitempty"` + FetchedAt time.Time `json:"fetched_at"` + Status int `json:"status"` // 200/304 ok, 404/451 negative, else error +} +``` + +bbolt bucket `catalog_popularity`: key = `o/r` (lower-case), value = JSON `starsEntry`. The whole bucket is loaded into memory when the provider is constructed, trimmed to the cap if needed, so the cap bounds the disk, not only the memory. Lazy loading was dropped in review: after a restart the in-memory count started at zero and the bucket grew without limit. Writes go through synchronously after each fetch (they are rare, ≤50/h). A new key over the cap evicts the oldest `FetchedAt`. + +## Flow + +``` +SearchAll(q) + fan-out (5s/source) ─► BuildCatalogHit: hit.Popularity = entry.Popularity (docker pulls) + + cache-only Lookup(GitHubRepoKey(entry.SourceCodeURL)) + merge + dedup + source filter ─► pool // merge order = source order + official := copy of official hits in pool, merge order // BEFORE any sort (FR-005) + sort pool by Rank; keys := repo keys (rank order) whose Lookup is stale/absent + provider.Resolve(ctx, keys, opts.PopularityWait) // ctx bounds the wait only; workers use provider ctx + re-apply Lookup to pool + official hits; re-sort pool by Rank + sections = {Official: official[:12], Popular: popular(pool)} // BEFORE truncation + results = pool[:limit] +``` + +## Wiring + +| Site | Change | +|---|---| +| `internal/runtime/runtime.go` (near `cache.NewManager`, ~L290) | `registries.SetPopularityProvider(registries.NewGitHubStarsProvider(registries.PopularityOptions{DB: storageManager.GetDB(), Logger: logger}))` unless the kill switch is set. `Close()` on shutdown stops the workers. | +| `cmd/mcpproxy/catalog_cmd.go` `catalogSearch` in-process fallback and `catalog show` | Memory-only provider (`DB: nil`). | +| `internal/registries/testhooks.go` | `SetPopularityProviderForTest`, `SetGitHubAPIBaseForTest`. | +| E2E | `scripts/test-api-e2e.sh` exports `MCPPROXY_CATALOG_POPULARITY=false`, so E2E never calls api.github.com. | + +## PR slicing + +- **PR A (backend, this branch)**: FR-001…FR-011, SC-001…SC-005. Base `109-j-catalog-add-server`. +- **PR B (display)**: FR-012. Web UI `CatalogSearch.vue` badge + vitest, Swift `CatalogView` badge + `CatalogTests`, CLI table column. Stacked on A. + +## Risks + +- **Stacking on an open PR.** 109-j is still in review, and round 5 made local-only changes (`16f9e8c9c` is not pushed). PR A touches `catalog.go` `SearchAll`/`buildSections`, so it may conflict when 109-j's later rounds land. Mitigation: keep catalog.go edits confined to those two functions plus `BuildCatalogHit`/`Rank`, and rebase once 109-j merges. +- **Section semantics change** (Official is no longer popularity-sorted). This is an intentional amendment of 109 FR-060. Existing 109 tests that assert Official order must be updated to source order, not deleted. + +## Review log + +- **zcode round 1 (spec, 2026-09-26)**: 9 findings, all verified and folded into FR-005/006/007/008/009/011 and the flow above. (1) Official must be built from the pre-sort merge order. (2) The per-source cap limits the pool, so empty q now fans out at 50/source, and the Docker single page is a documented limitation. (3) Background fetch uses the provider ctx, not the request ctx. (4) Overflow must not leave dedup entries behind. (5) No wait while paused or over budget. (6) Errors keep the last-known stars. (7) Lookup distinguishes negative from absent. (8) Own request, 10 s ctx timeout, never `registryGet`, no retries. (9) Kill switch lives in the constructor. +- **Opus subagent round 2 (code, 2026-09-26; zcode, opencode and codex were all out of quota)**: 4 findings, all verified and fixed, each with a regression test shown to fail without the fix. (1) A 200 that is oversized, not JSON, or has no `stargazers_count` is now a failed fetch, so it keeps the last-known stars and ETag. (2) After `Close`, workers drop buffered keys and shutdown-cancelled requests are not stored. (3) `applyCachedStars` falls back to source-native stars when a refresh turns a hit Negative. (4) A closed provider's `Resolve` returns at once, and `Close` wakes pending waiters. diff --git a/specs/110-catalog-popularity/spec.md b/specs/110-catalog-popularity/spec.md new file mode 100644 index 000000000..60b120e22 --- /dev/null +++ b/specs/110-catalog-popularity/spec.md @@ -0,0 +1,95 @@ +# Feature Specification: Real Popularity Signal for the Catalog + +**Feature Branch**: `110-catalog-popularity` +**Created**: 2026-09-26 +**Status**: Draft +**Input**: User description: "The Popularity struct and CatalogHit.Popularity are declared and consumed by Rank()'s popularity tiebreak and buildSections' Popular landing section, but no production code path ever populates them. The FR-060 tiebreak always compares 0-vs-0 and the empty-query Popular section renders identically to Official. Spec and implement a real popularity signal: pick a concrete source (GitHub stars via SourceCodeURL), design fetch + cache + rate-limit handling, wire it into BuildCatalogHit, and add tests proving Popular differs meaningfully from Official once real data exists." + +**Related**: Spec 109 (UX navigation consistency) FR-060 / FR-061 and data-model §9, PR #1383 (`109-j-catalog-add-server`, the branch this stacks on). The finding was raised and deferred twice in 109-j review rounds (1 and 5) as out of scope for a review fix. This spec amends 109 FR-060's definition of the landing sections. The rest of 109 is unchanged. + +## Context & Motivation + +These facts were checked at `origin/109-j-catalog-add-server` `b1ddae539`: + +1. **Nothing sets `Popularity`.** `BuildCatalogHit` (`internal/registries/catalog.go:227`) does not set it. `ServerEntry` (`types.go`) has no field for it, and none of the parsers (`official.go`, `reference.go`, `search.go` `parseDocker`/`parsePulse…`) read one. Every `popularityScore` is therefore 0. +2. **The Popular section is built from a truncated list.** `SearchAll` truncates the ranked pool to `limit` *before* it calls `buildSections(all)` (`catalog.go:190-199`). `Rank` sorts official hits first, so once there are `limit` or more official hits, Popular is a reordering of the same hits Official already shows. Real stars alone would not change that. +3. **Every default source is official, so the two sections cannot differ.** All three default sources (`official`, `reference`, `docker-mcp-catalog`, `internal/config/config.go:1664`) have `Provenance: official`, which means every default hit has `Official = Verified = true`. `Rank`'s third key is popularity, so the Official section ("official hits in rank order") is *already ordered by popularity*. On a default install, Popular would equal Official in both membership and order even with correct star counts. +4. **The data exists at no cost for one source.** Docker Hub's `GET /v2/repositories/mcp/` response, which `parseDocker` already fetches, includes `pull_count` for every image (checked live 2026-09-26: `mcp/fetch` 1,863,661 pulls; 245 images). The parser throws it away. +5. **Most other hits have a GitHub URL.** Official-registry entries carry `repository.url` → `SourceCodeURL` (`official.go:245`), and every reference server links into `github.com/modelcontextprotocol/servers`. GitHub's REST API (`GET /repos/{owner}/{repo}` → `stargazers_count`) allows **60 requests/hour per IP without a token** and 5,000 with one. +6. **The suggested cache does not fit.** `cache.Manager` (`internal/cache/manager.go`) stores `read_cache` tool responses. Its TTL is a fixed `DefaultTTL = 2h`, not 6h, and cannot be set per record. Its records carry Spec 105 authorization frames, and its cleanup sweep deletes expired entries. Refreshing every star count every 2h would exceed the unauthenticated budget with ~120 repos, and deleting expired entries rules out serve-stale. +7. **No surface shows the numbers yet.** The Web UI (`CatalogSearch.vue`), macOS (`CatalogView.swift`) and the CLI table do not render `popularity`, even though 109 FR-061 says each result shows "stars or installs when the source provides them". + +## Scope Boundary + +| Already exists | Reused, not rebuilt | +|---|---| +| `Popularity{Stars,Installs}`, `CatalogHit.Popularity`, `CatalogResult.Popularity`, the MCP `mcpCatalogServerEntry.Popularity`, TS `CatalogPopularity`, Swift `CatalogPopularity` | Types and wire shape unchanged (`popularity: {stars?, installs?}`, omitempty) | +| `Rank` (FR-060 ordering) | Keys unchanged. Only the popularity comparison changes from "sum" to "stars, then installs" (FR-004) | +| Registry SSRF-hardened client (`sharedRegistryClient`: dial-time private-IP block, redirect host pin, body cap) | Used for the GitHub fetch as is | +| `registries` package-level wiring (`SetVersion`, `SetRegistriesFromConfig`, `SetRegistriesForTest`) | The popularity provider is installed the same way (FR-010) | +| BBolt `config.db` via `storageManager.GetDB()` | A new bucket `catalog_popularity`. `cache.Manager` is not reused (fact 6) | + +**Out of scope**: npm/PyPI download counts (a follow-up could add them as more `Installs` sources), GitHub GraphQL batching (requires a token), a config-file knob (env only in v1, see FR-011), telemetry, and changes to `Rank`'s key order. + +## User Scenarios & Testing + +### User Story 1 — Popular means popular (Priority: P1) + +A user opens the Catalog with no query and sees an **Official** section (what the official sources list, in their own order) and a **Popular** section (the most-starred or most-pulled servers across every source). The two sections are visibly different lists. + +**Independent test**: use fixture sources with ≥12 official hits and known star/pull counts. Popular should be ordered by popularity, should include a high-star hit that falls outside Official's first 12, and should differ from Official in membership or order. + +1. **Given** real popularity for at least one hit, **When** q is empty, **Then** Popular contains only hits with a known signal, ordered stars desc, then installs desc, then `Rank`. It is drawn from the full merged pool before `limit` truncation, has at most one hit per GitHub repository, and is capped at 12. +2. **Given** no popularity is known for any hit (cold cache, GitHub unreachable, no Docker source), **When** q is empty, **Then** Popular is **empty**, not padded with zero-popularity hits, and each surface hides the empty section (Web UI and macOS already do). +3. **Given** q is empty, **Then** Official lists official-source hits in **source order**: registry list order, then each source's native order. It does not re-rank by popularity and is capped at 12. + +### User Story 2 — Popularity breaks ties in search results (Priority: P1) + +**Given** the query "github" matches several official hits, **Then** hits with more stars rank above those with fewer (FR-060 key 3), and the returned `popularity` shows the numbers. + +### User Story 3 — It never makes the catalog slower or flakier (Priority: P1) + +1. **Given** GitHub is slow or down, **Then** `SearchAll` returns within its existing budget plus at most the popularity wait (FR-007), and the affected hits simply have no stars. GitHub is never listed in `unavailable[]`, because it is not a catalog source. +2. **Given** the unauthenticated rate limit is exhausted, **Then** no further GitHub requests go out until the reset time, and cached values, including expired ones, keep being served. + +### User Story 4 — Users see the numbers (Priority: P2) + +The Web UI result row, the macOS catalog row and the CLI `catalog search` table show `★ 21.3k` or `⤓ 1.2M` when popularity is known, and nothing when it is not. + +### Edge cases + +- SourceCodeURL forms: `https://github.com/o/r`, `…/r.git`, `…/r/tree/main/src/x` (monorepo subpath), `git+https://…`, `http://`, `www.github.com`, a trailing slash, and mixed case. All of them normalize to the key `o/r` (lower-cased). Non-GitHub hosts, gist URLs, and owners or repos that fail GitHub's name rules produce no key and never cause a fetch. +- A monorepo (`modelcontextprotocol/servers`) gives its star count to every server that links into it. Popular shows only one of them (the first by `Rank`). Search tiebreaks still use the repo's count. +- A repo that returns 404 or 451 is negatively cached and shows no stars. A renamed repo's 301 (`/repositories/{id}`, same host) is followed, because the registry client pins redirects to the host only. +- The same `(source,id)` from two sources is already de-duplicated by `SearchAll`. The same repo from two different sources is fetched once, via one cache key. + +## Requirements + +### Functional Requirements + +- **FR-001 (source-native signal)**: `parseDocker` MUST set `Installs = pull_count` when the field is a non-negative number. Docker Hub `star_count` MUST NOT be mapped to `Stars`, because it is on a different scale from GitHub stars. The value travels on a new `ServerEntry.Popularity *Popularity` field tagged `json:"-"`, so `ServerEntry`'s JSON (GET `/registries/{id}/servers`, `search_servers` with `registry`) stays unchanged. +- **FR-002 (BuildCatalogHit)**: `BuildCatalogHit` MUST copy `entry.Popularity` into the hit and fill in `Stars` from the installed provider's **cache only**, with no network call. It stays synchronous and never blocks. +- **FR-003 (repo key)**: A pure `GitHubRepoKey(sourceCodeURL) (key string, ok bool)` MUST normalize the edge-case forms above. Owner: `^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$`. Repo: `^[A-Za-z0-9._-]{1,100}$`, excluding `.` and `..`. Any other input returns `ok=false`. +- **FR-004 (comparison)**: `Rank` and the Popular ordering MUST compare popularity as `(stars desc, installs desc)`, with missing = 0. Stars and installs are never added together or converted into each other. +- **FR-005 (sections)**: `buildSections` MUST take the full merged, de-duplicated, source-filtered pool **before** `limit` truncation. For an empty q, each source is fanned out with the per-source maximum (50) rather than the caller's `limit`, so the pool is as wide as each source will give. Official = official hits in merge (source-native) order, at most 12. This order MUST come from a copy of the pool taken **before** any `Rank` sort. With an empty q and all-official defaults, rank order *is* popularity order, so building Official from the ranked pool would bring back the bug this spec fixes. Popular = hits with `stars>0 ∨ installs>0`, sorted per FR-004 and then `Rank`, at most one per GitHub repo key, at most 12. This amends Spec 109 FR-060 and data-model §9. *Known limitation*: the Docker source fetches a single page of `hub.docker.com/v2/repositories/mcp/`, which is 10 images by default. Popular therefore ranks only within what each source returns, and paginating Docker is a follow-up. +- **FR-006 (fetch)**: The GitHub fetcher MUST send `GET https://api.github.com/repos/{owner}/{repo}` through the SSRF-hardened registry client. The host is a compile-time constant, overridable only by a test hook. The request carries `Accept: application/vnd.github+json`, `X-GitHub-Api-Version: 2022-11-28`, the versioned mcpproxy User-Agent, and `If-None-Match` when an ETag is cached. The response body is capped at 1 MiB and each request has a 10 s timeout. Only `stargazers_count` and `ETag` are read. The fetcher sends its own request on `sharedRegistryClient()`, with a per-request `context.WithTimeout(10s)` because the client's own timeout is 45 s. It MUST NOT use `registryGet`: that pins to `reg.ServersURL`, forces its own headers, and retries 429/5xx three times, which would burn budget against a rate-limited API. There are no retries; the breaker (FR-009) is the only backoff. +- **FR-007 (never block the search)**: `SearchAll` MUST resolve popularity *after* the per-source fan-out. Cached values come back immediately. Misses are queued for a background fetch, and `SearchAll` waits for them for at most `SearchOptions.PopularityWait`: 0 (unset) means the default of 800 ms, a negative value disables the wait, and the wait never outlives `ctx`. Misses still outstanding keep being fetched after `SearchAll` returns, so the next search has them. The caller's ctx bounds **only the wait**. Workers fetch under a context owned by the provider, created at construction and cancelled by `Close()`. `Resolve` returns immediately, without waiting, when the provider is paused (breaker), its budget is used up, or none of the requested keys were admitted to the queue. It waits only on the keys it admitted. Popularity failures never add entries to `unavailable[]`. +- **FR-008 (cache)**: Each entry `{stars, etag, fetched_at, status}` MUST be kept in memory and persisted to the bbolt bucket `catalog_popularity` when a store is attached. The entry is fresh for 24 h after a 200 or 304, 24 h after a 404 or 451 (negative), and 1 h after any other error. Expired entries are still served (stale-while-revalidate) and queued for refresh. A failed refresh (a transport error or any status except 200/304/404/451) MUST keep the last-known `stars`/`etag` and only move the revalidate time forward. Stars are cleared only on a definitive 404/451. `Lookup` MUST tell apart *fresh*, *stale*, *negative* (fresh 404/451) and *absent*. `Resolve` enqueues only *stale* and *absent* keys, so a dead repo is not fetched again until its negative TTL runs out. The store is capped at 5,000 keys, evicting the oldest `fetched_at` first. +- **FR-009 (rate limit)**: The fetcher MUST (a) run at most 4 requests at once; (b) de-duplicate in-flight and queued keys, with a queue capped at 256. A key that overflows is never recorded as queued (or is removed and its completion signal closed), so nothing waits on it and the next search can request it again; (c) spend at most 50 requests per rolling hour without a token and 4,000 with one; (d) stop all fetches until `X-RateLimit-Reset` when `X-RateLimit-Remaining` ≤ 5, or on 403/429 with `Retry-After` or `X-RateLimit-Remaining: 0` (using `Retry-After`, or 60 s when neither header is present); (e) fetch misses in the order `SearchAll` ranked them. +- **FR-010 (wiring)**: `registries.SetPopularityProvider(p)` installs the process-wide provider, and `nil` turns lookups off. The core runtime installs a provider backed by bbolt at startup. The CLI in-process fallback (`catalogSearch` when no daemon is running, and `catalog show`) installs a memory-only one. Tests use `SetPopularityProviderForTest`, which returns a restore func. +- **FR-011 (token & kill switch)**: If `MCPPROXY_GITHUB_TOKEN` is set, it is sent as `Authorization: Bearer …`, only to the constant GitHub API host. The generic `GITHUB_TOKEN` is deliberately **not** read, to avoid leaking an unrelated CI or dev token. `MCPPROXY_CATALOG_POPULARITY=false` (or `0`/`off`) turns off all outbound popularity fetches. The switch is read inside the provider constructor, which then returns a no-op provider, so the runtime and CLI call sites both honour it. Source-native Docker pulls still show. +- **FR-012 (display)**: The Web UI, macOS and CLI table MUST show compact popularity (`★ 1.2k`, `⤓ 3.4M`) when it is known. The REST, MCP and CLI JSON shapes are unchanged. + +### Success Criteria + +- **SC-001**: A fixture test with 14 official hits (one repo with the most stars sits outside the first 12 in source order) and one non-official Docker hit with pulls must show three things. Popular ≠ Official. Popular[0] is the most-starred hit. Popular contains a hit that Official does not. +- **SC-002**: With no signal known, Popular is empty and Official is unchanged. +- **SC-003**: With a stub GitHub server that sleeps 5 s, `SearchAll` returns within `PopularityWait` + 100 ms of the fan-out finishing, and a second call after the stub answers carries the stars. +- **SC-004**: After a 403 response with `X-RateLimit-Remaining: 0` and a reset time in the future, no further requests reach the stub before that reset time, and cached (stale) values are still returned. +- **SC-005**: Personal and server builds pass lint, and `go test -race ./internal/registries/...` passes. `./scripts/test-api-e2e.sh` passes with popularity turned off, since E2E must not depend on api.github.com. + +## Assumptions + +- Stars are an adequate proxy for "popular" in the MCP ecosystem. The GitHub star count is the signal most widely used by MCP directories (Glama, PulseMCP, mcp.so). +- 50 requests per hour covers a default catalog (~60–120 distinct repos) within about 2 hours of the first run, and the persisted cache means a restart costs nothing. Users who want it faster can set `MCPPROXY_GITHUB_TOKEN`. +- Repository slugs sent to api.github.com are public catalog data, not user data. The kill switch exists for air-gapped or offline setups. diff --git a/specs/110-catalog-popularity/tasks.md b/specs/110-catalog-popularity/tasks.md new file mode 100644 index 000000000..6fa80cc78 --- /dev/null +++ b/specs/110-catalog-popularity/tasks.md @@ -0,0 +1,41 @@ +# Tasks: Real Popularity Signal for the Catalog + +**Input**: [spec.md](spec.md), [plan.md](plan.md). TDD: every task writes its failing test first. + +## Phase 1 — Pure pieces (PR A) + +- [x] T001 [P] [US1] `GitHubRepoKey` + table test covering every edge-case form in the spec (`internal/registries/popularity_key.go`, `popularity_key_test.go`) — FR-003 +- [x] T002 [P] [US1] `parseDocker` maps `pull_count` → `ServerEntry.Popularity.Installs`, ignores `star_count`, and ignores negative or non-numeric values. Add `ServerEntry.Popularity json:"-"` plus a test that `ServerEntry` JSON is unchanged (`search.go`, `types.go`, `search_test.go`) — FR-001 +- [x] T003 [US2] Replace `popularityScore` sum with `comparePopularity` using (stars, installs). Add rank tests: stars beat installs, installs break ties between equal stars, existing tests stay green (`catalog.go`, `rank_test.go`) — FR-004 + +## Phase 2 — Provider (PR A) + +- [x] T004 [US3] `PopularityProvider` interface, `SetPopularityProvider`, `SetPopularityProviderForTest`, `SetGitHubAPIBaseForTest` (`popularity.go`, `testhooks.go`) — FR-010 +- [x] T005 [US3] `githubStarsProvider` fetch against an httptest stub. Cover: 200 (stars + ETag stored), 304 (refresh `FetchedAt`, keep stars), 404 (negative), 500 (1h error TTL), exact request headers, bearer only when `MCPPROXY_GITHUB_TOKEN` is set, body cap, and a 301 redirect on the same host (`popularity_github.go`, `popularity_github_test.go`) — FR-006, FR-011 +- [x] T006 [US3] TTL, serve-stale and requeue via an injected clock. Cover dedup of queued/in-flight keys, queue overflow drop, concurrency ≤4 (stub counts peak concurrency), rolling budget of 50/h, breaker on `Remaining ≤ 5`, and 403/429 with `Retry-After` (SC-004) — FR-008, FR-009 +- [x] T007 [US3] bbolt store: round-trip, lazy load, cap eviction, and surviving a provider restart (a temp DB) (`popularity_store.go`, `popularity_store_test.go`) — FR-008 +- [x] T008 [US3] `Resolve` wait bounds: returns by `wait` and by `ctx` cancel, fetches continue afterwards, and `wait=0` never blocks (SC-003) — FR-007 +- [x] T009 [US3] Kill switch: with `MCPPROXY_CATALOG_POPULARITY=false` no request ever reaches the stub — FR-011 + +## Phase 3 — Catalog integration (PR A) + +- [x] T010 [US1] `BuildCatalogHit` copies `entry.Popularity` and adds cache-only stars (`catalog.go`, `catalog_popularity_test.go`) — FR-002 +- [x] T011 [US1] `SearchAll`: add `SearchOptions.PopularityWait` (default 800 ms), Resolve after the fan-out, re-apply and re-rank, and build sections from the pool before truncation — FR-005, FR-007 +- [x] T012 [US1] `buildSections`: Official in source order, Popular signal-only, one per repo, capped at 12. **SC-001 and SC-002 tests** (fixture registries through `SetRegistriesForTest` + the stub provider) +- [x] T013 [US3] SearchAll with a GitHub stub that sleeps 5 s: returns within the wait, the second call has the stars, and `unavailable[]` is untouched (SC-003) +- [x] T014 Update any 109 test that asserted Official in popularity/rank order so it asserts source order instead (`internal/registries/*_test.go`, `internal/httpapi/catalog*_test.go`, `cmd/mcpproxy/catalog_cmd*_test.go`) + +## Phase 4 — Wiring (PR A) + +- [x] T015 Runtime installs the bbolt-backed provider and closes it on shutdown (`internal/runtime/runtime.go`) +- [x] T016 CLI in-process fallback and `catalog show` install the memory provider (`cmd/mcpproxy/catalog_cmd.go`) +- [x] T017 `scripts/test-api-e2e.sh` exports `MCPPROXY_CATALOG_POPULARITY=false` +- [x] T018 Docs: `docs/features/` catalog page — popularity sources, `MCPPROXY_GITHUB_TOKEN`, kill switch +- [x] T019 Verify: both golangci-lint runs (bare + `--build-tags server`), `go test -race ./internal/registries/... ./internal/httpapi/... ./cmd/mcpproxy/...`, and `./scripts/test-api-e2e.sh` + +## Phase 5 — Display (PR B, stacked) + +- [ ] T020 [P] [US4] Web UI: `formatPopularity` util + vitest (`frontend/tests/unit/`) + badge in `CatalogSearch.vue` result row +- [ ] T021 [P] [US4] macOS: badge in `CatalogView` row + `CatalogTests` formatting test +- [ ] T022 [P] [US4] CLI: `catalog search` table shows a POPULARITY column +- [ ] T023 [US4] Playwright web-ui sweep of the catalog landing with a seeded stub From 3b27b778a2ebaf15feaa6146884e21a86c54a7ee Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 28 Sep 2026 15:33:08 +0300 Subject: [PATCH 2/2] test(catalog): complete popularity coverage and docs --- docs/features/catalog-popularity.md | 32 ++++++++++++++++++++++ docs/registries.md | 33 ++--------------------- internal/registries/search_test.go | 42 +++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 31 deletions(-) create mode 100644 docs/features/catalog-popularity.md diff --git a/docs/features/catalog-popularity.md b/docs/features/catalog-popularity.md new file mode 100644 index 000000000..640cb328c --- /dev/null +++ b/docs/features/catalog-popularity.md @@ -0,0 +1,32 @@ +--- +title: Catalog popularity +sidebar_label: Catalog popularity +description: How MCPProxy ranks catalog results using GitHub stars and Docker Hub pull counts. +--- + +# Catalog popularity + +Catalog popularity uses real, source-native signals. It does not synthesize or +combine counts from different services: + +- **GitHub stars** are fetched for catalog entries whose source-code URL points + to a GitHub repository. The provider caches results for 24 hours and uses + ETag revalidation. It allows at most four concurrent requests and applies a + rolling request budget of 50 per hour without a token or 4,000 per hour with + one. +- **Docker Hub installs** come from the `pull_count` field in the + `docker-mcp-catalog` listing. Docker's `star_count` is ignored; it is not + comparable to GitHub stars. + +On a cold cache, a catalog search waits for at most 800 ms by default. Missing +GitHub values are fetched in the background and can appear in a later search. +GitHub is not a catalog source, so GitHub request failures do not add an entry +to the search response's `unavailable` list. + +Set **`MCPPROXY_GITHUB_TOKEN`** to raise the GitHub request budget. The generic +`GITHUB_TOKEN` environment variable is not read for this purpose. + +Set **`MCPPROXY_CATALOG_POPULARITY=false`** (or `0`/`off`) to disable outbound +GitHub requests, for example in an offline environment. Docker pull counts +remain available because they are part of the Docker catalog listing already +being fetched. diff --git a/docs/registries.md b/docs/registries.md index 7586dd5ba..ade5dbbdf 100644 --- a/docs/registries.md +++ b/docs/registries.md @@ -221,37 +221,8 @@ identically across all surfaces. ## Catalog popularity signal -The catalog's `Popular` landing section (and the popularity tiebreak in -search ranking) is backed by two real, source-native signals — no synthetic -scoring: - -- **GitHub stars** for any hit whose `source_code_url` resolves to a - `github.com//` URL (the official registry and reference - sources both carry one for almost every entry). Fetched from - `GET https://api.github.com/repos/{owner}/{repo}`, cached (with ETag - revalidation) for 24h, capped at 4 concurrent requests and a rolling - budget of 50 requests/hour without a token or 4,000/hour with one. -- **Docker Hub pull counts** (`pull_count` from the `docker-mcp-catalog` - registry's listing) map to `Installs`. Docker's own `star_count` is never - used — it lives on a different scale from GitHub stars (single digits vs. - tens of thousands) and the two are never summed or converted into one - another. - -Popularity fetching never blocks a search beyond a short bounded wait -(800ms by default): a cold miss is queued for a background fetch and simply -shows no stars on the current page, arriving on the next one. GitHub is -never listed in a search's `unavailable[]` — it is not a catalog source. - -**`MCPPROXY_GITHUB_TOKEN`** (not the generic `GITHUB_TOKEN`, which is -deliberately never read) raises the GitHub rate-limit budget from 50 to -4,000 requests/hour. Set it if your catalog has many distinct repositories -and stars are taking a while to fill in. - -**`MCPPROXY_CATALOG_POPULARITY=false`** (or `0`/`off`) disables all outbound -GitHub requests — useful for air-gapped or offline setups. Docker's -source-native install counts are unaffected either way, since they come from -the registry listing the Docker source already fetches, not from a separate -popularity call. +Catalog ordering, GitHub stars, Docker pull counts, rate limits, and opt-out +behavior are described in [Catalog popularity](features/catalog-popularity.md). ## Adding a discovered server diff --git a/internal/registries/search_test.go b/internal/registries/search_test.go index cc1e184cf..f3f0b97c2 100644 --- a/internal/registries/search_test.go +++ b/internal/registries/search_test.go @@ -279,6 +279,48 @@ func TestParseDocker(t *testing.T) { } } +func TestParseDockerPopularitySignalsAndJSONShape(t *testing.T) { + testData := map[string]interface{}{ + "results": []interface{}{ + map[string]interface{}{"name": "positive", "pull_count": float64(1234), "star_count": float64(9999)}, + map[string]interface{}{"name": "negative", "pull_count": float64(-1), "star_count": float64(42)}, + map[string]interface{}{"name": "non-numeric", "pull_count": "many", "star_count": float64(7)}, + map[string]interface{}{"name": "stars-only", "star_count": float64(88)}, + }, + } + + servers := parseDocker(testData) + if len(servers) != 4 { + t.Fatalf("expected 4 parsed Docker servers, got %d", len(servers)) + } + + if got := servers[0].Popularity; got == nil || got.Installs == nil || *got.Installs != 1234 { + t.Fatalf("expected pull_count 1234 to map to Installs, got %+v", got) + } + if servers[0].Popularity.Stars != nil { + t.Errorf("Docker star_count must not be mapped to GitHub Stars, got %d", *servers[0].Popularity.Stars) + } + for _, server := range servers[1:] { + if server.Popularity != nil { + t.Errorf("expected no popularity for %q with invalid or absent pull_count, got %+v", server.ID, server.Popularity) + } + } + + withPopularity, err := json.Marshal(servers[0]) + if err != nil { + t.Fatalf("marshal parsed ServerEntry: %v", err) + } + withoutPopularity := servers[0] + withoutPopularity.Popularity = nil + wantJSON, err := json.Marshal(withoutPopularity) + if err != nil { + t.Fatalf("marshal baseline ServerEntry: %v", err) + } + if !assert.JSONEq(t, string(wantJSON), string(withPopularity)) { + t.Errorf("source-native popularity changed the ServerEntry JSON shape: got %s, want %s", withPopularity, wantJSON) + } +} + func TestDerivePulseServerDetails(t *testing.T) { tests := []struct { name string