mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
fix(bm25): return FTS-matched symbols instead of arbitrary LIMIT 3 nodes (#806)
* fix(bm25): return FTS-matched symbols instead of arbitrary LIMIT 3 nodes Previously, bm25Search fetched up to 3 arbitrary symbols from the matched file using MATCH (n) WHERE n.filePath = $filePath LIMIT 3 (no ORDER BY). This meant the specific function or class that actually scored highest in the BM25 index could be completely absent from the results. Fix: propagate nodeId from each FTS hit through searchFTSFromLbug, then use those nodeIds in bm25Search to look up the exact matched nodes via WHERE n.id IN $nodeIds. Falls back to the old filePath-based lookup when nodeIds are unavailable. Also switches the per-file score aggregation from naive sum-of-all to sum-of-top-3, which prevents files with many mediocre matches (e.g. test files) from outranking files with a single highly-relevant symbol. * test(bm25): add unit tests for top-3 aggregation and nodeIds propagation Covers the new logic paths added in the previous commit: - top-3 score aggregation (file with 5+ matches → only top-3 contribute) - nodeIds propagation through BM25SearchResult - empty nodeId filtering - cross-table merge for the same file - result ranking by aggregated score Also fixes in-place entries.sort() mutation (bm25-index.ts:125) to use [...entries].sort() so the Map value is not silently modified. * style: apply prettier formatting * fix(test): use importOriginal to avoid missing export errors in vi.mock * fix(bm25): align queryFTSViaExecutor nodeId extraction to match lbug-adapter Use node.nodeId || node.id || '' in queryFTSViaExecutor to match the fallback logic in lbug-adapter.ts:1040. Without this, the MCP pool path could silently return empty nodeIds if LadybugDB surfaces the node id under node.nodeId rather than node.id. --------- Co-authored-by: jisue0224 <>
This commit is contained in:
parent
d858746476
commit
8f41a1ba17
3 changed files with 182 additions and 20 deletions
|
|
@ -17,6 +17,7 @@ export interface BM25SearchResult {
|
|||
filePath: string;
|
||||
score: number;
|
||||
rank: number;
|
||||
nodeIds?: string[];
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -79,7 +80,7 @@ async function queryFTSViaExecutor(
|
|||
indexName: string,
|
||||
query: string,
|
||||
limit: number,
|
||||
): Promise<Array<{ filePath: string; score: number }>> {
|
||||
): Promise<Array<{ filePath: string; score: number; nodeId: string }>> {
|
||||
// Escape single quotes and backslashes to prevent Cypher injection
|
||||
const escapedQuery = query.replace(/\\/g, '\\\\').replace(/'/g, "''");
|
||||
const cypher = `
|
||||
|
|
@ -96,6 +97,7 @@ async function queryFTSViaExecutor(
|
|||
return {
|
||||
filePath: node.filePath || '',
|
||||
score: typeof score === 'number' ? score : parseFloat(score) || 0,
|
||||
nodeId: node.nodeId || node.id || '',
|
||||
};
|
||||
});
|
||||
} catch {
|
||||
|
|
@ -167,17 +169,13 @@ export const searchFTSFromLbug = async (
|
|||
);
|
||||
}
|
||||
|
||||
// Merge results by filePath, summing scores for same file
|
||||
const merged = new Map<string, { filePath: string; score: number }>();
|
||||
// Collect all node scores per filePath to track which nodes actually matched
|
||||
const fileNodeScores = new Map<string, Array<{ score: number; nodeId: string }>>();
|
||||
|
||||
const addResults = (results: any[]) => {
|
||||
for (const r of results) {
|
||||
const existing = merged.get(r.filePath);
|
||||
if (existing) {
|
||||
existing.score += r.score;
|
||||
} else {
|
||||
merged.set(r.filePath, { filePath: r.filePath, score: r.score });
|
||||
}
|
||||
if (!fileNodeScores.has(r.filePath)) fileNodeScores.set(r.filePath, []);
|
||||
fileNodeScores.get(r.filePath)!.push({ score: r.score, nodeId: r.nodeId });
|
||||
}
|
||||
};
|
||||
|
||||
|
|
@ -187,6 +185,19 @@ export const searchFTSFromLbug = async (
|
|||
addResults(methodResults);
|
||||
addResults(interfaceResults);
|
||||
|
||||
// Sum the top-3 highest-scoring nodes per file and collect their nodeIds.
|
||||
// Summing all nodes naively inflates scores for files with many mediocre
|
||||
// matches (e.g. test files) over files with a single highly-relevant symbol.
|
||||
const merged = new Map<string, { filePath: string; score: number; nodeIds: string[] }>();
|
||||
for (const [filePath, entries] of fileNodeScores) {
|
||||
const top3 = [...entries].sort((a, b) => b.score - a.score).slice(0, 3);
|
||||
merged.set(filePath, {
|
||||
filePath,
|
||||
score: top3.reduce((acc, e) => acc + e.score, 0),
|
||||
nodeIds: top3.map((e) => e.nodeId).filter((id) => id),
|
||||
});
|
||||
}
|
||||
|
||||
// Sort by score descending and add rank
|
||||
const sorted = Array.from(merged.values())
|
||||
.sort((a, b) => b.score - a.score)
|
||||
|
|
@ -196,5 +207,6 @@ export const searchFTSFromLbug = async (
|
|||
filePath: r.filePath,
|
||||
score: r.score,
|
||||
rank: index + 1,
|
||||
nodeIds: r.nodeIds,
|
||||
}));
|
||||
};
|
||||
|
|
|
|||
|
|
@ -851,16 +851,30 @@ export class LocalBackend {
|
|||
for (const bm25Result of bm25Results) {
|
||||
const fullPath = bm25Result.filePath;
|
||||
try {
|
||||
const symbols = await executeParameterized(
|
||||
repo.id,
|
||||
`
|
||||
MATCH (n)
|
||||
WHERE n.filePath = $filePath
|
||||
RETURN n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine
|
||||
LIMIT 3
|
||||
`,
|
||||
{ filePath: fullPath },
|
||||
);
|
||||
// Prefer direct nodeId lookup (exact FTS-matched nodes) over filePath fallback.
|
||||
// Without this, LIMIT 3 on filePath returns arbitrary symbols rather than
|
||||
// the nodes that actually scored highest in the BM25 index.
|
||||
const nodeIds = bm25Result.nodeIds?.length ? bm25Result.nodeIds : null;
|
||||
const symbols = nodeIds
|
||||
? await executeParameterized(
|
||||
repo.id,
|
||||
`
|
||||
MATCH (n)
|
||||
WHERE n.id IN $nodeIds
|
||||
RETURN n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine
|
||||
`,
|
||||
{ nodeIds },
|
||||
)
|
||||
: await executeParameterized(
|
||||
repo.id,
|
||||
`
|
||||
MATCH (n)
|
||||
WHERE n.filePath = $filePath
|
||||
RETURN n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine
|
||||
LIMIT 3
|
||||
`,
|
||||
{ filePath: fullPath },
|
||||
);
|
||||
|
||||
if (symbols.length > 0) {
|
||||
for (const sym of symbols) {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,14 @@
|
|||
import { describe, it, expect } from 'vitest';
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { searchFTSFromLbug, type BM25SearchResult } from '../../src/core/search/bm25-index.js';
|
||||
|
||||
vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../../src/core/lbug/lbug-adapter.js')>();
|
||||
return {
|
||||
...actual,
|
||||
queryFTS: vi.fn().mockResolvedValue([]),
|
||||
};
|
||||
});
|
||||
|
||||
describe('BM25 search', () => {
|
||||
describe('searchFTSFromLbug', () => {
|
||||
it('returns empty array when LadybugDB is not initialized', async () => {
|
||||
|
|
@ -32,5 +40,133 @@ describe('BM25 search', () => {
|
|||
expect(result.score).toBe(1.5);
|
||||
expect(result.rank).toBe(1);
|
||||
});
|
||||
|
||||
it('accepts optional nodeIds field', () => {
|
||||
const result: BM25SearchResult = {
|
||||
filePath: 'src/index.ts',
|
||||
score: 1.5,
|
||||
rank: 1,
|
||||
nodeIds: ['func:id1', 'func:id2'],
|
||||
};
|
||||
expect(result.nodeIds).toEqual(['func:id1', 'func:id2']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('score aggregation', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it('sums only top-3 scoring nodes per file when more than 3 match', async () => {
|
||||
const { queryFTS } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
// File table: empty; Function table: 5 hits for the same file; rest: empty
|
||||
vi.mocked(queryFTS)
|
||||
.mockResolvedValueOnce([]) // File
|
||||
.mockResolvedValueOnce([
|
||||
// Function — 5 hits, scores 10/9/8/7/6
|
||||
{ filePath: 'src/views.py', score: 10, nodeId: 'func:node1', name: 'get_queryset' },
|
||||
{ filePath: 'src/views.py', score: 9, nodeId: 'func:node2', name: 'post' },
|
||||
{ filePath: 'src/views.py', score: 8, nodeId: 'func:node3', name: 'delete' },
|
||||
{ filePath: 'src/views.py', score: 7, nodeId: 'func:node4', name: 'patch' },
|
||||
{ filePath: 'src/views.py', score: 6, nodeId: 'func:node5', name: 'put' },
|
||||
])
|
||||
.mockResolvedValueOnce([]) // Class
|
||||
.mockResolvedValueOnce([]) // Method
|
||||
.mockResolvedValueOnce([]); // Interface
|
||||
|
||||
const results = await searchFTSFromLbug('queryset');
|
||||
|
||||
expect(results).toHaveLength(1);
|
||||
expect(results[0].filePath).toBe('src/views.py');
|
||||
// Only top-3 scores (10+9+8=27), not naive sum of all 5 (10+9+8+7+6=40)
|
||||
expect(results[0].score).toBe(27);
|
||||
expect(results[0].nodeIds).toEqual(['func:node1', 'func:node2', 'func:node3']);
|
||||
});
|
||||
|
||||
it('propagates nodeIds for files with fewer than 3 matching nodes', async () => {
|
||||
const { queryFTS } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
vi.mocked(queryFTS)
|
||||
.mockResolvedValueOnce([]) // File
|
||||
.mockResolvedValueOnce([
|
||||
// Function — 2 hits
|
||||
{ filePath: 'src/models.py', score: 5, nodeId: 'func:m1', name: 'save' },
|
||||
{ filePath: 'src/models.py', score: 3, nodeId: 'func:m2', name: 'delete' },
|
||||
])
|
||||
.mockResolvedValueOnce([]) // Class
|
||||
.mockResolvedValueOnce([]) // Method
|
||||
.mockResolvedValueOnce([]); // Interface
|
||||
|
||||
const results = await searchFTSFromLbug('model');
|
||||
|
||||
expect(results).toHaveLength(1);
|
||||
expect(results[0].score).toBe(8); // 5+3
|
||||
expect(results[0].nodeIds).toEqual(['func:m1', 'func:m2']);
|
||||
});
|
||||
|
||||
it('filters out empty nodeIds', async () => {
|
||||
const { queryFTS } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
vi.mocked(queryFTS)
|
||||
.mockResolvedValueOnce([]) // File
|
||||
.mockResolvedValueOnce([
|
||||
// Function — nodes with no id
|
||||
{ filePath: 'src/utils.py', score: 5, nodeId: '', name: 'helper' },
|
||||
{ filePath: 'src/utils.py', score: 3, nodeId: '', name: 'util' },
|
||||
])
|
||||
.mockResolvedValueOnce([]) // Class
|
||||
.mockResolvedValueOnce([]) // Method
|
||||
.mockResolvedValueOnce([]); // Interface
|
||||
|
||||
const results = await searchFTSFromLbug('util');
|
||||
|
||||
expect(results).toHaveLength(1);
|
||||
expect(results[0].nodeIds).toEqual([]);
|
||||
});
|
||||
|
||||
it('merges hits across multiple index tables for the same file', async () => {
|
||||
const { queryFTS } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
vi.mocked(queryFTS)
|
||||
.mockResolvedValueOnce([
|
||||
// File table
|
||||
{ filePath: 'src/auth.py', score: 4, nodeId: 'file:auth', name: 'auth.py' },
|
||||
])
|
||||
.mockResolvedValueOnce([
|
||||
// Function table
|
||||
{ filePath: 'src/auth.py', score: 9, nodeId: 'func:login', name: 'login' },
|
||||
])
|
||||
.mockResolvedValueOnce([
|
||||
// Class table
|
||||
{ filePath: 'src/auth.py', score: 7, nodeId: 'cls:User', name: 'User' },
|
||||
])
|
||||
.mockResolvedValueOnce([]) // Method
|
||||
.mockResolvedValueOnce([]); // Interface
|
||||
|
||||
const results = await searchFTSFromLbug('auth');
|
||||
|
||||
expect(results).toHaveLength(1);
|
||||
// All 3 hits (scores 9+7+4=20) — each from a different table, all top-3
|
||||
expect(results[0].score).toBe(20);
|
||||
expect(results[0].nodeIds).toEqual(['func:login', 'cls:User', 'file:auth']);
|
||||
});
|
||||
|
||||
it('ranks files by aggregated score descending', async () => {
|
||||
const { queryFTS } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
vi.mocked(queryFTS)
|
||||
.mockResolvedValueOnce([]) // File
|
||||
.mockResolvedValueOnce([
|
||||
// Function — hits across two files
|
||||
{ filePath: 'src/low.py', score: 2, nodeId: 'func:a', name: 'a' },
|
||||
{ filePath: 'src/high.py', score: 9, nodeId: 'func:b', name: 'b' },
|
||||
])
|
||||
.mockResolvedValueOnce([]) // Class
|
||||
.mockResolvedValueOnce([]) // Method
|
||||
.mockResolvedValueOnce([]); // Interface
|
||||
|
||||
const results = await searchFTSFromLbug('fn');
|
||||
|
||||
expect(results[0].filePath).toBe('src/high.py');
|
||||
expect(results[1].filePath).toBe('src/low.py');
|
||||
expect(results[0].rank).toBe(1);
|
||||
expect(results[1].rank).toBe(2);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue