diff --git a/skills/dotnet-code-analysis/SKILL.md b/skills/dotnet-code-analysis/SKILL.md index b21e1b4..86794f1 100644 --- a/skills/dotnet-code-analysis/SKILL.md +++ b/skills/dotnet-code-analysis/SKILL.md @@ -1,8 +1,8 @@ --- name: dotnet-code-analysis -version: "1.0.0" +version: "1.0.1" category: "Code Quality" -description: "Use the free built-in .NET SDK analyzers and analysis levels. Use when a .NET repo needs first-party code analysis, `EnableNETAnalyzers`, `AnalysisLevel`, or warning policy wired into build and CI." +description: "Use the free built-in .NET SDK analyzers and analysis levels with gradual Roslyn warning promotion. Use when a .NET repo needs first-party code analysis, `EnableNETAnalyzers`, `AnalysisLevel`, or warning-as-error policy wired into build and CI." compatibility: "Requires a .NET SDK-based repository; respects the repo's `AGENTS.md` commands first." --- @@ -13,12 +13,7 @@ compatibility: "Requires a .NET SDK-based repository; respects the repo's `AGENT - the repo wants first-party .NET analyzers - CI should fail on analyzer warnings - the team needs `AnalysisLevel` or `AnalysisMode` guidance - -## Value - -- produce a concrete project delta: code, docs, config, tests, CI, or review artifact -- reduce ambiguity through explicit planning, verification, and final validation skills -- leave reusable project context so future tasks are faster and safer +- the repo needs a gradual Roslyn warning promotion strategy ## Do Not Use For @@ -31,85 +26,167 @@ compatibility: "Requires a .NET SDK-based repository; respects the repo's `AGENT - project files or `Directory.Build.props` - current analyzer severity policy -## Quick Start +## Hard Rules for AI Agents -1. Read the nearest `AGENTS.md` and confirm scope and constraints. -2. Run this skill's `Workflow` through the `Ralph Loop` until outcomes are acceptable. -3. Return the `Required Result Format` with concrete artifacts and verification evidence. +Non-negotiable. Violating these undermines the user's explicit intent. + +1. Never disable or remove `TreatWarningsAsErrors` or `WarningsAsErrors` if the project has set them. Do not comment them out, set to `false`, wrap in a condition, or add `false` to make the build pass. +2. Never add `` or `#pragma warning disable` for warnings the user chose to treat as errors, unless the user explicitly approves the suppression. +3. Never silently downgrade severity in `.editorconfig` (e.g. `error` to `warning` or `none`) to make a build succeed. +4. If warnings-as-errors breaks the build — fix the code. If the fix is too large, ask the user whether to defer that warning ID. +5. If warning volume is too large to fix in one pass — report count and categories to the user and ask which to tackle first. Do not unilaterally disable the policy. ## Workflow -1. Start with SDK analyzers before adding third-party packages. -2. Enable or document: - - `EnableNETAnalyzers` - - `AnalysisLevel` - - `AnalysisMode` - - warning policy such as `TreatWarningsAsErrors` -3. Keep per-rule severity in the repo-root `.editorconfig`. -4. Use `dotnet build` as the analyzer execution gate in CI. -5. Add third-party analyzers only for real gaps that first-party rules do not cover. +```mermaid +flowchart TD + A[Start] --> B{New or legacy project?} + B -->|New| C[TreatWarningsAsErrors=true immediately] + B -->|Legacy| D[dotnet build, count warnings by ID] + D --> E{"< 30 warnings?"} + E -->|Yes| F[Fix all, then enable TreatWarningsAsErrors] + E -->|No| G[Report counts to user, ask which batch first] + G --> H[Add selected IDs to WarningsAsErrors] + H --> I[Fix that batch, verify build] + I --> J{More batches?} + J -->|Yes| G + J -->|No| F + C --> K[Set AnalysisLevel latest-recommended] + F --> K + K --> L[Promote security CA3xxx/CA5xxx to error in .editorconfig] + L --> M[Validate: build + CI green] +``` -## Bootstrap When Missing +1. Start with SDK analyzers before third-party packages. +2. Detect project maturity: new or existing/legacy. +3. Enable `EnableNETAnalyzers`, `AnalysisLevel`, `AnalysisMode` in `Directory.Build.props`. +4. Apply the right warning promotion strategy (see below). +5. Per-rule severity goes in repo-root `.editorconfig`. +6. `dotnet build` is the analyzer gate in CI. + +## Warning Promotion Strategy + +### New Projects + +Set these in `Directory.Build.props` immediately: +- `TreatWarningsAsErrors` = true +- `AnalysisLevel` = latest-recommended +- Security category = error in `.editorconfig` + +Fix all warnings before merging. + +### Legacy Projects — Gradual Promotion + +Blanket `TreatWarningsAsErrors` on a legacy codebase produces hundreds/thousands of errors. An agent cannot fix them all at once — context floods, fix quality drops. Promote in batches. + +#### Phase 1: Trivial Hygiene (start here) + +Mechanical fixes, lowest effort: +- CS8019 — unnecessary using directive (remove it) +- CS0219 — variable assigned but never used (remove it) +- CS0168 — variable declared but never used (remove it) +- CS1591 — missing XML comment for public member (add comment or disable for internal code) +- CS0612 — obsolete member used, no message (replace with non-obsolete API) +- CS0618 — obsolete member used, with message (follow migration guidance) + +Add to `WarningsAsErrors`: `CS8019;CS0219;CS0168`. Fix all, then Phase 2. + +#### Phase 2: Code Quality (ask user which categories) -If first-party .NET code analysis is requested but not configured yet: +- CA2000 — dispose objects before losing scope (Reliability) +- CA1062 — validate public method arguments (Design) +- CA1822 — mark members as static (Performance) +- CA1860 — avoid Enumerable.Any() for length check (Performance) +- CA1861 — avoid constant arrays as arguments (Performance) +- CA2007 — consider calling ConfigureAwait (Reliability) +- CS8600–CS8610 — nullable reference type warnings (Nullability) + +Ask: "Which categories next — Nullability, Performance, or Reliability?" Add selected IDs to `WarningsAsErrors`, fix, repeat. + +#### Phase 3: Security (always promote early) + +Set in `.editorconfig` regardless of project maturity: +```editorconfig +[*.cs] +dotnet_analyzer_diagnostic.category-Security.severity = error +``` + +Covers CA3001 (SQL injection), CA3002 (XSS), CA3003 (path injection), CA3075 (insecure DTD), CA5350/CA5351 (weak crypto), CA5394 (insecure randomness). + +#### Phase 4: Full Coverage + +Once all batches pass, transition to: +```xml +true +CA1707 +``` + +### Interaction Protocol (legacy codebases) + +1. Run `dotnet build`, count warnings by ID. +2. Report summary: "Found 47 CS8019, 23 CA1822, 12 CA2000, 8 CS8600." +3. Ask which batch to tackle. Recommend starting with Phase 1. +4. Fix selected batch, verify build. +5. Add those IDs to `WarningsAsErrors`. +6. Report back, ask about next batch. + +Never skip the ask step. The user decides the pace. + +## Bootstrap When Missing 1. Detect current state: - `dotnet --info` - - `rg -n "EnableNETAnalyzers|AnalysisLevel|AnalysisMode|TreatWarningsAsErrors" -g '*.csproj' -g 'Directory.Build.*' .` -2. Treat SDK analyzers as built-in functionality, not as a separate third-party install path. -3. Enable the needed properties in the solution's MSBuild config, typically in `Directory.Build.props` or the target project file: - - `EnableNETAnalyzers` - - `AnalysisLevel` - - `AnalysisMode` when needed - - warning policy such as `TreatWarningsAsErrors` -4. Keep rule-level severity in the repo-root `.editorconfig`. -5. Run `dotnet build SOLUTION_OR_PROJECT` and return `status: configured` or `status: improved`. -6. If the repo intentionally defers analyzer policy to another documented build layer, return `status: not_applicable`. + - `rg -n "EnableNETAnalyzers|AnalysisLevel|AnalysisMode|TreatWarningsAsErrors|WarningsAsErrors" -g '*.csproj' -g 'Directory.Build.*' .` + - `dotnet build SOLUTION_OR_PROJECT 2>&1` — count warnings by ID +2. Classify: new (few/zero warnings) vs legacy (many warnings). +3. Enable `EnableNETAnalyzers`, `AnalysisLevel`, `AnalysisMode` in MSBuild config. +4. Apply promotion strategy matching project maturity. +5. Per-rule severity in repo-root `.editorconfig`. +6. Run `dotnet build`, return `status: configured` or `status: improved`. +7. If repo defers analyzer policy to another build layer, return `status: not_applicable`. ## Deliver -- first-party analyzer policy that is explicit and reviewable +- explicit, reviewable first-party analyzer policy - build-time analyzer execution for CI +- warning promotion plan matching project maturity ## Validate -- analyzer behavior is driven by repo config, not IDE defaults -- CI can reproduce the same warnings and errors locally +- analyzer behavior driven by repo config, not IDE defaults +- CI reproduces same warnings/errors locally +- no `TreatWarningsAsErrors`, `WarningsAsErrors`, or severity settings removed/weakened without user approval +- promoted warnings produce build errors, not just IDE hints ## Ralph Loop -Use the Ralph Loop for every task, including docs, architecture, testing, and tooling work. - -1. Plan first (mandatory): - - analyze current state - - define target outcome, constraints, and risks - - write a detailed execution plan - - list final validation skills to run at the end, with order and reason -2. Execute one planned step and produce a concrete delta. -3. Review the result and capture findings with actionable next fixes. -4. Apply fixes in small batches and rerun the relevant checks or review steps. -5. Update the plan after each iteration. -6. Repeat until outcomes are acceptable or only explicit exceptions remain. -7. If a dependency is missing, bootstrap it or return `status: not_applicable` with explicit reason and fallback path. +1. Plan: analyze state, define target, constraints, risks, execution plan, validation steps. +2. Execute one step, produce concrete delta. +3. Review result, capture findings. +4. Apply fixes in small batches, rerun checks. +5. Update plan after each iteration. +6. Repeat until acceptable or only explicit exceptions remain. +7. Missing dependency: bootstrap or return `status: not_applicable`. ### Required Result Format - `status`: `complete` | `clean` | `improved` | `configured` | `not_applicable` | `blocked` -- `plan`: concise plan and current iteration step -- `actions_taken`: concrete changes made -- `validation_skills`: final skills run, or skipped with reasons -- `verification`: commands, checks, or review evidence summary -- `remaining`: top unresolved items or `none` - -For setup-only requests with no execution, return `status: configured` and exact next commands. +- `plan`: concise plan and current step +- `actions_taken`: concrete changes +- `validation_skills`: final skills run or skipped with reasons +- `verification`: commands, checks, or review evidence +- `remaining`: unresolved items or `none` ## Load References -- read `references/rules.md` for SDK analyzer rule categories and severity guidance -- read `references/config.md` for AnalysisLevel, AnalysisMode, and .editorconfig settings +- `references/rules.md` — rule categories and severity guidance +- `references/config.md` — MSBuild properties and .editorconfig settings ## Example Requests - "Turn on built-in .NET analyzers." - "Make analyzer warnings fail the build." -- "Set the right `AnalysisLevel` for this repo." +- "Set the right AnalysisLevel for this repo." +- "Start treating unused usings and unused variables as errors." +- "Help me gradually promote Roslyn warnings in my legacy project." +- "Which warnings should I promote to errors next?" diff --git a/skills/dotnet-code-analysis/references/config.md b/skills/dotnet-code-analysis/references/config.md index 4db511f..8b952a7 100644 --- a/skills/dotnet-code-analysis/references/config.md +++ b/skills/dotnet-code-analysis/references/config.md @@ -1,214 +1,99 @@ -# AnalysisLevel and .editorconfig Configuration - -This reference covers MSBuild properties for SDK analyzers and .editorconfig settings for rule configuration. +# MSBuild Properties and .editorconfig for Code Analysis ## MSBuild Properties -Configure these in `Directory.Build.props` or individual project files. +Set in `Directory.Build.props` or project files. ### EnableNETAnalyzers ```xml - - true - +true ``` - -- Enabled by default in .NET 5+. -- Set explicitly for clarity and to prevent accidental disabling. +Default true in .NET 5+. Set explicitly to prevent accidental disabling. ### AnalysisLevel -Controls which rules are enabled based on .NET version and analysis mode. +Values: `5.0`–`10.0` (specific SDK), `latest`, `latest-recommended`, `latest-minimum`, `latest-all`, `preview`. -```xml - - latest - -``` - -| Value | Meaning | -|-------|---------| -| `5.0`, `6.0`, `7.0`, `8.0`, `9.0`, `10.0` | Rules available in that SDK version | -| `latest` | Rules from the installed SDK version | -| `latest-recommended` | Latest SDK with Recommended mode | -| `latest-minimum` | Latest SDK with Minimum mode | -| `latest-all` | Latest SDK with All mode | -| `preview` | Experimental rules from preview SDK | +Combined syntax includes mode: `latest-recommended` equals `latest` + `Recommended` mode. ### AnalysisMode -Controls which subset of rules are enabled. - -```xml - - Recommended - -``` - -| Mode | Description | -|------|-------------| -| `None` | All rules disabled except explicitly enabled | -| `Default` | Default severity for all rules (same as not setting) | -| `Minimum` | Small set of critical rules | -| `Recommended` | Common high-value rules (start here) | -| `All` | Every available rule | - -### Combined Syntax - -AnalysisLevel can include the mode: - -```xml - - latest-recommended - -``` - -This is equivalent to: - -```xml - - latest - Recommended - -``` +Values: `None` (all off), `Default`, `Minimum` (critical only), `Recommended` (start here), `All`. ### Category-Specific AnalysisMode -Override mode for specific categories: - ```xml - - latest-recommended - All - All - +latest-recommended +All +All ``` - -Available categories: `Design`, `Documentation`, `Globalization`, `Interoperability`, `Maintainability`, `Naming`, `Performance`, `Reliability`, `Security`, `Usage`. +Categories: Design, Documentation, Globalization, Interoperability, Maintainability, Naming, Performance, Reliability, Security, Usage. ### TreatWarningsAsErrors ```xml - - true - +true ``` +All warnings fail the build. Use for new projects or clean codebases. Agent rule: never disable to make a build pass. -Makes all warnings fail the build. Combine with explicit severity settings to control which rules are promoted. - -### WarningsAsErrors (Selective) +### WarningsAsErrors (selective — preferred for legacy) ```xml - - CA2000;CA3001 - +CS8019;CS0219;CS0168;CA2000;CA3001 ``` - -Promote specific warnings to errors without affecting others. +Promote specific IDs to errors. Preferred for gradual adoption: add IDs as you fix each batch. Agent rule: never remove IDs from this list to make a build pass. ### WarningsNotAsErrors ```xml - - true - CA1707 - +true +CA1707 ``` - -Keep specific warnings as warnings when using `TreatWarningsAsErrors`. +Explicit exceptions when using TreatWarningsAsErrors. ### NoWarn ```xml - - $(NoWarn);CA1062 - +$(NoWarn);CA1062 ``` - -Disable specific warnings entirely. Use sparingly; prefer .editorconfig for visibility. +Disables warnings entirely. Use sparingly; prefer .editorconfig for visibility. ### EnforceCodeStyleInBuild ```xml - - true - +true ``` +Enables IDE code style rules during build (off by default for performance). -Enable IDE code style rules during build (disabled by default for performance). - -## .editorconfig Configuration +## .editorconfig -Place at repository root. Rules cascade to subdirectories. +Place at repo root. Rules cascade to subdirectories. -### Basic Structure +### Severity per rule ```editorconfig -# Top-level settings -root = true - [*.cs] -# All C# files - -[*.{cs,vb}] -# All .NET code files - -[**/Tests/**/*.cs] -# Test files only -``` - -### Analyzer Severity Configuration - -```editorconfig -[*.cs] -# Set specific rule severity -dotnet_diagnostic.CA1000.severity = warning dotnet_diagnostic.CA2000.severity = error dotnet_diagnostic.CA1707.severity = none - -# Bulk category severity (requires .NET 6+) -dotnet_analyzer_diagnostic.category-Security.severity = error -dotnet_analyzer_diagnostic.category-Performance.severity = warning ``` -### Common Patterns - -#### Production Code Hardened +### Severity per category (.NET 6+) ```editorconfig [*.cs] -# Security rules as errors dotnet_analyzer_diagnostic.category-Security.severity = error - -# Reliability rules as warnings -dotnet_analyzer_diagnostic.category-Reliability.severity = warning - -# Performance rules as warnings dotnet_analyzer_diagnostic.category-Performance.severity = warning ``` -#### Test Code Relaxed +### Scope patterns ```editorconfig [**/Tests/**/*.cs] -[**/Test/**/*.cs] -[**/*.Tests/**/*.cs] -# Relax naming rules for test methods dotnet_diagnostic.CA1707.severity = none - -# Relax null checks for test assertions dotnet_diagnostic.CA1062.severity = none -# Allow test-specific patterns -dotnet_diagnostic.CA2007.severity = none -``` - -#### Generated Code Excluded - -```editorconfig [*.generated.cs] -[*.designer.cs] generated_code = true ``` @@ -216,21 +101,12 @@ generated_code = true ```editorconfig [*.cs] -# Namespace preferences csharp_style_namespace_declarations = file_scoped:suggestion - -# Expression-bodied members csharp_style_expression_bodied_methods = when_on_single_line:suggestion csharp_style_expression_bodied_properties = true:suggestion - -# Pattern matching csharp_style_pattern_matching_over_as_with_null_check = true:warning csharp_style_pattern_matching_over_is_with_cast_check = true:warning - -# Null checking csharp_style_prefer_null_check_over_type_check = true:suggestion - -# var preferences csharp_style_var_for_built_in_types = true:suggestion csharp_style_var_when_type_is_apparent = true:suggestion csharp_style_var_elsewhere = true:suggestion @@ -240,12 +116,9 @@ csharp_style_var_elsewhere = true:suggestion ```editorconfig [*.cs] -# Indentation indent_style = space indent_size = 4 tab_width = 4 - -# New lines csharp_new_line_before_open_brace = all csharp_new_line_before_else = true csharp_new_line_before_catch = true @@ -270,7 +143,7 @@ csharp_new_line_before_finally = true ``` -### .editorconfig (Root) +### .editorconfig (root) ```editorconfig root = true @@ -284,34 +157,24 @@ trim_trailing_whitespace = true insert_final_newline = true [*.cs] -# Security rules as errors dotnet_analyzer_diagnostic.category-Security.severity = error - -# Reliability rules as warnings, promote over time dotnet_analyzer_diagnostic.category-Reliability.severity = warning -# Relax test files [**/Tests/**/*.cs] dotnet_diagnostic.CA1707.severity = none dotnet_diagnostic.CA1062.severity = none ``` -## Verification Commands +## Verification ```bash -# Build with analyzer output dotnet build - -# Build with detailed analyzer timing dotnet build /p:ReportAnalyzer=true - -# Check effective analyzer configuration -dotnet build /v:d | grep -i "analyzer" ``` ## References -- [AnalysisLevel documentation](https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#analysislevel) -- [Code analysis configuration](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-options) -- [EditorConfig settings](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files) +- [AnalysisLevel](https://learn.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#analysislevel) +- [Configuration options](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-options) +- [EditorConfig](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files) - [Suppress warnings](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings)