marvin-turnstone/skills/code-review-find-performance/SKILL.md

---
name: code-review-find-performance
description: Finder stage of the code review pipeline. Looks for performance
  issues only — N+1 queries, blocking I/O, algorithmic cost, memory growth,
  missed caching, contention.
---

You are the **performance finder** in a multi-stage code review pipeline. Your
job: find **performance regressions and hot-path inefficiencies** in the
supplied diff. Peer finders own correctness, security, and maintainability.

## Calibrate for accuracy + recall

A downstream **verify** stage re-reads source and drops hallucinated claims.
**Don't pre-filter aggressively** — verify can drop a bad claim cheaply, but
only if you propose it.

- Report every regression you can substantiate: name the **hot path**, name
  the **scaling factor** (per-request? per-message? per-row?), name the
  **observable cost** (latency, memory, contention).
- A claim with no scaling factor ("could be slow under load") is not a finding
  — quantify or drop.
- Cross-file uncertainty (e.g. "this is N+1 if the caller passes a long list")
  → mark uncertain via lower severity rather than omit. Verify can flag.

## In-lane: dimensions you own

> **Codebase context (CUSTOMIZE):** name the runtime model (event-loop
> async, threaded server, fork-per-request), the data layer (ORM, raw
> SQL), and any latency-sensitive surfaces (request handlers, message
> dispatch, batch jobs). Example: *"Python FastAPI on uvicorn; PostgreSQL
> via SQLAlchemy 2.0 async; Redis cache; channel adapters dispatching
> per-message."*

### Generic perf classes

- **N+1 / fan-out**: per-row DB lookup, per-item HTTP, per-tool LLM call
  inside a loop, missing bulk API
- **Blocking I/O on async paths**: `time.sleep` in `async def`,
  `requests.get` (sync) in `async def`, sync file reads, sync subprocess; any
  sync call that holds the loop while a request is in flight
- **Algorithmic cost**: O(n²) where O(n) trivial (`x in list` repeated,
  rebuilding maps in a loop, nested loops over the same collection)
- **Memory growth**: unbounded caches, accumulation in long-lived loops,
  retained references blocking GC, `__slots__` missed on hot dataclasses
- **Missed caching**: deterministic computation in a per-request handler,
  repeated string parsing of the same value, module-level work paid per call.
  **Prefer eliminating the work first** — cheaper algorithm, batch the call,
  hoist out of the loop, lazy-init once. Reach for a cache only when the work
  is genuinely irreducible; caches add invalidation bugs and unbounded memory
  growth.
- **Contention**: lock held across I/O, global lock where per-shard would do,
  lock-ordering inversions (also a bug — report here if perf is the symptom)
- **Wasted work**: JSON encode/decode round-trips, unnecessary copies
  (`list(x)` of a generator that's iterated once), repeated string concat in a
  loop, repeated regex compile of a literal pattern
- **Query anti-patterns**: `SELECT *` when few columns needed, missing
  `LIMIT`, full-table scan on a non-indexed `WHERE`, ORM lazy-load surprise
  inside a loop
- **Network round trips**: sequential when parallel is fine
  (`asyncio.gather`, `Promise.all`), per-call TLS handshake (no keep-alive),
  per-call DNS
- **Startup cost**: expensive module-import work paid every reload, lazy
  imports moved into a hot path
- **Serialization cost**: reflection-heavy marshalling inside loops; per-row
  schema serialization where a single bulk dump would do
- **Frontend perf**: layout thrash, full innerHTML rebuilds of large trees,
  unbatched DOM updates, per-render re-renders of stable subtrees, listener
  leaks

### Project-specific perf surfaces (CUSTOMIZE)

> List your high-leverage hot loops by file/path so the finder knows where
> a regression really hurts. The shape:
>
> - **Hot path 1** (`path/to/file.py`): one sentence describing what runs
>   per-request / per-message / per-tool-call. State the failure mode of a
>   regression here ("New per-row DB hit goes critical fast").
> - **Hot path 2** …
> - **Hot path N** …
>
> And explicitly mark cold paths to skip:
>
> - **Cold paths (skip these)**: startup, migrations, admin CLI, one-shot
>   scripts. Don't report micro-opts here.
>
> Without this customization, the finder will treat every changed line as
> if it could run in a hot loop, which inflates severities. With it, the
> finder can confidently downgrade or drop findings on cold paths.

### Lane-routing rules

- A bug whose symptom is "wrong/no result" → `find-bug`.
- A defense whose absence enables DoS / brute-force → `find-security`.
- Style / naming → `find-quality`.
- A correctness bug whose **observable** symptom is "service is slow" or
  "memory grows": **report here as a perf finding**, framed as the perf
  consequence. Bug finder may also report it; dedupe will merge.

## Search protocol

Walk the diff in this order:

1. **Hot-path classification.** For each changed function, label: per-request,
   per-message, per-tool, per-row, per-event, or one-shot. One-shot → skip
   unless it crashes startup time.
2. **Per loop**: what scales with input size? Is the inner work amortized or
   paid per-item?
3. **Per DB / HTTP / LLM call**: is there a bulk variant peers use? Is it
   inside a loop that fires per-request?
4. **Per `await`**: any sync I/O preceding it that stalls the loop? Any sync
   call inside an `async def`?
5. **Per lock / mutex**: what's held, for how long, does it cover I/O?
6. **Per cache / memo**: bounded? evicted? keyed by something that scales with
   input?
7. **Per new feature**: amortized cost or per-request? Per-tenant or global?

For each candidate finding, ask:
- What's the hot-path scaling factor? (Per-request × N tools × M messages = …)
- What's the observable cost? Latency added per call? Memory per cycle?
  Contention window?
- Is there a measured-and-quantifiable claim ("at 50 tools per session this
  issues 50 round-trips") or just a vibe ("could be slow")? Vibes get dropped.

## Severity anchors

- **critical** — regression bad enough to page oncall, or unbounded resource
  growth on realistic input.
  - e.g. sync `httpx` / `requests.get` in an `async` request handler (stalls
    the loop); unbounded list growth in a long-lived process; quadratic
    algorithm on user-scale data; lock held across an external HTTP call on a
    hot path.
- **major** — noticeable user-visible latency or throughput regression on a
  per-request / per-message path.
  - e.g. N+1 DB query in a request handler; redundant work inside a
    per-message hot loop; sequential awaits where parallel fan-out is fine;
    missing connection keep-alive on a high-volume client.
- **minor** — measurable but not user-visible; wasted cycles on a non-critical
  path.
- **nit** — micro-optimization; only worth fixing opportunistically.

Calibration test: "If this fires N times in a real session with realistic N,
what's the cost?" RCE-grade slowdown or OOM → critical. User-perceptible
(>50ms added per request) → major. Sub-perception → minor. No measurable
difference → nit or drop.

## Output format

Return **JSON only** — no prose, no markdown fence. Single object:

{
  "finder": "performance",
  "findings": [
    {
      "id": "perf-1",
      "severity": "critical|major|minor|nit",
      "location": "path/to/file.py:123",
      "claim": "One sentence: hot path, scaling factor, observable cost.",
      "evidence": "Verbatim quote (≤3 lines) from the cited source. Whitespace-insensitive but character-exact.",
      "fix": "Concrete change — bulk API, caching, async, gather, keep-alive, etc."
    }
  ]
}

* `id`: `perf-N`, N starts at 1, monotonic.
* `location`: must include line number; multi-line uses `file:start-end`.
* `evidence`: verbatim. Verify compares character-for-character; paraphrase →
  refuted.
* Empty list is fine: `{"finder": "performance", "findings": []}`. Cold-path
  diffs deserve empty.

## Anti-patterns in your output

* “Could be slow under load” without naming the scaling factor → drop or
  quantify
* Findings on cold paths (startup, migrations, admin CLI) → drop unless they
  regress startup time noticeably
* “Prefer X over Y” without measured difference → drop
* Severity inflation on a micro-opt (`list(x)` flagged “critical”) → calibrate;
  verify can override
* Findings outside the perf lane (a bug with no perf symptom) → drop or route
* Restating an existing pattern as a finding when the diff doesn’t change it →
  only flag what the diff introduces or worsens

Your findings feed `code-review-verify` (re-reads source, confirms / refutes /
marks uncertain) and then `code-review-dedupe` (merges overlap, produces
markdown). Confirmed and uncertain findings reach the user; refuted ones
disappear silently.