diff --git a/.claude/commands/pre-pr.md b/.claude/commands/pre-pr.md new file mode 100644 index 000000000..cc404b15b --- /dev/null +++ b/.claude/commands/pre-pr.md @@ -0,0 +1,61 @@ +# Pre-PR Validation + +Run before creating a PR to catch issues early. + +## Usage + +`/pre-pr` + +## Checks Performed + +### 1. Build & Test +```bash +dotnet build -c Release --warnaserror +dotnet test --no-build -c Release +``` + +### 2. New Public APIs +- Check for public classes/methods without XML documentation +- Check for new public APIs without corresponding tests + +### 3. Common Issues +- Generic catch clauses (`catch { }` or `catch (Exception)` without logging) +- TODO/FIXME comments that should be issues +- Console.WriteLine in library code (should use ILogger or AuthenticationOutput) +- Dead code (unused private methods, unreachable code) + +### 4. Changelog +- Verify CHANGELOG.md in affected package(s) is updated +- If not, prompt: "Update changelog for [package]?" + +### 5. Test Coverage (New Code) +- For each new class: does corresponding test file exist? +- Prompt: "No tests for [ClassName]. Add tests?" → Generate test stubs + +## Output + +``` +Pre-PR Validation +================= +[x] Build: PASS +[x] Tests: PASS (42 passed) +[!] Missing XML docs: MsalClientBuilder.CreateClient() +[x] No TODOs found +[!] Changelog not updated for PPDS.Auth +[!] No tests for: ThrottleDetector, MsalClientBuilder + +Fix issues? [Y/n] +``` + +## Behavior + +- On first failure: stop and report +- On warnings: list all, ask whether to fix +- Auto-fix what's possible (changelog stubs, test file creation) +- Manual fix guidance for others + +## When to Use + +- Before `git commit` for significant changes +- Before `gh pr create` +- After addressing bot review comments diff --git a/.claude/commands/review-bot-comments.md b/.claude/commands/review-bot-comments.md new file mode 100644 index 000000000..5a3555b7d --- /dev/null +++ b/.claude/commands/review-bot-comments.md @@ -0,0 +1,63 @@ +# Review Bot Comments + +Systematically address PR review comments from Copilot, Gemini, and CodeQL. + +## Usage + +`/review-bot-comments [pr-number]` + +## Process + +### 1. Fetch Comments +```bash +gh api repos/joshsmithxrm/ppds-sdk/pulls/[PR]/comments +``` + +### 2. Triage Each Comment + +For each bot comment, determine: + +| Verdict | Action | +|---------|--------| +| **Valid** | Fix the issue, reply "Fixed in [commit]" | +| **False Positive** | Reply with reason, dismiss | +| **Unclear** | Investigate before deciding | + +### 3. Common False Positives + +| Bot Claim | Why It's Often Wrong | +|-----------|---------------------| +| "Use .Where() instead of foreach+if" | Preference, not correctness | +| "Volatile needed with Interlocked" | Interlocked provides barriers | +| "OR should be AND" | Logic may be intentionally inverted (DeMorgan) | +| "Static field not thread-safe" | May be set once at startup | + +### 4. Common Valid Findings + +| Pattern | Usually Valid | +|---------|---------------| +| Unused variable/parameter | Yes - dead code | +| Missing null check | Check context | +| Resource not disposed | Yes - leak | +| Generic catch clause | Context-dependent | + +## Output + +```markdown +## Bot Review Triage - PR #82 + +| # | Bot | Finding | Verdict | Action | +|---|-----|---------|---------|--------| +| 1 | Gemini | Use constants in dict | Valid | Fixed in abc123 | +| 2 | Copilot | Add validation tests | Valid | Fixed in def456 | +| 3 | Copilot | Use .Where() | False Positive | Style preference | +| 4 | CodeQL | Generic catch | Valid (low) | Acceptable for disposal | + +All findings addressed: Yes +``` + +## When to Use + +- After opening a PR (before requesting review) +- After new bot comments appear +- Before merging diff --git a/.claude/hooks/pre-commit-validate.py b/.claude/hooks/pre-commit-validate.py new file mode 100644 index 000000000..740170b13 --- /dev/null +++ b/.claude/hooks/pre-commit-validate.py @@ -0,0 +1,85 @@ +#!/usr/bin/env python3 +""" +Pre-commit validation hook for PPDS SDK. +Runs dotnet build and test before allowing git commit. +""" +import json +import shlex +import subprocess +import sys +import os + +def main(): + try: + input_data = json.load(sys.stdin) + except json.JSONDecodeError: + print("⚠️ pre-commit-validate: Failed to parse input. Skipping validation.", file=sys.stderr) + sys.exit(0) + + tool_name = input_data.get("tool_name", "") + tool_input = input_data.get("tool_input", {}) + command = tool_input.get("command", "") + + # Only validate git commit commands (robust check using shlex) + if tool_name != "Bash": + sys.exit(0) + try: + parts = shlex.split(command) + is_git_commit = len(parts) >= 2 and os.path.basename(parts[0]) == "git" and parts[1] == "commit" + except ValueError: + is_git_commit = False + if not is_git_commit: + sys.exit(0) # Allow non-commit commands + + # Get project directory + project_dir = os.environ.get("CLAUDE_PROJECT_DIR", os.getcwd()) + + try: + # Run dotnet build + print("Running pre-commit validation...", file=sys.stderr) + build_result = subprocess.run( + ["dotnet", "build", "-c", "Release", "--nologo", "-v", "q"], + cwd=project_dir, + capture_output=True, + text=True, + timeout=300 # 5 minute timeout + ) + + if build_result.returncode != 0: + print("❌ Build failed. Fix errors before committing:", file=sys.stderr) + if build_result.stdout: + print(build_result.stdout, file=sys.stderr) + if build_result.stderr: + print(build_result.stderr, file=sys.stderr) + sys.exit(2) # Block commit + + # Run dotnet test (unit tests only - integration tests run on PR) + test_result = subprocess.run( + ["dotnet", "test", "--no-build", "-c", "Release", "--nologo", "-v", "q", + "--filter", "Category!=Integration"], + cwd=project_dir, + capture_output=True, + text=True, + timeout=300 # 5 minute timeout + ) + + if test_result.returncode != 0: + print("❌ Unit tests failed. Fix before committing:", file=sys.stderr) + if test_result.stdout: + print(test_result.stdout, file=sys.stderr) + if test_result.stderr: + print(test_result.stderr, file=sys.stderr) + sys.exit(2) # Block commit + + print("✅ Build and unit tests passed", file=sys.stderr) + sys.exit(0) # Allow commit + + except FileNotFoundError: + print("⚠️ pre-commit-validate: dotnet not found in PATH. Skipping validation.", file=sys.stderr) + sys.exit(0) + except subprocess.TimeoutExpired: + print("⚠️ pre-commit-validate: Build/test timed out after 5 minutes. Skipping validation.", file=sys.stderr) + sys.exit(0) + +if __name__ == "__main__": + main() diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 000000000..436598548 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,16 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "python \"$CLAUDE_PROJECT_DIR/.claude/hooks/pre-commit-validate.py\"", + "timeout": 120 + } + ] + } + ] + } +} diff --git a/.claude/settings.local.example.json b/.claude/settings.local.example.json index 9eb2a8a71..b0136bde8 100644 --- a/.claude/settings.local.example.json +++ b/.claude/settings.local.example.json @@ -79,8 +79,7 @@ "Bash(gh pr status:*)", "Bash(gh pr checkout:*)", "Bash(gh pr diff:*)", - "Bash(gh api repos/*/pulls/*/comments:*)", - "Bash(gh api repos/*/pulls/*/comments/*:*)", + "Bash(gh api:*", "Bash(gh issue list:*)", "Bash(gh issue view:*)", "Bash(gh issue status:*)", diff --git a/CLAUDE.md b/CLAUDE.md index dfe5930bb..12d366b6e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -115,9 +115,11 @@ dotnet clean 1. Create feature branch from `main` 2. Make changes -3. Run `dotnet build` and `dotnet test` -4. Update `CHANGELOG.md` -5. Create PR to `main` +3. **Add tests for new classes** (no new code without tests) +4. Update `CHANGELOG.md` (same commit, not after) +5. Run `/pre-pr` before committing +6. Create PR to `main` +7. Run `/review-bot-comments` after bots comment ### Code Conventions @@ -144,57 +146,21 @@ public class PluginStepAttribute : Attribute { } ### Code Comments -Comments explain WHY, not WHAT. The code documents what it does. +Comments explain WHY, not WHAT. ```csharp -// ❌ Bad - explains what (the code already shows this) -// Loop through all options and check if required +// ❌ Bad - explains what +// Loop through all options foreach (var option in command.Options) -// ❌ Bad - references external tool as justification -// Use [Required] prefix like Azure CLI does -option.Description = $"[Required] {desc}"; - -// ✅ Good - explains why (non-obvious side effect) +// ✅ Good - explains why // Required=false hides the default suffix; we show [Required] in description instead option.Required = false; - -// ✅ Good - explains why (workaround for framework limitation) -// Option validators only run when the option is present on command line, -// so we need command-level validation to catch missing required options -command.Validators.Add(result => { ... }); ``` ### Namespaces -```csharp -// PPDS.Plugins -namespace PPDS.Plugins; // Root -namespace PPDS.Plugins.Attributes; // Attributes -namespace PPDS.Plugins.Enums; // Enums - -// PPDS.Dataverse -namespace PPDS.Dataverse.Pooling; // Connection pool, IConnectionSource -namespace PPDS.Dataverse.BulkOperations; // Bulk API wrappers -namespace PPDS.Dataverse.Configuration; // Options, connection config -namespace PPDS.Dataverse.Resilience; // Throttle tracking, service protection - -// PPDS.Migration -namespace PPDS.Migration.Export; // IExporter -namespace PPDS.Migration.Import; // IImporter - -// PPDS.Auth -namespace PPDS.Auth.Profiles; // AuthProfile, ProfileStore, ProfileCollection -namespace PPDS.Auth.Credentials; // ICredentialProvider, credential implementations -namespace PPDS.Auth.Discovery; // GlobalDiscoveryService, EnvironmentResolver -namespace PPDS.Auth.Cloud; // CloudEnvironment, CloudEndpoints - -// PPDS.Cli -namespace PPDS.Cli.Commands.Auth; // Auth command group -namespace PPDS.Cli.Commands.Env; // Environment command group -namespace PPDS.Cli.Commands.Data; // Data command group (export, import, copy) -namespace PPDS.Cli.Infrastructure; // ServiceFactory, ProfileServiceFactory -``` +Follow existing patterns: `PPDS.{Package}.{Area}` (e.g., `PPDS.Auth.Credentials`). Infer from code. --- @@ -411,10 +377,57 @@ See [CLI README](src/PPDS.Cli/README.md) for full documentation. ## 🧪 Testing Requirements -- **Target 80% code coverage** -- Unit tests for all public API (attributes, enums) -- Run `dotnet test` before submitting PR -- All tests must pass before merge +| Package | Test Project | Status | +|---------|--------------|--------| +| PPDS.Plugins | PPDS.Plugins.Tests | ✅ | +| PPDS.Dataverse | PPDS.Dataverse.Tests | ✅ | +| PPDS.Cli | PPDS.Cli.Tests | ✅ | +| PPDS.Auth | **Needs test project** | ❌ | +| PPDS.Migration | **Needs test project** | ❌ | + +**Rules:** +- New public class → must have corresponding test class +- New public method → must have test coverage +- Mark integration tests with `[Trait("Category", "Integration")]` + +**Test filtering:** +- **Commits:** Unit tests only (`--filter Category!=Integration`) +- **PRs:** All tests including integration + +--- + +## 🤖 Bot Review Handling + +PRs get reviewed by Copilot, Gemini, and CodeQL. **Not all findings are valid.** + +| Finding Type | Action | +|--------------|--------| +| Unused code, resource leaks, missing tests | Usually valid - fix | +| "Use .Where()", style suggestions | Often preference - dismiss with reason | +| Logic errors (OR/AND) | Verify manually - bots misread DeMorgan | + +**Workflow:** After PR created, run `/review-bot-comments [PR#]` to triage. + +--- + +## 🛠️ Claude Commands & Hooks + +### Commands + +| Command | Purpose | +|---------|---------| +| `/pre-pr` | Validate before PR (build, test, changelog) | +| `/review-bot-comments [PR#]` | Triage bot review findings | +| `/handoff` | Session summary (workspace) | +| `/create-issue [repo]` | Create issue (workspace) | + +### Hooks (Automatic) + +| Hook | Trigger | Action | +|------|---------|--------| +| `pre-commit-validate.py` | `git commit` | Build + unit tests (skips integration), blocks if failed | + +Hooks in `.claude/settings.json`. Pre-commit runs ~10s, keeps broken code out of commits. ---