mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-29 06:32:28 +00:00
fix(search): index description field for FTS so doc comments are keyword-searchable (#2300)
* fix(search): index description column for FTS so doc comments are keyword-searchable Closes #2299. descriptionExtractor (#2286) populates the `description` column for every symbol table, but FTS only indexed name+content on 5 tables, so doc-comment keywords (Javadoc/KDoc/godoc/Rust ///) were invisible to BM25 keyword search. - Add `description` to the Function/Class/Method/Interface FTS indexes (File has no description column, left as name+content). - Add FTS indexes for the remaining EMBEDDABLE_LABELS symbol tables (Struct, Enum, Trait, Impl, Macro, Namespace, Constructor, TypeAlias, Typedef, Const, Property, Record, Union, Static, Variable). - createSearchFTSIndexes now drops-then-creates each index so the schema change reaches existing DBs on incremental re-analyze and --repair-fts (createFTSIndex is idempotent-by-name and would otherwise skip stale indexes). Tests: fts-schema column-subset + coverage guards; drop-before-create order; e2e doc-comment keyword search (Java class + Rust struct found by description-only terms). bm25-search assertions derive from FTS_INDEXES. * fix(review): apply autofix feedback - Guard the --repair-fts path on FTS-extension availability before createSearchFTSIndexes drops-then-creates indexes (P1 regression: without the gate, an unavailable extension could drop existing indexes then fail to recreate them, leaving the DB index-less). Mirrors the analyze path's ftsAvailable gate and fails loudly first. - Add a re-analyze upgrade integration test: seed an old name+content-only DB (no Struct index), run the real createSearchFTSIndexes(), and assert description keyword search + the previously un-indexed Struct now resolve. Proves drop-then-create upgrades a live stale index end-to-end. * fix(ci): add loadFTSExtension to --repair-fts test mocks The R3 review fix added a loadFTSExtension availability gate to the --repair-fts path, but run-analyze-fts-repair.test.ts mocked the lbug adapter without that export, so both repair tests threw `No "loadFTSExtension" export`. Add loadFTSExtension to the two mocks (returning true to preserve their original intent) and add a dedicated test proving the guard fails loudly — and does NOT drop any index — when the extension is unavailable. * test(fts): run fts-description-search in the sequential lbug-db project It was the only FTS-index-creating integration test left in the parallel `default` vitest project; every other ftsIndexes-using test (search-core, search-pool, augmentation, …) runs in the `lbug-db` project, which forces fileParallelism: false to avoid LadybugDB native mmap file-lock conflicts in parallel forks (Windows). Add it to the lbug-db include list and the default exclude list to match the convention and remove the flake risk. * test(ci): fail loudly when FTS extension is unavailable, never silently skip FTS-dependent lbug integration suites (search-core, search-pool, augmentation, fts-description-search, …) self-skip via ctx.skip() when the LadybugDB FTS extension can't load, emitting only a console.warn while the job stays green. That means a broken/missing FTS extension in CI would make these integration tests silently vanish with no signal — false confidence. withTestLbugDB now honors GITNEXUS_REQUIRE_FTS=1: when set and the extension is unavailable, setup() throws instead of skipping, so the suite fails loudly. The CI test jobs (ubuntu coverage + windows/macOS cross-platform) set the flag; local/offline runs leave it unset and keep skipping gracefully. (Verified the extension currently loads on all three runners, so this is a guard against regression, not a behavior change today.) * test(ci): run fts-description-search on macOS/Windows cross-platform jobs The new FTS description-search suite was registered in the sequential lbug-db vitest project (ubuntu/coverage) but absent from LBUG_NATIVE, so the macOS/Windows platform-sensitive jobs (which run only the explicit ALL_CROSS_PLATFORM allowlist via run-cross-platform.ts) never executed it. The GITNEXUS_REQUIRE_FTS=1 hardening on those jobs guarded the old FTS fixtures but not the new 20-index/description path. Add the suite to LBUG_NATIVE so the new path is validated cross-platform too. Refs #2299. * fix(search): verify FTS indexes cover description, not just queryability verifySearchFTSIndexes probed each index with QUERY_FTS_INDEX and treated 'queryable' as 'present'. A stale name+content-only index left on a pre-#2299 DB stays queryable yet silently misses the description column, so verification would pass green while doc-comment search stayed broken. Switch to a single CALL SHOW_INDEXES() that exposes property_names per index, and report an index as missing when it is absent OR does not cover its configured columns. Return contract (string[] of table.indexName) is unchanged, so both run-analyze.ts call sites are untouched. The per-index string interpolation is gone, so the now-dead safeIdentifier helper is removed. The real caller of the live function in tests is bm25-search.test.ts (the repair test mocks verifySearchFTSIndexes wholesale); its two probe-shaped cases are rewritten to feed SHOW_INDEXES rows and now assert column coverage, plus an absent-index case. Refs #2299. * test(search): assert description search via the public query surface The #2299 integration suite only exercised the searchFTSFromLbug helper. Add a third block that drives the public LocalBackend.callTool('query') path — which resolves the repo via the registry and routes BM25 through the pool adapter (a different connection context than the core-adapter helper) — and asserts a description-only keyword returns the seeded class. Reuses the existing description-only SEED and production FTS_INDEXES; partial-mocks repo-manager so listRegisteredRepos points at the test DB while cleanupOldKuzuFiles and the rest stay real. Refs #2299. * test(search): make lbug-core-adapter FTS gate honor GITNEXUS_REQUIRE_FTS lbug-core-adapter.test.ts has its own per-test FTS gate (skipUnlessFtsAvailable) that called ctx.skip() whenever the extension could not load — bypassing the GITNEXUS_REQUIRE_FTS=1 hardening that withTestLbugDB already honors. Since this file is in LBUG_NATIVE it runs on the ubuntu/macOS/windows jobs that all set GITNEXUS_REQUIRE_FTS=1, so an FTS regression on a runner would have let these FTS-primitive tests silently vanish from a green run — the exact gap #2299's test-infra hardening set out to close. Make the helper mirror withTestLbugDB: when GITNEXUS_REQUIRE_FTS=1 and the extension is unavailable, throw (hard fail) instead of skipping. Offline/local runs (no env var) still skip gracefully. Refs #2299.
This commit is contained in:
Gergő Magyar
GitHub
parent
51716acb48
commit
82f1e48f32
13 changed files with 499 additions and 55 deletions
8
.github/workflows/ci-tests.yml
vendored
8
.github/workflows/ci-tests.yml
vendored
|
|
@ -11,6 +11,10 @@ jobs:
|
|||
name: ubuntu / coverage
|
||||
runs-on: ubuntu-latest
|
||||
timeout-minutes: 25
|
||||
# Fail loudly (don't silently skip) if the FTS extension is unavailable, so
|
||||
# FTS-dependent lbug integration suites are guaranteed to run in CI.
|
||||
env:
|
||||
GITNEXUS_REQUIRE_FTS: '1'
|
||||
steps:
|
||||
# persist-credentials: false — this job runs tests and uploads a
|
||||
# test-reports artifact (if: always()). The default-persisted token in
|
||||
|
|
@ -77,6 +81,10 @@ jobs:
|
|||
os: [windows-latest, macos-latest]
|
||||
runs-on: ${{ matrix.os }}
|
||||
timeout-minutes: 20
|
||||
# Same guarantee on the platform-sensitive runners: FTS-dependent suites in
|
||||
# the cross-platform subset must run, not silently skip.
|
||||
env:
|
||||
GITNEXUS_REQUIRE_FTS: '1'
|
||||
steps:
|
||||
# persist-credentials: false — runs tests only, never pushes (zizmor
|
||||
# credential-persistence / artipacked audit).
|
||||
|
|
|
|||
|
|
@ -70,6 +70,7 @@ const LBUG_NATIVE = [
|
|||
'test/integration/local-backend-calltool.test.ts',
|
||||
'test/integration/search-core.test.ts',
|
||||
'test/integration/search-pool.test.ts',
|
||||
'test/integration/fts-description-search.test.ts',
|
||||
'test/integration/staleness-and-stability.test.ts',
|
||||
'test/integration/analyze-wal-checkpoint-failure.test.ts',
|
||||
];
|
||||
|
|
|
|||
|
|
@ -658,6 +658,22 @@ export async function runFullAnalysis(
|
|||
}
|
||||
try {
|
||||
await initLbug(lbugPath);
|
||||
// Gate on FTS availability BEFORE touching any index. createSearchFTSIndexes
|
||||
// now DROPs each index before recreating it (so schema changes reach existing
|
||||
// DBs); if the extension were unavailable, the drops would run and leave the
|
||||
// DB index-less, only failing at the create step. Fail loudly first — mirrors
|
||||
// the analyze path's `if (ftsAvailable)` gate below — so an unavailable
|
||||
// extension never destroys the existing indexes.
|
||||
const repairFtsAvailable = await loadFTSExtension(undefined, {
|
||||
policy: resolveAnalyzeInstallPolicy(),
|
||||
});
|
||||
if (!repairFtsAvailable) {
|
||||
throw new Error(
|
||||
'Cannot repair FTS indexes: the LadybugDB FTS extension is unavailable ' +
|
||||
'(not pre-installed and could not be installed on this machine). ' +
|
||||
'Run `gitnexus doctor` to install it, then retry `--repair-fts`.',
|
||||
);
|
||||
}
|
||||
progress('fts', 85, 'Repairing search indexes...');
|
||||
await createSearchFTSIndexes({
|
||||
onIndexStart: options.verbose
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { createFTSIndex } from '../lbug/lbug-adapter.js';
|
||||
import { createFTSIndex, dropFTSIndex } from '../lbug/lbug-adapter.js';
|
||||
import { FTS_INDEXES } from './fts-schema.js';
|
||||
|
||||
export interface CreateSearchFTSIndexesOptions {
|
||||
|
|
@ -11,6 +11,18 @@ export async function createSearchFTSIndexes(
|
|||
): Promise<void> {
|
||||
for (const { table, indexName, properties } of FTS_INDEXES) {
|
||||
options?.onIndexStart?.(table, indexName);
|
||||
// Drop first so the live `properties` always win. `createFTSIndex` is
|
||||
// idempotent-by-name (skips when the index already exists), so without the
|
||||
// drop a schema change — e.g. adding `description` (#2299) — would never
|
||||
// reach an existing `.lbug` DB on an incremental re-analyze or `--repair-fts`;
|
||||
// the old name+content index would silently persist. `dropFTSIndex` no-ops
|
||||
// when the index is absent (first-ever analyze) and clears the per-connection
|
||||
// memo so the create below actually runs.
|
||||
// ponytail: this rebuilds every FTS index on every analyze instead of
|
||||
// skipping when present; FTS build is proportional to symbol-table size and
|
||||
// runs inside the existing FTS phase. Gate on a stored schema fingerprint if
|
||||
// this rebuild cost ever shows up in analyze profiles.
|
||||
await dropFTSIndex(table, indexName);
|
||||
await createFTSIndex(table, indexName, [...properties]);
|
||||
options?.onIndexReady?.(table, indexName);
|
||||
}
|
||||
|
|
@ -19,25 +31,32 @@ export async function createSearchFTSIndexes(
|
|||
export async function verifySearchFTSIndexes(
|
||||
executeQuery: (cypher: string) => Promise<unknown[]>,
|
||||
): Promise<string[]> {
|
||||
const safeIdentifier = (value: string): string => {
|
||||
if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(value)) {
|
||||
throw new Error(`Invalid FTS identifier: ${value}`);
|
||||
}
|
||||
return value;
|
||||
};
|
||||
// Read the catalog once and check each configured index both EXISTS and
|
||||
// covers its expected columns. A queryability-only probe (CALL QUERY_FTS_INDEX
|
||||
// ... catch) is not enough: a stale `name+content`-only index left on a
|
||||
// pre-#2299 DB stays queryable yet silently misses `description`, so the probe
|
||||
// would pass while doc-comment search is still broken (#2299). SHOW_INDEXES
|
||||
// exposes `property_names` (STRING[]) per index, so we assert coverage directly.
|
||||
const rows = await executeQuery('CALL SHOW_INDEXES() RETURN *');
|
||||
|
||||
const propsByIndex = new Map<string, readonly string[]>();
|
||||
for (const row of rows) {
|
||||
if (typeof row !== 'object' || row === null) continue;
|
||||
const record = row as Record<string, unknown>;
|
||||
const indexName = record.index_name;
|
||||
const propertyNames = record.property_names;
|
||||
if (typeof indexName !== 'string' || !Array.isArray(propertyNames)) continue;
|
||||
propsByIndex.set(
|
||||
indexName,
|
||||
propertyNames.filter((p): p is string => typeof p === 'string'),
|
||||
);
|
||||
}
|
||||
|
||||
const missing: string[] = [];
|
||||
for (const { table, indexName } of FTS_INDEXES) {
|
||||
const safeTable = safeIdentifier(table);
|
||||
const safeIndex = safeIdentifier(indexName);
|
||||
const probe = `
|
||||
CALL QUERY_FTS_INDEX('${safeTable}', '${safeIndex}', '__gitnexus_fts_probe__', conjunctive := false)
|
||||
RETURN score
|
||||
LIMIT 1
|
||||
`;
|
||||
try {
|
||||
await executeQuery(probe);
|
||||
} catch {
|
||||
for (const { table, indexName, properties } of FTS_INDEXES) {
|
||||
const actual = propsByIndex.get(indexName);
|
||||
// Absent from the catalog, or present but not covering every expected column.
|
||||
if (!actual || !properties.every((p) => actual.includes(p))) {
|
||||
missing.push(`${table}.${indexName}`);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,10 +4,44 @@ export interface FTSIndexDefinition {
|
|||
readonly properties: readonly string[];
|
||||
}
|
||||
|
||||
// Shared by both index creation (`createSearchFTSIndexes`) and querying
|
||||
// (`searchFTSFromLbug` / `verifySearchFTSIndexes`) — the single source of truth
|
||||
// for which tables/columns are full-text searchable. Adding `description` here
|
||||
// makes doc comments (Javadoc/KDoc/JSDoc/Doxygen/godoc/RDoc) keyword-searchable
|
||||
// once they are populated by `descriptionExtractor` (#2270/#2286, issue #2299).
|
||||
//
|
||||
// IMPORTANT: every property must be a real column on its table (see
|
||||
// `core/lbug/schema.ts`). `File` has no `description` column, so it stays
|
||||
// name+content. All other entries below carry a `description` column.
|
||||
//
|
||||
// Tables beyond the original 5 mirror `EMBEDDABLE_LABELS` (embeddings/types.ts):
|
||||
// indexing the same set keeps a symbol's doc comment both keyword- and
|
||||
// semantically-searchable.
|
||||
const FTS_PROPERTIES = ['name', 'content', 'description'] as const;
|
||||
|
||||
export const FTS_INDEXES: readonly FTSIndexDefinition[] = [
|
||||
// File has no `description` column — keep it name+content only.
|
||||
{ table: 'File', indexName: 'file_fts', properties: ['name', 'content'] },
|
||||
{ table: 'Function', indexName: 'function_fts', properties: ['name', 'content'] },
|
||||
{ table: 'Class', indexName: 'class_fts', properties: ['name', 'content'] },
|
||||
{ table: 'Method', indexName: 'method_fts', properties: ['name', 'content'] },
|
||||
{ table: 'Interface', indexName: 'interface_fts', properties: ['name', 'content'] },
|
||||
// Original 5 (minus File) gain `description`.
|
||||
{ table: 'Function', indexName: 'function_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Class', indexName: 'class_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Method', indexName: 'method_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Interface', indexName: 'interface_fts', properties: FTS_PROPERTIES },
|
||||
// Remaining EMBEDDABLE_LABELS symbol tables — all CODE_ELEMENT_BASE-shaped
|
||||
// (or a superset), so all carry name + content + description columns.
|
||||
{ table: 'Constructor', indexName: 'constructor_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Struct', indexName: 'struct_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Enum', indexName: 'enum_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Trait', indexName: 'trait_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Impl', indexName: 'impl_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Macro', indexName: 'macro_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Namespace', indexName: 'namespace_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'TypeAlias', indexName: 'type_alias_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Typedef', indexName: 'typedef_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Const', indexName: 'const_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Property', indexName: 'property_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Record', indexName: 'record_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Union', indexName: 'union_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Static', indexName: 'static_fts', properties: FTS_PROPERTIES },
|
||||
{ table: 'Variable', indexName: 'variable_fts', properties: FTS_PROPERTIES },
|
||||
];
|
||||
|
|
|
|||
|
|
@ -74,12 +74,15 @@ export function withTestLbugDB(
|
|||
// init on Windows CI regularly exceeds 30s due to native resource setup.
|
||||
const timeout = options?.timeout ?? 120_000;
|
||||
|
||||
// Suites that seed FTS indexes need the optional FTS extension. It is not
|
||||
// guaranteed on every machine (e.g. the macOS platform-sensitive CI runner,
|
||||
// where it is neither pre-installed nor installable). Track availability so
|
||||
// setup can skip FTS seeding instead of throwing, and so every test in the
|
||||
// suite is skipped rather than failing against a missing index. (PR #1161.)
|
||||
// Suites that seed FTS indexes need the optional FTS extension. On a dev
|
||||
// machine it may be neither pre-installed nor installable (offline), so we
|
||||
// track availability and skip the suite rather than fail against a missing
|
||||
// index (PR #1161). In CI this graceful skip is dangerous: an FTS-dependent
|
||||
// integration suite would silently vanish while the job stays green. When
|
||||
// GITNEXUS_REQUIRE_FTS=1 (set by the CI test jobs), an unavailable extension
|
||||
// is a HARD FAILURE so these tests can never silently stop protecting.
|
||||
const ftsRequired = !!options?.ftsIndexes?.length;
|
||||
const ftsMustBeAvailable = process.env.GITNEXUS_REQUIRE_FTS === '1';
|
||||
let ftsAvailable = true;
|
||||
let ftsSkipWarned = false;
|
||||
|
||||
|
|
@ -97,11 +100,21 @@ export function withTestLbugDB(
|
|||
// 1b. Probe the FTS extension for suites that need it, mirroring the
|
||||
// analyze write path (`auto`: LOAD-first, then one bounded INSTALL).
|
||||
// When it still cannot load, the suite is skipped (see beforeEach)
|
||||
// and FTS seeding below is bypassed so setup never throws.
|
||||
// and FTS seeding below is bypassed so setup never throws — UNLESS
|
||||
// GITNEXUS_REQUIRE_FTS=1, in which case CI fails loudly instead of
|
||||
// letting an FTS-dependent integration suite silently disappear.
|
||||
if (ftsRequired) {
|
||||
ftsAvailable = await adapter.loadFTSExtension(undefined, {
|
||||
policy: resolveAnalyzeInstallPolicy(),
|
||||
});
|
||||
if (!ftsAvailable && ftsMustBeAvailable) {
|
||||
throw new Error(
|
||||
`[withTestLbugDB(${prefix})] FTS extension is required (GITNEXUS_REQUIRE_FTS=1) ` +
|
||||
'but could not be loaded or installed. FTS-dependent integration tests must not ' +
|
||||
'be silently skipped in CI — install/repair the LadybugDB FTS extension ' +
|
||||
'(see `gitnexus doctor`) or unset GITNEXUS_REQUIRE_FTS for offline/local runs.',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// 2. Drop stale FTS indexes from previous test file
|
||||
|
|
|
|||
162
gitnexus/test/integration/fts-description-search.test.ts
Normal file
162
gitnexus/test/integration/fts-description-search.test.ts
Normal file
|
|
@ -0,0 +1,162 @@
|
|||
/**
|
||||
* Integration test for issue #2299: doc-comment text stored in the `description`
|
||||
* column must be reachable via keyword (BM25/FTS) search.
|
||||
*
|
||||
* Drives the *production* `FTS_INDEXES` through the test harness (not a bespoke
|
||||
* fixture list), so removing `description` from a symbol table's index — or
|
||||
* dropping a table from FTS coverage — fails this test. Seed rows place the
|
||||
* searched keywords ONLY in `description` (never in `name`/`content`) to prove
|
||||
* the description column is what matches.
|
||||
*/
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import { withTestLbugDB, type IndexedDBHandle } from '../helpers/test-indexed-db.js';
|
||||
import { searchFTSFromLbug } from '../../src/core/search/bm25-index.js';
|
||||
import { FTS_INDEXES } from '../../src/core/search/fts-schema.js';
|
||||
import { LocalBackend } from '../../src/mcp/local/local-backend.js';
|
||||
import { listRegisteredRepos } from '../../src/storage/repo-manager.js';
|
||||
|
||||
// The public query surface (LocalBackend.callTool('query')) resolves the repo
|
||||
// via the registry and routes BM25 through the pool adapter, so the third block
|
||||
// below needs listRegisteredRepos mocked to point at the test DB. Inert for the
|
||||
// two core-adapter blocks — they call searchFTSFromLbug directly (no registry).
|
||||
vi.mock('../../src/storage/repo-manager.js', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../../src/storage/repo-manager.js')>();
|
||||
return {
|
||||
...actual,
|
||||
listRegisteredRepos: vi.fn().mockResolvedValue([]),
|
||||
};
|
||||
});
|
||||
|
||||
const SEED = [
|
||||
// Java class: Javadoc keywords live in `description`, NOT in name/content.
|
||||
`CREATE (n:Class {id: 'class:RetryScheduler', name: 'RetryScheduler', filePath: 'src/RetryScheduler.java', startLine: 5, endLine: 40, isExported: true, content: 'public class RetryScheduler schedules retries', description: 'Implements the circuit breaker pattern for distributed service mesh fault tolerance, isolating failing downstream dependencies to prevent cascade failures'})`,
|
||||
// Rust struct: a table with NO FTS index before this change. Keywords live in
|
||||
// `description` only — proves the new-table coverage half of the fix.
|
||||
`CREATE (n:Struct {id: 'struct:LruShard', name: 'LruShard', filePath: 'src/cache.rs', startLine: 1, endLine: 20, content: 'struct LruShard holds entries', description: 'least recently used eviction policy for a bounded capacity cache'})`,
|
||||
];
|
||||
|
||||
const PRODUCTION_FTS_INDEXES = FTS_INDEXES.map((i) => ({
|
||||
table: i.table,
|
||||
indexName: i.indexName,
|
||||
columns: [...i.properties],
|
||||
}));
|
||||
|
||||
withTestLbugDB(
|
||||
'fts-description-search',
|
||||
() => {
|
||||
describe('description column is keyword-searchable (#2299)', () => {
|
||||
it('finds a class by Javadoc keywords present only in description', async () => {
|
||||
const { results } = await searchFTSFromLbug('circuit breaker fault tolerance', 20);
|
||||
expect(results.map((r) => r.filePath)).toContain('src/RetryScheduler.java');
|
||||
});
|
||||
|
||||
it('still finds the same class by name (no regression)', async () => {
|
||||
const { results } = await searchFTSFromLbug('RetryScheduler', 20);
|
||||
expect(results.map((r) => r.filePath)).toContain('src/RetryScheduler.java');
|
||||
});
|
||||
|
||||
it('finds a Struct (previously un-indexed table) by its doc-comment keywords', async () => {
|
||||
const { results } = await searchFTSFromLbug('least recently used eviction', 20);
|
||||
expect(results.map((r) => r.filePath)).toContain('src/cache.rs');
|
||||
});
|
||||
});
|
||||
},
|
||||
{
|
||||
seed: SEED,
|
||||
ftsIndexes: PRODUCTION_FTS_INDEXES,
|
||||
},
|
||||
);
|
||||
|
||||
// Second scenario: simulate a pre-#2299 database — FTS indexes built with the old
|
||||
// name+content-only schema, and no Struct index at all — then run the real
|
||||
// createSearchFTSIndexes() exactly as an incremental re-analyze / --repair-fts
|
||||
// would. This proves the drop-then-create behavior actually UPGRADES a live stale
|
||||
// index (the whole reason U2 exists): the old code's idempotent-by-name create
|
||||
// would skip the existing class_fts and description search would stay broken.
|
||||
const OLD_SCHEMA_FTS_INDEXES = [
|
||||
{ table: 'File', indexName: 'file_fts', columns: ['name', 'content'] },
|
||||
{ table: 'Class', indexName: 'class_fts', columns: ['name', 'content'] },
|
||||
// Struct intentionally omitted — pre-#2299 had no FTS index for it.
|
||||
];
|
||||
|
||||
withTestLbugDB(
|
||||
'fts-description-reindex-upgrade',
|
||||
() => {
|
||||
describe('re-analyze upgrades stale FTS indexes so description becomes searchable (#2299)', () => {
|
||||
it('finds a class by description keywords after re-indexing an old name+content-only DB', async () => {
|
||||
const { results } = await searchFTSFromLbug('circuit breaker fault tolerance', 20);
|
||||
expect(results.map((r) => r.filePath)).toContain('src/RetryScheduler.java');
|
||||
});
|
||||
|
||||
it('makes a previously un-indexed table (Struct) searchable after re-indexing', async () => {
|
||||
const { results } = await searchFTSFromLbug('least recently used eviction', 20);
|
||||
expect(results.map((r) => r.filePath)).toContain('src/cache.rs');
|
||||
});
|
||||
});
|
||||
},
|
||||
{
|
||||
seed: SEED,
|
||||
ftsIndexes: OLD_SCHEMA_FTS_INDEXES,
|
||||
afterSetup: async () => {
|
||||
// Real production index build over the now-stale DB — drops then recreates.
|
||||
const { createSearchFTSIndexes } = await import('../../src/core/search/fts-indexes.js');
|
||||
await createSearchFTSIndexes();
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
// Third scenario: prove the description-only keyword is reachable through the
|
||||
// PUBLIC query surface (LocalBackend.callTool('query')), not just the
|
||||
// searchFTSFromLbug helper the blocks above exercise. callTool resolves the repo
|
||||
// via the registry and routes BM25 through the pool adapter — a different
|
||||
// connection context than the core-adapter helper — so this block needs
|
||||
// poolAdapter + a listRegisteredRepos mock + a LocalBackend built in afterSetup.
|
||||
// Reuses the same description-only SEED and production FTS_INDEXES.
|
||||
withTestLbugDB(
|
||||
'fts-description-search-public-query',
|
||||
(handle) => {
|
||||
describe('public query surface returns description-only matches (#2299)', () => {
|
||||
it('finds a class by description keywords via callTool("query")', async () => {
|
||||
const ext = handle as IndexedDBHandle & { _backend?: LocalBackend };
|
||||
expect(ext._backend).toBeDefined();
|
||||
const backend = ext._backend!;
|
||||
|
||||
type QuerySymbol = { id: string };
|
||||
type QueryResult = {
|
||||
error?: unknown;
|
||||
definitions?: QuerySymbol[];
|
||||
process_symbols?: QuerySymbol[];
|
||||
};
|
||||
const result: QueryResult = await backend.callTool('query', {
|
||||
query: 'circuit breaker fault tolerance',
|
||||
});
|
||||
|
||||
expect(result.error).toBeUndefined();
|
||||
const ids = [...(result.process_symbols ?? []), ...(result.definitions ?? [])].map(
|
||||
(s) => s.id,
|
||||
);
|
||||
expect(ids).toContain('class:RetryScheduler');
|
||||
});
|
||||
});
|
||||
},
|
||||
{
|
||||
seed: SEED,
|
||||
ftsIndexes: PRODUCTION_FTS_INDEXES,
|
||||
poolAdapter: true,
|
||||
afterSetup: async (handle) => {
|
||||
vi.mocked(listRegisteredRepos).mockResolvedValue([
|
||||
{
|
||||
name: 'test-repo',
|
||||
path: '/test/repo',
|
||||
storagePath: handle.tmpHandle.dbPath,
|
||||
indexedAt: new Date().toISOString(),
|
||||
lastCommit: 'abc123',
|
||||
stats: { files: 1, nodes: 2, communities: 0, processes: 0 },
|
||||
},
|
||||
]);
|
||||
const backend = new LocalBackend();
|
||||
await backend.init();
|
||||
(handle as IndexedDBHandle & { _backend?: LocalBackend })._backend = backend;
|
||||
},
|
||||
},
|
||||
);
|
||||
|
|
@ -37,10 +37,25 @@ const FTS_UNAVAILABLE_NOTE =
|
|||
/**
|
||||
* Dynamically skip an FTS-primitive test when the extension cannot load.
|
||||
* `ctx.skip()` aborts the test, so callers should `await` this first thing.
|
||||
*
|
||||
* Honors GITNEXUS_REQUIRE_FTS=1 the same way `withTestLbugDB` does (see
|
||||
* test/helpers/test-indexed-db.ts): when CI sets it, an unavailable extension is
|
||||
* a HARD FAILURE, never a silent skip — otherwise these FTS-primitive tests
|
||||
* (this file is in LBUG_NATIVE, so it runs on the ubuntu/macOS/windows jobs that
|
||||
* all set GITNEXUS_REQUIRE_FTS=1) could vanish from a green run. Offline/local
|
||||
* runs (no env var) still skip gracefully (#2299).
|
||||
*/
|
||||
const skipUnlessFtsAvailable = async (ctx: { skip: (note?: string) => void }): Promise<void> => {
|
||||
const { loadFTSExtension } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
if (!(await loadFTSExtension())) ctx.skip(FTS_UNAVAILABLE_NOTE);
|
||||
if (await loadFTSExtension()) return;
|
||||
if (process.env.GITNEXUS_REQUIRE_FTS === '1') {
|
||||
throw new Error(
|
||||
'FTS extension is required (GITNEXUS_REQUIRE_FTS=1) but could not be loaded or installed. ' +
|
||||
'FTS-dependent tests must not be silently skipped in CI — install/repair the LadybugDB ' +
|
||||
'FTS extension (see `gitnexus doctor`) or unset GITNEXUS_REQUIRE_FTS for offline/local runs.',
|
||||
);
|
||||
}
|
||||
ctx.skip(FTS_UNAVAILABLE_NOTE);
|
||||
};
|
||||
|
||||
// ─── Core LadybugDB Adapter ─────────────────────────────────────────────
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { searchFTSFromLbug, type BM25SearchResult } from '../../src/core/search/bm25-index.js';
|
||||
import { FTS_INDEXES } from '../../src/core/search/fts-schema.js';
|
||||
|
||||
vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import('../../src/core/lbug/lbug-adapter.js')>();
|
||||
|
|
@ -7,6 +8,7 @@ vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => {
|
|||
...actual,
|
||||
queryFTS: vi.fn().mockResolvedValue([]),
|
||||
createFTSIndex: vi.fn().mockResolvedValue(undefined),
|
||||
dropFTSIndex: vi.fn().mockResolvedValue(undefined),
|
||||
};
|
||||
});
|
||||
|
||||
|
|
@ -26,45 +28,63 @@ describe('BM25 search', () => {
|
|||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
it('creates the configured indexes on the writable analysis path', async () => {
|
||||
it('creates every configured index on the writable analysis path', async () => {
|
||||
const { createFTSIndex } = await import('../../src/core/lbug/lbug-adapter.js');
|
||||
const { createSearchFTSIndexes } = await import('../../src/core/search/fts-indexes.js');
|
||||
|
||||
await createSearchFTSIndexes();
|
||||
|
||||
expect(vi.mocked(createFTSIndex).mock.calls).toEqual([
|
||||
['File', 'file_fts', ['name', 'content']],
|
||||
['Function', 'function_fts', ['name', 'content']],
|
||||
['Class', 'class_fts', ['name', 'content']],
|
||||
['Method', 'method_fts', ['name', 'content']],
|
||||
['Interface', 'interface_fts', ['name', 'content']],
|
||||
]);
|
||||
expect(vi.mocked(createFTSIndex).mock.calls).toEqual(
|
||||
FTS_INDEXES.map((i) => [i.table, i.indexName, [...i.properties]]),
|
||||
);
|
||||
});
|
||||
|
||||
it('verifies all configured FTS indexes are queryable', async () => {
|
||||
const executeQuery = vi.fn().mockResolvedValue([]);
|
||||
it('returns no missing indexes when every configured index covers its columns', async () => {
|
||||
// One SHOW_INDEXES call returns a catalog row per configured index, each
|
||||
// covering exactly its expected properties.
|
||||
const showIndexesRows = FTS_INDEXES.map((i) => ({
|
||||
index_name: i.indexName,
|
||||
property_names: [...i.properties],
|
||||
}));
|
||||
const executeQuery = vi.fn().mockResolvedValue(showIndexesRows);
|
||||
const { verifySearchFTSIndexes } = await import('../../src/core/search/fts-indexes.js');
|
||||
|
||||
const missing = await verifySearchFTSIndexes(executeQuery);
|
||||
|
||||
expect(missing).toEqual([]);
|
||||
expect(executeQuery).toHaveBeenCalledTimes(5);
|
||||
expect(executeQuery).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('reports missing indexes when an FTS probe fails', async () => {
|
||||
const executeQuery = vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce([])
|
||||
.mockRejectedValueOnce(new Error('index does not exist'))
|
||||
.mockResolvedValueOnce([])
|
||||
.mockResolvedValueOnce([])
|
||||
.mockResolvedValueOnce([]);
|
||||
it('reports an index that exists but does not cover its configured columns', async () => {
|
||||
// Model a pre-#2299 stale Function index: present, but name+content only,
|
||||
// missing `description`. Every other index covers its columns.
|
||||
const staleIndex = 'function_fts';
|
||||
const showIndexesRows = FTS_INDEXES.map((i) => ({
|
||||
index_name: i.indexName,
|
||||
property_names: i.indexName === staleIndex ? ['name', 'content'] : [...i.properties],
|
||||
}));
|
||||
const executeQuery = vi.fn().mockResolvedValue(showIndexesRows);
|
||||
const { verifySearchFTSIndexes } = await import('../../src/core/search/fts-indexes.js');
|
||||
|
||||
const missing = await verifySearchFTSIndexes(executeQuery);
|
||||
|
||||
expect(missing).toEqual(['Function.function_fts']);
|
||||
});
|
||||
|
||||
it('reports an index that is absent from the catalog entirely', async () => {
|
||||
// Every configured index present and covering, except const_fts is missing.
|
||||
const absentIndex = 'const_fts';
|
||||
const showIndexesRows = FTS_INDEXES.filter((i) => i.indexName !== absentIndex).map((i) => ({
|
||||
index_name: i.indexName,
|
||||
property_names: [...i.properties],
|
||||
}));
|
||||
const executeQuery = vi.fn().mockResolvedValue(showIndexesRows);
|
||||
const { verifySearchFTSIndexes } = await import('../../src/core/search/fts-indexes.js');
|
||||
|
||||
const missing = await verifySearchFTSIndexes(executeQuery);
|
||||
|
||||
expect(missing).toEqual(['Const.const_fts']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('searchFTSFromLbug', () => {
|
||||
|
|
@ -289,13 +309,9 @@ describe('BM25 search', () => {
|
|||
const queryCalls = mockExecuteParameterized.mock.calls.filter((c) =>
|
||||
String(c[1]).includes('QUERY_FTS_INDEX'),
|
||||
);
|
||||
expect(queryCalls.map((c) => String(c[1]).match(/QUERY_FTS_INDEX\('([^']+)'/)?.[1])).toEqual([
|
||||
'File',
|
||||
'Function',
|
||||
'Class',
|
||||
'Method',
|
||||
'Interface',
|
||||
]);
|
||||
expect(queryCalls.map((c) => String(c[1]).match(/QUERY_FTS_INDEX\('([^']+)'/)?.[1])).toEqual(
|
||||
FTS_INDEXES.map((i) => i.table),
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
44
gitnexus/test/unit/fts-indexes.test.ts
Normal file
44
gitnexus/test/unit/fts-indexes.test.ts
Normal file
|
|
@ -0,0 +1,44 @@
|
|||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
// Hoisted so the vi.mock factory can close over the shared call log.
|
||||
const { calls } = vi.hoisted(() => ({ calls: [] as string[] }));
|
||||
|
||||
vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({
|
||||
dropFTSIndex: vi.fn(async (table: string, indexName: string) => {
|
||||
calls.push(`drop:${table}.${indexName}`);
|
||||
}),
|
||||
createFTSIndex: vi.fn(async (table: string, indexName: string) => {
|
||||
calls.push(`create:${table}.${indexName}`);
|
||||
}),
|
||||
}));
|
||||
|
||||
const { createSearchFTSIndexes } = await import('../../src/core/search/fts-indexes.js');
|
||||
const { FTS_INDEXES } = await import('../../src/core/search/fts-schema.js');
|
||||
|
||||
afterEach(() => {
|
||||
calls.length = 0;
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
describe('createSearchFTSIndexes', () => {
|
||||
it('drops each index before (re)creating it, in order, for every entry', async () => {
|
||||
await createSearchFTSIndexes();
|
||||
const expected = FTS_INDEXES.flatMap((i) => [
|
||||
`drop:${i.table}.${i.indexName}`,
|
||||
`create:${i.table}.${i.indexName}`,
|
||||
]);
|
||||
expect(calls).toEqual(expected);
|
||||
});
|
||||
|
||||
it('invokes onIndexStart/onIndexReady once per index', async () => {
|
||||
const started: string[] = [];
|
||||
const ready: string[] = [];
|
||||
await createSearchFTSIndexes({
|
||||
onIndexStart: (_t, name) => started.push(name),
|
||||
onIndexReady: (_t, name) => ready.push(name),
|
||||
});
|
||||
const expectedNames = FTS_INDEXES.map((i) => i.indexName);
|
||||
expect(started).toEqual(expectedNames);
|
||||
expect(ready).toEqual(expectedNames);
|
||||
});
|
||||
});
|
||||
61
gitnexus/test/unit/fts-schema.test.ts
Normal file
61
gitnexus/test/unit/fts-schema.test.ts
Normal file
|
|
@ -0,0 +1,61 @@
|
|||
import { describe, expect, it } from 'vitest';
|
||||
import { FTS_INDEXES } from '../../src/core/search/fts-schema.js';
|
||||
import { NODE_SCHEMA_QUERIES } from '../../src/core/lbug/schema.js';
|
||||
import { EMBEDDABLE_LABELS } from '../../src/core/embeddings/types.js';
|
||||
|
||||
/**
|
||||
* Parse a `CREATE NODE TABLE` DDL string into its table name and declared
|
||||
* column names. Kept `if`-free so it never introduces a vacuous assertion path.
|
||||
*/
|
||||
const parseTableColumns = (ddl: string): { table: string; columns: string[] } => {
|
||||
const table = ddl.match(/CREATE NODE TABLE\s+`?([A-Za-z_][A-Za-z0-9_]*)`?/)?.[1] ?? '';
|
||||
const body = ddl.slice(ddl.indexOf('(') + 1, ddl.lastIndexOf(')'));
|
||||
const columns = body
|
||||
.split('\n')
|
||||
.map((line) => line.trim())
|
||||
.map((line) => line.match(/^([A-Za-z_][A-Za-z0-9_]*)\s+[A-Za-z]/)?.[1] ?? '')
|
||||
.filter((col) => col.length > 0 && col !== 'PRIMARY');
|
||||
return { table, columns };
|
||||
};
|
||||
|
||||
const COLUMNS_BY_TABLE = new Map<string, Set<string>>(
|
||||
NODE_SCHEMA_QUERIES.map(parseTableColumns).map(({ table, columns }) => [table, new Set(columns)]),
|
||||
);
|
||||
|
||||
describe('FTS_INDEXES', () => {
|
||||
it('keeps File at name+content only (File has no description column)', () => {
|
||||
const file = FTS_INDEXES.find((i) => i.table === 'File');
|
||||
expect(file).toMatchObject({ properties: ['name', 'content'] });
|
||||
});
|
||||
|
||||
it('indexes description for the four original symbol tables', () => {
|
||||
const tables = ['Function', 'Class', 'Method', 'Interface'];
|
||||
const withoutDescription = tables.filter(
|
||||
(t) => !FTS_INDEXES.find((i) => i.table === t)?.properties.includes('description'),
|
||||
);
|
||||
expect(withoutDescription).toEqual([]);
|
||||
});
|
||||
|
||||
it('only indexes columns that actually exist on each table', () => {
|
||||
const violations = FTS_INDEXES.flatMap((index) => {
|
||||
const columns = COLUMNS_BY_TABLE.get(index.table) ?? new Set<string>();
|
||||
return index.properties
|
||||
.filter((prop) => !columns.has(prop))
|
||||
.map((prop) => `${index.table}.${prop}`);
|
||||
});
|
||||
expect(violations).toEqual([]);
|
||||
});
|
||||
|
||||
it('covers every embeddable symbol table', () => {
|
||||
const indexed = new Set(FTS_INDEXES.map((i) => i.table));
|
||||
const missing = EMBEDDABLE_LABELS.filter((label) => !indexed.has(label));
|
||||
expect(missing).toEqual([]);
|
||||
});
|
||||
|
||||
it('uses unique, valid index names', () => {
|
||||
const names = FTS_INDEXES.map((i) => i.indexName);
|
||||
expect(new Set(names).size).toBe(names.length);
|
||||
const invalid = names.filter((n) => !/^[A-Za-z_][A-Za-z0-9_]*$/.test(n));
|
||||
expect(invalid).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -117,6 +117,8 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => {
|
|||
deleteNodesForFile: vi.fn(async () => undefined),
|
||||
deleteAllCommunitiesAndProcesses: vi.fn(async () => undefined),
|
||||
queryImporters: vi.fn(async () => []),
|
||||
// Repair path now gates on FTS availability before drop-then-create.
|
||||
loadFTSExtension: vi.fn(async () => true),
|
||||
}));
|
||||
vi.doMock('../../src/core/search/fts-indexes.js', () => ({
|
||||
createSearchFTSIndexes: vi.fn(async () => undefined),
|
||||
|
|
@ -164,6 +166,8 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => {
|
|||
deleteNodesForFile: vi.fn(async () => undefined),
|
||||
deleteAllCommunitiesAndProcesses: vi.fn(async () => undefined),
|
||||
queryImporters: vi.fn(async () => []),
|
||||
// Extension loads; the throw under test comes from index creation itself.
|
||||
loadFTSExtension: vi.fn(async () => true),
|
||||
}));
|
||||
vi.doMock('../../src/core/search/fts-indexes.js', () => ({
|
||||
createSearchFTSIndexes: vi.fn(async () => {
|
||||
|
|
@ -200,6 +204,55 @@ describe('runFullAnalysis FTS repair and verification failure paths', () => {
|
|||
}
|
||||
});
|
||||
|
||||
it('fails repair mode loudly WITHOUT dropping indexes when the FTS extension is unavailable', async () => {
|
||||
// Regression guard (#2299): createSearchFTSIndexes now drops each index
|
||||
// before recreating it. If the extension is unavailable, the repair path must
|
||||
// bail before any drop runs — otherwise it would destroy the existing indexes
|
||||
// and then fail to recreate them, leaving the DB worse off.
|
||||
const createSearchFTSIndexes = vi.fn(async () => undefined);
|
||||
vi.doMock('../../src/core/lbug/lbug-adapter.js', () => ({
|
||||
initLbug: vi.fn(async () => undefined),
|
||||
loadGraphToLbug: vi.fn(async () => undefined),
|
||||
getLbugStats: vi.fn(async () => ({})),
|
||||
executeQuery: vi.fn(async () => []),
|
||||
executeWithReusedStatement: vi.fn(async () => []),
|
||||
closeLbug: vi.fn(async () => undefined),
|
||||
loadCachedEmbeddings: vi.fn(async () => ({ embeddingNodeIds: new Set(), embeddings: [] })),
|
||||
deleteNodesForFile: vi.fn(async () => undefined),
|
||||
deleteAllCommunitiesAndProcesses: vi.fn(async () => undefined),
|
||||
queryImporters: vi.fn(async () => []),
|
||||
// Extension cannot load — the guard must fail BEFORE any index is touched.
|
||||
loadFTSExtension: vi.fn(async () => false),
|
||||
}));
|
||||
vi.doMock('../../src/core/search/fts-indexes.js', () => ({
|
||||
createSearchFTSIndexes,
|
||||
verifySearchFTSIndexes: vi.fn(async () => []),
|
||||
}));
|
||||
|
||||
const tmpRepo = await createTempDir('gitnexus-run-analyze-repair-fts-unavailable-');
|
||||
try {
|
||||
const { storagePath, lbugPath } = getStoragePaths(tmpRepo.dbPath);
|
||||
await fs.mkdir(storagePath, { recursive: true });
|
||||
await saveMeta(storagePath, {
|
||||
repoPath: tmpRepo.dbPath,
|
||||
lastCommit: '',
|
||||
indexedAt: new Date().toISOString(),
|
||||
stats: {},
|
||||
});
|
||||
await createPlaceholderGraphStore(lbugPath);
|
||||
|
||||
const { runFullAnalysis } = await import('../../src/core/run-analyze.js');
|
||||
|
||||
await expect(
|
||||
runFullAnalysis(tmpRepo.dbPath, { repairFts: true }, { onProgress: () => {} }),
|
||||
).rejects.toThrow(/FTS extension is unavailable[\s\S]*gitnexus doctor/i);
|
||||
// The guard fires before drop-then-create, so no index is dropped.
|
||||
expect(createSearchFTSIndexes).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
await tmpRepo.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
it('fails full analyze when FTS verification reports missing indexes after creation', async () => {
|
||||
vi.doMock('../../src/core/lbug/lbug-adapter.js', () => ({
|
||||
initLbug: vi.fn(async () => undefined),
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ export default defineConfig({
|
|||
'test/integration/local-backend-calltool.test.ts',
|
||||
'test/integration/search-core.test.ts',
|
||||
'test/integration/search-pool.test.ts',
|
||||
'test/integration/fts-description-search.test.ts',
|
||||
'test/integration/augmentation.test.ts',
|
||||
'test/integration/staleness-and-stability.test.ts',
|
||||
'test/integration/lbug-lock-retry.test.ts',
|
||||
|
|
@ -91,6 +92,7 @@ export default defineConfig({
|
|||
'test/integration/local-backend-calltool.test.ts',
|
||||
'test/integration/search-core.test.ts',
|
||||
'test/integration/search-pool.test.ts',
|
||||
'test/integration/fts-description-search.test.ts',
|
||||
'test/integration/augmentation.test.ts',
|
||||
'test/integration/staleness-and-stability.test.ts',
|
||||
'test/integration/lbug-lock-retry.test.ts',
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue