Skip to content
This repository was archived by the owner on May 20, 2026. It is now read-only.

Commit 4a7122e

Browse files
jriekenbenibenjkieferrmCopilot
authored
Fix applyPatch-tool emitting incorrect edits (#1495)
* Hello Copilot (#1493) Co-authored-by: kieferrm <4674940+kieferrm@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> * Fix applyPatch-tool emitting incorrect edits Started to use applyPatch in stests (`joh/slow-meerkat`) which resulted in corrected files because of bogous edits. The culprit is that the applyPatch-tool computes multiple text edits and then dispatches them to the stream via individual calls. The semantics of that is that call `N` knows new offsets after applying edits from the `N-1` call. Tho, the calls aren't made like that by the tool. Instead, it should dispatch its edit in a single call * fix windows tests --------- Co-authored-by: Benjamin Christopher Simmonds <44439583+benibenj@users.noreply.github.com> Co-authored-by: kieferrm <4674940+kieferrm@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent c70ec03 commit 4a7122e

4 files changed

Lines changed: 578 additions & 6 deletions

File tree

‎src/extension/tools/node/applyPatchTool.tsx‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -342,12 +342,8 @@ export class ApplyPatchTool implements ICopilotTool<IApplyPatchToolParams> {
342342
} else {
343343
this._promptContext.stream.markdown('\n```\n');
344344
this._promptContext.stream.codeblockUri(notebookUri || uri, true);
345-
// TODO@joyceerhl hack: when an array of text edits for a single URI
346-
// are pushed in a single textEdit call, the edits are not applied
347-
const edits = Array.isArray(textEdit) ? textEdit : [textEdit];
348-
for (const textEdit of edits) {
349-
responseStream.textEdit(uri, textEdit);
350-
}
345+
346+
responseStream.textEdit(uri, textEdit);
351347
responseStream.textEdit(uri, true);
352348
this._promptContext.stream.markdown('\n' + '```\n');
353349
}
Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import { readFileSync } from 'fs';
7+
import { join } from 'path';
8+
import { beforeEach, expect, it, suite } from 'vitest';
9+
import { ITestingServicesAccessor } from '../../../../../platform/test/node/services';
10+
import { TestWorkspaceService } from '../../../../../platform/test/node/testWorkspaceService';
11+
import { IWorkspaceService } from '../../../../../platform/workspace/common/workspaceService';
12+
import { ChatResponseStreamImpl } from '../../../../../util/common/chatResponseStreamImpl';
13+
import { createTextDocumentData } from '../../../../../util/common/test/shims/textDocument';
14+
import { CancellationToken } from '../../../../../util/vs/base/common/cancellation';
15+
import { assertType } from '../../../../../util/vs/base/common/types';
16+
import { URI } from '../../../../../util/vs/base/common/uri';
17+
import { SyncDescriptor } from '../../../../../util/vs/platform/instantiation/common/descriptors';
18+
import { IInstantiationService } from '../../../../../util/vs/platform/instantiation/common/instantiation';
19+
import { ChatResponseTextEditPart } from '../../../../../vscodeTypes';
20+
import { ChatVariablesCollection } from '../../../../prompt/common/chatVariablesCollection';
21+
import { WorkingCopyOriginalDocument } from '../../../../prompts/node/inline/workingCopies';
22+
import { createExtensionUnitTestingServices } from '../../../../test/node/services';
23+
import { ApplyPatchTool, IApplyPatchToolParams } from '../../../node/applyPatchTool';
24+
25+
26+
suite('ApplyPatch Tool', () => {
27+
28+
let accessor: ITestingServicesAccessor;
29+
30+
const path = join(__dirname, 'fixtures/4302.ts.txt');
31+
const fileTsUri = URI.file(path);
32+
33+
beforeEach(function () {
34+
const services = createExtensionUnitTestingServices();
35+
36+
const content = String(readFileSync(path));
37+
38+
const testDoc = createTextDocumentData(fileTsUri, content, 'ts').document;
39+
services.define(IWorkspaceService, new SyncDescriptor(
40+
TestWorkspaceService, [[fileTsUri], [testDoc]]
41+
));
42+
43+
accessor = services.createTestingAccessor();
44+
});
45+
46+
it('makes changes atomically', async () => {
47+
48+
const input: IApplyPatchToolParams = JSON.parse(`{
49+
"explanation": "Condense the offSide language array and includes check into a single line.",
50+
"input": "*** Begin Patch\\n*** Update File: ${path.replaceAll('\\', '\\\\')}\\n@@\\n-\\tconst offSide = [\\n-\\t\\t'clojure',\\n-\\t\\t'coffeescript',\\n-\\t\\t'fsharp',\\n-\\t\\t'latex',\\n-\\t\\t'markdown',\\n-\\t\\t'pug',\\n-\\t\\t'python',\\n-\\t\\t'sql',\\n-\\t\\t'yaml',\\n-\\t].includes(languageId.toLowerCase());\\n+\\tconst offSide = ['clojure','coffeescript','fsharp','latex','markdown','pug','python','sql','yaml'].includes(languageId.toLowerCase());\\n*** End Patch\\n"
51+
}`);
52+
53+
const tool = accessor.get(IInstantiationService).createInstance(ApplyPatchTool);
54+
55+
expect(tool).toBeDefined();
56+
57+
const document = accessor.get(IWorkspaceService).textDocuments.find(doc => doc.uri.toString() === fileTsUri.toString());
58+
assertType(document);
59+
60+
const workingCopyDocument = new WorkingCopyOriginalDocument(document.getText());
61+
62+
let seenEdits = 0;
63+
64+
const stream = new ChatResponseStreamImpl((part) => {
65+
66+
if (part instanceof ChatResponseTextEditPart) {
67+
const offsetEdits = workingCopyDocument.transformer.toOffsetEdit(part.edits);
68+
69+
if (!workingCopyDocument.isNoop(offsetEdits)) {
70+
seenEdits++;
71+
workingCopyDocument.applyOffsetEdits(offsetEdits);
72+
}
73+
}
74+
75+
}, () => { }, () => { });
76+
77+
const input2 = await tool.resolveInput(input, {
78+
history: [],
79+
stream,
80+
query: 'put it all in one line',
81+
chatVariables: new ChatVariablesCollection([]),
82+
});
83+
84+
await tool.invoke({ input: input2, toolInvocationToken: undefined }, CancellationToken.None);
85+
86+
expect(seenEdits).toBe(1);
87+
expect(workingCopyDocument.text).toMatchFileSnapshot('fixtures/4302.ts.txt.expected');
88+
89+
});
90+
});
Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,248 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation and GitHub. All rights reserved.
3+
*--------------------------------------------------------------------------------------------*/
4+
import { CharCode } from '../../../util/vs/common/charCode';
5+
6+
export interface ISimpleTextModel {
7+
getLineCount(): number;
8+
getLineContent(lineNumber: number): string;
9+
getOptions(): { tabSize: number };
10+
}
11+
12+
export function computeRanges(model: ISimpleTextModel, languageId: string): FoldingRegions {
13+
const offSide = [
14+
'clojure',
15+
'coffeescript',
16+
'fsharp',
17+
'latex',
18+
'markdown',
19+
'pug',
20+
'python',
21+
'sql',
22+
'yaml',
23+
].includes(languageId.toLowerCase());
24+
return _computeRanges(model, offSide);
25+
}
26+
27+
function _computeRanges(model: ISimpleTextModel, offSide: boolean): FoldingRegions {
28+
const tabSize = model.getOptions().tabSize;
29+
const result = new RangesCollector();
30+
const previousRegions: PreviousRegion[] = [];
31+
const line = model.getLineCount() + 1;
32+
previousRegions.push({ indent: -1, endAbove: line, line }); // sentinel, to make sure there's at least one entry
33+
for (let line = model.getLineCount(); line > 0; line--) {
34+
const lineContent = model.getLineContent(line);
35+
const indent = computeIndentLevel(lineContent, tabSize);
36+
let previous = previousRegions[previousRegions.length - 1];
37+
if (indent === -1) {
38+
if (offSide) {
39+
// for offSide languages, empty lines are associated to the previous block
40+
// note: the next block is already written to the results, so this only
41+
// impacts the end position of the block before
42+
previous.endAbove = line;
43+
}
44+
continue; // only whitespace
45+
}
46+
if (previous.indent > indent) {
47+
// discard all regions with larger indent
48+
do {
49+
previousRegions.pop();
50+
previous = previousRegions[previousRegions.length - 1];
51+
} while (previous.indent > indent);
52+
// new folding range
53+
const endLineNumber = previous.endAbove - 1;
54+
if (endLineNumber - line >= 1) { // needs at east size 1
55+
result.insertFirst(line, endLineNumber, indent);
56+
}
57+
}
58+
if (previous.indent === indent) {
59+
previous.endAbove = line;
60+
} else { // previous.indent < indent
61+
// new region with a bigger indent
62+
previousRegions.push({ indent, endAbove: line, line });
63+
}
64+
}
65+
return result.toIndentRanges();
66+
}
67+
68+
interface PreviousRegion {
69+
indent: number; // indent or -2 if a marker
70+
endAbove: number; // end line number for the region above
71+
line: number; // start line of the region. Only used for marker regions.
72+
}
73+
74+
const MAX_FOLDING_REGIONS = 0xFFFF;
75+
const MAX_LINE_NUMBER = 0xFFFFFF;
76+
const MASK_INDENT = 0xFF000000;
77+
78+
class RangesCollector {
79+
private readonly _startIndexes: number[];
80+
private readonly _endIndexes: number[];
81+
private readonly _indentOccurrences: number[];
82+
private _length: number;
83+
84+
constructor() {
85+
this._startIndexes = [];
86+
this._endIndexes = [];
87+
this._indentOccurrences = [];
88+
this._length = 0;
89+
}
90+
public insertFirst(startLineNumber: number, endLineNumber: number, indent: number) {
91+
if (startLineNumber > MAX_LINE_NUMBER || endLineNumber > MAX_LINE_NUMBER) {
92+
return;
93+
}
94+
const index = this._length;
95+
this._startIndexes[index] = startLineNumber;
96+
this._endIndexes[index] = endLineNumber;
97+
this._length++;
98+
if (indent < 1000) {
99+
this._indentOccurrences[indent] = (this._indentOccurrences[indent] || 0) + 1;
100+
}
101+
}
102+
public toIndentRanges() {
103+
// reverse and create arrays of the exact length
104+
const startIndexes = new Uint32Array(this._length);
105+
const endIndexes = new Uint32Array(this._length);
106+
for (let i = this._length - 1, k = 0; i >= 0; i--, k++) {
107+
startIndexes[k] = this._startIndexes[i];
108+
endIndexes[k] = this._endIndexes[i];
109+
}
110+
return new FoldingRegions(startIndexes, endIndexes);
111+
}
112+
}
113+
114+
/**
115+
* Returns:
116+
* - -1 => the line consists of whitespace
117+
* - otherwise => the indent level is returned value
118+
*/
119+
export function computeIndentLevel(line: string, tabSize: number): number {
120+
let indent = 0;
121+
let i = 0;
122+
const len = line.length;
123+
while (i < len) {
124+
const chCode = line.charCodeAt(i);
125+
if (chCode === CharCode.Space) {
126+
indent++;
127+
} else if (chCode === CharCode.Tab) {
128+
indent = indent - indent % tabSize + tabSize;
129+
} else {
130+
break;
131+
}
132+
i++;
133+
}
134+
135+
if (i === len) {
136+
return -1; // line only consists of whitespace
137+
}
138+
return indent;
139+
}
140+
141+
export class FoldingRegions {
142+
private readonly _startIndexes: Uint32Array;
143+
private readonly _endIndexes: Uint32Array;
144+
145+
private _parentsComputed: boolean;
146+
147+
constructor(startIndexes: Uint32Array, endIndexes: Uint32Array) {
148+
this._startIndexes = startIndexes;
149+
this._endIndexes = endIndexes;
150+
this._parentsComputed = false;
151+
this.ensureParentIndices();
152+
}
153+
private ensureParentIndices() {
154+
if (!this._parentsComputed) {
155+
this._parentsComputed = true;
156+
const parentIndexes: number[] = [];
157+
const isInsideLast = (startLineNumber: number, endLineNumber: number) => {
158+
const index = parentIndexes[parentIndexes.length - 1];
159+
return this.getStartLineNumber(index) <= startLineNumber && this.getEndLineNumber(index) >= endLineNumber;
160+
};
161+
for (let i = 0, len = this._startIndexes.length; i < len; i++) {
162+
const startLineNumber = this._startIndexes[i];
163+
const endLineNumber = this._endIndexes[i];
164+
if (startLineNumber > MAX_LINE_NUMBER || endLineNumber > MAX_LINE_NUMBER) {
165+
throw new Error('startLineNumber or endLineNumber must not exceed ' + MAX_LINE_NUMBER);
166+
}
167+
while (parentIndexes.length > 0 && !isInsideLast(startLineNumber, endLineNumber)) {
168+
parentIndexes.pop();
169+
}
170+
const parentIndex = parentIndexes.length > 0 ? parentIndexes[parentIndexes.length - 1] : -1;
171+
parentIndexes.push(i);
172+
this._startIndexes[i] = startLineNumber + ((parentIndex & 0xFF) << 24);
173+
this._endIndexes[i] = endLineNumber + ((parentIndex & 0xFF00) << 16);
174+
}
175+
}
176+
}
177+
public get length(): number {
178+
return this._startIndexes.length;
179+
}
180+
public getStartLineNumber(index: number): number {
181+
return this._startIndexes[index] & MAX_LINE_NUMBER;
182+
}
183+
public getEndLineNumber(index: number): number {
184+
return this._endIndexes[index] & MAX_LINE_NUMBER;
185+
}
186+
// public toRegion(index: number): FoldingRegion {
187+
// return new FoldingRegion(this, index);
188+
// }
189+
public getParentIndex(index: number) {
190+
this.ensureParentIndices();
191+
const parent = ((this._startIndexes[index] & MASK_INDENT) >>> 24) + ((this._endIndexes[index] & MASK_INDENT) >>> 16);
192+
if (parent === MAX_FOLDING_REGIONS) {
193+
return -1;
194+
}
195+
return parent;
196+
}
197+
public contains(index: number, line: number) {
198+
return this.getStartLineNumber(index) <= line && this.getEndLineNumber(index) >= line;
199+
}
200+
private findIndex(line: number) {
201+
let low = 0, high = this._startIndexes.length;
202+
if (high === 0) {
203+
return -1; // no children
204+
}
205+
while (low < high) {
206+
const mid = Math.floor((low + high) / 2);
207+
if (line < this.getStartLineNumber(mid)) {
208+
high = mid;
209+
} else {
210+
low = mid + 1;
211+
}
212+
}
213+
return low - 1;
214+
}
215+
public findRange(line: number): number {
216+
let index = this.findIndex(line);
217+
if (index >= 0) {
218+
const endLineNumber = this.getEndLineNumber(index);
219+
if (endLineNumber >= line) {
220+
return index;
221+
}
222+
index = this.getParentIndex(index);
223+
while (index !== -1) {
224+
if (this.contains(index, line)) {
225+
return index;
226+
}
227+
index = this.getParentIndex(index);
228+
}
229+
}
230+
return -1;
231+
}
232+
// public toString() {
233+
// const res: string[] = [];
234+
// for (let i = 0; i < this.length; i++) {
235+
// res[i] = `[${foldSourceAbbr[this.getSource(i)]}${this.isCollapsed(i) ? '+' : '-'}] ${this.getStartLineNumber(i)}/${this.getEndLineNumber(i)}`;
236+
// }
237+
// return res.join(', ');
238+
// }
239+
// public toFoldRange(index: number): FoldRange {
240+
// return <FoldRange>{
241+
// startLineNumber: this._startIndexes[index] & MAX_LINE_NUMBER,
242+
// endLineNumber: this._endIndexes[index] & MAX_LINE_NUMBER,
243+
// type: this._types ? this._types[index] : undefined,
244+
// isCollapsed: this.isCollapsed(index),
245+
// source: this.getSource(index)
246+
// };
247+
// }
248+
}

0 commit comments

Comments
 (0)