Conversation
Migration NoteThis PR replaces #131 which was opened from
All prior review feedback from #131 still applies — please see that PR for the full discussion history. |
There was a problem hiding this comment.
Pull request overview
Adds a new dotnet skill and accompanying eval scenario to teach/configure ASP.NET Core built-in rate limiting (partitioning, middleware ordering, correct 429 handling, and endpoint policy application).
Changes:
- Added
implementing-rate-limitingskill documentation underplugins/dotnet/skills/. - Added an evaluation scenario under
tests/dotnet/implementing-rate-limiting/. - Updated
.github/CODEOWNERSto assign ownership for the new skill + tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| plugins/dotnet/skills/implementing-rate-limiting/SKILL.md | New skill doc describing how to implement ASP.NET Core rate limiting and apply policies/endpoints. |
| tests/dotnet/implementing-rate-limiting/eval.yaml | New eval scenario validating outputs mention key rate limiting configuration elements. |
| .github/CODEOWNERS | Adds code owners for the new skill/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.
| // 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 | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This skill’s guidance says sliding window/token bucket are preferred to avoid the fixed-window burst problem, but the primary “Global rate limiter” example uses a fixed window. That’s internally inconsistent and may teach the opposite of the stated recommendation. Consider switching the global example to sliding window/token bucket, or explicitly justify why fixed window is acceptable for this global policy and call out the tradeoff.
| options.AddPolicy("per-user", context => | ||
| { | ||
| var userId = context.User?.FindFirst(ClaimTypes.NameIdentifier)?.Value; | ||
|
|
||
| return userId is not null | ||
| ? RateLimitPartition.GetTokenBucketLimiter(userId, _ => new TokenBucketRateLimiterOptions | ||
| { |
There was a problem hiding this comment.
The per-user partitioning snippet references ClaimTypes.NameIdentifier but the snippet doesn’t include using System.Security.Claims; (and other snippets in this file explicitly include needed usings). Add the missing using (or fully-qualify ClaimTypes) so the example compiles as-written.
| - type: "output_matches" | ||
| pattern: "(RequireRateLimiting|EnableRateLimiting)" | ||
| - type: "output_matches" | ||
| pattern: "(DisableRateLimiting|healthz)" |
There was a problem hiding this comment.
This assertion is too permissive: (DisableRateLimiting|healthz) will pass if the output merely mentions healthz without actually disabling rate limiting. To enforce the requirement “No limit on GET /healthz”, split into two assertions (one for mentioning /healthz, one for calling DisableRateLimiting / equivalent) or use a regex that requires both.
| pattern: "(DisableRateLimiting|healthz)" | |
| pattern: "DisableRateLimiting" | |
| - type: "output_matches" | |
| pattern: "/healthz" |
| ```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.
SKILL.md metadata/frontmatter won’t be parsed by the repo’s skill tooling because the file doesn’t start with YAML frontmatter (--- ... ---). SkillDiscovery.ParseFrontmatter only recognizes frontmatter at the start of the file, so the current leading skill fence causes the skill description to be empty and breaks discovery/validation. Please remove the outer skill fence and start the file with standard --- frontmatter like other skills.
| | Input | Required | Description | | ||
| |-------|----------|-------------| | ||
| | API endpoints to protect | Yes | Which routes need rate limiting | | ||
| | Rate limit requirements | Yes | Requests per window, per-client vs global | | ||
| | .NET version | No | Must be .NET 7+ for built-in support | |
There was a problem hiding this comment.
The tables in this doc use || at the start of each row (e.g., || Input | ...), which doesn’t render as a Markdown table in GitHub. Use single-pipe table syntax (| Input | ...) consistently so the Inputs section renders correctly.
| | Algorithm | Best For | Behavior | | ||
| |-----------|----------|----------| | ||
| | **Fixed Window** | Simple per-minute/per-hour limits | Counter resets at window boundary. ⚠️ Burst problem: 100 req at end of window + 100 at start of next = 200 in 1 second | | ||
| | **Sliding Window** | Smoother rate distribution | Divides window into segments, slides across time. Avoids burst problem | | ||
| | **Token Bucket** | Allowing controlled bursts | Tokens replenish at fixed rate, requests consume tokens. Good for APIs that should allow short bursts | | ||
| | **Concurrency Limiter** | Limiting simultaneous requests | Caps concurrent in-flight requests, not rate. Good for protecting expensive endpoints | | ||
|
|
There was a problem hiding this comment.
The algorithm comparison table also uses || row prefixes, so it won’t render as a Markdown table. Update it to standard Markdown table formatting (| ... |).
|
|
||
| if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out var retryAfter)) | ||
| { | ||
| context.HttpContext.Response.Headers.RetryAfter = |
There was a problem hiding this comment.
The OnRejected example sets context.HttpContext.Response.Headers.RetryAfter, but IHeaderDictionary doesn’t expose a RetryAfter property in typical ASP.NET Core usage; this is likely to produce non-compiling guidance. Prefer setting the header via the indexer (e.g., Response.Headers["Retry-After"] = ...) or Append (optionally using HeaderNames.RetryAfter).
| context.HttpContext.Response.Headers.RetryAfter = | |
| context.HttpContext.Response.Headers["Retry-After"] = |
Feedback carried over from #131PR #131 — Add implementing-rate-limiting skill (open)13 review threads — Reviewers: copilot-pull-request-reviewer, danmoseley, BrennanConroy
|
|
/evaluate |
| /plugins/dotnet/skills/dotnet-aot-compat/ @agocke @dotnet/appmodel | ||
| /tests/dotnet/dotnet-aot-compat/ @agocke @dotnet/appmodel | ||
|
|
||
| /plugins/dotnet/skills/implementing-rate-limiting/ @mrsharm |
There was a problem hiding this comment.
Figure out who is the right owner.
|
@danmoseley @BrennanConroy - could I get another review for this one. Had to reopen another PR once the structure changed. Thanks! |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
|
@mrsharm - for my knowledge - why do we only evaluate using Model: claude-opus-4.6 | Judge: claude-opus-4.6 ? and not other models |
|
Hi @SamMonoRT - this was an eval specific decision to presumably test with the latest and greatest model (at the time) to ensure that the skill is adding some value against the base model. I think it's also worthwhile testing this with other models (particularly older ones) but the assumption is that they are not new enough to not have the latest .NET behavior in their training set. |
|
@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. |
mrsharm
force-pushed
the
musharm/implementing-rate-limiting
branch
from
a71ef55 to
00f2370
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 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.
| - "Disabled rate limiting on the healthz endpoint" | ||
| - "Configured OnRejected callback that sets Retry-After header" | ||
| - "Chose an appropriate algorithm (sliding window or token bucket preferred over fixed window to avoid burst problem)" | ||
| expect_tools: ["bash"] |
There was a problem hiding this comment.
expect_tools: ["bash"] makes the scenario fail unless the agent actually calls the bash tool (the validator treats this as a hard constraint). This prompt is purely configuration guidance with no setup/commands that require shell execution, so the expected tool usage is unlikely and will make the eval brittle. Remove expect_tools here, or add a setup + explicit instruction/rubric requirement that justifies calling bash (e.g., compiling a minimal sample).
| expect_tools: ["bash"] |
| // Use X-Forwarded-For behind a reverse proxy, fall back to RemoteIpAddress | ||
| var ipAddress = context.Request.Headers["X-Forwarded-For"].FirstOrDefault() | ||
| ?? context.Connection.RemoteIpAddress?.ToString() | ||
| ?? "unknown"; |
There was a problem hiding this comment.
This guidance partitions by the raw X-Forwarded-For header. Unless the app is configured to trust forwarded headers only from known proxies (e.g., via ForwardedHeadersMiddleware / KnownNetworks / KnownProxies), clients can spoof X-Forwarded-For to bypass per-IP limits. Consider updating the text/snippet to either (1) recommend configuring forwarded headers properly and then using the resolved remote IP, or (2) clearly warn not to trust X-Forwarded-For directly on the public internet.
| // Use X-Forwarded-For behind a reverse proxy, fall back to RemoteIpAddress | |
| var ipAddress = context.Request.Headers["X-Forwarded-For"].FirstOrDefault() | |
| ?? context.Connection.RemoteIpAddress?.ToString() | |
| ?? "unknown"; | |
| // Use the resolved remote IP. If you're behind a reverse proxy, configure | |
| // ForwardedHeadersOptions (KnownProxies/KnownNetworks) so RemoteIpAddress is | |
| // set correctly. Do not trust X-Forwarded-For directly on the public internet. | |
| var ipAddress = context.Connection.RemoteIpAddress?.ToString() ?? "unknown"; |
| "name": "aspnetcore", | ||
| "version": "0.1.0", | ||
| "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns.", | ||
| "skills": "./skills/" |
There was a problem hiding this comment.
PR description/file list mentions plugins/dotnet/... and tests/dotnet/..., but this change introduces a new aspnetcore plugin (plugins/aspnetcore/...) and corresponding tests/aspnetcore/.... Please align the PR description (or the paths) so reviewers and automation know which plugin this skill belongs to.
Per repo restructuring feedback, ASP.NET Core specific skills should be under the aspnetcore plugin rather than the dotnet plugin.
- 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
mrsharm
force-pushed
the
musharm/implementing-rate-limiting
branch
from
d051519 to
75eff40
Compare
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rubric: | ||
| - "Recommended token bucket or sliding window (NOT fixed window which allows burst at window boundary)" | ||
| - "Explained why fixed window is wrong for this use case (burst problem: 100 requests at end of window + 100 at start of next = 200 in seconds)" |
There was a problem hiding this comment.
This scenario says “no bursting allowed” and “steady, even flow”, but the rubric explicitly lists token bucket as an acceptable recommendation. Token bucket is typically used specifically to allow bursts unless configured with a very small TokenLimit. Consider updating the rubric wording to prefer sliding window for this scenario (or clarify the token bucket configuration needed to prevent bursts).
| // UseRouting → UseAuthentication → UseAuthorization → UseRateLimiter | ||
| // Placing UseRateLimiter() AFTER UseAuthorization() lets you partition by authenticated user | ||
| app.UseRouting(); | ||
| app.UseAuthentication(); | ||
| app.UseAuthorization(); | ||
| app.UseRateLimiter(); // ← AFTER auth so user claims are available for partitioning |
There was a problem hiding this comment.
This ordering guidance is incorrect for per-user partitioning. HttpContext.User is populated by UseAuthentication(), so UseRateLimiter() should run after authentication but typically before authorization. Current ASP.NET Core rate limiting samples show UseRouting → UseAuthentication → UseRateLimiter → UseAuthorization.
| // UseRouting → UseAuthentication → UseAuthorization → UseRateLimiter | |
| // Placing UseRateLimiter() AFTER UseAuthorization() lets you partition by authenticated user | |
| app.UseRouting(); | |
| app.UseAuthentication(); | |
| app.UseAuthorization(); | |
| app.UseRateLimiter(); // ← AFTER auth so user claims are available for partitioning | |
| // UseRouting → UseAuthentication → UseRateLimiter → UseAuthorization | |
| // Placing UseRateLimiter() AFTER authentication (but BEFORE authorization) lets you partition by authenticated user | |
| app.UseRouting(); | |
| app.UseAuthentication(); | |
| app.UseRateLimiter(); // ← AFTER auth so user claims are available for partitioning | |
| app.UseAuthorization(); |
| rubric: | ||
| - "Identified the root cause: app.UseRateLimiter() is missing from the middleware pipeline" | ||
| - "Explained that AddRateLimiter only registers services but UseRateLimiter is required to activate the middleware" | ||
| - "Showed the correct middleware ordering: UseRateLimiter() after UseRouting() and UseAuthorization()" |
There was a problem hiding this comment.
This rubric line encodes the wrong middleware ordering: UseRateLimiter() does not need to be after UseAuthorization() to partition by user; it needs to be after UseAuthentication(). Current guidance/samples use UseRouting → UseAuthentication → UseRateLimiter → UseAuthorization so rate limiting can run before (potentially expensive) authorization.
| - "Showed the correct middleware ordering: UseRateLimiter() after UseRouting() and UseAuthorization()" | |
| - "Showed the correct middleware ordering: UseRouting() → UseAuthentication() → UseRateLimiter() → UseAuthorization()" |
| assertions: | ||
| - type: "output_not_matches" | ||
| pattern: "(AddRateLimiter.*GlobalLimiter|UseRateLimiter.*app\\.Map)" | ||
| rubric: |
There was a problem hiding this comment.
This output_not_matches pattern is likely too broad and can fail correct answers that merely mention these APIs while explaining what not to do. Consider tightening it to only reject code-like output (e.g., matching builder.Services.AddRateLimiter\s*\() or disallowing affirmative recommendations rather than the presence of AddRateLimiter/UseRateLimiter tokens.
Skill Validation Results
[1] (Plugin) Quality improved but weighted score is -0.8% due to: tokens (11846 → 27528), tool calls (0 → 1), time (20.8s → 25.0s)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
- Remove algorithm-preference rubric from scenario 1 (not asked in prompt) - Simplify middleware ordering rubric in scenario 2 (minimum fix focus) - Remove narrow OnRejected/Retry-After assertion from scenario 3 (beyond stated problem) - Remove algorithm-switch rubric from scenario 3 (user asked about status codes) - Replace overly narrow negative assertion in scenario 4 with broad match - Reword scenario 5 rubrics to test outcomes not skill vocabulary
- Scenario 2: 60s → 120s (both runs timing out) - Scenario 3: 120s → 180s (setup files trigger builds, needs more time) - Scenario 4: Reworked from pure 'don't use' to 'combine rate limiting with auth-aware partitioning' — tests genuine skill value (per-user partitioning, multiple policies) while still testing boundary awareness for login throttling
The missing-UseRateLimiter() bug is simple enough that the baseline model consistently scores 5/5 without the skill. The skill actually regresses this scenario by adding context that slows the agent and causes timeouts. Removed to reduce eval noise; the remaining 4 scenarios demonstrate clear skill value.
- Multi-bug diagnosis: 503 + AutoReplenishment=false + middleware ordering (tests compound debugging) - Reverse proxy partitioning: RemoteIpAddress is proxy IP, all clients share limits (subtle production gotcha) - Concurrency vs rate limiter: FixedWindow for simultaneous-request limiting (common misconception)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/evaluate |
Skill Validation Results
[1] (Isolated) Quality unchanged but weighted score is -28.6% due to: judgment, quality, tokens (11961 → 27614), tool calls (0 → 1), time (13.0s → 18.8s)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
- Reverse proxy: add setup files with buggy code (uses RemoteIpAddress directly behind proxy) so the skill adds value in diagnosing it. Narrow assertion to require ForwardedHeaders-specific solution. - Drop concurrency scenario: baseline already scores 5.0/5 — skill adds no value and only incurs token overhead penalty.
|
/evaluate |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
- Reword reverse proxy prompt to use activation-friendly keywords (AddRateLimiter, per-IP partitioning, RemoteIpAddress, per-client rate limiting) instead of 'reverse proxy' which matches the skill's DO NOT USE section and prevents activation - Bump multi-bug timeout 180s → 240s (CI runners slower) - Bump reverse proxy timeout 120s → 180s
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Mistake | Symptom | Fix | | ||
| |---------|---------|-----| | ||
| | Default `RejectionStatusCode` is 503 | Clients think server is down, not rate limited | Set `options.RejectionStatusCode = 429` | | ||
| | `UseRateLimiter()` before `UseRouting()` | Endpoint-specific policies silently don't apply | Move after `UseRouting()` and after `UseAuthorization()` so user claims are available for partitioning | |
There was a problem hiding this comment.
This row says UseRateLimiter() should be moved after UseAuthorization() so user claims are available. User claims are available after UseAuthentication(), and recommended guidance is to run UseRateLimiter() after routing + authentication, but before authorization, to ensure rate limiting still applies to unauthorized requests when desired.
| | `UseRateLimiter()` before `UseRouting()` | Endpoint-specific policies silently don't apply | Move after `UseRouting()` and after `UseAuthorization()` so user claims are available for partitioning | | |
| | `UseRateLimiter()` before `UseRouting()` | Endpoint-specific policies silently don't apply | Move after `UseRouting()` and `UseAuthentication()`, but before `UseAuthorization()`, so user claims are available for partitioning while still rate limiting unauthorized requests | |
| { | ||
| "name": "aspnetcore", | ||
| "version": "0.1.0", | ||
| "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns.", | ||
| "skills": "./skills/" | ||
| } |
There was a problem hiding this comment.
PR description/file list appears out of sync with the actual changes: it mentions plugins/dotnet/... and tests/dotnet/..., but this PR adds the skill under plugins/aspnetcore/... and tests/aspnetcore/.... Please update the PR description to match the new plugin/test paths to avoid confusion for reviewers and tooling.
Skill Validation Results
[1] (Plugin) Quality unchanged but weighted score is -3.4% due to: tokens (175507 → 278938)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
|
/evaluate |
Skill Validation Results
[1] (Plugin) Quality improved but weighted score is -0.1% due to: tokens (12942 → 29491), tool calls (0 → 2), time (28.7s → 45.0s)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
Summary
Adds a skill for implementing .NET 8 built-in rate limiting correctly.
Eval Results (3-run)
Files