Conversation
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.
Migration NoteThis PR replaces #79 which was opened from
All prior review feedback from #79 still applies — please see that PR for the full discussion history. |
There was a problem hiding this comment.
Pull request overview
Adds a new refactoring-to-async skill to the dotnet plugin, along with an evaluation scenario and fixture project intended to test bottom-up async conversion guidance.
Changes:
- Added
refactoring-to-asyncskill documentation underplugins/dotnet/skills/. - Added a new eval scenario plus C# fixture project under
tests/dotnet/refactoring-to-async/. - Updated
.github/CODEOWNERSto assign ownership for the new skill and test directory.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/dotnet/skills/refactoring-to-async/SKILL.md | New skill content describing async refactoring workflow and pitfalls. |
| tests/dotnet/refactoring-to-async/eval.yaml | New eval scenarios/assertions for async refactoring and CPU-bound non-async guidance. |
| tests/dotnet/refactoring-to-async/UserService.cs | Fixture code containing sync I/O and blocking patterns to be refactored by the model. |
| tests/dotnet/refactoring-to-async/SyncService.csproj | Fixture project enabling dotnet build-style validation. |
| .github/CODEOWNERS | Adds code owners for the new skill and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Feedback carried over from #79 (refactoring-to-async)Code Review Comments (10 threads)@copilot —
@copilot —
@copilot —
@danmoseley —
@danmoseley —
@danmoseley —
@danmoseley —
@danmoseley —
@danmoseley —
@danmoseley —
Discussion Comments (5)
|
The 'should not apply to CPU-bound code' scenario was dropping agents into an empty workspace, causing them to give up instead of engaging with the problem. Now provides MatrixMultiplier.cs and .csproj so the agent has actual code to optimize.
|
/evaluate |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
SKILL.md changes: - Move when-to-use/not-use into description for lazy-loading activation - Clarify ConfigureAwait(false) guidance: app code vs library code - Streamline decision tree section eval.yaml changes: - Fix CPU-bound negative scenario: inline MatrixMultiplier.cs fixture - Make negative test prompt realistic (user doesn't state 'CPU-bound') - Add ConfigureAwait(false) library code scenario - Add partial async conversion scenario Eval results (3 runs, 4 scenarios): Refactor sync to async: 3.0 -> 5.0 (+2.0) PASS CPU-bound negative: 2.0 -> 5.0 (+3.0) PASS ConfigureAwait library code: 4.0 -> 4.0 ( 0.0) FAIL Partial async conversion: 3.3 -> 3.7 (+0.4) PASS Overall: +33.8% (3/4 passed)
|
/evaluate |
|
✅ Evaluation completed. View results | View workflow run |
|
@danmoseley - could you please take another look? I believe I have addressed your feedback. |
|
@mrsharm check the one scenario in which the skill doesn't get activated. |
@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. That's exactly what happened in all 3 runs. |
|
@mrsharm such tests must be annotated with Otherwise we have no way to distinguish between intentionally not activated and unintentionally not activated. |
|
/evaluate |
|
✅ Evaluation completed. View results | View workflow run |
|
@ViktorHofer - thanks! Been implemented for this and the other PRs. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
/evaluate |
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
…om/mrsharm/skills into musharm/refactoring-to-async-skill
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
.github/CODEOWNERS
Outdated
| /plugins/dotnet-upgrade/skills/migrate-nullable-references/ @danmoseley | ||
| /tests/dotnet-upgrade/migrate-nullable-references/ @danmoseley |
There was a problem hiding this comment.
There are now duplicate CODEOWNERS entries for the exact same migrate-nullable-references paths (lines 30-33). Since CODEOWNERS applies the last matching rule, the earlier two lines are redundant and can confuse future edits—please remove the duplicates or consolidate into a single pair of entries.
| /plugins/dotnet-upgrade/skills/migrate-nullable-references/ @danmoseley | |
| /tests/dotnet-upgrade/migrate-nullable-references/ @danmoseley |
Removed duplicate CODEOWNERS entries for nullable references.
| fixing thread pool starvation from .Result/.Wait()/.GetAwaiter().GetResult(), | ||
| modernizing sync-over-async code, adding CancellationToken support. | ||
| DO NOT USE FOR: CPU-bound computation (use Parallel.For or Task.Run instead), | ||
| code with no I/O operations, parallelizing work rather than making it async. |
There was a problem hiding this comment.
The budget for skills descriptions is limited which is why they are intended to be brief. This seems longer than I would expect for a skill.
There was a problem hiding this comment.
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). If anything evaluation may drive to more tokens (in order to force skill load when it should). I don't know whether human intuition can help much here or not.
|
|
||
| - name: "Refactor sync-over-async in ASP.NET Core controller" | ||
| prompt: | | ||
| This ASP.NET Core controller has performance issues under load. Some methods use .GetAwaiter().GetResult() and others mix sync and async patterns. Can you fix the async anti-patterns and convert it properly? |
There was a problem hiding this comment.
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 say "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 ...
The model would then need to infer what type of code this was to decide the right action from the skill.
There was a problem hiding this comment.
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.
|
|
||
| public User GetById(int id) | ||
| { | ||
| using var connection = new SqlConnection(_connectionString); |
There was a problem hiding this comment.
should the skill recommend using IAsyncDisposable (ie await using) ? and eval should check that all these using var get converted.
| | `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)` to every `await` in library code; omit in ASP.NET Core app code (no SynchronizationContext) | |
There was a problem hiding this comment.
| | Missing `ConfigureAwait(false)` in libraries | Can deadlock in UI/ASP.NET sync contexts | Add `.ConfigureAwait(false)` to every `await` in library code; omit in ASP.NET Core app code (no SynchronizationContext) | | |
| | 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) | |
| - "Converted controller action methods to async Task<ActionResult<T>>" | ||
| - "Replaced .GetAwaiter().GetResult(), .Wait(), and .Result with await" | ||
| - "Added CancellationToken parameter to async actions to support request cancellation" | ||
| - "Used Task.WhenAll or sequential awaits instead of blocking .Result in the loop" |
There was a problem hiding this comment.
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."
| | 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 | | ||
| | `ValueTask` vs `Task` confusion | Use `Task` by default; `ValueTask` only for hot-path methods that frequently return synchronously | |
There was a problem hiding this comment.
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.
| ``` | ||
|
|
||
| This should return zero results in the refactored code paths. | ||
|
|
There was a problem hiding this comment.
Is IAsyncEnumerable / await foreach in scope? Probably should be. If not, it should say it's out of scope. Similar for IAsyncDisposable
| Search for synchronous I/O patterns in the codebase: | ||
|
|
||
| ```bash | ||
| grep -rnE '\.Result\b|\.Wait\(\)|\.GetAwaiter\(\)\.GetResult\(\)|ReadToEnd\(\)|\.Read\(|\.Write\(' --include='*.cs' . |
There was a problem hiding this comment.
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.
|
/evaluate |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - type: "output_matches" | ||
| pattern: "(async Task<ActionResult|async Task<IActionResult)" | ||
| - type: "output_matches" | ||
| pattern: "(CancellationToken|cancellation)" |
There was a problem hiding this comment.
The controller scenario rubric requires removing sync-over-async anti-patterns (.Result/.Wait()/.GetAwaiter().GetResult()), but the assertions only check for an async signature + CancellationToken. Add an output_not_matches assertion (similar to scenario 1) to prevent false passes where the response keeps/mentions these blocking calls.
| pattern: "(CancellationToken|cancellation)" | |
| pattern: "(CancellationToken|cancellation)" | |
| - type: "output_not_matches" | |
| pattern: "\\.Result\\b(?=\\s*[);])|\\.Wait\\(\\)(?=\\s*[);])|\\.GetAwaiter\\(\\)\\.GetResult\\(\\)" |
| --- | ||
| name: refactoring-to-async | ||
| description: > | ||
| Convert synchronous .NET code to async/await, including proper Task propagation, | ||
| cancellation support, and avoiding common async anti-patterns. | ||
| USE FOR: converting blocking I/O calls (database, HTTP, file, stream) to async, | ||
| fixing thread pool starvation from .Result/.Wait()/.GetAwaiter().GetResult(), | ||
| modernizing sync-over-async code, adding CancellationToken support. | ||
| DO NOT USE FOR: CPU-bound computation (use Parallel.For or Task.Run instead), | ||
| code with no I/O operations, parallelizing work rather than making it async. |
There was a problem hiding this comment.
The PR description text appears to contain a duplicated sentence at the end ("Adds the refactoring-to-async skill..."). Consider cleaning that up so the description reads cleanly for reviewers and release notes.
Skill Validation Results
Model: claude-opus-4.6 | Judge: claude-opus-4.6 |
Adds the refactoring-to-async skill for bottom-up async conversion patterns in .NET.
Note: Replaces #79 (migrated from skills-old repo to new plugins/ structure).
What the Skill Teaches