TapTap
0f95f48899
CI / lint (pull_request) Successful in 1m31s
CI / sanitizers (address) (pull_request) Skipped
CI / sanitizers (undefined) (pull_request) Skipped
CI / fuzz-build (pull_request) Skipped
CI / coverage (pull_request) Skipped
CI / valgrind (pull_request) Skipped
CI / build-and-test (pull_request) Successful in 1m44s
- pr-review: replace invalid 'tea pr comment' with 'tea comment' (the former is not a tea subcommand) - integrator: drop stray '-M' from client examples (-M is now --remote-option and requires an argument), use the canonical pytest integration command, and bump the CI image tag to v10 - test-writer: build fuzz targets via -DENABLE_FUZZ=ON instead of hand-rolled -fsanitize flags; fix the fuzz binary path - cmake-expert: document -DSANITIZER=undefined, which is now live in CMakeLists.txt - README: add --allow-unauthenticated to the plain-TCP server example, use --preserve for metadata (not -M), and use the canonical integration command - AGENTS.md: use the canonical integration command
137 lines
3.7 KiB
Markdown
137 lines
3.7 KiB
Markdown
---
|
|
name: pr-review
|
|
description: 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:
|
|
```bash
|
|
tea pr checkout <number>
|
|
```
|
|
|
|
If already on a PR branch, verify with:
|
|
```bash
|
|
git branch --show-current
|
|
git log dev..HEAD --oneline
|
|
```
|
|
|
|
### Step 2: Get changed files
|
|
|
|
```bash
|
|
git diff dev --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`/`calloc` has a matching `free` on 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)
|
|
- `Data` objects 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)
|
|
- `done` flags checked properly in consumer loops
|
|
|
|
**Protocol Safety**
|
|
- `send_n_data` / `receive_n_data` return 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` — use `snprintf` with bounds
|
|
- `malloc` size 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:
|
|
1. **File:line** — exact location
|
|
2. **Severity** — critical / warning / style
|
|
3. **Category** — memory / thread / protocol / security / performance / logic / error
|
|
4. **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:
|
|
```bash
|
|
tea comment --repo TapTap/FastSync <number> "<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
|