Skip to content

chore(analyzers): MS VS Threading analyzers (Tier 2 close-out)#99

Merged
phuongnse merged 1 commit into
mainfrom
chore/tier2-vsthrd-async-analyzers
May 24, 2026
Merged

chore(analyzers): MS VS Threading analyzers (Tier 2 close-out)#99
phuongnse merged 1 commit into
mainfrom
chore/tier2-vsthrd-async-analyzers

Conversation

@phuongnse

@phuongnse phuongnse commented May 24, 2026

Copy link
Copy Markdown
Owner

Summary

Final Tier 2 piece. Replaces the originally-planned custom Roslyn analyzer for .Result/.Wait() on Task (PR #99 thread) with Microsoft's Microsoft.VisualStudio.Threading.Analyzers package — same rule plus a dozen related async pitfalls, maintained by Microsoft across .NET versions, no custom maintenance burden.

After re-evaluating: writing a custom Roslyn analyzer (new netstandard2.0 project, Microsoft.CodeAnalysis.Testing setup, packaging quirks, ongoing maintenance) for one rule when Microsoft ships exactly that rule for free is not a good ROI tradeoff. The DiagnosticAnalyzer infrastructure also adds a ~5h initial cost we don't recover.

Baseline build is clean — no existing violations to suppress or allow-list.

What this enables

Rule Catches
VSTHRD002 .Result / .Wait() / .GetAwaiter().GetResult() on Task (sync-over-async deadlock recipe). Type-aware — no false positives from domain types named Wait / Result.
VSTHRD100 async void methods (unrecoverable exceptions).
VSTHRD110 Unobserved async return values (forgotten await).
...and ~10 other rules Async-pattern hygiene.

What this intentionally disables

Two rules suppressed in .editorconfig with justification:

  • VSTHRD200 (Async-suffix naming) — would break MediatR / Wolverine handler dispatch which binds on the literal method name Handle.
  • VSTHRD111 (ConfigureAwait(bool)) — WPF/WinForms safeguard; modern ASP.NET Core has no SynchronizationContext to deadlock against.

Linked spec

No feature — process improvement. References:

Requirements & rules followed

  • Spec → code — encodes existing CLAUDE.md "never sync-over-async" P1 rule at build time.
  • Gate 0 — N/A.
  • Gate 1dotnet build Axis.sln clean (0 errors, 0 warnings); architecture tests 322/322; dotnet format exit 0.
  • Gate 2patterns.md § Async patterns documents the analyzer and disabled rules; agent-checklist.md CI-only gates list updated.
  • Gate 3 — no durable rule beyond what CLAUDE.md / patterns.md already state.
  • Workarounds — none.
  • No new TODO / FIXME / NotImplementedException / placeholder / stub under src/, tests/, frontend/src/

This closes Tier 2 of the architecture-fitness expansion. Cumulative coverage across PRs #94-99:

Layer Catches Where
Structural (assembly refs) Cross-module references, Domain purity, Shared kernel boundaries Axis.Architecture.Tests (PR #94)
Convention (types/methods) Handler signature, repo IQueryable leak, aggregate setters, endpoint naming Axis.Architecture.Tests (PR #95)
Security Committed secrets, vulnerable deps, CVE pins CI workflow + Directory.Packages.props (PR #96)
Runtime Endpoint authorization presence Axis.Api.Tests.Architecture (PR #97)
Source text Hardcoded conn strings, DateTime.Now, raw SQL cross-module, async deadlock scripts/check-doc-drift.sh (PR #98)
Async type-aware Sync-over-async, async-void, unobserved Task This PR

Summary by CodeRabbit

  • Chores

    • Enhanced build system with additional validation rules for code quality
    • Pinned development tool dependencies for consistency
  • Documentation

    • Updated async patterns guidelines with build-time enforcement details

Review Change Stack

Replaces the originally-planned custom Roslyn analyzer (PR #99 thread)
with Microsoft's battle-tested implementation of the same rule set.
After re-evaluating, writing a custom analyzer for one rule (.Result /
.Wait() on Task with type info) wasn't a good use of time when
Microsoft ships analyzers covering that exact class of bug plus many
related async pitfalls, and maintains them across .NET versions.

Wiring:
  - Directory.Packages.props pins the package version centrally.
  - Directory.Build.props references it from every project with
    PrivateAssets=all so consumers don't transitively pull the analyzer.

Two rules intentionally disabled in .editorconfig with justification:
  - VSTHRD200 (Async suffix) — clashes with MediatR / Wolverine handler
    discovery which binds on the literal method name `Handle`. Renaming
    would break dispatch.
  - VSTHRD111 (ConfigureAwait) — WPF/WinForms safeguard; modern
    ASP.NET Core has no SynchronizationContext to deadlock against.

The remaining VSTHRD rules (002 sync-over-async, 100 async-void,
110 unobserved Task, ...) fire as errors and gate the build. Baseline
build is clean — no existing violations to suppress or allow-list.

Updates patterns.md § Async patterns to document the analyzer and the
two disabled rules. agent-checklist.md CI-only gates list includes
the new analyzer category.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 74eab833-66cb-4410-94d9-280eaabc513f

📥 Commits

Reviewing files that changed from the base of the PR and between f3ecc30 and 123d5b3.

📒 Files selected for processing (5)
  • .editorconfig
  • Directory.Build.props
  • Directory.Packages.props
  • docs/playbooks/agent-checklist.md
  • docs/playbooks/patterns.md

📝 Walkthrough

Walkthrough

This PR introduces Microsoft.VisualStudio.Threading.Analyzers to enforce async-safety patterns across the codebase. The package is centrally versioned, referenced in all projects, configured with selective rule overrides for specific false-positive cases, and documented alongside existing async guidance and CI validation criteria.

Changes

Async-Safety Analyzer Integration

Layer / File(s) Summary
Analyzer Package Dependency Setup
Directory.Packages.props, Directory.Build.props
Microsoft.VisualStudio.Threading.Analyzers version 17.14.15 is centrally pinned and added as a shared PackageReference with PrivateAssets="all" so the dependency is not transitive.
Rule Configuration and Async Patterns Documentation
.editorconfig, docs/playbooks/patterns.md, docs/playbooks/agent-checklist.md
VSTHRD200 and VSTHRD111 rules are disabled via editorconfig overrides with inline rationale. Async patterns documentation enumerates active diagnostics (VSTHRD002, VSTHRD100, VSTHRD110), explains the disabled rules, and a new agent checklist item references the analyzer coverage.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Threading woes, be gone with glee,
Async-safe as can ever be,
Analyzers stand guard at the gate,
Catching sync-over-async before too late,
Patterns documented, rules configured right,
CodeRabbit hops forward into the night!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore(analyzers): MS VS Threading analyzers (Tier 2 close-out)' directly and clearly describes the main change: adding Microsoft Visual Studio Threading analyzers as part of Tier 2 work. It is specific, concise, and accurately summarizes the primary purpose of the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/tier2-vsthrd-async-analyzers

Comment @coderabbitai help to get the list of available commands and usage tips.

@phuongnse phuongnse merged commit 466584d into main May 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant