From 4e97a278d14fe4a546d5fb97a846ff02b4f311fa Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 21 Jul 2026 14:31:47 +0000 Subject: [PATCH 1/3] 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. --- gitnexus/src/mcp/local/local-backend.ts | 155 +++++++----------- gitnexus/test/unit/rename-edit-report.test.ts | 137 ++++++++++++++++ 2 files changed, 200 insertions(+), 92 deletions(-) create mode 100644 gitnexus/test/unit/rename-edit-report.test.ts 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); + }); +}); From 902186c4f89be7ee5cfa846ecab7be0ede2bb7bb Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 21 Jul 2026 14:41:19 +0000 Subject: [PATCH 2/3] style: prettier-format rename-edit-report test (#2605) --- gitnexus/test/unit/rename-edit-report.test.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/gitnexus/test/unit/rename-edit-report.test.ts b/gitnexus/test/unit/rename-edit-report.test.ts index ba651abf4..0d0993e96 100644 --- a/gitnexus/test/unit/rename-edit-report.test.ts +++ b/gitnexus/test/unit/rename-edit-report.test.ts @@ -66,9 +66,10 @@ const OCCURRENCE_LINES = RUST_SRC.split('\n') * (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 { 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] }, @@ -96,7 +97,10 @@ describe('rename edit report is faithful to apply (#2605)', () => { 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 }); + ).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); @@ -116,7 +120,10 @@ describe('rename edit report is faithful to apply (#2605)', () => { 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 }); + ).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); From 54c44d91decc05ba15f6ebe9804fc4aeba562242 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 21 Jul 2026 15:14:32 +0000 Subject: [PATCH 3/3] fix(mcp): reconcile rename report on partial failure; harden enumerate (#2605) Addresses gitnexus-review-agent findings on PR #2608: - MED: on a partial apply (a file's write throws), drop that file's edits from total_edits/graph_edits/text_search_edits/changes so the reported result describes what actually reached disk, not what was attempted. The comprehensive enumeration otherwise let a failing file contribute its entire line count as phantom 'applied' edits. failed_files still names every dropped file. Counts are now derived once from the reported set. - MED: hoist the word-boundary regexes out of the per-line loop (one compile each instead of one per line), reused by the apply loop. - LOW: apply loop reuses escapedOldName instead of recomputing the escape formula inline (removes a preview/apply drift risk). - Soften the in-code comment: enumeration gives per-call preview/apply consistency; the pre-existing two-read TOCTOU (external write between preview and apply) is out of scope and noted, not newly introduced. Tests: add a mixed graph-ref + text_search multi-file case (asserts per-file confidence and the never-downgrade guard, via a stubbed rg), and a partial-write-failure case (asserts only landed files are reported). Assert concrete graph_edits/text_search_edits splits, not just their sum. --- gitnexus/src/mcp/local/local-backend.ts | 78 +++++---- gitnexus/test/unit/rename-edit-report.test.ts | 165 ++++++++++++++---- 2 files changed, 177 insertions(+), 66 deletions(-) diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index dacd0a4cd..5b104dd32 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -4366,8 +4366,11 @@ export class LocalBackend { // 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. + // 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; @@ -4434,13 +4437,15 @@ export class LocalBackend { 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. + // 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(); - let graphEdits = 0; - let astSearchEdits = 0; for (const [filePath, confidence] of fileConfidence) { try { @@ -4448,20 +4453,15 @@ export class LocalBackend { 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])) { + if (!wordTest.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(), + new_text: lines[i].replace(wordReplace, new_name).trim(), confidence, }); - if (confidence === 'graph') { - graphEdits++; - } else { - astSearchEdits++; - } } if (edits.length > 0) { changes.set(filePath, { file_path: filePath, edits }); @@ -4471,39 +4471,55 @@ export class LocalBackend { } } - // Step 4: Apply or preview - const allChanges = Array.from(changes.values()); - const totalEdits = allChanges.reduce((sum, c) => sum + c.edits.length, 0); - + // 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 index 0d0993e96..b7088b9e7 100644 --- a/gitnexus/test/unit/rename-edit-report.test.ts +++ b/gitnexus/test/unit/rename-edit-report.test.ts @@ -7,11 +7,13 @@ * 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. + * 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'; @@ -26,8 +28,36 @@ vi.mock('../../src/mcp/core/embedder.js', () => ({ 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. @@ -55,16 +85,20 @@ mod tests { } `; -// 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); +/** 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 with NO incoming refs - * (the exact condition that made the old code report only the definition). */ -function stubbedBackend(): LocalBackend { +/** 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 }, @@ -73,20 +107,32 @@ function stubbedBackend(): LocalBackend { 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: [] }, + 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 backend: LocalBackend; 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'); - backend = stubbedBackend(); }); afterEach(async () => { @@ -95,22 +141,19 @@ describe('rename edit report is faithful to apply (#2605)', () => { }); 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 }, - ); + 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 - expect(result.graph_edits + result.text_search_edits).toBe(result.total_edits); + // 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 reportedLines = result.changes[0].edits - .map((e: { line: number }) => e.line) - .sort((a: number, b: number) => a - b); - expect(reportedLines).toEqual(OCCURRENCE_LINES); + 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'); @@ -118,12 +161,7 @@ describe('rename edit report is faithful to apply (#2605)', () => { }); 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 }, - ); + const result = await callRename(stubbedBackend(), tmpDir, { dry_run: false }); expect(result.applied).toBe(true); expect(result.total_edits).toBe(OCCURRENCE_LINES.length); @@ -135,10 +173,67 @@ describe('rename edit report is faithful to apply (#2605)', () => { 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, - ); + 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']); + }); });