Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill, implementing-health-checks, to guide users through configuring ASP.NET Core health checks for Kubernetes probes (liveness/readiness/startup), along with an evaluation scenario to validate activation behavior.
Changes:
- Added skill content describing package setup, tagging strategy, endpoint mapping, and Kubernetes probe configuration.
- Added eval scenarios to validate the skill activates for Kubernetes health checks requests and does not activate for OpenTelemetry/APM requests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/dotnet/skills/implementing-health-checks/SKILL.md | New skill documentation for implementing ASP.NET Core health checks and Kubernetes probes. |
| tests/dotnet/implementing-health-checks/eval.yaml | New evaluation scenarios for the skill’s activation and non-activation behavior. |
💡 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.
|
|
||
| ### Step 2: Register health checks with SEPARATE liveness and readiness | ||
|
|
||
| **Critical distinction** most implementations get wrong: |
There was a problem hiding this comment.
The sentence "Critical distinction most implementations get wrong:" reads grammatically incomplete. Consider rephrasing to something like "Critical distinction most implementations get wrong:" or "Critical distinction: most implementations get wrong" to improve readability.
| **Critical distinction** most implementations get wrong: | |
| **Critical distinction most implementations get wrong:** |
| app.MapHealthChecks("/healthz/ready", new HealthCheckOptions | ||
| { | ||
| Predicate = check => check.Tags.Contains("ready"), | ||
| ResponseWriter = WriteDetailedResponse |
There was a problem hiding this comment.
The readiness endpoint is configured to return a detailed JSON payload including per-check descriptions. In real deployments these descriptions can contain internal dependency names and failure details, which is risky if this endpoint is reachable outside the cluster or is marked anonymous. Consider defaulting readiness to the minimal response, or explicitly calling out that the detailed response should be protected (auth/network policy) or enabled only in non-production environments.
| ResponseWriter = WriteDetailedResponse | |
| // Use minimal response by default; expose detailed health only on protected/internal endpoints | |
| ResponseWriter = WriteMinimalResponse |
| - type: "output_not_contains" | ||
| value: "AddHealthChecks" | ||
| - type: "output_not_contains" | ||
| value: "MapHealthChecks" |
There was a problem hiding this comment.
The negative assertions use output_not_contains for the strings "AddHealthChecks" and "MapHealthChecks". This can make the scenario brittle if a good OpenTelemetry-focused answer briefly contrasts with health checks (e.g., "don't call AddHealthChecks here"), causing a false failure. Consider using output_not_matches to only reject actual method invocations (e.g., \b(AddHealthChecks|MapHealthChecks)\s*\(), and/or add positive assertions for expected OpenTelemetry terms ("OpenTelemetry", "AddOpenTelemetry", "tracing", "metrics", exporters).
| - type: "output_not_contains" | |
| value: "AddHealthChecks" | |
| - type: "output_not_contains" | |
| value: "MapHealthChecks" | |
| - type: "output_not_matches" | |
| pattern: "\\b(AddHealthChecks|MapHealthChecks)\\s*\\(" | |
| - type: "output_matches" | |
| pattern: "(OpenTelemetry|AddOpenTelemetry|tracing|metrics|exporter)" |
|
/eval |
Migration NoteThis PR replaces #88 which was opened from
All prior review feedback from #88 still applies — please see that PR for the full discussion history. |
Feedback carried over from #88Code Review Comments (3 threads)
Discussion Comments (2)
|
|
@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. |
Teaches ASP.NET Core health check configuration for Kubernetes: liveness vs readiness vs startup probes, custom health checks for dependencies (SQL, Redis, external APIs), and K8s YAML configuration. Eval results: +33.1% improvement over baseline (threshold: 10%) Includes eval.yaml with K8s probe scenario + negative test.
- Fix: Remove redundant Microsoft.Extensions.Diagnostics.HealthChecks package (already included in ASP.NET Core framework) - Fix: Add timeout: TimeSpan.FromSeconds(5) to all health check registrations to match Common Pitfalls guidance - Move skill from src/dotnet/skills/ to plugins/dotnet/skills/ - Move eval from src/dotnet/tests/ to tests/dotnet/
…add eval cases - Create dotnet-aspnet plugin (plugin.json, marketplace.json entries, CODEOWNERS, README) - Move skill from plugins/dotnet/ to plugins/dotnet-aspnet/ per Manish's feedback - Move tests from tests/dotnet/ to tests/dotnet-aspnet/ - Fix grammar: 'Critical distinction most implementations get wrong:' - Change readiness endpoint to use minimal response by default (security) - Fix startup probe to check process health only, not all checks - Add 'detailed response on public endpoint' to Common Pitfalls - Replace output_not_contains with output_not_matches regex (less brittle) - Add positive assertion for expected OpenTelemetry terms - Add eval: 'Separate startup probe from liveness' scenario - Add eval: 'Health checks with load balancer' scenario - Add eval: 'Do not confuse health checks with logging middleware' scenario
800b11d to
8108581
Compare
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 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.
| - "Did NOT suggest health checks for a request logging scenario" | ||
| - "Focused on HTTP logging or request logging middleware configuration" | ||
| timeout: 60 | ||
| timeout: 60 |
There was a problem hiding this comment.
This scenario defines timeout twice. YAML duplicate keys are easy to miss and the last value will win, which can hide edits or cause confusing behavior during evaluation. Remove the duplicate timeout entry so the scenario has a single, unambiguous timeout value.
| timeout: 60 |
| - type: "output_not_matches" | ||
| pattern: "\\b(AddHealthChecks|MapHealthChecks)\\s*\\(" | ||
| - type: "output_matches" | ||
| pattern: "(UseHttpLogging|UseSerilog|logging|middleware)" |
There was a problem hiding this comment.
The assertion pattern (UseHttpLogging|UseSerilog|logging|middleware) is so broad that a response mentioning unrelated “middleware” could satisfy it without actually recommending request logging. Tighten the pattern to require logging-specific terms (e.g., UseHttpLogging, Serilog, request logging, etc.) so this scenario can’t pass on false positives.
| pattern: "(UseHttpLogging|UseSerilog|logging|middleware)" | |
| pattern: "(UseHttpLogging|UseSerilog|request logging|http logging|HTTP logging|logging middleware)" |
|
|
||
| ```csharp | ||
| builder.Services.AddHealthChecksUI().AddInMemoryStorage(); | ||
| app.MapHealthChecksUI(); |
There was a problem hiding this comment.
app.MapHealthChecksUI() exposes the HealthChecks UI endpoint with detailed health information but no authentication or access control, which can leak internal dependency names, connection details, and health statuses to unauthenticated clients. An attacker who can reach this endpoint could use this operational data to map internal services or assist further attacks. Restrict access to the HealthChecks UI (for example by applying authorization, IP/network restrictions, or hosting it on an internal-only path) rather than exposing it directly on the public app pipeline.
| app.MapHealthChecksUI(); | |
| // Expose UI on a custom path and require authorization to avoid leaking details | |
| app.MapHealthChecksUI(options => | |
| { | |
| options.UIPath = "/health-ui"; // UI endpoint | |
| options.ApiPath = "/health-ui-api"; // backend API | |
| }).RequireAuthorization(); |
Skill Validation Results
[1] Quality improved but weighted score is -7.7% due to: completion (✓ → ✗), tokens (33788 → 208438), tool calls (3 → 21), time (26.5s → 97.2s)
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
DeagleGross
left a comment
There was a problem hiding this comment.
Thanks for writing this skill, but I am concerned about adding it in such form because:
-
there is an excellent doc which agent can read from, describing a general approach to building health-checks in aspnetcore: https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/health-checks?view=aspnetcore-10.0.
-
Skill has a broard number of packages included, and those are not represented in frontmatter. We need to decide whether this skill has to be a specific one related to Kubernetes / sql / redis, or it is a general-purpose skill around health-checks.
Let me know your thoughts please!
| ### Step 1: Add the health checks packages | ||
|
|
||
| ```bash | ||
| dotnet add package AspNetCore.HealthChecks.SqlServer # for SQL Server |
There was a problem hiding this comment.
Why are packages for SqlServer, Redis and Uris required for the skill about health-checks for Kubernetes and load balancer integration? Skill has to be very specific, and it feels like this one supports multiple different resources not really following the single idea.
| pattern: "(liveness|readiness|live|ready)" | ||
| - type: "output_matches" | ||
| pattern: "(healthz|health)" | ||
| rubric: |
There was a problem hiding this comment.
Skill clearly relies on installing specific NuGet package(s), and rubric does not validate that at all. Are those packages really required, or agent can generate code building GET endpoint handlers from scratch?
| @@ -0,0 +1,70 @@ | |||
| scenarios: | |||
| - name: "Add health checks with Kubernetes probes" | |||
| prompt: "I need to add health checks to my ASP.NET Core API for Kubernetes deployment. It should check the database and Redis connections and have separate liveness and readiness endpoints." | |||
There was a problem hiding this comment.
Prompt is overfitting for the name of skill. I dont imagine myself writing "i need to add health checks for MY ASPNETCORE API", I will simply ask agent "need to add health checks dependant on db" for example.
Summary
Adds the implementing-health-checks skill for configuring ASP.NET Core health checks with Kubernetes liveness, readiness, and startup probes.
Skill Validation Results
Overall improvement: +17.2% (3 runs)
Model: claude-opus-4.6 | Judge: claude-opus-4.6
What the Skill Teaches
Files