diff --git a/src/gateway/chat/execute-tool.ts b/src/gateway/chat/execute-tool.ts index a02edf9..5204aad 100644 --- a/src/gateway/chat/execute-tool.ts +++ b/src/gateway/chat/execute-tool.ts @@ -1,3 +1,4 @@ +import { applySearchReplaceEdits } from './search-replace-edit'; import path from 'path'; import fs from 'fs'; import { getConfig } from '../../config/config'; @@ -517,41 +518,10 @@ print(json.dumps({'slides': slides, 'total': len(prs.slides)}, ensure_ascii=Fals if (!fs.existsSync(filePath)) return { name, args, result: `"${filename}" not found. Use coder_write_file for new files.`, error: true }; const content = fs.readFileSync(filePath, 'utf-8'); const edits = String(args.edits || ''); - // Parse SEARCH/REPLACE blocks - const srRegex = /------- SEARCH\n([\s\S]*?)\n=======\n([\s\S]*?)\n\+\+\+\+\+\+\+ REPLACE/g; - let match; - let result = content; - let editCount = 0; - const failedEdits: string[] = []; - while ((match = srRegex.exec(edits)) !== null) { - const searchText = match[1]; - const replaceText = match[2]; - if (result.includes(searchText)) { - result = result.replace(searchText, replaceText); - editCount++; - } else { - // Fallback: try with normalized whitespace (trim each line) - const normalize = (s: string) => s.split('\n').map(l => l.trimEnd()).join('\n'); - if (normalize(result).includes(normalize(searchText))) { - // Find the original text with actual whitespace - const lines = result.split('\n'); - const searchLines = searchText.split('\n'); - const normLines = lines.map(l => l.trimEnd()); - const normSearch = searchLines.map(l => l.trimEnd()); - for (let i = 0; i <= normLines.length - normSearch.length; i++) { - if (normLines.slice(i, i + normSearch.length).join('\n') === normSearch.join('\n')) { - const originalText = lines.slice(i, i + searchLines.length).join('\n'); - result = result.replace(originalText, replaceText); - editCount++; - break; - } - } - } else { - const firstLine = searchText.split('\n')[0]?.slice(0, 80) || '(empty)'; - failedEdits.push(firstLine); - } - } - } + const _patch = applySearchReplaceEdits(content, edits); + const result = _patch.content; + const editCount = _patch.editCount; + const failedEdits = _patch.failedEdits; if (editCount > 0) { fs.writeFileSync(filePath, result, 'utf-8'); const msg = `${filename}: ${editCount} edit(s) applied`; @@ -560,8 +530,8 @@ print(json.dumps({'slides': slides, 'total': len(prs.slides)}, ensure_ascii=Fals } return { name, args, result: msg, error: false }; } - // No srRegex matches — check if the model used a different format - if (!edits.includes('------- SEARCH') && edits.length > 0) { + // No blocks parsed at all — the model used a different format. + if (!_patch.hadAnyBlock && edits.length > 0) { return { name, args, result: `No SEARCH/REPLACE blocks found. Use the format:\n------- SEARCH\nexact text\n=======\nreplacement\n+++++++ REPLACE`, error: true }; } return { name, args, result: `No edits applied. SEARCH text not found in "${filename}". Use coder_read_file to verify exact content, including whitespace and indentation. Failed matches:\n${failedEdits.join('\n')}`, error: true }; diff --git a/src/gateway/chat/search-replace-edit.ts b/src/gateway/chat/search-replace-edit.ts new file mode 100644 index 0000000..e8d6b42 --- /dev/null +++ b/src/gateway/chat/search-replace-edit.ts @@ -0,0 +1,96 @@ +/** + * search-replace-edit.ts + * + * Applies coder_patch_file's SEARCH/REPLACE blocks to a file's contents. + * + * This is the highest-stakes pure function in the tool layer: it rewrites the user's actual + * source files. A bug here does not throw — it silently writes the wrong bytes, or reports + * "3 edits applied" while having applied two. Until 2026-07-29 it lived inline inside + * executeTool()'s 1,349-line body next to the fs.writeFileSync that consumes its output, so + * there was no way to exercise it without touching the disk. + * + * The whitespace fallback is the subtle part. Models routinely reproduce a SEARCH block with + * trailing whitespace that differs from the file, so an exact `includes()` fails on text that + * is visually identical. Rather than fail the edit, we retry with every line right-trimmed, and + * on a hit map the match back to the ORIGINAL text so the replacement preserves the file's real + * bytes outside the matched span. Leading whitespace is deliberately NOT normalised — it is + * indentation, and normalising it would let an edit land on a differently-indented block. + * + * ⚠ KNOWN SHARP EDGE (documented by tests, behaviour predates the 2026-07-29 extraction): + * matching is substring-based, not line-anchored, so a SEARCH indented less than the file still + * matches inside the deeper line — " deep()" hits within " deep()". The surviving + * indentation usually makes the result benign, but it does mean an edit aimed at one line can + * land on another. Changing this to line-anchored matching would alter file-editing semantics + * for every caller, so it is left as-is and pinned by a test rather than silently "fixed". + */ + +/** Block delimiters coder_patch_file's tool description tells the model to emit. */ +const SEARCH_REPLACE_BLOCK = /------- SEARCH\n([\s\S]*?)\n=======\n([\s\S]*?)\n\+\+\+\+\+\+\+ REPLACE/g; + +export const SEARCH_MARKER = '------- SEARCH'; + +export interface SearchReplaceResult { + /** File contents after every applicable edit. Unchanged when nothing matched. */ + content: string; + /** How many blocks were actually applied. */ + editCount: number; + /** First line (≤80 chars) of each block whose SEARCH text could not be located. */ + failedEdits: string[]; + /** False when the edits text contains no SEARCH marker at all — a format error, not a miss. */ + hadAnyBlock: boolean; +} + +const rightTrimLines = (s: string) => s.split('\n').map(l => l.trimEnd()).join('\n'); + +export function applySearchReplaceEdits(originalContent: string, editsText: string): SearchReplaceResult { + const edits = String(editsText || ''); + let content = String(originalContent ?? ''); + let editCount = 0; + const failedEdits: string[] = []; + + // exec() with /g is stateful; use a fresh regex per call so concurrent calls can't interfere. + const re = new RegExp(SEARCH_REPLACE_BLOCK.source, 'g'); + let match: RegExpExecArray | null; + let sawBlock = false; + + while ((match = re.exec(edits)) !== null) { + sawBlock = true; + const searchText = match[1]; + const replaceText = match[2]; + + if (content.includes(searchText)) { + content = content.replace(searchText, replaceText); + editCount++; + continue; + } + + // Fallback: compare with trailing whitespace stripped, then locate the corresponding + // ORIGINAL span so the file's real bytes are what gets replaced. + const normContent = rightTrimLines(content); + const normSearch = rightTrimLines(searchText); + if (!normContent.includes(normSearch)) { + failedEdits.push(searchText.split('\n')[0]?.slice(0, 80) || '(empty)'); + continue; + } + + const lines = content.split('\n'); + const searchLines = searchText.split('\n'); + const normLines = lines.map(l => l.trimEnd()); + const normSearchLines = searchLines.map(l => l.trimEnd()); + let applied = false; + for (let i = 0; i <= normLines.length - normSearchLines.length; i++) { + if (normLines.slice(i, i + normSearchLines.length).join('\n') === normSearchLines.join('\n')) { + const originalText = lines.slice(i, i + searchLines.length).join('\n'); + content = content.replace(originalText, replaceText); + editCount++; + applied = true; + break; + } + } + // Normalised compare matched but the line-window scan did not — treat as a miss rather than + // silently reporting success for an edit that never happened. + if (!applied) failedEdits.push(searchText.split('\n')[0]?.slice(0, 80) || '(empty)'); + } + + return { content, editCount, failedEdits, hadAnyBlock: sawBlock || edits.includes(SEARCH_MARKER) }; +} diff --git a/tests/search-replace-edit.test.ts b/tests/search-replace-edit.test.ts new file mode 100644 index 0000000..68945fa --- /dev/null +++ b/tests/search-replace-edit.test.ts @@ -0,0 +1,140 @@ +/** + * search-replace-edit.test.ts + * + * This function rewrites the user's real source files. It has no way to "fail loudly" — a bug + * writes wrong bytes to disk, or reports "3 edits applied" having applied two, and the user + * finds out later from broken code. It was inline inside executeTool()'s 1,349-line body until + * 2026-07-29, so none of this had ever been exercised without touching the filesystem. + * + * The properties worth guarding, in rough order of how much damage a regression does: + * 1. never claim an edit that did not happen (silent corruption of the edit report) + * 2. never replace the wrong span (silent corruption of the file) + * 3. preserve bytes outside the matched span (indentation, trailing whitespace) + * 4. report misses so the model can retry + */ + +import { test, describe } from 'node:test'; +import assert from 'node:assert/strict'; +import { applySearchReplaceEdits } from '../src/gateway/chat/search-replace-edit'; + +const block = (search: string, replace: string) => + `------- SEARCH\n${search}\n=======\n${replace}\n+++++++ REPLACE`; + +describe('정확히 일치하는 경우', () => { + test('한 블록을 적용한다', () => { + const r = applySearchReplaceEdits('a\nb\nc\n', block('b', 'B')); + assert.equal(r.content, 'a\nB\nc\n'); + assert.equal(r.editCount, 1); + assert.deepEqual(r.failedEdits, []); + }); + + test('여러 블록을 순서대로 적용한다', () => { + const r = applySearchReplaceEdits('one\ntwo\nthree\n', block('one', '1') + '\n' + block('three', '3')); + assert.equal(r.content, '1\ntwo\n3\n'); + assert.equal(r.editCount, 2); + }); + + test('여러 줄 블록', () => { + const src = 'def f():\n return 1\n\nprint(f())\n'; + const r = applySearchReplaceEdits(src, block('def f():\n return 1', 'def f():\n return 42')); + assert.equal(r.content, 'def f():\n return 42\n\nprint(f())\n'); + assert.equal(r.editCount, 1); + }); + + test('첫 번째 일치만 바꾼다 — 전역 치환이 아니다', () => { + const r = applySearchReplaceEdits('x\nx\nx\n', block('x', 'y')); + assert.equal(r.content, 'y\nx\nx\n'); + assert.equal(r.editCount, 1); + }); +}); + +describe('후행 공백 폴백 — 모델이 흔히 틀리는 지점', () => { + test('파일에 후행 공백이 있어도 매칭된다', () => { + const src = 'const a = 1; \nconst b = 2;\n'; + const r = applySearchReplaceEdits(src, block('const a = 1;', 'const a = 99;')); + assert.equal(r.editCount, 1); + assert.ok(r.content.startsWith('const a = 99;'), r.content); + }); + + test('SEARCH 쪽에 후행 공백이 있어도 매칭된다', () => { + const r = applySearchReplaceEdits('const a = 1;\n', block('const a = 1; ', 'const a = 99;')); + assert.equal(r.editCount, 1); + assert.equal(r.content, 'const a = 99;\n'); + }); + + test('매칭 구간 밖의 바이트는 보존된다', () => { + // SEARCH가 'target'뿐이면 파일에 있던 후행 공백은 매칭 구간 밖이므로 그대로 남는다. + // 앞뒤 줄의 공백도 손대지 않는다. + const src = 'head \ntarget \ntail \n'; + const r = applySearchReplaceEdits(src, block('target', 'TARGET')); + assert.equal(r.editCount, 1); + assert.equal(r.content, 'head \nTARGET \ntail \n'); + }); + + test('⚠ 알려진 날카로운 모서리: SEARCH가 더 얕게 들여쓰기돼도 부분 문자열로 매칭된다', () => { + // 8칸 들여쓴 줄 안에 4칸 들여쓴 SEARCH가 부분 문자열로 들어있어 그대로 치환된다. + // 결과는 원래 들여쓰기가 유지되므로(4칸 + 치환문) 파이썬에서는 대개 무해하지만, + // "정확히 이 줄"을 노린 편집이 다른 줄에 적용될 수 있는 경로이기도 하다. + // 이는 추출 이전부터 있던 동작이며, 줄 단위 앵커로 바꾸는 것은 파일 편집 의미를 + // 바꾸는 일이라 별도 판단이 필요하다. 지금은 동작을 문서화만 해둔다. + const src = 'if x:\n deep()\n'; + const r = applySearchReplaceEdits(src, block(' deep()', ' shallow()')); + assert.equal(r.editCount, 1); + assert.equal(r.content, 'if x:\n shallow()\n'); + }); +}); + +describe('실패 보고 — 적용하지 않은 편집을 성공으로 세지 않는다', () => { + test('찾지 못한 블록은 failedEdits에 남고 editCount에 포함되지 않는다', () => { + const r = applySearchReplaceEdits('a\n', block('없는텍스트', 'x')); + assert.equal(r.editCount, 0); + assert.deepEqual(r.failedEdits, ['없는텍스트']); + assert.equal(r.content, 'a\n', '실패했는데 파일이 바뀜'); + }); + + test('성공과 실패가 섞이면 각각 정확히 집계된다', () => { + const r = applySearchReplaceEdits('a\nb\n', block('a', 'A') + '\n' + block('없음', 'x')); + assert.equal(r.editCount, 1); + assert.equal(r.failedEdits.length, 1); + assert.equal(r.content, 'A\nb\n'); + }); + + test('실패 라벨은 첫 줄 80자로 자른다', () => { + const long = 'z'.repeat(200); + const r = applySearchReplaceEdits('a\n', block(long, 'x')); + assert.equal(r.failedEdits[0].length, 80); + }); +}); + +describe('형식 오류 구분', () => { + test('SEARCH 마커가 아예 없으면 hadAnyBlock=false — 형식 안내를 보내야 하는 경우', () => { + const r = applySearchReplaceEdits('a\n', 'just some text, no markers'); + assert.equal(r.hadAnyBlock, false); + assert.equal(r.editCount, 0); + }); + + test('마커는 있는데 매칭이 안 된 경우는 형식 오류가 아니다', () => { + // 이 둘을 섞으면 모델에게 엉뚱한 안내가 나간다. + const r = applySearchReplaceEdits('a\n', block('없음', 'x')); + assert.equal(r.hadAnyBlock, true); + assert.equal(r.editCount, 0); + }); + + test('빈 입력에도 안전하다', () => { + const r = applySearchReplaceEdits('', ''); + assert.equal(r.content, ''); + assert.equal(r.editCount, 0); + assert.equal(r.hadAnyBlock, false); + }); +}); + +describe('호출 간 상태 오염 방지', () => { + test('연속 호출이 서로 영향을 주지 않는다 (정규식 lastIndex)', () => { + // /g 정규식을 모듈 스코프에서 재사용하면 두 번째 호출이 중간부터 시작해 조용히 편집을 건너뛴다. + const edits = block('a', 'A'); + const first = applySearchReplaceEdits('a\n', edits); + const second = applySearchReplaceEdits('a\n', edits); + assert.equal(first.editCount, 1); + assert.equal(second.editCount, 1, '두 번째 호출에서 편집이 유실됨 — lastIndex 오염'); + }); +});