Skip to content

Commit 846d632

Browse files
joshsmithxrmclaude
joshsmithxrm
and
claude
committed
feat: code scanning cleanup and Roslyn analyzers (#231)
Tune code scanning tools to reduce noise and add architectural enforcement: ## CodeQL - Disable 6 style rules that conflict with project conventions - Keep security-and-quality suite for actual security analysis - All 282 existing alerts dismissed (batch API) ## Bot Configuration - Add .github/copilot-instructions.md with ADR-based guidance - Add .gemini/config.yaml and styleguide.md for Gemini Code Assist - Focus bots on real issues: concurrency, performance, resource leaks ## Roslyn Analyzers (PPDS.Analyzers) - PPDS006: UseEarlyBoundEntities - flag string literals in QueryExpression - PPDS012: NoSyncOverAsync - flag .GetAwaiter().GetResult(), .Result, .Wait() - PPDS013: NoFireAndForgetInCtor - flag unawaited async calls in constructors ## EditorConfig - Fix generated code pattern to match subdirectories - Suppress CS8981 for lowercase enum names in generated code Closes #231 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent e317e58 commit 846d632

14 files changed

+791
-3
lines changed

‎.editorconfig‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,16 @@ csharp_new_line_before_catch = true
7070
csharp_new_line_before_finally = true
7171

7272
# Generated code - relax rules that are noisy for auto-generated files
73-
[**/Generated/*.cs]
73+
[**/Generated/**.cs]
7474
# Disable naming warnings (generated code may not follow conventions)
7575
dotnet_diagnostic.IDE1006.severity = none
7676

7777
# Disable documentation warnings (generated code has its own docs)
7878
dotnet_diagnostic.CS1591.severity = none
7979

80+
# Disable lowercase type name warnings (Power Platform uses lowercase for enums)
81+
dotnet_diagnostic.CS8981.severity = none
82+
8083
# Disable nullable warnings (generated code handles nullability its own way)
8184
dotnet_diagnostic.CS8618.severity = none
8285

‎.gemini/config.yaml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Gemini Code Assist Configuration
2+
# https://developers.google.com/gemini-code-assist/docs/customize-gemini-behavior-github
3+
4+
code_review:
5+
disable: false
6+
# Keep MEDIUM - Gemini finds real bugs (concurrency, performance, logic errors)
7+
comment_severity_threshold: MEDIUM
8+
max_review_comments: -1 # Unlimited
9+
pull_request_opened:
10+
help: false
11+
summary: true
12+
code_review: true
13+
include_drafts: true
14+
15+
# Don't need fun features for a professional SDK
16+
have_fun: false
17+
18+
# Memory for persistent context
19+
memory_config:
20+
disabled: false

‎.gemini/styleguide.md‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# PPDS Coding Standards
2+
3+
This style guide informs Gemini Code Assist about PPDS SDK architecture and patterns.
4+
5+
## Architecture (ADRs 0015, 0024, 0025, 0026)
6+
7+
### Layer Rules
8+
9+
- **CLI/TUI are THIN presentation adapters** - no business logic
10+
- **Application Services own all business logic** - located in `PPDS.Cli/Services/`
11+
- Services accept `IProgressReporter`, not `Console.WriteLine`
12+
- UIs never read/write files directly - use Application Services
13+
- Services return domain objects, presentation layers format output
14+
15+
### Shared Local State (ADR-0024)
16+
17+
All user data lives in `~/.ppds/`:
18+
- `profiles.json` - Auth profiles
19+
- `history/` - Query history
20+
- `settings.json` - User preferences
21+
22+
UIs call services for all persistent state - no direct file I/O.
23+
24+
## Dataverse Patterns
25+
26+
### Bulk Operations
27+
28+
- Use `CreateMultiple`, `UpdateMultiple`, `DeleteMultiple` - not loops with single operations
29+
- Single-record operations in loops cause 5-10x slowdown
30+
- Reference: `BulkOperationExecutor.cs`
31+
32+
### Counting Records
33+
34+
- Use FetchXML aggregate queries for counting
35+
- WRONG: Retrieve records and count in memory
36+
- CORRECT: `<fetch aggregate='true'><attribute aggregate='count'/></fetch>`
37+
38+
### CancellationToken
39+
40+
- All async methods must accept and propagate `CancellationToken`
41+
- Enables graceful shutdown on Ctrl+C
42+
- Check `cancellationToken.ThrowIfCancellationRequested()` in long loops
43+
44+
### Connection Pool
45+
46+
- Get client INSIDE parallel loops, not outside
47+
- Use `pool.GetTotalRecommendedParallelism()` as DOP ceiling
48+
- Don't hold single client across parallel iterations
49+
50+
## Concurrency
51+
52+
### Sync-over-Async
53+
54+
- NEVER use `.GetAwaiter().GetResult()` - causes deadlocks
55+
- NEVER use `.Result` or `.Wait()` on tasks
56+
- If sync context required, use `async void` event handlers
57+
58+
### Fire-and-Forget
59+
60+
- NEVER call async methods without await in constructors
61+
- Use `Loaded` events for async initialization
62+
- Race conditions occur when async completes before UI ready
63+
64+
### UI Thread Safety
65+
66+
- UI property updates must be on main thread
67+
- Use `Application.MainLoop.Invoke(() => ...)` in Terminal.Gui
68+
- Async methods resume on thread pool after await
69+
70+
### Re-entrancy
71+
72+
- Add guards for async operations triggered by UI events
73+
- User can scroll/click faster than operations complete
74+
- Pattern: `if (_isLoading) return; _isLoading = true; try { ... } finally { _isLoading = false; }`
75+
76+
## Error Handling (ADR-0026)
77+
78+
### Structured Exceptions
79+
80+
- Services throw `PpdsException` with `ErrorCode` and `UserMessage`
81+
- `ErrorCode` enables programmatic handling (retry on throttle, re-auth on expired)
82+
- `UserMessage` is safe to display - no GUIDs, stack traces, technical details
83+
84+
### User Messages
85+
86+
- WRONG: "FaultException: ErrorCode -2147204784"
87+
- CORRECT: "Your session has expired. Please log in again."
88+
89+
## Style Preferences
90+
91+
These are intentional choices - do NOT flag:
92+
93+
- `foreach` with `if` instead of LINQ `.Where()`
94+
- Explicit `if/else` instead of ternary operators
95+
- `catch (Exception ex)` at CLI command entry points
96+
97+
## Focus Areas
98+
99+
Please DO flag:
100+
101+
- Code duplication across files
102+
- Missing `CancellationToken` propagation
103+
- Sync-over-async patterns
104+
- Resource leaks (missing disposal)
105+
- Thread safety issues
106+
- API limits (e.g., TopCount > 5000)
107+
- Inefficient patterns (single-record loops, counting by retrieval)

‎.github/CODE_SCANNING.md‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
# Code Scanning Configuration
2+
3+
This document explains how code scanning tools are configured for the PPDS SDK repository.
4+
5+
## Overview
6+
7+
| Tool | Configuration | Purpose |
8+
|------|---------------|---------|
9+
| **CodeQL** | `.github/codeql/codeql-config.yml` | Security analysis (style rules disabled) |
10+
| **Copilot** | `.github/copilot-instructions.md` | PR review guidance |
11+
| **Gemini** | `.gemini/config.yaml`, `.gemini/styleguide.md` | PR review with architecture focus |
12+
| **Roslyn Analyzers** | `src/PPDS.Analyzers/` | Compile-time enforcement |
13+
14+
## CodeQL Configuration
15+
16+
CodeQL runs the `security-and-quality` query suite with style rules disabled.
17+
18+
### Disabled Rules
19+
20+
| Rule ID | Name | Why Disabled |
21+
|---------|------|--------------|
22+
| `cs/linq/missed-where` | LINQ Where suggestion | Conflicts with project style (prefer explicit foreach) |
23+
| `cs/linq/missed-select` | LINQ Select suggestion | Conflicts with project style |
24+
| `cs/missed-ternary-operator` | Ternary suggestion | Style preference, not quality |
25+
| `cs/nested-if-statements` | Nested if suggestion | Style preference, not quality |
26+
| `cs/catch-of-all-exceptions` | Generic catch clause | Intentional CLI pattern (top-level handlers) |
27+
| `cs/path-combine` | Path.Combine usage | All paths are controlled inputs in CLI context |
28+
29+
### Path Exclusions
30+
31+
Test code is excluded from analysis:
32+
- `tests/**`
33+
- `**/*Tests/**`
34+
- `**/*.Tests/**`
35+
36+
## Bot Review Assessment
37+
38+
Based on PR feedback analysis:
39+
40+
| Bot | Value | Notes |
41+
|-----|-------|-------|
42+
| **Gemini** | HIGH | Catches real bugs: concurrency, performance, logic errors |
43+
| **Copilot** | MIXED | Good for duplication, resource leaks; noisy for style |
44+
| **CodeQL** | LOW | Mostly style noise for this codebase |
45+
46+
## Roslyn Analyzers (PPDS.Analyzers)
47+
48+
Compile-time enforcement of architectural patterns. Analyzers run during build and show as warnings in IDE.
49+
50+
### Implemented Rules
51+
52+
| ID | Name | Description | Severity |
53+
|----|------|-------------|----------|
54+
| PPDS006 | UseEarlyBoundEntities | Flags string literals in QueryExpression | Warning |
55+
| PPDS012 | NoSyncOverAsync | Flags `.GetAwaiter().GetResult()`, `.Result`, `.Wait()` | Warning |
56+
| PPDS013 | NoFireAndForgetInCtor | Flags async calls in constructors without await | Warning |
57+
58+
### Planned Rules
59+
60+
| ID | Name | Description | Source |
61+
|----|------|-------------|--------|
62+
| PPDS001 | NoDirectFileIoInUi | UI layer using File.Read/Write directly | ADR-0024 |
63+
| PPDS002 | NoConsoleInServices | Service using Console.WriteLine | ADR-0015 |
64+
| PPDS003 | NoUiFrameworkInServices | Service referencing Spectre/Terminal.Gui | ADR-0025 |
65+
| PPDS004 | UseStructuredExceptions | Service throwing raw Exception | ADR-0026 |
66+
| PPDS005 | NoSdkInPresentation | CLI command calling ServiceClient directly | ADR-0015 |
67+
| PPDS007 | PoolClientInParallel | Pool client acquired outside parallel loop | ADR-0002/0005 |
68+
| PPDS008 | UseBulkOperations | Loop with single Delete/Update calls | Gemini PR#243 |
69+
| PPDS011 | PropagateCancellation | Async method not passing CancellationToken | Gemini PR#242 |
70+
71+
### Suppressing Analyzer Warnings
72+
73+
To suppress a specific warning in code:
74+
75+
```csharp
76+
#pragma warning disable PPDS006
77+
var query = new QueryExpression("customentity"); // No early-bound class available
78+
#pragma warning restore PPDS006
79+
```
80+
81+
To suppress project-wide in `.editorconfig`:
82+
83+
```ini
84+
[*.cs]
85+
dotnet_diagnostic.PPDS006.severity = none
86+
```
87+
88+
## Architecture (ADRs)
89+
90+
Bot instructions reference these Architecture Decision Records:
91+
92+
| ADR | Summary |
93+
|-----|---------|
94+
| [0015](../docs/adr/0015_APPLICATION_SERVICE_LAYER.md) | Application Services for CLI/TUI/Daemon |
95+
| [0024](../docs/adr/0024_SHARED_LOCAL_STATE.md) | Shared local state architecture |
96+
| [0025](../docs/adr/0025_UI_AGNOSTIC_PROGRESS.md) | UI-agnostic progress reporting |
97+
| [0026](../docs/adr/0026_STRUCTURED_ERROR_MODEL.md) | Structured error model |
98+
99+
## Related Issues
100+
101+
- [#231](https://github.com/joshsmithxrm/ppds-sdk/issues/231) - Tune code scanning tools to reduce noise
102+
- [#232](https://github.com/joshsmithxrm/ppds-sdk/issues/232) - ADR-0024 (style) - Prefer foreach over LINQ

‎.github/codeql/codeql-config.yml‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,30 @@
11
# CodeQL Configuration
2-
# Excludes test code from security analysis to reduce false positives.
32
# See: https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning
3+
# Query filter docs: https://github.blog/changelog/2022-08-31-code-scanning-customize-your-codeql-analysis-using-query-filters/
44

55
name: "PPDS SDK CodeQL Config"
66

77
paths-ignore:
8-
# Test projects - Path.Combine and similar alerts are false positives in test context
8+
# Test projects - excluded from security analysis
99
- tests/**
1010
- '**/*Tests/**'
1111
- '**/*.Tests/**'
1212

1313
queries:
1414
- uses: security-and-quality
15+
16+
# Disable style rules that conflict with project conventions
17+
# See .github/CODE_SCANNING.md for rationale
18+
query-filters:
19+
- exclude:
20+
id:
21+
# LINQ style suggestions - conflicts with ADR-0024 (prefer explicit foreach)
22+
- cs/linq/missed-where
23+
- cs/linq/missed-select
24+
# Style preferences - not quality issues
25+
- cs/missed-ternary-operator
26+
- cs/nested-if-statements
27+
# CLI patterns - intentional top-level exception handlers
28+
- cs/catch-of-all-exceptions
29+
# Path handling - all paths are controlled inputs in CLI context
30+
- cs/path-combine

‎.github/copilot-instructions.md‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
# Copilot Code Review Instructions
2+
3+
This file guides GitHub Copilot's code review behavior for the PPDS SDK repository.
4+
5+
## Architecture (ADRs 0015, 0024, 0025, 0026)
6+
7+
### Layer Rules
8+
9+
- CLI/TUI are THIN presentation adapters - no business logic
10+
- Application Services own all business logic
11+
- Services are UI-agnostic - no `Console.WriteLine`, no Spectre/Terminal.Gui references
12+
13+
### File I/O (ADR-0024: Shared Local State)
14+
15+
- UIs never read/write files directly
16+
- All file access through Application Services
17+
- WRONG: `File.ReadAllText` in command handler
18+
- CORRECT: `await _profileService.GetProfilesAsync()`
19+
20+
### Progress Reporting (ADR-0025: UI-Agnostic Progress)
21+
22+
- Services accept `IProgressReporter`, not write to console
23+
- UIs implement adapters for their display medium
24+
- Services return data, presentation layers format it
25+
26+
### Error Handling (ADR-0026: Structured Error Model)
27+
28+
- Services throw `PpdsException` with `ErrorCode` and `UserMessage`
29+
- Never expose technical details (GUIDs, stack traces) in `UserMessage`
30+
- UIs format errors for their medium
31+
32+
### Dataverse Patterns
33+
34+
- Use bulk APIs (`CreateMultiple`, `UpdateMultiple`) not loops with single operations
35+
- Use FetchXML aggregate for counting, not retrieve-and-count
36+
- Propagate `CancellationToken` through all async methods
37+
- Get pool client INSIDE parallel loops, not outside
38+
39+
## Style Preferences
40+
41+
Do NOT suggest:
42+
43+
- Converting `foreach` loops with `if` conditions to LINQ `.Where()` chains
44+
- Replacing `if/else` with ternary operators
45+
- "Simplifying" explicit control flow
46+
47+
These are intentional style choices per project conventions.
48+
49+
## Valuable Findings
50+
51+
DO flag:
52+
53+
- Code duplication across files
54+
- Resource leaks (missing disposal, using statements)
55+
- Missing `CancellationToken` propagation
56+
- Sync-over-async patterns (`.GetAwaiter().GetResult()`)
57+
- Thread safety issues in async code

‎PPDS.Sdk.sln‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.LiveTests.Fixtures", "
3737
EndProject
3838
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Cli.DaemonTests", "tests\PPDS.Cli.DaemonTests\PPDS.Cli.DaemonTests.csproj", "{04BEC730-FDD3-4204-A020-08092795BD03}"
3939
EndProject
40+
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "PPDS.Analyzers", "src\PPDS.Analyzers\PPDS.Analyzers.csproj", "{E7084D22-4885-4361-9BC3-208D6AAC82C8}"
41+
EndProject
4042
Global
4143
GlobalSection(SolutionConfigurationPlatforms) = preSolution
4244
Debug|Any CPU = Debug|Any CPU
@@ -227,6 +229,18 @@ Global
227229
{04BEC730-FDD3-4204-A020-08092795BD03}.Release|x64.Build.0 = Release|Any CPU
228230
{04BEC730-FDD3-4204-A020-08092795BD03}.Release|x86.ActiveCfg = Release|Any CPU
229231
{04BEC730-FDD3-4204-A020-08092795BD03}.Release|x86.Build.0 = Release|Any CPU
232+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
233+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|Any CPU.Build.0 = Debug|Any CPU
234+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x64.ActiveCfg = Debug|Any CPU
235+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x64.Build.0 = Debug|Any CPU
236+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x86.ActiveCfg = Debug|Any CPU
237+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Debug|x86.Build.0 = Debug|Any CPU
238+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|Any CPU.ActiveCfg = Release|Any CPU
239+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|Any CPU.Build.0 = Release|Any CPU
240+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x64.ActiveCfg = Release|Any CPU
241+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x64.Build.0 = Release|Any CPU
242+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x86.ActiveCfg = Release|Any CPU
243+
{E7084D22-4885-4361-9BC3-208D6AAC82C8}.Release|x86.Build.0 = Release|Any CPU
230244
EndGlobalSection
231245
GlobalSection(SolutionProperties) = preSolution
232246
HideSolutionNode = FALSE
@@ -247,5 +261,6 @@ Global
247261
{B1D38F8D-67C5-431F-BB47-BF02AE1E4AEB} = {0AB3BF05-4346-4AA6-1389-037BE0695223}
248262
{9FC294F4-9010-497A-808A-8F336AE6B5B4} = {0AB3BF05-4346-4AA6-1389-037BE0695223}
249263
{04BEC730-FDD3-4204-A020-08092795BD03} = {0AB3BF05-4346-4AA6-1389-037BE0695223}
264+
{E7084D22-4885-4361-9BC3-208D6AAC82C8} = {827E0CD3-B72D-47B6-A68D-7590B98EB39B}
250265
EndGlobalSection
251266
EndGlobal

0 commit comments

Comments
 (0)