Skip to content

Add refactoring-to-async skill#269

Open
mrsharm wants to merge 18 commits into
dotnet:mainfrom
mrsharm:musharm/refactoring-to-async-skill
Open

Add refactoring-to-async skill#269
mrsharm wants to merge 18 commits into
dotnet:mainfrom
mrsharm:musharm/refactoring-to-async-skill

Conversation

@mrsharm

@mrsharm mrsharm commented Mar 6, 2026

Copy link
Copy Markdown
Member

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

  • Bottom-up async conversion order — start from leaf I/O methods, propagate upward
  • Correct sync-to-async API mappings (stream.Read → ReadAsync, connection.Open → OpenAsync, etc.)
  • CancellationToken propagation through the entire call chain
  • Identifying sync-over-async anti-patterns (.Result, .Wait(), .GetAwaiter().GetResult())
  • ConfigureAwait(false) usage in library code vs ASP.NET Core apps
  • Recognizing when async does not apply (CPU-bound work)Adds the refactoring-to-async skill for bottom-up async conversion patterns in .NET.

@mrsharm

mrsharm commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

Migration Note

This PR replaces #79 which was opened from mrsharm/skills-old. The skill and eval files have been migrated to the new plugins/ directory structure:

  • src/dotnet/skills/refactoring-to-async/plugins/dotnet/skills/refactoring-to-async/
  • src/dotnet/tests/refactoring-to-async/tests/dotnet/refactoring-to-async/

All prior review feedback from #79 still applies — please see that PR for the full discussion history.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-async skill documentation under plugins/dotnet/skills/.
  • Added a new eval scenario plus C# fixture project under tests/dotnet/refactoring-to-async/.
  • Updated .github/CODEOWNERS to 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.

Comment thread tests/dotnet/refactoring-to-async/UserService.cs Outdated
Comment thread tests/dotnet/refactoring-to-async/eval.yaml Outdated
Comment thread tests/dotnet/refactoring-to-async/eval.yaml Outdated
Comment thread tests/dotnet/refactoring-to-async/eval.yaml Outdated
Comment thread plugins/dotnet/skills/refactoring-to-async/SKILL.md Outdated
Comment thread tests/dotnet/refactoring-to-async/UserService.cs Outdated
@mrsharm

mrsharm commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

Feedback carried over from #79 (refactoring-to-async)

Code Review Comments (10 threads)

@copiloteval.yaml L12

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 ...


@copilotSKILL.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). 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 `.W...


@copilotSKILL.md L155

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 -rnP '\.Result\b|\.Wait\(\)|\.GetAwaiter\(\)\.GetResult\(\)' --include="*.cs" .

@danmoseleySKILL.md L8

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 ...


@danmoseleySKILL.md L167

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.


@danmoseleySKILL.md L185

this may be a big high level/conceptual for this skill?


@danmoseleySKILL.md L53

| `reader.Read()` (DbDataReader) | `await reader.ReadAsync()` |
| `command.ExecuteNonQuery()` (DbCommand) | `await command.ExecuteNonQueryAsync()` |

maybe


@danmoseleyeval.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?


@danmoseleyeval.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


@danmoseleySKILL.md L148

| 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 meth...

Discussion Comments (5)

@ViktorHofer:

Please share the results as a comment similar to #75 (comment)
Also make sure that you test at least 3 runs (--runs 3 for local validation): https://github.com/dotnet/skills/pull/82/changes


@mrsharm:

Skill Validation Results ΓÇö refactoring-to-async

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%...


@mrsharm:

Skill Validation Results ΓÇö refactoring-to-async

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%...


@mrsharm:

Please share the results as a comment similar to #75 (comment) Also make sure that you test at least 3 runs (--runs 3 for local validation): https://github.com/dotnet/skills/pull/82/changes

Done.


@danmoseley:

add codeowners entry, move files around to match new pattern in main.


Comment thread .github/CODEOWNERS Outdated
@ViktorHofer

Copy link
Copy Markdown
Member

/evaluate

@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Skill Validation Results

Skill Scenario Baseline With Skill Δ Skills Loaded Overfit Verdict
refactoring-to-async Refactor synchronous service to async 4.0/5 5.0/5 +1.0 ✅ refactoring-to-async; tools: skill, grep ✅ 0.10
refactoring-to-async Async refactoring should not apply to CPU-bound code 2.7/5 5.0/5 +2.3 ℹ️ not activated (expected) ✅ 0.10
refactoring-to-async Library code should use ConfigureAwait(false) 4.3/5 5.0/5 +0.7 ✅ refactoring-to-async; tools: skill, bash ✅ 0.10
refactoring-to-async Partial async conversion in large codebase 3.3/5 4.0/5 +0.7 ✅ refactoring-to-async; tools: skill ✅ 0.10

Model: claude-opus-4.6 | Judge: claude-opus-4.6

Full results

@dotnet dotnet deleted a comment from github-actions Bot Mar 6, 2026
@dotnet dotnet deleted a comment from github-actions Bot Mar 6, 2026
@mrsharm

mrsharm commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

/evaluate

@dotnet dotnet deleted a comment from github-actions Bot Mar 6, 2026
@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

✅ Evaluation completed. View results | View workflow run

@mrsharm

mrsharm commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

@danmoseley - could you please take another look? I believe I have addressed your feedback.

@ViktorHofer

Copy link
Copy Markdown
Member

@mrsharm check the one scenario in which the skill doesn't get activated.

@mrsharm

mrsharm commented Mar 6, 2026

Copy link
Copy Markdown
Member Author

@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.

@ViktorHofer

Copy link
Copy Markdown
Member

@mrsharm such tests must be annotated with expect_activation: false:

expect_activation: false

Otherwise we have no way to distinguish between intentionally not activated and unintentionally not activated.

@mrsharm

mrsharm commented Mar 8, 2026

Copy link
Copy Markdown
Member Author

/evaluate

@github-actions

github-actions Bot commented Mar 8, 2026

Copy link
Copy Markdown
Contributor

✅ Evaluation completed. View results | View workflow run

@mrsharm

mrsharm commented Mar 8, 2026

Copy link
Copy Markdown
Member Author

@ViktorHofer - thanks! Been implemented for this and the other PRs.

@ManishJayaswal

ManishJayaswal commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

@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 is already under the correct plugin - dotnet. Please update the PR and submit again.

@jasonmalinowski @jaredpar - please review.

Copilot AI review requested due to automatic review settings March 10, 2026 17:57
@github-actions

github-actions Bot commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Skill Validation Results

Skill Scenario Quality Skills Loaded Overfit Verdict
refactoring-to-async Refactor synchronous service to async 3.3/5 → 4.7/5 🟢 ✅ refactoring-to-async; tools: skill, grep ✅ 0.10
refactoring-to-async Async refactoring should not apply to CPU-bound code 3.3/5 → 4.3/5 ⏰ 🟢 ℹ️ not activated (expected) ✅ 0.10 [1]
refactoring-to-async Refactor sync-over-async in ASP.NET Core controller 3.3/5 → 5.0/5 🟢 ✅ refactoring-to-async; tools: skill, grep ✅ 0.10 [2]
refactoring-to-async Convert file I/O operations to async 3.0/5 → 4.7/5 🟢 ✅ refactoring-to-async; tools: skill ✅ 0.10
refactoring-to-async Partial async conversion in large codebase 2.0/5 → 3.0/5 🟢 ✅ refactoring-to-async; tools: skill ✅ 0.10 [3]

[1] ⚠️ High run-to-run variance (CV=4.01) — consider re-running with --runs 5
[2] ⚠️ High run-to-run variance (CV=0.89) — consider re-running with --runs 5
[3] ⚠️ High run-to-run variance (CV=2.47) — consider re-running with --runs 5

timeout — run(s) hit the (120s) scenario timeout limit; scoring may be impacted by aborting model execution before it could produce its full output (increase via timeout in eval.yaml)

Model: claude-opus-4.6 | Judge: claude-opus-4.6

🔍 Full Results - additional metrics and failure investigation steps

▶ Sessions Visualisation -- interactive replay of all evaluation sessions

@mrsharm mrsharm disabled auto-merge April 7, 2026 21:40
@mrsharm

mrsharm commented Apr 7, 2026

Copy link
Copy Markdown
Member Author

@mrsharm if you click "update branch" here to rebase on latest main, you will have somewhat improved evaluation results next time, including a prompt and info for a local agent to optimize things. same for any of your PR's.

this one seems pretty good already :)

Thanks - done! Evals look good - can merge if things look good.

@AbhitejJohn

Copy link
Copy Markdown
Contributor

@mrsharm This PR was approved back in March and evals pass — is this still something you'd like to land? It likely needs a rebase on main at this point. Let us know if you need help getting it across the finish line.

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Skill Validation Results

Skill Scenario Quality Skills Loaded Overfit Verdict
refactoring-to-async Refactor synchronous service to async 2.7/5 → 2.3/5 🔴 ✅ refactoring-to-async; tools: skill ✅ 0.17 [1]
refactoring-to-async Async refactoring should not apply to CPU-bound code 3.7/5 → 2.0/5 🔴 ℹ️ not activated (expected) ✅ 0.17
refactoring-to-async Refactor sync-over-async in ASP.NET Core controller 3.0/5 → 3.0/5 ✅ refactoring-to-async; tools: skill / ✅ refactoring-to-async; tools: skill, read_bash ✅ 0.17 [2]
refactoring-to-async Convert file I/O operations to async 3.0/5 → 2.7/5 🔴 ✅ refactoring-to-async; tools: skill ✅ 0.17 [3]
refactoring-to-async Partial async conversion in large codebase 2.0/5 → 1.3/5 🔴 ✅ refactoring-to-async; tools: skill / ✅ refactoring-to-async; tools: skill, bash ✅ 0.17 [4]

[1] ⚠️ High run-to-run variance (CV=105%) — consider re-running with --runs 5
[2] ⚠️ High run-to-run variance (CV=81%) — consider re-running with --runs 5. (Isolated) Quality unchanged but weighted score is -4.1% due to: quality, tool calls (5 → 7)
[3] ⚠️ High run-to-run variance (CV=3076%) — consider re-running with --runs 5
[4] ⚠️ High run-to-run variance (CV=66%) — consider re-running with --runs 5

Model: claude-opus-4.6 | Judge: claude-opus-4.6

🔍 Full Results - additional metrics and failure investigation steps

To investigate failures, paste this to your AI coding agent:

For PR 269 in dotnet/skills, download eval artifacts with gh run download 27010617472 --repo dotnet/skills --pattern "skill-validator-results-*" --dir ./eval-results, then fetch https://raw.githubusercontent.com/dotnet/skills/00ae665171104a93f0786f723a2edf088d3b57e7/eng/skill-validator/src/docs/InvestigatingResults.md and follow it to analyze the results.json files. Diagnose each failure, suggest fixes to the eval.yaml and skill content, and tell me what to fix first.

▶ Sessions Visualisation -- interactive replay of all evaluation sessions
📊 Session Analytics (preview) -- aggregated metrics across evaluation sessions

@github-actions github-actions Bot added ready-to-merge PR state label and removed pr-state/ready-for-eval PR is mergeable and awaiting evaluation labels Jun 5, 2026
@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

✅ Approved by @ViktorHofer. cc @dotnet/skills-merge-approvers — ready to merge.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

✅ Approved by @ViktorHofer. cc @dotnet/skills-merge-approvers — ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants