fix(mcp): report every rename edit that apply writes (#2605)
rename() reported total_edits from a partial enumeration (definition line only, one-edit-per-graph-file then break, and text search that skipped any file already covered by the graph) while the apply step does a whole-file \boldName\b global replace on every touched file. When a private symbol's definition and all its call sites live in one file, only the definition line was reported (total_edits: 1) even though apply rewrote every occurrence, in both dry-run and apply. Rebuild changes/total_edits/graph_edits/text_search_edits from one file set: classify each file to rewrite (definition + graph refs = graph confidence; rg-only files = text_search, never downgrading a graph file), then enumerate every matching line per file with apply's exact escaped global regex. The reported edit list now equals what apply writes. Apply behavior is unchanged. Adds a regression test reproducing the issue's single-file Rust case (def + 3 same-file call sites, empty graph): total_edits is 4 in both dry-run and apply, and equals the replacements that land on disk.
This commit is contained in:
Gergo Magyar
@@ -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<string, { file_path: string; edits: any[] }>();
|
||||
|
||||
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<string, 'graph' | 'text_search'>();
|
||||
|
||||
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<string, { file_path: string; edits: RenameEdit[] }>();
|
||||
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);
|
||||
|
||||
@@ -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<void> }, 'ensureInitialized').mockResolvedValue(
|
||||
undefined,
|
||||
);
|
||||
vi.spyOn(backend as unknown as { context: () => Promise<unknown> }, '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<any> }
|
||||
).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<any> }
|
||||
).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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user