fix and enable agents window smoke tests#317764
Open
sandy081 wants to merge 4 commits into
Open
Conversation
Async commit-wait flows (_sendFirstChat, _sendFirstChatViaController, _sendSubsequentChat) unconditionally cleared _currentNewSession on completion. When a newer session was created while the previous one was still awaiting commit, the clear stomped the newer session's pointer — causing 'Session not found' errors on the next send. Extract _clearCurrentNewSessionIfMatch() that only clears when the value still points at the session that initiated the async flow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to re-enable and stabilize the Agents Window smoke tests by improving the mock LLM server integration and addressing a race in Copilot session creation/commit flows in the Agents Window provider.
Changes:
- Enable the previously skipped Claude and Local Agents Window smoke tests and rename the test cases for clarity.
- Route mock LLM server request logging through the smoke test logger to improve diagnosability.
- Add a guarded clearing helper for
_currentNewSessionand apply it to several async commit/cancellation paths to prevent races that dispose a newer session.
Show a summary per file
| File | Description |
|---|---|
| test/smoke/src/areas/agentsWindow/agentsWindow.test.ts | Enables Claude/Local smoke coverage and wires mock server logging into the smoke logger. |
| src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts | Prevents _currentNewSession from being cleared/disposed by stale async completions (partial coverage; see PR comment). |
| scripts/chat-simulation/common/mock-llm-server.js | Adds optional logger injection and uses it for request/debug output instead of direct console calls. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
Comment on lines
+1752
to
+1759
| private _clearCurrentNewSessionIfMatch(session: NewSession, leak?: boolean): void { | ||
| if (this._currentNewSession.value === session) { | ||
| if (leak) { | ||
| this._currentNewSession.clearAndLeak(); | ||
| } else { | ||
| this._currentNewSession.clear(); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.