-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: code scanning cleanup and Roslyn analyzers #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8af8697
feat: code scanning cleanup and Roslyn analyzers (#231)
joshsmithxrm 3d52637
fix: address bot review comments on analyzers
joshsmithxrm da82caa
fix: add analyzer suppressions for TUI code
joshsmithxrm 4243c83
chore: add analyzer triage process and refine PPDS013
joshsmithxrm 8d73174
fix: remove unused variable in PPDS013 analyzer
joshsmithxrm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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: `<fetch aggregate='true'><attribute aggregate='count'/></fetch>` | ||
|
|
||
| ### 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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.