Add implementing-server-sent-events skill#265
Conversation
Migration NoteThis PR replaces #147 which was opened from
All prior review feedback from #147 still applies — please see that PR for the full discussion history. |
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill documenting how to implement Server-Sent Events (SSE) endpoints in ASP.NET Core 8 minimal APIs, along with an evaluation scenario and CODEOWNERS entries to integrate it into the repo’s skill + eval workflow.
Changes:
- Added the
implementing-server-sent-eventsskill documentation underplugins/dotnet/skills/. - Added an eval scenario under
tests/dotnet/implementing-server-sent-events/. - Registered ownership for the new skill and eval paths in
.github/CODEOWNERS.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/dotnet/skills/implementing-server-sent-events/SKILL.md | New skill doc explaining SSE requirements, formatting, flushing, disconnect handling, and reconnection. |
| tests/dotnet/implementing-server-sent-events/eval.yaml | New eval scenario prompting an SSE endpoint implementation and scoring rubric expectations. |
| .github/CODEOWNERS | Assigns owners for the new skill and its eval scenario directory. |
💡 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.
| | Input | Required | Description | | ||
| |-------|----------|-------------| | ||
| | Event data source | Yes | The data to stream (IAsyncEnumerable, Channel, timer, etc.) | | ||
| | Event types | No | Named event types for `event:` field | | ||
| | Client reconnection | No | Whether to support Last-Event-ID reconnection | | ||
|
|
There was a problem hiding this comment.
The markdown table under "## Inputs" is malformed (it uses double pipes like || Input | Required | Description |), so it won’t render correctly. Please switch to the standard table format used in other skills (single | delimiters with a separator row).
| writer.AutoFlush = true; // Flushes after every Write | ||
| await writer.WriteLineAsync($"data: {msg}\n"); // Note: WriteLine adds one \n, we add one more |
There was a problem hiding this comment.
The StreamWriter example is misleading: WriteLineAsync appends TextWriter.NewLine (often \r\n on Windows), not a single \n as the comment states. For SSE it’s safer to explicitly write \n\n (or set writer.NewLine = "\n") so event framing is consistent across OSes.
| writer.AutoFlush = true; // Flushes after every Write | |
| await writer.WriteLineAsync($"data: {msg}\n"); // Note: WriteLine adds one \n, we add one more | |
| writer.AutoFlush = true; // Flushes after every Write | |
| writer.NewLine = "\n"; // Ensure consistent '\n' line endings across OSes | |
| await writer.WriteLineAsync($"data: {msg}"); // Writes "data" line ending with '\n' | |
| await writer.WriteLineAsync(); // Writes the blank line that terminates the SSE event |
| await context.Response.WriteAsync($"retry: 5000\n\n"); | ||
| await context.Response.Body.FlushAsync(); | ||
|
|
||
| var startFrom = lastEventId != null ? int.Parse(lastEventId) + 1 : 0; |
There was a problem hiding this comment.
This reconnection example assumes Last-Event-ID is an int (int.Parse(lastEventId)), but SSE event IDs are defined as opaque strings and the header can contain non-numeric values. Prefer treating it as a string (or using int.TryParse if you want to keep the numeric example) to avoid throwing and to better match the SSE spec.
| var startFrom = lastEventId != null ? int.Parse(lastEventId) + 1 : 0; | |
| var startFrom = 0; | |
| if (!string.IsNullOrEmpty(lastEventId) && int.TryParse(lastEventId, out var parsedId)) | |
| { | |
| startFrom = parsedId + 1; | |
| } |
Feedback carried over from #147Code Review CommentsCopilot on src/dotnet/skills/implementing-server-sent-events/SKILL.md (line 110): Same compile issue here: Discussion Commentsmrsharm: ## Eval Results: implementing-server-sent-events ### 3-Run Validation: +13.6% PASS | Metric | Baseline | With Skill | Change | |--------|----------|------------|--------| | Overall Score | 4.0/5 | 4.7/5 | +0.7 | | Quality (overall) | 4.0/5 | 4.7/5 | +40% | | Pairwise | ΓÇö | skill wins | consistent | The skill consistently improves SSE implementations by teaching the manual protocol (no built-in helper exists), proper flush behavior, disconnection handling, and reconnection support. Model: claude-opus-4.6 (baseline + skill + judge) --- danmoseley: needs CODEOWNERS entry --- |
Feedback carried over from #147Code Review Comments
Discussion
|
|
@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.
fd6c846 to
8654bb1
Compare
Per repo restructuring feedback, ASP.NET Core specific skills should be under the aspnetcore plugin rather than the dotnet plugin.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 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.
| // COMMON MISTAKE: Using file.CopyToAsync for very large files | ||
| // IFormFile buffers everything in memory first — can cause OutOfMemoryException |
There was a problem hiding this comment.
This note says IFormFile buffers uploads "everything in memory first", but earlier in the same section it correctly notes buffering to a temp file. The in-memory claim is misleading and could cause readers to draw the wrong conclusions about resource usage; please adjust it to reflect the actual buffering behavior (memory up to a threshold, then temp file) and/or focus the warning on when to prefer MultipartReader.
| // COMMON MISTAKE: Using file.CopyToAsync for very large files | |
| // IFormFile buffers everything in memory first — can cause OutOfMemoryException | |
| // COMMON MISTAKE: Using file.CopyToAsync with IFormFile for very large files | |
| // IFormFile uses buffering (memory + temp files) during model binding; for true streaming | |
| // of very large uploads, prefer MultipartReader as shown above to reduce memory/disk usage. |
| ## Files | ||
| - `plugins/aspnetcore/plugin.json` — ASP.NET Core plugin | ||
| - `plugins/aspnetcore/skills/implementing-server-sent-events/SKILL.md` — skill instructions | ||
| - `tests/aspnetcore/implementing-server-sent-events/eval.yaml` — 1 eval scenario (needs expansion) |
There was a problem hiding this comment.
This PR description file claims plugins/aspnetcore/plugin.json is part of the change and that the eval has "1 eval scenario (needs expansion)", but the PR as provided does not add plugins/aspnetcore/plugin.json, and the eval.yaml now contains multiple scenarios. Please update or remove this description file so it matches the actual changed files/structure.
| ## Files | ||
| - `plugins/aspnetcore/plugin.json` — new ASP.NET Core plugin | ||
| - `plugins/aspnetcore/skills/implementing-rate-limiting/SKILL.md` — skill instructions | ||
| - `tests/aspnetcore/implementing-rate-limiting/eval.yaml` — 5 eval scenarios |
There was a problem hiding this comment.
PR-267-description.md documents an "implementing-rate-limiting" skill and references files that are not part of this PR. Unless these PR-xxx description artifacts are intentionally being checked in for another purpose, they add noise and make it harder to review the actual changes; consider removing them from this PR or ensuring they correspond to included code.
| - `tests/aspnetcore/implementing-rate-limiting/eval.yaml` — 5 eval scenarios |
| --- | ||
| name: implementing-server-sent-events | ||
| description: Implement Server-Sent Events (SSE) endpoints in ASP.NET Core. Use when building real-time streaming from server to client without WebSockets. | ||
| --- |
There was a problem hiding this comment.
The new aspnetcore plugin directory under plugins/aspnetcore/ does not include a plugin.json, unlike the other plugin roots (e.g., plugins/dotnet/plugin.json, plugins/dotnet-data/plugin.json). Adding plugins/aspnetcore/plugin.json is important for plugin manifest validation/discovery and to keep the repo’s plugin structure consistent.
| --- | ||
| name: minimal-api-file-upload | ||
| description: > | ||
| Implement file upload endpoints in ASP.NET Core minimal APIs (.NET 8+). USE FOR: handling IFormFile | ||
| and IFormFileCollection parameters, configuring dual size limits (Kestrel + FormOptions), disabling | ||
| anti-forgery on upload endpoints, validating file content via magic bytes, safe filename generation, | ||
| streaming large files with MultipartReader. DO NOT USE FOR: MVC controller file uploads ([FromForm] | ||
| works directly with attributes), simple JSON body endpoints, very large files over 1GB (use streaming | ||
| with MultipartReader instead of IFormFile). | ||
| --- |
There was a problem hiding this comment.
The PR metadata/description indicates this change is only adding the implementing-server-sent-events skill, but this PR also introduces a second new skill (minimal-api-file-upload) and its evals. Please either update the PR title/description to reflect both skills or split this into separate PRs to keep review/eval attribution clear.
| context.Response.Headers.ContentType = "text/event-stream"; | ||
| context.Response.Headers.CacheControl = "no-cache"; |
There was a problem hiding this comment.
The ASP.NET Core examples set SSE headers via context.Response.Headers.ContentType / .CacheControl, but IHeaderDictionary doesn't expose these properties (this won't compile). Use context.Response.ContentType = "text/event-stream" and set cache-control via context.Response.Headers["Cache-Control"] = "no-cache" (or GetTypedHeaders().CacheControl). This pattern appears multiple times in this document, so update all occurrences for consistency.
| context.Response.Headers.ContentType = "text/event-stream"; | |
| context.Response.Headers.CacheControl = "no-cache"; | |
| context.Response.ContentType = "text/event-stream"; | |
| context.Response.Headers["Cache-Control"] = "no-cache"; |
| continue; | ||
|
|
||
| var fileName = disposition.FileName.Value; | ||
| var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(fileName)}"; |
There was a problem hiding this comment.
In the streaming example, the saved filename extension comes from Path.GetExtension(fileName) which is user-controlled input (and contradicts the earlier guidance to derive/validate extensions based on allowed types). Consider either fixing the extension based on validated content/allowlist, or omit the extension entirely to avoid trusting client-provided names.
| var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(fileName)}"; | |
| var safeFile = Guid.NewGuid().ToString(); |
BrennanConroy
left a comment
There was a problem hiding this comment.
Like I said on the original PR, there is built-in SSE support in 10.0.
https://learn.microsoft.com/en-us/aspnet/core/release-notes/aspnetcore-10.0?view=aspnetcore-10.0#support-for-server-sent-events-sse
And if you don't want to use that, you can use the SsEFormatter type to write SSE frames from System.Net.ServerSentEvents nupkg.
|
If #207 creates a dotnet-aspnet plugin, this skill should presumably move there. |
Summary
Adds a skill for implementing Server-Sent Events endpoints in ASP.NET Core 8 minimal APIs.
Eval Results (3-run)
Files