nes: support patch-based model response format #2667
Conversation
ulugbekna
force-pushed
the
ulugbekna/yearling-dolphin
branch
2 times, most recently
from
031b0c0 to
f6ac31d
Compare
model uses 0-based line numbers, so we should convert to 1-based for LineRange
ulugbekna
force-pushed
the
ulugbekna/yearling-dolphin
branch
from
8b4cebc to
67e872e
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces support for a patch-based model response format in the Next Edit System (NES), adding a new way for AI models to communicate code changes using a simplified diff-like syntax. The implementation adds a new prompting strategy PatchBased that uses a custom patch format (<filename>:<line> followed by - and + prefixed lines) instead of the existing XML-tagged or code block formats.
Key Changes
- Introduces
PatchBasedprompting strategy andCustomDiffPatchresponse format with corresponding handler logic - Refactors the anonymous type for streamed edits into a named
StreamedEdittype for better code maintainability - Adds comprehensive test coverage for patch parsing including edge cases with invalid headers and different line combinations
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/platform/inlineEdits/common/statelessNextEditProvider.ts |
Extracts inline type into named StreamedEdit type to improve code clarity and reusability |
src/platform/inlineEdits/common/dataTypes/xtabPromptOptions.ts |
Adds PatchBased prompting strategy enum value and CustomDiffPatch response format with mapping logic |
src/extension/xtab/test/node/xtabCustomDiffPatchResponseHandler.spec.ts |
Provides test coverage for patch parsing logic including simple patches, multiple patches, and various invalid header scenarios |
src/extension/xtab/node/xtabProvider.ts |
Integrates the new custom diff patch response handler into the provider's response processing pipeline and updates prediction logic |
src/extension/xtab/node/xtabCustomDiffPatchResponseHandler.ts |
Implements the core patch extraction and parsing logic including the Patch class and async generator for streaming patch processing |
src/extension/xtab/common/tags.ts |
Adds NO_EDIT response tag constant for indicating when no edits should be made |
src/extension/xtab/common/promptCrafting.ts |
Adds post-script instructions for the patch-based format explaining the expected output structure to the AI model |
| `); | ||
| }); | ||
|
|
||
| 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 -", which suggests patches without removed lines should be invalid. Either the test expectations should show an empty result (if patches should be discarded), or the test name should be updated to reflect that such patches are accepted.
| `); | ||
| }); | ||
|
|
||
| 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)'.
| postScript = `<|aggressive|>${aggressivenessLevel}<|/aggressive|>`; | ||
| break; | ||
| case PromptingStrategy.PatchBased: | ||
| postScript = `Output a modified diff style format with the changes you want. Each change patch must start with \`<filename>:<line number>\` and then include some non empty "anchor lines" preceded by \`-\` and the new lines meant to replace them preceded by \`+\`. Put your changes in the order that makes the most sense, for example edits inside the code_to_edit region and near the user's <|cursor|> should always be prioritized. Output "<NO_EDIT>" if you don't have a good edit candidate.`; |
There was a problem hiding this comment.
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.
| 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').
| return patches.map(p => p.toString()).join('\n'); | |
| return patches.join('\n'); |
| -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.
ulugbekna
force-pushed
the
ulugbekna/yearling-dolphin
branch
from
bd4e217 to
5057a7e
Compare
No description provided.