Add implementing-rate-limiting skill - #131
Conversation
Eval Results (3-run validation)Model: claude-opus-4.6 (baseline & skill) Scenario: Add per-client rate limiting to an ASP.NET Core API
Key Improvements
1-run screening
|
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill and evaluation scenario focused on implementing ASP.NET Core’s built-in rate limiting correctly (status codes, middleware ordering, partitioning, policies, and rejection handling).
Changes:
- Added a new eval scenario to validate responses include key rate-limiting configuration concepts (policies, 429 responses, endpoint opt-out).
- Added a new
implementing-rate-limitingskill document describing algorithms, configuration, and common pitfalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/dotnet/tests/implementing-rate-limiting/eval.yaml | New evaluation scenario for per-client + per-endpoint rate limiting requirements. |
| src/dotnet/skills/implementing-rate-limiting/SKILL.md | New skill content covering built-in rate limiting setup, middleware ordering, and partitioning patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```skill | ||
| --- | ||
| name: implementing-rate-limiting | ||
| description: Implement .NET 7+ built-in rate limiting middleware with correct algorithm selection, partitioning, and response handling. Use when adding API rate limiting without a third-party library. | ||
| --- | ||
|
|
There was a problem hiding this comment.
The file is wrapped in an outer fenced code block (skill ... ), which prevents the YAML frontmatter from being detected/stripped (SkillProfiler expects frontmatter to start at the beginning of the file with ---). It also makes the entire document a single code block, so the internal csharp fences won’t render as code blocks. Remove the outer skill wrapper and start the file directly with the --- frontmatter (like other skills in this repo).
| // Global rate limiter: fixed window | ||
| options.GlobalLimiter = PartitionedRateLimiter.Create<HttpContext, string>(context => | ||
| { | ||
| return RateLimitPartition.GetFixedWindowLimiter( | ||
| partitionKey: context.Connection.RemoteIpAddress?.ToString() ?? "unknown", | ||
| factory: _ => new FixedWindowRateLimiterOptions | ||
| { | ||
| PermitLimit = 100, | ||
| Window = TimeSpan.FromMinutes(1), | ||
| QueueProcessingOrder = QueueProcessingOrder.OldestFirst, | ||
| QueueLimit = 0 // Reject immediately, don't queue | ||
| }); | ||
| }); | ||
|
|
||
| // Named policy: sliding window for sensitive endpoints | ||
| options.AddSlidingWindowLimiter("api-sensitive", slidingOptions => | ||
| { | ||
| slidingOptions.PermitLimit = 10; | ||
| slidingOptions.Window = TimeSpan.FromMinutes(1); | ||
| slidingOptions.SegmentsPerWindow = 6; // 10-second segments | ||
| slidingOptions.QueueLimit = 0; |
There was a problem hiding this comment.
This section emphasizes fixed window’s burst problem, but the example immediately configures the global limiter as a fixed window. That inconsistency can steer readers toward the exact pitfall the skill warns about and may conflict with the eval rubric’s preference for sliding window/token bucket. Consider making the global limiter sliding window (or token bucket) in the main example, or explicitly justify why fixed window is acceptable for the global policy here.
| // Global rate limiter: fixed window | |
| options.GlobalLimiter = PartitionedRateLimiter.Create<HttpContext, string>(context => | |
| { | |
| return RateLimitPartition.GetFixedWindowLimiter( | |
| partitionKey: context.Connection.RemoteIpAddress?.ToString() ?? "unknown", | |
| factory: _ => new FixedWindowRateLimiterOptions | |
| { | |
| PermitLimit = 100, | |
| Window = TimeSpan.FromMinutes(1), | |
| QueueProcessingOrder = QueueProcessingOrder.OldestFirst, | |
| QueueLimit = 0 // Reject immediately, don't queue | |
| }); | |
| }); | |
| // Named policy: sliding window for sensitive endpoints | |
| options.AddSlidingWindowLimiter("api-sensitive", slidingOptions => | |
| { | |
| slidingOptions.PermitLimit = 10; | |
| slidingOptions.Window = TimeSpan.FromMinutes(1); | |
| slidingOptions.SegmentsPerWindow = 6; // 10-second segments | |
| slidingOptions.QueueLimit = 0; | |
| // Global rate limiter: sliding window for smoother distribution across clients | |
| options.GlobalLimiter = PartitionedRateLimiter.Create<HttpContext, string>(context => | |
| { | |
| return RateLimitPartition.GetSlidingWindowLimiter( | |
| partitionKey: context.Connection.RemoteIpAddress?.ToString() ?? "unknown", | |
| factory: _ => new SlidingWindowRateLimiterOptions | |
| { | |
| PermitLimit = 100, | |
| Window = TimeSpan.FromMinutes(1), | |
| SegmentsPerWindow = 6, // 10-second segments | |
| QueueProcessingOrder = QueueProcessingOrder.OldestFirst, | |
| QueueLimit = 0 // Reject immediately, don't queue | |
| }); | |
| }); | |
| // Named policy: token bucket for sensitive endpoints that can tolerate short bursts | |
| options.AddTokenBucketLimiter("api-sensitive", tokenOptions => | |
| { | |
| tokenOptions.TokenLimit = 10; // max burst size | |
| tokenOptions.TokensPerPeriod = 10; | |
| tokenOptions.ReplenishmentPeriod = TimeSpan.FromMinutes(1); | |
| tokenOptions.AutoReplenishment = true; | |
| tokenOptions.QueueProcessingOrder = QueueProcessingOrder.OldestFirst; | |
| tokenOptions.QueueLimit = 0; |
| options.AddPolicy("per-user", context => | ||
| { | ||
| var userId = context.User?.FindFirst(ClaimTypes.NameIdentifier)?.Value; | ||
|
|
There was a problem hiding this comment.
The per-user partitioning snippet uses ClaimTypes.NameIdentifier but doesn’t include the required using System.Security.Claims; (and it isn’t referenced earlier in the shown usings). To keep the snippet copy/pasteable, add that using (or fully-qualify System.Security.Claims.ClaimTypes).
| - type: "output_matches" | ||
| pattern: "(RequireRateLimiting|EnableRateLimiting)" | ||
| - type: "output_matches" | ||
| pattern: "(DisableRateLimiting|healthz)" |
There was a problem hiding this comment.
The health-check assertion is currently too permissive: (DisableRateLimiting|healthz) will pass if the model merely mentions healthz without actually disabling rate limiting. Consider splitting this into two assertions (one requiring DisableRateLimiting, one requiring healthz), and add an assertion that checks for setting a Retry-After header (e.g., Retry-After or MetadataName.RetryAfter) since that’s a stated requirement/rubric item.
| pattern: "(DisableRateLimiting|healthz)" | |
| pattern: "DisableRateLimiting" | |
| - type: "output_matches" | |
| pattern: "healthz" | |
| - type: "output_matches" | |
| pattern: "(Retry-After|MetadataName.RetryAfter)" |
|
|
||
| # Implementing Rate Limiting in ASP.NET Core (.NET 7+) | ||
|
|
||
| ## When to Use |
There was a problem hiding this comment.
move when to use/not use into description to enable lazy loading.
| @@ -0,0 +1,213 @@ | |||
| ```skill | |||
There was a problem hiding this comment.
and update to match directory structure in main
| @@ -0,0 +1,213 @@ | |||
| ```skill | |||
There was a problem hiding this comment.
this is wrapped in skill markdown block
this is wrong I think - remove it.
| ### Step 4: Per-user/per-tenant partitioning | ||
|
|
||
| ```csharp | ||
| // Per-authenticated-user rate limit |
There was a problem hiding this comment.
I guess you need using System.Security.Claims;
| // Custom response for rejected requests | ||
| options.OnRejected = async (context, cancellationToken) => | ||
| { | ||
| context.HttpContext.Response.StatusCode = StatusCodes.Status429TooManyRequests; |
There was a problem hiding this comment.
is this line needed given its set by default above to 429?
| @@ -0,0 +1,29 @@ | |||
| scenarios: | |||
There was a problem hiding this comment.
seems like we should have more than 1 scenario to cover more of the skill as there's several different directions to go.
| ```skill | ||
| --- | ||
| name: implementing-rate-limiting | ||
| description: Implement .NET 7+ built-in rate limiting middleware with correct algorithm selection, partitioning, and response handling. Use when adding API rate limiting without a third-party library. |
There was a problem hiding this comment.
| description: Implement .NET 7+ built-in rate limiting middleware with correct algorithm selection, partitioning, and response handling. Use when adding API rate limiting without a third-party library. | |
| description: Implement .NET 7+ built-in rate limiting (Microsoft.AspNetCore.RateLimiting / System.Threading.RateLimiting) with correct algorithm selection, partitioning, and response handling. Use when adding API rate limiting to ASP.NET Core without a third-party library. Covers fixed window, sliding window, token bucket, and concurrency limiter; middleware ordering; 429 status codes; per-client partitioning; and Retry-After headers. Not for distributed/multi-instance rate limiting (use AspNetCoreRateLimit with Redis or a gateway instead), gateway-layer limiting (YARP, nginx, Azure API Management), or pre-.NET 7 projects. |
this gives it more "keywords" to help activate it appropriately
There was a problem hiding this comment.
I think the use/do not use below can now be removed as it's all here
| // Rate limiting middleware MUST be after routing but before endpoint execution | ||
| // WRONG order will cause it to silently not apply to endpoints | ||
| app.UseRouting(); // must come first | ||
| app.UseRateLimiter(); // ← HERE — after UseRouting, before MapControllers/MapGet |
There was a problem hiding this comment.
Consider moving after UseAuthorization so you can rate limit based on user information.
| | Missing `RequireRateLimiting()` on endpoints | Global limiter works but named policies do nothing | Apply policies to endpoints explicitly | | ||
| | `AutoReplenishment = false` on token bucket | Tokens never replenish, all requests rejected after initial burst | Set `AutoReplenishment = true` (or use a background timer) | | ||
| | `QueueLimit > 0` without timeout | Requests queue indefinitely under sustained overload | Set `QueueLimit = 0` to reject immediately, or impose a timeout | | ||
| | Partitioning by `RemoteIpAddress` behind proxy | All requests share one IP (the proxy) | Partition by `X-Forwarded-For` header or authenticated user | |
There was a problem hiding this comment.
The code above uses RemoteIpAddress
|
Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. |
- Fix X-Forwarded-For spoofing: use RemoteIpAddress with ForwardedHeaders guidance - Fix Headers.RetryAfter to Headers["Retry-After"] indexer - Remove redundant StatusCode=429 in OnRejected (already set via RejectionStatusCode) - Split health-check assertion into two (DisableRateLimiting + /healthz) - Remove brittle expect_tools: ["bash"] - Update proxy partitioning guidance in mistakes table and validation checklist - Add CODEOWNERS entry for aspnetcore plugin
Skill: implementing-rate-limiting
Teaches the model to implement .NET 8 built-in rate limiting correctly, covering:
Eval Results (3-run)
Overall: +39.4% improvement - PASSED