-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Exp enable completions and NES fetcher #1245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
microsoft/vscode#270050