# Code Review Python

> Performs structured code reviews on Python and FastAPI codebases covering correctness, async safety, security, Pydantic v2 patterns, testing, and style.

- **Type:** Skill
- **Install:** `agentstack add skill-ucdavis-ai-skills-registry-code-review-python`
- **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior
- **Seller:** [ucdavis](https://agentstack.voostack.com/s/ucdavis)
- **Installs:** 0
- **Category:** [Agent Skills](https://agentstack.voostack.com/c/agent-skills)
- **Latest version:** 0.1.0
- **License:** MIT
- **Upstream author:** [ucdavis](https://github.com/ucdavis)
- **Source:** https://github.com/ucdavis/ai-skills-registry/tree/main/skills/code-review-python

## Install

```sh
agentstack add skill-ucdavis-ai-skills-registry-code-review-python
```

Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent.

## About

# Code Review Best Practices

When asked to review code — a file, a function, a PR diff, or a module — follow this checklist systematically. Always provide **specific, actionable feedback** with line references and concrete improvement suggestions. Never give vague praise or criticism.

---

## 1. Understand the Intent First

Before writing any comment:

- Read the code **top-to-bottom** at least once without commenting.
- Understand what the code is **supposed to do**, not just what it does.
- Note the surrounding context (imports, dependencies, callers).

---

## 2. Correctness & Logic

Check for bugs, edge-cases, and incorrect assumptions.

- **Off-by-one errors**, incorrect loop bounds, or fence-post mistakes.
- **None/null safety**: are optional values guarded before use?
- **Exception handling**: are exceptions caught at the right level? Are bare `except:` or `except Exception:` clauses swallowing errors silently?
- **Async correctness**: are `await` calls present where required? Is there any blocking I/O (e.g., `time.sleep`, synchronous DB calls) in an async context?
- **Return values**: is every code path returning a value? Are early returns correct?
- **Concurrency**: any shared mutable state in async code? Any race conditions?

**Example annotation style:**
```
# ❌ Bug: This will silently ignore ALL exceptions, masking real failures.
try:
    result = await some_call()
except Exception:
    pass

# ✅ Fix: Log and re-raise, or handle specifically.
try:
    result = await some_call()
except SpecificError as e:
    logger.error("Failed during some_call: %s", e)
    raise
```

---

## 3. Security

- **Never log secrets**, tokens, or PII (passwords, API keys, user data).
- **Input validation**: all external inputs (request bodies, query params, path params) must be validated — prefer Pydantic models over raw dict access.
- **SQL Injection**: never use raw string formatting in queries; always use parameterized queries or ORM methods.
- **CORS**: ensure `allow_origins=["*"]` is only used in local/dev environments; flag it with a `# FIXME` if it is not environment-gated.
- **Secrets in code**: flag any hardcoded credentials, tokens, or keys as **CRITICAL**.
- **Authentication & authorization**: verify that protected endpoints use proper dependency injection (e.g., `Depends(get_current_user)`).

---

## 4. Pydantic & Data Models

- Use `model_config = ConfigDict(...)` not `class Config`.
- Use `@field_validator` / `@model_validator`, not `@validator` / `@root_validator`.
- Use `model_dump()` / `model_dump_json()`, not `.dict()` / `.json()`.
- Use `BaseSettings` from `pydantic_settings` (separate package) for env config.
- Pydantic models should be the single source of truth for shape and validation — avoid duplicate manual validation.

---

## 5. FastAPI Patterns

- **Router organization**: routers should live in dedicated routing directories (e.g., `api/routers/`), with a single responsibility (one logical domain per router).
- **Dependency injection**: use `Depends()` for shared logic (auth, DB sessions, config). Avoid instantiating dependencies inside route functions.
- **Response models**: always declare `response_model=` on route decorators so the API contract is explicit and documented.
- **Status codes**: use explicit `status_code=` (e.g., `status_code=201` for creation, `status_code=204` for deletion).
- **Error responses**: use `HTTPException` with appropriate status codes. Never return `500` for client errors.
- **Lifespan events**: startup/shutdown logic belongs in lifespan context managers, not in `@app.on_event` (deprecated in FastAPI 0.93+).

---

## 6. Design & Architecture

- **Single Responsibility**: each function/class should do one thing. If a function is doing more than one logical thing, suggest splitting it.
- **DRY (Don't Repeat Yourself)**: flag duplicated logic and suggest extraction into a shared utility or base class.
- **Dependency direction**: high-level modules should not import from low-level implementation details.
- **Magic values**: flag any hardcoded strings or numbers that should be constants or config values.
- **God objects**: classes that know too much or do too much should be flagged for decomposition.

---

## 7. Testing

- **Coverage**: is the changed code covered by unit tests? If not, call this out explicitly.
- **Test scope**: unit tests should mock external I/O (e.g., `httpx`, DB, MCP clients). Integration tests may use real services.
- **Edge cases**: are tests covering error paths, not just the happy path?
- **Assertions**: tests must assert on the **expected output** — not just that the code ran without exception.
- **Test isolation**: tests must not depend on global state or execution order. Use fixtures to set up and tear down state.
- **Naming**: test names should follow `test___` convention.

---

## 8. Documentation & Readability

- **Docstrings**: public functions and classes must have docstrings. Use Google-style docstrings for consistency:
  ```python
  def my_func(x: int) -> str:
      """One-line summary.

      Args:
          x: Description of x.

      Returns:
          Description of the return value.

      Raises:
          ValueError: If x is negative.
      """
  ```
- **Inline comments**: complex or non-obvious logic must be explained inline. If the comment just restates what the code does, remove it.
- **Variable names**: single-letter names (except loop counters like `i`) and cryptic abbreviations should be flagged.
- **Function length**: functions longer than ~50 lines are a signal to refactor. Flag them.

---

## 9. Style & Consistency

- **Type hints**: all function signatures must have type hints for arguments and return values.
- **Imports**: standard library → third-party → local (PEP 8). No wildcard imports (`from foo import *`).
- **Formatting**: ensure code adheres to the project's formatting and linting rules (e.g., `ruff`, `black`). Flag obvious style deviations (long lines >120 chars, inconsistent quoting, etc.).
- **Logging**: use `logger.error("msg: %s", var)` (lazy %-formatting), never f-strings in log calls (performance anti-pattern).
- **TODO/FIXME comments**: all `# TODO` and `# FIXME` comments must include context. Flag any that are vague or very old.

---

## 10. Output Format

When delivering a code review, structure your response as follows:

### Summary
A 2–4 sentence overview: what does the code do, is it generally well-written, what is the most important concern?

### Critical Issues 🔴
Issues that **must** be fixed before merging: security vulnerabilities, data loss risks, correctness bugs, broken contracts. Number each item.

### Improvements 🟡
Things that are not blocking but meaningfully improve quality: design issues, missing tests, performance problems, unclear naming. Number each item.

### Minor / Nits 🟢
Style, convention, and cleanup items that improve consistency but have minimal functional impact. Can be bulleted for brevity.

### Positive Highlights ✅
Call out what is done well. This is not optional — good feedback is balanced.

---

## Quick Reference Checklist

Use this as a mental checklist before finalizing any review:

- [ ] Logic is correct for all known edge cases
- [ ] No silent exception swallowing
- [ ] No blocking I/O in async context
- [ ] All inputs are validated (Pydantic)
- [ ] No secrets hardcoded or logged
- [ ] CORS / auth guards are correct for environment
- [ ] Pydantic V2 syntax used throughout
- [ ] FastAPI router / dependency patterns are followed
- [ ] Test coverage exists for the changed code
- [ ] Type hints present on all public functions
- [ ] Docstrings present on public functions and classes
- [ ] No magic numbers or strings — use constants/config
- [ ] Logging uses %-formatting, not f-strings

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [ucdavis](https://github.com/ucdavis)
- **Source:** [ucdavis/ai-skills-registry](https://github.com/ucdavis/ai-skills-registry)
- **License:** MIT

Install and usage instructions live in the source repository linked above.

## Pricing

- **Free** — Free

## Security capabilities

Automated source analysis of v0.1.0 — what this tool can access:

- **Network access:** yes
- **Filesystem access:** no
- **Shell / process execution:** no
- **Environment & secrets:** no
- **Dynamic code execution:** no

*"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.*


## Versions

- **0.1.0** — security scan: passed — Imported from the upstream source.

## Links

- Listing page: https://agentstack.voostack.com/l/skill-ucdavis-ai-skills-registry-code-review-python
- Seller: https://agentstack.voostack.com/s/ucdavis
- Browse the marketplace: https://agentstack.voostack.com/browse

---
Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.