code-review
When to use
Use this skill when:
- Reviewing a PR (own or someone else's)
- Self-reviewing local changes before creating a PR
- Responding to review feedback on your PR
- The user asks to "review", "check", or "look at" code changes
Procedure: Review code
Mindset
- Be thorough but pragmatic — catch real bugs, not style nitpicks that tools handle.
- Understand intent first — read the PR description, linked ticket, and commit messages before looking at code.
- Check the full picture — a change in a service may require changes in tests, migrations, docs.
- Assume good intent — suggest improvements, don't criticize.
Review order
- Understand the goal — what is this change trying to achieve?
- Architecture — does the approach make sense? Right layer? Right pattern?
- Correctness — does it actually work? Edge cases? Error handling?
- Quality — types, naming, readability, DRY, SOLID?
- Security — input validation, authorization, injection?
- Performance — N+1 queries, missing indexes, unbounded queries?
- Tests — are new paths covered? Are existing tests still valid?
- Conventions — does it follow project standards?
Review checklist
The checks below are stack-agnostic. For framework-specific conventions (PSR-12 + declare(strict_types=1), FormRequest, single-action __invoke, API Resources, Pest, Blade escaping, Eloquent N+1) defer to the carve-outs:
Code quality
| Check |
What to look for |
| Type discipline |
New code is fully typed in the project's idiom (PHP typed properties + declare(strict_types=1), TS strict, Python type hints, Go / Rust by construction). |
| Style conformance |
Matches the project's formatter / linter output — no reformatting battles, no out-of-band style. |
| Naming |
Clear, descriptive names; matches the dominant casing in the surrounding code (camelCase / snake_case / PascalCase per the language's idiom). |
| Early returns |
No deep nesting. Guard clauses at the top. |
| Single responsibility |
Each class / module / function does one thing. HTTP handlers stay thin. |
| No magic |
No reach-through to globals (app(), $_GET, ambient context). No untyped data shapes leaking out of the I/O boundary. |
| Doc comments |
Only where the type system is insufficient (generics, complex shapes). No redundant docblocks. |
Architecture
| Check |
What to look for |
| Layer separation |
Business logic in services / use-cases, not in HTTP handlers. Domain models stay I/O-free. |
| Handler shape |
New handlers follow the framework's recommended shape (Laravel single-action __invoke, Next.js route handler, Express handler-per-route). See the stack carve-out. |
| Input validation |
Validated at the request boundary via the framework's primitive (Laravel FormRequest, Zod / class-validator, Pydantic, struct-tag validators). No ad-hoc inline if checks. |
| Response shaping |
Returns through a transformer / serializer / DTO. Never returns raw ORM entities. |
| DTOs / value objects |
Structured data between layers, not raw associative arrays / any / dict[str, Any]. |
| Dependency injection |
Constructor injection (or framework-idiomatic equivalent). No service-locator calls in business logic. |
Database & Performance
| Check |
What to look for |
| N+1 queries |
Relationship / association access in loops without eager / batch loading. |
| Missing indexes |
New columns used in WHERE / JOIN without a supporting index. |
| Unbounded queries |
Full-table reads (Model::all(), SELECT * without LIMIT, unpaged list endpoints). |
| Raw SQL |
Parameterised queries only. No string concatenation with user input. |
| Migrations |
Reversible. Targets the right connection / schema. Idempotent where the platform supports it. |
| Money / precision |
Uses an exact-precision type (PHP decimal / Math helper, TS bigint / decimal lib, Python Decimal), never float. |
Security
| Check |
What to look for |
| Authorization |
Authz check at every state-changing endpoint (Laravel Policy, Symfony voter, NestJS guard, framework middleware). No unprotected mutating routes. |
| Input validation |
All user input validated at the boundary via the framework's primitive. |
| Mass assignment |
No bulk-binding raw request payloads to ORM entities without an explicit allow-list ($fillable / $guarded in Laravel, DTO mapping in TS / Python). |
| Injection |
No raw queries / command lines / template strings with unescaped user input. |
| Output encoding |
Template output is escaped by default; raw / unescaped output is intentional and reviewed (Blade {{ }} vs {!! !!}, React dangerouslySetInnerHTML, Jinja |safe). |
| Sensitive data |
No secrets, tokens, or passwords in code, logs, or error responses. |
Tests
| Check |
What to look for |
| Coverage |
New code paths have tests. Bug fixes have regression tests (RED → GREEN). |
| Test quality |
Tests verify behaviour, not implementation details. |
| Framework idiom |
Correct conventions for the project's test framework (Pest / PHPUnit, Jest / Vitest, pytest, go test, cargo test) — see the stack carve-out for specifics. |
| Test data |
Provisioned via the project's idiom (seeders, factories, fixtures, builders). |
| Assertions |
Meaningful assertions. Not just "no exception thrown". |
| Flaky risks |
Time-dependent tests freeze the clock (travel(), jest.useFakeTimers(), freezegun). No reliance on execution speed. |
Before creating a PR
- Run the project's quality pipeline (see the stack carve-out for the exact commands — PHP:
quality-tools).
- Run tests via the project's runner (
make test, npm test, pytest, go test ./..., or the project's wrapper script).
- Ensure CI passes on the branch.
- Self-review the diff:
git diff origin/main..HEAD.
Receiving feedback
The response pattern
When receiving code review feedback, follow this sequence:
- READ — Complete feedback without reacting.
- UNDERSTAND — Restate the requirement in your own words (or ask if unclear).
- VERIFY — Check the suggestion against codebase reality.
- EVALUATE — Is it technically sound for THIS codebase?
- RESPOND — Technical acknowledgment or reasoned pushback.
- IMPLEMENT — One item at a time, test each.
If any item is unclear, STOP — do not implement anything yet. Items may be related;
partial understanding leads to wrong implementation.
No performative agreement
- Do NOT reply with "Great point!", "You're absolutely right!", "Excellent catch!" or similar.
- Instead: Just fix it. "Fixed." or "Updated — [brief description of what changed]."
- Actions speak louder than words — the code itself shows you heard the feedback.
Source-specific handling
Internal team feedback (trusted colleagues):
- Implement after understanding — no need for deep skepticism.
- Still ask if scope is unclear.
- Skip to action or technical acknowledgment.
External / Copilot / bot feedback (less context):
- Check: Technically correct for THIS codebase?
- Check: Does it break existing functionality?
- Check: Is there a reason for the current implementation?
- Check: Does the reviewer understand the full context?
- YAGNI check: If the reviewer suggests "implementing properly", grep the codebase
for actual usage. If unused → suggest removing (YAGNI).
- If it conflicts with existing architectural decisions → discuss with the team first.
When to push back
Push back when:
- Suggestion breaks existing functionality.
- Reviewer lacks full context.
- Violates YAGNI (unused feature).
- Technically incorrect for this stack.
- Legacy/compatibility reasons exist.
- Conflicts with architectural decisions.
How: Use technical reasoning, not defensiveness. Reference working tests/code.
Addressing PR comments systematically
When working through review comments on a PR:
- List all comments and review threads (
gh pr view --comments).
- Categorize: blocking → simple fixes → complex fixes.
- Clarify anything unclear BEFORE implementing.
- Fix one at a time, test each.
- Reply in the thread — not as a top-level PR comment.
# Reply to a specific review comment thread
gh api repos/{owner}/{repo}/pulls/comments/{comment_id}/replies \
-f body="Fixed in latest commit."
Output format
- Structure every finding by severity (Blocker / Suggestion / Nit) using the block below; never mix severities in one block.
- Group related findings; skip anything the project's linter or type-checker already catches — focus on logic, architecture, and judgment.
- End with a one-line verdict (approve / request-changes / comment) and a count of Blockers vs. Suggestions.
When reviewing code, structure feedback by severity:
🔴 **Blocker** — must fix before merge
Description of the issue and why it's critical.
🟡 **Suggestion** — should fix, improves quality
Description and suggested improvement.
🟢 **Nit** — optional, minor improvement
Description.
Group related findings. Don't repeat what the project's linter / type-checker already catches — focus on
logic, architecture, and things tools can't detect.
Adversarial review
Before creating a PR or presenting code changes, run the adversarial-review skill.
Focus on the "Code changes / Refactoring" attack questions.
Auto-trigger keywords
- code review
- PR review
- pull request
- review checklist
- review feedback
- review changes
- check my code
Gotcha
- Don't rewrite code that works and is tested just because you'd write it differently.
- The model tends to suggest changes that are out of scope — stay focused on the PR's intent.
- "I would prefer X" is not a valid review comment unless X prevents a bug or violates a rule.
- Always check if the PR has tests — missing tests is always worth flagging.
Do NOT
- Do NOT approve without actually reading the code.
- Do NOT agree with review comments without verifying them against the codebase.
- Do NOT use performative language when responding to feedback ("Great point!", "Excellent catch!").
- Do NOT nitpick style issues the project's formatter / auto-refactor (ECS, Prettier, Ruff, gofmt) handles automatically.
- Do NOT merge without CI passing and quality checks green.
Source: event4u-app/agent-config — distributed by TomeVault.
1---2name: code-review-2113description: Use when the user says \"review this\", \"check my code\", or wants feedback on changes. Reviews for correctness, quality, security, and coding standards.4---56# code-review78## When to use910Use this skill when:11- Reviewing a PR (own or someone else's)12- Self-reviewing local changes before creating a PR13- Responding to review feedback on your PR14- The user asks to "review", "check", or "look at" code changes1516## Procedure: Review code1718### Mindset1920- **Be thorough but pragmatic** — catch real bugs, not style nitpicks that tools handle.21- **Understand intent first** — read the PR description, linked ticket, and commit messages before looking at code.22- **Check the full picture** — a change in a service may require changes in tests, migrations, docs.23- **Assume good intent** — suggest improvements, don't criticize.2425### Review order26271. **Understand the goal** — what is this change trying to achieve?282. **Architecture** — does the approach make sense? Right layer? Right pattern?293. **Correctness** — does it actually work? Edge cases? Error handling?304. **Quality** — types, naming, readability, DRY, SOLID?315. **Security** — input validation, authorization, injection?326. **Performance** — N+1 queries, missing indexes, unbounded queries?337. **Tests** — are new paths covered? Are existing tests still valid?348. **Conventions** — does it follow project standards?3536## Review checklist3738The checks below are stack-agnostic. For framework-specific conventions (PSR-12 + `declare(strict_types=1)`, FormRequest, single-action `__invoke`, API Resources, Pest, Blade escaping, Eloquent N+1) defer to the carve-outs:39- PHP / Laravel → [`laravel`](../laravel/SKILL.md), [`laravel-validation`](../laravel-validation/SKILL.md), [`eloquent`](../eloquent/SKILL.md), [`pest-testing`](../pest-testing/SKILL.md), [`blade-ui`](../blade-ui/SKILL.md), [`php-coder`](../php-coder/SKILL.md)40- Symfony → [`symfony-workflow`](../symfony-workflow/SKILL.md)41- Next.js / TS → [`nextjs-patterns`](../nextjs-patterns/SKILL.md), [`react-shadcn-ui`](../react-shadcn-ui/SKILL.md)4243### Code quality4445| Check | What to look for |46|---|---|47| **Type discipline** | New code is fully typed in the project's idiom (PHP typed properties + `declare(strict_types=1)`, TS strict, Python type hints, Go / Rust by construction). |48| **Style conformance** | Matches the project's formatter / linter output — no reformatting battles, no out-of-band style. |49| **Naming** | Clear, descriptive names; matches the dominant casing in the surrounding code (camelCase / snake_case / PascalCase per the language's idiom). |50| **Early returns** | No deep nesting. Guard clauses at the top. |51| **Single responsibility** | Each class / module / function does one thing. HTTP handlers stay thin. |52| **No magic** | No reach-through to globals (`app()`, `$_GET`, ambient context). No untyped data shapes leaking out of the I/O boundary. |53| **Doc comments** | Only where the type system is insufficient (generics, complex shapes). No redundant docblocks. |5455### Architecture5657| Check | What to look for |58|---|---|59| **Layer separation** | Business logic in services / use-cases, not in HTTP handlers. Domain models stay I/O-free. |60| **Handler shape** | New handlers follow the framework's recommended shape (Laravel single-action `__invoke`, Next.js route handler, Express handler-per-route). See the stack carve-out. |61| **Input validation** | Validated at the request boundary via the framework's primitive (Laravel `FormRequest`, Zod / class-validator, Pydantic, struct-tag validators). No ad-hoc inline `if` checks. |62| **Response shaping** | Returns through a transformer / serializer / DTO. Never returns raw ORM entities. |63| **DTOs / value objects** | Structured data between layers, not raw associative arrays / `any` / `dict[str, Any]`. |64| **Dependency injection** | Constructor injection (or framework-idiomatic equivalent). No service-locator calls in business logic. |6566### Database & Performance6768| Check | What to look for |69|---|---|70| **N+1 queries** | Relationship / association access in loops without eager / batch loading. |71| **Missing indexes** | New columns used in `WHERE` / `JOIN` without a supporting index. |72| **Unbounded queries** | Full-table reads (`Model::all()`, `SELECT *` without `LIMIT`, unpaged list endpoints). |73| **Raw SQL** | Parameterised queries only. No string concatenation with user input. |74| **Migrations** | Reversible. Targets the right connection / schema. Idempotent where the platform supports it. |75| **Money / precision** | Uses an exact-precision type (PHP `decimal` / `Math` helper, TS bigint / decimal lib, Python `Decimal`), never `float`. |7677### Security7879| Check | What to look for |80|---|---|81| **Authorization** | Authz check at every state-changing endpoint (Laravel Policy, Symfony voter, NestJS guard, framework middleware). No unprotected mutating routes. |82| **Input validation** | All user input validated at the boundary via the framework's primitive. |83| **Mass assignment** | No bulk-binding raw request payloads to ORM entities without an explicit allow-list (`$fillable` / `$guarded` in Laravel, DTO mapping in TS / Python). |84| **Injection** | No raw queries / command lines / template strings with unescaped user input. |85| **Output encoding** | Template output is escaped by default; raw / unescaped output is intentional and reviewed (Blade `{{ }}` vs `{!! !!}`, React `dangerouslySetInnerHTML`, Jinja `\|safe`). |86| **Sensitive data** | No secrets, tokens, or passwords in code, logs, or error responses. |8788### Tests8990| Check | What to look for |91|---|---|92| **Coverage** | New code paths have tests. Bug fixes have regression tests (RED → GREEN). |93| **Test quality** | Tests verify behaviour, not implementation details. |94| **Framework idiom** | Correct conventions for the project's test framework (Pest / PHPUnit, Jest / Vitest, pytest, `go test`, `cargo test`) — see the stack carve-out for specifics. |95| **Test data** | Provisioned via the project's idiom (seeders, factories, fixtures, builders). |96| **Assertions** | Meaningful assertions. Not just "no exception thrown". |97| **Flaky risks** | Time-dependent tests freeze the clock (`travel()`, `jest.useFakeTimers()`, `freezegun`). No reliance on execution speed. |9899## Before creating a PR1001011. Run the project's quality pipeline (see the stack carve-out for the exact commands — PHP: `quality-tools`).1022. Run tests via the project's runner (`make test`, `npm test`, `pytest`, `go test ./...`, or the project's wrapper script).1033. Ensure CI passes on the branch.1044. Self-review the diff: `git diff origin/main..HEAD`.105106## Receiving feedback107108### The response pattern109110When receiving code review feedback, follow this sequence:1111121. **READ** — Complete feedback without reacting.1132. **UNDERSTAND** — Restate the requirement in your own words (or ask if unclear).1143. **VERIFY** — Check the suggestion against codebase reality.1154. **EVALUATE** — Is it technically sound for THIS codebase?1165. **RESPOND** — Technical acknowledgment or reasoned pushback.1176. **IMPLEMENT** — One item at a time, test each.118119If **any item is unclear**, STOP — do not implement anything yet. Items may be related;120partial understanding leads to wrong implementation.121122### No performative agreement123124- **Do NOT** reply with "Great point!", "You're absolutely right!", "Excellent catch!" or similar.125- **Instead:** Just fix it. "Fixed." or "Updated — [brief description of what changed]."126- Actions speak louder than words — the code itself shows you heard the feedback.127128### Source-specific handling129130**Internal team feedback** (trusted colleagues):131- Implement after understanding — no need for deep skepticism.132- Still ask if scope is unclear.133- Skip to action or technical acknowledgment.134135**External / Copilot / bot feedback** (less context):136- Check: Technically correct for THIS codebase?137- Check: Does it break existing functionality?138- Check: Is there a reason for the current implementation?139- Check: Does the reviewer understand the full context?140- **YAGNI check:** If the reviewer suggests "implementing properly", grep the codebase141 for actual usage. If unused → suggest removing (YAGNI).142- If it conflicts with existing architectural decisions → discuss with the team first.143144### When to push back145146Push back when:147- Suggestion breaks existing functionality.148- Reviewer lacks full context.149- Violates YAGNI (unused feature).150- Technically incorrect for this stack.151- Legacy/compatibility reasons exist.152- Conflicts with architectural decisions.153154How: Use technical reasoning, not defensiveness. Reference working tests/code.155156### Addressing PR comments systematically157158When working through review comments on a PR:1591601. **List** all comments and review threads (`gh pr view --comments`).1612. **Categorize**: blocking → simple fixes → complex fixes.1623. **Clarify** anything unclear BEFORE implementing.1634. **Fix** one at a time, test each.1645. **Reply in the thread** — not as a top-level PR comment.165166```bash167# Reply to a specific review comment thread168gh api repos/{owner}/{repo}/pulls/comments/{comment_id}/replies \169 -f body="Fixed in latest commit."170```171172## Output format1731741. Structure every finding by severity (Blocker / Suggestion / Nit) using the block below; never mix severities in one block.1752. Group related findings; skip anything the project's linter or type-checker already catches — focus on logic, architecture, and judgment.1763. End with a one-line verdict (approve / request-changes / comment) and a count of Blockers vs. Suggestions.177178When reviewing code, structure feedback by severity:179180```181🔴 **Blocker** — must fix before merge182Description of the issue and why it's critical.183184🟡 **Suggestion** — should fix, improves quality185Description and suggested improvement.186187🟢 **Nit** — optional, minor improvement188Description.189```190191Group related findings. Don't repeat what the project's linter / type-checker already catches — focus on192logic, architecture, and things tools can't detect.193194## Adversarial review195196Before creating a PR or presenting code changes, run the **`adversarial-review`** skill.197Focus on the "Code changes / Refactoring" attack questions.198199## Auto-trigger keywords200201- code review202- PR review203- pull request204- review checklist205- review feedback206- review changes207- check my code208209## Gotcha210211- Don't rewrite code that works and is tested just because you'd write it differently.212- The model tends to suggest changes that are out of scope — stay focused on the PR's intent.213- "I would prefer X" is not a valid review comment unless X prevents a bug or violates a rule.214- Always check if the PR has tests — missing tests is always worth flagging.215216## Do NOT217218- Do NOT approve without actually reading the code.219- Do NOT agree with review comments without verifying them against the codebase.220- Do NOT use performative language when responding to feedback ("Great point!", "Excellent catch!").221- Do NOT nitpick style issues the project's formatter / auto-refactor (ECS, Prettier, Ruff, gofmt) handles automatically.222- Do NOT merge without CI passing and quality checks green.223224---225> Source: [event4u-app/agent-config](https://github.com/event4u-app/agent-config) — distributed by [TomeVault](https://tomevault.io).226<!-- tomevault:4.0:skill_md:2026-06-16 -->
tomevault-io