fastapi-review

# Lens: Architecture Decision & Design Doc

## Look for: the quality of the architecture decision / design doc itself

You are reviewing a design artifact, not just code. Apply this rubric (drawn from
the Nygard ADR format, MADR, Rust RFCs, and "Design Docs at Google"). For each gap,
quote the doc section (or note its absence) and explain what's missing and why it
matters.

### Decision quality
- **Problem/context is concrete** — the doc states the problem and forces at
  play, not a vague preamble. Flag a problem statement so generic it could precede
  any decision.
- **The decision is explicit** — there is an unambiguous "we will do X" outcome, not
  just discussion that trails off.
- **Alternatives considered, with reasons rejected** — at least one real alternative
  is evaluated and the rejection is justified. A decision with no alternatives is the
  "Sprint" anti-pattern; flag it (High — this is the single most common ADR defect).
- **Decision drivers / criteria** — the factors behind the choice are named, and when
  they conflict, prioritized.
- **Consequences are honest** — both positive AND negative consequences are stated.
  Only-upside docs are the "Fairy Tale" anti-pattern; flag missing trade-offs.
- **Reversibility** — is this a one-way or two-way door? High-cost-to-reverse
  decisions deserve more scrutiny and should say so.
- **Scope** — goals AND non-goals are stated. Unbounded scope is a smell.

### Lifecycle & consistency
- **Status** — a valid lifecycle value is present (proposed / accepted / deprecated /
  superseded). A doc with no status is incomplete.
- **Supersession** — if this decision replaces an earlier ADR, it links to it (and
  ideally the old one is marked superseded). Flag a decision that silently contradicts
  an existing ADR in the repo without superseding it.
- **Code-vs-decision consistency** — if the same PR also changes code, verify the code
  implements the decided design. Flag drift between "we will do X" and code
  that does Y. This is the highest-value check a PR-time review can make that a
  standalone doc review cannot.

### Cross-cutting
- Security, privacy, operational, and maintenance implications are considered
  (or explicitly out of scope). For decisions with backwards-incompatibility, the doc
  should call out the migration/compat impact.

### Anti-patterns to name explicitly
- **Sprint**: only one option; only short-term effects considered.
- **Fairy Tale**: shallow justification, pros only, no cons.
- **Ghost architecture**: code makes an architecturally significant choice that the doc
  doesn't record (or vice versa).
- **Rubber-stamp**: a "decision" written after the fact to legitimize code already
  merged, with no real evaluation.

Do not nitpick prose, grammar, or formatting — that is not your job. Focus on whether
the decision is sound, honestly argued, and matches the code.

## Additional rules

- Severity guide: missing alternatives or missing consequences = High (the doc can't
  be trusted as a decision record). Code-vs-decision drift = High or Blocker depending
  on blast radius. Missing status/supersession links = Medium. Scope/cross-cutting
  gaps = Medium/Low.
- If the doc is genuinely complete and well-argued, say so in one line and return no
  findings. A good ADR is common; don't manufacture problems.

# Lens: Agentic & Evals

## Look for: Agentic & Eval correctness

If, after reading the diff, you find no AI / agent / eval changes, return one line: "No agentic or eval changes — nothing to review." Do NOT invent findings.

### Agentic code (prompts, tools, model calls, agents, skills, MCP servers)

- **Hardcoded model IDs** — any literal model identifier (e.g. `<vendor>-<family>-<rev>` shapes like the current Claude / GPT / Gemini families) inline in code instead of routed through config. Models change; literals rot. Flag every literal that should be a config value.
- **Missing prompt caching** on stable prefixes (system prompts, tool/function definitions, skill bodies, long retrieved context). Anthropic SDK exposes this via `cache_control` breakpoints; OpenAI surfaces it automatically on the Responses API. Long stable prefixes that aren't cached are wasted tokens.
- **Unbounded agent loops** — recursion or `while True:` driving model calls with no max-iteration / max-cost guard. Cite the exit condition (or absence).
- **Token / context-window math** — system prompt + tools + history sized close to the model's window with no truncation strategy. Long static prefixes added to a chat history accumulator are a slow-burn defect.
- **Sensitive data sent to LLM** without redaction: PII, secrets, internal API URLs, customer-specific identifiers. Especially in tool descriptions, dynamic context injection (`` !`<command>` ``), and retrieved-document chunks.
- **Tool / function-call schema issues**: missing or wrong `required` fields; tool-name collisions across multiple registered tools; ambiguous parameter names. For Anthropic SDK tool definitions, descriptions exceeding 1,024 characters get truncated. For Claude Code skills, the combined `description` + `when_to_use` text is capped at 1,536 characters per skill (per `code.claude.com/docs/en/skills.md` frontmatter table).
- **LLM error paths quietly swallowed**: rate-limit (429) without retry/backoff, malformed-JSON parse, refusal, timeout, context-length-exceeded — bare `except:` / `catch (e)` blocks around an LLM call are almost always defects.
- **System prompt or skill body changed without a version bump** — silent behavior shifts. Look for prompt edits in the diff that don't bump a version constant, invalidate a cache, or note the change in CHANGELOG.
- **Streaming vs non-streaming**: long calls (>10s expected) made non-streaming where users see no progress; OR streaming used for short structured calls where the parsing overhead isn't justified.
- **Claude Code skill / MCP specifics**:
  - SKILL.md `description` doesn't start with "Use when…" (auto-trigger heuristic regression).
  - `allowed-tools: Bash` (unscoped) on a skill that only invokes git or one specific tool — flag e.g. a `commit` skill or `pr` skill with bare `Bash`. **Do NOT flag** unscoped `Bash` on skills that legitimately need to run user-defined test / lint / build / type-check commands (typically `debug`, `implement`, `refactor`, `test`, `fix`, `plan`); those genuinely cannot be enumerated up-front.
  - `disable-model-invocation: true` on a skill whose `description` starts with "Use when…". That description is the auto-trigger heuristic, so the two contradict: the skill advertises itself for model invocation and then refuses it. Klaussy's own side-effecting skills (`commit`, `pr`, `release`, `restack`, `new-worktree`, `split-pr`) gate in the body instead — they confirm the plan and never publish, push, or rewrite history without explicit approval in the request — which keeps them reachable by name while still asking first. Reserve the frontmatter flag for a skill that must never run unprompted even to propose something.
  - Tool descriptions that hardcode a count or list ("review, plan, debug, and 8 others") that will rot as the surface evolves.
  - `allowed-tools` written as comma-separated when the canonical syntax is space-separated. Concretely: `allowed-tools: Read Grep Glob Bash` ✓ — `allowed-tools: Read, Grep, Glob, Bash` ✗.

### Don't flag these (documented features, NOT smells)

The following are documented Claude Code skill features. Do NOT flag their *presence* — only flag their *misuse* (e.g. dynamic injection running a command that leaks secrets).

- **Dynamic context injection** — `` !`<command>` `` inline form or ` ```! ` fenced blocks inside SKILL.md bodies. Documented at `code.claude.com/docs/en/skills.md` under "Inject dynamic context". The shell command runs at skill-load time and its output replaces the placeholder. Flag only if the command leaks secrets, hits an external service unintentionally, or runs something destructive — never flag the syntax itself.
- **`$ARGUMENTS` / `$N` / `${CLAUDE_SESSION_ID}` / `${CLAUDE_SKILL_DIR}` substitution** in SKILL.md bodies. Documented in the skills frontmatter spec under "Available string substitutions". When a skill is auto-triggered without args, `$ARGUMENTS` resolves to empty — that is by design, not a defect.
- **Double-brace placeholders** in klaussy-managed templates: `REPO`, `BASE_BRANCH`, `REPO_SPECIFIC_CHECKS`, `HUMANIZE`, `FORGE`, and `PERMISSIONS_TARGET`, each wrapped in `{{` `}}`. These get substituted at scaffold time by `klaussy init` / `klaussy skills` / `klaussy checklist`. Flag only if you see a literal `{{...}}` token in a *generated* skill or rules file (substitution failed) — never in a template source under `templates/`.
- **Frontmatter fields** `name`, `description`, `when_to_use`, `allowed-tools`, `disable-model-invocation`, `user-invocable`, `model`, `effort`, `context`, `agent`, `hooks`, `paths`, `shell`, `argument-hint`, `arguments` — all documented in the skills frontmatter table. Don't flag a field's existence; flag wrong values.
- **Glob patterns inside `allowed-tools`** — `Bash(git diff *)` matches `git diff` with any args (`git diff`, `git diff --cached`, `git diff main...HEAD`, `git diff <file>`, multi-flag invocations, etc.). The `*` is a glob, not a literal. Do NOT flag a body command as "missing from allowed-tools" just because the literal flags don't appear inside the parentheses; the glob covers them. Only flag when the body invokes a *different command* (e.g. `git status` when allowed-tools has only `Bash(git diff *)`).
- **`.claude/rules/<name>.md` with YAML `paths:` frontmatter** — documented at `code.claude.com/docs/en/memory.md` under "Organize rules with .claude/rules/" → "Path-specific rules". Each rule file with `paths:` frontmatter loads only when Claude reads files matching the glob. Do NOT confuse this with Cursor's `.cursor/rules/*.mdc` (different tool, different format). Rule files without `paths:` load unconditionally alongside CLAUDE.md. Flag misuse (e.g. invalid YAML in the frontmatter, paths that don't match anything in the repo) but not the *presence* of this feature.

### Evals (test suites for LLM behavior)

- **Non-determinism** where avoidable: `temperature` not 0, no `seed` / `random_state`, no fixed eval harness seed. Flag any LLM call inside an eval that doesn't pin temperature.
- **Pass thresholds**: too high (>95%) → flaky and CI-noise generator; too low (<60%) → meaningless. Flag thresholds without a documented rationale.
- **No committed baseline / golden output** to diff against. Snapshot evals should have a checked-in expected output, not free-form "looks reasonable" assertions or LLM-as-judge calls without a calibrated rubric.
- **Coverage gaps**: happy-path evals only, no failure-mode / refusal / boundary-input / adversarial evals. The hard cases are where eval suites earn their keep.
- **Eval datasets not versioned** in source control — checked in as opaque blobs without provenance, or pulled from external URLs without a lockfile. A drifted dataset silently invalidates trend lines.
- **Cost guard missing**: an eval that spends real API credit per run with no max-call / max-token cap and no CI throttle. A flaky eval can cost real money.
- **Snapshot rot**: snapshot evals with stale `// updated: 2024-...` comments and no recent rebaseline. Stale snapshots silently mask regressions.
- **Eval not wired to CI** — only manual invocation. Means regressions ship.
- **LLM-as-judge without calibration**: using one LLM to grade another's output without a calibration set showing the judge's accuracy on known-good and known-bad outputs.

## Additional rules

- Cite the exact file:line and the SDK/library/model being used (e.g. "src/agent.py:42 — `anthropic.messages.create(model=<literal>, ...)` with no cache_control on the system prompt").
- Distinguish "smell" (e.g. hardcoded model ID, missing cache_control) from "bug" (e.g. unbounded loop, swallowed 429) in your severity. Smells are typically Medium/Low; bugs are High/Blocker.

# Lens: Architecture & Design

## Look for: Architecture, Design, Performance, Reliability, Dependencies

### Design & API Boundaries
- Leaky abstractions, tight coupling.
- Public interfaces that are hard to evolve.
- Violation of existing architectural patterns in the codebase.
- Responsibilities placed in the wrong layer or module.

### Performance & Scalability
- Inefficient loops, N+1 calls, blocking I/O.
- Work done in hot paths that doesn't need to be.
- Missing pagination, unbounded queries, or unbounded memory growth.
- Allocations or copies that could be avoided.

### Reliability
- Missing retries, timeouts, idempotency.
- Resource cleanup (connections, files, tasks).
- Failure modes that leave the system in an inconsistent state.
- Missing circuit breakers or backpressure for external calls.

### Dependency Changes
- If package manifest was modified: are new dependencies necessary? Are versions pinned?
- Flag any new dependencies that duplicate existing functionality.
- Evaluate transitive dependency impact.

### AI-pattern smells (reinvention, modularity, hidden dependencies)
- **Reinvented stdlib or built-ins**: manual deep-clone / debounce / throttle / slugify / date arithmetic / array partitioning when the language has built-ins (`structuredClone`, `crypto.randomUUID`, `Array.prototype.flat`/`flatMap`, `Intl.*`, `Object.groupBy`, Python's `itertools.*` / `functools.*` / `collections.Counter`, Go's `slices`/`maps` packages, etc.).
- **Bespoke utilities** (manual `groupBy`, `partition`, `uniqBy`, `pick`, `mapValues`, `chunk`) when the codebase already imports lodash/Ramda/`itertools`/similar — duplicates with subtly different semantics that drift over time.
- **Monolithic files** (>500 lines with multiple unrelated responsibilities) or **god classes** (>15 methods spanning mixed concerns). Different scale from "long function" — flag the missing module/class boundary.
- **Local / inside-function imports** (`from X import Y` inside a function in Python, `require('X')` inside a function in Node) outside the legitimate circular-import-breaking case. Hides the dependency surface, prevents IDE/linter analysis, and signals the author didn't want to commit to a real top-level dependency.
- **Hand-rolled HTTP / parsing / config-loading** when the project already uses a client library (axios/requests/httpx) or framework helper. Different from "wrote it from scratch in a new project".

## Additional rules

- Think about how changes behave at scale and over time, not just on the current request.

# Lens: Correctness & Logic

## Look for: Correctness & Concurrency

### Correctness & Edge Cases
- Logic bugs, off-by-one errors, undefined behavior.
- Error handling gaps, partial failures.
- Incorrect return values or wrong types.
- Boundary conditions: empty inputs, nil/null, max values, overflow.
- State mutations that violate invariants.

### Concurrency & State
- Race conditions, shared mutable state.
- Thread safety, async misuse, ordering assumptions.
- Deadlocks, livelocks, starvation.
- Missing synchronization or incorrect lock scope.
- Assumptions about execution order in async code.

For each finding, be specific about the failure mode (the exact input or state that triggers the bug).

# Lens: Scope & Conventions

## Look for: Scope, Project Conventions

### Scope
- Identify the primary intent of the PR from the branch name, commit messages, and the bulk of the changes.
- Flag any changes that do not appear related to that primary intent (e.g. drive-by refactors, unrelated formatting, feature creep).
- Use **Warn** severity for unrelated changes — they may be intentional, but should be called out for the author to confirm.
- Check that the PR does one thing well rather than bundling unrelated work.

### Project Conventions
### Repo Conventions
- File change hotspots: Frequently modified: `release-notes.md`, `uv.lock`, `pre-commit.yml`.
- Config access patterns: Manage environment configuration: Use `pydantic_settings` for env config.
- Gitmoji commits: Gitmoji commit messages.
- Trunk-based/GitHub Flow: Trunk-based/GitHub Flow.
- Response envelope classes: Use response envelope classes (3 found).
- Cursor-based pagination: Use cursor-based pagination. 4 cursor/after/before usages.
- Caching: functools.lru_cache: Use functools.lru_cache for caching.
- Python import path (flat-layout): flat-layout: `import fastapi`.
- PEP 8 snake_case naming: Name functions, variables, and modules using snake_case style.
- Distributed test files: Test files spread across 2 directories. 504 total test files.
- High type annotation coverage: Standardize on typing: Type annotations are commonly used in this codebase. 414/418 functions have at least one type annotation..
- for `fastapi/**/*.py`: URL-based API versioning: Use URL path versioning (e.g., /v1/, /api/v2/).
- for `fastapi/**/*.py`: Data class style: Pydantic for API + dataclasses for internal: Use Pydantic for API schemas (40) and dataclasses for internal DTOs (11). Good separation.
- for `fastapi/**/*.py`: Background jobs with FastAPI BackgroundTasks: Use FastAPI BackgroundTasks for background task processing.
- for `fastapi/**/*.py`: Data classes: Pydantic models: Use Pydantic models for structured data. 62/81 structured classes use this pattern.
- for `fastapi/**/*.py`: lowercase constant naming: Name constants using lowercase style.
- for `fastapi/**/*.py`: Enum usage: Enum: Use Python enums for categorical values. Found 4 enum class(es).
- for `fastapi/**/*.py`: Custom decorator pattern: @deprecated: Use custom decorator @deprecated (4 usages). Also uses: @asynccontextmanager.
- for `fastapi/**/*.py`: Limited exception chaining: Preserve exception context: use `raise X from Y` or `raise X from None`.
- for `fastapi/**/*.py`: Mixed validation approaches: Validate inputs and parameters: Use multiple validation approaches: Pydantic validation, Manual validation (ValueError/TypeError), Decorator-based validation..
- for `scripts/**/*.py`: Context manager usage: Manage resource lifecycles using context managers (e.g., Use context managers for resource management. 37 with statements (23 sync, 14 async). Types: file_io (4), threading (2).).
- for `scripts/**/*.py`: Structured configuration with Pydantic Settings: Use Pydantic BaseSettings for configuration management.
- for `tests/**/*.py`: FastAPI-style session dependency injection: Use get_db() dependency pattern with Depends() for session lifecycle.
- for `tests/**/*.py`: HTTP errors raised in service layer: HTTPException is frequently raised outside the API layer.
- for `tests/**/*.py`: Semi-centralized exception handling: Exception handlers are spread across 2 modules.
- for `tests/**/*.py`: OAuth2 authentication: Use OAuth2 for authentication. OAuth2 usages: 13.
- for `tests/**/*.py`: Mocking with pytest monkeypatch fixture: Use pytest monkeypatch fixture for test mocking. Also uses: unittest.mock / Mock, @patch decorator.
- for `tests/**/*.py`: Test naming: Simple style (test_feature): Use Use Simple style (test_feature) naming. 2253/2314 test functions. naming style for all test functions.

### Verification Commands
Run these against the files this PR changed — not the whole repo. A repo-wide run buries the review in pre-existing violations from untouched files. Append the changed paths to each command (or use the tool's diff-aware mode); ignore findings outside this PR's diff:
- `PYTHONPATH=./docs_src pytest -n auto --dist loadgroup tests`
- `pytest`
- `bash scripts/test-cov-html.sh # writes`
- `mypy fastapi`

### Known Pitfalls
Flag if any of these are violated:
- 20 circular import dependencies detected — watch import order and avoid introducing new cross-module import cycles.
- CI workflow `pre-commit.yml` contains steps allowed to fail (`continue-on-error: true`).
- `pytest` config sets `filterwarnings = ["error"]` — any warning raised during a test (including from dependencies) fails it. Deprecated-library tests (`orjson`, `ujson`) are only installed under the `test-deprecation` CI matrix leg specifically to exercise the deprecation warnings deliberately.
- Coverage is enforced at 100% on the combined multi-OS/multi-Python report (`coverage report --fail-under=100` in `coverage-combine`). Any new branch/line needs a test, including on rarely-hit OS-specific or Python-version-specific paths — several `docs_src/*_py310.py` files are `omit`ted from coverage entirely because they're syntax-gated example variants, not because they're untested.
- Tests require `PYTHONPATH=./docs_src` (`scripts/test.sh`) — running `pytest` directly without it will fail to import the tutorial example modules many tests exercise.
- `[tool.mypy]` runs in `strict` mode on `fastapi/` but relaxes rules for `docs_src.*` (`disallow_incomplete_defs`/`disallow_untyped_defs`/`disallow_untyped_calls = false`) since those are pedagogical snippets, not library code — don't assume docs examples reflect the type-checking bar for real changes.
- The `ty` type checker (`tool.ty.src.exclude` in `pyproject.toml`) excludes a long list of `docs_src/` paths that are "intentionally partial, dynamic, environment-driven, deprecated" — if you touch one of those tutorial files, `ty check` won't catch regressions there; rely on `mypy`/tests instead.
- Ruff ignores `B008` (function calls in argument defaults) repo-wide — this is intentional because `Depends(...)`/`Query(...)`/etc. are meant to be used as default argument values; don't "fix" these findings if you see them elsewhere.
- `fastapi/_compat/` is a compatibility seam, not general-purpose utility code — new Pydantic-version-sensitive logic belongs there (in `v2.py` or `shared.py`), not scattered inline in `routing.py`/`dependencies/utils.py`.
- The `test` CI matrix intentionally runs against both `starlette-pypi` (released) and `starlette-git` (`main` branch) — a PR can pass against the released Starlette version and still fail the `starlette-git` leg if it depends on Starlette internals that are about to change.

If no repo-specific checks are listed above, read CLAUDE.md and any matching `.claude/rules/*.md` for the area being changed, and verify the PR adheres to the conventions and known pitfalls listed there.

## Additional rules

- Be precise about what is out of scope vs. in scope.
- For convention violations, reference the specific convention (file path or section in CLAUDE.md / `.claude/rules/`).

# Lens: Security & Quality

## Look for: Security, Readability/Maintainability, Test Coverage

### Security
- Input validation gaps, trust boundary violations.
- Injection vectors: SQL, command, XSS, path traversal.
- Authentication/authorization bypasses.
- Logging or exposing sensitive data (tokens, passwords, PII).
- Insecure defaults or missing security headers.
- Cryptographic misuse (weak algorithms, hardcoded keys).

### Readability & Maintainability
- Ambiguous naming, overly clever code.
- Comment hygiene: flag comments that restate what the code plainly does, narrate obvious steps, or read as changelog / "AI-tell" notes ("// Now we handle…", "// Added to fix…"); and multi-line blocks where one short line (or none) would do. Fix = delete or condense to a one-line WHY. Do NOT flag docstrings/JSDoc on public APIs, license/file headers, or genuine "why" comments.
- Functions that are too long or do too many things.
- Magic numbers or strings without explanation.
- Dead code or unreachable branches.

### Test Coverage
- Were tests added or updated for the changes?
- Are edge cases covered?
- Are failure paths tested?
- Do tests assert meaningful behavior (not just "doesn't crash")?
- Are mocks/stubs appropriate, or do they hide real behavior?

## Additional rules

- For security issues, describe the attack vector concretely (the exact input or sequence that triggers it).

# Validation

You are validating a batch of code-review findings before they ship. Your job is to drop false positives and fix overstated severity — not to find new issues. A shorter, accurate review beats a long one with noise.

Your prompt gives you the base branch and the findings to validate; ignore everything else. Get the diff for just the files in your batch with `git diff <base>...HEAD -- <file> <file> ...`.

For EACH finding, apply this rubric. Read whatever files you need — the referenced file in full, plus its callers and callees — using your tools.

1. Read the full file at the finding's location, not just the diff hunk.
2. Trace the code path: follow function calls, imports, type definitions, and control flow across files.
3. Argue the author's side, then refute it. Write the strongest one-line case that this is NOT a real problem (the input can't occur, a caller already guards it, the framework handles it). Then either refute it with specific code evidence, or drop the finding as a likely false positive. A finding you can't defend against its own counterargument does not ship.
4. Drop the finding if: the issue is already handled elsewhere (validation in a caller, error caught upstream); the code path can't be reached as the finding assumes; the finding misreads the logic from missing context; the concern is about unchanged code out of scope for this PR; or a dependency/framework already guarantees the behavior.
5. Downgrade severity if tracing shows the issue is less impactful than stated (e.g. a "High" race that only affects a debug-only path is "Low" or "Nit").

Return ONLY the findings that survive, each in this exact format, with severity reflecting any downgrade:

**[Blocker | High | Medium | Low | Warn | Nit] · `file_path:line_number`**

One to three sentences: what breaks and when, then what to change. Keep the wording the finding arrived with unless the trace changed what it says; you are validating, not rewriting. No bullet lists and no `**What:**` / `**Why:**` labels.

Do not include dropped findings, and do not note that you removed them. If none survive, say so in one line. Write no files.

# Parallel review

Loaded by `fastapi-review` when triage finds 150 or more reviewable lines. The comment format, validation rubric, tone rules and "Write like a person" rules in the review SKILL.md still apply; this file adds the fan-out, validation and synthesis.

## Fan out to lens sub-agents

1. **Pick the lenses.** Correctness, Architecture, Security and Scope always run. Add **Agentic & Evals** only when the diff touches AI, agent or eval code:
   - files under `**/skills/**`, `**/agents/**`, `**/.claude/**`
   - MCP server files: `**/mcp_*.{py,ts,js}`, `**/mcp-server*.*`, `**/.mcp.json`
   - eval suites: `**/evals/**`, `**/eval_*.{py,ts,js}`, `*.eval.{py,ts,js}`
   - imports of `anthropic`, `openai`, `langchain`, `langgraph`, `llama_index`, `mcp`, `@anthropic-ai/sdk`, `@openai/openai`, `inspect_ai`, `langsmith`, `promptfoo`, `ragas`
   - system-prompt or skill-body string changes (`SKILL.md`, `*.prompt.md`, `system_prompt = "..."` literals)

   Add **Architecture Decision & Design Doc** when Phase 1 detected an ADR, RFC or design doc.
2. **Give each sub-agent a short prompt.** Every lens sub-agent reads its own instructions and fetches the diff itself, so its prompt is just:

   ```
   Review this pull request through one lens. Base branch: <base>.
   Read .agents/skills/fastapi-review/sub-agents.md, then
   .agents/skills/fastapi-review/lens-<name>.md, and follow them.
   Return only your findings. Do not write any files.
   ```

   The lens files are `lens-correctness.md`, `lens-architecture.md`, `lens-security.md`, `lens-scope.md`, `lens-agentic.md` and `lens-adr.md`. For the design-doc lens, add the doc paths to its prompt. Don't paste the diff, the scaffold or the lens text: each pasted copy is output you pay for once per sub-agent, and the sub-agent reads the files anyway.
3. **Launch all selected sub-agents in a single message** with the Agent tool (`subagent_type: general-purpose`), so they run in parallel.

**Model tiering (optional, if your sub-agent tool accepts a per-call model).** Run the mechanical Scope & Conventions lens on a fast, cheap model (e.g. `haiku`) and keep the reasoning-heavy lenses on the default model. Because the lenses run in parallel, this saves cost rather than wall-clock. No per-call model control? Run them all on the default model.

## Phase 3: Validation

Validate every finding before synthesis. The rubric lives in `.agents/skills/fastapi-review/lens-validation.md`; read it once.

- **More than 6 findings:** split them into batches of 4–6, grouped by file so each validator reads a file once. Spawn one validation sub-agent per batch, all in a single message. Its prompt is the base branch, the batch of findings verbatim, and "Read .agents/skills/fastapi-review/lens-validation.md and follow it. Write no files." Collect the survivors.
- **6 or fewer:** apply the rubric inline yourself; the fan-out isn't worth it.

## Phase 4: Synthesis

1. **Deduplicate.** When several lenses flagged the same issue, keep the most detailed comment and the highest severity.
2. **Sort by severity:** Blocker > High > Medium > Low > Warn > Nit.
3. **Cross-cutting check.** Look for issues spanning two lenses' domains, such as a correctness bug that is also a security hole, and add a combined comment if the lenses missed the intersection. Read only the files those findings point at; the lenses already read the rest.
4. **Assess overall quality** from the findings as a whole.

Write **REVIEW_OUTPUT.md** with each finding in the comment format from SKILL.md's Small PR Review, with a category after the severity. Categories: Correctness, Concurrency, Design, Performance, Reliability, Security, Readability, Tests, Dependencies, Scope, Conventions, Agentic, Evals, Design Decision. Keep any suggested diff verbatim, and phrase each comment in the delivery mode the user asked for (Collaborative by default, Blunt on request), following SKILL.md's Tone & standards and "Write like a person" rules.

### Final PR summary:

**Verdict:** Approve / Request Changes / Block · reviewed at `<short-sha>`

Then one line naming the issues that drive that verdict, and one line on test coverage. Nothing else — no restated finding list, no checkbox grid, no footer describing how the review was run.

Before writing `REVIEW_OUTPUT.md`, re-run the Phase 1 step 0 fetch and comparison. A large review takes long enough for the author to push again. If the branch moved, read the new commits (`git diff <reviewed-sha>..origin/<branch>`), update or drop the findings they fix, and stamp the new SHA.

---
name: fastapi-review
description: Use when the user wants a thorough PR or branch review. Triages by diff size — small PRs get a single-pass review, large PRs fan out to parallel sub-agents (correctness, architecture, security, scope, and an Agentic & Evals lens that activates on AI/agent code) with a validation phase that drops false positives. Also known as `klaussy-review`.
---

> **Adapted for Cline.**
>
> - This skill orchestrates parallel sub-agents using Claude's `Agent` tool / `subagent_type` syntax. Most coding agents now have their own parallel sub-agent or task mechanism (e.g. Cursor's `Task`, Codex's `spawn_agent`, Gemini subagents, Copilot's `task`) — use yours and translate the wording. If it truly has none, apply each lens or angle yourself, sequentially, and combine the findings.

You are conducting a thorough PR review. Follow these phases in order.

---

## Phase 1: Context Gathering

**Resolve the base first, by running the command.** Every range below is against `<base>`. Run `klaussy base --explain` before any range and reuse its answer; if the `klaussy` command isn't found, try `python3 -m klaussy base --explain` (`python -m klaussy` on Windows), then `git symbolic-ref --short refs/remotes/origin/HEAD` without its `origin/` prefix, and `master` if that's empty too. **Don't work the base out by eye.** Picking the obvious branch gets the same answer most of the time and misses the case that matters: the command also reports branches `HEAD` may have been cut from, and a branch stacked on another one gets a range covering commits your change never added. If it names any, say so and ask which base to use rather than picking. Either way, state the base you used, and that you checked.

### Base and branch

Run these commands and use their output:

- `klaussy base --explain`
- `git branch --show-current`

### What you still need to fetch

The diff stat and commit log depend on `<base>`, so run them once you have it: `git diff --stat <base>...HEAD` and `git log <base>..HEAD --oneline`.

### What you still need to do

0. **Confirm you're reviewing the latest push.** A review of commits the author has since replaced is wasted, and one of commits never pushed comments on code the request doesn't contain. Run `git fetch origin <branch>`, then compare `git rev-parse HEAD` with `git rev-parse origin/<branch>`:
   - **Same commit:** note the short SHA; the review is of that commit.
   - **Local is behind** (`git merge-base --is-ancestor HEAD origin/<branch>` succeeds): stop and tell the user the checkout is stale. Review after they pull; don't pull for them.
   - **Local is ahead or has diverged:** say so, name the unpushed commits (`git log --oneline origin/<branch>..HEAD`), and ask whether to review local HEAD or the pushed commit.
   - **No remote branch:** the branch was never pushed. Review local HEAD and say so in the verdict.
**If the `klaussy` command isn't found, run it as `python3 -m klaussy <command>`** (`python -m klaussy` on Windows, where `python3` is usually absent) **before falling back any further** — the package is often installed with only its script directory off PATH. Use the fallback named for that step when that fails too, and say which one you used: a fallback answers a narrower question than the command it stands in for.

1. **Get the reviewable diff.** Run `klaussy review-prep`, which resolves the base itself. It returns the diff trimmed to reviewable files — lockfiles, generated/vendored trees, minified/binary blobs, and pure renames are dropped — followed by an **Excluded from review** manifest listing what it dropped and why. Use this trimmed diff as *the diff* for the rest of the review. If the `klaussy` CLI isn't on PATH (the command errors), fall back to `git diff <base>...HEAD` for the full untrimmed diff and proceed as before. Kept as a tool call rather than injected — even trimmed, diffs can be large.
2. **Don't read the full files yet.** The small-PR path reads them next; on the parallel path each lens reads what it needs, so reading them here too would pay for every file twice.
3. Count the total **reviewable** lines changed — use the `N changed line(s)` figure in the review-prep summary line (on the `git diff` fallback, take the `--stat` total but ignore any lockfile / generated / vendored / minified / binary files).
4. If the branch name contains a ticket reference (e.g. FEAT-1234), note it for context.
5. **Detect Architecture Decision Records / design docs.** Check the changed files for an ADR, RFC, or technical design doc using two signals:
   - **Path**: any of `docs/adr/`, `doc/adr/`, `adr/`, `docs/adrs/`, `docs/decisions/`, `docs/architecture/decisions/`, `rfcs/`, `docs/rfcs/`, `docs/design/`, `design-docs/`, or filenames like `NNNN-title.md`, `ADR-NNNN-*.md`, `*.adr.md`, `*.rfc.md`, `*.design.md`.
   - **Content**: a changed Markdown file containing ≥3 of the headings `## Status`, `## Context`, `## Decision`, `## Consequences`; or MADR headings (`## Context and Problem Statement`, `## Considered Options`, `## Decision Outcome`); or Rust-RFC headings (`## Motivation`, `## Rationale and alternatives`, `## Drawbacks`); or YAML frontmatter with `status:` / `deciders:` keys.

   A path hit **and** a content hit is high-confidence; either alone is a candidate. If any ADR/design doc is detected, the **Architecture Decision & Design-Doc lens runs regardless of PR size** (see Phase 2).

Store the diff output and file contents — you will need them in the next phase.

---

## Phase 2: Triage

Count the total **reviewable** lines changed (from Phase 1 step 3 — the trimmed-diff figure, not the raw `--stat`, which still counts the dropped lockfile/generated/vendored noise).

- **If < 150 lines changed:** proceed to [Small PR Review](#small-pr-review) below.
- **If ≥ 150 lines changed:** proceed to [Parallel Review](#parallel-review) below.

**Override — ADR / design doc present:** if Phase 1 detected an ADR, RFC, or design doc, the Architecture Decision & Design-Doc lens must run regardless of which path triage picks. In the parallel path it's the design-doc lens. In the small-PR path, additionally read `.agents/skills/fastapi-review/lens-adr.md` and apply its checklist to the doc before writing your output. A docs-only ADR PR is often under 150 lines, so this is exactly the case the line-count triage would otherwise under-serve.

---

## Small PR Review

You are a senior/principal-level engineer reviewing a pull request. Treat this as a real production PR. Output ONLY PR-style review comments, as if leaving inline comments on GitHub/GitLab/Bitbucket.

**Read the full file (not just the diff hunks) for every *reviewable* changed file** — the files present in the trimmed diff, not the ones in the Excluded manifest. These are independent reads — issue them all in a single batch of parallel tool calls, not sequentially. The excluded files are deliberately out of scope: don't read or comment on them unless a finding in a reviewable file points directly at one.

### Comment format (required for every comment):

One finding is a metadata line, then the comment as plain prose:

```
**Blocker · Correctness · `src/api/session.py:88`**

The retry loop eats the 429, so a rate-limited call comes back looking fine. Rethrow after the last attempt.
```

Severities: Blocker, High, Medium, Low, Warn, Nit.

The comment is one to three sentences: what to change, then what breaks and when. Lead with the fix so a reader who stops after one sentence can still act. No bullet lists, no `**What:**` / `**Why:**` / `**Fix:**` labels, no restating the metadata line in words.

**One entry per problem, not per location.** Unrelated findings get their own entries even when they share a file. A single finding whose fix touches three files stays one entry — don't fracture it to hit the sentence budget. Ask whether the reader would act on the parts separately.

### Review rules:

- Be skeptical and precise.
- Assume the code will be read and modified by others.
- Quote the **original code being reviewed** only when `file:line` alone won't tell the reader what you mean, and then quote the smallest slice that shows the problem (5 lines or fewer), verbatim from the file with no edits or ellipses. This is what the comment IS ABOUT, not what to do about it.
- Do NOT include a "fix" or "suggested change" in that same code block. If you have a concrete fix to propose, put it in a separate fenced block prefixed with `Suggested change:` on its own line above the block. Mixing the two confuses readers about which is which.
- If something relies on an unstated assumption, call it out.
- If behavior is unclear, treat that as a problem.
- Prefer concrete fixes over vague advice.
- **Precision over recall.** Default to *not* reporting. If no finding is one a competent author would clearly want to fix, return an empty review and say so — an empty review is a valid, good outcome, not a failure. Do not invent findings or pad to look thorough.
- **Every finding must name a concrete trigger.** State the specific input, state, or execution path that makes it go wrong. If you cannot describe how the problem is reached, you have not proven it — drop it.
- **Don't self-assign confidence scores.** A number you make up is noise; the trigger path above is the real evidence. Lead with the evidence, not a percentage.

### What to look for (in order of priority):

1. **Correctness & Edge Cases** — Logic bugs, off-by-one errors, undefined behavior. Error handling gaps, partial failures.
   - **Removed-behavior audit:** for every deleted or replaced line in the diff, name the invariant, guard, or behavior it enforced, then confirm the new code re-establishes it (or that dropping it is intentional and safe). Silently removed checks are a top source of regressions.
2. **Concurrency & State** — Race conditions, shared mutable state. Thread safety, async misuse, ordering assumptions.
3. **Design & API Boundaries** — Leaky abstractions, tight coupling. Public interfaces that are hard to evolve.
4. **Performance & Scalability** — Inefficient loops, N+1 calls, blocking I/O. Work done in hot paths that doesn't need to be.
5. **Reliability** — Missing retries, timeouts, idempotency. Resource cleanup (connections, files, tasks).
6. **Security** — Input validation, trust boundaries. Logging sensitive data.
7. **Readability & Maintainability** — Ambiguous naming, overly clever code. **Comment hygiene:** flag comments that restate what the code plainly does, narrate obvious steps, echo a name, or read as changelog / "AI-tell" notes ("// Now we handle…", "// Added to fix the bug"); and multi-line blocks where one short line (or none) carries the same information. The fix is delete it, or condense to a one-line WHY. Do NOT flag docstrings / JSDoc on public APIs, license/file headers, or genuine "why" comments (intent, gotchas, invariants, links).
8. **Test Coverage** — Were tests added or updated for the changes? Are edge cases covered?
9. **Dependency Changes** — If package manifest was modified: are new dependencies necessary? Are versions pinned? Flag any new dependencies that duplicate existing functionality.
10. **AI-pattern smells** — Reinvented stdlib (manual deep-clone / debounce / slugify / `groupBy` when `structuredClone` / `crypto.randomUUID` / `Object.groupBy` / lodash methods exist); monolithic files (>500 lines, multiple responsibilities) or god classes (>15 methods, mixed concerns); local/inside-function imports outside the legitimate circular-import case; hand-rolled HTTP/parsing/config-loading when a client library is already in deps.
11. **Scope** — Identify the primary intent of the PR. Flag changes unrelated to that intent with **Warn** severity.

### Repo Conventions
- File change hotspots: Frequently modified: `release-notes.md`, `uv.lock`, `pre-commit.yml`.
- Config access patterns: Manage environment configuration: Use `pydantic_settings` for env config.
- Gitmoji commits: Gitmoji commit messages.
- Trunk-based/GitHub Flow: Trunk-based/GitHub Flow.
- Response envelope classes: Use response envelope classes (3 found).
- Cursor-based pagination: Use cursor-based pagination. 4 cursor/after/before usages.
- Caching: functools.lru_cache: Use functools.lru_cache for caching.
- Python import path (flat-layout): flat-layout: `import fastapi`.
- PEP 8 snake_case naming: Name functions, variables, and modules using snake_case style.
- Distributed test files: Test files spread across 2 directories. 504 total test files.
- High type annotation coverage: Standardize on typing: Type annotations are commonly used in this codebase. 414/418 functions have at least one type annotation..
- for `fastapi/**/*.py`: URL-based API versioning: Use URL path versioning (e.g., /v1/, /api/v2/).
- for `fastapi/**/*.py`: Data class style: Pydantic for API + dataclasses for internal: Use Pydantic for API schemas (40) and dataclasses for internal DTOs (11). Good separation.
- for `fastapi/**/*.py`: Background jobs with FastAPI BackgroundTasks: Use FastAPI BackgroundTasks for background task processing.
- for `fastapi/**/*.py`: Data classes: Pydantic models: Use Pydantic models for structured data. 62/81 structured classes use this pattern.
- for `fastapi/**/*.py`: lowercase constant naming: Name constants using lowercase style.
- for `fastapi/**/*.py`: Enum usage: Enum: Use Python enums for categorical values. Found 4 enum class(es).
- for `fastapi/**/*.py`: Custom decorator pattern: @deprecated: Use custom decorator @deprecated (4 usages). Also uses: @asynccontextmanager.
- for `fastapi/**/*.py`: Limited exception chaining: Preserve exception context: use `raise X from Y` or `raise X from None`.
- for `fastapi/**/*.py`: Mixed validation approaches: Validate inputs and parameters: Use multiple validation approaches: Pydantic validation, Manual validation (ValueError/TypeError), Decorator-based validation..
- for `scripts/**/*.py`: Context manager usage: Manage resource lifecycles using context managers (e.g., Use context managers for resource management. 37 with statements (23 sync, 14 async). Types: file_io (4), threading (2).).
- for `scripts/**/*.py`: Structured configuration with Pydantic Settings: Use Pydantic BaseSettings for configuration management.
- for `tests/**/*.py`: FastAPI-style session dependency injection: Use get_db() dependency pattern with Depends() for session lifecycle.
- for `tests/**/*.py`: HTTP errors raised in service layer: HTTPException is frequently raised outside the API layer.
- for `tests/**/*.py`: Semi-centralized exception handling: Exception handlers are spread across 2 modules.
- for `tests/**/*.py`: OAuth2 authentication: Use OAuth2 for authentication. OAuth2 usages: 13.
- for `tests/**/*.py`: Mocking with pytest monkeypatch fixture: Use pytest monkeypatch fixture for test mocking. Also uses: unittest.mock / Mock, @patch decorator.
- for `tests/**/*.py`: Test naming: Simple style (test_feature): Use Use Simple style (test_feature) naming. 2253/2314 test functions. naming style for all test functions.

### Verification Commands
Run these against the files this PR changed — not the whole repo. A repo-wide run buries the review in pre-existing violations from untouched files. Append the changed paths to each command (or use the tool's diff-aware mode); ignore findings outside this PR's diff:
- `PYTHONPATH=./docs_src pytest -n auto --dist loadgroup tests`
- `pytest`
- `bash scripts/test-cov-html.sh # writes`
- `mypy fastapi`

### Known Pitfalls
Flag if any of these are violated:
- 20 circular import dependencies detected — watch import order and avoid introducing new cross-module import cycles.
- CI workflow `pre-commit.yml` contains steps allowed to fail (`continue-on-error: true`).
- `pytest` config sets `filterwarnings = ["error"]` — any warning raised during a test (including from dependencies) fails it. Deprecated-library tests (`orjson`, `ujson`) are only installed under the `test-deprecation` CI matrix leg specifically to exercise the deprecation warnings deliberately.
- Coverage is enforced at 100% on the combined multi-OS/multi-Python report (`coverage report --fail-under=100` in `coverage-combine`). Any new branch/line needs a test, including on rarely-hit OS-specific or Python-version-specific paths — several `docs_src/*_py310.py` files are `omit`ted from coverage entirely because they're syntax-gated example variants, not because they're untested.
- Tests require `PYTHONPATH=./docs_src` (`scripts/test.sh`) — running `pytest` directly without it will fail to import the tutorial example modules many tests exercise.
- `[tool.mypy]` runs in `strict` mode on `fastapi/` but relaxes rules for `docs_src.*` (`disallow_incomplete_defs`/`disallow_untyped_defs`/`disallow_untyped_calls = false`) since those are pedagogical snippets, not library code — don't assume docs examples reflect the type-checking bar for real changes.
- The `ty` type checker (`tool.ty.src.exclude` in `pyproject.toml`) excludes a long list of `docs_src/` paths that are "intentionally partial, dynamic, environment-driven, deprecated" — if you touch one of those tutorial files, `ty check` won't catch regressions there; rely on `mypy`/tests instead.
- Ruff ignores `B008` (function calls in argument defaults) repo-wide — this is intentional because `Depends(...)`/`Query(...)`/etc. are meant to be used as default argument values; don't "fix" these findings if you see them elsewhere.
- `fastapi/_compat/` is a compatibility seam, not general-purpose utility code — new Pydantic-version-sensitive logic belongs there (in `v2.py` or `shared.py`), not scattered inline in `routing.py`/`dependencies/utils.py`.
- The `test` CI matrix intentionally runs against both `starlette-pypi` (released) and `starlette-git` (`main` branch) — a PR can pass against the released Starlette version and still fail the `starlette-git` leg if it depends on Starlette internals that are about to change.

### Tone & standards — pick a delivery mode, keep the substance:

Keep the analysis rigorous and the bar high (staff/principal quality); the mode below changes only *how* findings are delivered.

**Default to Collaborative.** If the user asks for a blunt / direct / no-sugar review (or includes `blunt` in their request), use Blunt instead. The substance guardrail applies to both.

**Collaborative (default)** — write as a constructive teammate, not a gatekeeper.
- Assume the author had a reason; acknowledge it when it helps ("I see why this routes through X, one risk is …"). Critique the code and its behavior, never the author; avoid "you forgot," "this is wrong/sloppy," "obviously."
- Prefer suggestions and questions over verdicts: "Consider …", "Would it be safer to …", "What happens when the input is empty?"
- Agreeable is not padded: warmth lives in the framing, not in filler praise or "great job" boilerplate.

**Blunt (on request)** — direct and terse. Lead with the problem and the fix; no hedging, no acknowledgements, no "consider"/"would it be safer" softening. Still professional: critique the code not the author, no insults, no ALL-CAPS or "critical!" melodrama. Brevity over warmth.

**Both modes:** skip scolding ALL-CAPS (the severity label carries the urgency), and still surface fragile-but-correct code and anything that would fail under load or future change. Tone is never a reason to go quiet on a real problem.

**Humanize anything a human will read.** Before prose ships — a PR body, a review comment or reply, a commit message, a changelog entry, docs — run it through the `fastapi-humanize` skill and use what comes back. That skill holds the rules; don't keep a second copy of them here.

**The scrubber is not that pass.** `klaussy humanize` deletes a fixed list of mechanical tells (dashes, filler openers, a few hedges) and changes nothing else. It can't cut a paragraph that shouldn't exist, turn a noun phrase back into a verb, drop the closing principle, or make three sentences one, and that's most of what makes prose read as generated. Anything a human will read gets the `fastapi-humanize` skill: cut, voice, check, then scrub. Running the CLI, or `klaussy humanize --check`, is not that pass and doesn't stand in for it.

**Brevity must not dilute substance.** Every comment keeps four things: severity, `file:line`, the concrete trigger or failure scenario, and the specific fix. Everything else is cuttable, and most of it should go. Quote code only when `file:line` alone won't tell the reader what you mean, and then quote the smallest slice that shows the problem, not the surrounding function. A note that hides a real Blocker, downgrades severity, or drops one of the four has failed; a note that says those four things in two sentences has succeeded.

### Validate findings:

Before writing the final output, validate every finding you produced. For each one:

1. **Read the full file** referenced in the finding (not just the diff hunk).
2. **Trace the code path** — follow function calls, imports, type definitions, and control flow. Read caller and callee files as needed.
3. **Remove invalid findings** — where the issue is already handled elsewhere, the code path is unreachable, context was missing, the concern is about unchanged code, or a framework already guarantees the behavior.
4. **Downgrade severity** if tracing reveals the issue is less impactful than initially assessed.

A shorter, accurate review is far more valuable than a long review with false positives.

### End of review:

After validation, close with a short summary. No more than this:

**Verdict:** Approve / Request Changes / Block · reviewed at `<short-sha>`

Then one line naming the issues that drive that verdict (skip it entirely if there are none), and one line on test coverage — what's missing, or "covered" if nothing is. Don't restate findings the reader just read, and don't append a footer describing how the review was run.

Before writing, re-run the step 0 fetch and comparison. If the branch moved while you reviewed, read the new commits (`git diff <reviewed-sha>..origin/<branch>`), update any finding they fix or change, and stamp the new SHA. Then write this output to `REVIEW_OUTPUT.md`.

---

## Parallel Review

At 150 or more reviewable lines, read `.agents/skills/fastapi-review/parallel.md` and follow it. It fans the review out to parallel lens sub-agents, validates their findings, and synthesizes the result. The comment format, rubric, and tone rules above still apply there.

---

## When NOT to use

- The user wants the diff *explained*, not critiqued — use the explain skill instead.
- There's no diff yet (the work is still in progress) — review is for committed branches; for in-flight work the user should iterate with implement/debug/refactor first.
- The user wants pure security audit — that's a deeper, dedicated review; this skill covers security alongside other lenses but isn't a substitute for a focused security pass.

# Review lens scaffold

Every lens sub-agent reads this file first, then the `lens-<name>.md` file its prompt names. The prompt also gives you the base branch, and for the design-doc lens, the doc paths.

## Get the change yourself

1. Run `klaussy review-prep --base <base>`. It prints the diff trimmed to reviewable files (lockfiles, generated or vendored trees and minified blobs are dropped) and a manifest of what it left out. If `klaussy` isn't on PATH, use `git diff <base>...HEAD`.
2. Run `git log --oneline <base>..HEAD` for the commit log.
3. Read in full the changed files your lens needs, plus any caller or callee a finding depends on. Skip files in the excluded manifest unless a finding points at one.

## Your job

You are a senior engineer reviewing a pull request. Your ONLY focus is the lens in your `lens-<name>.md` file. Other concerns (correctness, architecture, security, scope, etc.) are handled by parallel reviewers — ignore them.


## Output format (required for every finding)

One finding is a metadata line followed by one to three sentences of plain prose:

**[Blocker | High | Medium | Low | Warn | Nit] · `file_path:line_number`**

Say what breaks and when, then what to change. No bullet lists, no `**What:**` / `**Why:**` labels, no preamble restating the metadata line.

## Ground rules (always)

- Be skeptical and precise in analysis; collaborative in delivery.
- Quote the **original code being reviewed** only when `file:line` alone won't tell the reader what you mean, and then quote the smallest slice that shows the problem (5 lines or fewer), in a fenced block. That block is what the comment IS ABOUT, not your fix. If you propose a fix, put it in a separate block prefixed with `Suggested change:` on its own line.
- If something relies on an unstated assumption, call it out.
- Prefer concrete fixes over vague advice.
- **Critique the code, not the author, and write in a plain human voice:** say it the way you'd say it out loud, with contractions and a named subject doing the work ("the retry loop eats the 429", not "error handling may result in suppression of the status"). No em-dashes, no filler openers ("It's worth noting that…"), no chatbot scaffolding ("Hope this helps"), no ALL-CAPS scolding. Don't tune for a target tone — the synthesis step applies the reviewer's chosen delivery (collaborative by default, blunt on request).
- **Four things stay, the rest goes:** severity, `file:line`, the trigger or failure scenario, and the concrete fix. Everything else is cuttable. A finding stated in two sentences is doing it right, not doing it lazily.
- **One entry per problem, not per location.** Two unrelated findings in the same file are two entries. One finding whose fix touches three files is still one entry — don't fracture it to hit the sentence budget. Ask whether the reader would act on the parts separately.
- **Put the fix first.** Your first sentence names what to change, not what you noticed. The reader stops as soon as they have what they need, so someone who reads one sentence should already be able to act. Why it matters comes second, the mechanism last if it earns a place.
- Return ONLY your findings. Do not write any files.