diff --git a/.editorconfig b/.editorconfig index b819dc0d..635657db 100644 --- a/.editorconfig +++ b/.editorconfig @@ -70,13 +70,16 @@ csharp_new_line_before_catch = true csharp_new_line_before_finally = true # Generated code - relax rules that are noisy for auto-generated files -[**/Generated/*.cs] +[**/Generated/**.cs] # Disable naming warnings (generated code may not follow conventions) dotnet_diagnostic.IDE1006.severity = none # Disable documentation warnings (generated code has its own docs) dotnet_diagnostic.CS1591.severity = none +# Disable lowercase type name warnings (Power Platform uses lowercase for enums) +dotnet_diagnostic.CS8981.severity = none + # Disable nullable warnings (generated code handles nullability its own way) dotnet_diagnostic.CS8618.severity = none diff --git a/.gemini/config.yaml b/.gemini/config.yaml new file mode 100644 index 00000000..d6177eac --- /dev/null +++ b/.gemini/config.yaml @@ -0,0 +1,20 @@ +# Gemini Code Assist Configuration +# https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github + +code_review: + disable: false + # Keep MEDIUM - Gemini finds real bugs (concurrency, performance, logic errors) + comment_severity_threshold: MEDIUM + max_review_comments: -1 # Unlimited + pull_request_opened: + help: false + summary: true + code_review: true + include_drafts: true + +# Don't need fun features for a professional SDK +have_fun: false + +# Memory for persistent context +memory_config: + disabled: false diff --git a/.gemini/styleguide.md b/.gemini/styleguide.md new file mode 100644 index 00000000..e4639ffa --- /dev/null +++ b/.gemini/styleguide.md @@ -0,0 +1,107 @@ +# PPDS Coding Standards + +This style guide informs Gemini Code Assist about PPDS SDK architecture and patterns. + +## Architecture (ADRs 0015, 0024, 0025, 0026) + +### Layer Rules + +- **CLI/TUI are THIN presentation adapters** - no business logic +- **Application Services own all business logic** - located in `PPDS.Cli/Services/` +- Services accept `IProgressReporter`, not `Console.WriteLine` +- UIs never read/write files directly - use Application Services +- Services return domain objects, presentation layers format output + +### Shared Local State (ADR-0024) + +All user data lives in `~/.ppds/`: +- `profiles.json` - Auth profiles +- `history/` - Query history +- `settings.json` - User preferences + +UIs call services for all persistent state - no direct file I/O. + +## Dataverse Patterns + +### Bulk Operations + +- Use `CreateMultiple`, `UpdateMultiple`, `DeleteMultiple` - not loops with single operations +- Single-record operations in loops cause 5-10x slowdown +- Reference: `BulkOperationExecutor.cs` + +### Counting Records + +- Use FetchXML aggregate queries for counting +- WRONG: Retrieve records and count in memory +- CORRECT: `` + +### CancellationToken + +- All async methods must accept and propagate `CancellationToken` +- Enables graceful shutdown on Ctrl+C +- Check `cancellationToken.ThrowIfCancellationRequested()` in long loops + +### Connection Pool + +- Get client INSIDE parallel loops, not outside +- Use `pool.GetTotalRecommendedParallelism()` as DOP ceiling +- Don't hold single client across parallel iterations + +## Concurrency + +### Sync-over-Async + +- NEVER use `.GetAwaiter().GetResult()` - causes deadlocks +- NEVER use `.Result` or `.Wait()` on tasks +- If sync context required, use `async void` event handlers + +### Fire-and-Forget + +- NEVER call async methods without await in constructors +- Use `Loaded` events for async initialization +- Race conditions occur when async completes before UI ready + +### UI Thread Safety + +- UI property updates must be on main thread +- Use `Application.MainLoop.Invoke(() => ...)` in Terminal.Gui +- Async methods resume on thread pool after await + +### Re-entrancy + +- Add guards for async operations triggered by UI events +- User can scroll/click faster than operations complete +- Pattern: `if (_isLoading) return; _isLoading = true; try { ... } finally { _isLoading = false; }` + +## Error Handling (ADR-0026) + +### Structured Exceptions + +- Services throw `PpdsException` with `ErrorCode` and `UserMessage` +- `ErrorCode` enables programmatic handling (retry on throttle, re-auth on expired) +- `UserMessage` is safe to display - no GUIDs, stack traces, technical details + +### User Messages + +- WRONG: "FaultException: ErrorCode -2147204784" +- CORRECT: "Your session has expired. Please log in again." + +## Style Preferences + +These are intentional choices - do NOT flag: + +- `foreach` with `if` instead of LINQ `.Where()` +- Explicit `if/else` instead of ternary operators +- `catch (Exception ex)` at CLI command entry points + +## Focus Areas + +Please DO flag: + +- Code duplication across files +- Missing `CancellationToken` propagation +- Sync-over-async patterns +- Resource leaks (missing disposal) +- Thread safety issues +- API limits (e.g., TopCount > 5000) +- Inefficient patterns (single-record loops, counting by retrieval) diff --git a/.github/CODE_SCANNING.md b/.github/CODE_SCANNING.md new file mode 100644 index 00000000..eb8ff31b --- /dev/null +++ b/.github/CODE_SCANNING.md @@ -0,0 +1,134 @@ +# Code Scanning Configuration + +This document explains how code scanning tools are configured for the PPDS SDK repository. + +## Overview + +| Tool | Configuration | Purpose | +|------|---------------|---------| +| **CodeQL** | `.github/codeql/codeql-config.yml` | Security analysis (style rules disabled) | +| **Copilot** | `.github/copilot-instructions.md` | PR review guidance | +| **Gemini** | `.gemini/config.yaml`, `.gemini/styleguide.md` | PR review with architecture focus | +| **Roslyn Analyzers** | `src/PPDS.Analyzers/` | Compile-time enforcement | + +## CodeQL Configuration + +CodeQL runs the `security-and-quality` query suite with style rules disabled. + +### Disabled Rules + +| Rule ID | Name | Why Disabled | +|---------|------|--------------| +| `cs/linq/missed-where` | LINQ Where suggestion | Conflicts with project style (prefer explicit foreach) | +| `cs/linq/missed-select` | LINQ Select suggestion | Conflicts with project style | +| `cs/missed-ternary-operator` | Ternary suggestion | Style preference, not quality | +| `cs/nested-if-statements` | Nested if suggestion | Style preference, not quality | +| `cs/catch-of-all-exceptions` | Generic catch clause | Intentional CLI pattern (top-level handlers) | +| `cs/path-combine` | Path.Combine usage | All paths are controlled inputs in CLI context | + +### Path Exclusions + +Test code is excluded from analysis: +- `tests/**` +- `**/*Tests/**` +- `**/*.Tests/**` + +## Bot Review Assessment + +Based on PR feedback analysis: + +| Bot | Value | Notes | +|-----|-------|-------| +| **Gemini** | HIGH | Catches real bugs: concurrency, performance, logic errors | +| **Copilot** | MIXED | Good for duplication, resource leaks; noisy for style | +| **CodeQL** | LOW | Mostly style noise for this codebase | + +## Roslyn Analyzers (PPDS.Analyzers) + +Compile-time enforcement of architectural patterns. Analyzers run during build and show as warnings in IDE. + +### Implemented Rules + +| ID | Name | Description | Severity | +|----|------|-------------|----------| +| PPDS006 | UseEarlyBoundEntities | Flags string literals in QueryExpression | Warning | +| PPDS012 | NoSyncOverAsync | Flags `.GetAwaiter().GetResult()`, `.Result`, `.Wait()` | Warning | +| PPDS013 | NoFireAndForgetInCtor | Flags async calls in constructors without await | Warning | + +### Planned Rules + +| ID | Name | Description | Source | +|----|------|-------------|--------| +| PPDS001 | NoDirectFileIoInUi | UI layer using File.Read/Write directly | ADR-0024 | +| PPDS002 | NoConsoleInServices | Service using Console.WriteLine | ADR-0015 | +| PPDS003 | NoUiFrameworkInServices | Service referencing Spectre/Terminal.Gui | ADR-0025 | +| PPDS004 | UseStructuredExceptions | Service throwing raw Exception | ADR-0026 | +| PPDS005 | NoSdkInPresentation | CLI command calling ServiceClient directly | ADR-0015 | +| PPDS007 | PoolClientInParallel | Pool client acquired outside parallel loop | ADR-0002/0005 | +| PPDS008 | UseBulkOperations | Loop with single Delete/Update calls | Gemini PR#243 | +| PPDS011 | PropagateCancellation | Async method not passing CancellationToken | Gemini PR#242 | + +### Suppressing Analyzer Warnings + +To suppress a specific warning in code: + +```csharp +#pragma warning disable PPDS006 +var query = new QueryExpression("customentity"); // No early-bound class available +#pragma warning restore PPDS006 +``` + +To suppress project-wide in `.editorconfig`: + +```ini +[*.cs] +dotnet_diagnostic.PPDS006.severity = none +``` + +### Triage Process for Analyzer Findings + +When an analyzer flags code, follow this decision tree: + +1. **Is this a real bug?** + - YES → Create GitHub issue, fix the code + - NO → Continue to step 2 + +2. **Is it a framework/API constraint?** + - YES → Suppress inline with WHY comment + - NO → Continue to step 3 + +3. **Can code be refactored to avoid the pattern?** + - YES → Refactor (no suppression needed) + - NO → Suppress inline with WHY comment + +### Known Safe Patterns + +| Pattern | Why Safe | Example | +|---------|----------|---------| +| Sync disposal in Terminal.Gui | Framework requires sync `Application.Run()` | `PpdsApplication.Dispose()` | +| DI factory sync-over-async | Factory delegates cannot be async | `ServiceRegistration.cs` | +| Fire-and-forget with `.ContinueWith()` | Errors are explicitly handled | `SqlQueryScreen` constructor | + +### Creating GitHub Issues for Real Bugs + +If a finding reveals a real bug: +1. Create issue with `bug` label +2. Reference the analyzer rule (e.g., "Found by PPDS012") +3. Include the file and line number + +## Architecture (ADRs) + +Bot instructions reference these Architecture Decision Records: + +| ADR | Summary | +|-----|---------| +| [0015](../docs/adr/0015_APPLICATION_SERVICE_LAYER.md) | Application Services for CLI/TUI/Daemon | +| [0024](../docs/adr/0024_SHARED_LOCAL_STATE.md) | Shared local state architecture | +| [0025](../docs/adr/0025_UI_AGNOSTIC_PROGRESS.md) | UI-agnostic progress reporting | +| [0026](../docs/adr/0026_STRUCTURED_ERROR_MODEL.md) | Structured error model | + +## Related Issues + +- [#231](https://github.com/joshsmithxrm/ppds-sdk/issues/231) - Tune code scanning tools to reduce noise +- [#232](https://github.com/joshsmithxrm/ppds-sdk/issues/232) - ADR-0024 (style) - Prefer foreach over LINQ +- [#246](https://github.com/joshsmithxrm/ppds-sdk/issues/246) - Analyzer triage process and PPDS013 refinement diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml index 46084714..ad687734 100644 --- a/.github/codeql/codeql-config.yml +++ b/.github/codeql/codeql-config.yml @@ -1,14 +1,30 @@ # CodeQL Configuration -# Excludes test code from security analysis to reduce false positives. # See: https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning +# Query filter docs: https://github.blog/changelog/2022-08-31-code-scanning-customize-your-codeql-analysis-using-query-filters/ name: "PPDS SDK CodeQL Config" paths-ignore: - # Test projects - Path.Combine and similar alerts are false positives in test context + # Test projects - excluded from security analysis - tests/** - '**/*Tests/**' - '**/*.Tests/**' queries: - uses: security-and-quality + +# Disable style rules that conflict with project conventions +# See .github/CODE_SCANNING.md for rationale +query-filters: + - exclude: + id: + # LINQ style suggestions - conflicts with ADR-0024 (prefer explicit foreach) + - cs/linq/missed-where + - cs/linq/missed-select + # Style preferences - not quality issues + - cs/missed-ternary-operator + - cs/nested-if-statements + # CLI patterns - intentional top-level exception handlers + - cs/catch-of-all-exceptions + # Path handling - all paths are controlled inputs in CLI context + - cs/path-combine diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..66cae00a --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,57 @@ +# Copilot Code Review Instructions + +This file guides GitHub Copilot's code review behavior for the PPDS SDK repository. + +## Architecture (ADRs 0015, 0024, 0025, 0026) + +### Layer Rules + +- CLI/TUI are THIN presentation adapters - no business logic +- Application Services own all business logic +- Services are UI-agnostic - no `Console.WriteLine`, no Spectre/Terminal.Gui references + +### File I/O (ADR-0024: Shared Local State) + +- UIs never read/write files directly +- All file access through Application Services +- WRONG: `File.ReadAllText` in command handler +- CORRECT: `await _profileService.GetProfilesAsync()` + +### Progress Reporting (ADR-0025: UI-Agnostic Progress) + +- Services accept `IProgressReporter`, not write to console +- UIs implement adapters for their display medium +- Services return data, presentation layers format it + +### Error Handling (ADR-0026: Structured Error Model) + +- Services throw `PpdsException` with `ErrorCode` and `UserMessage` +- Never expose technical details (GUIDs, stack traces) in `UserMessage` +- UIs format errors for their medium + +### Dataverse Patterns + +- Use bulk APIs (`CreateMultiple`, `UpdateMultiple`) not loops with single operations +- Use FetchXML aggregate for counting, not retrieve-and-count +- Propagate `CancellationToken` through all async methods +- Get pool client INSIDE parallel loops, not outside + +## Style Preferences + +Do NOT suggest: + +- Converting `foreach` loops with `if` conditions to LINQ `.Where()` chains +- Replacing `if/else` with ternary operators +- "Simplifying" explicit control flow + +These are intentional style choices per project conventions. + +## Valuable Findings + +DO flag: + +- Code duplication across files +- Resource leaks (missing disposal, using statements) +- Missing `CancellationToken` propagation +- Sync-over-async patterns (`.GetAwaiter().GetResult()`) +- Thread safety issues in async code diff --git a/PPDS.Sdk.sln b/PPDS.Sdk.sln index 5485da4c..c93bdd72 100644 --- a/PPDS.Sdk.sln +++ b/PPDS.Sdk.sln @@ -37,6 +37,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.LiveTests.Fixtures", " EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Cli.DaemonTests", "tests\PPDS.Cli.DaemonTests\PPDS.Cli.DaemonTests.csproj", "{04BEC730-FDD3-4204-A020-08092795BD03}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Analyzers", "src\PPDS.Analyzers\PPDS.Analyzers.csproj", "{E7084D22-4885-4361-9BC3-208D6AAC82C8}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -227,6 +229,18 @@ Global {04BEC730-FDD3-4204-A020-08092795BD03}.Release|x64.Build.0 = Release|Any CPU {04BEC730-FDD3-4204-A020-08092795BD03}.Release|x86.ActiveCfg = Release|Any CPU {04BEC730-FDD3-4204-A020-08092795BD03}.Release|x86.Build.0 = Release|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|Any CPU.Build.0 = Debug|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x64.ActiveCfg = Debug|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x64.Build.0 = Debug|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x86.ActiveCfg = Debug|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x86.Build.0 = Debug|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|Any CPU.ActiveCfg = Release|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|Any CPU.Build.0 = Release|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x64.ActiveCfg = Release|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x64.Build.0 = Release|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x86.ActiveCfg = Release|Any CPU + {E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -247,5 +261,6 @@ Global {B1D38F8D-67C5-431F-BB47-BF02AE1E4AEB} = {0AB3BF05-4346-4AA6-1389-037BE0695223} {9FC294F4-9010-497A-808A-8F336AE6B5B4} = {0AB3BF05-4346-4AA6-1389-037BE0695223} {04BEC730-FDD3-4204-A020-08092795BD03} = {0AB3BF05-4346-4AA6-1389-037BE0695223} + {E7084D22-4885-4361-9BC3-208D6AAC82C8} = {827E0CD3-B72D-47B6-A68D-7590B98EB39B} EndGlobalSection EndGlobal diff --git a/src/PPDS.Analyzers/DiagnosticIds.cs b/src/PPDS.Analyzers/DiagnosticIds.cs new file mode 100644 index 00000000..5ed44573 --- /dev/null +++ b/src/PPDS.Analyzers/DiagnosticIds.cs @@ -0,0 +1,36 @@ +namespace PPDS.Analyzers; + +/// +/// Diagnostic IDs for PPDS analyzers. +/// See .github/CODE_SCANNING.md for rationale on each rule. +/// +public static class DiagnosticIds +{ + // Architectural rules (from ADRs 0015/0024/0025/0026) + public const string NoDirectFileIoInUi = "PPDS001"; + public const string NoConsoleInServices = "PPDS002"; + public const string NoUiFrameworkInServices = "PPDS003"; + public const string UseStructuredExceptions = "PPDS004"; + public const string NoSdkInPresentation = "PPDS005"; + public const string UseEarlyBoundEntities = "PPDS006"; + public const string PoolClientInParallel = "PPDS007"; + + // Performance/Correctness rules (from PR bot feedback) + public const string UseBulkOperations = "PPDS008"; + public const string UseAggregateForCount = "PPDS009"; + public const string ValidateTopCount = "PPDS010"; + public const string PropagateCancellation = "PPDS011"; + public const string NoSyncOverAsync = "PPDS012"; + public const string NoFireAndForgetInCtor = "PPDS013"; +} + +/// +/// Diagnostic categories for PPDS analyzers. +/// +public static class DiagnosticCategories +{ + public const string Architecture = "PPDS.Architecture"; + public const string Performance = "PPDS.Performance"; + public const string Correctness = "PPDS.Correctness"; + public const string Style = "PPDS.Style"; +} diff --git a/src/PPDS.Analyzers/PPDS.Analyzers.csproj b/src/PPDS.Analyzers/PPDS.Analyzers.csproj new file mode 100644 index 00000000..c4c010f7 --- /dev/null +++ b/src/PPDS.Analyzers/PPDS.Analyzers.csproj @@ -0,0 +1,40 @@ + + + + netstandard2.0 + latest + enable + enable + + + true + false + true + true + + $(NoWarn);RS2008 + + + PPDS.Analyzers + 1.0.0 + PPDS Team + Roslyn analyzers for PPDS SDK architectural enforcement + analyzers;roslyn;ppds + true + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + diff --git a/src/PPDS.Analyzers/Rules/NoFireAndForgetInCtorAnalyzer.cs b/src/PPDS.Analyzers/Rules/NoFireAndForgetInCtorAnalyzer.cs new file mode 100644 index 00000000..e31be11c --- /dev/null +++ b/src/PPDS.Analyzers/Rules/NoFireAndForgetInCtorAnalyzer.cs @@ -0,0 +1,180 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace PPDS.Analyzers.Rules; + +/// +/// PPDS013: Detects async method calls in constructors without await. +/// Fire-and-forget in constructors causes race conditions. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class NoFireAndForgetInCtorAnalyzer : DiagnosticAnalyzer +{ + private static readonly DiagnosticDescriptor Rule = new( + id: DiagnosticIds.NoFireAndForgetInCtor, + title: "Avoid fire-and-forget async in constructors", + messageFormat: "Async method '{0}' called in constructor without await; use Loaded event for async initialization", + category: DiagnosticCategories.Correctness, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "Calling async methods in constructors without await causes race conditions. " + + "The async operation may complete before the UI is ready. " + + "Use Loaded events or factory methods for async initialization. " + + "See Gemini PR#242 feedback."); + + public override ImmutableArray SupportedDiagnostics => + ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeConstructor, SyntaxKind.ConstructorDeclaration); + } + + private static void AnalyzeConstructor(SyntaxNodeAnalysisContext context) + { + var constructor = (ConstructorDeclarationSyntax)context.Node; + + if (constructor.Body is null && constructor.ExpressionBody is null) + return; + + // Find all invocation expressions in the constructor + var invocations = constructor.DescendantNodes().OfType(); + + foreach (var invocation in invocations) + { + // Skip if this invocation is already awaited + if (IsAwaited(invocation)) + continue; + + // Check if the method returns a Task + var symbolInfo = context.SemanticModel.GetSymbolInfo(invocation, context.CancellationToken); + if (symbolInfo.Symbol is not IMethodSymbol methodSymbol) + continue; + + if (!ReturnsTask(methodSymbol)) + continue; + + // Skip common non-async patterns that return Task + if (IsCommonSafePattern(methodSymbol)) + continue; + + // Skip if the task has .ContinueWith() error handling attached + if (HasContinueWithErrorHandling(invocation)) + continue; + + var methodName = GetMethodName(invocation); + + var diagnostic = Diagnostic.Create( + Rule, + invocation.GetLocation(), + methodName); + + context.ReportDiagnostic(diagnostic); + } + } + + private static bool IsAwaited(InvocationExpressionSyntax invocation) + { + // Check if parent is an await expression + var parent = invocation.Parent; + + // Handle parenthesized expressions + while (parent is ParenthesizedExpressionSyntax) + { + parent = parent.Parent; + } + + return parent is AwaitExpressionSyntax; + } + + private static bool ReturnsTask(IMethodSymbol method) + { + var returnType = method.ReturnType; + + if (returnType is null) + return false; + + var typeName = returnType.Name; + var containingNamespace = returnType.ContainingNamespace?.ToDisplayString(); + + // Check for Task, Task, ValueTask, ValueTask + if (containingNamespace == "System.Threading.Tasks") + { + return typeName is "Task" or "ValueTask"; + } + + // Check for generic versions + if (returnType is INamedTypeSymbol namedType && namedType.IsGenericType) + { + var originalDef = namedType.OriginalDefinition; + var originalName = originalDef.Name; + var originalNamespace = originalDef.ContainingNamespace?.ToDisplayString(); + + if (originalNamespace == "System.Threading.Tasks") + { + return originalName is "Task" or "ValueTask"; + } + } + + return false; + } + + private static bool IsCommonSafePattern(IMethodSymbol method) + { + // Some Task-returning methods are commonly used in fire-and-forget scenarios + // and are generally safe (e.g., Task.Run for background work that's properly handled) + var methodName = method.Name; + var containingType = method.ContainingType?.Name; + + // Task.CompletedTask, Task.FromResult are safe + if (containingType == "Task" && methodName is "FromResult" or "FromException" or "FromCanceled") + return true; + + return false; + } + + private static bool HasContinueWithErrorHandling(InvocationExpressionSyntax invocation) + { + // Check if this invocation is part of a .ContinueWith() chain + // Pattern: _ = SomeAsync().ContinueWith(...) + var parent = invocation.Parent; + + // The invocation might be the expression in a member access like SomeAsync().ContinueWith + if (parent is MemberAccessExpressionSyntax memberAccess && + memberAccess.Name.Identifier.Text == "ContinueWith") + { + return true; + } + + // Also check if the entire statement is a discard assignment with ContinueWith + // Pattern: _ = SomeAsync().ContinueWith(t => { ... }); + // In this case, the invocation is SomeAsync(), and we need to check if it's + // the object of a .ContinueWith() call + if (parent is MemberAccessExpressionSyntax outerMemberAccess) + { + var grandparent = outerMemberAccess.Parent; + if (grandparent is InvocationExpressionSyntax && + outerMemberAccess.Name.Identifier.Text == "ContinueWith") + { + return true; + } + } + + return false; + } + + private static string GetMethodName(InvocationExpressionSyntax invocation) + { + return invocation.Expression switch + { + MemberAccessExpressionSyntax memberAccess => memberAccess.Name.Identifier.Text, + IdentifierNameSyntax identifier => identifier.Identifier.Text, + _ => invocation.Expression.ToString() + }; + } +} diff --git a/src/PPDS.Analyzers/Rules/NoSyncOverAsyncAnalyzer.cs b/src/PPDS.Analyzers/Rules/NoSyncOverAsyncAnalyzer.cs new file mode 100644 index 00000000..4bfa3b22 --- /dev/null +++ b/src/PPDS.Analyzers/Rules/NoSyncOverAsyncAnalyzer.cs @@ -0,0 +1,131 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace PPDS.Analyzers.Rules; + +/// +/// PPDS012: Detects sync-over-async patterns that can cause deadlocks. +/// Flags .GetAwaiter().GetResult(), .Result, and .Wait() on tasks. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class NoSyncOverAsyncAnalyzer : DiagnosticAnalyzer +{ + private static readonly DiagnosticDescriptor Rule = new( + id: DiagnosticIds.NoSyncOverAsync, + title: "Avoid sync-over-async patterns", + messageFormat: "'{0}' can cause deadlocks; use 'await' instead", + category: DiagnosticCategories.Correctness, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "Calling .GetAwaiter().GetResult(), .Result, or .Wait() on tasks can cause deadlocks. " + + "Use async/await patterns instead. See Gemini PR#242 feedback."); + + public override ImmutableArray SupportedDiagnostics => + ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeMemberAccess, SyntaxKind.SimpleMemberAccessExpression); + context.RegisterSyntaxNodeAction(AnalyzeInvocation, SyntaxKind.InvocationExpression); + } + + private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context) + { + var memberAccess = (MemberAccessExpressionSyntax)context.Node; + + // Check for .Result property access + if (memberAccess.Name.Identifier.Text != "Result") + return; + + // Verify it's on a Task type + var typeInfo = context.SemanticModel.GetTypeInfo(memberAccess.Expression, context.CancellationToken); + if (!IsTaskType(typeInfo.Type)) + return; + + var diagnostic = Diagnostic.Create( + Rule, + memberAccess.Name.GetLocation(), + ".Result"); + + context.ReportDiagnostic(diagnostic); + } + + private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context) + { + var invocation = (InvocationExpressionSyntax)context.Node; + + if (invocation.Expression is not MemberAccessExpressionSyntax memberAccess) + return; + + var methodName = memberAccess.Name.Identifier.Text; + + // Check for .Wait() call + if (methodName == "Wait") + { + var typeInfo = context.SemanticModel.GetTypeInfo(memberAccess.Expression, context.CancellationToken); + if (IsTaskType(typeInfo.Type)) + { + var diagnostic = Diagnostic.Create( + Rule, + memberAccess.Name.GetLocation(), + ".Wait()"); + + context.ReportDiagnostic(diagnostic); + return; + } + } + + // Check for .GetResult() call (from .GetAwaiter().GetResult()) + if (methodName == "GetResult" && + memberAccess.Expression is InvocationExpressionSyntax innerInvocation && + innerInvocation.Expression is MemberAccessExpressionSyntax innerMemberAccess && + innerMemberAccess.Name.Identifier.Text == "GetAwaiter") + { + var typeInfo = context.SemanticModel.GetTypeInfo(innerMemberAccess.Expression, context.CancellationToken); + if (IsTaskType(typeInfo.Type)) + { + var diagnostic = Diagnostic.Create( + Rule, + invocation.GetLocation(), + ".GetAwaiter().GetResult()"); + + context.ReportDiagnostic(diagnostic); + } + } + } + + private static bool IsTaskType(ITypeSymbol? type) + { + if (type is null) + return false; + + // Check for Task, Task, ValueTask, ValueTask + var typeName = type.Name; + var containingNamespace = type.ContainingNamespace?.ToDisplayString(); + + if (containingNamespace == "System.Threading.Tasks") + { + return typeName is "Task" or "ValueTask"; + } + + // Also check for generic versions + if (type is INamedTypeSymbol namedType && namedType.IsGenericType) + { + var originalDef = namedType.OriginalDefinition; + var originalName = originalDef.Name; + var originalNamespace = originalDef.ContainingNamespace?.ToDisplayString(); + + if (originalNamespace == "System.Threading.Tasks") + { + return originalName is "Task" or "ValueTask"; + } + } + + return false; + } +} diff --git a/src/PPDS.Analyzers/Rules/UseEarlyBoundEntitiesAnalyzer.cs b/src/PPDS.Analyzers/Rules/UseEarlyBoundEntitiesAnalyzer.cs new file mode 100644 index 00000000..dfd55490 --- /dev/null +++ b/src/PPDS.Analyzers/Rules/UseEarlyBoundEntitiesAnalyzer.cs @@ -0,0 +1,105 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace PPDS.Analyzers.Rules; + +/// +/// PPDS006: Detects string literals in QueryExpression constructor. +/// Use early-bound EntityLogicalName constants for type safety. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class UseEarlyBoundEntitiesAnalyzer : DiagnosticAnalyzer +{ + private static readonly DiagnosticDescriptor Rule = new( + id: DiagnosticIds.UseEarlyBoundEntities, + title: "Use early-bound entity constants", + messageFormat: "Use '{0}.EntityLogicalName' instead of string literal \"{1}\"", + category: DiagnosticCategories.Style, + defaultSeverity: DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "QueryExpression should use early-bound EntityLogicalName constants for compile-time safety. " + + "See CODE_SCANNING.md for details on available generated entities."); + + public override ImmutableArray SupportedDiagnostics => + ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeObjectCreation, SyntaxKind.ObjectCreationExpression); + } + + private static void AnalyzeObjectCreation(SyntaxNodeAnalysisContext context) + { + var objectCreation = (ObjectCreationExpressionSyntax)context.Node; + + // Check if it's a QueryExpression constructor using full metadata name for robustness + var typeInfo = context.SemanticModel.GetTypeInfo(objectCreation, context.CancellationToken); + var queryExpressionType = context.SemanticModel.Compilation.GetTypeByMetadataName("Microsoft.Xrm.Sdk.Query.QueryExpression"); + if (queryExpressionType is null || !SymbolEqualityComparer.Default.Equals(typeInfo.Type, queryExpressionType)) + return; + + // Check if the first argument is a string literal + var arguments = objectCreation.ArgumentList?.Arguments; + if (arguments is null || arguments.Value.Count == 0) + return; + + var firstArg = arguments.Value[0].Expression; + if (firstArg is not LiteralExpressionSyntax literal || !literal.IsKind(SyntaxKind.StringLiteralExpression)) + return; + + var entityName = literal.Token.ValueText; + + // Map known entity names to their early-bound class names + var suggestedClass = GetEarlyBoundClassName(entityName); + + var diagnostic = Diagnostic.Create( + Rule, + literal.GetLocation(), + suggestedClass, + entityName); + + context.ReportDiagnostic(diagnostic); + } + + private static string GetEarlyBoundClassName(string entityLogicalName) + { + // Map common entity names to their PascalCase class names + return entityLogicalName switch + { + "pluginassembly" => "PluginAssembly", + "pluginpackage" => "PluginPackage", + "plugintype" => "PluginType", + "sdkmessage" => "SdkMessage", + "sdkmessagefilter" => "SdkMessageFilter", + "sdkmessageprocessingstep" => "SdkMessageProcessingStep", + "sdkmessageprocessingstepimage" => "SdkMessageProcessingStepImage", + "solution" => "Solution", + "solutioncomponent" => "SolutionComponent", + "asyncoperation" => "AsyncOperation", + "importjob" => "ImportJob", + "systemuser" => "SystemUser", + "role" => "Role", + "publisher" => "Publisher", + "environmentvariabledefinition" => "EnvironmentVariableDefinition", + "environmentvariablevalue" => "EnvironmentVariableValue", + "workflow" => "Workflow", + "connectionreference" => "ConnectionReference", + "plugintracelog" => "PluginTraceLog", + _ => ToPascalCase(entityLogicalName) + }; + } + + private static string ToPascalCase(string input) + { + if (string.IsNullOrEmpty(input)) + return input; + + // Simple conversion: capitalize first letter + return char.ToUpperInvariant(input[0]) + input.Substring(1); + } +} diff --git a/src/PPDS.Cli/PPDS.Cli.csproj b/src/PPDS.Cli/PPDS.Cli.csproj index febed985..2d6a0fd9 100644 --- a/src/PPDS.Cli/PPDS.Cli.csproj +++ b/src/PPDS.Cli/PPDS.Cli.csproj @@ -57,6 +57,10 @@ + + diff --git a/src/PPDS.Cli/Services/ServiceRegistration.cs b/src/PPDS.Cli/Services/ServiceRegistration.cs index a5f46b84..6aa6abf4 100644 --- a/src/PPDS.Cli/Services/ServiceRegistration.cs +++ b/src/PPDS.Cli/Services/ServiceRegistration.cs @@ -57,7 +57,11 @@ public static IServiceCollection AddCliApplicationServices(this IServiceCollecti $"Profile '{profile.DisplayIdentifier}' is configured for ClientSecret auth but has no ApplicationId."); } + // DI factory delegates are synchronous; GetAsync is safe here because + // credential store uses file I/O, not network calls that would benefit from async. +#pragma warning disable PPDS012 // Sync-over-async: DI factory cannot be async var storedCredential = credentialStore.GetAsync(profile.ApplicationId).GetAwaiter().GetResult(); +#pragma warning restore PPDS012 if (storedCredential?.ClientSecret == null) { throw new InvalidOperationException( diff --git a/src/PPDS.Cli/Tui/PpdsApplication.cs b/src/PPDS.Cli/Tui/PpdsApplication.cs index 27dad056..4a38dd1f 100644 --- a/src/PPDS.Cli/Tui/PpdsApplication.cs +++ b/src/PPDS.Cli/Tui/PpdsApplication.cs @@ -49,7 +49,9 @@ public int Run(CancellationToken cancellationToken = default) // Note: Sync-over-async is required here because Terminal.Gui's Application.Run() // is synchronous and we need to clean up the session before returning. // The session disposal is fast (just releases pooled connections). +#pragma warning disable PPDS012 // Terminal.Gui requires sync disposal _session?.DisposeAsync().AsTask().GetAwaiter().GetResult(); +#pragma warning restore PPDS012 } } @@ -57,6 +59,8 @@ public void Dispose() { if (_disposed) return; _disposed = true; +#pragma warning disable PPDS012 // IDisposable.Dispose must be synchronous _session?.DisposeAsync().AsTask().GetAwaiter().GetResult(); +#pragma warning restore PPDS012 } } diff --git a/src/PPDS.Cli/Tui/Screens/SqlQueryScreen.cs b/src/PPDS.Cli/Tui/Screens/SqlQueryScreen.cs index 7c174cbb..6969cd62 100644 --- a/src/PPDS.Cli/Tui/Screens/SqlQueryScreen.cs +++ b/src/PPDS.Cli/Tui/Screens/SqlQueryScreen.cs @@ -105,6 +105,7 @@ public SqlQueryScreen(string? profileName, Action? deviceCodeCal Add(queryFrame, _filterFrame, _resultsTable, _statusLabel); // Load profile and environment info (fire-and-forget with error handling) +#pragma warning disable PPDS013 // Fire-and-forget with explicit error handling via ContinueWith _ = LoadProfileInfoAsync().ContinueWith(t => { if (t.IsFaulted && t.Exception != null) @@ -115,6 +116,7 @@ public SqlQueryScreen(string? profileName, Action? deviceCodeCal }); } }, TaskScheduler.Default); +#pragma warning restore PPDS013 // Set up keyboard shortcuts SetupKeyboardShortcuts();