cleveragents-core/docs/development/review_playbook.md

# Review Playbook & Priority Matrix

This document provides a comprehensive guide for conducting code reviews on the
CleverAgents project. It covers focus areas, skip rules, severity classification,
SLA targets, checklists, reviewer routing, and examples of acceptable versus
blocking findings.

---

## Focus Areas and Skip Rules

### What to Review Carefully

| Area | Reason |
| ---- | ------ |
| Domain model changes (`src/cleveragents/domain/`) | Core business logic; regressions break everything |
| Database migrations (`src/cleveragents/infrastructure/database/migrations/`) | Irreversible in production; data loss risk |
| Security-sensitive code (auth, secrets, server stubs) | Direct attack surface |
| CLI entry points (`src/cleveragents/cli/`) | User-facing; breaking changes affect all users |
| Public API contracts (schemas, configs) | Backward-compatibility obligations |
| Infrastructure persistence layer | Data integrity and query correctness |

### What to Skim

| Area | Reason |
| ---- | ------ |
| Auto-generated reference docs | Machine output; verify build passes instead |
| Benchmark result files (`build/asv/`) | Ephemeral artifacts; review the benchmark *code* not results |
| Lock files (`uv.lock`, `poetry.lock`) | Verify no unexpected additions; do not read line-by-line |
| Changelog entries | Quick factual check only |
| Whitelist files (`vulture_whitelist.py`) | Verify entries match real symbols; no deep review |

---

## Priority Matrix

Findings are classified into four severity levels. Every review comment **must**
include a severity tag so the author can triage efficiently.

| Severity | Label | Merge Policy | Examples |
| -------- | ----- | ------------ | -------- |
| **P0 – Critical** | `P0:blocker` | Must fix before merge | Security vulnerability, data loss risk, broken migration, credential leak |
| **P1 – High** | `P1:must-fix` | Must fix before merge | Logic bug in domain layer, missing error handling on public API, type unsafety |
| **P2 – Medium** | `P2:should-fix` | Fix in follow-up PR (within 3 days) | Missing docstring on public function, suboptimal query, minor code duplication |
| **P3 – Low** | `P3:nit` | Optional; author discretion | Style preference, naming bikeshed, TODO placement |

### Escalation Rules

- Any **P0** finding → request a second reviewer before merge.
- Two or more **P1** findings in the same subsystem → escalate to the subsystem
  owner (see routing table below).
- If a PR has only **P2** and **P3** findings, the reviewer may approve with
  comments and the author can address them in a follow-up.

---

## Review SLA Guidance

| PR Size | Target First Response | Target Approval/Request Changes |
| ------- | --------------------- | ------------------------------- |
| **S** (< 100 lines changed) | 4 hours | 8 hours |
| **M** (100–400 lines changed) | 8 hours | 24 hours |
| **L** (400–1000 lines changed) | 24 hours | 48 hours |
| **XL** (> 1000 lines changed) | 48 hours | 72 hours |

> **Note:** XL PRs should be rare. If a PR exceeds 1000 lines, consider
> splitting it into smaller, reviewable chunks.

---

## Checklist Templates

### Architecture Review Checklist

Use this checklist for PRs that modify domain models, services, or introduce
new subsystems.

- [ ] Domain model invariants are preserved
- [ ] No circular dependencies between modules
- [ ] New abstractions have clear single responsibility
- [ ] Public interfaces are documented with docstrings
- [ ] Type annotations are complete (no `Any` unless justified)
- [ ] Unit of Work boundaries are respected
- [ ] Repository pattern is followed for persistence
- [ ] No business logic in infrastructure layer
- [ ] Error types are domain-specific (not generic `Exception`)
- [ ] Changes are backward-compatible or migration path is documented

### CLI Review Checklist

Use this checklist for PRs that add or modify CLI commands.

- [ ] Command help text is clear and complete
- [ ] Exit codes follow conventions (0 = success, 1 = error)
- [ ] Output format is consistent with existing commands
- [ ] `--format json` output is parseable
- [ ] Error messages are user-friendly (no raw tracebacks)
- [ ] New commands are registered in the CLI group
- [ ] Tab completion works for new options
- [ ] Streaming output handles interrupts gracefully
- [ ] Commands are tested in behave scenarios

### DB Migration Review Checklist

Use this checklist for PRs that add or modify Alembic migrations.

- [ ] Migration has both `upgrade()` and `downgrade()` functions
- [ ] `downgrade()` is actually tested (not just a `pass`)
- [ ] No data-destructive operations without explicit confirmation
- [ ] Index additions use `IF NOT EXISTS` guard
- [ ] Column renames include data migration step
- [ ] Foreign key constraints are correct
- [ ] Migration chain is linear (no branch conflicts)
- [ ] Performance impact on large tables is documented
- [ ] Rollback procedure is documented in PR description

---

## Security-Sensitive Change Checklists

### Secrets and Credentials

- [ ] No secrets, API keys, or tokens in source code
- [ ] Secrets are loaded from environment variables or config files
- [ ] `.gitignore` covers any new secret file patterns
- [ ] No secrets logged at any log level
- [ ] Credential rotation procedure is documented

### Authentication and Authorization

- [ ] Auth checks cannot be bypassed via parameter manipulation
- [ ] Session handling follows secure defaults
- [ ] Token expiry is enforced
- [ ] Failed auth attempts are logged (without leaking credentials)
- [ ] No privilege escalation paths

### Server Stubs and API Endpoints

- [ ] Input validation on all user-supplied data
- [ ] Rate limiting is configured
- [ ] CORS policy is restrictive
- [ ] Error responses do not leak internal details
- [ ] All endpoints require authentication unless explicitly public

### Schema Migrations (Security)

- [ ] No raw SQL with string interpolation
- [ ] Migration does not grant excessive permissions
- [ ] Audit columns (`created_at`, `updated_at`) are preserved
- [ ] PII columns use appropriate encryption or hashing
- [ ] Backup procedure is documented for destructive changes

---

## PR Review Routing Table

This table defines the primary and secondary reviewers for each subsystem.
When opening a PR, assign the primary reviewer. If the primary is unavailable
within the SLA window, assign the secondary.

| Subsystem | Path Pattern | Primary Reviewer | Secondary Reviewer | Review Type |
| --------- | ------------ | ---------------- | ------------------ | ----------- |
| Domain models & services | `src/cleveragents/domain/` | Luis | Jeff | Architecture review |
| Persistence & repositories | `src/cleveragents/infrastructure/persistence/` | Luis | Hamza | Architecture review |
| CLI core commands | `src/cleveragents/cli/` | Jeff | Brent | CLI review |
| CLI tests | `features/*cli*`, `robot/*cli*` | Brent | Rui | CLI review |
| Behave tests & fixtures | `features/`, `features/steps/` | Brent | Rui | Test review |
| Robot tests & helpers | `robot/` | Brent | Rui | Test review |
| DB migrations | `src/cleveragents/infrastructure/database/migrations/` | Hamza | Luis | DB migration review |
| Actor/Skill/Tool configs | `src/cleveragents/*/actor*`, `src/cleveragents/*/skill*`, `src/cleveragents/*/tool*` | Aditya | Jeff | Architecture review |
| MCP adapter | `src/cleveragents/infrastructure/mcp/` | Aditya | Jeff | Architecture review |
| Security changes | `**/security*`, `**/auth*`, `**/secrets*` | Jeff | Brent | Security review |
| CI/CD changes | `.forgejo/`, `noxfile.py`, `pyproject.toml` | Brent | Jeff | CI review |
| Documentation | `docs/` | Brent | Rui | Docs review |
| Resource registry | `src/cleveragents/*/resource*` | Hamza | Luis | Architecture review |
| Decision/validation pipelines | `src/cleveragents/*/decision*`, `src/cleveragents/*/validation*` | Luis | Jeff | Architecture review |

---

## Acceptable vs Blocking Findings

### Acceptable Findings (P2–P3)

These findings should be noted but do not block merge.

**Example 1 – Naming nit (P3)**

```python
# Reviewer comment: P3:nit – Consider renaming `proc` to `process` for clarity.
def start_proc(config: Config) -> None:
    ...
```

> **Remediation:** Author may rename in a follow-up or leave as-is with
> justification.

**Example 2 – Missing docstring (P2)**

```python
# Reviewer comment: P2:should-fix – Public function missing docstring.
def calculate_priority(task: Task) -> int:
    return task.weight * task.urgency
```

> **Remediation:** Author opens a follow-up PR within 3 days to add
> docstrings to all public functions in the module.

**Example 3 – Minor duplication (P2)**

```python
# Reviewer comment: P2:should-fix – This validation logic is duplicated in
# action_service.py:45. Consider extracting to a shared helper.
```

> **Remediation:** Author creates a tech-debt ticket and addresses in the
> next sprint.

### Blocking Findings (P0–P1)

These findings **must** be resolved before merge.

**Example 1 – SQL injection (P0)**

```python
# Reviewer comment: P0:blocker – Raw SQL with string interpolation.
# Use parameterized queries.
query = f"SELECT * FROM users WHERE name = '{user_input}'"
```

> **Remediation:** Replace with parameterized query:
> `session.execute(text("SELECT * FROM users WHERE name = :name"), {"name": user_input})`

**Example 2 – Missing error handling (P1)**

```python
# Reviewer comment: P1:must-fix – FileNotFoundError not caught.
# CLI will show raw traceback to user.
def load_config(path: str) -> Config:
    with open(path) as f:
        return Config.parse(f.read())
```

> **Remediation:** Wrap in try/except and raise a domain-specific error:
>
> ```python
> def load_config(path: str) -> Config:
>     try:
>         with open(path) as f:
>             return Config.parse(f.read())
>     except FileNotFoundError:
>         raise ConfigNotFoundError(f"Config file not found: {path}")
> ```

**Example 3 – Broken migration (P0)**

```python
# Reviewer comment: P0:blocker – downgrade() is empty. This migration
# cannot be rolled back.
def downgrade() -> None:
    pass
```

> **Remediation:** Implement the reverse operation in `downgrade()` or
> document why rollback is intentionally unsupported.

---

## Required Test Matrix for Reviewers

Before approving any PR, reviewers must verify that the following nox sessions
pass. The author should include CI results or local run output in the PR
description.

| Nox Session | Purpose | Required For |
| ----------- | ------- | ------------ |
| `nox -s lint` | Ruff linting | All PRs |
| `nox -s typecheck` | Pyright type checking | All PRs |
| `nox -s unit_tests` | Behave BDD unit tests | All PRs |
| `nox -s integration_tests` | Integration test suite | PRs touching domain/infra |
| `nox -s coverage_report` | Coverage check (≥97%) | All PRs |
| `nox -s security_scan` | Bandit + Semgrep + Vulture | All PRs |
| `nox -s dead_code` | Vulture dead code detection | PRs adding new code |
| `nox -s complexity` | Radon complexity analysis | PRs adding new functions |
| `nox -s benchmark` | ASV benchmark suite | PRs affecting performance |

### Minimum Gate for Merge

A PR may only be merged if **all** of the following are true:

1. `nox -s lint` passes with zero warnings
2. `nox -s typecheck` passes with zero errors
3. `nox -s unit_tests` passes with zero failures
4. `nox -s coverage_report` shows ≥97% coverage
5. `nox -s security_scan` reports zero high/critical findings
6. At least one reviewer has approved
7. All P0 and P1 findings are resolved