diff --git a/internal/adapters/outbound/searchsql/intent_order_test.go b/internal/adapters/outbound/searchsql/intent_order_test.go new file mode 100644 index 00000000..0e6108a7 --- /dev/null +++ b/internal/adapters/outbound/searchsql/intent_order_test.go @@ -0,0 +1,92 @@ +//go:build fts5 + +package searchsql + +import ( + "slices" + "testing" + + intentapp "github.com/tae2089/code-context-graph/internal/app/search/intent" + requestctx "github.com/tae2089/code-context-graph/internal/ctx" +) + +// The same corpus, indexed in a different sequence, has to answer the same +// question the same way. +// +// Recorded reasons are one sentence long, so exact score ties are the normal +// case rather than the edge case, and whatever breaks those ties decides the +// answer. Breaking them by node id makes the answer a function of the order the +// rows happened to be written: re-index a repository from a clean checkout, or +// let a webhook rebuild arrive in a different order, and the same question comes +// back with a different top hit. Nobody can act on an answer like that, and +// nobody can measure one either. +// +// The two orders here are the same twelve declarations, seeded ascending and +// descending, so every declaration's id is different between the two databases +// while its name, path and reason are identical. +func TestQueryIntent_SameCorpusSeededEitherWayGivesTheSameAnswer(t *testing.T) { + const count = 12 + ascending := tiedIntentNames(count) + descending := slices.Clone(ascending) + slices.Reverse(descending) + + forward := answerTiedIntent(t, ascending, count) + backward := answerTiedIntent(t, descending, count) + + if len(forward) != count || len(backward) != count { + t.Fatalf("got %d and %d answers, want %d each", len(forward), len(backward), count) + } + for i := range forward { + if forward[i] != backward[i] { + t.Fatalf("row %d is %s when the corpus is seeded ascending and %s when it is seeded descending;"+ + " the answer depends on insertion order", i, forward[i], backward[i]) + } + } +} + +// Asking for one more row has to extend the answer, not reshuffle it — the +// promise the tie-break exists to keep. This is checked against a corpus seeded +// backwards, because a tie-break that happens to agree with insertion order +// keeps that promise for the wrong reason. +func TestQueryIntent_ExtendsRatherThanReshufflesOnABackwardsSeededCorpus(t *testing.T) { + const count = 12 + descending := slices.Clone(tiedIntentNames(count)) + slices.Reverse(descending) + + short := answerTiedIntent(t, descending, 4) + long := answerTiedIntent(t, descending, count) + + if len(short) != 4 || len(long) != count { + t.Fatalf("got %d and %d answers, want 4 and %d", len(short), len(long), count) + } + for i, name := range short { + if long[i] != name { + t.Fatalf("row %d is %s at limit 4 but %s at limit %d", i, name, long[i], count) + } + } +} + +// answerTiedIntent seeds one database in the given sequence and returns the +// answer as qualified names, which are the same in every seeding while the ids +// are not. +func answerTiedIntent(t *testing.T, indexes []int, limit int) []string { + t.Helper() + db := setupTestDB(t) + seedTiedIntentNodes(t, db, indexes) + backend := buildIntentIndex(t, db) + ctx := requestctx.WithNamespace(t.Context(), requestctx.DefaultNamespace) + + result, err := NewReader(db, backend).QueryIntent(ctx, "what keeps the queue draining", limit) + if err != nil { + t.Fatalf("QueryIntent: %v", err) + } + return qualifiedNames(result) +} + +func qualifiedNames(result intentapp.Result) []string { + names := make([]string, 0, len(result.Hits)) + for _, hit := range result.Hits { + names = append(names, hit.Node.QualifiedName) + } + return names +} diff --git a/internal/adapters/outbound/searchsql/intent_reasons_test.go b/internal/adapters/outbound/searchsql/intent_reasons_test.go index 5cb61ce0..710d40c5 100644 --- a/internal/adapters/outbound/searchsql/intent_reasons_test.go +++ b/internal/adapters/outbound/searchsql/intent_reasons_test.go @@ -82,16 +82,18 @@ func TestIntentIndex_HoldsOneDocumentPerReasonTag(t *testing.T) { func TestQueryIntent_ExtraRulesDoNotSinkTheDeclarationThatWroteThem(t *testing.T) { db := setupTestDB(t) const shared = "verify the signature so a push from anywhere else is rejected" - // The loaded declaration is seeded first so it holds the lower node id. Ties - // break on id, so on equal footing it must come first — and under the joined - // index it cannot, because its three rules lengthened its one document. - loaded := seedReasoned(t, db, "verifyLoaded", + // Ties break on identity, and `reasons/verifyAll.go` sorts before + // `reasons/verifyBare.go`, so on equal footing the loaded declaration must come + // first — and under the joined index it cannot, because its three rules + // lengthened its one document. It is seeded second on purpose: it holds the + // higher node id, so an answer that still leans on id order gets this backwards. + bare := seedReasoned(t, db, "verifyBare", graph.DocTag{Kind: graph.TagIntent, Value: shared}) + loaded := seedReasoned(t, db, "verifyAll", graph.DocTag{Kind: graph.TagIntent, Value: shared}, graph.DocTag{Kind: graph.TagDomainRule, Value: "the shared secret is read from the environment and never logged"}, graph.DocTag{Kind: graph.TagDomainRule, Value: "an unsigned request is refused before the body is parsed"}, graph.DocTag{Kind: graph.TagDomainRule, Value: "a signature that does not compare in constant time is a defect"}, ) - bare := seedReasoned(t, db, "verifyBare", graph.DocTag{Kind: graph.TagIntent, Value: shared}) backend := buildIntentIndex(t, db) ctx := requestctx.WithNamespace(t.Context(), requestctx.DefaultNamespace) @@ -108,10 +110,10 @@ func TestQueryIntent_ExtraRulesDoNotSinkTheDeclarationThatWroteThem(t *testing.T if !bareSeen || !loadedSeen { t.Fatalf("both declarations must answer; got %v", answeringNodes(result)) } - // Same reason, same words, so neither may be ranked below the other. Ties - // break on node id, which is the only order the two can legitimately differ in. - if (bareRank < loadedRank) != (bare.ID < loaded.ID) { - t.Errorf("rank order (%d before %d) does not follow the tiebreak; the three domain rules moved the score", + // Same reason, same words, so the score cannot separate them and the identity + // tiebreak decides: file path first, which puts verifyAll ahead of verifyBare. + if loadedRank > bareRank { + t.Errorf("verifyBare answered at %d and verifyAll at %d, want the file-path order; the three domain rules moved the score", bareRank, loadedRank) } } diff --git a/internal/adapters/outbound/searchsql/intent_test.go b/internal/adapters/outbound/searchsql/intent_test.go index c9a512fd..a15f5193 100644 --- a/internal/adapters/outbound/searchsql/intent_test.go +++ b/internal/adapters/outbound/searchsql/intent_test.go @@ -54,7 +54,25 @@ func seedIntentFixture(t *testing.T, db *gorm.DB) (reasoned, namesake graph.Node // every one of them scores identically and only the tiebreak decides the order. func seedTiedIntentFixture(t *testing.T, db *gorm.DB, count int) { t.Helper() + seedTiedIntentNodes(t, db, tiedIntentNames(count)) +} + +// tiedIntentNames is the fixture's declarations in ascending order, which is +// both their file order and, when they are seeded in this sequence, their id +// order. A test that seeds them in some other sequence takes the two apart. +func tiedIntentNames(count int) []int { + indexes := make([]int, 0, count) for i := range count { + indexes = append(indexes, i) + } + return indexes +} + +// seedTiedIntentNodes writes the tied declarations in the sequence given, so a +// caller can decide which declaration gets the lowest id. +func seedTiedIntentNodes(t *testing.T, db *gorm.DB, indexes []int) { + t.Helper() + for _, i := range indexes { node := graph.Node{ QualifiedName: fmt.Sprintf("tied.decl%02d", i), Kind: graph.NodeKindFunction, diff --git a/internal/adapters/outbound/searchsql/postgres.go b/internal/adapters/outbound/searchsql/postgres.go index 6fc6dd18..8a1ff86d 100644 --- a/internal/adapters/outbound/searchsql/postgres.go +++ b/internal/adapters/outbound/searchsql/postgres.go @@ -113,10 +113,10 @@ type resultRow struct { // rather than reordering them — so the same repository indexed twice could answer // differently. `n.id` last makes the key total. // -// The key order is deliberately NOT rank.compareIdentity's, which compares file -// path before qualified name (and then kind and namespace). See the SQLite -// matchRows for why that difference matters and why aligning them belongs to #106 -// rather than here. +// The key order is deliberately NOT graph.CompareIdentity's, which compares file +// path before qualified name (and then kind, namespace and start line). See the +// SQLite matchRows for why that difference matters and why #106 left retrieval on +// this order while giving rerank and intent scoring the shared one. // // @intent let Query run the same retrieval twice with a different expression. func (p *PostgresBackend) matchRows(ctx context.Context, db *gorm.DB, tsQuery, ns string, limit int) ([]resultRow, error) { @@ -184,7 +184,13 @@ func (p *PostgresBackend) Query(ctx context.Context, db *gorm.DB, query string, // them, so it could not tell a distinctive word from a filler one. Retrieval is // what the GIN index is genuinely good at; scoring moved to intentrank, which // counts the corpus and gives both backends the same answer. -// @intent hand every candidate reason to shared scoring, in whatever order the index produced. +// +// The join onto nodes carries each candidate's identity — path, qualified name, +// kind, namespace, start line — because that is what intentrank breaks its score +// ties on, and it drops reason rows whose node is gone rather than spending a row +// of the candidate cap on one that cannot be scored. The SQLite twin does the +// same; both have to, or the two backends tie-break on different information. +// @intent hand every candidate reason to shared scoring, with the identity that scoring breaks ties on. // @requires maxCandidates must be greater than 0. // @return returns unordered candidates with the exact text that was indexed for each. func (p *PostgresBackend) MatchIntent(ctx context.Context, db *gorm.DB, query string, maxCandidates int) ([]intentrank.Doc, error) { @@ -198,8 +204,10 @@ func (p *PostgresBackend) MatchIntent(ctx context.Context, db *gorm.DB, query st var docs []intentrank.Doc if err := db.WithContext(ctx).Raw(` - SELECT sr.node_id, sr.content + SELECT sr.node_id, sr.content, + n.file_path, n.qualified_name, n.kind, n.namespace, n.start_line FROM search_reasons sr + JOIN nodes n ON n.id = sr.node_id WHERE sr.reason_tsv @@ to_tsquery('simple', ?) AND sr.namespace = ? LIMIT ?`, tsQuery, requestctx.FromContext(ctx), maxCandidates).Scan(&docs).Error; err != nil { diff --git a/internal/adapters/outbound/searchsql/sqlite.go b/internal/adapters/outbound/searchsql/sqlite.go index 2f1f21cc..b7f0435b 100644 --- a/internal/adapters/outbound/searchsql/sqlite.go +++ b/internal/adapters/outbound/searchsql/sqlite.go @@ -287,15 +287,19 @@ type ftsRow struct { // tied rows past it are not reordered but dropped, so the same repository indexed // twice could answer differently. `n.id` last makes the key total. // -// The key order here is deliberately NOT rank.compareIdentity's, which compares -// file path before qualified name (and then kind and namespace). The difference -// is not cosmetic: it decides which tied rows survive the LIMIT. File-path-first -// would cut on file boundaries more often, which is closer to what wire.FileGroup -// promises one layer up — that a file appearing in an answer appears whole. -// Aligning the two is #106's job, where compareIdentity is reused across -// retrieval, rerank and intent together and one measurement covers all three. -// Do not flip these keys on their own: every ranking number and every recapture -// figure recorded for #103 was measured with this order. +// The key order here is deliberately NOT graph.CompareIdentity's, which compares +// file path before qualified name (and then kind, namespace and start line). The +// difference is not cosmetic: it decides which tied rows survive the LIMIT. +// File-path-first would cut on file boundaries more often, which is closer to what +// wire.FileGroup promises one layer up — that a file appearing in an answer appears +// whole. +// +// #106 gave rerank and intent scoring that one shared key; retrieval is the layer +// it left alone, because flipping these keys changes which rows the LIMIT keeps and +// so moves measured ranking numbers. Do not flip them on their own: every ranking +// number and every recapture figure recorded for #103 was measured with this order, +// so the flip needs a -capture-golden recapture and a re-judgment landing as its own +// change. // // @intent let Query run the same retrieval twice with a different expression. func (s *SQLiteBackend) matchRows(ctx context.Context, db *gorm.DB, ftsQuery, ns string, limit int) ([]ftsRow, error) { @@ -362,7 +366,13 @@ func (s *SQLiteBackend) Query(ctx context.Context, db *gorm.DB, query string, li // intentrank so that this backend and the PostgreSQL one answer the same // question the same way. What is left here is retrieval, which is what the index // is for. -// @intent hand every candidate reason to shared scoring, in whatever order the index produced. +// +// The join onto nodes carries each candidate's identity — path, qualified name, +// kind, namespace, start line — because that is what intentrank breaks its score +// ties on. It also drops index rows whose node is gone, the same way matchRows +// does: an orphan cannot be scored and would otherwise spend a row of the +// candidate cap. +// @intent hand every candidate reason to shared scoring, with the identity that scoring breaks ties on. // @requires maxCandidates must be greater than 0. // @return returns unordered candidates with the exact text that was indexed for each. func (s *SQLiteBackend) MatchIntent(ctx context.Context, db *gorm.DB, query string, maxCandidates int) ([]intentrank.Doc, error) { @@ -376,9 +386,11 @@ func (s *SQLiteBackend) MatchIntent(ctx context.Context, db *gorm.DB, query stri var docs []intentrank.Doc if err := db.WithContext(ctx).Raw( - `SELECT CAST(node_id AS INTEGER) AS node_id, content + `SELECT CAST(intent_fts.node_id AS INTEGER) AS node_id, intent_fts.content, + n.file_path, n.qualified_name, n.kind, n.namespace, n.start_line FROM intent_fts - WHERE intent_fts MATCH ? AND namespace = ? + JOIN nodes n ON n.id = CAST(intent_fts.node_id AS INTEGER) + WHERE intent_fts MATCH ? AND intent_fts.namespace = ? LIMIT ?`, ftsQuery, requestctx.FromContext(ctx), maxCandidates).Scan(&docs).Error; err != nil { return nil, trace.Wrap(err, "intent fts query") } diff --git a/internal/app/search/intentrank/rank.go b/internal/app/search/intentrank/rank.go index 6a76bd84..41ffac7b 100644 --- a/internal/app/search/intentrank/rank.go +++ b/internal/app/search/intentrank/rank.go @@ -10,15 +10,38 @@ import ( "github.com/tae2089/code-context-graph/internal/app/search/identtoken" "github.com/tae2089/code-context-graph/internal/app/search/queryterm" + "github.com/tae2089/code-context-graph/internal/domain/graph" ) // Doc is one candidate the index admitted: a node and one recorded reason that // was indexed for it. A node that recorded several reasons arrives as several // Docs sharing a node id. +// +// It carries the node's identity alongside its id because scoring ties are the +// normal case here, and the tie-break has to mean the same thing after a +// re-index. An id cannot: it is handed out in the order rows were written. // @intent carry the exact indexed text into scoring so the score is computed over what was matched. type Doc struct { - NodeID uint - Content string + NodeID uint + Content string + FilePath string + QualifiedName string + Kind graph.NodeKind + Namespace string + StartLine int +} + +// identity is who this candidate's node is, in the form every layer of search +// breaks ties on. +// @intent keep the fields that make up the tie-break named in one place. +func (d Doc) identity() graph.Identity { + return graph.Identity{ + FilePath: d.FilePath, + QualifiedName: d.QualifiedName, + Kind: d.Kind, + Namespace: d.Namespace, + StartLine: d.StartLine, + } } // Match is one declaration the question reached, and the terms of the question @@ -132,9 +155,10 @@ func Rank(question string, docs []Doc, corpusSize, limit int) Result { // term, summed over terms, charges each term exactly the length of the reason // it was written in and still counts how much of the question was answered. type scored struct { - nodeID uint - score float64 - best []float64 + nodeID uint + identity graph.Identity + score float64 + best []float64 } grouped := make([]scored, 0, len(docs)) position := make(map[uint]int, len(docs)) @@ -143,7 +167,7 @@ func Rank(question string, docs []Doc, corpusSize, limit int) Result { if !seen { at = len(grouped) position[doc.NodeID] = at - grouped = append(grouped, scored{nodeID: doc.NodeID, best: make([]float64, len(groups))}) + grouped = append(grouped, scored{nodeID: doc.NodeID, identity: doc.identity(), best: make([]float64, len(groups))}) } node := &grouped[at] for g := range groups { @@ -165,14 +189,21 @@ func Rank(question string, docs []Doc, corpusSize, limit int) Result { results = append(results, node) } - // The node id is not a preference, it is the promise that asking for one more - // row extends the answer instead of reshuffling it. Reasons are one sentence - // long, so exact ties are common rather than rare. + // The tie-break is not a preference, it is the promise that asking for one + // more row extends the answer instead of reshuffling it. Reasons are one + // sentence long, so exact ties are common rather than rare. + // + // It breaks on identity and not on the node id, which used to sit here. An id + // is handed out in the order rows were written, so it kept that promise for a + // single database and broke a bigger one: index the same repository twice in a + // different order and the same question came back with a different top hit. + // Identity is the same key the reranker uses, so the two layers of one search + // cannot disagree about who comes first. sort.SliceStable(results, func(a, b int) bool { if results[a].score != results[b].score { return results[a].score > results[b].score } - return results[a].nodeID < results[b].nodeID + return graph.CompareIdentity(results[a].identity, results[b].identity) < 0 }) matches := make([]Match, 0, min(limit, len(results))) diff --git a/internal/app/search/intentrank/rank_test.go b/internal/app/search/intentrank/rank_test.go index d0ff3f02..1b6af4dd 100644 --- a/internal/app/search/intentrank/rank_test.go +++ b/internal/app/search/intentrank/rank_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/tae2089/code-context-graph/internal/app/search/intentrank" + "github.com/tae2089/code-context-graph/internal/domain/graph" ) // rank is the shorthand every test here uses: score these documents against this @@ -44,17 +45,60 @@ func TestRank_PrefersTheRarerWord(t *testing.T) { // Two documents that earn exactly the same score must always come back in the // same order, or asking for one more row reshuffles the answer. -func TestRank_BreaksTiesByNodeID(t *testing.T) { +// +// The order is the identity order, and the ids here run against it on purpose: +// ascending file path is descending node id, so a tie broken by id would answer +// 3, 5, 7 while a tie broken by identity answers 7, 5, 3. +func TestRank_BreaksTiesByIdentityNotByNodeID(t *testing.T) { + docs := tiedDocs() + got := rank(t, "what keeps the queue draining", len(docs), docs...) + if !slices.Equal(got, []uint{7, 5, 3}) { + t.Fatalf("got %v, want the file-path order 7, 5, 3 on a three-way tie", got) + } +} + +// The same corpus with its ids handed out differently is the same answer. This +// is the unit-level form of what re-indexing a repository does: same +// declarations, same reasons, different ids. +func TestRank_TiedAnswerDoesNotDependOnWhichIDsWereHandedOut(t *testing.T) { + docs := tiedDocs() + reassigned := make([]intentrank.Doc, 0, len(docs)) + for i, doc := range docs { + doc.NodeID = uint(100 - i) + reassigned = append(reassigned, doc) + } + + first := namesInAnswerOrder(rank(t, "what keeps the queue draining", len(docs), docs...), docs) + second := namesInAnswerOrder(rank(t, "what keeps the queue draining", len(reassigned), reassigned...), reassigned) + if !slices.Equal(first, second) { + t.Fatalf("answer is %v with one set of ids and %v with another", first, second) + } +} + +// tiedDocs is three declarations whose recorded reason is byte-identical, so all +// three score the same and only the tie-break decides the order. +func tiedDocs() []intentrank.Doc { same := "keep the queue draining under backpressure" - docs := []intentrank.Doc{ - {NodeID: 7, Content: same}, - {NodeID: 3, Content: same}, - {NodeID: 5, Content: same}, + return []intentrank.Doc{ + {NodeID: 7, Content: same, FilePath: "queue/drain.go", QualifiedName: "queue.Drain", Kind: graph.NodeKindFunction}, + {NodeID: 3, Content: same, FilePath: "queue/worker.go", QualifiedName: "queue.Worker", Kind: graph.NodeKindFunction}, + {NodeID: 5, Content: same, FilePath: "queue/pump.go", QualifiedName: "queue.Pump", Kind: graph.NodeKindFunction}, } - got := rank(t, "what keeps the queue draining", len(docs), docs...) - if !slices.Equal(got, []uint{3, 5, 7}) { - t.Fatalf("got %v, want ascending node ids on a three-way tie", got) +} + +// namesInAnswerOrder reads an answer back as qualified names, which stay the +// same across two seedings while the ids do not. +func namesInAnswerOrder(answer []uint, docs []intentrank.Doc) []string { + names := make([]string, 0, len(answer)) + for _, id := range answer { + for _, doc := range docs { + if doc.NodeID == id { + names = append(names, doc.QualifiedName) + break + } + } } + return names } // A short Latin term matches whole words only. `get` reaching `getAnnotation` diff --git a/internal/app/search/rank/rank.go b/internal/app/search/rank/rank.go index 9282c328..df6a3b84 100644 --- a/internal/app/search/rank/rank.go +++ b/internal/app/search/rank/rank.go @@ -231,21 +231,14 @@ func RerankGroups(query string, groups [][]graph.Node, limit int) []graph.Node { } // compareIdentity orders two structurally tied candidates by who they are. -// File path comes first so a tie group reads as whole files, matching how the -// evidence list will group it anyway; namespace is included because federated -// search can hold the same file in two repositories. +// +// The key itself lives in the domain, as graph.CompareIdentity, because the +// intent scorer has to break its ties the same way. Two layers of one search +// disagreeing about who comes first is how an answer starts depending on which +// layer produced it. // @intent break structural ties by node identity so the order never depends on which backend retrieved the pool. func compareIdentity(a, b graph.Node) int { - if by := cmp.Compare(a.FilePath, b.FilePath); by != 0 { - return by - } - if by := cmp.Compare(a.QualifiedName, b.QualifiedName); by != 0 { - return by - } - if by := cmp.Compare(a.Kind, b.Kind); by != 0 { - return by - } - return cmp.Compare(a.Namespace, b.Namespace) + return graph.CompareIdentity(a.Identity(), b.Identity()) } // applyLimit bounds the result slice, treating a non-positive limit as unbounded. diff --git a/internal/domain/graph/identity.go b/internal/domain/graph/identity.go new file mode 100644 index 00000000..80c0a872 --- /dev/null +++ b/internal/domain/graph/identity.go @@ -0,0 +1,60 @@ +package graph + +import "cmp" + +// Identity is who a node is, told apart from which row it happens to be. +// +// The database gives every node an id in the order the rows were written, so an +// id says when a node was indexed, not what it is. Re-index the same repository +// from a clean checkout and every id can differ while every Identity here stays +// the same. Anything that has to produce the same answer twice — a tie-break, a +// stable sort, a comparison across two databases — belongs on this and not on +// the id. +// +// The four fields are the columns of the node table's uniqueness index +// (namespace, qualified_name, file_path, start_line) plus the kind, so two +// different nodes cannot share one Identity. +// @intent give ranking a key that survives re-indexing, which the node id does not. +type Identity struct { + FilePath string + QualifiedName string + Kind NodeKind + Namespace string + StartLine int +} + +// Identity says who this node is. +// @intent read a node's stable identity without repeating which fields make it up. +func (n Node) Identity() Identity { + return Identity{ + FilePath: n.FilePath, + QualifiedName: n.QualifiedName, + Kind: n.Kind, + Namespace: n.Namespace, + StartLine: n.StartLine, + } +} + +// CompareIdentity orders two nodes by who they are, for the callers that have +// run out of reasons to prefer one over the other. +// +// File path comes first so a tie group reads as whole files, matching how an +// evidence list groups it anyway. Namespace is in there because federated search +// can hold the same file in two repositories, and start line because one file +// can declare the same qualified name twice. +// @intent give every layer of search one tie-break, so two layers cannot disagree about who comes first. +func CompareIdentity(a, b Identity) int { + if by := cmp.Compare(a.FilePath, b.FilePath); by != 0 { + return by + } + if by := cmp.Compare(a.QualifiedName, b.QualifiedName); by != 0 { + return by + } + if by := cmp.Compare(a.Kind, b.Kind); by != 0 { + return by + } + if by := cmp.Compare(a.Namespace, b.Namespace); by != 0 { + return by + } + return cmp.Compare(a.StartLine, b.StartLine) +}