Skip to content

[cherry-pick] nes: add optimized PatchBased02 prompt strategy - #332316

Merged
Dmitriy Vasyura (dmitrivMS) merged 1 commit into
mainfrom
cherry-pick/332018
Aug 24, 2026
Merged

[cherry-pick] nes: add optimized PatchBased02 prompt strategy#332316
Dmitriy Vasyura (dmitrivMS) merged 1 commit into
mainfrom
cherry-pick/332018

Conversation

@vs-code-engineering

Copy link
Copy Markdown
Contributor

Cherry-pick of #332018 from release/1.135.

Summary

Adds patchBased02Optimized, an implementation-neutral prompting strategy for the optimized inline-suggestion treatment. It inherits everything from patchBased02WithRecentLineNumbers (current/recent-file line numbers, cross-file edits/imports, disabled next-cursor prediction) and additionally bakes in the client, latency, and completions-unification knobs this unified model was tuned for, so a single promptingStrategy value carries the whole treatment rather than relying on many separate client settings or experiment/deployment toggles.

This makes the treatment self-contained enough to ship purely as the prompt template of a copilot-proxy /models deployment: the proxy only specifies promptStrategy: "patchBased02Optimized" and the client derives everything else.

Analogous to the change for 2-in-1 models made in this PR: #321755

Baked-in configuration

patchBased02Optimized spreads the shared patchBased02WithRecentLineNumbers config and adds:

Setting Baked value
patchModelPredictionKind currentLineCompleted
splitPatchOnDiff true
patchFastYieldLineWithCursor true
extraDebounceEndOfLine 0
nesMimicGhostTextBehavior true
cacheDelay 200
rebasedCacheDelay 0
debounce 0
supportsUnifiedCompletions true

How it works

  • These are exposed as optional ModelConfiguration fields (each with a MODEL_CONFIGURATION_VALIDATOR entry), following the same pattern used to bake in allowImportChanges and supportsNextCursorLinePrediction.
  • Every read site resolves via modelConfig.<field> ?? experimentDefault, so behavior is unchanged for any strategy that does not set a value.
  • selectedModelConfiguration() returns the strategy-resolved config, so consumers outside xtabProvider observe the baked values:
    • xtabProvider (patchModelPredictionKind, splitPatchOnDiff, patchFastYieldLineWithCursor, extraDebounceEndOfLine) via the already-resolved model config.
    • NextEditProvider (cacheDelay, rebasedCacheDelay) and UserInteractionMonitor (debounce) inject IInlineEditsModelService.
    • inline completion provider (nesMimicGhostTextBehavior) via its existing model service.
  • Adds a reusable NullInlineEditsModelService test double for the newly-injected dependency.

Completions unification

supportsUnifiedCompletions bakes completions unification into the strategy, following the exact supportsNextCursorLinePrediction precedent where a baked capability overrides the deployment toggle at each read site. When the selected NES model's strategy sets it, the client runs as the single unified provider — one baked boolean replaces all three unification settings:

  • inlineCompletionProvider.ts: unification resolves via selectedModelConfiguration().supportsUnifiedCompletions ?? InlineEditsUnification (replaces github.copilot.chat.advanced.inlineEdits.unification).
  • jointInlineCompletionProvider.ts: the baked flag is observed reactively (via onModelListUpdated) and OR-ed into the effective modelUnification, which both suppresses the separate GitHub Copilot completions provider and pushes github.copilot into the provider excludes (replaces inlineCompletionsUnificationModel (ExP) and excludedProviders: "completions,github.copilot" (local)).

The baked value never replaces the existing experiment path for other models — it only short-circuits it when set, so unrelated deployments are unaffected.

Intended deployment

{
  "modelConfiguration": {
    "modelName": "copilot-suggestions-lysithea-0024",
    "promptingStrategy": "patchBased02Optimized",
    "includeTagsInCurrentFile": false
  }
}

No inlineCompletionsUnificationModel / unification / excludedProviders toggles are required — the strategy carries them.

Testing

  • npm run typecheck --prefix extensions/copilot
  • focused Vitest coverage for applyStrategyConfig, MODEL_CONFIGURATION_VALIDATOR, pickSystemPrompt, NextEditProvider caching/speculative, and UserInteractionMonitor
  • npm run precommit

Copilot AI balanced review requested due to automatic review settings August 24, 2026 12:51
@vs-code-engineering vs-code-engineering Bot added the cherry-pick-artifact Auto-generated cherry-pick PR label Aug 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a self-contained optimized PatchBased02 prompting strategy for unified inline suggestions.

Changes:

  • Bakes model, latency, caching, and unification settings into the strategy.
  • Propagates strategy configuration across inline-edit providers.
  • Adds supporting model-service stubs and tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
xtabPromptOptions.spec.ts Tests strategy configuration.
inlineEditsModelService.ts (node) Resolves selected strategy settings.
inlineEditsModelService.ts (common) Adds null model service.
xtabPromptOptions.ts Defines optimized strategy and settings.
xtabProvider.spec.ts Covers prompt selection.
xtabProvider.ts Applies optimized runtime settings.
promptCrafting.ts Supports optimized prompt formatting.
jointInlineCompletionProvider.ts Integrates unified completions.
inlineCompletionProvider.ts Reads model-specific behavior.
userInteractionMonitor.spec.ts Updates test dependencies.
nextEditProvider.ts Applies model-specific cache delays.
userInteractionMonitor.ts Applies model-specific debounce.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// A model whose strategy bakes in `supportsUnifiedCompletions` runs as the single unified
// provider: this stands in for the `modelUnification` deployment toggle so the behavior can
// be driven purely from the selected model's prompting strategy.
const modelUnification = this._supportsUnifiedCompletions.read(reader) || (unificationStateValue?.modelUnification ?? false);
Comment on lines +545 to +546
/** Optimized PatchBased02 variant with line numbers on recent docs. */
PatchBased02Optimized = 'patchBased02Optimized',
@dmitrivMS
Dmitriy Vasyura (dmitrivMS) merged commit e845953 into main Aug 24, 2026
28 checks passed
@dmitrivMS
Dmitriy Vasyura (dmitrivMS) deleted the cherry-pick/332018 branch August 24, 2026 16:42
@vs-code-engineering vs-code-engineering Bot added this to the 1.135.0 milestone Aug 24, 2026
Ulugbek Abdullaev (ulugbekna) added a commit that referenced this pull request Aug 30, 2026
The `patchBased02Optimized` strategy (#332018 / #332316) bakes in the client and latency knobs its model was tuned for, so a `copilot-proxy` `/models` deployment can ship the whole treatment as one `promptStrategy` value.

Every read site resolved it as `modelConfig.x ?? getExperimentBasedConfig(x)`. Since `getExperimentBasedConfig` already resolves user setting -> experiment treatment -> default, putting the model configuration first placed it above *both*: once a strategy baked a knob in, neither the user nor a flight could move it, and an A/B arm assigning one of these settings measured nothing. Reads now go through `resolveModelConfigValue`, which consults the strategy only when the setting is not explicitly driven: user setting, then experiment treatment, then model configuration, then the setting default. Registration-time reads use the observable form, since an imperative read inside an `autorun` would not subscribe and would miss a treatment arriving after the graph was built.

`supportsUnifiedCompletions` was resolved in two places -- `InlineEditProviderFeature` for the provider `excludes` and `InlineCompletionProviderImpl` per request -- which were free to disagree. If registration excluded the separate provider while a request declined to stand in for it, the user would be left with no inline suggestion at all. Both, plus the joint provider, now resolve through one `observeUnifiedCompletions`, which keeps the existing OR with the core `modelUnification` toggle.

`defaultModelConfiguration()` now applies `applyStrategyConfig` as well, so the `forceUseDefaultModel` fallback stops silently dropping every baked knob.

The strategy is renamed from `patchBased02Optimized` to `patchBased02Unified`: "optimized" did not say what the variant does, while "unified" names its defining property and matches the existing `xtabUnifiedModel`. Nothing sends the old wire value -- no live experiment assigns it -- so the wire value is renamed with the identifier rather than letting the two spellings drift apart.

Note that flights currently assigning these settings win over the baked values, so the strategy only becomes self-contained for those users once those flights are retired. The startup window on the fetched `/models` path (#332837) is unchanged and remains a known limitation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ulugbek Abdullaev (ulugbekna) added a commit that referenced this pull request Aug 30, 2026
…3423)

* configuration: refactor: hoist experiment-based config resolution into the base class

`ConfigurationServiceImpl` and `DefaultsOnlyConfigurationService` each carried a verbatim copy of the five-step treatment-variable lookup, so every change to the lookup order had to be made twice.

Move the shared resolution into `AbstractConfigurationService`: `getExperimentBasedConfig` becomes concrete there, built from a `_getExperimentTreatment` helper and a `_getUserConfiguredExperimentBasedValue` hook that defaults to "nothing configured" and is overridden by the implementation that can actually read user settings. No behavior change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* configuration: fix: invalidate config observables for every treatment name a setting is published under

`getExperimentBasedConfig` resolves a treatment under any of five names -- the setting's `experimentName`, `copilotchat.config.<id>`, `config.<fullyQualifiedId>`, and two more for a migrated setting's old id -- but `updateExperimentBasedConfiguration` only recognized `config.<fullyQualifiedId>` and the old id's equivalent.

A treatment arriving under one of the other names therefore assigned the setting without notifying anyone observing it, so every `getExperimentBasedConfigObservable` consumer kept serving the pre-treatment value until some unrelated configuration change happened to invalidate it.

Match the same alias list the resolution uses. The new set is a superset of the old one, so nothing that used to invalidate stops doing so.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* configuration: feat: expose explicitly-set experiment-based config values

`getExperimentBasedConfig` resolves user setting -> experiment treatment -> setting default, and there was no way to ask which of those three a value came from. Callers that have another source of defaults therefore had to place that source *above* the setting, defeating both the user's own configuration and the remote experiment lever.

Add `getExperimentBasedConfigIfSet` (plus an observable form) which reports only the explicitly provided value, so such a caller can fall back to its own default only when the setting is not driven by the user or by an experiment.

The observable cache is now keyed by accessor as well as setting id: it previously keyed on the setting id alone, so observing one setting through two accessors would have handed back the wrong observable. No key could reach both accessors before, since a simple and an experiment-based setting are distinct types, but a third accessor makes the collision reachable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* nes: fix: let settings and experiments override baked strategy values

The `patchBased02Optimized` strategy (#332018 / #332316) bakes in the client and latency knobs its model was tuned for, so a `copilot-proxy` `/models` deployment can ship the whole treatment as one `promptStrategy` value.

Every read site resolved it as `modelConfig.x ?? getExperimentBasedConfig(x)`. Since `getExperimentBasedConfig` already resolves user setting -> experiment treatment -> default, putting the model configuration first placed it above *both*: once a strategy baked a knob in, neither the user nor a flight could move it, and an A/B arm assigning one of these settings measured nothing. Reads now go through `resolveModelConfigValue`, which consults the strategy only when the setting is not explicitly driven: user setting, then experiment treatment, then model configuration, then the setting default. Registration-time reads use the observable form, since an imperative read inside an `autorun` would not subscribe and would miss a treatment arriving after the graph was built.

`supportsUnifiedCompletions` was resolved in two places -- `InlineEditProviderFeature` for the provider `excludes` and `InlineCompletionProviderImpl` per request -- which were free to disagree. If registration excluded the separate provider while a request declined to stand in for it, the user would be left with no inline suggestion at all. Both, plus the joint provider, now resolve through one `observeUnifiedCompletions`, which keeps the existing OR with the core `modelUnification` toggle.

`defaultModelConfiguration()` now applies `applyStrategyConfig` as well, so the `forceUseDefaultModel` fallback stops silently dropping every baked knob.

The strategy is renamed from `patchBased02Optimized` to `patchBased02Unified`: "optimized" did not say what the variant does, while "unified" names its defining property and matches the existing `xtabUnifiedModel`. Nothing sends the old wire value -- no live experiment assigns it -- so the wire value is renamed with the identifier rather than letting the two spellings drift apart.

Note that flights currently assigning these settings win over the baked values, so the strategy only becomes self-contained for those users once those flights are retired. The startup window on the fetched `/models` path (#332837) is unchanged and remains a known limitation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick-artifact Auto-generated cherry-pick PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants