Exp enable completions and NES fetcher #1245
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds experiment-based settings to control which fetcher implementation is used for completions and next edit suggestions, introduces tracking of failing (known-bad) fetchers, and extends fetch fallback logic to respect user-selected or disabled fetchers.
- Adds new experimental config keys and schema entries (completionsFetcher, nesFetcher).
- Extends fetch fallback pipeline to track failing fetchers and allow explicit fetcher selection with safeguards (disabled or known-bad).
- Passes selected fetcher through to next edit suggestions provider.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/networking/vscode-node/fetcherServiceImpl.ts | Stores and updates known-bad fetchers from fallback results. |
| src/platform/networking/test/node/fetcherFallback.spec.ts | Updates tests for new parameters and adds cases for useFetcher behavior. |
| src/platform/networking/node/fetcherFallback.ts | Adds config-aware fetcher selection, tracking of failing fetchers, and updated function signature. |
| src/platform/configuration/common/configurationService.ts | Introduces new experimental config keys for choosing fetchers. |
| src/extension/xtab/node/xtabProvider.ts | Passes experiment-based fetcher selection into streaming request options. |
| package.nls.json | Adds localized descriptions for new settings. |
| package.json | Declares new settings in configuration schema and updates completionsCore hash. |
| this._availableFetchers = updatedFetchers; | ||
| } | ||
| if (updatedKnownBadFetchers) { | ||
| this._knownBadFetchers = updatedKnownBadFetchers; |
There was a problem hiding this comment.
Replacing the existing this._knownBadFetchers set with updatedKnownBadFetchers discards previously known bad fetchers on calls where only new failures are surfaced; this causes earlier failing fetchers to be forgotten unless they fail again in the same run. Merge instead: for (const f of updatedKnownBadFetchers) this._knownBadFetchers.add(f); to preserve cumulative state.
| this._knownBadFetchers = updatedKnownBadFetchers; | |
| for (const f of updatedKnownBadFetchers) { | |
| this._knownBadFetchers.add(f); | |
| } |
Copilot uses AI. Check for mistakes.
| export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], url: string, options: FetchOptions, knownBadFetchers: Set<string>, configurationService: IConfigurationService, logService: ILogService): Promise<{ response: Response; updatedFetchers?: IFetcher[]; updatedKnownBadFetchers?: Set<string> }> { | ||
| if (options.retryFallbacks && availableFetchers.length > 1) { | ||
| let firstResult: { ok: boolean; response: Response } | { ok: false; err: any } | undefined; | ||
| const updatedKnownBadFetchers = new Set<string>(); |
There was a problem hiding this comment.
updatedKnownBadFetchers is initialized empty instead of cloning/unioning the incoming knownBadFetchers, and the caller later replaces its state (see FetcherService) leading to loss of previously known bad fetchers; either (a) initialize with new Set(knownBadFetchers) and add new ones, or (b) return only newly failed fetchers but have the caller union rather than replace to ensure consistent accumulation semantics.
| const updatedKnownBadFetchers = new Set<string>(); | |
| const updatedKnownBadFetchers = new Set(knownBadFetchers); |
Copilot uses AI. Check for mistakes.
| firstResult = result; | ||
| } | ||
| if (!result.ok) { | ||
| updatedKnownBadFetchers.add(fetcher.getUserAgentLibrary()); |
There was a problem hiding this comment.
updatedKnownBadFetchers is initialized empty instead of cloning/unioning the incoming knownBadFetchers, and the caller later replaces its state (see FetcherService) leading to loss of previously known bad fetchers; either (a) initialize with new Set(knownBadFetchers) and add new ones, or (b) return only newly failed fetchers but have the caller union rather than replace to ensure consistent accumulation semantics.
Copilot uses AI. Check for mistakes.
| updatedFetchers.splice(updatedFetchers.indexOf(fetcher), 1); | ||
| updatedFetchers.unshift(fetcher); | ||
| return { response: result.response, updatedFetchers }; | ||
| return { response: result.response, updatedFetchers, updatedKnownBadFetchers }; |
There was a problem hiding this comment.
updatedKnownBadFetchers is initialized empty instead of cloning/unioning the incoming knownBadFetchers, and the caller later replaces its state (see FetcherService) leading to loss of previously known bad fetchers; either (a) initialize with new Set(knownBadFetchers) and add new ones, or (b) return only newly failed fetchers but have the caller union rather than replace to ensure consistent accumulation semantics.
Copilot uses AI. Check for mistakes.
| export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], url: string, options: FetchOptions, knownBadFetchers: Set<string>, configurationService: IConfigurationService, logService: ILogService): Promise<{ response: Response; updatedFetchers?: IFetcher[]; updatedKnownBadFetchers?: Set<string> }> { | ||
| if (options.retryFallbacks && availableFetchers.length > 1) { | ||
| let firstResult: { ok: boolean; response: Response } | { ok: false; err: any } | undefined; | ||
| const updatedKnownBadFetchers = new Set<string>(); |
There was a problem hiding this comment.
The parameter and return types use Set while the codebase introduces FetcherId; narrowing these to Set improves type safety and consistency with other parts (tests already use Set).
| export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], url: string, options: FetchOptions, knownBadFetchers: Set<string>, configurationService: IConfigurationService, logService: ILogService): Promise<{ response: Response; updatedFetchers?: IFetcher[]; updatedKnownBadFetchers?: Set<string> }> { | |
| if (options.retryFallbacks && availableFetchers.length > 1) { | |
| let firstResult: { ok: boolean; response: Response } | { ok: false; err: any } | undefined; | |
| const updatedKnownBadFetchers = new Set<string>(); | |
| export async function fetchWithFallbacks(availableFetchers: readonly IFetcher[], url: string, options: FetchOptions, knownBadFetchers: Set<FetcherId>, configurationService: IConfigurationService, logService: ILogService): Promise<{ response: Response; updatedFetchers?: IFetcher[]; updatedKnownBadFetchers?: Set<FetcherId> }> { | |
| if (options.retryFallbacks && availableFetchers.length > 1) { | |
| let firstResult: { ok: boolean; response: Response } | { ok: false; err: any } | undefined; | |
| const updatedKnownBadFetchers = new Set<FetcherId>(); |
Copilot uses AI. Check for mistakes.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
microsoft/vscode#270050