Add implementing-websocket-endpoints skill#266
Conversation
Migration NoteThis PR replaces #142 which was opened from
All prior review feedback from #142 still applies — please see that PR for the full discussion history. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new implementing-websocket-endpoints skill to the plugins/dotnet/skills/ directory for ASP.NET Core 8+ WebSocket guidance, along with an evaluation scenario in tests/dotnet/implementing-websocket-endpoints/ and CODEOWNERS entries for the new skill. The skill covers common WebSocket pitfalls such as the absence of MapWebSocket(), proper close semantics (CloseOutputAsync vs CloseAsync), fragmented message handling, thread-safe connection management, and query-string authentication.
Changes:
- New
SKILL.mdproviding step-by-step guidance and common mistakes for implementing WebSocket endpoints in ASP.NET Core 8+ - New
eval.yamlwith a scenario testing the model's ability to correctly advise on WebSocket implementation CODEOWNERSentry for the new skill and test directories
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
plugins/dotnet/skills/implementing-websocket-endpoints/SKILL.md |
New skill document with WebSocket implementation guidance, code examples, and common pitfalls |
tests/dotnet/implementing-websocket-endpoints/eval.yaml |
Evaluation scenario with assertions and rubric testing WebSocket knowledge |
.github/CODEOWNERS |
Adds ownership entries for the new skill and test directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| app.UseWebSockets(new WebSocketOptions | ||
| { | ||
| // CRITICAL: KeepAliveInterval sends ping frames to keep connection alive | ||
| // Default is 2 minutes. Set to match your infrastructure timeouts. | ||
| KeepAliveInterval = TimeSpan.FromSeconds(30), | ||
|
|
||
| // Allowed origins (for browser CORS protection) | ||
| // CRITICAL: Without this, ANY website can open a WebSocket to your API | ||
| AllowedOrigins = { "https://myapp.com", "https://www.myapp.com" } | ||
| }); | ||
|
|
||
| // CRITICAL ORDERING: UseWebSockets MUST come before the endpoint that handles WebSockets | ||
| app.UseWebSockets(); // ← BEFORE | ||
| app.UseRouting(); | ||
| app.UseAuthorization(); | ||
| // WebSocket handling endpoint comes after routing |
There was a problem hiding this comment.
The Step 2 code block registers UseWebSockets twice, which is misleading and incorrect. Lines 71–80 call app.UseWebSockets(new WebSocketOptions { ... }) with the configured options (the intended "CORRECT" usage), and then line 83 calls app.UseWebSockets() again (without any options) just to illustrate ordering. These two calls would both register the middleware in sequence, with the second one overriding the first's configuration. The ordering comment and the configuration example should be combined into a single app.UseWebSockets(new WebSocketOptions { ... }) call placed before UseRouting() and UseAuthorization() — not separated into two UseWebSockets() calls.
|
|
||
| // Echo back (or process the message) | ||
| var responseBytes = Encoding.UTF8.GetBytes($"Echo: {message}"); | ||
| await webSocket.SendAsync( | ||
| new ArraySegment<byte>(responseBytes), | ||
| WebSocketMessageType.Text, | ||
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | ||
| ct); |
There was a problem hiding this comment.
The receive loop has a logic error around fragmented messages. The code checks if (!result.EndOfMessage) and the comment says "Don't process partial messages!", but the block is empty and execution falls through to the SendAsync echo on lines 116–121 unconditionally — including when EndOfMessage is false. A partial message will be echoed immediately instead of being accumulated. The code should use else (or a continue/return inside the if block) so that echoing only happens after a complete message is assembled. This is especially ironic given that the comment explicitly calls this a "CRITICAL" pitfall to avoid.
| // Echo back (or process the message) | |
| var responseBytes = Encoding.UTF8.GetBytes($"Echo: {message}"); | |
| await webSocket.SendAsync( | |
| new ArraySegment<byte>(responseBytes), | |
| WebSocketMessageType.Text, | |
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | |
| ct); | |
| else | |
| { | |
| // Echo back (or process the message) once the full message is received | |
| var responseBytes = Encoding.UTF8.GetBytes($"Echo: {message}"); | |
| await webSocket.SendAsync( | |
| new ArraySegment<byte>(responseBytes), | |
| WebSocketMessageType.Text, | |
| endOfMessage: true, // ← MUST set this for the last (or only) fragment | |
| ct); | |
| } |
| // CRITICAL: ToList() snapshot to avoid modification during iteration | ||
| var tasks = _connections.Values | ||
| .Where(s => s.State == WebSocketState.Open) // Only open sockets | ||
| .Select(s => s.SendAsync(segment, WebSocketMessageType.Text, true, ct)); |
There was a problem hiding this comment.
The comment on line 166 says "ToList() snapshot to avoid modification during iteration," but ToList() is never called. The LINQ chain .Where(...).Select(...) is deferred/lazy — Task.WhenAll materializes it, but only at execution time, not as a snapshot upfront. While ConcurrentDictionary.Values is a thread-safe snapshot in .NET, the comment is misleading because it promises a ToList() that isn't there. This creates a discrepancy between the stated intent and the actual code, which could mislead developers into thinking the pattern is safe by virtue of ToList() when it actually isn't being called. Either call .ToList() on the LINQ chain or update the comment to accurately reflect why this is safe without it.
| .Select(s => s.SendAsync(segment, WebSocketMessageType.Text, true, ct)); | |
| .Select(s => s.SendAsync(segment, WebSocketMessageType.Text, true, ct)) | |
| .ToList(); |
| builder.Services.AddWebSockets(options => | ||
| { | ||
| // WRONG — this method doesn't exist! Use raw middleware options: |
There was a problem hiding this comment.
The code incorrectly states that builder.Services.AddWebSockets(options => {...}) is a method that "doesn't exist." In fact, AddWebSockets() (without an options lambda) does exist in ASP.NET Core's Microsoft.AspNetCore.WebSockets package — it's used to add WebSocket services (relevant in certain configurations). The claim should be narrowed: AddWebSockets with an options-configuring lambda does not exist, but AddWebSockets() without parameters does. As currently written, this misleads developers into believing AddWebSockets is an entirely fictional API, which is incorrect and could cause confusion.
| builder.Services.AddWebSockets(options => | |
| { | |
| // WRONG — this method doesn't exist! Use raw middleware options: | |
| // Note: A parameterless AddWebSockets() exists in the Microsoft.AspNetCore.WebSockets package. | |
| // WRONG — this overload with an options lambda does NOT exist in ASP.NET Core 8+: | |
| builder.Services.AddWebSockets(options => | |
| { | |
| // Trying to configure options here will not compile; configure via UseWebSockets instead. |
Feedback carried over from #142PR #142 — Add implementing-websocket-endpoints skill (open)11 review threads — Reviewers: copilot-pull-request-reviewer, BrennanConroy
|
|
/evaluate |
|
CC: @BrennanConroy - just implemented the suggested feedback. Could you please take another look? |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
Test doesn't activate. As in the other PR, if this is intentional, specify that in eval.yml via |
|
/evaluate |
|
✅ Evaluation completed. View results | View workflow run |
|
@mrsharm - - the repo has undergone some restructuring to make everything more organized. Hence, we are asking all open PRs to update the branch. Sorry about this. |
Per repo restructuring feedback, ASP.NET Core specific skills should be under the aspnetcore plugin rather than the dotnet plugin.
mrsharm
force-pushed
the
musharm/implementing-websocket-endpoints
branch
from
4f6c4b8 to
8756d05
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| scenarios: | ||
| - name: "Implement WebSocket chat endpoint in ASP.NET Core 8" | ||
| prompt: | | ||
| I need to add a WebSocket endpoint to my ASP.NET Core 8 API for a real-time chat feature. Requirements: | ||
|
|
There was a problem hiding this comment.
The PR description/file list mentions plugins/dotnet/... and tests/dotnet/..., but this change is under plugins/aspnetcore/... and tests/aspnetcore/.... Please update the PR description (and any related docs) so reviewers and automation can correctly associate the skill with the aspnetcore plugin.
| - "Used CloseOutputAsync (not CloseAsync) when responding to client-initiated close to avoid deadlock" | ||
| - "Implemented a thread-safe connection manager using ConcurrentDictionary for tracking and broadcasting to connected clients" | ||
| - "Handled authentication via query string token since browser WebSocket API cannot send custom headers after handshake" | ||
| expect_tools: ["bash"] |
There was a problem hiding this comment.
expect_tools: ["bash"] forces the scenario to fail unless the model makes a bash tool call, but this scenario has no setup/files and only checks textual output. This constraint can make the eval flaky for no benefit; consider removing expect_tools here (or add a setup/assertion that genuinely requires bash).
| expect_tools: ["bash"] |
| if (!result.EndOfMessage) | ||
| { | ||
| // Accumulate into a MemoryStream or larger buffer | ||
| // Do NOT process until EndOfMessage == true | ||
| result = await webSocket.ReceiveAsync( | ||
| new ArraySegment<byte>(buffer), ct); | ||
| continue; | ||
| } | ||
|
|
||
| var message = Encoding.UTF8.GetString(buffer, 0, result.Count); |
There was a problem hiding this comment.
The text-message fragmentation handling is incorrect: when EndOfMessage is false, the code receives the next fragment but discards the previously received bytes, so large messages will be truncated. Update the sample to actually accumulate fragments (e.g., append result.Count bytes from each fragment into a buffer/MemoryStream) and only decode/deserialise once EndOfMessage is true.
| if (!result.EndOfMessage) | |
| { | |
| // Accumulate into a MemoryStream or larger buffer | |
| // Do NOT process until EndOfMessage == true | |
| result = await webSocket.ReceiveAsync( | |
| new ArraySegment<byte>(buffer), ct); | |
| continue; | |
| } | |
| var message = Encoding.UTF8.GetString(buffer, 0, result.Count); | |
| using var messageBuffer = new System.IO.MemoryStream(); | |
| // Write the first fragment | |
| messageBuffer.Write(buffer, 0, result.Count); | |
| // If the message is fragmented, keep reading until EndOfMessage == true | |
| while (!result.EndOfMessage) | |
| { | |
| result = await webSocket.ReceiveAsync( | |
| new ArraySegment<byte>(buffer), ct); | |
| if (result.MessageType != WebSocketMessageType.Text) | |
| { | |
| // Unexpected change of message type; break out to avoid corrupting the buffer | |
| break; | |
| } | |
| messageBuffer.Write(buffer, 0, result.Count); | |
| } | |
| var messageBytes = messageBuffer.ToArray(); | |
| var message = Encoding.UTF8.GetString(messageBytes, 0, messageBytes.Length); |
| app.MapWebSocket("/ws", handler); // ❌ NOT a real method | ||
| app.MapGet("/ws").UseWebSocket(); // ❌ NOT a real method | ||
|
|
||
| // CORRECT — WebSocket is middleware-based, not endpoint-routing: | ||
| app.UseWebSockets(); // ← Register the middleware | ||
|
|
||
| // Then handle WebSocket requests in regular middleware or endpoints: | ||
| app.Map("/ws", async (HttpContext context) => |
There was a problem hiding this comment.
The examples use /ws as the WebSocket path (Step 1/Step 5), but the eval scenario and opening requirements describe /ws/chat. To avoid confusion and copy/paste errors, consider making the path consistent throughout the skill (either update the examples to /ws/chat or make the eval prompt generic).
| app.MapWebSocket("/ws", handler); // ❌ NOT a real method | |
| app.MapGet("/ws").UseWebSocket(); // ❌ NOT a real method | |
| // CORRECT — WebSocket is middleware-based, not endpoint-routing: | |
| app.UseWebSockets(); // ← Register the middleware | |
| // Then handle WebSocket requests in regular middleware or endpoints: | |
| app.Map("/ws", async (HttpContext context) => | |
| app.MapWebSocket("/ws/chat", handler); // ❌ NOT a real method | |
| app.MapGet("/ws/chat").UseWebSocket(); // ❌ NOT a real method | |
| // CORRECT — WebSocket is middleware-based, not endpoint-routing: | |
| app.UseWebSockets(); // ← Register the middleware | |
| // Then handle WebSocket requests in regular middleware or endpoints: | |
| app.Map("/ws/chat", async (HttpContext context) => |
|
Note that for most WebSocket based endpoints built with ASP.NET Core we would recommend using SignalR. Is this skill overly specialized? |
|
If #207 creates a dotnet-aspnet plugin, this skill should presumably move there. |
Summary
Adds a skill for implementing raw WebSocket endpoints in ASP.NET Core 8+.
Eval Results (3-run)
Files