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 (alembic/)	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	alembic/	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)

# 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)

# 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)

# 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)

# 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)

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

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)

# 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