Add implementing-server-sent-events skill - #147
Conversation
Eval Results: implementing-server-sent-events3-Run Validation: +13.6% PASS
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) |
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill intended to guide implementation of Server-Sent Events (SSE) endpoints in ASP.NET Core 8 minimal APIs, along with an evaluation scenario to validate the skill’s impact.
Changes:
- Introduces the
implementing-server-sent-eventsskill documentation (SSE headers, framing, flushing, disconnect handling, reconnection). - Adds a new
eval.yamlscenario/rubric for the skill.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/dotnet/tests/implementing-server-sent-events/eval.yaml | Adds an evaluation scenario and rubric for SSE endpoint implementation. |
| src/dotnet/skills/implementing-server-sent-events/SKILL.md | Adds a new skill doc describing how to implement SSE in ASP.NET Core minimal APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| context.Response.Headers.ContentType = "text/event-stream"; | ||
| context.Response.Headers.CacheControl = "no-cache"; |
There was a problem hiding this comment.
Same compile issue here: context.Response.Headers.ContentType / .CacheControl are not valid APIs on IHeaderDictionary. Update this snippet to use context.Response.ContentType / Headers["Cache-Control"] (or typed headers) so the example builds.
| 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"; |
| context.Response.Headers.ContentType = "text/event-stream"; | ||
| context.Response.Headers.CacheControl = "no-cache"; | ||
| context.Response.Headers["Connection"] = "keep-alive"; |
There was a problem hiding this comment.
Same header API issue in this snippet: context.Response.Headers.ContentType / .CacheControl won’t compile. Please update consistently across all examples (Step 5/6) to avoid copy/paste failures.
| // Option B: Use StreamWriter with AutoFlush = true | ||
| await using var writer = new StreamWriter(context.Response.Body, leaveOpen: true); | ||
| 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.
StreamWriter.WriteLineAsync uses the platform newline by default (often \r\n), and this example also appends an extra \n, which can produce confusing line endings for SSE. Prefer writing explicit \n (or set writer.NewLine = "\n" and use WriteAsync) to keep the SSE framing unambiguous cross-platform.
| await writer.WriteLineAsync($"data: {msg}\n"); // Note: WriteLine adds one \n, we add one more | |
| await writer.WriteAsync($"data: {msg}\n\n"); // Explicit \n\n for SSE event framing |
| 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.
int.Parse(lastEventId) will throw if the client sends a non-integer Last-Event-ID header (or an out-of-range value), turning a reconnect into a 500. Use int.TryParse and fall back to a safe default (or return 400) to make the reconnection example robust.
| var startFrom = lastEventId != null ? int.Parse(lastEventId) + 1 : 0; | |
| var startFrom = 0; | |
| if (!string.IsNullOrEmpty(lastEventId) && int.TryParse(lastEventId, out var parsedId) && parsedId >= 0) | |
| { | |
| startFrom = parsedId + 1; | |
| } |
| ## When Not to Use | ||
| - Bidirectional communication needed → use WebSockets | ||
| - Binary data streaming → use WebSockets or gRPC streaming | ||
| - Need more than 6 concurrent connections per domain in HTTP/1.1 → use HTTP/2 | ||
|
|
There was a problem hiding this comment.
This skill doc doesn’t include an explicit “Validation” section/checklist. CONTRIBUTING.md asks that changes include validation steps a reviewer can follow; consider adding a short section (e.g., how to run the sample, curl an SSE stream, verify Last-Event-ID behavior, and test via nginx) so the skill is verifiable.
| scenarios: | ||
| - name: "Implement SSE notification endpoint in ASP.NET Core 8 minimal API" |
There was a problem hiding this comment.
This eval file is under src/dotnet/tests/..., but repo convention (and CI) expects evals at tests/<plugin>/<skill-name>/eval.yaml (e.g. tests/dotnet/implementing-server-sent-events/eval.yaml). With the current location, the evaluation workflow that runs --tests-dir ./tests/dotnet will not pick up these 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.
This SKILL.md is placed under src/dotnet/skills/..., but the repository layout expects skills under plugins/<plugin>/skills/<skill-name>/SKILL.md so they are packaged and discovered (e.g. plugins/dotnet/skills/implementing-server-sent-events/SKILL.md). As-is, the evaluation workflow that validates ./plugins/dotnet/skills will not discover or test this skill.
| context.Response.Headers.ContentType = "text/event-stream"; | ||
| context.Response.Headers.CacheControl = "no-cache"; |
There was a problem hiding this comment.
These snippets use context.Response.Headers.ContentType / .CacheControl, but HttpResponse.Headers is an IHeaderDictionary and doesn't expose those properties. This will not compile in ASP.NET Core; set context.Response.ContentType (or Headers["Content-Type"]) and Headers["Cache-Control"] (or use context.Response.GetTypedHeaders()).
| 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"; |
|
|
||
| # Implementing Server-Sent Events (SSE) in ASP.NET Core | ||
|
|
||
| ## When to Use |
There was a problem hiding this comment.
move when to use and when not to use entirely or largely into the description, which is the thing the ai reads when deciding whether or not to load. probably shouldn't have this section here.
| // Each event MUST end with TWO newlines (\n\n) | ||
|
|
||
| // Simple data event: | ||
| await context.Response.WriteAsync($"data: {message}\n\n", ct); |
There was a problem hiding this comment.
should {message} be sanitized for newlines here and below. AI says - risk of injection attack
|
needs CODEOWNERS entry |
|
|
||
| # Implementing Server-Sent Events (SSE) in ASP.NET Core | ||
|
|
||
| ## When to Use |
There was a problem hiding this comment.
SSE endpoints hold long-lived HTTP connections. Without connection limits, an attacker (or misbehaving client) can exhaust server resources by opening many concurrent connections. Consider adding a note about:
- Enforcing per-client or total connection limits (e.g., via middleware or a
SemaphoreSlim) - Requiring authentication before establishing the SSE connection
- Setting an idle timeout to drop stale connections
This is especially relevant for a skill doc since agents will copy the pattern as-is without thinking about production hardening.
| 4. **Not handling RequestAborted**: The loop runs forever if you don't check `context.RequestAborted`. `OperationCanceledException` on disconnect is normal. | ||
| 5. **Returning a result after streaming**: Don't `return Results.Ok()` after writing SSE events — the response body is already being written. | ||
| 6. **Missing Content-Type header**: Must be exactly `text/event-stream`, not `application/json` or `text/plain`. | ||
| 7. **Missing X-Accel-Buffering: no**: Reverse proxies (nginx) buffer responses by default, breaking SSE. |
There was a problem hiding this comment.
Worth adding a note about CORS configuration. SSE endpoints are frequently consumed cross-origin via the browser EventSource API, which does not support custom headers. Without app.UseCors(...) or a CORS policy on the endpoint, cross-origin clients will silently fail. This is a common deployment surprise.
|
|
||
| ### Step 1: CRITICAL — There Is No Built-In MapSSE() or MapServerSentEvents() | ||
|
|
||
| ASP.NET Core has NO built-in SSE endpoint helper. You must manually write the SSE protocol. |
There was a problem hiding this comment.
Wrong. We have TypedResults.ServerSentEvents
https://learn.microsoft.com/en-us/aspnet/core/release-notes/aspnetcore-10.0?view=aspnetcore-10.0#support-for-server-sent-events-sse
Most of this Skill should probably be rewritten to use TypedResults.ServerSentEvents
|
Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. |
New Skill: implementing-server-sent-events
Adds a skill for implementing Server-Sent Events (SSE) endpoints in ASP.NET Core 8 minimal APIs.
Key Gotchas Covered
\n\n) to terminate eventsHttpContext.RequestAbortedfor clean disconnect handlingX-Accel-Buffering: nofor reverse proxy compatibilityLast-Event-IDheader for client reconnectionEval Results (3-run validation)