Add .NET API design review agent and skill by artl93 · Pull Request #15 · dotnet/skills

artl93 · 2026-02-12T21:09:51Z

.NET API Design Review Agent & Skill

This PR introduces a reviewing-dotnet-api-design agent and skill for reviewing .NET API designs against established C# conventions. The skill explicitly avoids citing the copyrighted Framework Design Guidelines book — all conventions are sourced from established patterns in the BCL itself.

What it does

Reviews public .NET API surfaces for consistency with established C# conventions across 9 areas: naming, type design, member design, error handling, collections, resource management, extensibility, and breaking changes.

What the skill adds over base Claude

Capability	Without Skill	With Skill
Caller-first methodology	Jumps straight into reviewing code	Writes calling code BEFORE reviewing — awkward caller code means the API needs work
Severity classification	Flat list of observations	Critical / Warning / Suggestion grouping with specific criteria for each
Convention-specific catches	Misses `List<T>` in public API (valid code)	Flags as Critical — convention is `Collection<T>` / `ReadOnlyCollection<T>`
Mutable struct detection	May note "consider immutability"	Flags as Critical with full type design rationale
Error handling conventions	Generic advice	Flags missing `paramName`, recommends `ThrowIfNull` helpers
Breaking change assessment	Not included	10-row table of what's breaking vs safe
Structured workflow	Ad-hoc review	9-step methodology: classify → naming → types → members → errors → collections → resources → extensibility → breaking changes

Architecture

SKILL.md acts as a router — it defines the 9-step workflow and points to 5 reference files loaded on-demand via progressive disclosure:

agents/reviewing-dotnet-api-design.agent.md    (43 lines)
skills/reviewing-dotnet-api-design/
├── SKILL.md                                    (109 lines)
└── references/
    ├── naming-conventions.md                   (71 lines)
    ├── type-design-patterns.md                 (135 lines)
    ├── member-design-patterns.md               (157 lines)
    ├── error-handling-patterns.md               (176 lines)
    └── api-review-checklist.md                 (150 lines)
tests/reviewing-dotnet-api-design/
├── README.md                                   (prompt + 10-point evaluation criteria)
├── test-api.cs                                 (test asset with 6 deliberate violations + 3 strengths)
├── bad.md                                      (expected output without skill)
└── good.md                                     (expected output with skill)

Total: 841 lines (agent + skill + references). Trimmed per Claude skill authoring best practices — only adds context Claude doesn't already have.

Test cases

tests/reviewing-dotnet-api-design/ includes a test asset (test-api.cs) with 6 embedded violations and 3 correctly implemented patterns. The README defines a 10-point evaluation rubric comparing with-skill vs without-skill output.

Closes #10

artl93 · 2026-02-12T21:12:53Z

Validated with Humanizer.

artl93

Hold off until I've done some work.

artl93 · 2026-02-13T20:16:08Z

@@ -0,0 +1,126 @@
+---
+description: "Use this agent when the user wants to review, design, or improve .NET API surfaces for consistency with established C# conventions.\n\nTrigger phrases include:\n- 'review my API design'\n- 'is this API consistent with .NET conventions?'\n- 'check my public API surface'\n- 'help me design this .NET API'\n- 'review my naming conventions'\n- 'should this be a class or struct?'\n- 'is this a breaking change?'\n- 'prepare an API proposal'\n- 'check my exception design'\n- 'review my library API'\n\nExamples:\n- User asks 'Does my API follow .NET naming conventions?' → invoke this agent to review naming against C# conventions\n- User shares a class and asks 'Is this API well-designed?' → invoke this agent to perform a full API design review\n- User asks 'Should I use a class or struct for this type?' → invoke this agent to analyze type design\n- User says 'I need to add a new overload without breaking existing callers' → invoke this agent to assess breaking change risk\n- User asks 'Review this API proposal before I submit it' → invoke this agent to apply the full API review checklist"


@artl93 - note to self - trim the heck out of this and everything else. Reduce using the best practices from Anthropic using copilot.

Agent (126→43 lines, -66%): - Rename to reviewing-dotnet-api-design (gerund form) - Rewrite description: third person, no trigger phrases/examples - Remove redundant sections: Mission, Core Design Philosophy, Review Methodology (duplicated skill), When to Ask, Tone - Keep: key principles, output format, skill reference, escalation SKILL.md (360→109 lines, -70%): - Rename to reviewing-dotnet-api-design (gerund form) - Convert from inline conventions to router pattern - Steps 2-5 now point to reference files instead of duplicating content - Remove When to Use, When Not to Use, Failure Modes - Consolidate validation checklist and common pitfalls Reference files (1,003→689 lines, -31%): - naming-conventions.md: 177→71 (-60%) — removed prose, kept tables - type-design-patterns.md: 199→135 (-32%) — removed explanations Claude knows - member-design-patterns.md: 266→157 (-41%) — kept non-obvious patterns - error-handling-patterns.md: 195→176 (-10%) — light trim, unique value - api-review-checklist.md: 166→150 (-10%) — trimmed process prose Total: 1,489→841 lines (-44%)

Test asset (test-api.cs) contains 6 deliberate API design violations: - Mutable struct with reference-type field (Critical) - List<T> return in public API (Critical) - Noun-named method (Warning) - Missing paramName on exception (Warning) - Unsealed leaf class (Warning) - Expensive property (Suggestion) Plus 3 correctly implemented patterns (event, IDisposable, EventArgs). README defines 10-point evaluation criteria comparing with/without skill. bad.md and good.md show expected behavior difference.

Replace easy violations (labeled comments, obvious issues) with a realistic message broker API containing 14 violations that require .NET-specific convention knowledge to catch: Hard catches (unlikely without skill): - Mutable struct >16 bytes with Dictionary<> field - [Flags] enum with non-power-of-two values (ExactlyOnce = 3) - Parameter name inconsistency across related methods - Async argument validation inside Task.Run instead of synchronous - Unpaired operator == (missing !=, Equals, GetHashCode) - Missing TryParse companion for Parse method - Abbreviated parameter name in public API (msg vs message) The test code compiles and works correctly — violations are invisible without convention knowledge from the skill's reference files.

Removed instruction to avoid citing specific resources.

bartonjs

I've remembered today is a holiday and I'm not working. I'll look more later.

bartonjs · 2026-02-17T00:49:19Z

+
+## ToString Pattern
+
+Every type should override `ToString()` with a human-readable representation.


No, no it shouldn't.

The guideline is "override ToString whenever an interesting human-readable string
can be returned." "whenever" is a very important word there.

bartonjs · 2026-02-17T00:53:23Z

+- [ ] Properties are cheap and idempotent
+- [ ] Methods used for operations, conversions, expensive work
+- [ ] Overloads have consistent parameter order
+- [ ] Simplest overload delegates to most complete


Some of these are about implementation (which can be visible to the caller), but aren't really visible in an API proposal. Like which ctor delegates to which. (The asmmeta or ref.cs just says they exist, it doesn't show the deferral)

bartonjs · 2026-02-17T00:54:21Z

+- [ ] Standard exception types used
+- [ ] No direct `Exception` or `SystemException` throwing
+- [ ] `paramName` set on argument exceptions
+- [ ] Exception messages are clear and actionable


All of these parts about exceptions aren't measurable during an API Review, so they couldn't be assessed in an API Review preflight.

bartonjs · 2026-02-17T00:57:24Z

+
+Note: Use `value` as the parameter name in property setter exceptions.
+
+### Legacy Pattern (pre-.NET 6)


I'm not sure where "pre-.NET 6" comes from. SetName would be correct if it can only be set once, or the setter would have an interdependency with other properties, or a few other reasons why a property setter would be bad.

bartonjs · 2026-02-17T00:58:38Z

+1. Parameter order is consistent across all overloads
+2. Simpler overloads delegate to the most complete one
+3. Parameter names are identical across overloads
+4. `CancellationToken` is always the last parameter


CancellationToken should also (almost) always be defaulted.

bartonjs · 2026-02-17T00:59:42Z

+2. Simpler overloads delegate to the most complete one
+3. Parameter names are identical across overloads
+4. `CancellationToken` is always the last parameter
+5. `params` array overload is the most flexible variant


We probably don't want any new params[]. Maybe params ReadOnlySpan. Or just make people type square braces. But probably isn't something really to be encouraged.

bartonjs · 2026-02-17T01:01:35Z

+
+## Property Names
+
+Boolean properties use `Is`/`Can`/`Has` prefix.


Sometimes. Like "CanRead" is better than "Read", but "Closed" is probably better than "IsClosed" But, if there's a Closed event, then the property ends up as IsClosed because the event wins the name conflict.

bartonjs · 2026-02-17T01:04:37Z

+    // Abstract: derived types MUST implement
+    public abstract int Read(byte[] buffer, int offset, int count);
+    public abstract void Write(byte[] buffer, int offset, int count);
+    public abstract long Length { get; }


public abstract and public virtual (for things that take parameters) are almost an anti-pattern at this point. Instead, public int Read(byte[] buffer, int offset, int count) should do argument validation and call protected abstract int ReadCore(...). And, of course, these days the ReadCore would probably be span-based.

jeffschwMSFT

please rebase into the new repro structure and run the skill validator

artl93 · 2026-02-27T17:06:57Z

@jeffschwMSFT - ran evals on this - I don't think this skill is worth it compared to what the models can do.

… Workforce

Add .NET API design reviewer agent and api-design-cop skill

e29b1a8

artl93 marked this pull request as ready for review February 12, 2026 21:12

artl93 commented Feb 13, 2026

View reviewed changes

artl93 changed the title ~~Add .NET API design reviewer agent and api-design-cop skill~~ Add .NET API design review agent and skill Feb 14, 2026

artl93 and others added 3 commits February 13, 2026 16:57

Clarify guidelines for .NET API design review

826ffc9

Removed instruction to avoid citing specific resources.

artl93 requested review from bartonjs and stephentoub February 17, 2026 00:38

bartonjs reviewed Feb 17, 2026

View reviewed changes

jeffschwMSFT requested changes Feb 21, 2026

View reviewed changes

artl93 closed this Feb 27, 2026

github-actions Bot mentioned this pull request Jun 3, 2026

fix(dotnet): add Claude Code-shaped .lsp.json so LSP loads in Claude Code #598

Merged

3 tasks

moesac0970 pushed a commit to moesac0970/skills that referenced this pull request Jul 4, 2026

task(dotnet#15): remove dotnet-blazor/author-component - no Blazor in…

d555ba7

… Workforce

		@@ -0,0 +1,126 @@
		---
		description: "Use this agent when the user wants to review, design, or improve .NET API surfaces for consistency with established C# conventions.\n\nTrigger phrases include:\n- 'review my API design'\n- 'is this API consistent with .NET conventions?'\n- 'check my public API surface'\n- 'help me design this .NET API'\n- 'review my naming conventions'\n- 'should this be a class or struct?'\n- 'is this a breaking change?'\n- 'prepare an API proposal'\n- 'check my exception design'\n- 'review my library API'\n\nExamples:\n- User asks 'Does my API follow .NET naming conventions?' → invoke this agent to review naming against C# conventions\n- User shares a class and asks 'Is this API well-designed?' → invoke this agent to perform a full API design review\n- User asks 'Should I use a class or struct for this type?' → invoke this agent to analyze type design\n- User says 'I need to add a new overload without breaking existing callers' → invoke this agent to assess breaking change risk\n- User asks 'Review this API proposal before I submit it' → invoke this agent to apply the full API review checklist"


		## ToString Pattern

		Every type should override `ToString()` with a human-readable representation.


		Note: Use `value` as the parameter name in property setter exceptions.

		### Legacy Pattern (pre-.NET 6)


		## Property Names

		Boolean properties use `Is`/`Can`/`Has` prefix.

Uh oh!

Conversation

artl93 commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

.NET API Design Review Agent & Skill

What it does

What the skill adds over base Claude

Architecture

Test cases

Uh oh!

artl93 commented Feb 12, 2026

Uh oh!

artl93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffschwMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

artl93 commented Feb 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

artl93 commented Feb 12, 2026 •

edited

Loading