Fix session handling and allow additional characters#296304
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Agent Sessions (standalone vs/sessions) experience by improving session lifecycle handling, moving “Run Script” configuration from workspace storage into a per-repo sessions config file, and adjusting agent feedback UI contributions.
Changes:
- Add session APIs to create “pending” sessions and to commit worktree files + refresh the agent sessions model.
- Introduce
.vscode/sessions.json+ a newSessionsConfigurationServiceto read/write session scripts, and rework the Run Script UI to use it. - Refactor agent feedback editor integrations and add a glyph-margin decoration contribution; update hygiene to allow an em dash.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/contrib/sessions/browser/sessionsManagementService.ts | Adds pending-session creation and a worktree commit+refresh helper API. |
| src/vs/sessions/contrib/chat/browser/sessionsConfigurationService.ts | New service to watch/read/write .vscode/sessions.json scripts for the active session. |
| src/vs/sessions/contrib/chat/browser/runScriptAction.ts | Switches Run Script actions to scripts sourced from SessionsConfigurationService. |
| src/vs/sessions/contrib/chat/browser/newChatViewPane.ts | Uses SessionsManagementService.createNewPendingSession when generating pending session resources. |
| src/vs/sessions/contrib/chat/browser/chat.contribution.ts | Registers the new sessions configuration service singleton. |
| src/vs/sessions/contrib/aiCustomizationManagement/browser/aiCustomizationManagementEditor.ts | Reuses new commitWorktreeFiles API instead of executing commit command directly. |
| src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackGlyphMargin.css | Adds styling for new feedback glyph-margin decorations. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts | Removes comment-controller thread syncing; keeps feedback storage + navigation logic. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackGlyphMarginContribution.ts | New editor contribution that decorates glyph margin based on feedback + hover hints. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorUtils.ts | Simplifies session lookup helper to return `URI |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorOverlay.ts | Updates overlay session resolution to use getSessionForResource. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts | Adapts to new getSessionForResource return type. |
| src/vs/sessions/contrib/agentFeedback/browser/agentFeedback.contribution.ts | Imports the new glyph margin contribution. |
| build/hygiene.ts | Allows em dash in the unicode hygiene regex. |
| .vscode/sessions.json | Adds initial scripts configuration for this repo. |
Comments suppressed due to low confidence (1)
src/vs/sessions/contrib/chat/browser/sessionsConfigurationService.ts:95
getScripts(session: IActiveSessionItem)suggests callers can request scripts for an arbitrary session, but the implementation ignores the parameter and returns a single observable tied to the current active session. This mismatch is easy to misuse; consider either removing the parameter from the API or implementing per-session observables keyed by session/worktree URI.
getScripts(_session: IActiveSessionItem): IObservable<readonly ISessionScript[]> {
return this._scripts;
}
| // Read the file (async, updates _scripts when done) | ||
| this._readScripts(configUri).then( | ||
| scripts => this._scripts.set(scripts, undefined), | ||
| () => this._scripts.set([], undefined), | ||
| ); |
There was a problem hiding this comment.
The async _readScripts(configUri).then(...) can resolve after the active session (or configUri) has changed, and will then overwrite _scripts with stale data for the wrong session. Consider tracking a monotonically increasing request id / the current configUri and only applying the result if it still matches, or use a cancellation pattern for the in-flight read.
This issue also appears on line 93 of the same file.
| await this._fileService.writeFile(uri, VSBuffer.fromString(data)); | ||
| this._logService.trace(`[SessionsConfigurationService] Wrote ${scripts.length} script(s) to ${uri.toString()}`); | ||
|
|
||
| await this._activeSessionService.commitWorktreeFiles(session, [uri]); |
There was a problem hiding this comment.
_writeScripts always calls commitWorktreeFiles(session, [uri]), but _getConfigFileUri can return a path under session.repository even when session.worktree is undefined. In that case commitWorktreeFiles will throw. Either restrict config writes to worktree-backed sessions (return undefined when !session.worktree), or conditionally skip the commit/refresh step when there is no worktree.
| await this._activeSessionService.commitWorktreeFiles(session, [uri]); | |
| if (session.worktree) { | |
| await this._activeSessionService.commitWorktreeFiles(session, [uri]); | |
| } |
| } else { | ||
| // Register an action for each script | ||
| for (let i = 0; i < scripts.length; i++) { | ||
| const script = scripts[i]; | ||
| const actionId = `${RUN_SCRIPT_ACTION_ID}.${i}`; | ||
|
|
There was a problem hiding this comment.
When scripts exist, this branch only registers actions with IDs like workbench.action.agentSessions.runScript.${i} and does not register the base workbench.action.agentSessions.runScript. The sessions auxiliary bar expects the primary split-button action to have the base ID (see src/vs/sessions/browser/parts/auxiliaryBarPart.ts:188-221), so the run split button may stop working once scripts are configured. Consider always registering a stable primary action with the base ID (e.g. runs the default/first script or opens a picker), and keep per-script actions as dropdown entries.
| async commitWorktreeFiles(session: IActiveSessionItem, fileUris: URI[]): Promise<void> { | ||
| const worktreeUri = session.worktree; | ||
| if (!worktreeUri) { | ||
| throw new Error('Cannot commit worktree files: active session has no associated worktree'); |
There was a problem hiding this comment.
commitWorktreeFiles throws a plain Error with a hard-coded English message. Since this can be triggered indirectly from UI flows (e.g. adding scripts / committing worktree edits), prefer a localized error (or a typed error that callers can translate/handle) and include actionable context (e.g. session id) or simply no-op when there is no worktree if that’s an expected state.
| throw new Error('Cannot commit worktree files: active session has no associated worktree'); | |
| throw new Error(localize( | |
| 'sessions.commitWorktreeFiles.noWorktree', | |
| "Cannot commit worktree files for session {0}: the active session has no associated worktree.", | |
| session.resource.toString() | |
| )); |
| if (this.viewMode !== 'editor' || !this.currentEditingIsWorktree) { | ||
| return; | ||
| } | ||
| this.currentWorktreeUri = getActiveSessionRoot(this.activeSessionService); | ||
| this.currentActiveSession = this.activeSessionService.getActiveSession() ?? undefined; | ||
| })); |
There was a problem hiding this comment.
While editing a worktree-backed file, this autorun overwrites currentActiveSession whenever the active session changes. That can cause the auto-commit on goBackToList() to commit the file against a different session/worktree than the one the file belongs to. Consider capturing the session/worktree once when the embedded editor is opened and not updating it, or validating that fileUri is inside session.worktree before committing.
| let navigationBearings = undefined; | ||
| for (const candidate of candidates) { | ||
| const sessionResource = agentFeedbackService.getMostRecentSessionForResource(candidate); | ||
| const sessionResource = getSessionForResource(candidate, chatEditingService, agentSessionsService); | ||
| if (sessionResource && agentFeedbackService.getFeedback(sessionResource).length > 0) { | ||
| shouldShow = true; | ||
| navigationBearings = agentFeedbackService.getNavigationBearing(sessionResource); | ||
| break; | ||
| } |
There was a problem hiding this comment.
getSessionForResource(...) returns the first session that contains the resource, but not necessarily a session that actually has feedback for it. If the same file is associated with multiple sessions, this can cause the overlay to hide even though another session has feedback. Consider using agentFeedbackService.getMostRecentSessionForResource(candidate) (which is feedback-aware) or extending the lookup to search for any session-with-feedback for the candidate resource.
Copilot Generated Description:Adjust session resource retrieval logic and update regex to permit a new character. Include fixes for agent feedback contributions and enhance session management integration.