# Pr Review

> Review a GitHub pull request against the LMCache coding standards

- Skill: `lmcache/pr-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add lmcache/pr-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/lmcache/pr-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: lmcache (https://skillmd.com/u/lmcache)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/lmcache/pr-review

---


# PR Review: LMCache Code Quality Standard

Review a pull request against the LMCache coding standards defined in `docs/coding_standards.md`.

## Inputs

`$ARGUMENTS` is a PR number (e.g., `3032`) or a GitHub PR URL.

## Review Process

### Step 0 -- Load the standards

Read `docs/coding_standards.md` at the repo root. This is the authoritative reference for
all quality checks. Also read `AGENTS.md` for the quick-reference checklist.

### Step 1 -- Understand the PR

1. Fetch the PR metadata (`get`) to understand the title, description, author, and base branch.
2. Fetch the diff (`get_diff`) to see exactly what changed.
3. Fetch the list of changed files (`get_files`) for a structural overview.
4. Read any design docs referenced in the PR description. `docs/design/` mirrors
   the `lmcache/` package tree, so for each changed module `lmcache/<path>/`
   check `docs/design/<path>/` for relevant docs.
5. For each **changed public symbol** (function, method, class, field), note its fully qualified name. You will use these in Step 4 to find callers.

### Step 2 -- Design Doc Compliance

If a relevant design doc exists (check `docs/design/<path>/` for each changed
module `lmcache/<path>/`; `docs/design/` mirrors the `lmcache/` package tree):

- Check that the implementation conforms to documented contracts, invariants, and assumptions.
- Present compliance as a table:

| Requirement | Status | Notes |
|-------------|--------|-------|
| ... | pass/fail | ... |

If no design doc is relevant, state that and skip this section.

### Step 3 -- Coding Quality Review

Review the diff against these standards (from `docs/coding_standards.md`):

**Typing (Section 2)**:
- All new/modified functions have type hints for arguments and return values.
- No use of `Any` or bare generic containers (`list`, `dict`, `tuple`).
- No `assert` used for runtime validation -- must use `if/raise ValueError`.

**Docstrings (Section 3)**:
- All new/modified public functions have complete docstrings (summary, args, returns, raises, notes).
- Docstrings match actual current behavior (not planned future behavior).
- Modified functions have updated docstrings.

**Function and Interface Design (Section 4)**:
- Function names are self-explanatory.
- No ambiguous return values (e.g., `None` meaning two different things).
- No boolean parameters where an enum or function split would be clearer.
- Dict/container parameters have documented schemas.
- Caller assumptions documented in docstrings.
- Class interfaces are minimal (no unnecessary public methods).
- No cross-class private member access.

**New Functionality (Section 5)**:
- Design doc present for non-trivial features.
- Tests cover new features (error-severity if missing).
- Tests verify public interface, not implementation details.
- Tests do not access private members.

**Modifications (Section 6)**:
- Docstrings updated for changed functions.
- User/design docs updated if behavior changes.
- Checked for reusable existing code.

**Code Organization (Section 7)**:
- License header present on new Python files.
- Import ordering correct (Standard / Third Party / First Party / Local).
- All imports at file top (no lazy imports).
- `import torch` before native C extensions.
- Uses `init_logger(__name__)` from `lmcache.logging`, not bare `logging.getLogger()`.
- Operational logs at DEBUG level (not INFO).
- Log levels appropriate (no WARNING for expected concurrent conditions).
- Module-level helpers at top; private methods at end of class.

**Resource Management (Section 7.5-7.6)**:
- Error paths release locks, free pool entries, close file descriptors.
- No unbounded collection growth (sets/dicts cleaned up when resources freed).
- No unnecessary memory copies in hot paths.
- CUDA/GPU resources properly managed.

### Step 4 -- Caller Impact Analysis (Section 6.2)

This step is what distinguishes a real review from a diff-reader. For each changed
public symbol identified in Step 1, verify the change is safe for **every unchanged caller**.

**Triggers -- run this step whenever the PR:**
- Adds, removes, renames, or reorders parameters of a public function/method
- Changes parameter types, defaults, or accepted domains (e.g., adds `None` to accepted values, narrows a type)
- Changes return types or the meaning of sentinel returns (e.g., what `None` means)
- Adds, removes, or changes raised exceptions
- Changes side effects, ordering, blocking behavior, or thread-safety guarantees
- Modifies a method on a base class or protocol (subclasses are callers)
- Changes an invariant of a class field accessed by public API

**How to find callers:**

1. Use `Grep` with the symbol name across the repo (do not filter to only changed files).
   Include `tests/`, `benchmarks/`, `lmcache/integration/`, and `docs/` in the search.
2. For common function names that may have many false matches, use `Grep` with
   `-A 2 -B 2` to see context, or use the `Agent` (Explore) tool with a focused
   prompt (e.g., "find all callers of `L1Manager.is_key_evictable` and summarize
   what they do with the return value").
3. For base-class/protocol changes, also grep for subclasses
   (`class \w+\(<BaseName>`).

**For each caller, check:**
- Is the new signature compatible with this call site? (Keyword arg renames are especially dangerous.)
- Does the caller still handle the new return type / new exceptions correctly?
- Does the caller pass the output to something else whose contract would now be violated?
- If the change is backwards-compatible at the signature level, is it also backwards-compatible at the **semantic** level (e.g., does a now-optional param change default behavior)?

**Flag as error severity:**
- Any unchanged caller that is now broken and not updated in the PR.
- Any protocol/base-class change where at least one implementer is not updated.

**Flag as warning severity:**
- Callers whose assumptions are weakened by the change and whose correctness is not obviously preserved.

Report findings as part of the issues list, with the path:line of the affected caller and a one-line explanation of the impact.

### Step 5 -- Thread Safety (Section 8)

- Shared state protected by locks.
- Lock granularity appropriate.
- Concurrent access from multiple controller threads is safe.
- Even debug/test-only methods hold locks when accessing shared state.

### Step 6 -- Test Coverage (Section 5.2)

- New features have tests (error-severity if missing).
- Bug fixes have regression tests (error-severity if missing).
- Tests verify public API and docstring contract.
- Tests do not access private members or test private functions directly.
- Note what is tested and what is missing (especially failure paths and concurrent access).

### Step 7 -- PR Structure (Section 1.1)

- Is the PR appropriately scoped? If too many changed files, suggest how to break it down.

## Output Format

### Summary
One paragraph describing what the PR does.

### Design Doc Compliance
Table (if applicable) or "No relevant design doc."

### Issues

Group issues by severity, following the calibration from `docs/coding_standards.md` Section 9.2:

**error** (must fix before merge):
- Missing SPDX header, missing type hints, missing docstrings on public functions,
  new feature with zero tests, bug fix with no regression test, cross-class private
  member access, poor architectural decision, ambiguous return values, `assert` for
  runtime validation, docstring that misrepresents behavior.

**warning** (should fix):
- Docstring could be more detailed, tests that test implementation details, naming
  could be clearer, code could be more modular, missing `strict=True` on `zip` of
  parallel lists, log level too high for operational messages.

**info** (suggestion, non-blocking):
- Optional naming improvement, minor refactor for readability, style preference.

For each issue, include:
- File path and line number(s)
- What the issue is and why it matters
- Suggested fix (if straightforward)

### Summary Table

| Severity | Count | Key Items |
|----------|-------|-----------|
| error    | N     | ... |
| warning  | N     | ... |
| info     | N     | ... |

## Reviewer Rules

- The review is scoped to the diff, with **one exception**: unchanged callers of
  changed symbols (Step 4). Flag a caller in unchanged code only when the PR's
  changes demonstrably affect it; do not review unrelated issues in those files.
- Do NOT praise the code. Only report issues or confirm the PR is clean.
- Do NOT pad findings. If the PR is clean, say "PR is clean" and list what was checked.
- Be precise about file paths and line numbers.
- If unsure whether something is an issue, treat it as `info`, not `error`.
- Do NOT leave trivial or nitpick comments that linters would catch.
- Focus on design decisions, architectural soundness, correctness, caller impact, thread safety, code cleanliness, modularity, and maintainability.