diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml index ebfcf5f62a..6009e29c10 100644 --- a/.github/workflows/ci-tests.yml +++ b/.github/workflows/ci-tests.yml @@ -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). diff --git a/gitnexus/scripts/cross-platform-tests.ts b/gitnexus/scripts/cross-platform-tests.ts index 129a788a31..f7733ef19c 100644 --- a/gitnexus/scripts/cross-platform-tests.ts +++ b/gitnexus/scripts/cross-platform-tests.ts @@ -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', ]; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 0b5d9c9bab..7a169352d6 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.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 diff --git a/gitnexus/src/core/search/fts-indexes.ts b/gitnexus/src/core/search/fts-indexes.ts index 01147beed7..cc3d7a638c 100644 --- a/gitnexus/src/core/search/fts-indexes.ts +++ b/gitnexus/src/core/search/fts-indexes.ts @@ -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 { 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, ): Promise { - 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(); + for (const row of rows) { + if (typeof row !== 'object' || row === null) continue; + const record = row as Record; + 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}`); } } diff --git a/gitnexus/src/core/search/fts-schema.ts b/gitnexus/src/core/search/fts-schema.ts index 404a320ede..eca162448e 100644 --- a/gitnexus/src/core/search/fts-schema.ts +++ b/gitnexus/src/core/search/fts-schema.ts @@ -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 }, ]; diff --git a/gitnexus/test/helpers/test-indexed-db.ts b/gitnexus/test/helpers/test-indexed-db.ts index 811c58dbac..c6cca74625 100644 --- a/gitnexus/test/helpers/test-indexed-db.ts +++ b/gitnexus/test/helpers/test-indexed-db.ts @@ -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 diff --git a/gitnexus/test/integration/fts-description-search.test.ts b/gitnexus/test/integration/fts-description-search.test.ts new file mode 100644 index 0000000000..05d162f76a --- /dev/null +++ b/gitnexus/test/integration/fts-description-search.test.ts @@ -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(); + 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; + }, + }, +); diff --git a/gitnexus/test/integration/lbug-core-adapter.test.ts b/gitnexus/test/integration/lbug-core-adapter.test.ts index 6cc245410e..b7af2ac0e9 100644 --- a/gitnexus/test/integration/lbug-core-adapter.test.ts +++ b/gitnexus/test/integration/lbug-core-adapter.test.ts @@ -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 => { 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 ───────────────────────────────────────────── diff --git a/gitnexus/test/unit/bm25-search.test.ts b/gitnexus/test/unit/bm25-search.test.ts index 984e6364aa..a9a65725c7 100644 --- a/gitnexus/test/unit/bm25-search.test.ts +++ b/gitnexus/test/unit/bm25-search.test.ts @@ -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(); @@ -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), + ); }); }); }); diff --git a/gitnexus/test/unit/fts-indexes.test.ts b/gitnexus/test/unit/fts-indexes.test.ts new file mode 100644 index 0000000000..f94819f147 --- /dev/null +++ b/gitnexus/test/unit/fts-indexes.test.ts @@ -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); + }); +}); diff --git a/gitnexus/test/unit/fts-schema.test.ts b/gitnexus/test/unit/fts-schema.test.ts new file mode 100644 index 0000000000..37abcba709 --- /dev/null +++ b/gitnexus/test/unit/fts-schema.test.ts @@ -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>( + 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(); + 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([]); + }); +}); diff --git a/gitnexus/test/unit/run-analyze-fts-repair.test.ts b/gitnexus/test/unit/run-analyze-fts-repair.test.ts index 21e737659f..4d8a0c15c5 100644 --- a/gitnexus/test/unit/run-analyze-fts-repair.test.ts +++ b/gitnexus/test/unit/run-analyze-fts-repair.test.ts @@ -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), diff --git a/gitnexus/vitest.config.ts b/gitnexus/vitest.config.ts index 89719cd80f..741ecc6a5b 100644 --- a/gitnexus/vitest.config.ts +++ b/gitnexus/vitest.config.ts @@ -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',