fix(skill): rank recalled skills by relevance rather than by slug

skill/bundled/skills/deletion/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: deletion
-description: Use when removing anything that already exists: dead code, an unused function or package, a duplicate, a feature nobody calls, an option, a flag, a dependency, or scaffolding left from earlier work. Covers how much evidence a removal needs, which grows with how far the thing reaches, why a search of the tree is not proof that nothing runs it, which of two copies should die and what stops the next one, retiring a surface other people call, and what to leave alone. Carries one refusal above all others: a test, an assertion, an error path, or a check may never be removed, weakened or skipped as a way of getting a run to green.
+description: Use when removing anything that already exists: dead code, an unused function or package, a duplicate, a feature nobody calls, an option, a flag, a dependency, or the leftovers an earlier change left behind and nobody has cleaned up. Covers how much evidence a removal needs, which grows with how far the thing reaches, why a search of the tree is not proof that nothing runs it, which of two copies should die and what stops the next one, retiring a surface other people call, and what to leave alone. Carries one refusal above all others: a test, an assertion, an error path, or a check may never be removed, weakened or skipped as a way of getting a run to green.
 metadata:
   flynnhq.com/title: Deletion
   flynnhq.com/tags: '["deletion","refactoring","dead-code","duplication","maintenance"]'

skill/bundled/skills/domain-language/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: domain-language
-description: Use when choosing the name for something, when the same concept is going by several words, or when working out what the things in this domain are and what each term means. Covers finding the word the project already uses before inventing another one, where a term comes from when no domain expert is available to ask, splitting a word that is doing two jobs, why an implementation word names nothing about this business, why a name that no longer describes the behaviour is a defect and has to be renamed, where the glossary lives so it stays true, and an audit that fails the build when the code and the agreed vocabulary drift apart.
+description: Use when deciding what to call something, when the same concept is going by several words, or when working out what the things in this domain are and what each term means. Covers finding the word the project already uses before inventing another one, where a term comes from when no domain expert is available to ask, splitting a word that is doing two jobs, why an implementation word names nothing about this business, why a name that no longer describes the behaviour is a defect and has to be renamed, where the glossary lives so it stays true, and an audit that fails the build when the code and the agreed vocabulary drift apart.
 metadata:
   flynnhq.com/title: Domain language
   flynnhq.com/tags: '["naming","domain-model","glossary","vocabulary","readability"]'

skill/failure_test.go

@@ -221,6 +221,35 @@ func TestSearchOrdersSameSlugAcrossScopes(t *testing.T) {
 	}
 }
 
+// TestSearchCapsToTheBestMatches proves the limit selects on how well a skill
+// answers the query, not on how its slug sorts. Recall asks for a bounded number of
+// candidates per term, so a cut that ran alphabetically would hide the skill the
+// query is about behind any number of skills that merely mention the word, and
+// nothing would report it.
+func TestSearchCapsToTheBestMatches(t *testing.T) {
+	ctx := context.Background()
+	s := skill.NewStore(backing(t))
+	for _, slug := range []string{"alpha", "beta", "gamma"} {
+		if _, err := s.Upsert(ctx, state.Skill{Slug: slug, Body: "a deploy is mentioned here"}); err != nil {
+			t.Fatalf("upsert %s: %v", slug, err)
+		}
+	}
+	if _, err := s.Upsert(ctx, state.Skill{
+		Slug:        "zeta",
+		Description: "How to deploy the service.",
+	}); err != nil {
+		t.Fatalf("upsert zeta: %v", err)
+	}
+
+	got, err := s.Search(ctx, "deploy", 1)
+	if err != nil {
+		t.Fatalf("search: %v", err)
+	}
+	if len(got) != 1 || got[0].Slug != "zeta" {
+		t.Fatalf("search with limit 1 = %+v, want zeta: the description is where a skill states its subject", got)
+	}
+}
+
 // TestSearchLimitCaps proves a positive limit caps the result while a limit <= 0 returns
 // everything matched.
 func TestSearchLimitCaps(t *testing.T) {

skill/skill.go

@@ -134,13 +134,19 @@ func (s *Store) List(ctx context.Context, scope state.Scope) ([]state.Skill, err
 }
 
 // Search returns live skills whose name, description, body, or tags contain query
-// (case insensitive), across every scope, ordered by slug and capped at limit
-// (limit <= 0 means no cap). An empty query matches every live skill.
+// (case insensitive), across every scope, best match first and capped at limit
+// (limit <= 0 means no cap). An empty query matches every live skill, in slug
+// order, since with nothing to match on there is nothing to be better at.
 //
 // The description is searched because it is what a skill says about itself at
 // discovery: the specification asks an author to put the words that identify a
 // relevant task there, so a search that skipped it would refuse to find a skill by
 // the one text written to be found by.
+//
+// Ordering by match rather than by slug is what makes the limit a top-k. Cut an
+// alphabetical list and the skills that survive are the ones whose names sort
+// early, which for a term much of the library shares excludes the best answer
+// without reporting anything.
 func (s *Store) Search(ctx context.Context, query string, limit int) ([]state.Skill, error) {
 	rs, err := s.rs.ListAll(ctx, Kind, nil)
 	if err != nil {
@@ -157,7 +163,20 @@ func (s *Store) Search(ctx context.Context, query string, limit int) ([]state.Sk
 			out = append(out, sk)
 		}
 	}
-	sort.Slice(out, func(i, j int) bool { return lessBySlug(out[i], out[j]) })
+	if q == "" {
+		sort.Slice(out, func(i, j int) bool { return lessBySlug(out[i], out[j]) })
+	} else {
+		score := make(map[string]float64, len(out))
+		for _, sk := range out {
+			score[sk.ID] = matchScore(sk, q)
+		}
+		sort.Slice(out, func(i, j int) bool {
+			if score[out[i].ID] != score[out[j].ID] {
+				return score[out[i].ID] > score[out[j].ID]
+			}
+			return lessBySlug(out[i], out[j])
+		})
+	}
 	if limit > 0 && len(out) > limit {
 		out = out[:limit]
 	}
@@ -282,6 +301,27 @@ func matches(sk state.Skill, lowerQuery string) bool {
 		strings.Contains(strings.ToLower(strings.Join(sk.Tags, " ")), lowerQuery)
 }
 
+// matchScore says how well a skill answers a query, so a capped search returns the
+// best matches rather than the first ones alphabetically. A hit counts for what the
+// field it landed in is worth: the description is written to be searched, the name
+// is the handle someone types, tags are deliberate, and the body is long enough
+// that a passing mention there says little. Repeats count, and the body's are
+// capped, so a long document cannot outscore a description that is about the query.
+func matchScore(sk state.Skill, lowerQuery string) float64 {
+	count := func(field string) int { return strings.Count(strings.ToLower(field), lowerQuery) }
+	body := count(sk.Body)
+	if body > maxBodyHitsScored {
+		body = maxBodyHitsScored
+	}
+	return 10*float64(count(sk.Description)) +
+		5*float64(count(sk.Name)) +
+		3*float64(count(strings.Join(sk.Tags, " "))) +
+		float64(body)
+}
+
+// maxBodyHitsScored bounds how far repeating a word in a body can lift a skill.
+const maxBodyHitsScored = 3
+
 // translateErr maps the resource foundation's errors onto the state boundary's, so a
 // SkillStore caller sees state.ErrConflict / state.ErrNotFound regardless of the
 // backing store.

skill/skillrecall/skillrecall.go

@@ -16,6 +16,7 @@ package skillrecall
 
 import (
 	"context"
+	"math"
 	"sort"
 	"strings"
 
@@ -35,6 +36,14 @@ const DefaultLimit = 5
 // descriptions existed, kept so the fallback costs what it always did.
 const fallbackOfferLen = 240
 
+// candidatePool multiplies the offer limit to size each per-term search. A store
+// answers a term with its best matches capped at what it was asked for, so asking
+// for exactly the offer limit hands ranking a set that was already cut by the
+// store's own tie-break. Below that cut sits every skill that shares the term and
+// sorted later, which for a common word is most of a growing library. Gathering
+// wider costs one bounded query per term and gives Rank something to choose from.
+const candidatePool = 4
+
 // Recall returns the skills an objective is offered, best first, capped at limit
 // (limit <= 0 uses DefaultLimit). It is the whole of stage one: the objective's
 // content words become queries, the store's search answers each, and the union is
@@ -60,7 +69,7 @@ func Gather(ctx context.Context, skills state.SkillStore, terms []string, limit
 	seen := map[string]bool{}
 	var out []state.Skill
 	for _, term := range terms {
-		found, err := skills.Search(ctx, term, limit)
+		found, err := skills.Search(ctx, term, limit*candidatePool)
 		if err != nil {
 			continue
 		}
@@ -74,31 +83,49 @@ func Gather(ctx context.Context, skills state.SkillStore, terms []string, limit
 	return out
 }
 
-// Rank orders candidate skills by relevance (how many of the objective's keywords
-// each carries), boosted for verified skills and for those with a strong confirmed
-// track record, then caps the result at limit (limit <= 0 uses DefaultLimit).
-// Relevance dominates; verification and confidence break ties between similarly
-// relevant skills.
+// Rank orders candidate skills by relevance, then caps the result at limit
+// (limit <= 0 uses DefaultLimit). Relevance is decided first and on its own;
+// verification and a confirmed track record break ties between candidates the
+// objective's words cannot separate, and never outrank a better match.
+//
+// A term is worth what it discriminates. Counting matched terms equally lets a
+// skill win on a word most of the library carries, which is how one skill takes
+// another's objectives simply by being wordy, so each term is weighted by how few
+// of the candidates carry it.
 func Rank(terms []string, cands []state.Skill, limit int) []state.Skill {
 	if limit <= 0 {
 		limit = DefaultLimit
 	}
 	type scored struct {
-		s     state.Skill
-		score float64
+		s         state.Skill
+		relevance float64
+		evidence  float64
 	}
+	// Scored over the text the offer will carry, not over the body. A skill ranked
+	// on words the model never sees produces an offer that reads as irrelevant to
+	// the objective that surfaced it.
+	hays := make([]string, len(cands))
+	for i, s := range cands {
+		hays[i] = strings.ToLower(s.Slug + " " + s.Name + " " + Offer(s) + " " + strings.Join(s.Tags, " "))
+	}
+	weights := termWeights(terms, hays)
+
 	ss := make([]scored, len(cands))
 	for i, s := range cands {
-		// Scored over the text the offer will carry, not over the body. A skill ranked
-		// on words the model never sees produces an offer that reads as irrelevant to
-		// the objective that surfaced it.
-		hay := strings.ToLower(s.Slug + " " + s.Name + " " + Offer(s) + " " + strings.Join(s.Tags, " "))
-		score := float64(MatchScore(terms, hay)+verifiedBoost(s.Tags)) + learn.Confidence(s.Reads, s.Wins)
-		ss[i] = scored{s, score}
+		relevance := 0.0
+		for _, t := range terms {
+			if strings.Contains(hays[i], t) {
+				relevance += weights[t]
+			}
+		}
+		ss[i] = scored{s, relevance, float64(verifiedBoost(s.Tags)) + learn.Confidence(s.Reads, s.Wins)}
 	}
 	sort.SliceStable(ss, func(i, j int) bool {
-		if ss[i].score != ss[j].score {
-			return ss[i].score > ss[j].score
+		if ss[i].relevance != ss[j].relevance {
+			return ss[i].relevance > ss[j].relevance
+		}
+		if ss[i].evidence != ss[j].evidence {
+			return ss[i].evidence > ss[j].evidence
 		}
 		return ss[i].s.Slug < ss[j].s.Slug
 	})
@@ -127,6 +154,33 @@ func Offer(s state.Skill) string {
 	return text.Clip(strings.TrimSpace(s.Body), fallbackOfferLen)
 }
 
+// termWeights gives each term what a match on it is worth: the fewer candidates
+// carry it, the more it says about the ones that do. A term every candidate has
+// separates nobody and is worth least; a term one candidate has is why that
+// candidate is here at all. Every weight stays above zero, so a skill matching
+// more of the objective still beats one matching less of it.
+//
+// The document frequency is counted over the gathered candidates rather than the
+// whole library, because those are the skills being chosen between and it needs no
+// second pass over the store.
+func termWeights(terms []string, hays []string) map[string]float64 {
+	weights := make(map[string]float64, len(terms))
+	for _, t := range terms {
+		df := 0
+		for _, h := range hays {
+			if strings.Contains(h, t) {
+				df++
+			}
+		}
+		if df == 0 {
+			weights[t] = 0
+			continue
+		}
+		weights[t] = math.Log(1 + float64(len(hays))/float64(df))
+	}
+	return weights
+}
+
 // MatchScore counts how many distinct terms appear in text, the lexical relevance
 // signal recall ranks on. The text is expected lowercased, as the terms are.
 func MatchScore(terms []string, text string) int {

skill/skillrecall/skillrecall_test.go

@@ -81,6 +81,63 @@ func TestRankBreaksTiesOnEvidenceAndNeverOverRelevance(t *testing.T) {
 	}
 }
 
+// The best answer is offered however its name sorts. A store answers a term with
+// its best matches capped at what it was asked for, so recall asking for exactly
+// the offer limit used to hand ranking a set the store had already cut, and for a
+// word much of a library shares that cut is alphabetical: the skill that is
+// actually about the objective was never a candidate, and nothing reported it.
+func TestRecallOffersTheBestMatchWhateverItsSlugSortsAs(t *testing.T) {
+	var seeds []state.Skill
+	for _, slug := range []string{"a-one", "b-two", "c-three", "d-four", "e-five", "f-six", "g-seven"} {
+		seeds = append(seeds, state.Skill{
+			Slug: slug, Name: slug,
+			Description: "Something about the database.",
+			// Mentions the word in passing, so it matches the term and is a worse
+			// answer than the skill whose subject it is.
+			Body: "A migration is mentioned here once.",
+		})
+	}
+	best := state.Skill{
+		Slug: "z-migrations", Name: "z-migrations",
+		Description: "How to run a database migration safely.",
+	}
+	store := library(t, append(seeds, best)...)
+
+	got := skillrecall.Recall(context.Background(), store, "run a database migration", 0)
+	if len(got) == 0 || got[0].Slug != "z-migrations" {
+		t.Errorf("Recall offered %v, want z-migrations first: it is the only skill that mentions migration", slugs(got))
+	}
+}
+
+// A term most candidates carry cannot decide between them, and one that only a few
+// carry is why those few are here at all. Weighting every matched term the same is
+// how a skill takes another's objectives by being wordy rather than by being right.
+func TestRankWeighsARareTermAboveACommonOne(t *testing.T) {
+	var cands []state.Skill
+	for _, slug := range []string{"a-common", "b-common", "c-common", "d-common"} {
+		cands = append(cands, state.Skill{
+			Slug: slug, Name: slug,
+			Description: "Everything about the service and the database.",
+		})
+	}
+	// Carries one term the others do not, and one fewer of the terms they share.
+	rare := state.Skill{Slug: "e-rare", Name: "e-rare", Description: "Sharding the database."}
+	cands = append(cands, rare)
+
+	terms := skillrecall.Keywords("sharding the database service")
+	if got := skillrecall.Rank(terms, cands, 0); got[0].Slug != "e-rare" {
+		t.Errorf("ranked %v; want e-rare first, since sharding separates it and database does not", slugs(got))
+	}
+}
+
+func slugs(skills []state.Skill) []string {
+	out := make([]string, len(skills))
+	for i, s := range skills {
+		out[i] = s.Slug
+	}
+	return out
+}
+
 // A skill with no description falls back to the head of its body, which is how a
 // skill the distiller minted stays reachable. It is a fallback and not a policy: the
 // head of a procedure is a poor account of when to reach for it.

storage/sqlite/projection_internal_test.go

@@ -333,6 +333,32 @@ func TestRecallScopedWithNoQuery(t *testing.T) {
 	}
 }
 
+// TestSkillSearchRanksTheDescriptionAboveThePassingMention covers what the limit
+// selects on. The FTS branch is ordered by bm25 with the description weighted
+// highest, so a capped search answers with the skill the query is about rather than
+// the one whose slug sorts first, which is what recall depends on when it asks each
+// keyword for a bounded set of candidates.
+func TestSkillSearchRanksTheDescriptionAboveThePassingMention(t *testing.T) {
+	ctx := context.Background()
+	s := newStore(t)
+	for _, slug := range []string{"a", "b", "c"} {
+		if _, err := s.Skills().Upsert(ctx, state.Skill{Slug: slug, Body: "a deploy is mentioned here once"}); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if _, err := s.Skills().Upsert(ctx, state.Skill{Slug: "z", Description: "How to deploy the service."}); err != nil {
+		t.Fatal(err)
+	}
+
+	got, err := s.Skills().Search(ctx, "deploy", 1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(got) != 1 || got[0].Slug != "z" {
+		t.Fatalf("search with limit 1 = %+v, want z: its description is about the query", got)
+	}
+}
+
 // TestSkillSearchAppliesTheLimit covers the FTS branch's capped shape: a text search with a
 // limit matches the same rows an uncapped one does, cut to the limit, so a caller asking
 // for the top k does not silently get everything.

storage/sqlite/skills.go

@@ -131,9 +131,17 @@ func (s *skills) Search(ctx context.Context, query string, limit int) ([]state.S
 			rows, err = s.p.reads().QueryContext(ctx, sqlStr)
 		}
 	} else {
+		// Ordered by how well the row matches, then by slug for a stable answer.
+		// A limit cuts this list, so ordering it by slug would make the cap select
+		// alphabetically: for a term many skills share, everything sorted after the
+		// first few is unreachable however well it matches. The bm25 weights follow
+		// the columns of skills_fts and say where a hit counts most: the description
+		// is what a skill publishes about when to reach for it, the name is the
+		// handle, and the body is the long text a stray word lands in.
 		sqlStr := `SELECT ` + skillColsQualified + `
 			FROM skills s JOIN skills_fts f ON f.skill_id = s.id
-			WHERE f.skills_fts MATCH ? AND s.deleted = 0 ORDER BY s.slug`
+			WHERE f.skills_fts MATCH ? AND s.deleted = 0
+			ORDER BY bm25(skills_fts, 0.0, 5.0, 10.0, 1.0, 3.0), s.slug`
 		if limit > 0 {
 			sqlStr += ` LIMIT ?`
 			rows, err = s.p.reads().QueryContext(ctx, sqlStr, ftsPhrase(q), limit)