---
name: sql-review
description: Read-only review of SQL code (raw SQL strings in Python repos, .sql files, schema/index definitions; tuned for SQL Server + pyodbc) against 10 rules - formatting, SELECT *, CTEs, index-friendly WHERE, DISTINCT misuse, COUNT(*) vs COUNT(col), parameterized queries, composite index order, row-by-row loops, UNION vs UNION ALL. Reviews through a subagent and writes every issue with a before/after fix plan to query_tracker.md. Use this whenever the user asks to review, check, audit or "SQL check pannu" any query, repository/DAO file, slow query, or DB code before merging, even if they don't say "sql-review".
---

# SQL Review

Audit SQL code against the 10 rules below. A subagent does the review; you verify its findings and turn them into a fix plan in `query_tracker.md`. The rules are written for SQL Server + pyodbc. When the code uses another database, apply the same rule in that dialect's syntax and say so.

## Ground rules

- **Read-only on code and databases.** Don't edit code. Don't run queries against any database, because the connection string may point at production. `query_tracker.md` is the only file you write. Fix code only if the user asks after reading the plan.
- **Every finding needs `file:line` evidence.** No evidence, no claim.
- **Don't guess table sizes.** If a finding only matters on a big table, say "matters if the table is large".

## Workflow

1. **Scope.** Grep `*.py` and `*.sql` for `SELECT|INSERT INTO|UPDATE .* SET|DELETE FROM|MERGE|CREATE INDEX|executemany` to list the files with SQL. Skip tests and one-off migration scripts unless the user points at them.
2. **Review through a subagent.** Always do this, even for a single file: it keeps file dumps out of the main context. Use a read-only agent (`Explore`). With more than ~10 files, split them by folder across 2-3 subagents and launch them in parallel. Subagents can't write report files, so they must return findings as text. Use the prompt template below.
3. **Verify.** Open the `file:line` of every HIGH finding and a few others yourself before trusting them. Drop any finding you can't confirm.
4. **Write `query_tracker.md`** at the root of the reviewed project (the git root), in the format below. If the file already exists, update it instead of overwriting (see "Re-runs").
5. **Reply in chat** with the 10-row summary table, the count of findings by severity, and a link to the tracker. Nothing more.

### Subagent prompt template

```
Read-only SQL review. Read the "The 10 rules" section of
~/.claude/skills/sql-review/SKILL.md and apply every rule to these files:
<file list>
Learn the placeholder style from the connection helper (pyodbc ?, psycopg %s, SQLAlchemy :name).
Don't edit anything and don't run queries. Return text only.

First give one line per rule: <rule #> | OK/PARTLY/ISSUE/N/A | one evidence file:line.
Then give one block per finding:
FINDING
rule: <1-10>
severity: HIGH (rule 7) | WRONG-RESULT (rules 5, 6) | PERF (rules 4, 8, 9) | STYLE (rules 1, 2, 3, 10)
where: <file:line>
problem: <one sentence>
before: <exact current snippet>
after: <proposed replacement>
check: <how to prove the change is safe, e.g. same row count on a sample, test name>
```

## The 10 rules

### 1. Formatting
Long queries (3+ clauses) should be multi-line: keywords uppercase, one column or clause per line, consistent identifier casing. Code is written once and read many times. Short one-line queries are fine; don't flag them.

### 2. Fetch only what you need
- `SELECT *` (except inside `EXISTS (SELECT * ...)`): wastes I/O and network, and breaks callers when a column is added.
- List queries that can grow without bound and feed a UI or a `fetchall()`: use `TOP (?)` or `OFFSET ... FETCH`. SQL Server has no `LIMIT`.

### 3. CTE over nested subqueries
Two or more levels of nested derived tables → suggest `WITH` steps. This is for readability only; SQL Server inlines CTEs, so don't claim a speed gain.

### 4. Index-friendly (sargable) predicates
Flag any function or expression wrapped around a column in `WHERE` / `JOIN ... ON`: `CAST`/`CONVERT(col)`, `YEAR(col)`, `UPPER`/`LOWER`/`TRIM`/`LTRIM`/`RTRIM(col)`, `ISNULL`/`COALESCE(col, x) = ?`, `col + 1 = ?`, and `LIKE '%x'` with a leading wildcard. The index stores the raw column values, so the optimizer can't seek once the column is transformed.
SQL Server exceptions, so don't overstate them: `CAST(datetime_col AS DATE) = ?` can still seek, though row estimates suffer (call it PARTLY, not ISSUE). `UPPER`/`LOWER` are usually pointless anyway because the default collation is case-insensitive.
Fixes: a range (`col >= ? AND col < ?`) for dates; store normalized data (backfill + `NOT NULL DEFAULT`) instead of `COALESCE`; or a computed column with an index on it.

### 5. DISTINCT must not hide a bad join
`SELECT DISTINCT` of parent columns after a 1-to-many `JOIN` is hiding duplicates the join created. Suggest `WHERE EXISTS (...)`. `COUNT(DISTINCT x)` and genuine unique-value lists are fine.

### 6. COUNT(*) vs COUNT(col)
`COUNT(col)` skips NULLs. Flag it only when the function name, docstring, variable or comment says "total rows" or "number of X". Counting non-null values on purpose is fine.

### 7. Parameterized queries (highest severity)
Values put into SQL through an f-string, `%`, `.format()` or `+` create a SQL injection risk, and every distinct value is a new statement that defeats plan reuse.
Don't flag these: identifiers spliced in from a whitelist or constant (name the whitelist's line), and generated placeholder lists such as `IN (?, ?, ?)`.

### 8. Composite index column order
Read the `CREATE INDEX` statements and compare them with the `WHERE` columns. An index helps only when its **leading** column appears in the query's equality predicate. SQL Server has no skip-scan, so `(A, B)` does not help `WHERE B = ?`.
Also flag:
- hot, frequently polled queries (queue claims, status polls) whose filter column has no index
- indexes that are an exact prefix of another index (redundant)

Phrase these as "likely", since you don't know the data volume.

### 9. Set-based, not row-by-row
- A loop that calls `cursor.execute` once per row for `INSERT`/`UPDATE` → `cursor.fast_executemany = True` + `executemany`, a single `UPDATE ... WHERE id IN (...)`, or `MERGE`.
- **N+1:** a query per item inside a loop over earlier results → one `IN`-list query.

### 10. UNION ALL over UNION
`UNION` sorts and removes duplicates. When the branches can't overlap (disjoint tables or filters) or duplicates don't matter, use `UNION ALL`. Keep `UNION` when deduplication is really needed.

## query_tracker.md format

Order the plan by severity: HIGH, then WRONG-RESULT, then PERF, then STYLE. Within a severity level, put hot paths first (queue polls, per-request queries).

````markdown
# Query Tracker

Reviewed: <YYYY-MM-DD> · Scope: <paths> · DB: <SQL Server / pyodbc `?`>
Progress: 0/<N> done

## Summary

| # | Rule | Verdict | Evidence |
|---|---|---|---|
| 1 | Formatting | OK/PARTLY/ISSUE/N/A | file:line |
...one row per rule, 10 rows...

## Plan

### Q1. <short title> (rule <n>, <SEVERITY>)
- [ ] Status: todo
- Where: `file:line`
- Problem: <one sentence>
- Before:
  ```sql
  <current snippet>
  ```
- After:
  ```sql
  <proposed snippet>
  ```
- Check: <how to verify after the change>
````

One `Q` block per change. When the same pattern repeats in several places, use one block and list every `file:line` under "Where".

### Re-runs

When `query_tracker.md` already exists:
- Keep the existing Q numbers and their `[x]` done marks.
- Mark an item the new review no longer finds as `- [x] Status: resolved (not found on <date>)`.
- Append new findings with the next free Q number.
- Update the Reviewed date, the Progress count and the Summary table.