AHP listSessions mappers silently drop optional wire fields
- 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
Assessment
This issue has not been assessed yet.