Conversation
|
Please share the results as a comment similar to #75 (comment) |
Teaches bottom-up async conversion patterns: identifying sync-over-async, CancellationToken propagation, ConfigureAwait usage, and ValueTask selection. Eval results: +46.9% improvement over baseline (threshold: 10%) Includes eval.yaml with positive scenario + negative test and fixture files.
mrsharm
force-pushed
the
musharm/refactoring-to-async-skill
branch
from
d251570 to
8c2fb53
Compare
mrsharm
changed the title
Skill Validation Results — refactoring-to-async
Overall improvement: +18.8% (3 runs, not statistically significant) Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
1 similar comment
Skill Validation Results — refactoring-to-async
Overall improvement: +18.8% (3 runs, not statistically significant) Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
mrsharm
changed the title
Done. |
There was a problem hiding this comment.
Pull request overview
Adds a new .NET skill (refactoring-to-async) along with evaluation scenarios and fixture code to teach and validate bottom-up async/await refactoring patterns (Task propagation, CancellationToken flow, and avoiding sync-over-async).
Changes:
- Added
refactoring-to-asyncskill documentation (SKILL.md) describing a structured async conversion workflow. - Added eval scenarios to validate async refactoring behavior and avoidance of async for CPU-bound work.
- Added a C# fixture project (
SyncService) used by the eval scenario as refactoring input.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/dotnet/tests/refactoring-to-async/eval.yaml | New eval scenarios + assertions/rubric for async refactoring and CPU-bound guidance. |
| src/dotnet/tests/refactoring-to-async/UserService.cs | Fixture code containing sync I/O + sync-over-async patterns for the agent to refactor. |
| src/dotnet/tests/refactoring-to-async/SyncService.csproj | Fixture project configuration and dependency needed to compile the sample. |
| src/dotnet/skills/refactoring-to-async/SKILL.md | New skill guidance and workflow (search patterns, conversion order, cancellation, validation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - type: "output_matches" | ||
| pattern: "(CancellationToken|cancellation)" | ||
| - type: "output_not_matches" | ||
| pattern: "\\.Result\\b|\\.Wait\\(\\)" |
There was a problem hiding this comment.
The scenario requires identifying sync-over-async patterns like .Result/.Wait() (rubric line 14), 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). This creates false negatives and makes the rubric effectively incompatible with the assertions. Consider tightening the regex to only match actual code usage (e.g., requiring a trailing semicolon) or removing this assertion and relying on other checks.
| pattern: "\\.Result\\b|\\.Wait\\(\\)" | |
| pattern: "\\.Result\\b\\s*;|\\.Wait\\(\\)\\s*;" |
| Search for synchronous I/O patterns in the codebase: | ||
|
|
||
| ```bash | ||
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()\|ReadToEnd()\|\.Read()\|\.Write(" --include="*.cs" . |
There was a problem hiding this comment.
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). Also, \.Read() only matches parameterless Read() calls and will miss the common Read(...) overloads. Consider switching to rg (ripgrep) or grep -P/grep -E with a corrected pattern (e.g., matching \.Read\( and \.Write\(, and using a portable word boundary like \<Result\> or PCRE \b with -P).
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()\|ReadToEnd()\|\.Read()\|\.Write(" --include="*.cs" . | |
| grep -rnP "\.Result\b|\.Wait\(\)|\.GetAwaiter\(\)\.GetResult\(\)|ReadToEnd\(\)|\.Read\(|\.Write\(" --include="*.cs" . |
| Search for remaining issues: | ||
|
|
||
| ```bash | ||
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()" --include="*.cs" . |
There was a problem hiding this comment.
Same issue as Step 1: grep without -P won't treat \b as a word boundary, so this command may not reliably find .Result usages. Adjust the command to use a regex flavor that supports word boundaries (or use \</\>), so the "should return zero results" guidance is accurate.
| grep -rn "\.Result\b\|\.Wait()\|\.GetAwaiter()\.GetResult()" --include="*.cs" . | |
| grep -rnP '\.Result\b|\.Wait\(\)|\.GetAwaiter\(\)\.GetResult\(\)' --include="*.cs" . |
jeffschwMSFT
requested review from
agocke,
artl93,
danmoseley and
stephentoub
|
|
||
| # Refactoring to Async | ||
|
|
||
| ## When to Use |
There was a problem hiding this comment.
move any part of use/not use into decription similar to
https://github.com/dotnet/runtime/pull/125005/changes#diff-ded9450821c1df27638def6250c00784c7f795e3a9c56ad13d09a34853a0d09bR3-R4
if it can avoid unnecessaryt 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. Whereas possibly "code is CPU bound" might need loading the skill? but in general I think ?all of this should move up there.
|
add codeowners entry, move files around to match new pattern in main. |
| | `task.Result` or `task.Wait()` | Blocks thread, risks deadlock | `await task` | | ||
| | `async void` methods | Exceptions crash the process | `async Task` (except event handlers) | | ||
| | `Task.Run` wrapping async I/O | Wastes a thread pool thread | Call async method directly | | ||
| | Missing `ConfigureAwait(false)` in libraries | Can deadlock in UI/ASP.NET sync contexts | Add `ConfigureAwait(false)` in library code | |
There was a problem hiding this comment.
this 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.
| | Deadlock after conversion | Ensure `await` is used everywhere; no `.Result` mixed with `await` | | ||
| | Performance worse after conversion | Async adds overhead for CPU-bound work; only use for I/O | | ||
| | Forgetting to update tests | Test methods must return `Task` and use `await` | | ||
| | Breaking interface consumers | Consider keeping sync wrappers temporarily during staged migration | |
There was a problem hiding this comment.
this may be a big high level/conceptual for this skill?
| | `Thread.Sleep(ms)` | `await Task.Delay(ms, ct)` | | ||
| | `task.Result` | `await task` | | ||
| | `task.Wait()` | `await task` | | ||
|
|
There was a problem hiding this comment.
| | `reader.Read()` (DbDataReader) | `await reader.ReadAsync()` | | |
| | `command.ExecuteNonQuery()` (DbCommand) | `await command.ExecuteNonQueryAsync()` | | |
maybe
| timeout: 120 | ||
|
|
||
| - name: "Async refactoring should not apply to CPU-bound code" | ||
| prompt: "I have a method that does heavy matrix multiplication using nested for loops. Can you make it faster?" |
There was a problem hiding this comment.
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?
| - "Did NOT suggest converting CPU-bound computation to async/await" | ||
| - "Suggested parallelism approaches (Parallel.For, Task.Run, SIMD, or algorithmic optimization)" | ||
| - "Correctly identified that async is for I/O-bound work, not CPU-bound work" | ||
| timeout: 60 |
There was a problem hiding this comment.
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
| | Error | Fix | | ||
| |---|---| | ||
| | `CS4032`: `await` in non-async method | Add `async` to the method signature and return `Task` or `Task<T>` | | ||
| | `CS0029`: Cannot convert `Task<T>` to `T` | Add `await` before the call | | ||
| | `CS0127`: Method returns `Task` but body returns value | Change return type to `Task<T>` | | ||
| | `CS1998`: Async method lacks `await` | Remove `async` if no awaits are needed, or the method is genuinely sync | |
There was a problem hiding this comment.
| | Error | Fix | | |
| |---|---| | |
| | `CS4032`: `await` in non-async method | Add `async` to the method signature and return `Task` or `Task<T>` | | |
| | `CS0029`: Cannot convert `Task<T>` to `T` | Add `await` before the call | | |
| | `CS0127`: Method returns `Task` but body returns value | Change return type to `Task<T>` | | |
| | `CS1998`: Async method lacks `await` | Remove `async` if no awaits are needed, or the method is genuinely sync | | |
| | Error | Fix | | |
| |---|---| | |
| Error | Fix | | |
| |---|---| | |
| | `CS4032`: `await` in non-async method | Add `async` to the method signature and return `Task` or `Task<T>` | | |
| | `CS0029`: Cannot convert `Task<T>` to `T` | Add `await` before the call | | |
| | `CS0127`: Method returns `Task` but body returns value | Change return type to `Task<T>` | | |
| | `CS1983`: Return type of async method must be void, Task, Task<T>, ValueTask, or ValueTask<T> | Wrap the return type: `async string Foo()` → `async Task<string> Foo()` | | |
| | `CS1998`: Async method lacks `await` | Remove `async` if no awaits are needed, or the method is genuinely sync | | |
| | `CS0535`: Does not implement interface member | Update the interface to match the new async signatures (or vice versa) | | |
| | `CS7036`: No argument given for required parameter `cancellationToken` | Add `ct` argument at call sites after adding `CancellationToken` to signatures | | |
| | `CS1503`: Argument type mismatch (`Task<T>` passed where `T` expected) | Add `await` at the call site | |
if you want to add more?
|
Closing: replaced by new PR from mrsharm/skills with plugins/ directory structure. |
Summary
Adds the refactoring-to-async skill that teaches bottom-up async conversion patterns for .NET codebases.
Eval Results
Skill Validation Results — refactoring-to-async
Overall improvement: +18.8% (3 runs, not statistically significant)
Model: claude-opus-4.6 | Judge: claude-opus-4.6
What the Skill Teaches
Why This Skill Passes
The base model frequently struggles with the correct order of async conversion - suggesting changes to calling methods first instead of leaf methods. This skill teaches the precise procedural sequence.
Files