nes: support patch-based model response format #2667
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| import { NoNextEditReason, PushEdit, StreamedEdit } from '../../../platform/inlineEdits/common/statelessNextEditProvider'; | ||
| import { Result } from '../../../util/common/result'; | ||
| import { AsyncIterableObject } from '../../../util/vs/base/common/async'; | ||
| import { LineReplacement } from '../../../util/vs/editor/common/core/edits/lineEdit'; | ||
| import { LineRange } from '../../../util/vs/editor/common/core/ranges/lineRange'; | ||
| import { OffsetRange } from '../../../util/vs/editor/common/core/ranges/offsetRange'; | ||
| import { StringText } from '../../../util/vs/editor/common/core/text/abstractText'; | ||
| import { ResponseTags } from '../common/tags'; | ||
|
|
||
|
|
||
| class Patch { | ||
| public removedLines: string[] = []; | ||
| public addedLines: string[] = []; | ||
|
|
||
| private constructor( | ||
| public readonly filename: string, | ||
| public readonly lineNumZeroBased: number, | ||
| ) { } | ||
|
|
||
| public static ofLine(line: string): Patch | null { | ||
| const match = line.match(/^(.+):(\d+)$/); | ||
| if (!match) { | ||
| return null; | ||
| } | ||
| const [, filename, lineNumber] = match; | ||
| return new Patch(filename, parseInt(lineNumber, 10)); | ||
| } | ||
|
|
||
| addLine(line: string) { | ||
| const contentLine = line.slice(1); | ||
| if (line.startsWith('-')) { | ||
| this.removedLines.push(contentLine); | ||
| return true; | ||
| } else if (line.startsWith('+')) { | ||
| this.addedLines.push(contentLine); | ||
| return true; | ||
| } else { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| public toString(): string { | ||
| return [ | ||
| `${this.filename}:${this.lineNumZeroBased}`, | ||
| ...this.removedLines.map(l => `-${l}`), | ||
| ...this.addedLines.map(l => `+${l}`), | ||
| ].join('\n'); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| export class XtabCustomDiffPatchResponseHandler { | ||
|
|
||
| public static async handleResponse( | ||
| pushEdit: PushEdit, | ||
| linesStream: AsyncIterableObject<string>, | ||
| documentBeforeEdits: StringText, | ||
| window: OffsetRange | undefined, | ||
| ): Promise<void> { | ||
| let editCount = 0; | ||
| for await (const edit of XtabCustomDiffPatchResponseHandler.extractEdits(linesStream)) { | ||
| editCount++; | ||
| pushEdit(Result.ok({ | ||
| edit: XtabCustomDiffPatchResponseHandler.resolveEdit(edit), | ||
| window, | ||
| // targetDocument, // TODO@ulugbekna: implement target document resolution | ||
| } satisfies StreamedEdit)); | ||
| } | ||
| if (editCount === 0) { | ||
| pushEdit(Result.error(new NoNextEditReason.NoSuggestions(documentBeforeEdits, window, undefined))); | ||
| } | ||
| } | ||
|
|
||
| private static resolveEdit(patch: Patch): LineReplacement { | ||
| return new LineReplacement(new LineRange(patch.lineNumZeroBased + 1, patch.lineNumZeroBased + 1 + patch.removedLines.length), patch.addedLines); | ||
| } | ||
|
|
||
| public static async *extractEdits(linesStream: AsyncIterableObject<string>): AsyncGenerator<Patch> { | ||
| let currentPatch: Patch | null = null; | ||
| for await (const line of linesStream) { | ||
| // if no current patch, try to parse a new one | ||
| if (line.trim() === ResponseTags.NO_EDIT) { | ||
| break; | ||
| } | ||
| if (currentPatch === null) { | ||
| currentPatch = Patch.ofLine(line); | ||
| continue; | ||
| } | ||
| // try to add line to current patch | ||
| if (currentPatch.addLine(line)) { | ||
| continue; | ||
| } else { // line does not belong to current patch, yield current and start new | ||
| if (currentPatch) { | ||
| yield currentPatch; | ||
| } | ||
| currentPatch = Patch.ofLine(line); | ||
| } | ||
| } | ||
| if (currentPatch) { | ||
| yield currentPatch; | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,96 @@ | ||||||
| /*--------------------------------------------------------------------------------------------- | ||||||
| * Copyright (c) Microsoft Corporation. All rights reserved. | ||||||
| * Licensed under the MIT License. See License.txt in the project root for license information. | ||||||
| *--------------------------------------------------------------------------------------------*/ | ||||||
|
|
||||||
| import { describe, expect, it } from 'vitest'; | ||||||
| import { AsyncIterableObject } from '../../../../util/vs/base/common/async'; | ||||||
| import { XtabCustomDiffPatchResponseHandler } from '../../node/xtabCustomDiffPatchResponseHandler'; | ||||||
|
|
||||||
| describe('XtabCustomDiffPatchResponseHandler', () => { | ||||||
|
|
||||||
| async function collectPatches(patchText: string): Promise<string> { | ||||||
| const linesStream = AsyncIterableObject.fromArray(patchText.split('\n')); | ||||||
| const patches: string[] = []; | ||||||
| for await (const patch of XtabCustomDiffPatchResponseHandler.extractEdits(linesStream)) { | ||||||
| patches.push(patch.toString()); | ||||||
| } | ||||||
| return patches.map(p => p.toString()).join('\n'); | ||||||
|
There was a problem hiding this comment. Redundant call to toString(). The patches array already contains strings (from patch.toString() on line 16), so calling .map(p => p.toString()) again on line 18 is unnecessary. Remove the redundant .map() call and simply use patches.join('\n').
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| it('should parse a simple patch correctly', async () => { | ||||||
| const patchText = `file1.txt:10 | ||||||
| -Old line 1 | ||||||
| -Old line 2 | ||||||
| +New line 1 | ||||||
| +New line 2`; | ||||||
| const patches = await collectPatches(patchText); | ||||||
| expect(patches).toEqual(patchText); | ||||||
| }); | ||||||
|
|
||||||
| it('should parse a simple patch correctly', async () => { | ||||||
| const patchText = `/absolutePath/to/my_file.ts:1 | ||||||
| -Old line 1 | ||||||
| +New line 1 | ||||||
| +New line 2 | ||||||
| relative/path/to/another_file.js:42 | ||||||
| -Removed line | ||||||
| +Added line`; | ||||||
| const patches = await collectPatches(patchText); | ||||||
| expect(patches).toEqual(patchText); | ||||||
| }); | ||||||
|
|
||||||
| it('discard a patch if no valid header', async () => { | ||||||
| const patchText = `myFile.ts: | ||||||
| +New line 1 | ||||||
| +New line 2 | ||||||
| another_file.js:32 | ||||||
| -Removed line | ||||||
| +Added line`; | ||||||
| const patches = await collectPatches(patchText); | ||||||
| expect(patches).toMatchInlineSnapshot(` | ||||||
| "another_file.js:32 | ||||||
| -Removed line | ||||||
| +Added line" | ||||||
| `); | ||||||
| }); | ||||||
|
|
||||||
| it('discard a patch if no valid header - 2', async () => { | ||||||
| const patchText = `myFile.ts:42 | ||||||
| +New line 1 | ||||||
| +New line 2 | ||||||
| another_file.js: | ||||||
| -Removed line | ||||||
| +Added line`; | ||||||
| const patches = await collectPatches(patchText); | ||||||
| expect(patches).toMatchInlineSnapshot(` | ||||||
| "myFile.ts:42 | ||||||
| +New line 1 | ||||||
| +New line 2" | ||||||
| `); | ||||||
| }); | ||||||
|
|
||||||
| it('discard a patch has no removed lines', async () => { | ||||||
|
There was a problem hiding this comment. Test name is misleading and the behavior may not match the prompt description. The test name says "discard a patch has no removed lines" but the expectation shows the patch is NOT discarded. According to the prompt description in promptCrafting.ts line 133, patches must include "some non empty 'anchor lines' preceded by |
||||||
| const patchText = `myFile.ts:42 | ||||||
| +New line 1 | ||||||
| +New line 2`; | ||||||
| const patches = await collectPatches(patchText); | ||||||
| expect(patches).toMatchInlineSnapshot(` | ||||||
| "myFile.ts:42 | ||||||
| +New line 1 | ||||||
| +New line 2" | ||||||
| `); | ||||||
| }); | ||||||
|
|
||||||
| it('discard a patch has no new lines', async () => { | ||||||
|
There was a problem hiding this comment. Test name is misleading. The test name says "discard a patch has no new lines" but the expectation shows the patch is NOT discarded - it's included in the output. Consider renaming to something like 'should parse patch with only removed lines (deletions)'. |
||||||
| const patchText = `myFile.ts:42 | ||||||
| -Old line 1 | ||||||
| -Old line 2`; | ||||||
| const patches = await collectPatches(patchText); | ||||||
| expect(patches).toMatchInlineSnapshot(` | ||||||
| "myFile.ts:42 | ||||||
| -Old line 1 | ||||||
| -Old line 2" | ||||||
| `); | ||||||
| }); | ||||||
| }); | ||||||
|
There was a problem hiding this comment. Missing test coverage for the NO_EDIT tag scenario. The implementation on line 87 handles the case where the model outputs "<NO_EDIT>", but there's no test verifying this behavior. Consider adding a test case that verifies the handler correctly stops processing and returns no edits when it encounters the NO_EDIT tag. |
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prompt description states that each patch "must...include some non empty 'anchor lines' preceded by
-", which implies that patches without removed lines should be invalid. However, the implementation in xtabCustomDiffPatchResponseHandler.ts does not validate that patches have removed lines. Consider adding validation to ensure patches conform to the format described in the prompt, or update the prompt description if patches with only additions are intentionally supported.