diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index dac37b6ef..dacd0a4cd 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -4361,44 +4361,28 @@ export class LocalBackend { return { error: 'New name is the same as the current name.' }; } - // Step 2: Collect edits from graph (high confidence) - const changes = new Map(); - - const addEdit = ( - filePath: string, - line: number, - oldText: string, - newText: string, - confidence: string, - ) => { - if (!changes.has(filePath)) { - changes.set(filePath, { file_path: filePath, edits: [] }); - } - changes.get(filePath)!.edits.push({ line, old_text: oldText, new_text: newText, confidence }); + // Steps 2+3: Determine the set of files the apply step will rewrite, then + // enumerate every occurrence in each. The apply step (Step 4) does a + // whole-file `\boldName\b` global replace on every file in `changes`, so the + // reported edit list MUST enumerate every matching line in every such file — + // otherwise the preview under-reports what lands, and the same partial list + // comes back after apply (#2605). Building `changes` from one file set is the + // single source of truth that keeps the report and the apply provably in sync. + type RenameEdit = { + line: number; + old_text: string; + new_text: string; + confidence: 'graph' | 'text_search'; }; + const escapedOldName = oldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - // The definition itself - if (sym.filePath && sym.startLine) { - try { - const content = await fs.readFile(assertSafePath(sym.filePath), 'utf-8'); - const lines = content.split('\n'); - const lineIdx = sym.startLine - 1; - if (lineIdx >= 0 && lineIdx < lines.length && lines[lineIdx].includes(oldName)) { - const defRegex = new RegExp( - `\\b${oldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`, - 'g', - ); - addEdit( - sym.filePath, - sym.startLine, - lines[lineIdx].trim(), - lines[lineIdx].replace(defRegex, new_name).trim(), - 'graph', - ); - } - } catch (e) { - logQueryError('rename:read-definition', e); - } + // Classify each file to rewrite by how it was discovered. Definition and + // graph-ref files carry graph confidence; files found only by text search + // carry text_search confidence. A graph-classified file is never downgraded. + const fileConfidence = new Map(); + + if (sym.filePath) { + fileConfidence.set(sym.filePath, 'graph'); } // All incoming refs from graph (callers, importers, etc.) @@ -4408,44 +4392,13 @@ export class LocalBackend { ...(lookupResult.incoming.extends || []), ...(lookupResult.incoming.implements || []), ]; - - let graphEdits = changes.size > 0 ? 1 : 0; // count definition edit - for (const ref of allIncoming) { - if (!ref.filePath) continue; - try { - const content = await fs.readFile(assertSafePath(ref.filePath), 'utf-8'); - const lines = content.split('\n'); - for (let i = 0; i < lines.length; i++) { - if (lines[i].includes(oldName)) { - addEdit( - ref.filePath, - i + 1, - lines[i].trim(), - lines[i] - .replace( - new RegExp(`\\b${oldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`, 'g'), - new_name, - ) - .trim(), - 'graph', - ); - graphEdits++; - break; // one edit per file from graph refs - } - } - } catch (e) { - logQueryError('rename:read-ref', e); + if (ref.filePath) { + fileConfidence.set(ref.filePath, 'graph'); } } - // Step 3: Text search for refs the graph might have missed - let astSearchEdits = 0; - const graphFiles = new Set( - [sym.filePath, ...allIncoming.map((r) => r.filePath)].filter(Boolean), - ); - - // Simple text search across the repo for the old name (in files not already covered by graph) + // Text search for files the graph might have missed entirely. try { const { execFileSync } = await import('child_process'); const rgArgs = [ @@ -4472,34 +4425,52 @@ export class LocalBackend { for (const file of files) { const normalizedFile = file.replace(/\\/g, '/').replace(/^\.\//, ''); - if (graphFiles.has(normalizedFile)) continue; // already covered by graph - - try { - const content = await fs.readFile(assertSafePath(normalizedFile), 'utf-8'); - const lines = content.split('\n'); - const regex = new RegExp(`\\b${oldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`, 'g'); - for (let i = 0; i < lines.length; i++) { - regex.lastIndex = 0; - if (regex.test(lines[i])) { - regex.lastIndex = 0; - addEdit( - normalizedFile, - i + 1, - lines[i].trim(), - lines[i].replace(regex, new_name).trim(), - 'text_search', - ); - astSearchEdits++; - } - } - } catch (e) { - logQueryError('rename:text-search-read', e); + // Never downgrade a graph-classified file to text_search. + if (!fileConfidence.has(normalizedFile)) { + fileConfidence.set(normalizedFile, 'text_search'); } } } catch (e) { logQueryError('rename:ripgrep', e); } + // Enumerate every `\boldName\b` line in every file to rewrite, using the same + // escaped global regex the apply step uses. A file with no matching line is + // dropped (apply would write nothing to it). Because apply's per-file global + // replace rewrites exactly these lines, the reported list equals what lands. + const changes = new Map(); + let graphEdits = 0; + let astSearchEdits = 0; + + for (const [filePath, confidence] of fileConfidence) { + try { + const content = await fs.readFile(assertSafePath(filePath), 'utf-8'); + const lines = content.split('\n'); + const edits: RenameEdit[] = []; + for (let i = 0; i < lines.length; i++) { + if (!new RegExp(`\\b${escapedOldName}\\b`).test(lines[i])) { + continue; + } + edits.push({ + line: i + 1, + old_text: lines[i].trim(), + new_text: lines[i].replace(new RegExp(`\\b${escapedOldName}\\b`, 'g'), new_name).trim(), + confidence, + }); + if (confidence === 'graph') { + graphEdits++; + } else { + astSearchEdits++; + } + } + if (edits.length > 0) { + changes.set(filePath, { file_path: filePath, edits }); + } + } catch (e) { + logQueryError('rename:enumerate', e); + } + } + // Step 4: Apply or preview const allChanges = Array.from(changes.values()); const totalEdits = allChanges.reduce((sum, c) => sum + c.edits.length, 0); diff --git a/gitnexus/test/unit/rename-edit-report.test.ts b/gitnexus/test/unit/rename-edit-report.test.ts new file mode 100644 index 000000000..ba651abf4 --- /dev/null +++ b/gitnexus/test/unit/rename-edit-report.test.ts @@ -0,0 +1,137 @@ +/** + * Regression test for issue #2605: `rename` must report every edit it applies. + * + * The apply step does a whole-file `\boldName\b` global replace on each touched + * file, but the reported `changes`/`total_edits` were built from a partial + * enumeration that (a) recorded only the definition line, (b) recorded one edit + * per graph-ref file then broke, and (c) skipped text-search on any file already + * covered by the graph. When a private symbol's definition and all its call + * sites live in one file, only the definition line was reported (total_edits: 1) + * while apply rewrote every occurrence. This test drives the exact single-file + * case with an empty graph (no incoming refs) and asserts report == apply. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +// Prevent onnxruntime / native search adapters from loading at import time +// (mirrors test/unit/calltool-dispatch.test.ts). We drive the private rename() +// directly, so the graph/DB/embedding layers are never exercised. +vi.mock('../../src/core/search/bm25-index.js', () => ({ + searchFTSFromLbug: vi.fn().mockResolvedValue({ results: [], ftsAvailable: true }), +})); +vi.mock('../../src/mcp/core/embedder.js', () => ({ + embedQuery: vi.fn().mockResolvedValue([]), + getEmbeddingDims: vi.fn().mockReturnValue(384), +})); + +import { LocalBackend } from '../../src/mcp/local/local-backend.js'; + +// The #2605 repro: a private free fn with exactly 4 textual occurrences of +// `rename_target` — the definition, one production call, two test calls — all +// in the same file. +const RUST_SRC = `fn rename_target(x: u32) -> u32 { + x + 1 +} + +pub fn prod_call() -> u32 { + rename_target(1) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn unit_one() { + assert_eq!(rename_target(1), 2); + } + + #[test] + fn unit_two() { + assert_eq!(rename_target(2), 3); + } +} +`; + +// 1-based occurrence lines of `rename_target` in RUST_SRC — the ground truth the +// report must match. Computed from the fixture (not hardcoded) so editing the +// snippet cannot silently desync the expectation. +const OCCURRENCE_LINES = RUST_SRC.split('\n') + .map((line, i) => (/\brename_target\b/.test(line) ? i + 1 : 0)) + .filter((n) => n > 0); + +/** Build a backend whose graph lookup returns the symbol with NO incoming refs + * (the exact condition that made the old code report only the definition). */ +function stubbedBackend(): LocalBackend { + const backend = new LocalBackend(); + vi.spyOn(backend as unknown as { ensureInitialized: () => Promise }, 'ensureInitialized').mockResolvedValue( + undefined, + ); + vi.spyOn(backend as unknown as { context: () => Promise }, 'context').mockResolvedValue({ + status: 'success', + symbol: { name: 'rename_target', filePath: 'src/lib.rs', startLine: OCCURRENCE_LINES[0] }, + incoming: { calls: [], imports: [], extends: [], implements: [] }, + }); + return backend; +} + +describe('rename edit report is faithful to apply (#2605)', () => { + let backend: LocalBackend; + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-2605-')); + await fs.mkdir(path.join(tmpDir, 'src')); + await fs.writeFile(path.join(tmpDir, 'src', 'lib.rs'), RUST_SRC, 'utf-8'); + backend = stubbedBackend(); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + await fs.rm(tmpDir, { recursive: true, force: true }); + }); + + it('previews every occurrence that apply will rewrite (dry_run)', async () => { + const result = await ( + backend as unknown as { rename: (r: unknown, p: unknown) => Promise } + ).rename({ repoPath: tmpDir }, { symbol_name: 'rename_target', new_name: 'renamed_fn', dry_run: true }); + + expect(result.applied).toBe(false); + expect(result.files_affected).toBe(1); + expect(result.total_edits).toBe(OCCURRENCE_LINES.length); // 4, not 1 + expect(result.graph_edits + result.text_search_edits).toBe(result.total_edits); + + const reportedLines = result.changes[0].edits + .map((e: { line: number }) => e.line) + .sort((a: number, b: number) => a - b); + expect(reportedLines).toEqual(OCCURRENCE_LINES); + + // A dry run leaves the file untouched. + const onDisk = await fs.readFile(path.join(tmpDir, 'src', 'lib.rs'), 'utf-8'); + expect(onDisk).toContain('rename_target'); + }); + + it('reports exactly what it wrote (apply)', async () => { + const result = await ( + backend as unknown as { rename: (r: unknown, p: unknown) => Promise } + ).rename({ repoPath: tmpDir }, { symbol_name: 'rename_target', new_name: 'renamed_fn', dry_run: false }); + + expect(result.applied).toBe(true); + expect(result.total_edits).toBe(OCCURRENCE_LINES.length); + + const onDisk = await fs.readFile(path.join(tmpDir, 'src', 'lib.rs'), 'utf-8'); + const renamedCount = (onDisk.match(/\brenamed_fn\b/g) || []).length; + const stragglers = (onDisk.match(/\brename_target\b/g) || []).length; + expect(renamedCount).toBe(OCCURRENCE_LINES.length); // all 4 rewritten + expect(stragglers).toBe(0); + + // The reported edit count equals the number of replacements that landed. + const reportedEdits = result.changes.reduce( + (n: number, c: { edits: unknown[] }) => n + c.edits.length, + 0, + ); + expect(reportedEdits).toBe(renamedCount); + }); +});