diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index dac37b6ef..5b104dd32 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -4361,44 +4361,31 @@ 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 makes + // the preview enumerate exactly the files the apply loop rewrites, using the + // same word-boundary regex. (This is per-call consistency; the apply loop + // still re-reads each file, so an external write landing between preview and + // apply is a pre-existing gap this method does not lock against.) + 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 +4395,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,67 +4428,98 @@ 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); } - // Step 4: Apply or preview - const allChanges = Array.from(changes.values()); - const totalEdits = allChanges.reduce((sum, c) => sum + c.edits.length, 0); + // Enumerate every `\boldName\b` line in each file to rewrite, so the previewed + // file set is exactly the set the apply loop below rewrites. A file with no + // matching line is dropped (apply would write nothing to it). `wordTest` + // (non-global) probes each line; `wordReplace` (global) rewrites it and is + // reused by the apply loop — compiled once each rather than once per line, + // and one escaping formula serves both passes. + const wordTest = new RegExp(`\\b${escapedOldName}\\b`); + const wordReplace = new RegExp(`\\b${escapedOldName}\\b`, 'g'); + const changes = new Map(); + 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 (!wordTest.test(lines[i])) { + continue; + } + edits.push({ + line: i + 1, + old_text: lines[i].trim(), + new_text: lines[i].replace(wordReplace, new_name).trim(), + confidence, + }); + } + if (edits.length > 0) { + changes.set(filePath, { file_path: filePath, edits }); + } + } catch (e) { + logQueryError('rename:enumerate', e); + } + } + + // Step 4: Apply or preview. const failedFiles: string[] = []; if (!dry_run) { - // Apply edits to files - for (const change of allChanges) { + for (const change of changes.values()) { try { const fullPath = assertSafePath(change.file_path); - let content = await fs.readFile(fullPath, 'utf-8'); - const regex = new RegExp(`\\b${oldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\b`, 'g'); - content = content.replace(regex, new_name); - await fs.writeFile(fullPath, content, 'utf-8'); + const content = await fs.readFile(fullPath, 'utf-8'); + await fs.writeFile(fullPath, content.replace(wordReplace, new_name), 'utf-8'); } catch (e) { - // A swallowed write failure must not be reported as a full success - // (#2283): record the file so the result can degrade to 'partial' - // with the unwritten files listed, rather than masquerading as done. + // A swallowed write failure must not be reported as success (#2283): + // record the file so the result degrades to 'partial'. logQueryError('rename:apply-edit', e); failedFiles.push(change.file_path); } } + // A file whose write threw did not land, so drop its edits from the + // reported result — total_edits/changes must describe what actually + // reached disk, not what was attempted (#2605: the report matches reality + // even on partial failure). failed_files still names every dropped file. + for (const f of failedFiles) { + changes.delete(f); + } + } + + // Counts derive from the reported set (dry-run: every enumerated file; + // apply: only files that landed), so the graph/text_search split always + // sums to total_edits and never overstates a partial apply. + const reported = Array.from(changes.values()); + let graphEdits = 0; + let astSearchEdits = 0; + for (const change of reported) { + for (const edit of change.edits) { + if (edit.confidence === 'graph') { + graphEdits++; + } else { + astSearchEdits++; + } + } } return { status: failedFiles.length > 0 ? 'partial' : 'success', old_name: oldName, new_name, - files_affected: allChanges.length, - total_edits: totalEdits, + files_affected: reported.length, + total_edits: graphEdits + astSearchEdits, graph_edits: graphEdits, text_search_edits: astSearchEdits, - changes: allChanges, + changes: reported, applied: !dry_run, ...(failedFiles.length > 0 && { failed_files: failedFiles }), }; 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..b7088b9e7 --- /dev/null +++ b/gitnexus/test/unit/rename-edit-report.test.ts @@ -0,0 +1,239 @@ +/** + * 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. These tests drive the single-file repro, + * a mixed graph/text_search multi-file rename, and a partial write failure, and + * assert the report matches what apply actually writes in each case. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'node:fs'; +import fsPromises from 'fs/promises'; +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), +})); + +// rename() shells out to `rg -l` to discover text-search files. Stub it so the +// ripgrep-discovery branch is deterministic and driveable (rg is not reliably +// on PATH inside the vitest worker). Default: no hits. +const { execFileSyncMock } = vi.hoisted(() => ({ execFileSyncMock: vi.fn(() => '') })); +vi.mock('child_process', async (importActual) => { + const actual = await importActual(); + return { ...actual, execFileSync: execFileSyncMock }; +}); + +import { LocalBackend } from '../../src/mcp/local/local-backend.js'; + +type Incoming = { + calls: { filePath: string }[]; + imports: { filePath: string }[]; + extends: { filePath: string }[]; + implements: { filePath: string }[]; +}; +const EMPTY_INCOMING: Incoming = { calls: [], imports: [], extends: [], implements: [] }; + +type RenameResult = { + status: string; + applied: boolean; + files_affected: number; + total_edits: number; + graph_edits: number; + text_search_edits: number; + changes: { file_path: string; edits: { line: number; confidence: string }[] }[]; + failed_files?: string[]; +}; + +// 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 `src` — the ground truth the + * report must match. Computed (not hardcoded) so editing a fixture cannot + * silently desync the expectation. */ +function occurrenceLines(src: string): number[] { + return src + .split('\n') + .map((line, i) => (/\brename_target\b/.test(line) ? i + 1 : 0)) + .filter((n) => n > 0); +} +const OCCURRENCE_LINES = occurrenceLines(RUST_SRC); + +/** Build a backend whose graph lookup returns the symbol (definition at + * src/lib.rs) with the given incoming refs. */ +function stubbedBackend(incoming: Incoming = EMPTY_INCOMING): 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, + }); + return backend; +} + +function callRename( + backend: LocalBackend, + repoPath: string, + params: Record, +): Promise { + return ( + backend as unknown as { rename: (r: unknown, p: unknown) => Promise } + ).rename({ repoPath }, { symbol_name: 'rename_target', new_name: 'renamed_fn', ...params }); +} + +const editsFor = (r: RenameResult, file: string) => + r.changes.find((c) => c.file_path === file)?.edits ?? []; + +describe('rename edit report is faithful to apply (#2605)', () => { + let tmpDir: string; + + beforeEach(async () => { + execFileSyncMock.mockReturnValue(''); // default: no ripgrep hits + 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'); + }); + + 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 callRename(stubbedBackend(), tmpDir, { 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 + // Concrete split, not just the sum: all occurrences are in the definition + // file, so they are graph-confidence and text_search is zero. + expect(result.graph_edits).toBe(OCCURRENCE_LINES.length); + expect(result.text_search_edits).toBe(0); + + const edits = editsFor(result, 'src/lib.rs'); + expect(edits.map((e) => e.line).sort((a, b) => a - b)).toEqual(OCCURRENCE_LINES); + expect(edits.every((e) => e.confidence === 'graph')).toBe(true); + + // 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 callRename(stubbedBackend(), tmpDir, { 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, c) => n + c.edits.length, 0); + expect(reportedEdits).toBe(renamedCount); + }); + + it('enumerates all occurrences across graph-ref and text-search files, keeping confidence per file', async () => { + // A graph-referencing file (not the definition) with MULTIPLE occurrences — + // the exact "one edit per file then break" bug's other original trigger. + const CALLER = 'use crate::rename_target;\nfn a() { rename_target(1); rename_target(2); }\n'; + // A file discovered only by ripgrep — the text_search branch. + const NOTES = '// see rename_target for details\n'; + await fs.writeFile(path.join(tmpDir, 'src', 'caller.rs'), CALLER, 'utf-8'); + await fs.writeFile(path.join(tmpDir, 'src', 'notes.rs'), NOTES, 'utf-8'); + // rg reports the definition file (already graph — exercises never-downgrade) + // and the text-only file. + execFileSyncMock.mockReturnValue('src/lib.rs\nsrc/notes.rs\n'); + + const backend = stubbedBackend({ + ...EMPTY_INCOMING, + calls: [{ filePath: 'src/caller.rs' }], + }); + const result = await callRename(backend, tmpDir, { dry_run: true }); + + const callerOcc = occurrenceLines(CALLER).length; // 3 + const notesOcc = occurrenceLines(NOTES).length; // 1 + + expect(result.files_affected).toBe(3); + expect(result.total_edits).toBe(OCCURRENCE_LINES.length + callerOcc + notesOcc); + // Split is concrete: definition + graph-ref file are graph; the rg-only file + // is text_search. A file reached by both graph and rg keeps graph (never + // downgraded). + expect(result.graph_edits).toBe(OCCURRENCE_LINES.length + callerOcc); + expect(result.text_search_edits).toBe(notesOcc); + + expect(editsFor(result, 'src/lib.rs').every((e) => e.confidence === 'graph')).toBe(true); + expect(editsFor(result, 'src/caller.rs').map((e) => e.confidence)).toEqual(['graph', 'graph']); + expect(editsFor(result, 'src/notes.rs').map((e) => e.confidence)).toEqual(['text_search']); + }); + + it('reports only files that landed when a write fails mid-apply (#2605 partial)', async () => { + const CALLER = 'fn a() { rename_target(1); rename_target(2); }\n'; + await fs.writeFile(path.join(tmpDir, 'src', 'caller.rs'), CALLER, 'utf-8'); + const backend = stubbedBackend({ ...EMPTY_INCOMING, calls: [{ filePath: 'src/caller.rs' }] }); + + // caller.rs write throws; lib.rs succeeds. + vi.spyOn(fsPromises, 'writeFile').mockImplementation( + async (p: Parameters[0]) => { + if (String(p).endsWith(`${path.sep}caller.rs`)) { + throw Object.assign(new Error('EACCES'), { code: 'EACCES' }); + } + }, + ); + + const result = await callRename(backend, tmpDir, { dry_run: false }); + + expect(result.status).toBe('partial'); + expect(result.failed_files).toEqual(['src/caller.rs']); + // The failed file's edits are NOT reported as applied: totals describe only + // what reached disk (lib.rs), never the attempted caller.rs occurrences. + expect(result.files_affected).toBe(1); + expect(result.total_edits).toBe(OCCURRENCE_LINES.length); + expect(result.graph_edits).toBe(OCCURRENCE_LINES.length); + expect(result.changes.map((c) => c.file_path)).toEqual(['src/lib.rs']); + }); +});