Add new exp TAS assignments endpoint (/api/v1/assignments) alongside legacy TAS - #329653
Conversation
There was a problem hiding this comment.
Pull request overview
Adds the new TAS assignments endpoint alongside legacy TAS in VS Code core and Copilot.
Changes:
- Adds endpoint discovery, transport, filters, and result merging.
- Recreates clients when account endpoints change.
- Upgrades TAS dependencies and telemetry schemas.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/services/assignment/common/assignmentService.ts |
Integrates the assignments endpoint in core. |
src/vs/workbench/services/assignment/common/assignmentFilters.ts |
Adds GitHub assignment filters. |
src/vs/platform/assignment/common/assignment.ts |
Adds core assignment filter keys/providers. |
src/vs/base/common/defaultAccount.ts |
Models entitlement endpoints. |
remote/web/package.json |
Upgrades tas-client. |
remote/web/package-lock.json |
Updates locked web dependency. |
remote/package.json |
Upgrades remote tas-client. |
remote/package-lock.json |
Updates locked remote dependency. |
package.json |
Upgrades root tas-client. |
package-lock.json |
Updates locked root dependency. |
extensions/copilot/src/platform/telemetry/vscode-node/microsoftExperimentationService.ts |
Integrates assignments into Copilot experimentation. |
extensions/copilot/src/platform/telemetry/node/baseExperimentationService.ts |
Supports delegate recreation. |
extensions/copilot/src/platform/telemetry/common/baseTelemetryService.ts |
Declares assignments telemetry. |
extensions/copilot/src/platform/authentication/common/copilotToken.ts |
Models and validates the exp endpoint. |
extensions/copilot/package.json |
Upgrades vscode-tas-client. |
extensions/copilot/package-lock.json |
Updates Copilot TAS dependencies. |
Review details
Files not reviewed (3)
- extensions/copilot/package-lock.json: Generated file
- remote/package-lock.json: Generated file
- remote/web/package-lock.json: Generated file
Suppressed comments (1)
src/vs/workbench/services/assignment/common/assignmentService.ts:193
- Replacing the promise leaves the previous
ExperimentationServicealive. Each instance owns therefetchIntervalpolling timer, so when the entitlement endpoint appears or changes, the old client continues polling and issuing duplicate TAS requests. Dispose the previous resolved client during replacement and the current client whenWorkbenchAssignmentServiceis disposed, while guarding setup races.
this.tasClient = this.setupTASClient();
- Files reviewed: 12/16 changed files
- Comments generated: 3
- Review effort level: Balanced
|
|
||
| // The assignments endpoint is sourced from account entitlements, which load | ||
| // asynchronously. Re-setup the client when it first appears or changes. | ||
| this._register(this.defaultAccountService.onDidChangeDefaultAccount(() => { |
There was a problem hiding this comment.
AI Review: This listener can miss the initial asynchronous account load. DefaultAccountService.setDefaultAccountProvider() assigns the first refresh() result directly and opens its barrier without firing onDidChangeDefaultAccount, so if this service is created first it snapshots no exp endpoint and stays legacy-only until a later account change. Please await the service's initial account readiness before the first TAS setup (and cover this startup ordering in a test).
| this.networkInitialized = false; | ||
|
|
||
| // Dispose the previously created client so it stops auto-polling the (legacy) endpoint. | ||
| WorkbenchAssignmentService.disposeTasClient(this.tasClient); |
There was a problem hiding this comment.
AI Review: Disposing the superseded tas-client does not make an in-flight fetch inert. In 0.4.2, dispose() only stops the current polling timer; an already-running getFeaturesAsync() can still update the shared memento and then call StartPolling() after its await, reviving the old client. The generation guard only protects networkInitialized. Please add cancellation/disposal invalidation in the client so stale fetches cannot write storage or restart polling; the Copilot delegate-replacement path has the same race.
| private _createDelegate(): ITASExperimentationService { | ||
| const delegate = this._delegateFn(this._globalState, this._userInfoStore); | ||
| this._delegateDisposable.value = toDisposable(() => (delegate as unknown as { dispose?(): void }).dispose?.()); | ||
| delegate.initialFetch.then(() => { |
There was a problem hiding this comment.
AI Review: When an endpoint change recreates the delegate, completion of the replacement's initialFetch only logs. Previously read treatment values are never compared or announced, so experiment-backed configuration can remain stale until another trigger, potentially the hourly refresh. Please signal treatment changes after the current replacement delegate finishes its initial fetch, with a generation/current-delegate guard for superseded fetches.
| this.productService.version, | ||
| this.productService.nameLong, | ||
| this.telemetryService.devDeviceId, | ||
| targetPopulation, |
There was a problem hiding this comment.
AI Review: tasSetupDisposables owns this provider and its entitlement subscription, but the store itself is never registered with WorkbenchAssignmentService. Re-setup clears it, but final service disposal leaves the current providers/listeners alive. Please register the store with the service (while retaining clear() for replacements).
| this.treatmentRequests = []; | ||
| } | ||
|
|
||
| dispose(): void { } |
There was a problem hiding this comment.
AI Review: The production change adds endpoint-driven delegate replacement, disposal, and asynchronous initial-fetch behavior, but the only test update is this no-op method. Please add focused tests for late endpoint arrival/change, disposal of the old delegate, ignoring superseded fetch completion, and notifying treatment changes after the replacement fetch.
|
|
||
| /** | ||
| * Formats an ISO release date into the `yyyymmddHH` form the experimentation backend | ||
| * expects (10 digits, fits within int32). Returns an empty string when unavailable. |
There was a problem hiding this comment.
AI Review: The fits within int32 claim is incorrect: a current yyyymmddHH value such as 2026080712 is about 20.3 billion, well above int32 max 2,147,483,647. Please correct the contract comment (here and in the Copilot copy) or change the encoding if the backend truly requires an int32.
|
|
||
| private _createDelegate(): ITASExperimentationService { | ||
| const delegate = this._delegateFn(this._globalState, this._userInfoStore); | ||
| this._delegateDisposable.value = toDisposable(() => (delegate as unknown as { dispose?(): void }).dispose?.()); |
There was a problem hiding this comment.
AI Review: IExperimentationService in tas-client@0.4.2 already requires dispose(), and vscode-tas-client@0.3.0 returns that interface, so this unknown cast and optional call hide the actual lifecycle contract. Please call delegate.dispose() directly (and do the same in the core helper); this will also make future API mismatches fail at compile time.
Summary
Wires the new Treatment Assignment Service endpoint (
<exp-host>/api/v1/assignments) into both VS Code core and the Copilot extension, running in parallel with the legacydefault.exp-tas.com/vscode/abendpoint and merging the two result sets so the new endpoint can be validated with real experiments before the legacy one is retired.