Actionable Review Format Standards Skill

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

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

Severity Classification

Severity Levels

| Level | Icon | Criteria | Action Required | |-------|------|----------|-----------------| | CRITICAL | 🔴 | Security vulnerabilities, data loss risk, system crashes | Must fix before merge | | HIGH | 🟠 | Significant bugs, missing authorization, performance blockers | Should fix before merge | | MEDIUM | 🟡 | Code quality issues, minor bugs, missing validation | Fix soon, not blocking | | LOW | 🟢 | Style issues, minor improvements, suggestions | Nice to have | | INFO | 💡 | Educational comments, alternative approaches | No action required |

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

| Check | Status | Notes | |-------|--------|-------| | Authorization | ✅ Pass / ❌ Fail | {details} | | Input Validation | ✅ Pass / ❌ Fail | {details} | | Data Exposure | ✅ Pass / ❌ Fail | {details} | | Secrets | ✅ Pass / ❌ Fail | {details} |

⚡ Performance Summary

| Check | Status | Notes | |-------|--------|-------| | N+1 Queries | ✅ Pass / ❌ Fail | {details} | | Async Patterns | ✅ Pass / ❌ Fail | {details} | | Pagination | ✅ Pass / ❌ Fail | {details} | | Query Optimization | ✅ Pass / ❌ Fail | {details} |

✅ 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:

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

Severity Quick Guide

| If you find... | Severity | |----------------|----------| | Security vulnerability | 🔴 CRITICAL | | Missing authorization | 🔴 CRITICAL | | Data corruption risk | 🔴 CRITICAL | | Null reference exception | 🟠 HIGH | | N+1 query pattern | 🟠 HIGH | | Blocking async | 🟡 MEDIUM | | Missing validation | 🟡 MEDIUM | | Naming issues | 🟢 LOW | | Missing docs | 🟢 LOW |

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

| Check | Status | Notes | |-------|--------|-------| | Authorization | ❌ Fail | DeleteAsync missing [Authorize] | | Input Validation | ✅ Pass | FluentValidation in place | | Data Exposure | ✅ Pass | DTOs properly scoped | | Secrets | ✅ Pass | No hardcoded values |

⚡ Performance Summary

| Check | Status | Notes | |-------|--------|-------| | N+1 Queries | ❌ Fail | Loop in GetListAsync | | Async Patterns | ✅ Pass | Proper async/await | | Pagination | ✅ Pass | Using PageBy | | Query Optimization | ✅ Pass | WhereIf pattern used |

✅ 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

Agent Skills: Actionable Review Format Standards

Install this agent skill to your local

Skill Files