microsoft / microsoft/vscode

AHP listSessions mappers silently drop optional wire fields

Open
#327,626 0 comments 0 reactions 1 assignee Claimed by @osortega View on GitHub
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

The AHP `root/listSessions` payload is mapped by two independent, hand-written, field-by-field object literals:

- **producer** — `SessionSummary` built in `src/vs/platform/agentHost/node/protocolServerHandler.ts`
- **consumer** — `IAgentSessionMetadata` built in `src/vs/platform/agentHost/browser/remoteAgentHostProtocolClient.ts`

Neither is exhaustive over `SessionSummary`, and `satisfies SessionSummary` does **not** flag a missing *optional* property. An optional field added to the protocol is therefore silently dropped by either side with no compile error and no test failure. The provider-level tests can't catch it either: `MockAgentHostService.listSessions` returns its stored metadata verbatim and bypasses both mappers entirely.

This has already produced the same user-visible bug twice. Workspace-less quick chats carry their `agentHost.workspaceless` marker on `_meta`; both mappers dropped `_meta`; so any quick chat first materialized from a listing (window reload, list refresh) was classified as a workspace session rooted at the host's throwaway `~/.copilot/chats/` scratch dir, and the sessions sidebar grouped it under a section header labelled with a raw session UUID instead of under "Chats". Fixed for `_meta` in fe8a00d9d51 — but the trap itself is still armed. `annotations?: AnnotationsSummary` is on the wire today and dropped by the consumer; it has no client consumer yet, so it is latent rather than broken.

### Proposal

Make the drift impossible to reintroduce at compile time:

1. **Producer** — annotate the wire item with a mapped type that strips optionality, e.g. `{ [K in keyof SessionSummary]-?: SessionSummary[K] | undefined }`, forcing every field to be named. Every transport serializes (WebSocket JSON, IPC channel, relay), so an explicitly-`undefined` key is indistinguishable from an absent one on the wire, and the mapper already writes `activity`, `workingDirectories` and `changes` in exactly that style.
2. **Consumer** — destructure `SessionSummary` exhaustively with a rest element constrained to be empty, so a newly added wire field fails the build until the client decides whether it needs it.

### Why not just add a round-trip test

The producer is in the `node` layer and the consumer is in `browser`, so no single test can import both without violating layering (there is no precedent for that in the existing tests). A round-trip test would also only ever protect the fields it happens to cover, whereas the type-level guards cover every future field.

### Related

The deeper coupling worth revisiting separately: the host puts a throwaway scratch cwd on the wire as a real `workingDirectories` entry and then ships the "ignore it" signal out-of-band in a generic `_meta` bag. Making workspace-less-ness a first-class field of the session summary would remove the need for the marker to survive the mapping at all.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.