actionable-review-format-standards

Imported from David-Li0406/meta-skill-evloving (skill-flow/data/skills-refined-agentskillos-36k/skillsmp/actionable-review-format-standards/SKILL.md). Install upstream with npx skills add David-Li0406/meta-skill-evloving --skill actionable-review-format-standards. Copyright stays with the author.

Actionable Review Format Standards

Standardized output format for code reviews ensuring consistent, actionable, and prioritized feedback across all reviewer agents.

When to Use This Skill

• Generating code review reports
• Formatting PR feedback
• Creating security audit reports
• Producing performance review outputs
• Any review output requiring severity classification

Core Principles

1. Every issue has a severity - Never leave findings unclassified
2. Every issue has a location - Always include file:line references
3. Every blocking issue has a fix - Provide code snippets for Critical/High
4. Summary before details - Lead with counts and verdicts
5. Categorize by concern - Group Security, Performance, Patterns separately

Severity Classification

Severity Levels

Severity Decision Tree

Is it a security vulnerability?
├── Yes → CRITICAL
└── No → Can it cause data loss or corruption?
         ├── Yes → CRITICAL
         └── No → Can it cause system crash/downtime?
                  ├── Yes → HIGH
                  └── No → Does it break functionality?
                           ├── Yes → HIGH
                           └── No → Does it affect performance significantly?
                                    ├── Yes → MEDIUM
                                    └── No → Is it a code quality issue?
                                             ├── Yes → MEDIUM/LOW
                                             └── No → LOW/INFO

Severity Examples

🔴 CRITICAL - Security
- SQL injection vulnerability
- Missing authorization on delete endpoint
- Hardcoded credentials in source code
- PII exposure in logs

🟠 HIGH - Must Fix
- Missing null checks causing NullReferenceException
- N+1 query in frequently called method
- Business logic error causing wrong calculations
- Missing input validation on public API

🟡 MEDIUM - Should Fix
- Blocking async call (.Result, .Wait())
- Missing error handling
- Inefficient LINQ query
- Duplicate code that should be extracted

🟢 LOW - Nice to Have
- Variable naming improvements
- Missing XML documentation
- Code formatting inconsistencies
- Minor refactoring opportunities

💡 INFO - Educational
- Alternative pattern suggestion
- Performance optimization tip
- Best practice recommendation

Location Format

Standard Format

{FilePath}:{LineNumber}

Examples

✅ Good:
- `src/Application/PatientAppService.cs:45`
- `src/Domain/Patient.cs:23-28` (range)
- `src/Application/Validators/CreatePatientDtoValidator.cs:12`

❌ Bad:
- `PatientAppService.cs` (missing path)
- `line 45` (missing file)
- `src/Application/` (missing file and line)

Multi-Location Issues

When an issue spans multiple files:

**[MEDIUM]** Duplicate validation logic
- `src/Application/PatientAppService.cs:45`
- `src/Application/DoctorAppService.cs:52`
- `src/Application/AppointmentAppService.cs:38`

**Suggestion**: Extract to shared `ValidationHelper` class.

Issue Format

Single Issue Template

**[{SEVERITY}]** `{file:line}` - {Category}

{Brief description of the issue}

**Problem**:
```{language}
// Current code
{problematic code}

Fix:

// Suggested fix
{corrected code}

Why: {Explanation of impact/risk}

### Compact Issue Format (for tables)

```markdown
| Severity | Location | Category | Issue | Fix |
|----------|----------|----------|-------|-----|
| 🔴 CRITICAL | `File.cs:42` | Security | Missing `[Authorize]` | Add `[Authorize(Permissions.Delete)]` |
| 🟠 HIGH | `File.cs:67` | Performance | N+1 query in loop | Use `.Include()` or batch query |

Report Structure

Full Review Report Template

# Code Review: {PR Title}

**Date**: {YYYY-MM-DD}
**Reviewer**: {agent-name}
**Files Reviewed**: {count}
**Lines Changed**: +{added} / -{removed}

---

## Verdict

{✅ APPROVE | 💬 APPROVE WITH COMMENTS | 🔄 REQUEST CHANGES}

**Summary**: {1-2 sentence overview}

---

## Issue Summary

| Severity | Count | Blocking |
|----------|-------|----------|
| 🔴 CRITICAL | {n} | Yes |
| 🟠 HIGH | {n} | Yes |
| 🟡 MEDIUM | {n} | No |
| 🟢 LOW | {n} | No |

---

## 🔴 Critical Issues

{If none: "No critical issues found."}

### [CRITICAL] `{file:line}` - {Title}

{Description}

**Problem**:
```{lang}
{code}

Fix:

{code}

🟠 High Issues

{Issues in same format}

🟡 Medium Issues

{Issues in same format or table format for brevity}

🟢 Low Issues / Suggestions

• {file:line} [nit]: {suggestion}
• {file:line} [style]: {suggestion}

🔒 Security Summary

⚡ Performance Summary

✅ What's Good

• {Positive observation 1}
• {Positive observation 2}
• {Positive observation 3}

Action Items

Must fix before merge:

• {Critical/High issue 1}
• {Critical/High issue 2}

Should fix soon:

• {Medium issue 1}
• {Medium issue 2}

Technical Debt Noted

• {Future improvement 1}
• {Future improvement 2}

---

## Category Labels

Use consistent category labels to classify issues:

| Category | Description | Examples |
|----------|-------------|----------|
| **Security** | Vulnerabilities, auth issues | Missing auth, SQL injection, XSS |
| **Performance** | Efficiency issues | N+1, blocking async, missing pagination |
| **DDD** | Domain design issues | Public setters, anemic entities |
| **ABP** | Framework pattern violations | Wrong base class, missing GuidGenerator |
| **Validation** | Input validation issues | Missing validators, weak rules |
| **Error Handling** | Exception handling issues | Silent catch, wrong exception type |
| **Async** | Async/await issues | Blocking calls, missing cancellation |
| **Testing** | Test quality issues | Missing tests, flaky tests |
| **Style** | Code style issues | Naming, formatting |
| **Documentation** | Doc issues | Missing comments, outdated docs |

---

## Feedback Language

### Use Constructive Language

```markdown
❌ Bad:
"This is wrong."
"You should know better."
"Why didn't you use X?"

✅ Good:
"Consider using X because..."
"This could cause Y. Here's a fix:"
"Have you considered X? It would improve Y."

Differentiate Blocking vs Non-Blocking

🚫 [blocking]: Must fix before merge
💭 [suggestion]: Consider for improvement
📝 [nit]: Minor style preference, not blocking
📚 [learning]: Educational note, no action needed

Quick Reference

Minimum Requirements

Every review output MUST include:

1. Verdict - Approve/Request Changes
2. Issue count by severity
3. All Critical/High issues with fixes
4. File:line references for all issues
5. At least one positive observation

Severity Quick Guide

Example Output

# Code Review: Add Patient CRUD API

**Date**: 2025-12-13
**Reviewer**: abp-code-reviewer
**Files Reviewed**: 5
**Lines Changed**: +245 / -12

---

## Verdict

🔄 REQUEST CHANGES

**Summary**: Good implementation of Patient CRUD with proper ABP patterns. Found 1 critical security issue (missing authorization) and 2 performance concerns that need attention.

---

## Issue Summary

| Severity | Count | Blocking |
|----------|-------|----------|
| 🔴 CRITICAL | 1 | Yes |
| 🟠 HIGH | 2 | Yes |
| 🟡 MEDIUM | 1 | No |
| 🟢 LOW | 2 | No |

---

## 🔴 Critical Issues

### [CRITICAL] `src/Application/PatientAppService.cs:67` - Security

**Missing authorization on DeleteAsync**

**Problem**:
```csharp
public async Task DeleteAsync(Guid id)
{
    await _repository.DeleteAsync(id);
}

Fix:

[Authorize(ClinicManagementSystemPermissions.Patients.Delete)]
public async Task DeleteAsync(Guid id)
{
    await _repository.DeleteAsync(id);
}

Why: Any authenticated user can delete patients without permission check.

🟠 High Issues

[HIGH] src/Application/PatientAppService.cs:34 - Performance

N+1 query pattern in GetListAsync

Problem:

foreach (var patient in patients)
{
    patient.Appointments = await _appointmentRepository.GetListAsync(a => a.PatientId == patient.Id);
}

Fix:

var patientIds = patients.Select(p => p.Id).ToList();
var appointments = await _appointmentRepository.GetListAsync(a => patientIds.Contains(a.PatientId));
var grouped = appointments.GroupBy(a => a.PatientId).ToDictionary(g => g.Key, g => g.ToList());
foreach (var patient in patients)
{
    patient.Appointments = grouped.GetValueOrDefault(patient.Id, new List<Appointment>());
}

🔒 Security Summary

⚡ Performance Summary

✅ What's Good

• Excellent entity encapsulation with private setters
• Proper use of GuidGenerator.Create()
• Clean FluentValidation implementation
• Good separation of concerns

Action Items

Must fix before merge:

• Add [Authorize] to DeleteAsync
• Fix N+1 query in GetListAsync

Should fix soon:

• Add XML documentation to public methods