fix: require models field in ModelList validator (fixes #328226)
- Dominant language
- TypeScript
- Stars
- 193k
- Forks
- 42.4k
- PR merge metrics
- PR metrics pending
Description
### Summary
`get nesModels` in `ProxyModelsService` throws `TypeError: Cannot read properties of undefined (reading 'filter')` because `this._models.models` is `undefined` while `this._models` itself is defined. This happens when the `/models` proxy endpoint returns a JSON object that lacks the `models` key: the response validator treats `models` as optional and produces `{ models: undefined }`, which violates the `ModelList.t` type contract (`models: Model.t[]`). The getter then calls `.filter` on `undefined`. Impact: 7.1x spike (44 → 314 affected users) across the 0.56.0 → 0.58.0 range, predominantly macOS.
Fixes microsoft/vscode\#328226
Recommended reviewer: `@ulugbekna`
### Culprit Commit
This is a pre-existing type-contract defect in the validation layer, surfaced (not introduced) by the recent spike. The `ModelList` validator has declared `models` without `vRequired` since it was introduced.
| Field | Value |
|-------|-------|
| Commit | [`3c78ed8`](https://github.com/microsoft/vscode/commit/3c78ed81dda850fa78544e8ff737955660faf440) |
| Author | `@ulugbekna` |
| PR | #2325 |
| Message | nes: support /models on proxy and model picker |
| Why | Introduced `ModelList.validator = vObj({ models: vArray(...) })` without `vRequired`. Because `vObj` skips validation for non-required fields that are missing, a `/models` response without a `models` key validates successfully to `{ models: undefined }`, violating the declared `ModelList.t` type. The runtime spike appears to be a server-side response-shape change; the bypass that lets the bad shape through is this validator. |
### Code Flow
```mermaid
sequenceDiagram
participant Server as /models endpoint
participant Fetch as _fetchLatestModels
participant Validator as ModelList.validator
participant Store as this._models
participant Getter as get nesModels
Server->>Fetch: JSON without "models" key
Fetch->>Validator: validate(jsonData)
Note over Validator: ⚠️ Root cause:
models not vRequired →
missing field skipped →
returns { models: undefined }
Validator->>Store: this._models = { models: undefined }
Store->>Getter: onModelListUpdated fires
Note over Getter: 💥 this._models?.models.filter(...)
undefined.filter → TypeError
```
### Affected Files
| File | Role | Evidence |
|------|------|----------|
| `extensions/copilot/src/platform/proxyModels/node/proxyModelsService.ts` | crash site | L68: `return this._models?.models.filter(...)` |
| `extensions/copilot/src/platform/inlineEdits/common/dataTypes/inlineEditsModelsTypes.ts` | root cause | L47-L49: `validator = vObj({ models: vArray(Model.validator) })` — `models` not marked required |
| `extensions/copilot/src/platform/configuration/common/validator.ts` | bypass mechanism | L138-L143: `vObj` skips non-required fields that are `undefined`, so a missing `models` passes validation |
### Repro Steps
1. Configure the proxy `/models` endpoint (or a mock) to return a JSON object that omits the `models` key (e.g. `{}` or `{ "someOtherKey": 1 }`).
2. Trigger a Copilot token store update so the `autorun` in `ProxyModelsService` fetches `/models`.
3. The response validates to `{ models: undefined }` and is stored in `this._models`, then `onModelListUpdated` fires.
4. The `inlineEditsModelService` observable reads `proxyModelsService.nesModels`, invoking the getter → `undefined.filter` → `TypeError`.
### How the Fix Works
**Chosen approach** (`extensions/copilot/src/platform/inlineEdits/common/dataTypes/inlineEditsModelsTypes.ts`): wrap the `models` field validator in `vRequired`: `models: vRequired(vArray(Model.validator))`. This fixes the defect at the data producer (the validator that constructs the `ModelList.t` value), not at the crash site. With `models` required, a `/models` response missing the key now fails validation; `_fetchLatestModels` takes its existing error branch (`throw new Error('Invalid /models response data: ...')`), which is caught and reported through the existing `logService.error` telemetry path, and `_models` is never assigned an object whose `models` is `undefined`. After this change, `inlineEditsModelsTypes.ts:48` cannot produce a `ModelList.t` whose `models` is `undefined`, so `proxyModelsService.ts:68` can no longer call `.filter` on `undefined`. The sibling getters `cursorJumpModels` and `instantApplyModels` are protected by the same change.
**Alternatives considered**:
- Guard at the crash site (`this._models?.models?.filter(...)` or an early return): rejected because it patches the consumer/crash site and coerces the type-contract violation into a benign value, hiding the fact that an invalid response shape was accepted and silencing the existing telemetry — it fixes the symptom, not the producer.
- Widen `ModelList.t` to `models?: Model.t[]`: rejected because the field is genuinely required by every consumer; making it optional pushes null-checks to every call site instead of rejecting the bad payload once.
**Bypass identified**: optional-field-treated-as-present at `extensions/copilot/src/platform/inlineEdits/common/dataTypes/inlineEditsModelsTypes.ts:48`
**Why it fails**: `vObj` (`validator.ts:138-143`) skips validation for fields that are not `vRequired` when the incoming value is `undefined`, so a `/models` response lacking `models` validates to `{ models: undefined }`, which the declared type says is `Model.t[]`.
### Recommended Owner
`@ulugbekna` — introduced and owns `ProxyModelsService`, the `ModelList` wire types, and the NES `/models` path; author of the surrounding commits and highly active in `microsoft/vscode` (100+ commits in the last 90 days).
> Generated by [errors-fix](https://github.com/microsoft/vscode-engineering/actions/runs/30556489194) · opus48 · 594.9 AIC · ⌖ 11.5 AIC · ⊞ 18.1K · [◷](https://github.com/search?q=repo%3Amicrosoft%2Fvscode+%22gh-aw-workflow-id%3A+errors-fix%22&type=pullrequests)
---
> [!NOTE]
> This was originally intended as a pull request, but the git push operation failed.
>
> **Original error:** The process '/usr/bin/git' failed with exit code 128
>
> **Workflow Run:** [View run details and download bundle artifact](https://github.com/microsoft/vscode-engineering/actions/runs/30556489194)
>
> The bundle file is available in the `agent` artifact in the workflow run linked above.
To create a pull request with the changes:
```sh
# Download the artifact from the workflow run
gh run download 30556489194 -n agent -D /tmp/agent-30556489194
# Fetch the bundle into a temporary ref, then update the local branch
git fetch /tmp/agent-30556489194/aw-microsoft-vscode-errors-fix-328226-modellist-validator.bundle refs/heads/errors-fix/328226-modellist-validator:refs/bundles/create-pr-errors-fix-328226-modellist-validator-6284537bdf29f4e2-967394ad
git update-ref refs/heads/errors-fix/328226-modellist-validator-6284537bdf29f4e2 refs/bundles/create-pr-errors-fix-328226-modellist-validator-6284537bdf29f4e2-967394ad
git checkout errors-fix/328226-modellist-validator-6284537bdf29f4e2
# Ensure the working tree matches the updated branch
git reset --hard
# Remove the temporary bundle ref
git update-ref -d refs/bundles/create-pr-errors-fix-328226-modellist-validator-6284537bdf29f4e2-967394ad
# Push the branch to origin
git push https://github.com/bryanchen-d/vscode.git errors-fix/328226-modellist-validator-6284537bdf29f4e2
# Create the pull request
gh pr create --title 'fix: require models field in ModelList validator (fixes #328226)' --base main --head bryanchen-d:errors-fix/328226-modellist-validator-6284537bdf29f4e2 --repo microsoft/vscode
```
Contributor guide
Assessment
This issue has not been assessed yet.