Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions .claude/commands/pre-pr.md
Original file line number Diff line number Diff line change
@@ -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
63 changes: 63 additions & 0 deletions .claude/commands/review-bot-comments.md
Original file line number Diff line number Diff line change
@@ -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
85 changes: 85 additions & 0 deletions .claude/hooks/pre-commit-validate.py
Original file line number Diff line number Diff line change
@@ -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()
16 changes: 16 additions & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"hooks": {
"PreToolUse": [
{
"matcher": "Bash",
"hooks": [
{
"type": "command",
"command": "python \"$CLAUDE_PROJECT_DIR/.claude/hooks/pre-commit-validate.py\"",
"timeout": 120
}
]
}
]
}
}
3 changes: 1 addition & 2 deletions .claude/settings.local.example.json
Original file line number Diff line number Diff line change
Expand Up @@ -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:*)",
Expand Down
109 changes: 61 additions & 48 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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.

---

Expand Down Expand Up @@ -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.

---

Expand Down
Loading