From 6ab2aeaf743d6e63b70f87d9952e87ffe41c119d Mon Sep 17 00:00:00 2001 From: hive-quality Date: Sat, 26 Sep 2026 14:27:55 -0400 Subject: [PATCH] [quality] cover match engine gaps: ScoreForRepo, PersistRematchResults, LLMFromEnv Adds engine_gap_test.go covering previously 0% functions in pkg/match: - ScoreForRepo: fresh score + persist + notify, cache hit (no gateway traffic), RepoHash invalidation replacing in place, append for new repos, weak-score notify suppression, store error propagation - PersistRematchResults: happy path with threshold-gated notifications, ErrRepoChanged on stale hash or vanished repo, ErrIdeaChanged on concurrent idea edit - LLMFromEnv/envValue: disabled without base URL, trailing-slash trim, default model, legacy IDEATE_ prefix fallback, DIBS_ precedence - firstNonEmpty and nonHiveCNCF (registry filtering, nil registry) pkg/match coverage: 70.5% -> 85.8%. Test-only change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: hive-quality --- pkg/match/engine_gap_test.go | 301 +++++++++++++++++++++++++++++++++++ 1 file changed, 301 insertions(+) create mode 100644 pkg/match/engine_gap_test.go diff --git a/pkg/match/engine_gap_test.go b/pkg/match/engine_gap_test.go new file mode 100644 index 0000000..7288c7c --- /dev/null +++ b/pkg/match/engine_gap_test.go @@ -0,0 +1,301 @@ +package match + +// Gap-closing tests for previously uncovered pkg/match functions: +// ScoreForRepo (repo-side feed scoring + cache replace/append + notify), +// PersistRematchResults (ErrRepoChanged / ErrIdeaChanged / notify), +// LLMFromEnv + envValue (env config incl. legacy IDEATE_ prefix), +// firstNonEmpty, and nonHiveCNCF registry filtering. + +import ( + "context" + "testing" + "time" + + "github.com/hivecommons/dibs/pkg/registry" + "github.com/hivecommons/dibs/pkg/store" +) + +// notifyRecorder records NewMatch calls. +type notifyRecorder struct { + calls []notifyCall +} + +type notifyCall struct { + IdeaAuthor string + RepoOwner string + RepoID string + Score float64 +} + +func (n *notifyRecorder) NewMatch(ideaAuthor, repoOwner string, idea *store.Idea, repo *registry.RepoProfile, score float64) { + n.calls = append(n.calls, notifyCall{IdeaAuthor: ideaAuthor, RepoOwner: repoOwner, RepoID: repo.RepoID, Score: score}) +} + +// TestScoreForRepoFreshCachedAndInvalidated: a fresh score is computed, +// persisted onto the idea, and notified when strong; a second call is served +// from the cache with zero gateway traffic; a repo-profile change invalidates +// the hash and the rescore REPLACES the cached entry instead of appending. +func TestScoreForRepoFreshCachedAndInvalidated(t *testing.T) { + st, reg := newFixtures(t) + if err := reg.Merge([]registry.RepoProfile{{RepoID: "org/feed", Owner: "bob"}}); err != nil { + t.Fatalf("Merge: %v", err) + } + srv, calls := fakeGateway(t, func(string) string { return `{"score": 90, "reason": "strong fit"}` }) + rec := ¬ifyRecorder{} + e := &Engine{Store: st, Registry: reg, LLM: &LLM{BaseURL: srv.URL + "/v1", Model: "test"}, Notifier: rec} + idea := createIdea(t, st, "T", "B body") + rp, err := reg.Get("org/feed") + if err != nil { + t.Fatalf("Get: %v", err) + } + + m, err := e.ScoreForRepo(context.Background(), idea, rp) + if err != nil { + t.Fatalf("ScoreForRepo: %v", err) + } + if m.Score != 90 || !m.ByLLM || m.RepoID != "org/feed" { + t.Fatalf("fresh match: %+v", m) + } + // 90 >= NotifyThreshold: both sides are told. + if len(rec.calls) != 1 || rec.calls[0].RepoID != "org/feed" || rec.calls[0].IdeaAuthor != "alice" || rec.calls[0].RepoOwner != "bob" || rec.calls[0].Score != 90 { + t.Fatalf("notify calls: %+v", rec.calls) + } + // Persisted onto the idea record. + idea, _ = st.Get(idea.ID) + if len(idea.Matches) != 1 || idea.Matches[0].RepoID != "org/feed" || idea.MatchesUpdatedAt.IsZero() { + t.Fatalf("persisted matches: %+v", idea.Matches) + } + + // Cached: same hash, no gateway call, no extra notify. + before := calls.Load() + m2, err := e.ScoreForRepo(context.Background(), idea, rp) + if err != nil { + t.Fatalf("ScoreForRepo (cached): %v", err) + } + if calls.Load() != before { + t.Fatalf("cached score must not hit the gateway (calls %d → %d)", before, calls.Load()) + } + if m2.RepoHash != m.RepoHash || len(rec.calls) != 1 { + t.Fatalf("cached result changed: %+v, notifies %d", m2, len(rec.calls)) + } + + // Repo profile edit invalidates; the rescore replaces the entry in place. + if _, err := reg.ApplyOwnerUpdate("org/feed", "bob", registry.OwnerUpdate{Appetite: strPtr("new appetite")}); err != nil { + t.Fatalf("ApplyOwnerUpdate: %v", err) + } + rp, _ = reg.Get("org/feed") + idea, _ = st.Get(idea.ID) + m3, err := e.ScoreForRepo(context.Background(), idea, rp) + if err != nil { + t.Fatalf("ScoreForRepo (invalidated): %v", err) + } + if calls.Load() == before { + t.Fatal("repo edit must invalidate the cached score") + } + if m3.RepoHash == m.RepoHash { + t.Fatalf("hash must change with the profile: %q", m3.RepoHash) + } + idea, _ = st.Get(idea.ID) + if len(idea.Matches) != 1 || idea.Matches[0].RepoHash != m3.RepoHash { + t.Fatalf("rescore must replace, not append: %+v", idea.Matches) + } +} + +// TestScoreForRepoAppendsNewRepo: scoring a second repo appends alongside the +// first instead of overwriting it, and a weak score never notifies. +func TestScoreForRepoAppendsNewRepo(t *testing.T) { + st, reg := newFixtures(t) + if err := reg.Merge([]registry.RepoProfile{ + {RepoID: "org/a", Owner: "bob"}, + {RepoID: "org/b", Owner: "carol"}, + }); err != nil { + t.Fatalf("Merge: %v", err) + } + srv, _ := fakeGateway(t, func(string) string { return `{"score": 10, "reason": "weak"}` }) + rec := ¬ifyRecorder{} + e := &Engine{Store: st, Registry: reg, LLM: &LLM{BaseURL: srv.URL + "/v1", Model: "test"}, Notifier: rec} + idea := createIdea(t, st, "T", "B body") + for _, id := range []string{"org/a", "org/b"} { + rp, err := reg.Get(id) + if err != nil { + t.Fatalf("Get %s: %v", id, err) + } + if _, err := e.ScoreForRepo(context.Background(), idea, rp); err != nil { + t.Fatalf("ScoreForRepo %s: %v", id, err) + } + idea, _ = st.Get(idea.ID) + } + if len(idea.Matches) != 2 { + t.Fatalf("want 2 persisted matches, got %+v", idea.Matches) + } + if len(rec.calls) != 0 { + t.Fatalf("scores below NotifyThreshold must not notify: %+v", rec.calls) + } +} + +// TestScoreForRepoMutateError: an unknown idea surfaces the store error. +func TestScoreForRepoMutateError(t *testing.T) { + st, reg := newFixtures(t) + if err := reg.Merge([]registry.RepoProfile{{RepoID: "org/a", Owner: "bob"}}); err != nil { + t.Fatalf("Merge: %v", err) + } + e := &Engine{Store: st, Registry: reg} + rp, _ := reg.Get("org/a") + ghost := &store.Idea{ID: "no-such-idea", Author: "alice", Title: "T", Body: "B"} + if _, err := e.ScoreForRepo(context.Background(), ghost, rp); err == nil { + t.Fatal("expected error persisting to unknown idea") + } +} + +// TestPersistRematchResultsHappyPathAndNotify: results commit when nothing +// drifted, and strong persisted matches notify while weak ones stay silent. +func TestPersistRematchResultsHappyPathAndNotify(t *testing.T) { + st, reg := newFixtures(t) + if err := reg.Merge([]registry.RepoProfile{ + {RepoID: "org/strong", Owner: "bob"}, + {RepoID: "org/weak", Owner: "carol"}, + }); err != nil { + t.Fatalf("Merge: %v", err) + } + rec := ¬ifyRecorder{} + e := &Engine{Store: st, Registry: reg, Notifier: rec} + idea := createIdea(t, st, "T", "B body") + strongRp, _ := reg.Get("org/strong") + weakRp, _ := reg.Get("org/weak") + matches := []store.Match{ + {RepoID: "org/strong", Score: NotifyThreshold, RepoHash: RepoHash(strongRp)}, + {RepoID: "org/weak", Score: NotifyThreshold - 1, RepoHash: RepoHash(weakRp)}, + } + cncf := []CNCFMatch{{Name: "Istio", RepoID: "istio/istio", Score: 42}} + + if err := e.PersistRematchResults(idea.ID, idea.UpdatedAt, "the tldr", matches, cncf); err != nil { + t.Fatalf("PersistRematchResults: %v", err) + } + got, _ := st.Get(idea.ID) + if got.TLDR != "the tldr" || len(got.Matches) != 2 || len(got.CNCFMatches) != 1 || got.MatchesUpdatedAt.IsZero() { + t.Fatalf("persisted idea: tldr=%q matches=%+v cncf=%+v", got.TLDR, got.Matches, got.CNCFMatches) + } + // Only the >=threshold match notifies. + if len(rec.calls) != 1 || rec.calls[0].RepoID != "org/strong" || rec.calls[0].RepoOwner != "bob" { + t.Fatalf("notify calls: %+v", rec.calls) + } +} + +// TestPersistRematchResultsRepoChanged: a stale RepoHash (or a repo that +// vanished from the registry) aborts the commit with ErrRepoChanged. +func TestPersistRematchResultsRepoChanged(t *testing.T) { + st, reg := newFixtures(t) + if err := reg.Merge([]registry.RepoProfile{{RepoID: "org/a", Owner: "bob"}}); err != nil { + t.Fatalf("Merge: %v", err) + } + e := &Engine{Store: st, Registry: reg} + idea := createIdea(t, st, "T", "B body") + + stale := []store.Match{{RepoID: "org/a", Score: 90, RepoHash: "stale-hash"}} + if err := e.PersistRematchResults(idea.ID, idea.UpdatedAt, "t", stale, nil); err != ErrRepoChanged { + t.Fatalf("stale hash: want ErrRepoChanged, got %v", err) + } + missing := []store.Match{{RepoID: "org/gone", Score: 90, RepoHash: "x"}} + if err := e.PersistRematchResults(idea.ID, idea.UpdatedAt, "t", missing, nil); err != ErrRepoChanged { + t.Fatalf("missing repo: want ErrRepoChanged, got %v", err) + } + got, _ := st.Get(idea.ID) + if got.TLDR != "" || len(got.Matches) != 0 { + t.Fatalf("aborted commit must not persist anything: %+v", got) + } +} + +// TestPersistRematchResultsIdeaChanged: an idea edited mid-rematch aborts the +// commit with ErrIdeaChanged. +func TestPersistRematchResultsIdeaChanged(t *testing.T) { + st, reg := newFixtures(t) + e := &Engine{Store: st, Registry: reg} + idea := createIdea(t, st, "T", "B body") + if err := e.PersistRematchResults(idea.ID, idea.UpdatedAt.Add(-time.Second), "t", nil, nil); err != ErrIdeaChanged { + t.Fatalf("want ErrIdeaChanged, got %v", err) + } +} + +// TestLLMFromEnv: unset base URL disables the LLM; DIBS_ vars build a client +// with a trimmed base URL and the default model; legacy IDEATE_ names are +// honored as fallbacks but never override DIBS_ values. +func TestLLMFromEnv(t *testing.T) { + for _, k := range []string{"DIBS_LLM_BASE_URL", "DIBS_LLM_API_KEY", "DIBS_LLM_MODEL", + "IDEATE_LLM_BASE_URL", "IDEATE_LLM_API_KEY", "IDEATE_LLM_MODEL"} { + t.Setenv(k, "") + } + if l := LLMFromEnv(); l != nil { + t.Fatalf("no base URL must disable the LLM, got %+v", l) + } + + t.Setenv("DIBS_LLM_BASE_URL", "http://litellm:4000/v1/") + l := LLMFromEnv() + if l == nil { + t.Fatal("expected an LLM") + } + if l.BaseURL != "http://litellm:4000/v1" { + t.Fatalf("trailing slash must be trimmed: %q", l.BaseURL) + } + if l.Model != DefaultModel || l.APIKey != "" { + t.Fatalf("defaults: model=%q key=%q", l.Model, l.APIKey) + } + + t.Setenv("DIBS_LLM_MODEL", " custom-model ") + t.Setenv("DIBS_LLM_API_KEY", "sk-test") + l = LLMFromEnv() + if l.Model != "custom-model" || l.APIKey != "sk-test" { + t.Fatalf("explicit vars: model=%q key=%q", l.Model, l.APIKey) + } + + // Legacy IDEATE_ prefix is a fallback for each var independently. + t.Setenv("DIBS_LLM_BASE_URL", "") + t.Setenv("DIBS_LLM_MODEL", "") + t.Setenv("DIBS_LLM_API_KEY", "") + t.Setenv("IDEATE_LLM_BASE_URL", "http://legacy:4000/v1") + t.Setenv("IDEATE_LLM_MODEL", "legacy-model") + t.Setenv("IDEATE_LLM_API_KEY", "sk-legacy") + l = LLMFromEnv() + if l == nil || l.BaseURL != "http://legacy:4000/v1" || l.Model != "legacy-model" || l.APIKey != "sk-legacy" { + t.Fatalf("legacy prefix fallback: %+v", l) + } + + // DIBS_ wins over IDEATE_ when both are set. + t.Setenv("DIBS_LLM_BASE_URL", "http://new:4000/v1") + if l = LLMFromEnv(); l.BaseURL != "http://new:4000/v1" { + t.Fatalf("DIBS_ must win over IDEATE_: %q", l.BaseURL) + } +} + +func TestFirstNonEmpty(t *testing.T) { + if got := firstNonEmpty("", "", "third", "fourth"); got != "third" { + t.Fatalf("firstNonEmpty = %q", got) + } + if got := firstNonEmpty(); got != "" { + t.Fatalf("no args must return empty, got %q", got) + } + if got := firstNonEmpty("", ""); got != "" { + t.Fatalf("all empty must return empty, got %q", got) + } +} + +// TestNonHiveCNCFFiltersRegistryRepos: CNCF suggestions that are already +// hive-managed repos are dropped; a nil registry keeps everything. +func TestNonHiveCNCFFiltersRegistryRepos(t *testing.T) { + _, reg := newFixtures(t) + if err := reg.Merge([]registry.RepoProfile{{RepoID: "istio/istio", Owner: "bob"}}); err != nil { + t.Fatalf("Merge: %v", err) + } + in := []CNCFMatch{ + {Name: "Istio", RepoID: "istio/istio"}, + {Name: "Vitess", RepoID: "vitessio/vitess"}, + } + e := &Engine{Registry: reg} + out := e.nonHiveCNCF(in) + if len(out) != 1 || out[0].RepoID != "vitessio/vitess" { + t.Fatalf("hive-managed repo must be filtered: %+v", out) + } + e = &Engine{} + if out = e.nonHiveCNCF(in); len(out) != 2 { + t.Fatalf("nil registry must keep everything: %+v", out) + } +}