From 6b42e6951829411aa784a5f375fdd90610aa3160 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Tue, 10 Mar 2026 13:44:14 -0700 Subject: [PATCH 01/16] Move minimal-api-file-upload skill to aspnetcore plugin with 3 eval scenarios Per repo restructuring feedback, ASP.NET Core specific skills should be under the aspnetcore plugin rather than the dotnet plugin. --- plugins/aspnetcore/plugin.json | 6 + .../skills/minimal-api-file-upload/SKILL.md | 196 ++++++++++++++++++ .../minimal-api-file-upload/eval.yaml | 67 ++++++ 3 files changed, 269 insertions(+) create mode 100644 plugins/aspnetcore/plugin.json create mode 100644 plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md create mode 100644 tests/aspnetcore/minimal-api-file-upload/eval.yaml diff --git a/plugins/aspnetcore/plugin.json b/plugins/aspnetcore/plugin.json new file mode 100644 index 0000000000..00c373186d --- /dev/null +++ b/plugins/aspnetcore/plugin.json @@ -0,0 +1,6 @@ +{ + "name": "aspnetcore", + "version": "0.1.0", + "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns.", + "skills": "./skills/" +} diff --git a/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md b/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md new file mode 100644 index 0000000000..e179f85bb1 --- /dev/null +++ b/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md @@ -0,0 +1,196 @@ +--- +name: minimal-api-file-upload +description: File upload endpoints in ASP.NET minimal APIs (.NET 8+) +--- + +# Implementing File Uploads in ASP.NET Core Minimal APIs + +## When to Use +- File upload endpoints in ASP.NET Core minimal APIs (.NET 8+) +- Handling IFormFile or IFormFileCollection parameters +- When you need size limits, content type validation, or streaming large files + +## When Not to Use +- MVC controllers ΓåÆ `[FromForm] IFormFile` works directly with attributes +- Simple JSON body ΓåÆ no file upload needed +- Very large files (> 1GB) ΓåÆ use streaming with `MultipartReader` instead + +## Inputs + +| Input | Required | Description | +|-------|----------|-------------| +| File parameter(s) | Yes | IFormFile or IFormFileCollection | +| Size limits | Yes | Max file/request size | +| Allowed types | No | Content type or extension restrictions | + +## Workflow + +### Step 1: CRITICAL ΓÇö IFormFile Requires [FromForm] in Minimal APIs (Not Automatic) + +```csharp +// COMMON MISTAKE: Expecting IFormFile to bind automatically +app.MapPost("/upload", (IFormFile file) => ...); +// In early .NET versions, this worked differently. In .NET 8: + +// CRITICAL: IFormFile IS bound automatically from form data in .NET 8 +// BUT when you mix IFormFile with other parameters, you need [FromForm] +app.MapPost("/upload-with-metadata", + ([FromForm] IFormFile file, [FromForm] string description) => +{ + return Results.Ok(new { file.FileName, Description = description }); +}); + +// CRITICAL: For multiple files, use IFormFileCollection +app.MapPost("/upload-multiple", (IFormFileCollection files) => +{ + return Results.Ok(files.Select(f => new { f.FileName, f.Length })); +}); +``` + +### Step 2: CRITICAL ΓÇö File Size Limits Are Separate from Request Size Limits + +```csharp +// CRITICAL: There are TWO different size limits and you need to configure BOTH + +// 1. Request body size limit (Kestrel level) ΓÇö default is 30MB +builder.WebHost.ConfigureKestrel(options => +{ + options.Limits.MaxRequestBodySize = 100 * 1024 * 1024; // 100 MB +}); + +// 2. Form options ΓÇö multipart body length limit ΓÇö default is 128MB +builder.Services.Configure(options => +{ + options.MultipartBodyLengthLimit = 100 * 1024 * 1024; // 100 MB + options.ValueLengthLimit = 1024 * 1024; // 1 MB for form values + options.MultipartHeadersLengthLimit = 16384; // 16 KB for section headers +}); + +// COMMON MISTAKE: Only increasing Kestrel MaxRequestBodySize +// upload still fails because FormOptions.MultipartBodyLengthLimit is exceeded + +// COMMON MISTAKE: Only increasing FormOptions +// upload fails with "Request body too large" from Kestrel before reaching form parsing + +// CRITICAL: Per-endpoint override with RequestSizeLimit attribute +app.MapPost("/upload-large", [RequestSizeLimit(200_000_000)] (IFormFile file) => +{ + return Results.Ok(new { file.FileName, file.Length }); +}); + +// CRITICAL: To disable the limit entirely (for streaming): +app.MapPost("/upload-unlimited", [DisableRequestSizeLimit] async (HttpContext context) => +{ + // Handle manually +}); +``` + +### Step 3: CRITICAL ΓÇö Anti-Forgery Auto-Validates Form Uploads in .NET 8 + +```csharp +// CRITICAL: In .NET 8 with UseAntiforgery(), ALL form-bound endpoints +// automatically validate anti-forgery tokens, INCLUDING file uploads + +builder.Services.AddAntiforgery(); +var app = builder.Build(); +app.UseAntiforgery(); + +// This endpoint now REQUIRES an anti-forgery token: +app.MapPost("/upload", (IFormFile file) => Results.Ok(file.FileName)); +// Without the token ΓåÆ 400 Bad Request + +// CRITICAL: For API-only file uploads (no anti-forgery needed), opt out: +app.MapPost("/api/upload", (IFormFile file) => Results.Ok(file.FileName)) + .DisableAntiforgery(); // CRITICAL: Must explicitly opt out + +// COMMON MISTAKE: Getting 400 errors on file uploads and not realizing +// it's because UseAntiforgery() is in the pipeline +``` + +### Step 4: CRITICAL ΓÇö Validate File Content, Not Just Extension + +```csharp +app.MapPost("/upload", async (IFormFile file) => +{ + // CRITICAL: Check content type AND file signature (magic bytes) + // NEVER trust file extension alone ΓÇö it can be spoofed + + var allowedTypes = new[] { "image/jpeg", "image/png", "image/gif" }; + if (!allowedTypes.Contains(file.ContentType)) + return Results.BadRequest("File type not allowed"); + + // CRITICAL: Check magic bytes for file type verification + using var stream = file.OpenReadStream(); + var header = new byte[8]; + await stream.ReadAsync(header, 0, 8); + stream.Position = 0; + + // JPEG: FF D8 FF + // PNG: 89 50 4E 47 + var isJpeg = header[0] == 0xFF && header[1] == 0xD8 && header[2] == 0xFF; + var isPng = header[0] == 0x89 && header[1] == 0x50 && header[2] == 0x4E && header[3] == 0x47; + + if (!isJpeg && !isPng) + return Results.BadRequest("File content doesn't match declared type"); + + // CRITICAL: Generate a safe filename ΓÇö never use user-provided filename directly + var safeFileName = $"{Guid.NewGuid()}{Path.GetExtension(file.FileName)}"; + // NEVER: var path = Path.Combine("uploads", file.FileName); // Path traversal! + + var filePath = Path.Combine("uploads", safeFileName); + Directory.CreateDirectory("uploads"); + using var fileStream = File.Create(filePath); + await file.CopyToAsync(fileStream); + + return Results.Ok(new { FileName = safeFileName, file.Length }); +}); +``` + +### Step 5: CRITICAL ΓÇö Streaming Large Files Without Buffering + +```csharp +// CRITICAL: IFormFile buffers the entire file in memory by default +// For large files, use MultipartReader for streaming + +app.MapPost("/upload-stream", + [DisableRequestSizeLimit] + async (HttpContext context) => +{ + var boundary = context.Request.GetMultipartBoundary(); + if (string.IsNullOrEmpty(boundary)) + return Results.BadRequest("Not a multipart request"); + + var reader = new MultipartReader(boundary, context.Request.Body); + + // CRITICAL: ReadNextSectionAsync returns null when there are no more sections + while (await reader.ReadNextSectionAsync() is { } section) + { + var contentDisposition = section.GetContentDispositionHeader(); + if (contentDisposition == null) continue; + + if (contentDisposition.IsFileDisposition()) + { + var fileName = contentDisposition.FileName.Value; + var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(fileName)}"; + + // CRITICAL: Stream directly to disk ΓÇö never buffer in memory + using var fileStream = File.Create(Path.Combine("uploads", safeFile)); + await section.Body.CopyToAsync(fileStream); + } + } + + return Results.Ok("Uploaded"); +}).DisableAntiforgery(); + +// COMMON MISTAKE: Using file.CopyToAsync for very large files +// IFormFile buffers everything in memory first ΓÇö can cause OutOfMemoryException +``` + +## Common Mistakes + +1. **Only configuring one size limit**: Must configure BOTH Kestrel `MaxRequestBodySize` AND `FormOptions.MultipartBodyLengthLimit`. +2. **400 errors from anti-forgery**: In .NET 8, `UseAntiforgery()` auto-validates form uploads. Use `.DisableAntiforgery()` for API endpoints. +3. **Trusting file.FileName**: User-provided filename can contain path traversal. Always generate a safe filename. +4. **Trusting Content-Type only**: Content type can be spoofed. Check magic bytes for actual file type. +5. **Using IFormFile for large files**: IFormFile buffers in memory. Use `MultipartReader` for streaming. +6. **Missing GetMultipartBoundary extension**: Must use `context.Request.GetMultipartBoundary()`, not parse manually. diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml new file mode 100644 index 0000000000..13223a1093 --- /dev/null +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -0,0 +1,67 @@ +scenarios: + - name: "Implement secure file upload in ASP.NET Core 8 minimal API" + prompt: | + I need to implement a file upload endpoint in my ASP.NET Core 8 minimal API. + The endpoint should accept image files (JPEG and PNG only), reject files over 10MB, + and save them to an "uploads" folder. My app already has UseAntiforgery() in the pipeline. + Show me the complete implementation including size limits configuration and the endpoint. + assertions: + - type: "output_matches" + pattern: "(IFormFile|IFormFileCollection)" + - type: "output_matches" + pattern: "(Guid\\.NewGuid|Path\\.GetRandomFileName)" + - type: "output_matches" + pattern: "(ContentType|image/jpeg|image/png)" + rubric: + - "Configures BOTH Kestrel MaxRequestBodySize AND FormOptions.MultipartBodyLengthLimit" + - "Uses .DisableAntiforgery() on the file upload endpoint to prevent automatic 400 errors" + - "Generates a safe filename (Guid.NewGuid or similar) instead of using user-provided IFormFile.FileName directly" + - "Validates file content beyond just checking file extension — checks ContentType or magic bytes" + - "Correctly uses IFormFile parameter binding in the minimal API endpoint" + timeout: 120 + + - name: "Fix file upload returning 400 Bad Request" + prompt: | + My ASP.NET Core 8 minimal API file upload endpoint returns 400 Bad Request with no error details. The endpoint works fine when I remove UseAntiforgery() from Program.cs. What's going on? + setup: + files: + - path: "Program.cs" + content: | + var builder = WebApplication.CreateBuilder(args); + builder.Services.AddAntiforgery(); + var app = builder.Build(); + app.UseAntiforgery(); + app.MapPost("/upload", (IFormFile file) => + { + return Results.Ok(new { file.FileName, file.Length }); + }); + app.Run(); + - path: "UploadBug.csproj" + content: | + + + net8.0 + + + assertions: + - type: "output_matches" + pattern: "(DisableAntiforgery|antiforgery|anti-forgery)" + rubric: + - "Identified the root cause: UseAntiforgery() middleware rejects file upload requests that lack an antiforgery token" + - "Recommended using .DisableAntiforgery() on the file upload endpoint" + - "Explained that minimal API file upload endpoints typically need antiforgery disabled since the token cannot be included in multipart form uploads from API clients" + timeout: 60 + + - name: "Upload multiple files with metadata in minimal API" + prompt: | + I need to upload multiple files along with a description string in a single request to my ASP.NET Core 8 minimal API. How do I bind both the files and the form field? + assertions: + - type: "output_matches" + pattern: "(IFormFileCollection|IFormFile)" + - type: "output_matches" + pattern: "(FromForm|\\[FromForm\\])" + rubric: + - "Used IFormFileCollection for multiple files" + - "Used [FromForm] attribute on parameters when mixing files with other form fields" + - "Showed correct multipart/form-data binding pattern" + timeout: 120 From 5ff29b5b2412e5ac92f9a62bef93f84071828080 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Tue, 10 Mar 2026 14:11:10 -0700 Subject: [PATCH 02/16] Increase eval timeouts to 180s to avoid timeout-related scoring penalties --- tests/aspnetcore/minimal-api-file-upload/eval.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index 13223a1093..0d39f682dc 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -18,7 +18,7 @@ scenarios: - "Generates a safe filename (Guid.NewGuid or similar) instead of using user-provided IFormFile.FileName directly" - "Validates file content beyond just checking file extension — checks ContentType or magic bytes" - "Correctly uses IFormFile parameter binding in the minimal API endpoint" - timeout: 120 + timeout: 180 - name: "Fix file upload returning 400 Bad Request" prompt: | @@ -50,7 +50,7 @@ scenarios: - "Identified the root cause: UseAntiforgery() middleware rejects file upload requests that lack an antiforgery token" - "Recommended using .DisableAntiforgery() on the file upload endpoint" - "Explained that minimal API file upload endpoints typically need antiforgery disabled since the token cannot be included in multipart form uploads from API clients" - timeout: 60 + timeout: 180 - name: "Upload multiple files with metadata in minimal API" prompt: | @@ -64,4 +64,4 @@ scenarios: - "Used IFormFileCollection for multiple files" - "Used [FromForm] attribute on parameters when mixing files with other form fields" - "Showed correct multipart/form-data binding pattern" - timeout: 120 + timeout: 180 From db4eaa103f787e2138cb312519a9bd8048833102 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Fri, 27 Mar 2026 04:26:17 -0700 Subject: [PATCH 03/16] Generalized and addressed feedback --- .../skills/minimal-api-file-upload/SKILL.md | 108 +++++++++++------- .../minimal-api-file-upload/eval.yaml | 50 ++++---- 2 files changed, 90 insertions(+), 68 deletions(-) diff --git a/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md b/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md index e179f85bb1..db7ffa653e 100644 --- a/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md +++ b/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md @@ -11,9 +11,9 @@ description: File upload endpoints in ASP.NET minimal APIs (.NET 8+) - When you need size limits, content type validation, or streaming large files ## When Not to Use -- MVC controllers ΓåÆ `[FromForm] IFormFile` works directly with attributes -- Simple JSON body ΓåÆ no file upload needed -- Very large files (> 1GB) ΓåÆ use streaming with `MultipartReader` instead +- MVC controllers → `[FromForm] IFormFile` works directly with attributes +- Simple JSON body → no file upload needed +- Very large files (> 1GB) → use streaming with `MultipartReader` instead ## Inputs @@ -25,43 +25,44 @@ description: File upload endpoints in ASP.NET minimal APIs (.NET 8+) ## Workflow -### Step 1: CRITICAL ΓÇö IFormFile Requires [FromForm] in Minimal APIs (Not Automatic) +### Step 1: CRITICAL — Understand IFormFile Binding in Minimal APIs ```csharp -// COMMON MISTAKE: Expecting IFormFile to bind automatically +// In .NET 8+ minimal APIs, IFormFile binds automatically from multipart/form-data +// when it is the only complex parameter. app.MapPost("/upload", (IFormFile file) => ...); -// In early .NET versions, this worked differently. In .NET 8: -// CRITICAL: IFormFile IS bound automatically from form data in .NET 8 -// BUT when you mix IFormFile with other parameters, you need [FromForm] +// CRITICAL: When you mix files with other form fields, use [FromForm] on all +// form-bound parameters (or group them into a single [FromForm] DTO). app.MapPost("/upload-with-metadata", ([FromForm] IFormFile file, [FromForm] string description) => { return Results.Ok(new { file.FileName, Description = description }); }); -// CRITICAL: For multiple files, use IFormFileCollection +// Multiple files: IFormFileCollection also binds automatically from multipart/form-data. +// You only need [FromForm] if you mix it with other form fields, as shown above. app.MapPost("/upload-multiple", (IFormFileCollection files) => { return Results.Ok(files.Select(f => new { f.FileName, f.Length })); }); ``` -### Step 2: CRITICAL ΓÇö File Size Limits Are Separate from Request Size Limits +### Step 2: CRITICAL — File Size Limits Are Separate from Request Size Limits ```csharp // CRITICAL: There are TWO different size limits and you need to configure BOTH -// 1. Request body size limit (Kestrel level) ΓÇö default is 30MB +// 1. Request body size limit (Kestrel level) — default is 30MB builder.WebHost.ConfigureKestrel(options => { - options.Limits.MaxRequestBodySize = 100 * 1024 * 1024; // 100 MB + options.Limits.MaxRequestBodySize = 10 * 1024 * 1024; // 10 MB }); -// 2. Form options ΓÇö multipart body length limit ΓÇö default is 128MB +// 2. Form options — multipart body length limit — default is 128MB builder.Services.Configure(options => { - options.MultipartBodyLengthLimit = 100 * 1024 * 1024; // 100 MB + options.MultipartBodyLengthLimit = 10 * 1024 * 1024; // 10 MB options.ValueLengthLimit = 1024 * 1024; // 1 MB for form values options.MultipartHeadersLengthLimit = 16384; // 16 KB for section headers }); @@ -85,10 +86,10 @@ app.MapPost("/upload-unlimited", [DisableRequestSizeLimit] async (HttpContext co }); ``` -### Step 3: CRITICAL ΓÇö Anti-Forgery Auto-Validates Form Uploads in .NET 8 +### Step 3: CRITICAL — Anti-Forgery Auto-Validates Form Uploads in .NET 8+ ```csharp -// CRITICAL: In .NET 8 with UseAntiforgery(), ALL form-bound endpoints +// CRITICAL: In .NET 8+ with UseAntiforgery(), ALL form-bound endpoints // automatically validate anti-forgery tokens, INCLUDING file uploads builder.Services.AddAntiforgery(); @@ -97,7 +98,7 @@ app.UseAntiforgery(); // This endpoint now REQUIRES an anti-forgery token: app.MapPost("/upload", (IFormFile file) => Results.Ok(file.FileName)); -// Without the token ΓåÆ 400 Bad Request +// Without the token → 400 Bad Request // CRITICAL: For API-only file uploads (no anti-forgery needed), opt out: app.MapPost("/api/upload", (IFormFile file) => Results.Ok(file.FileName)) @@ -105,24 +106,34 @@ app.MapPost("/api/upload", (IFormFile file) => Results.Ok(file.FileName)) // COMMON MISTAKE: Getting 400 errors on file uploads and not realizing // it's because UseAntiforgery() is in the pipeline + +// WARNING: DisableAntiforgery() is safe for unauthenticated endpoints and +// endpoints using JWT bearer authentication. However, for endpoints +// authenticated with cookies, disabling antiforgery removes CSRF protection +// and exposes the endpoint to cross-site request forgery attacks. +// For cookie-authenticated endpoints, include a valid antiforgery token instead. ``` -### Step 4: CRITICAL ΓÇö Validate File Content, Not Just Extension +### Step 4: CRITICAL — Validate File Content, Not Just Extension ```csharp app.MapPost("/upload", async (IFormFile file) => { // CRITICAL: Check content type AND file signature (magic bytes) - // NEVER trust file extension alone ΓÇö it can be spoofed + // NEVER trust file extension alone — it can be spoofed - var allowedTypes = new[] { "image/jpeg", "image/png", "image/gif" }; + // Allow only JPEG/PNG by default. To support more (e.g., GIF), + // add the MIME type here AND validate its magic bytes below. + var allowedTypes = new[] { "image/jpeg", "image/png" }; if (!allowedTypes.Contains(file.ContentType)) return Results.BadRequest("File type not allowed"); // CRITICAL: Check magic bytes for file type verification using var stream = file.OpenReadStream(); var header = new byte[8]; - await stream.ReadAsync(header, 0, 8); + var bytesRead = await stream.ReadAsync(header, 0, header.Length); + if (bytesRead < 4) + return Results.BadRequest("File content is too short or invalid"); stream.Position = 0; // JPEG: FF D8 FF @@ -133,8 +144,10 @@ app.MapPost("/upload", async (IFormFile file) => if (!isJpeg && !isPng) return Results.BadRequest("File content doesn't match declared type"); - // CRITICAL: Generate a safe filename ΓÇö never use user-provided filename directly - var safeFileName = $"{Guid.NewGuid()}{Path.GetExtension(file.FileName)}"; + // CRITICAL: Generate a safe filename — never use user-provided filename directly + // Derive the extension from the validated content type, not from user input + var extension = isJpeg ? ".jpg" : ".png"; + var safeFileName = $"{Guid.NewGuid()}{extension}"; // NEVER: var path = Path.Combine("uploads", file.FileName); // Path traversal! var filePath = Path.Combine("uploads", safeFileName); @@ -146,18 +159,25 @@ app.MapPost("/upload", async (IFormFile file) => }); ``` -### Step 5: CRITICAL ΓÇö Streaming Large Files Without Buffering +### Step 5: CRITICAL — Streaming Large Files Without Buffering ```csharp -// CRITICAL: IFormFile buffers the entire file in memory by default -// For large files, use MultipartReader for streaming +// CRITICAL: IFormFile uses multipart form parsing that buffers with a memory threshold +// and spills to temp files. For very large uploads this can consume significant +// memory and disk. Use MultipartReader for streaming without buffering. app.MapPost("/upload-stream", [DisableRequestSizeLimit] async (HttpContext context) => { - var boundary = context.Request.GetMultipartBoundary(); - if (string.IsNullOrEmpty(boundary)) + // Extract the multipart boundary from the Content-Type header + var contentType = context.Request.ContentType; + if (contentType == null) + return Results.BadRequest("Missing Content-Type"); + + var boundary = HeaderUtilities.RemoveQuotes( + MediaTypeHeaderValue.Parse(contentType).Boundary).Value; + if (string.IsNullOrWhiteSpace(boundary)) return Results.BadRequest("Not a multipart request"); var reader = new MultipartReader(boundary, context.Request.Body); @@ -165,15 +185,20 @@ app.MapPost("/upload-stream", // CRITICAL: ReadNextSectionAsync returns null when there are no more sections while (await reader.ReadNextSectionAsync() is { } section) { - var contentDisposition = section.GetContentDispositionHeader(); - if (contentDisposition == null) continue; + // Parse Content-Disposition to identify file sections + if (!ContentDispositionHeaderValue.TryParse(section.ContentDisposition, out var contentDisposition)) + continue; - if (contentDisposition.IsFileDisposition()) + if (contentDisposition.DispositionType.Equals("form-data") + && !string.IsNullOrEmpty(contentDisposition.FileName.Value)) { - var fileName = contentDisposition.FileName.Value; - var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(fileName)}"; + // Sanitize the user-provided filename to prevent path traversal + var originalFileName = contentDisposition.FileName.Value ?? string.Empty; + var sanitizedFileName = Path.GetFileName(originalFileName.Trim('"')); + var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(sanitizedFileName)}"; - // CRITICAL: Stream directly to disk ΓÇö never buffer in memory + // CRITICAL: Stream directly to disk — avoids buffering in memory + Directory.CreateDirectory("uploads"); using var fileStream = File.Create(Path.Combine("uploads", safeFile)); await section.Body.CopyToAsync(fileStream); } @@ -182,15 +207,16 @@ app.MapPost("/upload-stream", return Results.Ok("Uploaded"); }).DisableAntiforgery(); -// COMMON MISTAKE: Using file.CopyToAsync for very large files -// IFormFile buffers everything in memory first ΓÇö can cause OutOfMemoryException +// COMMON MISTAKE: Using IFormFile for very large files +// Multipart form parsing can buffer large uploads and consume memory/disk. +// Use MultipartReader for streaming directly to storage. ``` ## Common Mistakes 1. **Only configuring one size limit**: Must configure BOTH Kestrel `MaxRequestBodySize` AND `FormOptions.MultipartBodyLengthLimit`. -2. **400 errors from anti-forgery**: In .NET 8, `UseAntiforgery()` auto-validates form uploads. Use `.DisableAntiforgery()` for API endpoints. -3. **Trusting file.FileName**: User-provided filename can contain path traversal. Always generate a safe filename. -4. **Trusting Content-Type only**: Content type can be spoofed. Check magic bytes for actual file type. -5. **Using IFormFile for large files**: IFormFile buffers in memory. Use `MultipartReader` for streaming. -6. **Missing GetMultipartBoundary extension**: Must use `context.Request.GetMultipartBoundary()`, not parse manually. +2. **400 errors from anti-forgery**: In .NET 8+, `UseAntiforgery()` auto-validates form uploads. Use `.DisableAntiforgery()` for API endpoints (safe for JWT/unauthenticated; do NOT disable for cookie-authenticated endpoints). +3. **Trusting file.FileName**: User-provided filename can contain path traversal. Always generate a safe filename with `Guid.NewGuid()`. +4. **Trusting Content-Type only**: Content type is client-spoofable. Always check magic bytes for actual file type verification. +5. **Using IFormFile for very large files**: Multipart form parsing buffers with a memory threshold and spills to temp files. Use `MultipartReader` for streaming directly to storage. +6. **Deriving file extension from user input**: Use the validated content type or magic bytes to determine the extension, not `Path.GetExtension(file.FileName)`. diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index 0d39f682dc..80470ed33e 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -10,33 +10,34 @@ scenarios: pattern: "(IFormFile|IFormFileCollection)" - type: "output_matches" pattern: "(Guid\\.NewGuid|Path\\.GetRandomFileName)" - - type: "output_matches" - pattern: "(ContentType|image/jpeg|image/png)" rubric: - - "Configures BOTH Kestrel MaxRequestBodySize AND FormOptions.MultipartBodyLengthLimit" - - "Uses .DisableAntiforgery() on the file upload endpoint to prevent automatic 400 errors" - - "Generates a safe filename (Guid.NewGuid or similar) instead of using user-provided IFormFile.FileName directly" - - "Validates file content beyond just checking file extension — checks ContentType or magic bytes" - - "Correctly uses IFormFile parameter binding in the minimal API endpoint" + - "Configures both the Kestrel request body size limit and the form multipart body length limit — not just one of them" + - "Handles the antiforgery middleware that would otherwise reject the upload with a 400 error" + - "Does not save to disk using the original user-provided filename — generates a safe name to prevent path traversal" + - "Verifies the actual file content (e.g., magic bytes or file signatures), not just the Content-Type header which can be spoofed" timeout: 180 - - name: "Fix file upload returning 400 Bad Request" + - name: "Fix large file uploads failing despite increasing Kestrel limit" prompt: | - My ASP.NET Core 8 minimal API file upload endpoint returns 400 Bad Request with no error details. The endpoint works fine when I remove UseAntiforgery() from Program.cs. What's going on? + I'm trying to upload 50MB files in my ASP.NET Core minimal API. I increased the + Kestrel request body size limit to 100MB but uploads still fail with a + "Request body too large" error. What am I missing? setup: files: - path: "Program.cs" content: | var builder = WebApplication.CreateBuilder(args); - builder.Services.AddAntiforgery(); + builder.WebHost.ConfigureKestrel(options => + { + options.Limits.MaxRequestBodySize = 100 * 1024 * 1024; + }); var app = builder.Build(); - app.UseAntiforgery(); app.MapPost("/upload", (IFormFile file) => { return Results.Ok(new { file.FileName, file.Length }); }); app.Run(); - - path: "UploadBug.csproj" + - path: "UploadApp.csproj" content: | @@ -45,23 +46,18 @@ scenarios: assertions: - type: "output_matches" - pattern: "(DisableAntiforgery|antiforgery|anti-forgery)" + pattern: "(MultipartBodyLengthLimit|FormOptions)" rubric: - - "Identified the root cause: UseAntiforgery() middleware rejects file upload requests that lack an antiforgery token" - - "Recommended using .DisableAntiforgery() on the file upload endpoint" - - "Explained that minimal API file upload endpoints typically need antiforgery disabled since the token cannot be included in multipart form uploads from API clients" + - "Identified that there is a second, separate size limit (the multipart form body length limit) that also needs to be increased" + - "Provided a concrete fix that configures the form options multipart limit in addition to the Kestrel limit" + - "Explained why configuring only the Kestrel limit is insufficient — the form parser has its own independent limit" timeout: 180 - - name: "Upload multiple files with metadata in minimal API" + - name: "File upload should not use magic bytes for JSON API" + expect_activation: false prompt: | - I need to upload multiple files along with a description string in a single request to my ASP.NET Core 8 minimal API. How do I bind both the files and the form field? - assertions: - - type: "output_matches" - pattern: "(IFormFileCollection|IFormFile)" - - type: "output_matches" - pattern: "(FromForm|\\[FromForm\\])" + I'm building a REST API in ASP.NET Core that accepts JSON payloads. + How do I validate the request body schema? rubric: - - "Used IFormFileCollection for multiple files" - - "Used [FromForm] attribute on parameters when mixing files with other form fields" - - "Showed correct multipart/form-data binding pattern" - timeout: 180 + - "Response focuses on JSON validation (e.g., model binding, FluentValidation, data annotations) and does NOT suggest magic bytes or file upload patterns" + timeout: 60 From cdd8ba104d7aaca369e6db41851b9d1618d25a29 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Fri, 27 Mar 2026 04:58:11 -0700 Subject: [PATCH 04/16] Improve eval scenarios: remove noisy tests, add dual-limit and security review scenarios --- .../minimal-api-file-upload/eval.yaml | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index 80470ed33e..8eb86c9278 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -61,3 +61,48 @@ scenarios: rubric: - "Response focuses on JSON validation (e.g., model binding, FluentValidation, data annotations) and does NOT suggest magic bytes or file upload patterns" timeout: 60 + + - name: "Review file upload endpoint for security issues" + prompt: | + Please review this file upload endpoint for security issues and fix any problems you find. + setup: + files: + - path: "Program.cs" + content: | + using Microsoft.AspNetCore.Http; + + var builder = WebApplication.CreateBuilder(args); + builder.Services.AddAntiforgery(); + var app = builder.Build(); + app.UseAntiforgery(); + + app.MapPost("/api/upload", async (IFormFile file) => + { + var allowedTypes = new[] { "image/jpeg", "image/png" }; + if (!allowedTypes.Contains(file.ContentType)) + return Results.BadRequest("Invalid file type"); + + var uploadPath = Path.Combine("uploads", file.FileName); + Directory.CreateDirectory("uploads"); + using var stream = File.Create(uploadPath); + await file.CopyToAsync(stream); + + return Results.Ok(new { file.FileName, file.Length }); + }); + + app.Run(); + - path: "UploadApp.csproj" + content: | + + + net8.0 + + + assertions: + - type: "output_matches" + pattern: "(path traversal|safe.?name|Guid|sanitiz)" + rubric: + - "Identified that using file.FileName directly for the save path allows path traversal attacks" + - "Identified that checking only ContentType is insufficient because it can be spoofed by the client" + - "Identified that the antiforgery middleware will reject API upload requests with a 400 error" + timeout: 180 From afef016839c85962dfa5a568ec548c0d74bbe06e Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Fri, 27 Mar 2026 05:00:55 -0700 Subject: [PATCH 05/16] Add CODEOWNERS for aspnetcore plugin --- .github/CODEOWNERS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 14bd9a398c..b21d9d961b 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -79,6 +79,10 @@ /plugins/dotnet-ai/skills/technology-selection/ @luisquintanilla @artl93 /tests/dotnet-ai/technology-selection/ @luisquintanilla @artl93 +# aspnetcore (ASP.NET Core web development) +/plugins/aspnetcore/ @BrennanConroy @adityamandaleeka @halter73 +/tests/aspnetcore/ @BrennanConroy @adityamandaleeka @halter73 + # dotnet-data (data access, Entity Framework) /plugins/dotnet-data/skills/optimizing-ef-core-queries/ @dotnet/efteam /tests/dotnet-data/optimizing-ef-core-queries/ @dotnet/efteam From bc0604e97445d322e375bf686d7cc1574235fd61 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Fri, 27 Mar 2026 05:58:21 -0700 Subject: [PATCH 06/16] Fix eval: enrich prompts for skill activation, bump security review timeout to 240s --- tests/aspnetcore/minimal-api-file-upload/eval.yaml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index 8eb86c9278..af78665149 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -19,9 +19,10 @@ scenarios: - name: "Fix large file uploads failing despite increasing Kestrel limit" prompt: | - I'm trying to upload 50MB files in my ASP.NET Core minimal API. I increased the - Kestrel request body size limit to 100MB but uploads still fail with a - "Request body too large" error. What am I missing? + I'm trying to upload 50MB files in my ASP.NET Core 8 minimal API. I already + increased the Kestrel request body size limit to 100MB, but uploads still fail + with a "Request body too large" error when using IFormFile. Here's my Program.cs. + What am I missing? setup: files: - path: "Program.cs" @@ -64,7 +65,9 @@ scenarios: - name: "Review file upload endpoint for security issues" prompt: | - Please review this file upload endpoint for security issues and fix any problems you find. + I have an ASP.NET Core 8 minimal API with a file upload endpoint that accepts + image uploads. Please review the code in Program.cs for security issues and + suggest fixes. The endpoint uses IFormFile to receive uploaded files. setup: files: - path: "Program.cs" @@ -105,4 +108,4 @@ scenarios: - "Identified that using file.FileName directly for the save path allows path traversal attacks" - "Identified that checking only ContentType is insufficient because it can be spoofed by the client" - "Identified that the antiforgery middleware will reject API upload requests with a 400 error" - timeout: 180 + timeout: 240 From e426dc483b8fc522337033eb26f0422c160ed048 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Fri, 27 Mar 2026 06:27:41 -0700 Subject: [PATCH 07/16] Drop size-limit scenario (baseline already 5.0), keep 3 high-signal scenarios --- .../minimal-api-file-upload/eval.yaml | 37 ------------------- 1 file changed, 37 deletions(-) diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index af78665149..8332105991 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -17,43 +17,6 @@ scenarios: - "Verifies the actual file content (e.g., magic bytes or file signatures), not just the Content-Type header which can be spoofed" timeout: 180 - - name: "Fix large file uploads failing despite increasing Kestrel limit" - prompt: | - I'm trying to upload 50MB files in my ASP.NET Core 8 minimal API. I already - increased the Kestrel request body size limit to 100MB, but uploads still fail - with a "Request body too large" error when using IFormFile. Here's my Program.cs. - What am I missing? - setup: - files: - - path: "Program.cs" - content: | - var builder = WebApplication.CreateBuilder(args); - builder.WebHost.ConfigureKestrel(options => - { - options.Limits.MaxRequestBodySize = 100 * 1024 * 1024; - }); - var app = builder.Build(); - app.MapPost("/upload", (IFormFile file) => - { - return Results.Ok(new { file.FileName, file.Length }); - }); - app.Run(); - - path: "UploadApp.csproj" - content: | - - - net8.0 - - - assertions: - - type: "output_matches" - pattern: "(MultipartBodyLengthLimit|FormOptions)" - rubric: - - "Identified that there is a second, separate size limit (the multipart form body length limit) that also needs to be increased" - - "Provided a concrete fix that configures the form options multipart limit in addition to the Kestrel limit" - - "Explained why configuring only the Kestrel limit is insufficient — the form parser has its own independent limit" - timeout: 180 - - name: "File upload should not use magic bytes for JSON API" expect_activation: false prompt: | From 714da75df527cd7abb00cd83166c4af5db365a63 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Fri, 27 Mar 2026 07:22:04 -0700 Subject: [PATCH 08/16] Reframe security review as file upload fix to improve skill activation --- tests/aspnetcore/minimal-api-file-upload/eval.yaml | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index 8332105991..102c3058eb 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -26,11 +26,13 @@ scenarios: - "Response focuses on JSON validation (e.g., model binding, FluentValidation, data annotations) and does NOT suggest magic bytes or file upload patterns" timeout: 60 - - name: "Review file upload endpoint for security issues" + - name: "Fix broken file upload endpoint in ASP.NET Core minimal API" prompt: | - I have an ASP.NET Core 8 minimal API with a file upload endpoint that accepts - image uploads. Please review the code in Program.cs for security issues and - suggest fixes. The endpoint uses IFormFile to receive uploaded files. + My ASP.NET Core 8 minimal API file upload endpoint has several problems. + It's supposed to accept JPEG and PNG image uploads via IFormFile but it + doesn't work correctly in production — uploads fail, and I'm worried the + file handling isn't safe. Can you fix the file upload implementation in + Program.cs? setup: files: - path: "Program.cs" From d91cacffe10134caeb8e4d66bba6d6b275b65ac8 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Mon, 30 Mar 2026 11:14:28 -0700 Subject: [PATCH 09/16] Review --- .../skills/minimal-api-file-upload/SKILL.md | 19 +++-- .../minimal-api-file-upload/eval.yaml | 75 +++++++------------ 2 files changed, 39 insertions(+), 55 deletions(-) diff --git a/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md b/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md index db7ffa653e..15083e270e 100644 --- a/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md +++ b/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md @@ -144,11 +144,20 @@ app.MapPost("/upload", async (IFormFile file) => if (!isJpeg && !isPng) return Results.BadRequest("File content doesn't match declared type"); - // CRITICAL: Generate a safe filename — never use user-provided filename directly - // Derive the extension from the validated content type, not from user input + // CRITICAL: Never use the user-provided filename directly for the save path — it can + // contain path traversal characters (e.g., "../../../etc/passwd"). + // + // Option A (recommended): Generate a completely safe filename from the validated content var extension = isJpeg ? ".jpg" : ".png"; var safeFileName = $"{Guid.NewGuid()}{extension}"; - // NEVER: var path = Path.Combine("uploads", file.FileName); // Path traversal! + // + // Option B: If retaining the original filename matters (e.g., user-facing downloads), + // sanitize it first: strip path components, remove dangerous characters, and validate. + // var sanitized = Path.GetFileName(file.FileName); // strips directory parts + // sanitized = Regex.Replace(sanitized, @"[^\w.\-]", "_"); // allow only safe chars + // var safeFileName = $"{Guid.NewGuid()}_{sanitized}"; // prefix with GUID for uniqueness + // + // NEVER: var path = Path.Combine("uploads", file.FileName); // Path traversal! var filePath = Path.Combine("uploads", safeFileName); Directory.CreateDirectory("uploads"); @@ -216,7 +225,7 @@ app.MapPost("/upload-stream", 1. **Only configuring one size limit**: Must configure BOTH Kestrel `MaxRequestBodySize` AND `FormOptions.MultipartBodyLengthLimit`. 2. **400 errors from anti-forgery**: In .NET 8+, `UseAntiforgery()` auto-validates form uploads. Use `.DisableAntiforgery()` for API endpoints (safe for JWT/unauthenticated; do NOT disable for cookie-authenticated endpoints). -3. **Trusting file.FileName**: User-provided filename can contain path traversal. Always generate a safe filename with `Guid.NewGuid()`. +3. **Trusting file.FileName**: User-provided filename can contain path traversal. Either generate a safe filename with `Guid.NewGuid()`, or sanitize the original name by stripping path components (`Path.GetFileName`) and removing dangerous characters before use. 4. **Trusting Content-Type only**: Content type is client-spoofable. Always check magic bytes for actual file type verification. 5. **Using IFormFile for very large files**: Multipart form parsing buffers with a memory threshold and spills to temp files. Use `MultipartReader` for streaming directly to storage. -6. **Deriving file extension from user input**: Use the validated content type or magic bytes to determine the extension, not `Path.GetExtension(file.FileName)`. +6. **Deriving file extension from user input**: Prefer deriving the extension from the validated content type or magic bytes rather than `Path.GetExtension(file.FileName)`. If the original extension must be preserved, validate it against the detected content type. diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/aspnetcore/minimal-api-file-upload/eval.yaml index 102c3058eb..441097f87a 100644 --- a/tests/aspnetcore/minimal-api-file-upload/eval.yaml +++ b/tests/aspnetcore/minimal-api-file-upload/eval.yaml @@ -17,60 +17,35 @@ scenarios: - "Verifies the actual file content (e.g., magic bytes or file signatures), not just the Content-Type header which can be spoofed" timeout: 180 - - name: "File upload should not use magic bytes for JSON API" - expect_activation: false + - name: "Upload multiple files with metadata in minimal API" prompt: | - I'm building a REST API in ASP.NET Core that accepts JSON payloads. - How do I validate the request body schema? + Show me how to add a file upload endpoint to an ASP.NET Core 8 minimal API + that accepts multiple image files along with a text description field from + the same form submission. The total size can be up to 50MB. Just show me + the code — I don't need a full project. + assertions: + - type: "output_matches" + pattern: "(IFormFileCollection|IFormFile|List)" + - type: "output_matches" + pattern: "(MaxRequestBodySize|MultipartBodyLengthLimit)" rubric: - - "Response focuses on JSON validation (e.g., model binding, FluentValidation, data annotations) and does NOT suggest magic bytes or file upload patterns" - timeout: 60 + - "Configures both the Kestrel request body size limit and the form multipart body length limit for the 50MB requirement" + - "Uses [FromForm] correctly when mixing file parameters with non-file form fields like description" + - "Does not save uploaded files using the original user-provided filenames" + timeout: 180 - - name: "Fix broken file upload endpoint in ASP.NET Core minimal API" + - name: "Stream very large file uploads without buffering" prompt: | - My ASP.NET Core 8 minimal API file upload endpoint has several problems. - It's supposed to accept JPEG and PNG image uploads via IFormFile but it - doesn't work correctly in production — uploads fail, and I'm worried the - file handling isn't safe. Can you fix the file upload implementation in - Program.cs? - setup: - files: - - path: "Program.cs" - content: | - using Microsoft.AspNetCore.Http; - - var builder = WebApplication.CreateBuilder(args); - builder.Services.AddAntiforgery(); - var app = builder.Build(); - app.UseAntiforgery(); - - app.MapPost("/api/upload", async (IFormFile file) => - { - var allowedTypes = new[] { "image/jpeg", "image/png" }; - if (!allowedTypes.Contains(file.ContentType)) - return Results.BadRequest("Invalid file type"); - - var uploadPath = Path.Combine("uploads", file.FileName); - Directory.CreateDirectory("uploads"); - using var stream = File.Create(uploadPath); - await file.CopyToAsync(stream); - - return Results.Ok(new { file.FileName, file.Length }); - }); - - app.Run(); - - path: "UploadApp.csproj" - content: | - - - net8.0 - - + I need to accept uploads of video files up to 2GB in my ASP.NET Core 8 minimal API. + Using IFormFile crashes with out-of-memory errors for files over 500MB. + How do I stream large uploads directly to disk without buffering the whole file? assertions: - type: "output_matches" - pattern: "(path traversal|safe.?name|Guid|sanitiz)" + pattern: "(MultipartReader|ReadNextSectionAsync)" + - type: "output_matches" + pattern: "(DisableRequestSizeLimit|MaxRequestBodySize)" rubric: - - "Identified that using file.FileName directly for the save path allows path traversal attacks" - - "Identified that checking only ContentType is insufficient because it can be spoofed by the client" - - "Identified that the antiforgery middleware will reject API upload requests with a 400 error" - timeout: 240 + - "Uses MultipartReader to stream upload sections directly to disk instead of buffering via IFormFile" + - "Disables or increases the request size limit to accommodate 2GB files" + - "Does not use the user-provided filename directly when saving streamed sections to disk" + timeout: 180 From a77e824e95efaec4cb0a56a5267ba6d9491ed102 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Mon, 30 Mar 2026 13:14:14 -0700 Subject: [PATCH 10/16] Contribution guide followed --- .claude-plugin/marketplace.json | 5 +++++ .github/plugin/marketplace.json | 5 +++++ README.md | 1 + 3 files changed, 11 insertions(+) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 7a38356554..434697be2f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -53,6 +53,11 @@ "name": "dotnet-test", "source": "./plugins/dotnet-test", "description": "Skills for running, diagnosing, and migrating .NET tests: test execution, filtering, platform detection, and MSTest workflows." + }, + { + "name": "aspnetcore", + "source": "./plugins/aspnetcore", + "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns." } ] } \ No newline at end of file diff --git a/.github/plugin/marketplace.json b/.github/plugin/marketplace.json index 7a38356554..434697be2f 100644 --- a/.github/plugin/marketplace.json +++ b/.github/plugin/marketplace.json @@ -53,6 +53,11 @@ "name": "dotnet-test", "source": "./plugins/dotnet-test", "description": "Skills for running, diagnosing, and migrating .NET tests: test execution, filtering, platform detection, and MSTest workflows." + }, + { + "name": "aspnetcore", + "source": "./plugins/aspnetcore", + "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns." } ] } \ No newline at end of file diff --git a/README.md b/README.md index 7e14d680ee..6b8ba5b5e2 100644 --- a/README.md +++ b/README.md @@ -18,6 +18,7 @@ This repository contains the .NET team's curated set of core skills and custom a | [dotnet-ai](plugins/dotnet-ai/) | AI and ML skills for .NET: technology selection, LLM integration, agentic workflows, RAG pipelines, MCP, and classic ML with ML.NET. | | [dotnet-template-engine](plugins/dotnet-template-engine/) | .NET Template Engine skills: template discovery, project scaffolding, and template authoring. | | [dotnet-test](plugins/dotnet-test/) | Skills for running, diagnosing, and migrating .NET tests: test execution, filtering, platform detection, and MSTest workflows. | +| [aspnetcore](plugins/aspnetcore/) | ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns. | ## Installation From 218faf083ef7798cd3bf21143fb625479e1d3916 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Mon, 30 Mar 2026 13:20:18 -0700 Subject: [PATCH 11/16] Updated name --- .claude-plugin/marketplace.json | 4 ++-- .github/CODEOWNERS | 6 +++--- .github/plugin/marketplace.json | 4 ++-- README.md | 2 +- plugins/{aspnetcore => dotnet-aspnet}/plugin.json | 2 +- .../skills/minimal-api-file-upload/SKILL.md | 0 .../minimal-api-file-upload/eval.yaml | 0 7 files changed, 9 insertions(+), 9 deletions(-) rename plugins/{aspnetcore => dotnet-aspnet}/plugin.json (87%) rename plugins/{aspnetcore => dotnet-aspnet}/skills/minimal-api-file-upload/SKILL.md (100%) rename tests/{aspnetcore => dotnet-aspnet}/minimal-api-file-upload/eval.yaml (100%) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 434697be2f..17137fc7ae 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -55,8 +55,8 @@ "description": "Skills for running, diagnosing, and migrating .NET tests: test execution, filtering, platform detection, and MSTest workflows." }, { - "name": "aspnetcore", - "source": "./plugins/aspnetcore", + "name": "dotnet-aspnet", + "source": "./plugins/dotnet-aspnet", "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns." } ] diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index b21d9d961b..c900e0dbf0 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -79,9 +79,9 @@ /plugins/dotnet-ai/skills/technology-selection/ @luisquintanilla @artl93 /tests/dotnet-ai/technology-selection/ @luisquintanilla @artl93 -# aspnetcore (ASP.NET Core web development) -/plugins/aspnetcore/ @BrennanConroy @adityamandaleeka @halter73 -/tests/aspnetcore/ @BrennanConroy @adityamandaleeka @halter73 +# dotnet-aspnet (ASP.NET Core web development) +/plugins/dotnet-aspnet/ @BrennanConroy @adityamandaleeka @halter73 +/tests/dotnet-aspnet/ @BrennanConroy @adityamandaleeka @halter73 # dotnet-data (data access, Entity Framework) /plugins/dotnet-data/skills/optimizing-ef-core-queries/ @dotnet/efteam diff --git a/.github/plugin/marketplace.json b/.github/plugin/marketplace.json index 434697be2f..17137fc7ae 100644 --- a/.github/plugin/marketplace.json +++ b/.github/plugin/marketplace.json @@ -55,8 +55,8 @@ "description": "Skills for running, diagnosing, and migrating .NET tests: test execution, filtering, platform detection, and MSTest workflows." }, { - "name": "aspnetcore", - "source": "./plugins/aspnetcore", + "name": "dotnet-aspnet", + "source": "./plugins/dotnet-aspnet", "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns." } ] diff --git a/README.md b/README.md index 6b8ba5b5e2..28dcb6b57e 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ This repository contains the .NET team's curated set of core skills and custom a | [dotnet-ai](plugins/dotnet-ai/) | AI and ML skills for .NET: technology selection, LLM integration, agentic workflows, RAG pipelines, MCP, and classic ML with ML.NET. | | [dotnet-template-engine](plugins/dotnet-template-engine/) | .NET Template Engine skills: template discovery, project scaffolding, and template authoring. | | [dotnet-test](plugins/dotnet-test/) | Skills for running, diagnosing, and migrating .NET tests: test execution, filtering, platform detection, and MSTest workflows. | -| [aspnetcore](plugins/aspnetcore/) | ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns. | +| [dotnet-aspnet](plugins/dotnet-aspnet/) | ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns. | ## Installation diff --git a/plugins/aspnetcore/plugin.json b/plugins/dotnet-aspnet/plugin.json similarity index 87% rename from plugins/aspnetcore/plugin.json rename to plugins/dotnet-aspnet/plugin.json index 00c373186d..af8da7f565 100644 --- a/plugins/aspnetcore/plugin.json +++ b/plugins/dotnet-aspnet/plugin.json @@ -1,5 +1,5 @@ { - "name": "aspnetcore", + "name": "dotnet-aspnet", "version": "0.1.0", "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns.", "skills": "./skills/" diff --git a/plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md b/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md similarity index 100% rename from plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md rename to plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md diff --git a/tests/aspnetcore/minimal-api-file-upload/eval.yaml b/tests/dotnet-aspnet/minimal-api-file-upload/eval.yaml similarity index 100% rename from tests/aspnetcore/minimal-api-file-upload/eval.yaml rename to tests/dotnet-aspnet/minimal-api-file-upload/eval.yaml From 538c33c19f6b287a00cf1dbdb500291d936cc4f5 Mon Sep 17 00:00:00 2001 From: Viktor Hofer <7412651+ViktorHofer@users.noreply.github.com> Date: Mon, 30 Mar 2026 22:21:56 +0200 Subject: [PATCH 12/16] Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- plugins/dotnet-aspnet/plugin.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/dotnet-aspnet/plugin.json b/plugins/dotnet-aspnet/plugin.json index af8da7f565..9f8e6ba2ae 100644 --- a/plugins/dotnet-aspnet/plugin.json +++ b/plugins/dotnet-aspnet/plugin.json @@ -2,5 +2,5 @@ "name": "dotnet-aspnet", "version": "0.1.0", "description": "ASP.NET Core web development skills including middleware, endpoints, real-time communication, and API patterns.", - "skills": "./skills/" + "skills": ["./skills/"] } From 79395cbe45122704467c56f74267d2bbd5f29427 Mon Sep 17 00:00:00 2001 From: "Mukund Raghav Sharma (Moko)" <68247673+mrsharm@users.noreply.github.com> Date: Mon, 30 Mar 2026 13:54:45 -0700 Subject: [PATCH 13/16] Update plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md b/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md index 15083e270e..7e58702d03 100644 --- a/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md +++ b/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md @@ -184,8 +184,11 @@ app.MapPost("/upload-stream", if (contentType == null) return Results.BadRequest("Missing Content-Type"); - var boundary = HeaderUtilities.RemoveQuotes( - MediaTypeHeaderValue.Parse(contentType).Boundary).Value; + // Safely parse the Content-Type header to avoid FormatException from MediaTypeHeaderValue.Parse + if (!MediaTypeHeaderValue.TryParse(contentType, out var mediaType)) + return Results.BadRequest("Invalid Content-Type"); + + var boundary = HeaderUtilities.RemoveQuotes(mediaType.Boundary).Value; if (string.IsNullOrWhiteSpace(boundary)) return Results.BadRequest("Not a multipart request"); From a44a996b7a9adde68176f3fdb49de05398eb5906 Mon Sep 17 00:00:00 2001 From: Mukund Raghav Sharma Date: Mon, 30 Mar 2026 14:42:35 -0700 Subject: [PATCH 14/16] Address PR #264 review feedback from Brennan and Copilot - Rename plugin from aspnetcore to dotnet-aspnet per repo conventions - Fix streaming filename: remove user-controlled extension, use bare GUID - Remove explicit sanitization code (Option B) per Brennan's feedback - Add case-insensitive MIME type comparison - Validate ContentType matches magic bytes (reject mismatches) - Fix stream rewind + CopyToAsync: copy from already-opened stream - Reword streaming section to accurately describe buffering vs chunks - Update marketplace.json, README.md, CODEOWNERS for dotnet-aspnet rename --- .../skills/minimal-api-file-upload/SKILL.md | 42 +- pr-264-description.md | 42 + pr-comments.md | 792 ++++++++++++++++++ 3 files changed, 855 insertions(+), 21 deletions(-) create mode 100644 pr-264-description.md create mode 100644 pr-comments.md diff --git a/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md b/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md index 7e58702d03..829980df04 100644 --- a/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md +++ b/plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md @@ -125,7 +125,7 @@ app.MapPost("/upload", async (IFormFile file) => // Allow only JPEG/PNG by default. To support more (e.g., GIF), // add the MIME type here AND validate its magic bytes below. var allowedTypes = new[] { "image/jpeg", "image/png" }; - if (!allowedTypes.Contains(file.ContentType)) + if (!allowedTypes.Contains(file.ContentType, StringComparer.OrdinalIgnoreCase)) return Results.BadRequest("File type not allowed"); // CRITICAL: Check magic bytes for file type verification @@ -134,35 +134,33 @@ app.MapPost("/upload", async (IFormFile file) => var bytesRead = await stream.ReadAsync(header, 0, header.Length); if (bytesRead < 4) return Results.BadRequest("File content is too short or invalid"); - stream.Position = 0; // JPEG: FF D8 FF // PNG: 89 50 4E 47 var isJpeg = header[0] == 0xFF && header[1] == 0xD8 && header[2] == 0xFF; var isPng = header[0] == 0x89 && header[1] == 0x50 && header[2] == 0x4E && header[3] == 0x47; - if (!isJpeg && !isPng) - return Results.BadRequest("File content doesn't match declared type"); + // Determine the actual content type from magic bytes + string? detectedContentType = isJpeg ? "image/jpeg" : isPng ? "image/png" : null; + if (detectedContentType is null) + return Results.BadRequest("File content is not a supported image format (only JPEG and PNG are allowed)."); + + // Ensure the declared Content-Type matches what the magic bytes detected + if (!string.Equals(file.ContentType, detectedContentType, StringComparison.OrdinalIgnoreCase)) + return Results.BadRequest("File content type does not match the declared ContentType header."); // CRITICAL: Never use the user-provided filename directly for the save path — it can // contain path traversal characters (e.g., "../../../etc/passwd"). - // - // Option A (recommended): Generate a completely safe filename from the validated content - var extension = isJpeg ? ".jpg" : ".png"; + // Generate a safe filename; derive the extension from validated content, not user input. + var extension = detectedContentType == "image/jpeg" ? ".jpg" : ".png"; var safeFileName = $"{Guid.NewGuid()}{extension}"; - // - // Option B: If retaining the original filename matters (e.g., user-facing downloads), - // sanitize it first: strip path components, remove dangerous characters, and validate. - // var sanitized = Path.GetFileName(file.FileName); // strips directory parts - // sanitized = Regex.Replace(sanitized, @"[^\w.\-]", "_"); // allow only safe chars - // var safeFileName = $"{Guid.NewGuid()}_{sanitized}"; // prefix with GUID for uniqueness - // // NEVER: var path = Path.Combine("uploads", file.FileName); // Path traversal! var filePath = Path.Combine("uploads", safeFileName); Directory.CreateDirectory("uploads"); + stream.Position = 0; using var fileStream = File.Create(filePath); - await file.CopyToAsync(fileStream); + await stream.CopyToAsync(fileStream); return Results.Ok(new { FileName = safeFileName, file.Length }); }); @@ -171,9 +169,11 @@ app.MapPost("/upload", async (IFormFile file) => ### Step 5: CRITICAL — Streaming Large Files Without Buffering ```csharp -// CRITICAL: IFormFile uses multipart form parsing that buffers with a memory threshold -// and spills to temp files. For very large uploads this can consume significant -// memory and disk. Use MultipartReader for streaming without buffering. +// CRITICAL: IFormFile relies on multipart form parsing that buffers content in memory +// (up to a threshold) then spills to temp files on disk. For very large uploads, +// this overhead is unnecessary if you can process the data in chunks. +// Use MultipartReader to stream directly — e.g., to a final storage location — +// without buffering the entire file first. app.MapPost("/upload-stream", [DisableRequestSizeLimit] @@ -207,7 +207,7 @@ app.MapPost("/upload-stream", // Sanitize the user-provided filename to prevent path traversal var originalFileName = contentDisposition.FileName.Value ?? string.Empty; var sanitizedFileName = Path.GetFileName(originalFileName.Trim('"')); - var safeFile = $"{Guid.NewGuid()}{Path.GetExtension(sanitizedFileName)}"; + var safeFile = $"{Guid.NewGuid()}"; // CRITICAL: Stream directly to disk — avoids buffering in memory Directory.CreateDirectory("uploads"); @@ -228,7 +228,7 @@ app.MapPost("/upload-stream", 1. **Only configuring one size limit**: Must configure BOTH Kestrel `MaxRequestBodySize` AND `FormOptions.MultipartBodyLengthLimit`. 2. **400 errors from anti-forgery**: In .NET 8+, `UseAntiforgery()` auto-validates form uploads. Use `.DisableAntiforgery()` for API endpoints (safe for JWT/unauthenticated; do NOT disable for cookie-authenticated endpoints). -3. **Trusting file.FileName**: User-provided filename can contain path traversal. Either generate a safe filename with `Guid.NewGuid()`, or sanitize the original name by stripping path components (`Path.GetFileName`) and removing dangerous characters before use. +3. **Trusting file.FileName**: User-provided filename can contain path traversal. Generate a safe filename with `Guid.NewGuid()` and derive the extension from validated content. 4. **Trusting Content-Type only**: Content type is client-spoofable. Always check magic bytes for actual file type verification. -5. **Using IFormFile for very large files**: Multipart form parsing buffers with a memory threshold and spills to temp files. Use `MultipartReader` for streaming directly to storage. +5. **Using IFormFile for very large files**: Multipart form parsing buffers with a memory threshold and spills to temp files. Use `MultipartReader` to stream data in chunks directly to storage without buffering the entire file. 6. **Deriving file extension from user input**: Prefer deriving the extension from the validated content type or magic bytes rather than `Path.GetExtension(file.FileName)`. If the original extension must be preserved, validate it against the detected content type. diff --git a/pr-264-description.md b/pr-264-description.md new file mode 100644 index 0000000000..3b8517c48b --- /dev/null +++ b/pr-264-description.md @@ -0,0 +1,42 @@ +## Summary +Adds the **minimal-api-file-upload** skill for handling file uploads in ASP.NET Core 8+ minimal APIs. + +> **Note:** Replaces #155 (migrated from skills-old repo to new plugins/ structure). + +## What the Skill Teaches + +The base model consistently gets several file upload patterns wrong. This skill addresses five key gaps: + +1. **Dual size limits** — Must configure BOTH `Kestrel.MaxRequestBodySize` AND `FormOptions.MultipartBodyLengthLimit`. The base model typically configures only one, causing cryptic upload failures. +2. **Antiforgery auto-validation** — In .NET 8+, `UseAntiforgery()` silently rejects all form-bound uploads with 400 Bad Request. Must call `.DisableAntiforgery()` on API endpoints (with appropriate security caveats for cookie-auth). +3. **Magic byte validation** — The base model relies solely on `ContentType` which is client-spoofable. The skill teaches JPEG/PNG file signature verification. +4. **Safe filename generation** — The base model often uses `file.FileName` directly, enabling path traversal. The skill teaches GUID-based names and also covers sanitizing user-provided filenames when they must be retained (per reviewer feedback from @mikekistler). +5. **Streaming with MultipartReader** — For very large files (>500MB), IFormFile buffering causes OOM. The skill teaches direct-to-disk streaming via `MultipartReader`. + +## Eval Results (3-run local validation) + +| Scenario | Baseline | With Skill | Δ | Overfit | Verdict | +|----------|----------|------------|---|---------|---------| +| Implement secure file upload | 3.7/5 | **5.0/5** | +35% | 0.08 ✅ | ✅ | +| Upload multiple files with metadata | 3.3/5 | **5.0/5** | +52% | 0.08 ✅ | ✅ | +| Stream very large file uploads | 4.7/5 | **5.0/5** | +6% | 0.08 ✅ | ✅ | + +Model: claude-opus-4.6 | Judge: claude-opus-4.6 + +## Files + +- `plugins/dotnet-aspnet/plugin.json` (new plugin) +- `plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md` +- `tests/dotnet-aspnet/minimal-api-file-upload/eval.yaml` + +## Review feedback addressed + +- Added filename sanitization as valid alternative to GUID-only approach (per @mikekistler) +- Removed code fence wrapper from SKILL.md frontmatter (per Copilot review) +- Removed `image/gif` from allowlist to match JPEG/PNG-only eval scope +- Fixed `GetMultipartBoundary()` — replaced with standard `HeaderUtilities` boundary parsing +- Fixed misleading "IFormFile buffers entirely in memory" claim — clarified memory threshold + temp file behavior +- Derived file extension from validated magic bytes instead of user-controlled `Path.GetExtension(file.FileName)` +- Added security caveat for `DisableAntiforgery()` on cookie-authenticated endpoints (per @halter73) +- Moved skill from `plugins/dotnet` to `plugins/dotnet-aspnet` per reviewer guidance +- Iterated eval scenarios through 5+ runs — removed noisy tests (baseline-perfect antiforgery diagnosis, non-activation JSON test with efficiency noise), simplified multi-file prompt to prevent agent project scaffolding overhead diff --git a/pr-comments.md b/pr-comments.md new file mode 100644 index 0000000000..0649b5ec05 --- /dev/null +++ b/pr-comments.md @@ -0,0 +1,792 @@ +# PR Comments Summary + +## Table of Contents +- [PR #264 — Add minimal-api-file-upload skill](#pr-264--add-minimal-api-file-upload-skill) + - [Old PR #155 (closed)](#old-pr-155-closed--add-minimal-api-file-upload-skill) +- [PR #200 — Add migrating-newtonsoft-to-system-text-json skill](#pr-200--add-migrating-newtonsoft-to-system-text-json-skill) + - [Old PR #89 (closed)](#old-pr-89-closed--add-migrating-newtonsoft-to-system-text-json-skill) +- [PR #268 — Add configuring-opentelemetry-dotnet skill](#pr-268--add-configuring-opentelemetry-dotnet-skill) + - [Old PR #91 (closed)](#old-pr-91-closed--add-configuring-opentelemetry-dotnet-skill) +- [PR #269 — Add refactoring-to-async skill](#pr-269--add-refactoring-to-async-skill) + - [Old PR #79 (closed)](#old-pr-79-closed--add-refactoring-to-async-skill) + +--- + +# PR #264 — Add minimal-api-file-upload skill + +**State:** Open | **Author:** mrsharm | **Created:** 2026-03-06 | **Comments:** 23 +**Replaces:** #155 + +## Discussion Comments + +### mrsharm (2026-03-06) +> **Migration Note** +> +> This PR replaces #155 which was opened from `mrsharm/skills-old`. The skill and eval files have been migrated to the new `plugins/` directory structure: +> +> - `src/dotnet/skills/minimal-api-file-upload/` → `plugins/dotnet/skills/minimal-api-file-upload/` +> - `src/dotnet/tests/minimal-api-file-upload/` → `tests/dotnet/minimal-api-file-upload/` +> +> All prior review feedback from #155 still applies — please see that PR for the full discussion history. + +### mrsharm (2026-03-06) +> **Feedback carried over from #155** +> +> **Copilot** on SKILL.md (line 43): Step 1 is internally contradictory: it's titled as if IFormFile requires [FromForm] (not automatic), but later in this section it states IFormFile *is* bound automatically in .NET 8. Please rewrite this section to present one clear rule. +> +> **Copilot** on SKILL.md (line 138): The "safe filename" still uses `Path.GetExtension(file.FileName)`, which derives the extension from user-controlled input. Prefer choosing the extension based on the validated signature. +> +> **Copilot** on SKILL.md (line 163): `context.Request.GetMultipartBoundary()` isn't a built-in ASP.NET Core API. Please include the helper implementation or switch to the standard boundary parsing approach. +> +> **Copilot** on eval.yaml (line 12): Rubric criterion allows validating only `ContentType`, but the PR description explicitly calls out ContentType as client-spoofable. Tighten this rubric to require signature/magic-byte checking. +> +> **Copilot** on SKILL.md (line 64): These examples set the global Kestrel request body limit to 100MB, which can be confusing given the scenario is about enforcing a 10MB maximum. +> +> **Copilot** on SKILL.md (line 125): When reading magic bytes, the code ignores the return value from ReadAsync. Capture the bytes-read and fail fast if fewer than the required bytes are available. +> +> **Copilot** on SKILL.md (line 118): The allowed MIME types include `image/gif`, but the scenario/rubric for this skill is JPEG + PNG only. +> +> **Copilot** on SKILL.md (line 153): The statement that "IFormFile buffers the entire file in memory by default" is inaccurate for ASP.NET Core. Multipart parsing uses buffering with a memory threshold and typically spills to a temp file. +> +> **Copilot** on SKILL.md (line 172): `section.GetContentDispositionHeader()` / `contentDisposition.IsFileDisposition()` also aren't built-in APIs. +> +> **timheuer**: This feels _too_ verbose a name -- "minimal-api-file-upload"? +> +> **timheuer**: Any validation that "8" is going to influence too much? +> +> **timheuer**: Strike "8" and put more information in the 'when to use'? +> +> **timheuer**: - File upload endpoints in ASP.NET minimal APIs (.NET 8+) +> +> **halter73** on SKILL.md (line 108): @GrabYourPitchforks @blowdart This should be okay for unauthenticated endpoints and endpoints using JWT bearer authentication, but I worry that this might cause people to disable antiforgery for endpoints authenticated with cookies. +> +> **ViktorHofer**: As discussed offline, this PR will need to be re-submitted from a connected fork. Also please update this PR based on the new repo folder structure (plugins instead of src). + +### ViktorHofer (2026-03-08) +> Same feedback as in the other PRs regarding the skill not getting activated. If intentional, add `expect_activation: false` + +### ManishJayaswal (2026-03-10) +> @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. This skill should be under ASP plugin. Please update the PR and submit again. +> @adityamandaleeka @BrennanConroy - please review + +### danmoseley (2026-03-24) +> If #207 creates a dotnet-aspnet plugin, this skill should presumably move there. + +### danmoseley (2026-03-25) +> [running evaluate just to check the new evaluate improvements...] + +### danmoseley (2026-03-25) — Eval Failure Diagnosis +> **Verdict: FAIL** — overall +2.5% improvement (needs ≥10%) +> +> | Scenario | Quality | Score | Status | +> |----------|---------|-------|--------| +> | Implement secure upload | 4.3→5.0 | +15.1% | ✅ | +> | Fix 400 Bad Request | **5.0→4.3** | **-20.7%** | ❌ timeout | +> | Upload multiple + metadata | 3.7→5.0 | +40.5% | ✅ | +> +> **Scenario 2 is sinking the entire eval.** Scenarios 1 & 3 are solid. +> +> **Root Causes (priority order):** +> 1. **Timeout too low (60s)** — fix first. The skilled runs hit the 60s ceiling. **Fix:** `timeout: 60` → `timeout: 180` +> 2. **Baseline already perfect (5.0/5)** — The anti-forgery gotcha is well-documented. No headroom. **Fix options:** Add `reject_tools: ["bash", "edit"]`, or tighten the prompt, or remove the scenario. +> 3. **Rubric item #3 flagged as "technique" by overfitting analysis** — Soften to something like: "Explained why file upload endpoints commonly need antiforgery handling adjusted" +> +> **Recommended Fix Order:** +> 1. Increase timeout for scenario 2: `60` → `180` +> 2. Add `reject_tools: ["bash", "edit"]` to scenario 2 +> 3. *(Optional)* Soften rubric item 3 + +## Review Comments (PR #264) + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` +> In the streaming example, `contentDisposition.FileName.Value` is used directly to compute the extension. This value can include paths and other unexpected content; using it directly reintroduces path traversal / spoofing risks. Prefer extracting a safe base name first. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` +> The text claims `IFormFile` "buffers the entire file in memory by default". ASP.NET Core typically buffers form bodies with a memory threshold and spills to disk, so the guidance here is misleading. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` +> The Step 1 heading/example says it's a "COMMON MISTAKE" to expect `IFormFile` to bind automatically, but immediately below it says `IFormFile IS bound automatically from form data in .NET 8`. This is internally inconsistent. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` +> The allowlist includes `image/gif`, but the eval scenario and the surrounding guidance emphasize JPEG/PNG-only uploads. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` L151 +> `safeFileName` uses `Path.GetExtension(file.FileName)`, which is attacker-controlled. Prefer deriving the extension from the validated file signature/content type. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` +> This snippet calls `context.Request.GetMultipartBoundary()`, but that isn't a standard ASP.NET Core API. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/plugin.json` L5 +> Plugin manifests in this repo conventionally use the array form for `skills` (e.g., `"skills": ["./skills/"]`). + +### copilot-pull-request-reviewer — `plugins/aspnetcore/plugin.json` L6 +> The PR description lists paths under `plugins/dotnet/...` and `tests/dotnet/...`, but this PR adds an `aspnetcore` plugin. Please update the PR description. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` +> The markdown contains mojibake/encoding artifacts (e.g., "ΓåÆ", "ΓÇö"). Replace these with proper Unicode characters. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` +> Step 1 is internally contradictory: the heading says IFormFile binding is "not automatic", but the snippet then states it's bound automatically in .NET 8. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L144 +> The content-type allowlist includes "image/gif" but the subsequent magic-bytes validation only recognizes JPEG/PNG, so GIF uploads would pass the ContentType check and then fail later. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` +> The example reads 8 bytes from the stream but doesn't verify how many bytes were actually read. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` +> This states that IFormFile buffers the entire file in memory by default, which is misleading in ASP.NET Core. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` +> The sample calls `context.Request.GetMultipartBoundary()`, but there is no such built-in API. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L198 +> In the streaming example, the saved filename's extension is taken from the user-supplied filename. Prefer deriving the extension from validated content. + +### copilot-pull-request-reviewer — `tests/aspnetcore/minimal-api-file-upload/eval.yaml` L3 +> The PR description's file list references `plugins/dotnet/...` and `tests/dotnet/...`, but this PR adds the skill under `plugins/aspnetcore/...` and `tests/aspnetcore/...`. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L25 +> The markdown table under **Inputs** uses `||` at the start of each row/header, which doesn't render as a proper table. + +### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L138 +> In the magic-bytes example, the stream is rewound (`stream.Position = 0;`) but the subsequent save uses `file.CopyToAsync(...)`, which opens a new stream and ignores the rewound one. + +### mikekistler — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L147 (2026-03-28) +> I think another valid approach is to validate the filename provided before using it. Often the filename provided by the user has some significance and must be retained. + +--- + +## Old PR #155 (closed) — Add minimal-api-file-upload skill + +**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-03-02 | **Closed:** 2026-03-06 + +### Discussion Comments + +#### mrsharm (2026-03-02) — Eval Results +> ### 3-Run Validation: +38.9% PASS +> +> | Metric | Value | +> |--------|-------| +> | Overall Improvement | **+38.9%** | +> | Confidence Interval | [+9.1%, +62.5%] significant | +> | Effect Size (g) | +100.0% | +> | Baseline Quality | 3.0/5 | +> | Skill Quality | 5.0/5 | +> +> Baseline consistently misses: Only configures one of the two required size limits, relies on ContentType alone for validation, uses user-provided filenames without sanitization. + +#### ViktorHofer (2026-03-04) +> As discussed offline in the "dotnet/skills content" chat, this PR will need to be re-submitted from a connected fork. Also please update this PR based on the new repo folder structure (plugins instead of src). + +#### mrsharm (2026-03-06) +> Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. + +### Review Comments (PR #155) + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L43 +> Step 1 is internally contradictory: it's titled as if IFormFile requires [FromForm] (not automatic), but later in this section it states IFormFile *is* bound automatically in .NET 8. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L138 +> The "safe filename" still uses `Path.GetExtension(file.FileName)`, which derives the extension from user-controlled input. Prefer choosing the extension based on the validated signature. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L163 +> This example uses `context.Request.GetMultipartBoundary()`, but that helper isn't defined anywhere in this repo and isn't a built-in ASP.NET Core API. + +#### copilot-pull-request-reviewer — `src/dotnet/tests/minimal-api-file-upload/eval.yaml` L12 +> Rubric criterion allows validating only `ContentType`, but the PR description explicitly calls out ContentType as client-spoofable. Tighten this rubric to require signature/magic-byte checking. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L64 +> These examples set the global Kestrel request body limit to 100MB, which can be confusing given the scenario is about enforcing a 10MB maximum. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L125 +> When reading magic bytes, the code ignores the return value from ReadAsync. Capture the bytes-read and fail fast if fewer than the required bytes are available. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L118 +> The allowed MIME types include `image/gif`, but the scenario/rubric for this skill is JPEG + PNG only. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L153 +> The statement that "IFormFile buffers the entire file in memory by default" is inaccurate for ASP.NET Core. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L172 +> `section.GetContentDispositionHeader()` / `contentDisposition.IsFileDisposition()` aren't built-in APIs. + +#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` +> This feels _too_ verbose a name -- "minimal-api-file-upload"? + +#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` +> Any validation that "8" is going to influence too much? + +#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` +> Strike "8" and put more information in the 'when to use'? (note below) + +#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` +> - File upload endpoints in ASP.NET minimal APIs (.NET 8+) + +#### halter73 — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L108 +> @GrabYourPitchforks @blowdart This should be okay for unauthenticated endpoints and endpoints using JWT bearer authentication, but I worry that this might cause people to disable antiforgery for endpoints authenticated with cookies. I wonder what the best way to communicate this potential security threat in the skill document. + +--- + +# PR #200 — Add migrating-newtonsoft-to-system-text-json skill + +**State:** Open | **Author:** mrsharm | **Created:** 2026-03-04 | **Comments:** 9 +**Replaces:** #89 + +## Discussion Comments + +### danmoseley (2026-03-07) +> now we have the new plugins factoring in main, this should not be in the general dotnet plugin. please move it to the dotnet-upgrade plugin which seems a better fit. + +### timheuer (2026-03-09) +> This should go in `dotnet-upgrade` plugin + +### ManishJayaswal (2026-03-10) +> @vijayrkn @tlmii @wtgodbe @noahfalk - please review + +### abpiskunov (2026-03-10) — Detailed Skill Analysis +> I ran this new skill with Claude Opus 4.6 and pointed it to anthropics best practices link just in case and here what it said. Also i am wondering about evals file in this PR. I copied prompt from it to Gemini chat and Claude chat without this skill provided in fresh chat. Both generated same code that would pass evals 100% as far as i can tell... That raises question about the skill content and how efficient are evals. +> +> **What to Keep (High Value):** +> - Behavioral differences table (Step 1) — enforces completeness +> - ConfigureJsonOptions method (Step 2) — specific combo Claude won't reliably produce in full +> - Attribute mapping table (Step 3) — quick-reference, enforces complete coverage +> - Common Pitfalls table — highest-value section +> - Validation checklist — enforces completeness +> - grep commands for finding Newtonsoft usages +> +> **What to Cut or Compress:** +> - Full converter before/after code (Step 4) — Replace with "Key differences in converter API" bullet list only +> - Full JToken/JObject replacement examples (Step 5) — Keep the mapping table, cut the JsonNode mutable DOM code example +> - Polymorphic serialization section (Step 6) — Cut entirely. Claude knows [JsonDerivedType] thoroughly +> - Package reference / using statement changes (Step 7) — Cut the full XML and using-statement blocks +> - The "When Not to Use" section — "already using System.Text.Json" and "user wants to keep Newtonsoft" are obvious +> +> **What to Add:** +> - Conditional workflow at the top (ASP.NET Core app vs class library vs console/worker app) +> - Copy-and-track checklist — Restructure validation section into pattern with checkboxes +> - Explicit feedback loop — After each major step, add: "Run dotnet build to verify no compilation errors" +> - More trigger keywords in the description — "Json.NET", "removing Newtonsoft dependency", "serialization breaking changes", "AOT compatibility" +> +> **Net Effect:** ~250 lines → ~140-160 lines, focused on completeness-enforcement content. ~40% token reduction while increasing practical value. + +### danmoseley (2026-03-28) +> evaluation looks good. @mrsharm you just need review signoffs — particularly @eiriktsarpalis who made the requested change review. I believe I addressed that. + +## Review Comments (PR #200) + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKILL.md` (resolved) +> This repeats the earlier claim that `JsonExtensionData` "must" use `Dictionary` and that `Dictionary` is invalid. System.Text.Json supports extension data on `IDictionary` as well. + +### copilot-pull-request-reviewer — `tests/dotnet-upgrade/.../eval.yaml` L5 (resolved) +> This eval is being added under `src/dotnet/tests/...`, but the repo layout expects scenarios under `tests///eval.yaml`. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) +> `DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull` is not Newtonsoft.Json's default (Json.NET includes nulls unless configured). + +### copilot-pull-request-reviewer — `tests/dotnet/.../eval.yaml` (resolved) +> The rubric's casing note is inconsistent with the skill doc. + +### copilot-pull-request-reviewer — `tests/dotnet/.../eval.yaml` (resolved) +> `expect_tools: ["bash"]` forces the model to make at least one bash tool call. This prompt is primarily a code-migration explanation and doesn't require shell usage. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) +> This SKILL.md is wrapped in a fenced code block (```skill ... ```), which means the YAML frontmatter and the rest of the content won't be parsed/rendered like other skills. + +### copilot-pull-request-reviewer — `plugins/dotnet-upgrade/skills/.../SKILL.md` L223 (resolved) +> The polymorphism section implies System.Text.Json will automatically emit a discriminator for `[JsonDerivedType]` types. STJ polymorphism requires explicit opt-in/configuration. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) +> The doc contradicts itself on default property naming. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) +> `[JsonExtensionData]` doesn't have to be `Dictionary` only. + +### copilot-pull-request-reviewer — `plugins/dotnet-upgrade/skills/.../SKILL.md` L215 (resolved) +> This example configures `JsonSerializerSettings` with `TypeNameHandling = TypeNameHandling.Auto`, which is a known unsafe pattern when used with untrusted JSON. + +### GrabYourPitchforks — `plugins/dotnet/skills/.../SKILL.md` (resolved, 2 comments) +> Setting `Preserve` significantly increases the attack surface of JSON deserialization. It shouldn't be promoted as a "try it and see if it solves your problem" mechanism. +> +> To defend against this attack, the application should consider what happens if the adversary controls the _edges_ in the object graph, not just the _values_ of the nodes. + +### GrabYourPitchforks / eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L88 (resolved, 3 comments) +> **GrabYourPitchforks:** Similarly, this is a potentially dangerous setting [ReadCommentHandling] and should not be encouraged unless the application has thought through the consequences. We chose not to allow this by default in S.T.J because it could lead to desynced deserialization attacks. +> +> **GrabYourPitchforks:** To defend against this attack, the application should: 1) ensure all deserializers operate in strict RFC compliance mode; or 2) ensure all components use the same deserializer library; or 3) ensure the frontend reserializes the payload before sending to backend. +> +> **eiriktsarpalis:** I would add that case insensitive property handling similarly makes systems susceptible to interoperability attacks. It should start off assuming the highest bar possible (using `JsonSerializerDefaults.Strict`) and then instruct the agent to interview the user about particular requirements that force the loosening of individual settings. + +### JamesNK — `plugins/dotnet/skills/.../SKILL.md` (resolved) +> I think just always recommend JsonNode and friends. It's closer than JsonDocument/JsonElement. + +### JamesNK / eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L41 (resolved, 3 comments) +> **JamesNK:** STJ also escapes non-ASCII characters by default. Should recommend using relaxed encoder. +> +> **eiriktsarpalis:** I think we should caveat any such recommendation with the trade-offs: escaped JSON strings are semantically equivalent to unescaped ones. +> +> **JamesNK:** I'll leave it up to you. But if it's text being embedded in an HTML page then asp.net core will escape HTML characters anyway. I've never understood why STJ does this. + +### eiriktsarpalis — `plugins/dotnet/skills/.../SKILL.md` (resolved) +> I'm pretty sure newer models can figure out regexes like that on their own, so I would just skip that part for fear of it going out of sync. + +### ViktorHofer — `PR-FEEDBACK.md` (resolved) +> Remove this file + +### eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L14 (**open**, 2026-03-30) +> Consider adding an explicit instruction to the agent that it should be adding baseline serialization tests for the application models while performing the migration. + +### eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L284 (**open**, 2026-03-30) +> Consider adding links to the official STJ docs at learn. + +### eiriktsarpalis — `tests/dotnet-upgrade/.../eval.yaml` L2 (**open**, 2026-03-30) +> Shouldn't we add evals covering more scenarios? Custom converters? Naming policies? etc? Consider mining real-world snippets either from dotnet org repos or public github repos in general (e.g. via grep.app) + +--- + +## Old PR #89 (closed) — Add migrating-newtonsoft-to-system-text-json skill + +**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-02-23 | **Closed:** 2026-03-04 + +### Discussion Comments + +#### mrsharm (2026-02-25) — Eval Results (posted twice) +> | Skill | Test | Baseline | With Skill | Δ | Verdict | +> |-------|------|----------|------------|---|---------| +> | migrating-newtonsoft-to-system-text-json | Migrate model with Newtonsoft.Json attributes to System.Text.Json | 4.0/5 | 4.3/5 | +0.3 | ✅ | +> +> **Overall improvement: +10.8%** (3 runs, not statistically significant) + +#### mrsharm (2026-02-26) +> Any feedback here? @ericstj @mcastro-x? + +### Review Comments (PR #89) + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` (resolved) +> The claim that System.Text.Json "throws by default (.NET 8+)" for extra JSON properties is incorrect. System.Text.Json ignores extra JSON properties by default across all versions. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` (resolved) +> The claim about null to non-nullable value type behavior is misleading. Both Newtonsoft.Json and System.Text.Json set non-nullable value types to their default values. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L68 +> The comment "Newtonsoft default" on line 68 is misleading. PropertyNameCaseInsensitive is not a Newtonsoft.Json default behavior. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` (resolved) +> The claim that Newtonsoft.Json uses "camelCase by default" for property naming is incorrect. + +#### copilot-pull-request-reviewer — `src/dotnet/tests/.../eval.yaml` L45 +> The rubric item claims PropertyNameCaseInsensitive should be configured "to match Newtonsoft default case-insensitive behavior", but Newtonsoft.Json is case-sensitive by default. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L5 +> The skill metadata format is incorrect. Skills in this repository use standard YAML frontmatter delimited by `---`, not code fence blocks with ` ```skill`. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L44 +> The claim that Newtonsoft.Json is "case-insensitive" by default is incorrect. + +#### copilot-pull-request-reviewer — `src/dotnet/tests/.../eval.yaml` +> The prompt asks to "match Newtonsoft.Json's default behavior", but this will lead to incorrect configuration guidance. + +#### copilot-pull-request-reviewer — `src/dotnet/tests/.../eval.yaml` +> The rubric expects warnings about "default casing" as a behavioral difference, but since both libraries use the same default casing, this warning would be misleading. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L243 +> The pitfall "Forgetting PropertyNameCaseInsensitive = true" implies this is a required configuration to match Newtonsoft.Json behavior, but this is not accurate. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L67 +> The comment "Newtonsoft default" on line 67 is incorrect. Newtonsoft.Json does not use camelCase by default. + +--- + +# PR #268 — Add configuring-opentelemetry-dotnet skill + +**State:** Open | **Author:** mrsharm | **Created:** 2026-03-06 | **Comments:** 12 +**Replaces:** #91 + +## Discussion Comments + +### mrsharm (2026-03-06) +> **Migration Note** +> +> This PR replaces #91 which was opened from `mrsharm/skills-old`. All prior review feedback from #91 still applies. + +### mrsharm (2026-03-06) — Feedback carried over from #91 +> | Reviewer | Feedback | +> |----------|----------| +> | copilot | Missing `using OpenTelemetry.Trace;` for `SetStatus()`/`RecordException()` | +> | copilot | File wrapped in `` ```skill `` code block — metadata won't parse | +> | copilot | `return order;` references undefined variable | +> | **tarekgh** | Does `SetDbStatementForText` exist in latest SqlClient instrumentation? | +> | **tarekgh** | Does it need to reference `OpenTelemetry.Instrumentation.Runtime` package? | +> | **tarekgh** | Traces don't configure endpoint but metrics do? | +> | **tarekgh** | Missing `using` directives | +> | **tarekgh** | Is `GetQueueDepth` just demonstrating the idea? | +> | **tarekgh** | Is HttpClient instrumentation accurate for clients not from `IHttpClientFactory`? | +> | **noahfalk** | Fix package list (suggestion provided) | +> | **noahfalk** | Fix description (suggestion provided) | +> | **noahfalk** | SQL instrumentation should be clearly marked **optional** | +> | **noahfalk** | Runtime metrics should be marked **optional** | +> | **noahfalk** | Custom tracing spans should also be optional (but more useful) | +> | **noahfalk** | Use `IMeterFactory` instead of static `Meter` per official guidance | +> | **noahfalk** | What about logs/metrics verification? | +> | **noahfalk** | `IMeterFactory` again for custom metrics section | +> | **noahfalk** | Eval prompts should be simpler/more generalized | + +### danmoseley (2026-03-07) +> Now we have new plugin factoring, moved the skill from `plugins/dotnet/` to `plugins/dotnet-diag/`. OpenTelemetry is an observability/diagnostics concern and fits better alongside the other diagnostic skills. + +### ManishJayaswal (2026-03-10) +> @BrennanConroy @adityamandaleeka - please review. @mrsharm - you may want to update the branch. This skill should also be under ASP plugin. + +### danmoseley (2026-03-24) +> @mrsharm this seems ASP.NET specific? If so should probably go in the dotnet-aspnet plugin not dotnet-diag? + +### danmoseley (2026-03-28) +> @mrsharm to get this restarted again for you I got to copilot review clean, and also got copilot to drive up the score using the instructions below the eval table. That looks pretty good too. I did _not_ review the changes it made. Hope its OK for you to do that... + +## Review Comments (PR #268) + +### copilot-pull-request-reviewer — `tests/dotnet-diag/.../eval.yaml` (resolved) +> Scenario rubric lists "correct packages" but omits `OpenTelemetry.Instrumentation.Http`. + +### copilot-pull-request-reviewer — `plugins/dotnet-diag/skills/.../SKILL.md` (resolved) +> `SKILL.md` frontmatter is wrapped in a fenced code block (```skill). Remove the code fence. + +### copilot-pull-request-reviewer — `plugins/dotnet-diag/skills/.../SKILL.md` (resolved) +> Step 1 says to "install exactly these" packages, but Step 2 uses `AddSqlClientInstrumentation()` and `AddRuntimeInstrumentation()` unconditionally. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L183 (resolved) +> The `ProcessOrderAsync` example returns `order`, but no `order` variable is defined in the snippet. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L237 (resolved) +> `TagList` is used but not in scope with the shown `using System.Diagnostics.Metrics;`. + +### copilot-pull-request-reviewer — `plugins/dotnet-diag/skills/.../SKILL.md` (resolved) +> The context propagation snippet uses `Propagators`, `PropagationContext`, and `Baggage` without showing the required namespaces. + +### copilot-pull-request-reviewer — Multiple files (resolved) +> Step 1 says "Install exactly these", but later code samples use additional APIs requiring more NuGet packages. Also the PR description lists wrong plugin paths. Various CODEOWNERS issues. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` (resolved) +> Tracing config explicitly sets the OTLP endpoint, but metrics and logging call exporter with defaults — can lead to different endpoints. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L190 (resolved) +> The OrderService sample returns `order`, but no `order` variable is created/assigned. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/plugin.json` L6 (resolved) +> This PR introduces a new plugin (plugins/dotnet-aspnet), but it is not registered in the plugin marketplaces. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L188 (resolved) +> The `activity?.RecordException(ex)` call relies on the OpenTelemetry Trace extension method. As written, snippet only imports `System.Diagnostics`. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L135 (resolved) +> Tracing/metrics configure explicit OTLP endpoint, but logging uses `logging.AddOtlpExporter()` without setting endpoint. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L215 (resolved) +> The custom metrics example uses `IMeterFactory`, but the snippet doesn't include the namespace/import for it. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L12 (resolved) +> The skill says it's used for setting up exporters "(OTLP, Jaeger, Prometheus)", but the workflow only shows OTLP/Console configuration. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L27 (resolved) +> The Inputs table uses a leading double `||` on each row. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L299 (resolved) +> The Common Pitfalls table also uses a leading double `||`. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L39/L43 (resolved) +> Package list in Step 1 doesn't include the package that provides `builder.Logging.AddOpenTelemetry(...)` or `OtlpExportProtocol`. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L27 (resolved) +> Inputs table suggests "Jaeger, Prometheus (all accept OTLP)". Prometheus does not accept OTLP directly. + +### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` (resolved) +> Pitfall "Missing HTTP client spans" states `AddHttpClientInstrumentation()` only works with `HttpClient` from DI. This is inaccurate — it works for `new HttpClient()` too. + +### copilot-pull-request-reviewer — `tests/dotnet-aspnet/.../eval.yaml` L37 (resolved) +> The second scenario is a negative activation test but doesn't set `expect_activation: false`. + +--- + +## Old PR #91 (closed) — Add configuring-opentelemetry-dotnet skill + +**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-02-23 | **Closed:** 2026-03-06 + +### Discussion Comments + +#### tarekgh (2026-02-24) +> CC @JamesNK @rajkumar-rangaraj + +#### mrsharm (2026-02-25) — Eval Results +> | Skill | Test | Baseline | With Skill | Δ | Verdict | +> |-------|------|----------|------------|---|---------| +> | configuring-opentelemetry-dotnet | Add OpenTelemetry tracing and metrics to an ASP.NET Core API | 5.0/5 | 4.3/5 | -0.7 | ✅ | +> | configuring-opentelemetry-dotnet | OpenTelemetry skill should not activate for simple logging question | 5.0/5 | 5.0/5 | 0.0 | ✅ | +> +> **Overall improvement: +15.3%** (3 runs, not statistically significant) + +#### noahfalk (2026-02-25) +> Just curious, have you compared any results between writing all this guidance inline vs. having the skill reference pre-existing docs? The skill is certainly more compact, but its not clear to me how the tradeoff between compactness vs. depth/breadth affects the results. (I'm also not sure we have enough test cases to draw much conclusion from the automated results alone) + +#### steveisok (2026-02-25) +> External docs inform; inline skills steer. Pointing to docs puts more faith in the LLM figuring it out. Doing it inline allows you a much more direct way to influence. + +#### rajkumar-rangaraj (2026-02-25) +> I recently updated the **OpenTelemetry .NET documentation** to improve how OpenTelemetry configuration is explained and structured. We intentionally **separated the builder-based configuration docs** so they can be consumed by **agents and automation workflows**. It might be worth borrowing a similar approach for skills documentation by: +> - Clearly separating **what needs to be configured** from **how it is wired together** +> - Identifying which parts are **agent-friendly** vs purely human-oriented examples + +#### rajkumar-rangaraj (2026-02-25) +> Adding OpenTelemetry .NET maintainers to get their perspective too. @alanwest @Kielek @martincostello + +#### mrsharm (2026-03-06) +> Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. + +### Review Comments (PR #91) + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L166 +> In the spans example, `SetStatus(...)` and `RecordException(...)` are OpenTelemetry extension methods on `Activity` (namespace `OpenTelemetry.Trace`). The snippet only includes `using System.Diagnostics;`, so it won't compile. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L6 +> SKILL.md is wrapped in a fenced ```skill code block, but the skill validator only parses YAML frontmatter when the file starts with `---`. + +#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L161 +> In the OrderService example, `return order;` references an `order` variable that isn't defined in the snippet. + +#### tarekgh — `src/dotnet/skills/.../SKILL.md` L85 +> does `SetDbStatementForText` exist in the latest SqlClient instrumentation? + +#### tarekgh — `src/dotnet/skills/.../SKILL.md` L101 +> does it need to reference OpenTelemetry.Instrumentation.Runtime package? + +#### tarekgh — `src/dotnet/skills/.../SKILL.md` L104 +> these doesn't need to configure the endpoint as you did with metrics? + +#### tarekgh — `src/dotnet/skills/.../SKILL.md` L225 +> do we need `using` directives here? + +#### tarekgh — `src/dotnet/skills/.../SKILL.md` L203 +> I assume `GetQueueDepth` just demonstrating the idea and doesn't have to exist in the code. + +#### tarekgh — `src/dotnet/skills/.../SKILL.md` L260 +> Is this accurate? wouldn't the instrumentation still work with the spans created from clients not created from IHttpClientFactory? + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L27 +> (suggestion) Observability backend | No | Where to export: Jaeger, Prometheus, OTLP collector, Aspire Dashboard + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L17 +> (suggestion) The user's application doesn't use ASP.NET + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L88 +> Many apps probably have no need to do this [SQL instrumentation]. You might want to segregate this into another step that is clearly optional. + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L121 +> I'd recommend marking this [runtime metrics] optional. I suspect many customers don't need this. + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L175 +> This [custom tracing spans] should also be optional, though probably more apps would benefit from it. + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L183 +> The official guidance is for apps using DI to use IMeterFactory rather than creating a static Meter. + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L246 +> And logs/metrics? + +#### noahfalk — `src/dotnet/skills/.../SKILL.md` L263 +> IMeterFactory again :) + +#### noahfalk — `src/dotnet/tests/.../eval.yaml` L4 +> I'd recommend either adding more eval prompts, or if we will only have a limited number focus on prompts that are simpler and more generalized. I'd guess prompts like these are going to be more common: "Please enable telemetry for my app", "Help set up OpenTelemetry", "I want to record some metrics, how do I do that?" + +--- + +# PR #269 — Add refactoring-to-async skill + +**State:** Open | **Author:** mrsharm | **Created:** 2026-03-06 | **Comments:** 36 +**Replaces:** #79 + +## Discussion Comments + +### mrsharm (2026-03-06) +> **Migration Note** +> +> This PR replaces #79. All prior review feedback from #79 still applies. + +### mrsharm (2026-03-06) — Feedback carried over from #79 +> **@Copilot** — `eval.yaml` L12: The scenario requires identifying sync-over-async patterns like `.Result`/`.Wait()`, but the `output_not_matches` assertion fails the run if the agent output contains `.Result` or `.Wait()` anywhere (including when calling them out as anti-patterns). +> +> **@Copilot** — `SKILL.md` L35: The `grep` pattern uses `\b` for a word-boundary, but `grep`'s default regex syntax does not treat `\b` as a word boundary. +> +> **@Copilot** — `SKILL.md` L155: Same issue as Step 1: `grep` without `-P` won't treat `\b` as a word boundary. +> +> **@danmoseley** — `SKILL.md` L8: move any part of use/not use into description similar to runtime formatting, if it can avoid unnecessary reloading. +> +> **@danmoseley** — `SKILL.md` L167: this [ConfigureAwait(false)] is missing from all the examples. maybe indicate those are app code? It should be used consistently or not at all. +> +> **@danmoseley** — `SKILL.md` L185: this may be a bit high level/conceptual for this skill? +> +> **@danmoseley** — `SKILL.md` L53: (suggestion) add `(DbDataReader)` and `(DbCommand)` qualifiers to the sync→async mapping table. +> +> **@danmoseley** — `eval.yaml` L25: is this really something the user would write? seems like a gimme as written. more likely the user would say "make DoIt() async so it's faster" and the AI would have to figure out that it's CPU bound? +> +> **@danmoseley** — `eval.yaml` L33: do you expect `ConfigureAwait(false)` in this UserService case? either way, should be a test for the opposite case (eg winforms) and verify in each case it's present or not as expected. +> +> **@danmoseley** — `SKILL.md` L148: (suggestion to expand the error→fix table with more compiler error codes) + +### ViktorHofer (2026-03-06) +> @mrsharm check the one scenario in which the skill doesn't get activated. + +### mrsharm (2026-03-06) +> @ViktorHofer: Intentional - is a negative test and is the correct outcome. The eval suggests optimizing matrix multiplication (CPU-bound), not about converting I/O to async. The skill's description targets I/O-bound async refactoring, so the agent should recognize this isn't an async problem and not load the skill. + +### ViktorHofer (2026-03-08) +> @mrsharm such tests must be annotated with `expect_activation: false`. Otherwise we have no way to distinguish between intentionally not activated and unintentionally not activated. + +### mrsharm (2026-03-08) +> @ViktorHofer - thanks! Been implemented for this and the other PRs. + +### ManishJayaswal (2026-03-10) +> @mrsharm - the repo has undergone some restructuring. This skill is already under the correct plugin - dotnet. Please update the PR and submit again. +> @jasonmalinowski @jaredpar - please review. + +### mrsharm (2026-03-06) +> @danmoseley - could you please take another look? I believe I have addressed your feedback. + +## Review Comments (PR #269) + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/UserService.cs` (resolved) +> The comment says "Sync-over-async: blocking call", but the code is using the synchronous `HttpClient.Send(...)` API. Either adjust the comment or change the sample to a true sync-over-async pattern. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` L3 (resolved) +> PR description 'Files' list appears incomplete relative to the actual changes. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) +> `output_not_matches` is currently broad enough that it will also fail when the model *mentions* `.Result` / `.Wait()` as anti-patterns in prose. Consider tightening the regex. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) +> The CPU-bound scenario's `output_not_matches` pattern includes `async Task`, which can easily appear in an explanation. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) +> The Stream `ReadAsync`/`WriteAsync` examples don't match the actual Stream APIs. Recommend updating to use the correct overloads. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/UserService.cs` (resolved) +> `_httpClient` is injected and stored but never used in `UserRepository`. + +### mrsharm — `.github/CODEOWNERS` (resolved) +> Update this to the right owner. @danmoseley / @timheuer - who should be the right owner for this skill? + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) +> `output_not_matches` regex will fail if the response mentions `.GetAwaiter().GetResult()` in explanatory text. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) +> The `ApiClient.cs` fixture uses `StreamReader` but doesn't include `using System.IO;`. + +### copilot-pull-request-reviewer — `.github/CODEOWNERS` (resolved, 2 comments) +> These CODEOWNERS entries for the new dotnet skill/tests are placed under the `# dotnet-upgrade` section. Move them under the `# dotnet` section. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) +> PR description text appears to contain a duplicated sentence at the end. + +### jaredpar — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved, 2 comments) +> **jaredpar:** The budget for skills descriptions is limited. This seems longer than I would expect for a skill. +> +> **danmoseley:** We only validate the hard limit of 1024 chars. Beyond that, it's hard to score whether there are too many (waste) or appropriate (prevents loading the skill inappropriately). + +### jaredpar / danmoseley — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved, 2 comments) +> **jaredpar:** I worry that the prompt here is too strongly connected to the prompts in the skill. Basically the skill says "for ASP.NET do X" and the eval says "this is ASP.NET". It feels like the test is being fitted for the skill vs. the user experience. My intuition is that customers would more naturally write: "This controller has performance issues under load..." +> +> **danmoseley:** yes, I agree this is arguably "overfitting". Current overfitting judge doesn't catch it. @JanKrivanek I wonder whether this suggests we should tune prompt given to the overfitting judge. + +### danmoseley — `tests/dotnet/refactoring-to-async/UserService.cs` L23 (resolved) +> should the skill recommend using `IAsyncDisposable` (ie `await using`)? and eval should check that all these `using var` get converted. + +### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) +> (suggestion) Missing `ConfigureAwait(false)` in libraries | Can deadlock in Winforms/WPF/ASP.NET sync contexts | Add `.ConfigureAwait(false)` to every `await` in library code; omit in ASP.NET Core app code (no SynchronizationContext) + +### danmoseley — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) +> AI says, "Task.WhenAll fires all HTTP requests concurrently, which could overwhelm a downstream service. Sequential await should be the preferred default; WhenAll should only be suggested with a concurrency caveat." + +### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` L212 (resolved) +> There should be a bit more mention of `ValueTask` in the guidance higher up and its limitations over `Task`. Eg., cannot be awaited multiple times, cannot be stored. + +### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` L185 (resolved) +> Is `IAsyncEnumerable` / `await foreach` in scope? Probably should be. If not, it should say it's out of scope. Similar for `IAsyncDisposable`. + +### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) +> Suggest to provide a list of example method names, rather than write the grep for it. It's great at writing grep, and giving it the command suggests that this is a complete list. Plus if this hits something inappropriate eg a random property named "Result" it can adjust itself. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` L177 (resolved) +> The controller scenario rubric requires removing sync-over-async anti-patterns, but the assertions only check for an async signature + CancellationToken. Add an `output_not_matches` assertion. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) +> This CancellationToken example also leaves the `HttpResponseMessage` undisposed. Should use `using var response = await ...`. + +### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) +> In the async "After" example, `HttpResponseMessage` is not disposed. Should demonstrate disposing it. + +### copilot-pull-request-reviewer — `.github/CODEOWNERS` (resolved) +> CODEOWNERS entries placed under the `# dotnet-upgrade` section — should be under `# dotnet`. + +### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) +> Scenario 1's rubric calls out removing `.GetAwaiter().GetResult()` sync-over-async, but the `output_not_matches` assertion only checks for `.Result` and `.Wait()`. + +--- + +## Old PR #79 (closed) — Add refactoring-to-async skill + +**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-02-23 | **Closed:** 2026-03-06 + +### Discussion Comments + +#### ViktorHofer (2026-02-23) +> Please share the results as a comment similar to #75. Also make sure that you test at least 3 runs (`--runs 3` for local validation). + +#### mrsharm (2026-02-25) — Eval Results (posted twice) +> | Skill | Test | Baseline | With Skill | Δ | Verdict | +> |-------|------|----------|------------|---|---------| +> | refactoring-to-async | Refactor synchronous service to async | 3.3/5 | 5.0/5 | +1.7 | ✅ | +> | refactoring-to-async | Async refactoring should not apply to CPU-bound code | 1.0/5 | 1.0/5 | 0.0 | ✅ | +> +> **Overall improvement: +18.8%** (3 runs, not statistically significant) + +#### mrsharm (2026-02-25) +> > Please share the results as a comment similar to #75. Also make sure that you test at least 3 runs. +> +> Done. + +#### danmoseley (2026-03-02) +> add codeowners entry, move files around to match new pattern in main. + +#### mrsharm (2026-03-06) +> Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. + +### Review Comments (PR #79) + +#### copilot-pull-request-reviewer — `src/dotnet/tests/refactoring-to-async/eval.yaml` L12 +> The scenario requires identifying sync-over-async patterns like `.Result`/`.Wait()`, but the `output_not_matches` assertion fails the run if the agent output contains `.Result` or `.Wait()` anywhere (including when calling them out as anti-patterns). + +#### copilot-pull-request-reviewer — `src/dotnet/skills/refactoring-to-async/SKILL.md` L35 +> The `grep` pattern uses `\b` for a word-boundary, but `grep`'s default regex syntax does not treat `\b` as a word boundary (it's typically interpreted as a backspace). + +#### copilot-pull-request-reviewer — `src/dotnet/skills/refactoring-to-async/SKILL.md` L155 +> Same issue as Step 1: `grep` without `-P` won't treat `\b` as a word boundary. + +#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L8 +> move any part of use/not use into description similar to runtime formatting, if it can avoid unnecessary reloading. Possibly not all can go there. For example: user wants to parallelize work is something that can be known before loading the skill, and avoid load. + +#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L167 +> this [ConfigureAwait(false)] is missing from all the examples. maybe indicate those are app code? It might be worth mentioning as instructions, not just in anti-patterns — if code is library add `.ConfigureAwait(false)` to every await. It should be used consistently or not at all. + +#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L185 +> this may be a bit high level/conceptual for this skill? + +#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L53 +> (suggestion) Add `(DbDataReader)` and `(DbCommand)` qualifiers to the sync→async mapping table entries. + +#### danmoseley — `src/dotnet/tests/refactoring-to-async/eval.yaml` L25 +> is this really something the user would write? seems like a gimme as written. more likely the user would say "make DoIt() async so it's faster" and the AI would have to figure out that it's CPU bound? + +#### danmoseley — `src/dotnet/tests/refactoring-to-async/eval.yaml` L33 +> do you expect `ConfigureAwait(false)` in this UserService case? either way, should be a test for the opposite case (eg winforms) and verify in each case it's present or not as expected. + +#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L148 +> (suggestion to expand the error→fix table with more compiler error codes like CS4032, CS0029, CS0127, CS1983, CS1998, CS0535, CS7036, CS1503) From 6c46f82bb0154064abf80b25b61a9eaca79012bf Mon Sep 17 00:00:00 2001 From: Viktor Hofer <7412651+ViktorHofer@users.noreply.github.com> Date: Tue, 31 Mar 2026 10:27:06 +0200 Subject: [PATCH 15/16] Delete pr-comments.md --- pr-comments.md | 792 ------------------------------------------------- 1 file changed, 792 deletions(-) delete mode 100644 pr-comments.md diff --git a/pr-comments.md b/pr-comments.md deleted file mode 100644 index 0649b5ec05..0000000000 --- a/pr-comments.md +++ /dev/null @@ -1,792 +0,0 @@ -# PR Comments Summary - -## Table of Contents -- [PR #264 — Add minimal-api-file-upload skill](#pr-264--add-minimal-api-file-upload-skill) - - [Old PR #155 (closed)](#old-pr-155-closed--add-minimal-api-file-upload-skill) -- [PR #200 — Add migrating-newtonsoft-to-system-text-json skill](#pr-200--add-migrating-newtonsoft-to-system-text-json-skill) - - [Old PR #89 (closed)](#old-pr-89-closed--add-migrating-newtonsoft-to-system-text-json-skill) -- [PR #268 — Add configuring-opentelemetry-dotnet skill](#pr-268--add-configuring-opentelemetry-dotnet-skill) - - [Old PR #91 (closed)](#old-pr-91-closed--add-configuring-opentelemetry-dotnet-skill) -- [PR #269 — Add refactoring-to-async skill](#pr-269--add-refactoring-to-async-skill) - - [Old PR #79 (closed)](#old-pr-79-closed--add-refactoring-to-async-skill) - ---- - -# PR #264 — Add minimal-api-file-upload skill - -**State:** Open | **Author:** mrsharm | **Created:** 2026-03-06 | **Comments:** 23 -**Replaces:** #155 - -## Discussion Comments - -### mrsharm (2026-03-06) -> **Migration Note** -> -> This PR replaces #155 which was opened from `mrsharm/skills-old`. The skill and eval files have been migrated to the new `plugins/` directory structure: -> -> - `src/dotnet/skills/minimal-api-file-upload/` → `plugins/dotnet/skills/minimal-api-file-upload/` -> - `src/dotnet/tests/minimal-api-file-upload/` → `tests/dotnet/minimal-api-file-upload/` -> -> All prior review feedback from #155 still applies — please see that PR for the full discussion history. - -### mrsharm (2026-03-06) -> **Feedback carried over from #155** -> -> **Copilot** on SKILL.md (line 43): Step 1 is internally contradictory: it's titled as if IFormFile requires [FromForm] (not automatic), but later in this section it states IFormFile *is* bound automatically in .NET 8. Please rewrite this section to present one clear rule. -> -> **Copilot** on SKILL.md (line 138): The "safe filename" still uses `Path.GetExtension(file.FileName)`, which derives the extension from user-controlled input. Prefer choosing the extension based on the validated signature. -> -> **Copilot** on SKILL.md (line 163): `context.Request.GetMultipartBoundary()` isn't a built-in ASP.NET Core API. Please include the helper implementation or switch to the standard boundary parsing approach. -> -> **Copilot** on eval.yaml (line 12): Rubric criterion allows validating only `ContentType`, but the PR description explicitly calls out ContentType as client-spoofable. Tighten this rubric to require signature/magic-byte checking. -> -> **Copilot** on SKILL.md (line 64): These examples set the global Kestrel request body limit to 100MB, which can be confusing given the scenario is about enforcing a 10MB maximum. -> -> **Copilot** on SKILL.md (line 125): When reading magic bytes, the code ignores the return value from ReadAsync. Capture the bytes-read and fail fast if fewer than the required bytes are available. -> -> **Copilot** on SKILL.md (line 118): The allowed MIME types include `image/gif`, but the scenario/rubric for this skill is JPEG + PNG only. -> -> **Copilot** on SKILL.md (line 153): The statement that "IFormFile buffers the entire file in memory by default" is inaccurate for ASP.NET Core. Multipart parsing uses buffering with a memory threshold and typically spills to a temp file. -> -> **Copilot** on SKILL.md (line 172): `section.GetContentDispositionHeader()` / `contentDisposition.IsFileDisposition()` also aren't built-in APIs. -> -> **timheuer**: This feels _too_ verbose a name -- "minimal-api-file-upload"? -> -> **timheuer**: Any validation that "8" is going to influence too much? -> -> **timheuer**: Strike "8" and put more information in the 'when to use'? -> -> **timheuer**: - File upload endpoints in ASP.NET minimal APIs (.NET 8+) -> -> **halter73** on SKILL.md (line 108): @GrabYourPitchforks @blowdart This should be okay for unauthenticated endpoints and endpoints using JWT bearer authentication, but I worry that this might cause people to disable antiforgery for endpoints authenticated with cookies. -> -> **ViktorHofer**: As discussed offline, this PR will need to be re-submitted from a connected fork. Also please update this PR based on the new repo folder structure (plugins instead of src). - -### ViktorHofer (2026-03-08) -> Same feedback as in the other PRs regarding the skill not getting activated. If intentional, add `expect_activation: false` - -### ManishJayaswal (2026-03-10) -> @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. This skill should be under ASP plugin. Please update the PR and submit again. -> @adityamandaleeka @BrennanConroy - please review - -### danmoseley (2026-03-24) -> If #207 creates a dotnet-aspnet plugin, this skill should presumably move there. - -### danmoseley (2026-03-25) -> [running evaluate just to check the new evaluate improvements...] - -### danmoseley (2026-03-25) — Eval Failure Diagnosis -> **Verdict: FAIL** — overall +2.5% improvement (needs ≥10%) -> -> | Scenario | Quality | Score | Status | -> |----------|---------|-------|--------| -> | Implement secure upload | 4.3→5.0 | +15.1% | ✅ | -> | Fix 400 Bad Request | **5.0→4.3** | **-20.7%** | ❌ timeout | -> | Upload multiple + metadata | 3.7→5.0 | +40.5% | ✅ | -> -> **Scenario 2 is sinking the entire eval.** Scenarios 1 & 3 are solid. -> -> **Root Causes (priority order):** -> 1. **Timeout too low (60s)** — fix first. The skilled runs hit the 60s ceiling. **Fix:** `timeout: 60` → `timeout: 180` -> 2. **Baseline already perfect (5.0/5)** — The anti-forgery gotcha is well-documented. No headroom. **Fix options:** Add `reject_tools: ["bash", "edit"]`, or tighten the prompt, or remove the scenario. -> 3. **Rubric item #3 flagged as "technique" by overfitting analysis** — Soften to something like: "Explained why file upload endpoints commonly need antiforgery handling adjusted" -> -> **Recommended Fix Order:** -> 1. Increase timeout for scenario 2: `60` → `180` -> 2. Add `reject_tools: ["bash", "edit"]` to scenario 2 -> 3. *(Optional)* Soften rubric item 3 - -## Review Comments (PR #264) - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` -> In the streaming example, `contentDisposition.FileName.Value` is used directly to compute the extension. This value can include paths and other unexpected content; using it directly reintroduces path traversal / spoofing risks. Prefer extracting a safe base name first. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` -> The text claims `IFormFile` "buffers the entire file in memory by default". ASP.NET Core typically buffers form bodies with a memory threshold and spills to disk, so the guidance here is misleading. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` -> The Step 1 heading/example says it's a "COMMON MISTAKE" to expect `IFormFile` to bind automatically, but immediately below it says `IFormFile IS bound automatically from form data in .NET 8`. This is internally inconsistent. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` -> The allowlist includes `image/gif`, but the eval scenario and the surrounding guidance emphasize JPEG/PNG-only uploads. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` L151 -> `safeFileName` uses `Path.GetExtension(file.FileName)`, which is attacker-controlled. Prefer deriving the extension from the validated file signature/content type. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/minimal-api-file-upload/SKILL.md` -> This snippet calls `context.Request.GetMultipartBoundary()`, but that isn't a standard ASP.NET Core API. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/plugin.json` L5 -> Plugin manifests in this repo conventionally use the array form for `skills` (e.g., `"skills": ["./skills/"]`). - -### copilot-pull-request-reviewer — `plugins/aspnetcore/plugin.json` L6 -> The PR description lists paths under `plugins/dotnet/...` and `tests/dotnet/...`, but this PR adds an `aspnetcore` plugin. Please update the PR description. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` -> The markdown contains mojibake/encoding artifacts (e.g., "ΓåÆ", "ΓÇö"). Replace these with proper Unicode characters. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` -> Step 1 is internally contradictory: the heading says IFormFile binding is "not automatic", but the snippet then states it's bound automatically in .NET 8. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L144 -> The content-type allowlist includes "image/gif" but the subsequent magic-bytes validation only recognizes JPEG/PNG, so GIF uploads would pass the ContentType check and then fail later. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` -> The example reads 8 bytes from the stream but doesn't verify how many bytes were actually read. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` -> This states that IFormFile buffers the entire file in memory by default, which is misleading in ASP.NET Core. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` -> The sample calls `context.Request.GetMultipartBoundary()`, but there is no such built-in API. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L198 -> In the streaming example, the saved filename's extension is taken from the user-supplied filename. Prefer deriving the extension from validated content. - -### copilot-pull-request-reviewer — `tests/aspnetcore/minimal-api-file-upload/eval.yaml` L3 -> The PR description's file list references `plugins/dotnet/...` and `tests/dotnet/...`, but this PR adds the skill under `plugins/aspnetcore/...` and `tests/aspnetcore/...`. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L25 -> The markdown table under **Inputs** uses `||` at the start of each row/header, which doesn't render as a proper table. - -### copilot-pull-request-reviewer — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L138 -> In the magic-bytes example, the stream is rewound (`stream.Position = 0;`) but the subsequent save uses `file.CopyToAsync(...)`, which opens a new stream and ignores the rewound one. - -### mikekistler — `plugins/aspnetcore/skills/minimal-api-file-upload/SKILL.md` L147 (2026-03-28) -> I think another valid approach is to validate the filename provided before using it. Often the filename provided by the user has some significance and must be retained. - ---- - -## Old PR #155 (closed) — Add minimal-api-file-upload skill - -**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-03-02 | **Closed:** 2026-03-06 - -### Discussion Comments - -#### mrsharm (2026-03-02) — Eval Results -> ### 3-Run Validation: +38.9% PASS -> -> | Metric | Value | -> |--------|-------| -> | Overall Improvement | **+38.9%** | -> | Confidence Interval | [+9.1%, +62.5%] significant | -> | Effect Size (g) | +100.0% | -> | Baseline Quality | 3.0/5 | -> | Skill Quality | 5.0/5 | -> -> Baseline consistently misses: Only configures one of the two required size limits, relies on ContentType alone for validation, uses user-provided filenames without sanitization. - -#### ViktorHofer (2026-03-04) -> As discussed offline in the "dotnet/skills content" chat, this PR will need to be re-submitted from a connected fork. Also please update this PR based on the new repo folder structure (plugins instead of src). - -#### mrsharm (2026-03-06) -> Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. - -### Review Comments (PR #155) - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L43 -> Step 1 is internally contradictory: it's titled as if IFormFile requires [FromForm] (not automatic), but later in this section it states IFormFile *is* bound automatically in .NET 8. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L138 -> The "safe filename" still uses `Path.GetExtension(file.FileName)`, which derives the extension from user-controlled input. Prefer choosing the extension based on the validated signature. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L163 -> This example uses `context.Request.GetMultipartBoundary()`, but that helper isn't defined anywhere in this repo and isn't a built-in ASP.NET Core API. - -#### copilot-pull-request-reviewer — `src/dotnet/tests/minimal-api-file-upload/eval.yaml` L12 -> Rubric criterion allows validating only `ContentType`, but the PR description explicitly calls out ContentType as client-spoofable. Tighten this rubric to require signature/magic-byte checking. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L64 -> These examples set the global Kestrel request body limit to 100MB, which can be confusing given the scenario is about enforcing a 10MB maximum. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L125 -> When reading magic bytes, the code ignores the return value from ReadAsync. Capture the bytes-read and fail fast if fewer than the required bytes are available. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L118 -> The allowed MIME types include `image/gif`, but the scenario/rubric for this skill is JPEG + PNG only. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L153 -> The statement that "IFormFile buffers the entire file in memory by default" is inaccurate for ASP.NET Core. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L172 -> `section.GetContentDispositionHeader()` / `contentDisposition.IsFileDisposition()` aren't built-in APIs. - -#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` -> This feels _too_ verbose a name -- "minimal-api-file-upload"? - -#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` -> Any validation that "8" is going to influence too much? - -#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` -> Strike "8" and put more information in the 'when to use'? (note below) - -#### timheuer — `src/dotnet/skills/implementing-form-file-uploads-minimal-apis/SKILL.md` -> - File upload endpoints in ASP.NET minimal APIs (.NET 8+) - -#### halter73 — `src/dotnet/skills/minimal-api-file-upload/SKILL.md` L108 -> @GrabYourPitchforks @blowdart This should be okay for unauthenticated endpoints and endpoints using JWT bearer authentication, but I worry that this might cause people to disable antiforgery for endpoints authenticated with cookies. I wonder what the best way to communicate this potential security threat in the skill document. - ---- - -# PR #200 — Add migrating-newtonsoft-to-system-text-json skill - -**State:** Open | **Author:** mrsharm | **Created:** 2026-03-04 | **Comments:** 9 -**Replaces:** #89 - -## Discussion Comments - -### danmoseley (2026-03-07) -> now we have the new plugins factoring in main, this should not be in the general dotnet plugin. please move it to the dotnet-upgrade plugin which seems a better fit. - -### timheuer (2026-03-09) -> This should go in `dotnet-upgrade` plugin - -### ManishJayaswal (2026-03-10) -> @vijayrkn @tlmii @wtgodbe @noahfalk - please review - -### abpiskunov (2026-03-10) — Detailed Skill Analysis -> I ran this new skill with Claude Opus 4.6 and pointed it to anthropics best practices link just in case and here what it said. Also i am wondering about evals file in this PR. I copied prompt from it to Gemini chat and Claude chat without this skill provided in fresh chat. Both generated same code that would pass evals 100% as far as i can tell... That raises question about the skill content and how efficient are evals. -> -> **What to Keep (High Value):** -> - Behavioral differences table (Step 1) — enforces completeness -> - ConfigureJsonOptions method (Step 2) — specific combo Claude won't reliably produce in full -> - Attribute mapping table (Step 3) — quick-reference, enforces complete coverage -> - Common Pitfalls table — highest-value section -> - Validation checklist — enforces completeness -> - grep commands for finding Newtonsoft usages -> -> **What to Cut or Compress:** -> - Full converter before/after code (Step 4) — Replace with "Key differences in converter API" bullet list only -> - Full JToken/JObject replacement examples (Step 5) — Keep the mapping table, cut the JsonNode mutable DOM code example -> - Polymorphic serialization section (Step 6) — Cut entirely. Claude knows [JsonDerivedType] thoroughly -> - Package reference / using statement changes (Step 7) — Cut the full XML and using-statement blocks -> - The "When Not to Use" section — "already using System.Text.Json" and "user wants to keep Newtonsoft" are obvious -> -> **What to Add:** -> - Conditional workflow at the top (ASP.NET Core app vs class library vs console/worker app) -> - Copy-and-track checklist — Restructure validation section into pattern with checkboxes -> - Explicit feedback loop — After each major step, add: "Run dotnet build to verify no compilation errors" -> - More trigger keywords in the description — "Json.NET", "removing Newtonsoft dependency", "serialization breaking changes", "AOT compatibility" -> -> **Net Effect:** ~250 lines → ~140-160 lines, focused on completeness-enforcement content. ~40% token reduction while increasing practical value. - -### danmoseley (2026-03-28) -> evaluation looks good. @mrsharm you just need review signoffs — particularly @eiriktsarpalis who made the requested change review. I believe I addressed that. - -## Review Comments (PR #200) - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/migrating-newtonsoft-to-system-text-json/SKILL.md` (resolved) -> This repeats the earlier claim that `JsonExtensionData` "must" use `Dictionary` and that `Dictionary` is invalid. System.Text.Json supports extension data on `IDictionary` as well. - -### copilot-pull-request-reviewer — `tests/dotnet-upgrade/.../eval.yaml` L5 (resolved) -> This eval is being added under `src/dotnet/tests/...`, but the repo layout expects scenarios under `tests///eval.yaml`. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) -> `DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull` is not Newtonsoft.Json's default (Json.NET includes nulls unless configured). - -### copilot-pull-request-reviewer — `tests/dotnet/.../eval.yaml` (resolved) -> The rubric's casing note is inconsistent with the skill doc. - -### copilot-pull-request-reviewer — `tests/dotnet/.../eval.yaml` (resolved) -> `expect_tools: ["bash"]` forces the model to make at least one bash tool call. This prompt is primarily a code-migration explanation and doesn't require shell usage. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) -> This SKILL.md is wrapped in a fenced code block (```skill ... ```), which means the YAML frontmatter and the rest of the content won't be parsed/rendered like other skills. - -### copilot-pull-request-reviewer — `plugins/dotnet-upgrade/skills/.../SKILL.md` L223 (resolved) -> The polymorphism section implies System.Text.Json will automatically emit a discriminator for `[JsonDerivedType]` types. STJ polymorphism requires explicit opt-in/configuration. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) -> The doc contradicts itself on default property naming. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/.../SKILL.md` (resolved) -> `[JsonExtensionData]` doesn't have to be `Dictionary` only. - -### copilot-pull-request-reviewer — `plugins/dotnet-upgrade/skills/.../SKILL.md` L215 (resolved) -> This example configures `JsonSerializerSettings` with `TypeNameHandling = TypeNameHandling.Auto`, which is a known unsafe pattern when used with untrusted JSON. - -### GrabYourPitchforks — `plugins/dotnet/skills/.../SKILL.md` (resolved, 2 comments) -> Setting `Preserve` significantly increases the attack surface of JSON deserialization. It shouldn't be promoted as a "try it and see if it solves your problem" mechanism. -> -> To defend against this attack, the application should consider what happens if the adversary controls the _edges_ in the object graph, not just the _values_ of the nodes. - -### GrabYourPitchforks / eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L88 (resolved, 3 comments) -> **GrabYourPitchforks:** Similarly, this is a potentially dangerous setting [ReadCommentHandling] and should not be encouraged unless the application has thought through the consequences. We chose not to allow this by default in S.T.J because it could lead to desynced deserialization attacks. -> -> **GrabYourPitchforks:** To defend against this attack, the application should: 1) ensure all deserializers operate in strict RFC compliance mode; or 2) ensure all components use the same deserializer library; or 3) ensure the frontend reserializes the payload before sending to backend. -> -> **eiriktsarpalis:** I would add that case insensitive property handling similarly makes systems susceptible to interoperability attacks. It should start off assuming the highest bar possible (using `JsonSerializerDefaults.Strict`) and then instruct the agent to interview the user about particular requirements that force the loosening of individual settings. - -### JamesNK — `plugins/dotnet/skills/.../SKILL.md` (resolved) -> I think just always recommend JsonNode and friends. It's closer than JsonDocument/JsonElement. - -### JamesNK / eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L41 (resolved, 3 comments) -> **JamesNK:** STJ also escapes non-ASCII characters by default. Should recommend using relaxed encoder. -> -> **eiriktsarpalis:** I think we should caveat any such recommendation with the trade-offs: escaped JSON strings are semantically equivalent to unescaped ones. -> -> **JamesNK:** I'll leave it up to you. But if it's text being embedded in an HTML page then asp.net core will escape HTML characters anyway. I've never understood why STJ does this. - -### eiriktsarpalis — `plugins/dotnet/skills/.../SKILL.md` (resolved) -> I'm pretty sure newer models can figure out regexes like that on their own, so I would just skip that part for fear of it going out of sync. - -### ViktorHofer — `PR-FEEDBACK.md` (resolved) -> Remove this file - -### eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L14 (**open**, 2026-03-30) -> Consider adding an explicit instruction to the agent that it should be adding baseline serialization tests for the application models while performing the migration. - -### eiriktsarpalis — `plugins/dotnet-upgrade/skills/.../SKILL.md` L284 (**open**, 2026-03-30) -> Consider adding links to the official STJ docs at learn. - -### eiriktsarpalis — `tests/dotnet-upgrade/.../eval.yaml` L2 (**open**, 2026-03-30) -> Shouldn't we add evals covering more scenarios? Custom converters? Naming policies? etc? Consider mining real-world snippets either from dotnet org repos or public github repos in general (e.g. via grep.app) - ---- - -## Old PR #89 (closed) — Add migrating-newtonsoft-to-system-text-json skill - -**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-02-23 | **Closed:** 2026-03-04 - -### Discussion Comments - -#### mrsharm (2026-02-25) — Eval Results (posted twice) -> | Skill | Test | Baseline | With Skill | Δ | Verdict | -> |-------|------|----------|------------|---|---------| -> | migrating-newtonsoft-to-system-text-json | Migrate model with Newtonsoft.Json attributes to System.Text.Json | 4.0/5 | 4.3/5 | +0.3 | ✅ | -> -> **Overall improvement: +10.8%** (3 runs, not statistically significant) - -#### mrsharm (2026-02-26) -> Any feedback here? @ericstj @mcastro-x? - -### Review Comments (PR #89) - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` (resolved) -> The claim that System.Text.Json "throws by default (.NET 8+)" for extra JSON properties is incorrect. System.Text.Json ignores extra JSON properties by default across all versions. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` (resolved) -> The claim about null to non-nullable value type behavior is misleading. Both Newtonsoft.Json and System.Text.Json set non-nullable value types to their default values. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L68 -> The comment "Newtonsoft default" on line 68 is misleading. PropertyNameCaseInsensitive is not a Newtonsoft.Json default behavior. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` (resolved) -> The claim that Newtonsoft.Json uses "camelCase by default" for property naming is incorrect. - -#### copilot-pull-request-reviewer — `src/dotnet/tests/.../eval.yaml` L45 -> The rubric item claims PropertyNameCaseInsensitive should be configured "to match Newtonsoft default case-insensitive behavior", but Newtonsoft.Json is case-sensitive by default. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L5 -> The skill metadata format is incorrect. Skills in this repository use standard YAML frontmatter delimited by `---`, not code fence blocks with ` ```skill`. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L44 -> The claim that Newtonsoft.Json is "case-insensitive" by default is incorrect. - -#### copilot-pull-request-reviewer — `src/dotnet/tests/.../eval.yaml` -> The prompt asks to "match Newtonsoft.Json's default behavior", but this will lead to incorrect configuration guidance. - -#### copilot-pull-request-reviewer — `src/dotnet/tests/.../eval.yaml` -> The rubric expects warnings about "default casing" as a behavioral difference, but since both libraries use the same default casing, this warning would be misleading. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L243 -> The pitfall "Forgetting PropertyNameCaseInsensitive = true" implies this is a required configuration to match Newtonsoft.Json behavior, but this is not accurate. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L67 -> The comment "Newtonsoft default" on line 67 is incorrect. Newtonsoft.Json does not use camelCase by default. - ---- - -# PR #268 — Add configuring-opentelemetry-dotnet skill - -**State:** Open | **Author:** mrsharm | **Created:** 2026-03-06 | **Comments:** 12 -**Replaces:** #91 - -## Discussion Comments - -### mrsharm (2026-03-06) -> **Migration Note** -> -> This PR replaces #91 which was opened from `mrsharm/skills-old`. All prior review feedback from #91 still applies. - -### mrsharm (2026-03-06) — Feedback carried over from #91 -> | Reviewer | Feedback | -> |----------|----------| -> | copilot | Missing `using OpenTelemetry.Trace;` for `SetStatus()`/`RecordException()` | -> | copilot | File wrapped in `` ```skill `` code block — metadata won't parse | -> | copilot | `return order;` references undefined variable | -> | **tarekgh** | Does `SetDbStatementForText` exist in latest SqlClient instrumentation? | -> | **tarekgh** | Does it need to reference `OpenTelemetry.Instrumentation.Runtime` package? | -> | **tarekgh** | Traces don't configure endpoint but metrics do? | -> | **tarekgh** | Missing `using` directives | -> | **tarekgh** | Is `GetQueueDepth` just demonstrating the idea? | -> | **tarekgh** | Is HttpClient instrumentation accurate for clients not from `IHttpClientFactory`? | -> | **noahfalk** | Fix package list (suggestion provided) | -> | **noahfalk** | Fix description (suggestion provided) | -> | **noahfalk** | SQL instrumentation should be clearly marked **optional** | -> | **noahfalk** | Runtime metrics should be marked **optional** | -> | **noahfalk** | Custom tracing spans should also be optional (but more useful) | -> | **noahfalk** | Use `IMeterFactory` instead of static `Meter` per official guidance | -> | **noahfalk** | What about logs/metrics verification? | -> | **noahfalk** | `IMeterFactory` again for custom metrics section | -> | **noahfalk** | Eval prompts should be simpler/more generalized | - -### danmoseley (2026-03-07) -> Now we have new plugin factoring, moved the skill from `plugins/dotnet/` to `plugins/dotnet-diag/`. OpenTelemetry is an observability/diagnostics concern and fits better alongside the other diagnostic skills. - -### ManishJayaswal (2026-03-10) -> @BrennanConroy @adityamandaleeka - please review. @mrsharm - you may want to update the branch. This skill should also be under ASP plugin. - -### danmoseley (2026-03-24) -> @mrsharm this seems ASP.NET specific? If so should probably go in the dotnet-aspnet plugin not dotnet-diag? - -### danmoseley (2026-03-28) -> @mrsharm to get this restarted again for you I got to copilot review clean, and also got copilot to drive up the score using the instructions below the eval table. That looks pretty good too. I did _not_ review the changes it made. Hope its OK for you to do that... - -## Review Comments (PR #268) - -### copilot-pull-request-reviewer — `tests/dotnet-diag/.../eval.yaml` (resolved) -> Scenario rubric lists "correct packages" but omits `OpenTelemetry.Instrumentation.Http`. - -### copilot-pull-request-reviewer — `plugins/dotnet-diag/skills/.../SKILL.md` (resolved) -> `SKILL.md` frontmatter is wrapped in a fenced code block (```skill). Remove the code fence. - -### copilot-pull-request-reviewer — `plugins/dotnet-diag/skills/.../SKILL.md` (resolved) -> Step 1 says to "install exactly these" packages, but Step 2 uses `AddSqlClientInstrumentation()` and `AddRuntimeInstrumentation()` unconditionally. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L183 (resolved) -> The `ProcessOrderAsync` example returns `order`, but no `order` variable is defined in the snippet. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L237 (resolved) -> `TagList` is used but not in scope with the shown `using System.Diagnostics.Metrics;`. - -### copilot-pull-request-reviewer — `plugins/dotnet-diag/skills/.../SKILL.md` (resolved) -> The context propagation snippet uses `Propagators`, `PropagationContext`, and `Baggage` without showing the required namespaces. - -### copilot-pull-request-reviewer — Multiple files (resolved) -> Step 1 says "Install exactly these", but later code samples use additional APIs requiring more NuGet packages. Also the PR description lists wrong plugin paths. Various CODEOWNERS issues. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` (resolved) -> Tracing config explicitly sets the OTLP endpoint, but metrics and logging call exporter with defaults — can lead to different endpoints. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L190 (resolved) -> The OrderService sample returns `order`, but no `order` variable is created/assigned. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/plugin.json` L6 (resolved) -> This PR introduces a new plugin (plugins/dotnet-aspnet), but it is not registered in the plugin marketplaces. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L188 (resolved) -> The `activity?.RecordException(ex)` call relies on the OpenTelemetry Trace extension method. As written, snippet only imports `System.Diagnostics`. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L135 (resolved) -> Tracing/metrics configure explicit OTLP endpoint, but logging uses `logging.AddOtlpExporter()` without setting endpoint. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L215 (resolved) -> The custom metrics example uses `IMeterFactory`, but the snippet doesn't include the namespace/import for it. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L12 (resolved) -> The skill says it's used for setting up exporters "(OTLP, Jaeger, Prometheus)", but the workflow only shows OTLP/Console configuration. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L27 (resolved) -> The Inputs table uses a leading double `||` on each row. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L299 (resolved) -> The Common Pitfalls table also uses a leading double `||`. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L39/L43 (resolved) -> Package list in Step 1 doesn't include the package that provides `builder.Logging.AddOpenTelemetry(...)` or `OtlpExportProtocol`. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` L27 (resolved) -> Inputs table suggests "Jaeger, Prometheus (all accept OTLP)". Prometheus does not accept OTLP directly. - -### copilot-pull-request-reviewer — `plugins/dotnet-aspnet/skills/.../SKILL.md` (resolved) -> Pitfall "Missing HTTP client spans" states `AddHttpClientInstrumentation()` only works with `HttpClient` from DI. This is inaccurate — it works for `new HttpClient()` too. - -### copilot-pull-request-reviewer — `tests/dotnet-aspnet/.../eval.yaml` L37 (resolved) -> The second scenario is a negative activation test but doesn't set `expect_activation: false`. - ---- - -## Old PR #91 (closed) — Add configuring-opentelemetry-dotnet skill - -**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-02-23 | **Closed:** 2026-03-06 - -### Discussion Comments - -#### tarekgh (2026-02-24) -> CC @JamesNK @rajkumar-rangaraj - -#### mrsharm (2026-02-25) — Eval Results -> | Skill | Test | Baseline | With Skill | Δ | Verdict | -> |-------|------|----------|------------|---|---------| -> | configuring-opentelemetry-dotnet | Add OpenTelemetry tracing and metrics to an ASP.NET Core API | 5.0/5 | 4.3/5 | -0.7 | ✅ | -> | configuring-opentelemetry-dotnet | OpenTelemetry skill should not activate for simple logging question | 5.0/5 | 5.0/5 | 0.0 | ✅ | -> -> **Overall improvement: +15.3%** (3 runs, not statistically significant) - -#### noahfalk (2026-02-25) -> Just curious, have you compared any results between writing all this guidance inline vs. having the skill reference pre-existing docs? The skill is certainly more compact, but its not clear to me how the tradeoff between compactness vs. depth/breadth affects the results. (I'm also not sure we have enough test cases to draw much conclusion from the automated results alone) - -#### steveisok (2026-02-25) -> External docs inform; inline skills steer. Pointing to docs puts more faith in the LLM figuring it out. Doing it inline allows you a much more direct way to influence. - -#### rajkumar-rangaraj (2026-02-25) -> I recently updated the **OpenTelemetry .NET documentation** to improve how OpenTelemetry configuration is explained and structured. We intentionally **separated the builder-based configuration docs** so they can be consumed by **agents and automation workflows**. It might be worth borrowing a similar approach for skills documentation by: -> - Clearly separating **what needs to be configured** from **how it is wired together** -> - Identifying which parts are **agent-friendly** vs purely human-oriented examples - -#### rajkumar-rangaraj (2026-02-25) -> Adding OpenTelemetry .NET maintainers to get their perspective too. @alanwest @Kielek @martincostello - -#### mrsharm (2026-03-06) -> Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. - -### Review Comments (PR #91) - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L166 -> In the spans example, `SetStatus(...)` and `RecordException(...)` are OpenTelemetry extension methods on `Activity` (namespace `OpenTelemetry.Trace`). The snippet only includes `using System.Diagnostics;`, so it won't compile. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L6 -> SKILL.md is wrapped in a fenced ```skill code block, but the skill validator only parses YAML frontmatter when the file starts with `---`. - -#### copilot-pull-request-reviewer — `src/dotnet/skills/.../SKILL.md` L161 -> In the OrderService example, `return order;` references an `order` variable that isn't defined in the snippet. - -#### tarekgh — `src/dotnet/skills/.../SKILL.md` L85 -> does `SetDbStatementForText` exist in the latest SqlClient instrumentation? - -#### tarekgh — `src/dotnet/skills/.../SKILL.md` L101 -> does it need to reference OpenTelemetry.Instrumentation.Runtime package? - -#### tarekgh — `src/dotnet/skills/.../SKILL.md` L104 -> these doesn't need to configure the endpoint as you did with metrics? - -#### tarekgh — `src/dotnet/skills/.../SKILL.md` L225 -> do we need `using` directives here? - -#### tarekgh — `src/dotnet/skills/.../SKILL.md` L203 -> I assume `GetQueueDepth` just demonstrating the idea and doesn't have to exist in the code. - -#### tarekgh — `src/dotnet/skills/.../SKILL.md` L260 -> Is this accurate? wouldn't the instrumentation still work with the spans created from clients not created from IHttpClientFactory? - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L27 -> (suggestion) Observability backend | No | Where to export: Jaeger, Prometheus, OTLP collector, Aspire Dashboard - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L17 -> (suggestion) The user's application doesn't use ASP.NET - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L88 -> Many apps probably have no need to do this [SQL instrumentation]. You might want to segregate this into another step that is clearly optional. - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L121 -> I'd recommend marking this [runtime metrics] optional. I suspect many customers don't need this. - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L175 -> This [custom tracing spans] should also be optional, though probably more apps would benefit from it. - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L183 -> The official guidance is for apps using DI to use IMeterFactory rather than creating a static Meter. - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L246 -> And logs/metrics? - -#### noahfalk — `src/dotnet/skills/.../SKILL.md` L263 -> IMeterFactory again :) - -#### noahfalk — `src/dotnet/tests/.../eval.yaml` L4 -> I'd recommend either adding more eval prompts, or if we will only have a limited number focus on prompts that are simpler and more generalized. I'd guess prompts like these are going to be more common: "Please enable telemetry for my app", "Help set up OpenTelemetry", "I want to record some metrics, how do I do that?" - ---- - -# PR #269 — Add refactoring-to-async skill - -**State:** Open | **Author:** mrsharm | **Created:** 2026-03-06 | **Comments:** 36 -**Replaces:** #79 - -## Discussion Comments - -### mrsharm (2026-03-06) -> **Migration Note** -> -> This PR replaces #79. All prior review feedback from #79 still applies. - -### mrsharm (2026-03-06) — Feedback carried over from #79 -> **@Copilot** — `eval.yaml` L12: The scenario requires identifying sync-over-async patterns like `.Result`/`.Wait()`, but the `output_not_matches` assertion fails the run if the agent output contains `.Result` or `.Wait()` anywhere (including when calling them out as anti-patterns). -> -> **@Copilot** — `SKILL.md` L35: The `grep` pattern uses `\b` for a word-boundary, but `grep`'s default regex syntax does not treat `\b` as a word boundary. -> -> **@Copilot** — `SKILL.md` L155: Same issue as Step 1: `grep` without `-P` won't treat `\b` as a word boundary. -> -> **@danmoseley** — `SKILL.md` L8: move any part of use/not use into description similar to runtime formatting, if it can avoid unnecessary reloading. -> -> **@danmoseley** — `SKILL.md` L167: this [ConfigureAwait(false)] is missing from all the examples. maybe indicate those are app code? It should be used consistently or not at all. -> -> **@danmoseley** — `SKILL.md` L185: this may be a bit high level/conceptual for this skill? -> -> **@danmoseley** — `SKILL.md` L53: (suggestion) add `(DbDataReader)` and `(DbCommand)` qualifiers to the sync→async mapping table. -> -> **@danmoseley** — `eval.yaml` L25: is this really something the user would write? seems like a gimme as written. more likely the user would say "make DoIt() async so it's faster" and the AI would have to figure out that it's CPU bound? -> -> **@danmoseley** — `eval.yaml` L33: do you expect `ConfigureAwait(false)` in this UserService case? either way, should be a test for the opposite case (eg winforms) and verify in each case it's present or not as expected. -> -> **@danmoseley** — `SKILL.md` L148: (suggestion to expand the error→fix table with more compiler error codes) - -### ViktorHofer (2026-03-06) -> @mrsharm check the one scenario in which the skill doesn't get activated. - -### mrsharm (2026-03-06) -> @ViktorHofer: Intentional - is a negative test and is the correct outcome. The eval suggests optimizing matrix multiplication (CPU-bound), not about converting I/O to async. The skill's description targets I/O-bound async refactoring, so the agent should recognize this isn't an async problem and not load the skill. - -### ViktorHofer (2026-03-08) -> @mrsharm such tests must be annotated with `expect_activation: false`. Otherwise we have no way to distinguish between intentionally not activated and unintentionally not activated. - -### mrsharm (2026-03-08) -> @ViktorHofer - thanks! Been implemented for this and the other PRs. - -### ManishJayaswal (2026-03-10) -> @mrsharm - the repo has undergone some restructuring. This skill is already under the correct plugin - dotnet. Please update the PR and submit again. -> @jasonmalinowski @jaredpar - please review. - -### mrsharm (2026-03-06) -> @danmoseley - could you please take another look? I believe I have addressed your feedback. - -## Review Comments (PR #269) - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/UserService.cs` (resolved) -> The comment says "Sync-over-async: blocking call", but the code is using the synchronous `HttpClient.Send(...)` API. Either adjust the comment or change the sample to a true sync-over-async pattern. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` L3 (resolved) -> PR description 'Files' list appears incomplete relative to the actual changes. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) -> `output_not_matches` is currently broad enough that it will also fail when the model *mentions* `.Result` / `.Wait()` as anti-patterns in prose. Consider tightening the regex. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) -> The CPU-bound scenario's `output_not_matches` pattern includes `async Task`, which can easily appear in an explanation. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) -> The Stream `ReadAsync`/`WriteAsync` examples don't match the actual Stream APIs. Recommend updating to use the correct overloads. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/UserService.cs` (resolved) -> `_httpClient` is injected and stored but never used in `UserRepository`. - -### mrsharm — `.github/CODEOWNERS` (resolved) -> Update this to the right owner. @danmoseley / @timheuer - who should be the right owner for this skill? - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) -> `output_not_matches` regex will fail if the response mentions `.GetAwaiter().GetResult()` in explanatory text. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) -> The `ApiClient.cs` fixture uses `StreamReader` but doesn't include `using System.IO;`. - -### copilot-pull-request-reviewer — `.github/CODEOWNERS` (resolved, 2 comments) -> These CODEOWNERS entries for the new dotnet skill/tests are placed under the `# dotnet-upgrade` section. Move them under the `# dotnet` section. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) -> PR description text appears to contain a duplicated sentence at the end. - -### jaredpar — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved, 2 comments) -> **jaredpar:** The budget for skills descriptions is limited. This seems longer than I would expect for a skill. -> -> **danmoseley:** We only validate the hard limit of 1024 chars. Beyond that, it's hard to score whether there are too many (waste) or appropriate (prevents loading the skill inappropriately). - -### jaredpar / danmoseley — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved, 2 comments) -> **jaredpar:** I worry that the prompt here is too strongly connected to the prompts in the skill. Basically the skill says "for ASP.NET do X" and the eval says "this is ASP.NET". It feels like the test is being fitted for the skill vs. the user experience. My intuition is that customers would more naturally write: "This controller has performance issues under load..." -> -> **danmoseley:** yes, I agree this is arguably "overfitting". Current overfitting judge doesn't catch it. @JanKrivanek I wonder whether this suggests we should tune prompt given to the overfitting judge. - -### danmoseley — `tests/dotnet/refactoring-to-async/UserService.cs` L23 (resolved) -> should the skill recommend using `IAsyncDisposable` (ie `await using`)? and eval should check that all these `using var` get converted. - -### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) -> (suggestion) Missing `ConfigureAwait(false)` in libraries | Can deadlock in Winforms/WPF/ASP.NET sync contexts | Add `.ConfigureAwait(false)` to every `await` in library code; omit in ASP.NET Core app code (no SynchronizationContext) - -### danmoseley — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) -> AI says, "Task.WhenAll fires all HTTP requests concurrently, which could overwhelm a downstream service. Sequential await should be the preferred default; WhenAll should only be suggested with a concurrency caveat." - -### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` L212 (resolved) -> There should be a bit more mention of `ValueTask` in the guidance higher up and its limitations over `Task`. Eg., cannot be awaited multiple times, cannot be stored. - -### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` L185 (resolved) -> Is `IAsyncEnumerable` / `await foreach` in scope? Probably should be. If not, it should say it's out of scope. Similar for `IAsyncDisposable`. - -### danmoseley — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) -> Suggest to provide a list of example method names, rather than write the grep for it. It's great at writing grep, and giving it the command suggests that this is a complete list. Plus if this hits something inappropriate eg a random property named "Result" it can adjust itself. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` L177 (resolved) -> The controller scenario rubric requires removing sync-over-async anti-patterns, but the assertions only check for an async signature + CancellationToken. Add an `output_not_matches` assertion. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) -> This CancellationToken example also leaves the `HttpResponseMessage` undisposed. Should use `using var response = await ...`. - -### copilot-pull-request-reviewer — `plugins/dotnet/skills/refactoring-to-async/SKILL.md` (resolved) -> In the async "After" example, `HttpResponseMessage` is not disposed. Should demonstrate disposing it. - -### copilot-pull-request-reviewer — `.github/CODEOWNERS` (resolved) -> CODEOWNERS entries placed under the `# dotnet-upgrade` section — should be under `# dotnet`. - -### copilot-pull-request-reviewer — `tests/dotnet/refactoring-to-async/eval.yaml` (resolved) -> Scenario 1's rubric calls out removing `.GetAwaiter().GetResult()` sync-over-async, but the `output_not_matches` assertion only checks for `.Result` and `.Wait()`. - ---- - -## Old PR #79 (closed) — Add refactoring-to-async skill - -**State:** Closed (not merged) | **Author:** mrsharm | **Created:** 2026-02-23 | **Closed:** 2026-03-06 - -### Discussion Comments - -#### ViktorHofer (2026-02-23) -> Please share the results as a comment similar to #75. Also make sure that you test at least 3 runs (`--runs 3` for local validation). - -#### mrsharm (2026-02-25) — Eval Results (posted twice) -> | Skill | Test | Baseline | With Skill | Δ | Verdict | -> |-------|------|----------|------------|---|---------| -> | refactoring-to-async | Refactor synchronous service to async | 3.3/5 | 5.0/5 | +1.7 | ✅ | -> | refactoring-to-async | Async refactoring should not apply to CPU-bound code | 1.0/5 | 1.0/5 | 0.0 | ✅ | -> -> **Overall improvement: +18.8%** (3 runs, not statistically significant) - -#### mrsharm (2026-02-25) -> > Please share the results as a comment similar to #75. Also make sure that you test at least 3 runs. -> -> Done. - -#### danmoseley (2026-03-02) -> add codeowners entry, move files around to match new pattern in main. - -#### mrsharm (2026-03-06) -> Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. - -### Review Comments (PR #79) - -#### copilot-pull-request-reviewer — `src/dotnet/tests/refactoring-to-async/eval.yaml` L12 -> The scenario requires identifying sync-over-async patterns like `.Result`/`.Wait()`, but the `output_not_matches` assertion fails the run if the agent output contains `.Result` or `.Wait()` anywhere (including when calling them out as anti-patterns). - -#### copilot-pull-request-reviewer — `src/dotnet/skills/refactoring-to-async/SKILL.md` L35 -> The `grep` pattern uses `\b` for a word-boundary, but `grep`'s default regex syntax does not treat `\b` as a word boundary (it's typically interpreted as a backspace). - -#### copilot-pull-request-reviewer — `src/dotnet/skills/refactoring-to-async/SKILL.md` L155 -> Same issue as Step 1: `grep` without `-P` won't treat `\b` as a word boundary. - -#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L8 -> move any part of use/not use into description similar to runtime formatting, if it can avoid unnecessary reloading. Possibly not all can go there. For example: user wants to parallelize work is something that can be known before loading the skill, and avoid load. - -#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L167 -> this [ConfigureAwait(false)] is missing from all the examples. maybe indicate those are app code? It might be worth mentioning as instructions, not just in anti-patterns — if code is library add `.ConfigureAwait(false)` to every await. It should be used consistently or not at all. - -#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L185 -> this may be a bit high level/conceptual for this skill? - -#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L53 -> (suggestion) Add `(DbDataReader)` and `(DbCommand)` qualifiers to the sync→async mapping table entries. - -#### danmoseley — `src/dotnet/tests/refactoring-to-async/eval.yaml` L25 -> is this really something the user would write? seems like a gimme as written. more likely the user would say "make DoIt() async so it's faster" and the AI would have to figure out that it's CPU bound? - -#### danmoseley — `src/dotnet/tests/refactoring-to-async/eval.yaml` L33 -> do you expect `ConfigureAwait(false)` in this UserService case? either way, should be a test for the opposite case (eg winforms) and verify in each case it's present or not as expected. - -#### danmoseley — `src/dotnet/skills/refactoring-to-async/SKILL.md` L148 -> (suggestion to expand the error→fix table with more compiler error codes like CS4032, CS0029, CS0127, CS1983, CS1998, CS0535, CS7036, CS1503) From b4fd58358a9cd42c84873231aeb0bc4dce9bf05a Mon Sep 17 00:00:00 2001 From: Viktor Hofer <7412651+ViktorHofer@users.noreply.github.com> Date: Tue, 31 Mar 2026 10:27:17 +0200 Subject: [PATCH 16/16] Delete pr-264-description.md --- pr-264-description.md | 42 ------------------------------------------ 1 file changed, 42 deletions(-) delete mode 100644 pr-264-description.md diff --git a/pr-264-description.md b/pr-264-description.md deleted file mode 100644 index 3b8517c48b..0000000000 --- a/pr-264-description.md +++ /dev/null @@ -1,42 +0,0 @@ -## Summary -Adds the **minimal-api-file-upload** skill for handling file uploads in ASP.NET Core 8+ minimal APIs. - -> **Note:** Replaces #155 (migrated from skills-old repo to new plugins/ structure). - -## What the Skill Teaches - -The base model consistently gets several file upload patterns wrong. This skill addresses five key gaps: - -1. **Dual size limits** — Must configure BOTH `Kestrel.MaxRequestBodySize` AND `FormOptions.MultipartBodyLengthLimit`. The base model typically configures only one, causing cryptic upload failures. -2. **Antiforgery auto-validation** — In .NET 8+, `UseAntiforgery()` silently rejects all form-bound uploads with 400 Bad Request. Must call `.DisableAntiforgery()` on API endpoints (with appropriate security caveats for cookie-auth). -3. **Magic byte validation** — The base model relies solely on `ContentType` which is client-spoofable. The skill teaches JPEG/PNG file signature verification. -4. **Safe filename generation** — The base model often uses `file.FileName` directly, enabling path traversal. The skill teaches GUID-based names and also covers sanitizing user-provided filenames when they must be retained (per reviewer feedback from @mikekistler). -5. **Streaming with MultipartReader** — For very large files (>500MB), IFormFile buffering causes OOM. The skill teaches direct-to-disk streaming via `MultipartReader`. - -## Eval Results (3-run local validation) - -| Scenario | Baseline | With Skill | Δ | Overfit | Verdict | -|----------|----------|------------|---|---------|---------| -| Implement secure file upload | 3.7/5 | **5.0/5** | +35% | 0.08 ✅ | ✅ | -| Upload multiple files with metadata | 3.3/5 | **5.0/5** | +52% | 0.08 ✅ | ✅ | -| Stream very large file uploads | 4.7/5 | **5.0/5** | +6% | 0.08 ✅ | ✅ | - -Model: claude-opus-4.6 | Judge: claude-opus-4.6 - -## Files - -- `plugins/dotnet-aspnet/plugin.json` (new plugin) -- `plugins/dotnet-aspnet/skills/minimal-api-file-upload/SKILL.md` -- `tests/dotnet-aspnet/minimal-api-file-upload/eval.yaml` - -## Review feedback addressed - -- Added filename sanitization as valid alternative to GUID-only approach (per @mikekistler) -- Removed code fence wrapper from SKILL.md frontmatter (per Copilot review) -- Removed `image/gif` from allowlist to match JPEG/PNG-only eval scope -- Fixed `GetMultipartBoundary()` — replaced with standard `HeaderUtilities` boundary parsing -- Fixed misleading "IFormFile buffers entirely in memory" claim — clarified memory threshold + temp file behavior -- Derived file extension from validated magic bytes instead of user-controlled `Path.GetExtension(file.FileName)` -- Added security caveat for `DisableAntiforgery()` on cookie-authenticated endpoints (per @halter73) -- Moved skill from `plugins/dotnet` to `plugins/dotnet-aspnet` per reviewer guidance -- Iterated eval scenarios through 5+ runs — removed noisy tests (baseline-perfect antiforgery diagnosis, non-activation JSON test with efficiency noise), simplified multi-file prompt to prevent agent project scaffolding overhead