8141158a3d
New agents: - architect: system design, module interactions, data flow - debugger: crash/memory/thread debugging with ASan, TSan, valgrind, gdb - security-auditor: TLS, input validation, buffer safety, crypto audit - refactorer: DRY, separation of concerns, API simplification - integrator: integration tests, CI/CD pipeline, end-to-end verification - code-explainer: architecture walkthrough, code explanation New skills: - debug-workflow: structured debugging workflow - refactor: code restructuring with test verification - security-audit: full security review with checklist - benchmark: performance benchmarking with multi-run medians - release: version bump, tests, tagging Improved existing: - c-reviewer: added security checklist - cmake-expert: added ASan/TSan/UBSan configs, ccache, cross-compilation - perf-analyst: added perf/valgrind/gprof commands - test-writer: added fuzzing harnesses, integration test patterns - pr-build: added sanitizer build variants - pr-review: added security review, performance impact assessment
3.7 KiB
3.7 KiB
name, description
| name | description |
|---|---|
| pr-review | Reviews a pull request for bugs, memory safety, thread safety, and style issues. Use when the user says "review PR", "review this PR", "review pull request", or wants a code review of changes. |
PR Review Skill
Read-only code review of a pull request branch. Produces a report — does NOT edit files.
Workflow
Step 1: Identify the PR branch
If the user specifies a PR number, check it out:
tea pr checkout <number>
If already on a PR branch, verify with:
git branch --show-current
git log main..HEAD --oneline
Step 2: Get changed files
git diff main --name-only -- '*.c' '*.h'
This gives the list of C source and header files changed in the PR.
Step 3: Read all changed files
Use the Read tool to read every changed .c and .h file. Read full files — don't skip any.
Step 4: Review each file
For each changed file, review for:
Memory Safety
- Every
malloc/callochas a matchingfreeon all code paths (including error paths) - No use-after-free (pointers used after
*_destroy()is called) - No double-free
- Null checks after allocation before use
- Correct buffer sizes (strlen + 1 for null terminators)
Dataobjects created/destroyed properly
Thread Safety
- Shared state accessed under mutex
- No race conditions on queue operations
- Condition variable signals under lock
- No deadlock potential (consistent lock ordering)
doneflags checked properly in consumer loops
Protocol Safety
send_n_data/receive_n_datareturn values checked- Status codes validated before use
- Config serialization handles partial reads
Logic Errors
- Off-by-one in loops/buffers
- Incorrect size calculations
- Wrong enum values or comparisons
- Missing break statements in switch
Error Handling
- Resources freed on error paths (no leaks)
- Functions return appropriate error values
- Error messages are useful
Security
- No
strcpy/strcat/sprintf— usesnprintfwith bounds mallocsize calculations don't overflow- Path traversal prevention (
..in filenames) - No fixed-size stack buffers for unbounded input
- TLS error codes checked after
SSL_read/SSL_write - No hardcoded certificates, keys, or credentials
- Received file permissions validated (no SUID/SGID injection)
- Denial of service: bounded memory, malformed messages handled
Performance Impact
- Unnecessary memory copies in hot paths
- Excessive malloc/free in tight loops
- Missing
sendfile()opportunity for large files - Compression level appropriate for use case
- Queue sizing appropriate for workload
Step 5: Categorize findings
For each issue:
- File:line — exact location
- Severity — critical / warning / style
- Category — memory / thread / protocol / security / performance / logic / error
- Description — what's wrong and how to fix it
Step 6: Output report
Print a formatted summary:
=== PR REVIEW SUMMARY ===
Branch: <branch-name>
Files reviewed: <count>
Issues found: <count>
CRITICAL: <count>
WARNING: <count>
STYLE: <count>
=== ISSUES ===
[1] src/shared/compression.c:42 — CRITICAL (memory)
Potential leak: data returned from data_compress() not freed on error path
Fix: Add data_destroy(compressed) before return false
...
=== VERDICT ===
[PASS] No critical issues found
— or —
[FAIL] <N> critical issues must be fixed before merge
Step 7: Optional PR comment
If the user wants to post the review as a PR comment:
tea pr comment <number> --comment "<review report>"
Rules
- Do NOT edit any source files
- Do NOT run builds or tests
- Do NOT commit or push
- Report ALL issues — don't filter or minimize
- Be specific about line numbers and fix suggestions