diff --git a/gitnexus/src/core/embeddings/embedding-pipeline.ts b/gitnexus/src/core/embeddings/embedding-pipeline.ts index c1732da87..302a97e64 100644 --- a/gitnexus/src/core/embeddings/embedding-pipeline.ts +++ b/gitnexus/src/core/embeddings/embedding-pipeline.ts @@ -26,7 +26,6 @@ import { type EmbeddableNode, type SemanticSearchResult, type ModelProgress, - type EmbeddingContext, EMBEDDABLE_LABELS, isShortLabel, LABEL_METHOD, @@ -80,7 +79,7 @@ const ensureVectorExtensionAvailable = async (): Promise => { * invalidate existing vectors, such as metadata/header shape changes, * structural container context changes, or preceding-context formatting rules. */ -export const EMBEDDING_TEXT_VERSION = 'v2'; +export const EMBEDDING_TEXT_VERSION = 'v4'; /** * Compute a stable content fingerprint for an embeddable node. @@ -255,6 +254,42 @@ export interface EmbeddingPipelineResult { semanticMode: 'vector-index' | 'exact-scan'; } +/** + * DELETE stale embedding rows for the given nodeIds so they can be re-inserted. + * + * Kuzu forbids SET on vector-indexed properties; DELETE-then-INSERT is the + * sanctioned pattern. A `"does not exist"` error means the rows are already gone + * (safe to proceed); any other error risks vector-index corruption, so it + * propagates and aborts the pipeline. + * + * Called per-batch (just before each batch's INSERT), not once up front — see + * the caller comment / KTD7: an up-front bulk delete of every stale row leaves + * the whole index deleted-not-reinserted if the re-embed is interrupted. Per-batch + * interleaving bounds that window to a single batch. + */ +const deleteStaleEmbeddingRows = async ( + executeWithReusedStatement: ( + cypher: string, + paramsList: Array>, + ) => Promise, + nodeIds: string[], +): Promise => { + if (nodeIds.length === 0) return; + try { + await executeWithReusedStatement( + `MATCH (e:${EMBEDDING_TABLE_NAME} {nodeId: $nodeId}) DELETE e`, + nodeIds.map((nodeId) => ({ nodeId })), + ); + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + if (!msg.includes('does not exist')) { + throw new Error( + `[embed] Failed to delete stale embedding rows — aborting to prevent vector-index corruption: ${msg}`, + ); + } + } +}; + /** * Run the embedding pipeline * @@ -263,11 +298,9 @@ export interface EmbeddingPipelineResult { * @param onProgress - Callback for progress updates * @param config - Optional configuration override * @param skipNodeIds - Optional set of node IDs that already have embeddings (incremental mode) - * @param context - Optional repo/server context for metadata enrichment * @param existingEmbeddings - Optional map of nodeId → contentHash for incremental mode. * Nodes whose hash matches are skipped; nodes with a changed hash are DELETE'd * and re-embedded; nodes not in the map are embedded fresh. - */ export const runEmbeddingPipeline = async ( executeQuery: (cypher: string) => Promise, @@ -278,7 +311,6 @@ export const runEmbeddingPipeline = async ( onProgress: EmbeddingProgressCallback, config: Partial = {}, skipNodeIds?: Set, - context?: EmbeddingContext, existingEmbeddings?: Map, ): Promise => { const finalConfig = resolveEmbeddingConfig(config); @@ -321,21 +353,16 @@ export const runEmbeddingPipeline = async ( // Phase 2: Query embeddable nodes let nodes = await queryEmbeddableNodes(executeQuery); - // Apply context metadata - if (context?.repoName) { - for (const node of nodes) { - node.repoName = context.repoName; - node.serverName = context.serverName; - } - } - // Incremental mode: compare content hashes, delete stale rows, skip fresh ones. // Computed hashes for stale nodes are cached so batchInsertEmbeddings can reuse them // (avoids double computation). const computedStaleHashes = new Map(); + // Stale rows are DELETE'd per-batch (just before each batch's INSERT) rather + // than all up front — see U6 / KTD7. `staleNodeIds` is consulted inside the + // batch loop; it stays empty in full (non-incremental) mode so no deletes fire. + const staleNodeIds = new Set(); if (existingEmbeddings && existingEmbeddings.size > 0) { const beforeCount = nodes.length; - const staleNodeIds: string[] = []; nodes = nodes.filter((n) => { const existingHash = existingEmbeddings.get(n.id); if (existingHash === undefined) { @@ -346,40 +373,16 @@ export const runEmbeddingPipeline = async ( if (currentHash !== existingHash) { // Content changed — cache hash for reuse during insert, mark for DELETE + re-embed computedStaleHashes.set(n.id, currentHash); - staleNodeIds.push(n.id); + staleNodeIds.add(n.id); return true; } // Hash matches — skip (fresh); no need to cache hash for skipped nodes return false; }); - // DELETE stale embedding rows so they can be re-inserted - // (Kuzu forbids SET on vector-indexed properties; DELETE-then-INSERT is the sanctioned pattern) - if (staleNodeIds.length > 0) { - if (isDev) { - logger.info(`🔄 Deleting ${staleNodeIds.length} stale embedding rows for re-embed`); - } - try { - await executeWithReusedStatement( - `MATCH (e:${EMBEDDING_TABLE_NAME} {nodeId: $nodeId}) DELETE e`, - staleNodeIds.map((nodeId) => ({ nodeId })), - ); - } catch (err) { - // "does not exist" = rows already gone — safe to proceed. - // All other errors risk vector-index corruption (Kuzu requires DELETE-before-INSERT - // for vector-indexed properties) — propagate so the pipeline aborts cleanly. - const msg = err instanceof Error ? err.message : String(err); - if (!msg.includes('does not exist')) { - throw new Error( - `[embed] Failed to delete stale embedding rows — aborting to prevent vector-index corruption: ${msg}`, - ); - } - } - } - if (isDev) { logger.info( - `📦 Incremental embeddings: ${beforeCount} total, ${existingEmbeddings.size} cached, ${staleNodeIds.length} stale, ${nodes.length} to embed`, + `📦 Incremental embeddings: ${beforeCount} total, ${existingEmbeddings.size} cached, ${staleNodeIds.size} stale, ${nodes.length} to embed`, ); } } @@ -504,6 +507,12 @@ export const runEmbeddingPipeline = async ( } } + // U6 / KTD7: delete this batch's stale rows immediately before its inserts, + // so an interrupted re-embed loses at most one batch (not the whole index). + // Preserves Kuzu's required DELETE-before-INSERT for vector-indexed rows. + const batchStaleIds = batch.filter((n) => staleNodeIds.has(n.id)).map((n) => n.id); + await deleteStaleEmbeddingRows(executeWithReusedStatement, batchStaleIds); + // Embed chunk texts in sub-batches to control memory const EMBED_SUB_BATCH = finalConfig.subBatchSize; for (let si = 0; si < allTexts.length; si += EMBED_SUB_BATCH) { diff --git a/gitnexus/src/core/embeddings/text-generator.ts b/gitnexus/src/core/embeddings/text-generator.ts index 74e90e9ce..b9be36b6c 100644 --- a/gitnexus/src/core/embeddings/text-generator.ts +++ b/gitnexus/src/core/embeddings/text-generator.ts @@ -1,7 +1,7 @@ /** * Text Generator Module * - * Generates enriched embedding text from code nodes with metadata. + * Generates compact, description-forward embedding text from code nodes. * Supports chunkable labels (Function/Method with AST chunking), * Class-specific structural text, and short-node direct embed. * @@ -58,33 +58,51 @@ const cleanContent = (content: string): string => { }; /** - * Build metadata header for a node + * Compact location signal for the embedding header: the last 1-2 path segments + * (immediate parent dir + basename), never the full deep path. + * + * #2333 / PR #2334 tri-review: U1 dropped the location entirely, which regressed + * path/service-qualified semantic search (e.g. `billing/handler` vs + * `identity/handler` in a monorepo) — and FTS indexes only name/content/description, + * never `filePath`, so there is no keyword backfill. The bounded form restores the + * discriminating tokens (service dir + filename-concept) at a fraction of the + * dilution the full path caused. */ -const buildMetadataHeader = (node: EmbeddableNode, config: Partial): string => { +const boundedLocation = (filePath: string): string => { + const segments = filePath.replace(/\\/g, '/').split('/').filter(Boolean); + return segments.slice(-2).join('/'); +}; + +/** + * Build a compact, description-forward header for embedding text. + * + * Issue #2333 (sub-issue of #2326), Option A: lead the embedding text with the + * symbol name + doc-comment description and drop the low-signal metadata lines + * (`Repo`/`Server`/`Export` and the verbose full `Path`). For short doc comments + * those lines used to be ~25-30% of the embedding text, diluting the description's + * semantic weight in the vector and weakening description-shaped search — worst + * for CJK, where a complete concept is often 4-20 characters. + * + * A *bounded* location signal (last 1-2 path segments) is kept after the + * description — see `boundedLocation` for why the full path drop was reversed. + * + * Full metadata is unaffected: it lives on the graph node properties, which is + * what display/context tools read. Only the embedding text changes here. + * + * Option B (reorder only, keep metadata) was rejected — mean-pooled embeddings + * weight by token proportion, not position, so reordering alone barely moves the + * signal. Option C (a separate description-only embedding + hybrid merge) is + * deferred to follow-up; build it only if Option A proves insufficient against + * real measurement. Any change to this template MUST bump EMBEDDING_TEXT_VERSION. + */ +const buildEmbeddingHeader = (node: EmbeddableNode, config: Partial): string => { const parts: string[] = []; // Label + name parts.push(`${node.label}: ${node.name}`); - // Repo name - if (node.repoName) { - parts.push(`Repo: ${node.repoName}`); - } - - // Server name (optional) - if (node.serverName) { - parts.push(`Server: ${node.serverName}`); - } - - // Full file path - parts.push(`Path: ${node.filePath}`); - - // Export status - if (node.isExported !== undefined) { - parts.push(`Export: ${node.isExported}`); - } - - // Description (truncated) + // Description hoisted above everything else so its semantic signal dominates + // the embedding vector and is never the part lost to token-limit truncation. if (node.description) { const maxLen = config.maxDescriptionLength ?? DEFAULT_EMBEDDING_CONFIG.maxDescriptionLength; const truncated = truncateDescription(node.description, maxLen); @@ -93,6 +111,16 @@ const buildMetadataHeader = (node: EmbeddableNode, config: Partial, prevTail?: string, ): string => { - const header = buildMetadataHeader(node, config); + const header = buildEmbeddingHeader(node, config); const parts = [header]; if (prevTail) { parts.push(`[preceding context]: ...${cleanContent(prevTail)}`); @@ -128,7 +156,7 @@ const generateStructuralTypeText = ( chunkIndex?: number, prevTail?: string, ): string => { - const header = buildMetadataHeader(node, config); + const header = buildEmbeddingHeader(node, config); const parts: string[] = [header]; const isFirstChunk = chunkIndex === undefined || chunkIndex === 0; const cleanedContent = cleanContent(node.content); @@ -253,7 +281,7 @@ export const generateEmbeddingText = ( prevTail?: string, ): string => { if (isShortLabel(node.label)) { - const header = buildMetadataHeader(node, config); + const header = buildEmbeddingHeader(node, config); const cleaned = cleanContent(node.content); return `${header}\n\n${cleaned}`; } diff --git a/gitnexus/src/core/embeddings/types.ts b/gitnexus/src/core/embeddings/types.ts index 309a3683e..71cad5d65 100644 --- a/gitnexus/src/core/embeddings/types.ts +++ b/gitnexus/src/core/embeddings/types.ts @@ -289,14 +289,6 @@ export interface CachedEmbedding { contentHash?: string; } -/** - * Context info for embedding pipeline (repo/server metadata enrichment) - */ -export interface EmbeddingContext { - repoName?: string; - serverName?: string; -} - /** * Model download progress from transformers.js */ diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 8ba5673b3..06438ba4f 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -1369,21 +1369,6 @@ export async function runFullAnalysis( } } - const { readServerMapping } = await import('./embeddings/server-mapping.js'); - // Mirror the registry's name-resolution chain so the server-mapping - // lookup key stays aligned with the final registry name (#1259): - // --name → remote-derived → canonical-root basename - // (preserved-alias is intentionally NOT consulted here — server - // mappings are addressed by the operationally-meaningful name the - // user configures, not by a sticky registry-only alias they may not - // know about. The previous canonical-only logic ignored both --name - // and remote-derived names, silently breaking server-mapping for - // anyone with a `--name` alias or remote-named repo.) - const projectName = - options.registryName ?? - getInferredRepoName(repoPath) ?? - path.basename(resolveRepoIdentityRoot(repoPath)); - const serverName = await readServerMapping(projectName); const embeddingResult = await runEmbeddingPipeline( executeQuery, executeWithReusedStatement, @@ -1399,7 +1384,6 @@ export async function runFullAnalysis( }, {}, cachedEmbeddingNodeIds.size > 0 ? cachedEmbeddingNodeIds : undefined, - { repoName: projectName, serverName }, existingEmbeddings, ); if (embeddingResult.semanticMode === 'exact-scan') { diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index bb4cfcef4..383bed5ad 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -1767,7 +1767,6 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => }, {}, // config: use defaults undefined, // skipNodeIds - undefined, // context existingEmbeddings, ); diff --git a/gitnexus/test/unit/embedding-chunking.test.ts b/gitnexus/test/unit/embedding-chunking.test.ts index 244efe63d..cd7103319 100644 --- a/gitnexus/test/unit/embedding-chunking.test.ts +++ b/gitnexus/test/unit/embedding-chunking.test.ts @@ -105,10 +105,11 @@ describe('embedding-chunking integration', () => { const text = generateEmbeddingText(node, chunks[0].text); expect(text).toContain('Function: test'); - expect(text).toContain('Repo: my-project'); - expect(text).toContain('Server: my-service'); - expect(text).toContain('Export: true'); expect(text).toContain('function hello()'); + // #2333: verbose metadata is no longer part of embedding text. + expect(text).not.toContain('Repo: my-project'); + expect(text).not.toContain('Server: my-service'); + expect(text).not.toContain('Export: true'); }); it('long function produces multiple chunks', () => { @@ -282,7 +283,7 @@ describe('embedding-chunking integration', () => { expect(secondText).toContain('age: u32,'); }); - it('metadata is present in every chunk', () => { + it('header is present in every chunk', () => { const longContent = 'x'.repeat(3000); const node = makeNode({ content: longContent, @@ -294,9 +295,11 @@ describe('embedding-chunking integration', () => { for (const chunk of chunks) { const text = generateEmbeddingText(node, chunk.text); + // The compact header (name, + description when present) repeats on every + // chunk so each chunk keeps its identity; #2333 dropped the metadata lines. expect(text).toContain('Function: test'); - expect(text).toContain('Repo: test-repo'); - expect(text).toContain('Path: src/test.ts'); + expect(text).not.toContain('Repo: test-repo'); + expect(text).not.toContain('Path: src/test.ts'); } }); }); diff --git a/gitnexus/test/unit/embedding-pipeline.test.ts b/gitnexus/test/unit/embedding-pipeline.test.ts index 91f182db2..570593a2d 100644 --- a/gitnexus/test/unit/embedding-pipeline.test.ts +++ b/gitnexus/test/unit/embedding-pipeline.test.ts @@ -101,11 +101,29 @@ describe('contentHashForNode', () => { expect(contentHashForNode(original)).not.toBe(contentHashForNode(edited)); }); - it('changes when filePath differs', () => { - const a = makeNode({ filePath: 'src/a.ts' }); - const b = makeNode({ filePath: 'src/b.ts' }); - // Different filePaths lead to different embedding text ⇒ different hashes - expect(contentHashForNode(a)).not.toBe(contentHashForNode(b)); + it('depends on the bounded location (last 1-2 segments) but not the deep path prefix (#2333 U3)', () => { + // U3 reinstated a BOUNDED location signal (last 1-2 path segments) in the + // embedding header, so the hash now tracks that signal — but only it, not the + // full deep prefix. Same last-2-segments ⇒ identical embedding text ⇒ identical + // hash, even with a totally different prefix. + const samePrefixA = makeNode({ filePath: 'src/very/deep/nested/svc/Impl.ts' }); + const samePrefixB = makeNode({ filePath: 'other/svc/Impl.ts' }); + expect(contentHashForNode(samePrefixA)).toBe(contentHashForNode(samePrefixB)); + + // Different last segments (e.g. a real service-folder move) ⇒ different bounded + // location ⇒ different hash, so the re-embed correctly picks up the new location. + const billing = makeNode({ filePath: 'billing/handler.ts' }); + const identity = makeNode({ filePath: 'identity/handler.ts' }); + expect(contentHashForNode(billing)).not.toBe(contentHashForNode(identity)); + }); + + it('is independent of repoName/serverName/isExported (#2333 — dropped from header)', () => { + // #2333 dropped these three (alongside filePath) from the embedding header. + // The hash must not depend on them; if any were re-added to the header, this + // assertion flips and flags the silent re-coupling before it ships. + const a = makeNode({ repoName: 'repo-a', serverName: 'svc-a', isExported: true }); + const b = makeNode({ repoName: 'repo-b', serverName: 'svc-b', isExported: false }); + expect(contentHashForNode(a)).toBe(contentHashForNode(b)); }); it('produces identical hash regardless of config vs finalConfig when config is empty', () => { @@ -116,7 +134,7 @@ describe('contentHashForNode', () => { }); it('exports a text template version marker', () => { - expect(EMBEDDING_TEXT_VERSION).toBe('v2'); + expect(EMBEDDING_TEXT_VERSION).toBe('v4'); }); }); @@ -290,7 +308,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, // skipNodeIds - undefined, // context existingEmbeddings, ); @@ -326,7 +343,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, // skipNodeIds - undefined, // context existingEmbeddings, ); @@ -402,7 +418,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, - undefined, new Map(), ); @@ -410,9 +425,19 @@ describe('runEmbeddingPipeline incremental filter', () => { const classText = embeddedTexts.find((text) => text.includes('Class: Parser')); const enumText = embeddedTexts.find((text) => text.includes('Enum: Status')); - expect(classText).toContain('Export: true'); + // #2333 dropped Export/metadata from embedding text, but the description + // assertions still prove the positional column mapping is correct. The Class + // row carries isExported at index 7 and description at index 8; the Enum row + // has no isExported column (description at index 7), exercising the other + // mapping branch. The toContain checks below are the primary guard: an + // off-by-one would put the boolean from index 7 into description, so the real + // text would be absent, failing here. expect(classText).toContain('Parses typed payloads.'); - expect(enumText).not.toContain('Export:'); + // Header-integrity guard (#2333 U5): the embedding text must start with the + // `Label: name` header. A positional mis-map that corrupted the header line + // (e.g. the name column shifting) is caught here directly, instead of via the + // old narrow `not.toContain('\ntrue')` coincidence. + expect(classText).toMatch(/^Class: Parser\n/); expect(enumText).toContain('Represents user status.'); }); @@ -435,7 +460,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, // skipNodeIds - undefined, // context existingEmbeddings, ); @@ -468,7 +492,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, // skipNodeIds - undefined, // context existingEmbeddings, ); @@ -481,6 +504,87 @@ describe('runEmbeddingPipeline incremental filter', () => { expect(createCalls.length).toBeGreaterThanOrEqual(1); }); + it('deletes each batch stale rows interleaved with its insert, not all up front (#2333 U6)', async () => { + mockEmbedderSetup(); + + const n1 = makeNode({ id: 'Function:a:src/a.ts', name: 'a', filePath: 'src/a.ts' }); + const n2 = makeNode({ id: 'Function:b:src/b.ts', name: 'b', filePath: 'src/b.ts' }); + // Both stale (hash mismatch) → both re-embed. + const existingEmbeddings = new Map([ + [n1.id, 'wronghash1'], + [n2.id, 'wronghash2'], + ]); + + const executeQuery = mockExecuteQuery([n1, n2]); + const executeWithReusedStatement = mockExecuteWithReusedStatement(); + + const { runEmbeddingPipeline } = + await import('../../src/core/embeddings/embedding-pipeline.js'); + + await runEmbeddingPipeline( + executeQuery, + executeWithReusedStatement, + onProgress, + { batchSize: 1 }, // one node per batch → two batches + undefined, // skipNodeIds + existingEmbeddings, + ); + + // U6 / KTD7: per-batch interleaving means TWO separate DELETE calls (one per + // batch), not one up-front bulk delete of both stale rows. + const deleteCalls = stmtCalls.filter((c) => c.cypher.includes('DELETE')); + expect(deleteCalls.length).toBe(2); + + // Ordering proof: batch 1's INSERT lands BEFORE batch 2's DELETE. An up-front + // bulk delete would put both DELETEs before any INSERT, failing this — so an + // interrupted re-embed can lose at most one batch, never the whole index. + const insertN1 = stmtCalls.findIndex( + (c) => c.cypher.includes('CREATE') && c.params.some((p) => p.nodeId === n1.id), + ); + const deleteN2 = stmtCalls.findIndex( + (c) => c.cypher.includes('DELETE') && c.params.some((p) => p.nodeId === n2.id), + ); + expect(insertN1).toBeGreaterThanOrEqual(0); + expect(deleteN2).toBeGreaterThanOrEqual(0); + expect(insertN1).toBeLessThan(deleteN2); + }); + + it('deletes only stale nodes — new and unchanged nodes are never deleted (#2333 U6)', async () => { + mockEmbedderSetup(); + + const unchanged = makeNode({ id: 'Function:u:src/u.ts', name: 'u', filePath: 'src/u.ts' }); + const stale = makeNode({ id: 'Function:s:src/s.ts', name: 's', filePath: 'src/s.ts' }); + const brandNew = makeNode({ id: 'Function:n:src/n.ts', name: 'n', filePath: 'src/n.ts' }); + const unchangedHash = contentHashForNode(unchanged, DEFAULT_EMBEDDING_CONFIG); + const existingEmbeddings = new Map([ + [unchanged.id, unchangedHash], // hash matches → skipped, no delete + [stale.id, 'wronghash'], // hash mismatch → deleted + re-embed + // brandNew absent from the map → new → embedded, no delete + ]); + + const executeQuery = mockExecuteQuery([unchanged, stale, brandNew]); + const executeWithReusedStatement = mockExecuteWithReusedStatement(); + + const { runEmbeddingPipeline } = + await import('../../src/core/embeddings/embedding-pipeline.js'); + + await runEmbeddingPipeline( + executeQuery, + executeWithReusedStatement, + onProgress, + { batchSize: 1 }, + undefined, // skipNodeIds + existingEmbeddings, + ); + + const deletedIds = stmtCalls + .filter((c) => c.cypher.includes('DELETE')) + .flatMap((c) => c.params.map((p) => p.nodeId)); + expect(deletedIds).toContain(stale.id); + expect(deletedIds).not.toContain(brandNew.id); + expect(deletedIds).not.toContain(unchanged.id); + }); + it('calls createVectorIndex even when zero nodes need embedding after filter', async () => { mockEmbedderSetup(); @@ -501,7 +605,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, // skipNodeIds - undefined, // context existingEmbeddings, ); @@ -623,7 +726,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, { chunkSize: 90, overlap: 0 }, undefined, - undefined, new Map(), ); @@ -678,7 +780,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, { chunkSize: CLASS_CHUNK_SIZE, overlap: CLASS_OVERLAP }, undefined, - undefined, new Map(), ); @@ -714,7 +815,6 @@ describe('runEmbeddingPipeline incremental filter', () => { onProgress, {}, undefined, // skipNodeIds - undefined, // context existingEmbeddings, ), ).rejects.toThrow('vector-index corruption'); diff --git a/gitnexus/test/unit/text-generator.test.ts b/gitnexus/test/unit/text-generator.test.ts index e411428df..a573f9b65 100644 --- a/gitnexus/test/unit/text-generator.test.ts +++ b/gitnexus/test/unit/text-generator.test.ts @@ -19,32 +19,30 @@ const baseNode: EmbeddableNode = { describe('text-generator', () => { describe('generateEmbeddingText', () => { - it('includes metadata header for Function', () => { + it('leads with name and code, dropping verbose metadata lines (#2333)', () => { const node: EmbeddableNode = { ...baseNode, isExported: true, repoName: 'backend-user-ms', }; const text = generateEmbeddingText(node, node.content); + // Compact embedding header: name + code remain. expect(text).toContain('Function: parseJSON'); - expect(text).toContain('Repo: backend-user-ms'); - expect(text).toContain('Path: src/utils/parser.ts'); - expect(text).toContain('Export: true'); expect(text).toContain('function parseJSON'); + // Low-signal metadata lines are intentionally excluded from embedding text. + expect(text).not.toContain('Repo: backend-user-ms'); + expect(text).not.toContain('Path: src/utils/parser.ts'); + expect(text).not.toContain('Export: true'); }); - it('includes Server line when serverName is set', () => { + it('excludes the Server line from embedding text even when serverName is set (#2333)', () => { const node: EmbeddableNode = { ...baseNode, repoName: 'backend-user-ms', serverName: 'user-service', }; const text = generateEmbeddingText(node, node.content); - expect(text).toContain('Server: user-service'); - }); - - it('omits Server line when serverName is undefined', () => { - const text = generateEmbeddingText(baseNode, baseNode.content); + expect(text).not.toContain('Server: user-service'); expect(text).not.toContain('Server:'); }); @@ -57,6 +55,179 @@ describe('text-generator', () => { expect(text).toContain('This function parses JSON text'); }); + // #2333: short doc comments must not be diluted by metadata. The description + // is hoisted directly under the name, ahead of the code body, and the + // low-signal metadata lines are dropped from embedding text entirely. + it('hoists a short English description above the code body (#2333)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Method', + name: 'updateMaterialExpiryDate', + description: 'validate user', + isExported: false, + repoName: 'my-project', + content: + 'function updateMaterialExpiryDate(paramMap) {\n // ... a long method body ...\n return doWork(paramMap);\n}', + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('validate user'); + // Description appears before the code body. + expect(text.indexOf('validate user')).toBeLessThan(text.indexOf('return doWork')); + // Metadata noise removed. + expect(text).not.toContain('Repo: my-project'); + expect(text).not.toContain('Path:'); + expect(text).not.toContain('Export:'); + }); + + it('hoists a short CJK description above the code body (#2333)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Method', + name: 'updateMaterialExpiryDate', + description: '更新物料有效期', + isExported: false, + repoName: 'my-project', + content: + 'function updateMaterialExpiryDate(paramMap) {\n // ... a long method body ...\n return doWork(paramMap);\n}', + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('更新物料有效期'); + expect(text.indexOf('更新物料有效期')).toBeLessThan(text.indexOf('return doWork')); + expect(text).not.toContain('Repo: my-project'); + expect(text).not.toContain('Path:'); + expect(text).not.toContain('Export:'); + }); + + it('hoists description in short-label nodes too (#2333)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Const', + name: 'MAX_RETRIES', + description: 'retry ceiling', + content: 'const MAX_RETRIES = 5;', + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Const: MAX_RETRIES'); + expect(text).toContain('retry ceiling'); + expect(text.indexOf('retry ceiling')).toBeLessThan(text.indexOf('const MAX_RETRIES = 5;')); + expect(text).not.toContain('Path:'); + }); + + it('keeps structural Methods/Properties lines under the compact header (#2333)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Class', + name: 'Parser', + description: 'JSON parser', + repoName: 'my-project', + methodNames: ['parseJSON', 'validate'], + fieldNames: ['options', 'cache'], + content: `class Parser { + options: ParserOptions; + private cache: Map; + parseJSON(text: string) { return JSON.parse(text); } + validate() { return true; } +}`, + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Class: Parser'); + expect(text).toContain('JSON parser'); + // Structural signal must survive the compact-header change. + expect(text).toContain('Methods: parseJSON, validate'); + expect(text).toContain('Properties: options, cache'); + // Description is hoisted ahead of the structural lines (ordering guard for + // the structural path, mirroring the function/method ordering checks). + expect(text.indexOf('JSON parser')).toBeLessThan(text.indexOf('Container:')); + expect(text.indexOf('JSON parser')).toBeLessThan(text.indexOf('Methods:')); + // Metadata noise still dropped. + expect(text).not.toContain('Repo: my-project'); + }); + + it('emits no description line and no metadata when description is absent (#2333)', () => { + const node: EmbeddableNode = { + ...baseNode, + isExported: true, + repoName: 'my-project', + description: undefined, + }; + const text = generateEmbeddingText(node, node.content); + // Header is the name line, then the bounded location line, then a blank + // line, then the code body — no stray empty description line, no verbose + // metadata. + expect( + text.startsWith('Function: parseJSON\nLoc: utils/parser.ts\n\nfunction parseJSON'), + ).toBe(true); + expect(text).not.toContain('Repo:'); + expect(text).not.toContain('Export:'); + // Only the bounded last-1-2 segments — never the verbose deep path. + expect(text).not.toContain('Path:'); + expect(text).not.toContain('src/utils/parser.ts'); + }); + + // U3 (#2333 PR #2334 tri-review): a BOUNDED location signal (last 1-2 path + // segments) is reinstated so path/service-qualified semantic search keeps a + // discriminator, since FTS does not index filePath. + it('emits a bounded location (last 2 segments), not the full deep path (#2333 U3)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Method', + name: 'updateMaterialExpiryDate', + filePath: 'src/main/java/com/example/service/MaterialServiceImpl.java', + content: 'function updateMaterialExpiryDate() { return doWork(); }', + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Loc: service/MaterialServiceImpl.java'); + // The deep prefix is dropped entirely. + expect(text).not.toContain('src/main/java/com/example'); + }); + + it('emits just the basename for a root-level file (#2333 U3)', () => { + const node: EmbeddableNode = { ...baseNode, filePath: 'index.ts' }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Loc: index.ts'); + // No leading slash and no stray "undefined/" prefix from slicing one segment. + expect(text).not.toContain('Loc: /index.ts'); + expect(text).not.toContain('undefined'); + }); + + it('disambiguates same-named symbols in different service folders (#2333 U3)', () => { + const billing = generateEmbeddingText( + { ...baseNode, name: 'handler', filePath: 'billing/handler.ts' }, + 'function handler() {}', + ); + const identity = generateEmbeddingText( + { ...baseNode, name: 'handler', filePath: 'identity/handler.ts' }, + 'function handler() {}', + ); + expect(billing).toContain('Loc: billing/handler.ts'); + expect(identity).toContain('Loc: identity/handler.ts'); + // The two embedding texts differ — the regression the tri-review flagged + // (both collapsing to identical vectors) is fixed. + expect(billing).not.toBe(identity); + }); + + it('keeps the description ahead of the location signal (#2333 U3)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Method', + name: 'doThing', + description: 'batch import rows', + filePath: 'svc/importer.ts', + content: 'function doThing() { return run(); }', + }; + const text = generateEmbeddingText(node, node.content); + // description leads, then the location line, then the code body. + expect(text.indexOf('batch import rows')).toBeLessThan(text.indexOf('Loc: svc/importer.ts')); + expect(text.indexOf('Loc: svc/importer.ts')).toBeLessThan(text.indexOf('return run()')); + }); + + it('normalizes Windows path separators in the location signal (#2333 U3)', () => { + const node: EmbeddableNode = { ...baseNode, filePath: 'src\\svc\\Foo.ts' }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Loc: svc/Foo.ts'); + expect(text).not.toContain('\\'); + }); + it('generates short node text for TypeAlias without chunking', () => { const node: EmbeddableNode = { ...baseNode, @@ -167,6 +338,56 @@ describe('text-generator', () => { expect(text).toContain('struct User {'); }); + // U5 (#2333 PR #2334): Interface and Struct route through the same + // generateStructuralTypeText path as Class, so the description-forward + // ordering must hold for them too — guards against a future per-label + // specialization silently reordering the header. + it('keeps an Interface description ahead of its structural lines (#2333 U5)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Interface', + name: 'Handler', + description: 'event handler contract', + methodNames: ['handle', 'validate'], + fieldNames: ['name'], + content: `interface Handler { + handle(event: Event): void; + readonly name: string; +}`, + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Interface: Handler'); + expect(text).toContain('event handler contract'); + expect(text).toContain('Methods: handle, validate'); + expect(text).toContain('Properties: name'); + expect(text.indexOf('event handler contract')).toBeLessThan(text.indexOf('Container:')); + expect(text.indexOf('event handler contract')).toBeLessThan(text.indexOf('Methods:')); + expect(text).toContain('Loc: utils/parser.ts'); + expect(text).not.toContain('Repo:'); + }); + + it('keeps a Struct description ahead of its structural lines (#2333 U5)', () => { + const node: EmbeddableNode = { + ...baseNode, + label: 'Struct', + name: 'User', + description: 'user record', + fieldNames: ['name', 'age'], + content: `struct User { + name: String, + age: u32, +}`, + }; + const text = generateEmbeddingText(node, node.content); + expect(text).toContain('Struct: User'); + expect(text).toContain('user record'); + expect(text).toContain('Properties: name, age'); + expect(text).toContain('Container: struct User {'); + expect(text.indexOf('user record')).toBeLessThan(text.indexOf('Container:')); + expect(text.indexOf('user record')).toBeLessThan(text.indexOf('Properties:')); + expect(text).toContain('Loc: utils/parser.ts'); + }); + it('keeps compact container context on later structural chunks', () => { const node: EmbeddableNode = { ...baseNode,