mirror of
https://github.com/microsoft/vscode.git
synced 2026-09-09 02:12:20 +01:00
Ulugbek AbdullaevandCopilot
ee9c82e739
* 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>