Sessions

Before Making Any Changes

MANDATORY: Before writing or modifying any code in src/vs/sessions/, you must read these documents:

1. `.github/instructions/coding-guidelines.instructions.md` — Naming conventions, code style, string localization, disposable management, and DI patterns. 2. `.github/instructions/source-code-organization.instructions.md` — Layers, target environments, dependency injection, and folder structure conventions.

Then read the relevant spec for the area you are changing (see table below). If you modify the implementation, you must update the corresponding spec to keep it in sync.

Specification Documents

Common Pitfalls

• Wrong menu IDs: Never use MenuId.* from vs/platform/actions for Agents window UI. Always use Menus.* from browser/menus.ts.
• Events instead of observables: Session state must flow through IObservable, not Event. Use autorun/derived for reactive UI, not onDid* event listeners.
• Importing from providers: Non-provider contrib/* code must never import from contrib/providers/*. Extract shared interfaces to services/ or common/.
• `IAgentSessionsService` in shared code: IAgentSessionsService (vs/workbench/contrib/chat/browser/agentSessions/agentSessionsService) is a Copilot-provider internal and may be imported only by the Copilot chat sessions provider (contrib/providers/copilotChatSessions/). Shared sessions code (core/services/non-provider contribs, e.g. the sessions list or visible-sessions grid) must stay provider-agnostic and go through ISession/ISessionsManagementService — never reach into model.observeSession(...) etc. for lazy loading. This is enforced by an ESLint no-restricted-imports ban scoped to src/vs/sessions/** (Copilot provider exempted).
• Missing entry point import: New contribution files must be imported in the appropriate sessions.*.main.ts entry point to be loaded (for example sessions.common.main.ts, sessions.desktop.main.ts, sessions.web.main.ts, or sessions.web.main.internal.ts).
• Modifying workbench code: Prefer extending/wrapping workbench classes in the sessions layer over modifying shared workbench components.
• Timeouts as fixes: Never use setTimeout/disposableTimeout/arbitrary delays to fix bugs or implement behaviour. They are race-prone guesses that mask the real ordering/state problem. Drive logic off deterministic signals instead — observables (autorun/derived), explicit events (onDidChange*), lifecycle phases, or awaiting the actual async operation.
• Stashed state read back later (side-channels): Never stash a value on a service during one method call and read it back from a separate query later, assuming it is still valid (e.g. a Set/flag set in openSession and consumed by a shouldX() pull-API). This is fragile temporal coupling. Instead, make it reactive state that is set atomically together with its source of truth and consumed reactively. Example: per-activation intent like "open in background / preserve focus" is exposed as an IObservable set in the same transaction as activeSession (via a single internal setter so it can never go stale), and read with .read(reader) in the consumer's autorun — never via a consume-once getter.
• Provider-owned model/mode selection belongs in the loaded chat model, with draft persistence driven by debounce: For AHP-backed chats, setModel / setAgent must push the selection into the loaded IChatModel.inputModel (like _updateChatSessionState) and let the draft-sync debounce emit chat/draftChanged. Do not immediately dispatch a model/agent-only draft from the provider, because it can overwrite unsaved typed text before the debounced full input-state draft is persisted.
• Blocking on a "pending/waiting" state instead of creating + upgrading: When an entity (e.g. a draft session) depends on something that registers asynchronously, don't withhold creation behind a pending/waiting state. Prefer creating immediately with the best available data, then replace/upgrade it once the awaited dependency arrives (driven by an onDidChange*/observable signal), cancelling the upgrade if the user changes the inputs meanwhile. Do not bound the upgrade with a timeout or even a lifecycle milestone like LifecyclePhase.Eventually — an agent host connects lazily and can surface its session types arbitrarily late, which would lock in the wrong fallback. Let the upgrade listener live for the consumer's lifetime instead.
• Over-commenting: Don't write long explanatory comments narrating what the code does or justifying ordinary patterns. Hard rules: JSDoc = 1–2 short sentences max (never enumerate every branch/feature, restate the signature, or list what the function does NOT do); inline method comments = 1 line max, only for a genuine workaround/non-obvious constraint, never to narrate the next statement. Default to no comment — if code needs a paragraph to explain, rename/extract instead. Before writing any comment longer than one line, delete it or shorten it to one line.
• Inserting/removing DOM on demand for transient UI (e.g. inline rename inputs): Don't insertBefore/appendChild+remove() a widget on the tab/row element itself when an interaction starts/ends — that churns the parent's child list and depends on event ordering during teardown. Also don't eagerly build a heavy widget (e.g. an InputBox) per row "just in case", since most rows never use it. Instead, create a stable, empty container alongside the label once, toggle its visibility via a CSS class on the row (e.g. .editing), and create the widget inside that container lazily only while editing — disposing it and emptying the container (reset(container)) when done (InputBox.dispose() does not detach its own node). Prefer the shared themed widget (InputBox + defaultInputBoxStyles) over a hand-rolled <input>.
• Collapsing distinct provider identities in pickers: Do not collapse extension-backed chat session ids (e.g. copilotcli) and agent-host ids (e.g. agent-host-copilotcli) based only on friendly names or well-known provider enums. They can coexist in the Agents window and route to different infrastructure; keep the exact session type id through selection/delegation and hide ambiguous legacy targets when an agent-host target supersedes them.
• Resolving a session's provider via the create-only tracking map: On the agent host, resolve the owning provider for any per-session operation (createChat, disposeChat, sendMessage, …) through AgentService._findProviderForSession, never the raw _sessionToProvider map. That map is populated only by createSession, so a restored session (alive in the state manager after a host restart but never created in this process) is absent from it — a direct lookup throws no provider for session and silently breaks the feature (e.g. Add Chat did nothing for restored sessions while messaging worked, because messaging already used the fallback). _findProviderForSession falls back to the session URI's scheme provider, which is what makes restored sessions work.
• Dispatching per-chat side-channel actions (agent/model) to the session URI: An agent-host session can own multiple peer chats, each with its own backend conversation (CopilotAgent._chatSessions). Conversation side-channel actions like SessionAgentChanged/SessionModelChanged must be dispatched to the per-chat turn channel (_resolveTurnDispatchChannel, which carries a chatId fragment for peer chats), not session.toString(). The session URI resolves to the session's default chat (_sessions), so dispatching there silently applies the change to the wrong conversation and an additional chat never sees the agent/model swap. The host must also forward the chatChannel through agentSideEffects.handleAction → changeAgent/changeModel, which apply it to _chatSessions when present. The protocol models summary.agent/summary.model at session level only, so equality guards comparing against session summary are valid for the default chat but must be skipped for peer chats.
• Do not infer or fall back from a peer chat channel after progress was emitted: Agent progress signals for chat-scoped actions, especially tool-call readiness and permission requests, must be emitted with the exact ahp-chat://... channel that owns the tool. Do not recover by scanning active turns, remapping ChatToolCallConfirmed, or using parseDefaultChatUri(...) ?? sessionUri in AgentSideEffects; malformed/misrouted chat channels should fail loudly so the producer or dispatch path is fixed. handleToolCallConfirmed and _toolCallAgents must use the chat channel URI containing the tool call; keying by the parent session URI makes confirmations miss the pending SDK request.
• Do not synthesize default chat URIs in the workbench handler: AgentHostSessionHandler must source the upstream default chat URI from hydrated SessionState.defaultChat / SessionState.chats and store that mapping in its chat-resource-to-upstream-URI map. Calling buildDefaultChatUri(session) in the handler assumes one server URI shape and hides protocol/provider bugs; dispatch turn lifecycle and pending/input actions through the mapped upstream chat URI instead.
• Model subagents as chats, not sessions: A subagent spawned from a tool call belongs to the parent session as an additional chat with origin.kind === "tool", hidden from the chat tab strip. Do not call restoreSession for subagents; that creates _sessionStates without a matching _chatStates entry, so later chat actions hit "Action for unknown chat". Add a chat on the parent session and dispatch the subagent turn to that chat URI.
• Keep case-sensitive ids out of URI authority: URI authorities are case-insensitive, so do not place tool call ids in the ahp-chat authority. Subagent chat URIs use a stable subagent authority and put the encoded tool call id in the path; use buildSubagentChatUri(...) instead of buildChatUri(..., \subagent-${toolCallId}\).
• Selected custom agent must be in the SDK's `customAgents`, not just `pluginDirectories`: The Copilot SDK validates the session-start agent: option (passed to createSession/resumeSession) against the customAgents list by name only — it does NOT consult pluginDirectories. copilotSessionLauncher._buildSessionConfig deliberately omits agents from file-dir plugins from customAgents (relying on the SDK's pluginDirectories discovery to avoid duplicates), so selecting a plugin/extension-contributed agent (e.g. "Inbox") otherwise fails with Custom agent '<name>' not found. The fix (toSdkSessionCustomAgents) force-adds the resolved selected agent into customAgents while every other file-dir agent still loads via pluginDirectories. Note the agent picker offers VS Code chat modes from IChatModeService, but only plugin/extension storage agents are synced to the host (SYNCABLE_STORAGE_SOURCES); user/local agents are never synced, so _resolveAgentName returns undefined for them and no agent: is sent.
• Derive SDK custom-agent names exactly like `parseAgentFile`: _resolveAgentName resolves the selected agent through the plugin parser, which trims the frontmatter name (getStringValue('name')?.trim() || nameFromFile). When building the SDK customAgents list (toSdkCustomAgents), derive the name the same way (?.trim() || agent.name); reading the raw frontmatter name without trimming yields a config name that won't match the trimmed resolvedAgentName, so the SDK still rejects the session with Custom agent '<name>' not found.
• Peer chats have no server `summary`, so dedup side-channel dispatch against the last value sent for that chat: equality guards before dispatching SessionModelChanged/SessionAgentChanged compare against summary.model/summary.agent, which only exist for the session's default chat. For peer chats, track the last-dispatched model/agent on the AgentHostChatSession instance (auto-cleaned on dispose) and diff against that — otherwise every peer-chat turn redundantly re-dispatches (and re-resolves the agent), and an intentional "clear selection" (undefined) can't be detected.
• Scrollable transcript surfaces must use workbench scrollbars: Don't make Agents/voice transcript regions scrollable with native overflow-y: auto on the content node. Wrap transcript content in DomScrollableElement/list widgets so scrollbars match VS Code theming and remain usable in narrow auxiliary-window layouts.
• *Background-sending a multi-chat composer must reset the composer before dispatching the send, not concurrently: in `NewChatInSessionWidget._send`, creating the replacement untitled chat (`openNewChatInSession({ forceNew: true })` → `provider.createNewChat`) and the fire-and-forget background `sendRequest` both reach into shared chat-session state (`acquireOrLoadSession` / `getOrCreateChatSession`) for chats in the same group*. Running them concurrently (send first, reset second) raced and left the sent chat stuck spinning with its message never dispatched, plus a second empty "New Chat" tab. Fully await the composer reset first, then fire the background send so it runs on its own.
• Chat tab order is the provider's stable creation order; don't reorder in the renderer: the agent host delivers state.chats in stable creation order (append on add, replace-in-place on update — see agentHostStateManager/the session reducer), and a genuinely new chat is appended last. The renderer's rebuild autorun (chatCompositeBar.ts) must render that order as-is. Do not partition/move in-composer Untitled chats to the end: a draft is already last, and reordering by status makes a tab jump when a draft commits out of creation order (e.g. sending the 3rd of three drafts first moved it to the front). A chat's Untitled presentation (via AdditionalChat._isNew, needed so sessionView.ts shows the composer) is independent of tab order and must not drive it. Also note _restorePeerChats (agentService.ts) must seed restored chats in getChats() order, not in Promise.all resolution order, or the catalog scrambles on reload.
• A new chat must report `SessionStatus.Untitled` until its first request is sent, regardless of how the provider creates it: sessionView.ts only shows the new-chat composer (which owns the Alt+Enter background-send handler) when activeChat.status === Untitled. The agent host commits a new peer chat eagerly, so its host status is Completed — surfacing the standard chat widget and breaking background send. Gate the chat's presented status on a provider-side isNew flag (AdditionalChat.markNew/markSent, set in createNewChat and cleared in sendRequest's committed-chat branch), not on the host-reported status.
• Service operations should return a result or throw, not `undefined` for unsupported cases: capability-gated operations like forkChatInSession must throw when the provider/session cannot perform them. Keep fallback decisions in the caller before invoking the service instead of encoding fallback as an undefined service result.
• Drop a fork when its turn point is unknown, don't forward it empty: in AgentService.createChat/createSession, if the requested fork turnId/turnIndex resolves to no source turns, set fork: undefined and fall through to a fresh create. Forwarding the fork with an empty turn slice makes the Copilot provider call sessions.fork with no toEventId, inheriting the entire backend conversation while the new chat UI is seeded with zero turns — an inconsistent hidden-history chat.
• A responsive-layout autorun must re-baseline (not react) to controller-driven restores, holding the flag across the async reveal: the desktop [D7] responsive sidebar hides the sessions sidebar when small + editor + aux-bar are all open. Switching sessions restores layout via two async paths — the desktop aux-bar restore (openView/openViewContainer) and the base controller's editor working-set apply (_applyWorkingSet, which reveals the editor part after an await and runs on a Sequencer microtask). Both reveal parts in a later autorun run, so an inline "same-run session changed" check only absorbs the synchronous transition and the async reveal still auto-hid the sidebar on navigation. Fix: a shared base-controller _withSessionLayoutRestore(work) epoch wraps both restore paths (the working-set wrap is the critical one for non-modal editors); the D7 autorun re-baselines _previousSpaceConstrained while _isRestoringSessionLayout is true. Also gate the constrained derivation on !multipleSessionsVisibleObs so the feature is disabled with multiple sessions visible. Never use a setTimeout to bridge the async reveal — tie the flag to the actual promise.
• A promise-tied "epoch" helper must decrement synchronously for void/sync work, only deferring for real Promises: _withSessionLayoutRestore increments a depth counter, runs work(), and decrements when done. If it always schedules the decrement on a microtask (Promise.resolve(result).finally(...)) — even when work() returns undefined (the common no-op restore, e.g. a session with no workspace) — the depth stays elevated for the entire synchronous caller/test body, so _isRestoringSessionLayout reads true forever and the consumer (D7) silently stops acting. Only defer the decrement when work() returns a thenable; for void/sync (or throwing) work, decrement in the finally.

Capturing Feedback (meta-rule)

Whenever the user flags a wrong pattern, rejects an approach, or gives design/rules feedback, automatically add it as a concise pitfall/learning to this Common Pitfalls section (or the most relevant spec doc) in the same change — without being asked again. Keep each entry 1–3 sentences: the anti-pattern, why it is wrong, and the preferred pattern.

Validating Changes

You must run these checks before declaring work complete:

1. npm run typecheck-client — TypeScript compilation check. Do not run `tsc` directly. 2. npm run valid-layers-check — MANDATORY. Catches layering violations. If this fails, fix the imports before proceeding. 3. scripts/test.sh --grep <pattern> — unit tests for affected areas