nes: implement cursor or whole edit window early cancellation of async request due to divergence#309871
nes: implement cursor or whole edit window early cancellation of async request due to divergence#309871
Conversation
…c request due to divergence
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements configurable early cancellation of async xtab inline-edit requests when the user’s intermediate edits diverge from the model’s streamed output, expanding the check from cursor-line-only to optionally cover the whole edit window.
Changes:
- Introduces
EarlyDivergenceCancellationMode(Off/Cursor/EditWindow) and wires it into the team-internal config. - Updates
XtabProviderstreaming pipeline to cancel the fetch early on detected divergence, with backward compatibility for legacy boolean config values. - Adds extensive integration and unit tests covering cursor vs edit-window behavior and compatibility edge cases.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/platform/inlineEdits/common/dataTypes/xtabPromptOptions.ts | Adds an enum to represent divergence-cancellation scope modes. |
| extensions/copilot/src/platform/configuration/common/configurationService.ts | Changes config type from boolean to the new enum and updates defaults. |
| extensions/copilot/src/extension/xtab/node/xtabProvider.ts | Implements mode-driven divergence checking and backward-compatible config parsing. |
| extensions/copilot/src/extension/xtab/node/cursorLineDivergence.ts | Generalizes cursor-line helpers to line-agnostic helpers. |
| extensions/copilot/src/extension/xtab/test/node/*.spec.ts | Updates existing tests and adds new coverage for modes + backward compat. |
Copilot's findings
Comments suppressed due to low confidence (2)
extensions/copilot/src/extension/xtab/test/node/cursorLineDivergence.spec.ts:16
- The test suite descriptions still reference the old function names (
isModelCursorLineCompatible,getCurrentCursorLine) even though the code now testsisModelLineCompatible/getCurrentLine. Updating thesedescribe(...)strings will make failures and test reports easier to understand.
describe('isModelCursorLineCompatible', () => {
extensions/copilot/src/extension/xtab/test/node/cursorLineDivergence.spec.ts:765
- The test suite descriptions still reference the old function names (
isModelCursorLineCompatible,getCurrentCursorLine) even though the code now testsisModelLineCompatible/getCurrentLine. Updating thesedescribe(...)strings will make failures and test reports easier to understand.
describe('getCurrentCursorLine', () => {
- Files reviewed: 6/6 changed files
- Comments generated: 6
| if (shouldCheckLine(lineIdx)) { | ||
| const docLineIdx = editWindowLineRange.start + lineIdx; | ||
| const currentLine = getCurrentLine(transformer, docLineIdx, intermediateEdit); | ||
| if (currentLine !== undefined) { | ||
| const originalLine = editWindowLines[lineIdx]; | ||
| if (currentLine !== originalLine // user changed this line | ||
| && !isModelLineCompatible(originalLine, currentLine, line) // model's line isn't compatible with user's typing | ||
| ) { |
There was a problem hiding this comment.
editWindowLines[lineIdx]can beundefinedwhen the model streams more lines thaneditWindowLines.length(which is plausible for insertions / longer completions). That will flow intoisModelLineCompatible(originalLine, ...)and can throw (sinceoriginalLineis expected to be a string). Fix by bounding the divergence check to the edit window (e.g.,if (lineIdx >= editWindowLines.length) { /* skip check */ }or incorporate the bound intoshouldCheckLineforEditWindow` mode).
| const mappedOffset = lineStartOffset + delta; | ||
| const currentDoc = intermediateEdit.apply(originalDoc.text); | ||
| const currentTransformer = new PositionOffsetTransformer(currentDoc); |
There was a problem hiding this comment.
getCurrentLine()applies the fullintermediateEditto the entire document and constructs a newPositionOffsetTransformeron every call. InEditWindowmode this can happen once per streamed line, which is potentially expensive (O(lines * docSize)). Consider precomputingcurrentDoc+currentTransformeronce inlinesWithIntermediateEditDivergenceCheckand passing them into a helper, or refactoringgetCurrentLine` to accept a pre-applied doc/transformer.
| export const InlineEditsXtabNNonSignificantLinesToConverge = defineTeamInternalSetting<number>('chat.advanced.inlineEdits.xtabProvider.nNonSignificantLinesToConverge', ConfigType.ExperimentBased, ResponseProcessor.DEFAULT_DIFF_PARAMS.nLinesToConverge); | ||
| export const InlineEditsXtabNSignificantLinesToConverge = defineTeamInternalSetting<number>('chat.advanced.inlineEdits.xtabProvider.nSignificantLinesToConverge', ConfigType.ExperimentBased, ResponseProcessor.DEFAULT_DIFF_PARAMS.nSignificantLinesToConverge); | ||
| export const InlineEditsXtabEarlyCursorLineDivergenceCancellation = defineTeamInternalSetting<boolean>('chat.advanced.inlineEdits.xtabProvider.earlyCursorLineDivergenceCancellation', ConfigType.ExperimentBased, false); | ||
| export const InlineEditsXtabEarlyCursorLineDivergenceCancellation = defineTeamInternalSetting<EarlyDivergenceCancellationMode>('chat.advanced.inlineEdits.xtabProvider.earlyCursorLineDivergenceCancellation', ConfigType.ExperimentBased, EarlyDivergenceCancellationMode.Off); |
There was a problem hiding this comment.
This setting changed from boolean to an enum-like string, but there is no validator ensuring only Off | Cursor | EditWindow are accepted. Adding a vEnum(...) validator (similar to other settings in this file) would prevent invalid string values from silently disabling checks or selecting unintended behavior at runtime.
| // const earlyDivergenceCancellationRaw = | ||
| const earlyDivergenceMode = backwardCompatSetting<boolean | undefined, EarlyDivergenceCancellationMode>( | ||
| this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabEarlyCursorLineDivergenceCancellation, this.expService), | ||
| (value) => (value === false || value === undefined) ? EarlyDivergenceCancellationMode.Off : (value === true ? EarlyDivergenceCancellationMode.Cursor : value) |
There was a problem hiding this comment.
There’s a stray commented-out line (// const earlyDivergenceCancellationRaw =) that should be removed. Also, the backward-compat mapping returns value in the last branch; if the intent is to pass through modern enum values, consider typing the incoming value as a union (e.g., boolean | EarlyDivergenceCancellationMode | undefined) and explicitly handling unexpected strings to avoid silently accepting invalid modes.
| // const earlyDivergenceCancellationRaw = | |
| const earlyDivergenceMode = backwardCompatSetting<boolean | undefined, EarlyDivergenceCancellationMode>( | |
| this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabEarlyCursorLineDivergenceCancellation, this.expService), | |
| (value) => (value === false || value === undefined) ? EarlyDivergenceCancellationMode.Off : (value === true ? EarlyDivergenceCancellationMode.Cursor : value) | |
| const earlyDivergenceMode = backwardCompatSetting<boolean | EarlyDivergenceCancellationMode | undefined, EarlyDivergenceCancellationMode>( | |
| this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabEarlyCursorLineDivergenceCancellation, this.expService), | |
| (value) => { | |
| switch (value) { | |
| case false: | |
| case undefined: | |
| return EarlyDivergenceCancellationMode.Off; | |
| case true: | |
| return EarlyDivergenceCancellationMode.Cursor; | |
| case EarlyDivergenceCancellationMode.Off: | |
| case EarlyDivergenceCancellationMode.Cursor: | |
| case EarlyDivergenceCancellationMode.EditWindow: | |
| return value; | |
| default: | |
| return EarlyDivergenceCancellationMode.Off; | |
| } | |
| } |
| && !isModelLineCompatible(originalLine, currentLine, line) // model's line isn't compatible with user's typing | ||
| ) { | ||
| setLineDiverged(true); | ||
| tracer.trace(`Line ${lineIdx} DIVERGED (mode=${mode}): model="${line}" current="${currentLine}"`); |
There was a problem hiding this comment.
This trace message logs the full model line and the user’s current line content. With EditWindow mode it can log non-cursor lines as well, increasing the amount of potentially sensitive user code captured in traces. Consider truncating/ellipsizing the logged line content, logging only metadata (line index, lengths), or gating the full-text log behind a stricter debug flag.
| tracer.trace(`Line ${lineIdx} DIVERGED (mode=${mode}): model="${line}" current="${currentLine}"`); | |
| tracer.trace( | |
| `Line ${lineIdx} DIVERGED (mode=${mode}, docLine=${docLineIdx}, modelLength=${line.length}, currentLength=${currentLine.length}, originalLength=${originalLine.length})` | |
| ); |
| /** | ||
| * Controls the scope of the early divergence cancellation check. | ||
| * | ||
| * - `Cursor`: only check the cursor line for divergence (original behavior). | ||
| * - `EditWindow`: check every line in the edit window for divergence. | ||
| */ | ||
| export enum EarlyDivergenceCancellationMode { | ||
| Cursor = 'cursor', | ||
| EditWindow = 'editWindow', | ||
| Off = 'off', | ||
| } |
There was a problem hiding this comment.
The JSDoc omits the Off mode even though it’s part of the enum and is now the default in config. Please document Off explicitly (e.g., “disable early divergence cancellation checks”) to keep the API self-explanatory.
…num validator, use switch for backward compat
No description provided.