Add reverse-agreement rebase for NES cache behind experiment flag#310793
Add reverse-agreement rebase for NES cache behind experiment flag#310793
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an experiment-guarded “reverse agreement” path to the inline-edits rebase engine so cached/model edits can be rebased when the user types more than the model predicted at the same location, preserving and offering only the unconsumed remainder.
Changes:
- Add
reverseAgreementhandling totryRebaseEdits(guarded bynesConfigs.reverseAgreement) and propagate the config fromNextEditCache. - Introduce
chat.advanced.inlineEdits.reverseAgreementas an experiment-based TeamInternal setting (defaultfalse) and include it in rebase failure diagnostics. - Add unit + integration coverage for reverse-agreement scenarios (including a Fibonacci regression test).
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/inlineEdits/common/editRebase.ts | Implements reverse-agreement rebase branch behind config flag. |
| extensions/copilot/src/platform/configuration/common/configurationService.ts | Adds TeamInternal experiment-based config key for reverse agreement. |
| extensions/copilot/src/extension/inlineEdits/node/nextEditCache.ts | Wires reverse-agreement config into NesRebaseConfigs passed to rebase. |
| extensions/copilot/src/extension/inlineEdits/node/rebaseResult.ts | Includes reverseAgreement in auto-generated failure test output. |
| extensions/copilot/src/extension/inlineEdits/test/common/editRebase.spec.ts | Adds reverse-agreement unit tests (positive/negative/edge cases). |
| extensions/copilot/src/extension/inlineEdits/test/node/nextEditCacheRebase.spec.ts | Adds integration regression test for cache rebase with the flag enabled. |
| extensions/copilot/.esbuild.ts | Changes watch-mode rebuild strategy to use ctx.watch() per context. |
Copilot's findings
Comments suppressed due to low confidence (1)
extensions/copilot/src/extension/inlineEdits/test/node/nextEditCacheRebase.spec.ts:132
- The test hard-codes a user-specific absolute path (
/Users/ulugbekna/...). This is non-portable and can be problematic on Windows/CI. Use a generic test path (e.g./test/nodeTypesDefinitionProvider.ts) or construct a temp path instead.
// Initialize workspace doc with the CURRENT document state
// (so checkEditConsistency(documentBeforeEdit + userEditSince = currentDoc) passes)
- Files reviewed: 7/7 changed files
- Comments generated: 4
| // Document states at different points in time | ||
| const docAtRequest18 = docPrefix + 'class Fibonacci '; // offset 1944 → "class Fibonacci " ends at 1960 | ||
| const currentDoc = docPrefix + 'class Fibonacci {\n\t'; // offset 1944 → "class Fibonacci {\n\t" ends at 1963 | ||
|
|
||
| function makeSource(): NextEditFetchRequest { | ||
| const logContext = new InlineEditRequestLogContext('test', 0, undefined); | ||
| return new NextEditFetchRequest(generateUuid(), logContext, undefined, false); | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| configService = new InMemoryConfigurationService(new DefaultsOnlyConfigurationService()); | ||
| configService.setConfig(ConfigKey.TeamInternal.InlineEditsReverseAgreement, true); | ||
| obsWorkspace = new MutableObservableWorkspace(); | ||
| logService = new LogServiceImpl([]); | ||
| expService = new NullExperimentationService(); | ||
|
|
||
| docId = DocumentId.create(URI.file('/Users/ulugbekna/code/tsq/src/nodeTypesDefinitionProvider.ts').toString()); | ||
| // Initialize workspace doc with the CURRENT document state | ||
| // (so checkEditConsistency(documentBeforeEdit + userEditSince = currentDoc) passes) | ||
| obsWorkspace.addDocument({ id: docId, initialValue: currentDoc }); | ||
|
|
||
| cache = new NextEditCache(obsWorkspace, logService, configService, expService); | ||
| }); | ||
|
|
||
| it('rebases cached edit when model predicted class Fibonacci { and user typed the same', () => { | ||
| // Scenario from real usage: | ||
| // documentBeforeEdit (at cache time): ...class Fibonacci \n (ends at offset 1960) | ||
| // Model's edit: replace [1944,1960) "class Fibonacci " → "class Fibonacci {" | ||
| // Model also has a 2nd edit: insert at 1961 → class body | ||
| // User then typed "{\n\t" → userEditSince: [1944,1960) → "class Fibonacci {\n\t" | ||
| // | ||
| // The user's typing is a superset of the model's first edit (model: "{", user: "{\n\t"), | ||
| // so rebase should succeed and the 2nd edit (class body) should be offered. | ||
| const cachedEdit = cache.setKthNextEdit( | ||
| docId, | ||
| new StringText(docAtRequest18), | ||
| new OffsetRange(1944, 1960), // editWindow | ||
| new StringReplacement(new OffsetRange(1944, 1960), 'class Fibonacci {'), | ||
| 0, |
There was a problem hiding this comment.
This test relies on hard-coded offsets (e.g. 1944/1960/1961/1963). That makes the test brittle if docPrefix changes. Consider computing these offsets from docPrefix.length (and/or indexOf) so the scenario remains correct even if the prefix text is edited.
This issue also appears on line 131 of the same file.
| rebuild(); | ||
| for (const ctx of contexts) { | ||
| await ctx.watch(); | ||
| } |
There was a problem hiding this comment.
This PR’s description is focused on NES reverse-agreement rebase, but this hunk changes the Copilot extension esbuild watch implementation (removing the repo-level watcher/debounce and switching to ctx.watch()). If this change is intentional, please update the PR description/scope; otherwise, consider splitting it into a separate PR to keep review and risk contained.
| const docAtRequest18 = docPrefix + 'class Fibonacci '; // offset 1944 → "class Fibonacci " ends at 1960 | ||
| const currentDoc = docPrefix + 'class Fibonacci {\n\t'; // offset 1944 → "class Fibonacci {\n\t" ends at 1963 |
There was a problem hiding this comment.
docAtRequest6 is declared but never used. With noUnusedLocals/linting enabled this will fail the build; please remove it (or use it in the test).
| beforeEach(() => { | ||
| configService = new InMemoryConfigurationService(new DefaultsOnlyConfigurationService()); | ||
| configService.setConfig(ConfigKey.TeamInternal.InlineEditsReverseAgreement, true); |
There was a problem hiding this comment.
InMemoryConfigurationService.setConfig(...) returns a Promise. Please await it in beforeEach (or prefix with void) to avoid a floating promise lint violation and to ensure the config is applied before NextEditCache is constructed.
| beforeEach(() => { | |
| configService = new InMemoryConfigurationService(new DefaultsOnlyConfigurationService()); | |
| configService.setConfig(ConfigKey.TeamInternal.InlineEditsReverseAgreement, true); | |
| beforeEach(async () => { | |
| configService = new InMemoryConfigurationService(new DefaultsOnlyConfigurationService()); | |
| await configService.setConfig(ConfigKey.TeamInternal.InlineEditsReverseAgreement, true); |
When the user types more text than the model predicted at the same
position (e.g., model: '{', user: '{\n\t'), the rebase engine now
recognizes that the model's edit is consumed by the user's typing and
offers the remaining unconsumed model edits as rebased suggestions.
The new behavior is gated behind the 'reverseAgreement' experiment
flag in NesRebaseConfigs (config key:
chat.advanced.inlineEdits.reverseAgreement, default: false).
Adds 10 unit tests covering positive, negative, and edge cases.
88e8618 to
9b1a83d
Compare
When the user types more text than the model predicted at the same position (e.g., model predicts `{`, user types `{\n\t`), the rebase engine now recognizes that the model's edit is consumed by the user's typing and offers the remaining unconsumed model edits as rebased suggestions.
Changes
Key design decisions