diff --git a/src/gateway/chat/search-replace-edit.ts b/src/gateway/chat/search-replace-edit.ts index e8d6b42..2232a64 100644 --- a/src/gateway/chat/search-replace-edit.ts +++ b/src/gateway/chat/search-replace-edit.ts @@ -16,12 +16,21 @@ * 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". + * LINE ANCHORING (2026-07-29). Matching used to be plain substring search, which let a SEARCH + * indented less than the file match *inside* a deeper line — " deep()" hit within + * " deep()", so an edit aimed at one line could land on another. Requiring every match to + * start at a line boundary would have been the obvious fix, but it breaks the common and + * legitimate case of editing a fragment within a line ("return 1" → "return 42"), where the + * model never asserted anything about indentation. + * + * So the anchor is conditional, on what the SEARCH text itself claims: + * - leading whitespace, or spans multiple lines → must start at a line boundary + * (the block is asserting indentation / whole-line structure, so it has to line up) + * - single line with no leading whitespace → substring match, as before + * (a fragment inside a line; indentation was never part of the claim) + * + * A rejected match fails CLOSED: the edit is reported as not-found and the model retries with + * fuller context, instead of silently rewriting the wrong line. */ /** Block delimiters coder_patch_file's tool description tells the model to emit. */ @@ -42,6 +51,28 @@ export interface SearchReplaceResult { const rightTrimLines = (s: string) => s.split('\n').map(l => l.trimEnd()).join('\n'); +/** True when the SEARCH text makes a claim about indentation or whole-line structure. */ +function requiresLineAnchor(searchText: string): boolean { + return /^[ \t]/.test(searchText) || searchText.includes('\n'); +} + +/** + * Index of the first acceptable occurrence of `searchText`, or -1. + * When the search requires anchoring, occurrences that start mid-line are skipped rather than + * failing outright — the same text may legitimately appear again at a proper line start. + */ +function findMatchIndex(content: string, searchText: string): number { + if (!searchText) return -1; + const mustAnchor = requiresLineAnchor(searchText); + let from = 0; + for (;;) { + const idx = content.indexOf(searchText, from); + if (idx === -1) return -1; + if (!mustAnchor || idx === 0 || content[idx - 1] === '\n') return idx; + from = idx + 1; + } +} + export function applySearchReplaceEdits(originalContent: string, editsText: string): SearchReplaceResult { const edits = String(editsText || ''); let content = String(originalContent ?? ''); @@ -58,8 +89,11 @@ export function applySearchReplaceEdits(originalContent: string, editsText: stri const searchText = match[1]; const replaceText = match[2]; - if (content.includes(searchText)) { - content = content.replace(searchText, replaceText); + const hit = findMatchIndex(content, searchText); + if (hit !== -1) { + // Splice by index rather than String.replace(): replace() would re-scan from the start and + // could land on an earlier, unanchored occurrence, undoing the anchoring decision above. + content = content.slice(0, hit) + replaceText + content.slice(hit + searchText.length); editCount++; continue; } @@ -73,19 +107,26 @@ export function applySearchReplaceEdits(originalContent: string, editsText: stri continue; } + // This scan compares whole lines, so it is line-anchored by construction — the fallback + // cannot reintroduce the mid-line match the fast path now rejects. const lines = content.split('\n'); const searchLines = searchText.split('\n'); const normLines = lines.map(l => l.trimEnd()); const normSearchLines = searchLines.map(l => l.trimEnd()); + const normSearchJoined = normSearchLines.join('\n'); let applied = false; + let offset = 0; for (let i = 0; i <= normLines.length - normSearchLines.length; i++) { - if (normLines.slice(i, i + normSearchLines.length).join('\n') === normSearchLines.join('\n')) { + if (normLines.slice(i, i + normSearchLines.length).join('\n') === normSearchJoined) { + // Splice at the offset we already know, for the same reason as the fast path: replace() + // would re-scan from position 0 and could hit an earlier, unanchored occurrence. const originalText = lines.slice(i, i + searchLines.length).join('\n'); - content = content.replace(originalText, replaceText); + content = content.slice(0, offset) + replaceText + content.slice(offset + originalText.length); editCount++; applied = true; break; } + offset += lines[i].length + 1; // +1 for the '\n' consumed by split } // 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. diff --git a/tests/search-replace-edit.test.ts b/tests/search-replace-edit.test.ts index 68945fa..5c62fbb 100644 --- a/tests/search-replace-edit.test.ts +++ b/tests/search-replace-edit.test.ts @@ -71,16 +71,45 @@ describe('후행 공백 폴백 — 모델이 흔히 틀리는 지점', () => { assert.equal(r.content, 'head \nTARGET \ntail \n'); }); - test('⚠ 알려진 날카로운 모서리: SEARCH가 더 얕게 들여쓰기돼도 부분 문자열로 매칭된다', () => { - // 8칸 들여쓴 줄 안에 4칸 들여쓴 SEARCH가 부분 문자열로 들어있어 그대로 치환된다. - // 결과는 원래 들여쓰기가 유지되므로(4칸 + 치환문) 파이썬에서는 대개 무해하지만, - // "정확히 이 줄"을 노린 편집이 다른 줄에 적용될 수 있는 경로이기도 하다. - // 이는 추출 이전부터 있던 동작이며, 줄 단위 앵커로 바꾸는 것은 파일 편집 의미를 - // 바꾸는 일이라 별도 판단이 필요하다. 지금은 동작을 문서화만 해둔다. + test('들여쓰기가 다르면 매칭을 거부한다 — fail closed', () => { + // 이전에는 4칸 SEARCH가 8칸 줄 안에서 부분 매칭돼 엉뚱한 줄을 고칠 수 있었다. + // 이제는 선행 공백이 있으면 줄 시작 정렬을 요구하므로 거부되고, 모델이 더 정확한 + // 컨텍스트로 재시도하게 된다. 조용히 잘못 고치는 것보다 실패가 낫다. const src = 'if x:\n deep()\n'; const r = applySearchReplaceEdits(src, block(' deep()', ' shallow()')); + assert.equal(r.editCount, 0); + assert.equal(r.content, src, '거부됐는데 파일이 변경됨'); + assert.deepEqual(r.failedEdits, [' deep()']); + }); +}); + +describe('줄 앵커 규칙 — SEARCH가 무엇을 주장하는지에 따라 달라진다', () => { + test('선행 공백 없는 한 줄은 부분 매칭 허용 — 들여쓰기를 주장하지 않았다', () => { + const r = applySearchReplaceEdits('def f():\n return 1\n', block('return 1', 'return 42')); assert.equal(r.editCount, 1); - assert.equal(r.content, 'if x:\n shallow()\n'); + assert.equal(r.content, 'def f():\n return 42\n', '들여쓰기가 보존돼야 한다'); + }); + + test('선행 공백이 있으면 줄 시작에 정렬돼야 한다', () => { + const r = applySearchReplaceEdits('x = 1\n y = 2\n', block(' y = 2', ' y = 3')); + assert.equal(r.editCount, 1, '정확히 정렬된 경우는 통과해야 한다'); + assert.equal(r.content, 'x = 1\n y = 3\n'); + }); + + test('여러 줄 SEARCH는 선행 공백이 없어도 줄 시작을 요구한다', () => { + // 줄 중간에서 시작하는 다중 줄 매칭은 구조를 깨뜨린다. + const src = 'prefix a = 1\nb = 2\n'; + const r = applySearchReplaceEdits(src, block('a = 1\nb = 2', 'REPLACED')); + assert.equal(r.editCount, 0); + assert.equal(r.content, src); + }); + + test('앵커 실패 후 뒤쪽의 올바른 위치를 계속 찾는다', () => { + // 첫 등장이 줄 중간이어도 포기하지 않고, 제대로 정렬된 다음 등장을 찾아야 한다. + const src = 'wrapper target()\n target()\n'; + const r = applySearchReplaceEdits(src, block(' target()', ' fixed()')); + assert.equal(r.editCount, 1); + assert.equal(r.content, 'wrapper target()\n fixed()\n', '줄 시작에 정렬된 두 번째 등장을 고쳐야 한다'); }); });