Add sql-review skill
Read-only SQL review against 10 rules (SQL Server + pyodbc). The review runs in a subagent and every issue lands in query_tracker.md with a before/after fix plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
VasanthandClaude Opus 5.5 committed
1 parent
4e08a444eb
commit
3d2db6aca6
3 files changed
+214
No files matched your search
@@ -0,0 +1,84 @@
|
||||
# sql-review
|
||||
|
||||
A Claude Code skill that reviews SQL code against 10 rules and writes a fix plan to `query_tracker.md`. It never edits code and never runs queries.
|
||||
|
||||
Tuned for **SQL Server + pyodbc** (raw SQL strings in Python, `.sql` files, `CREATE INDEX` definitions). Other databases work too; the rules are applied in that dialect.
|
||||
|
||||
## Install
|
||||
|
||||
Copy this folder to `~/.claude/skills/sql-review/` and start a new Claude Code session.
|
||||
|
||||
## Use
|
||||
|
||||
In any project, type one of these:
|
||||
|
||||
- `/sql-review`
|
||||
- `/sql-review src/superbill/repositories`
|
||||
- "SQL check pannu" / "review the queries in run_repo.py"
|
||||
|
||||
## What it does
|
||||
|
||||
1. Finds the files that contain SQL. Tests and one-off migration scripts are skipped.
|
||||
2. Hands the review to a **subagent**, so file dumps stay out of the main chat. With more than 10 files it runs 2-3 subagents in parallel.
|
||||
3. Opens every HIGH finding at its `file:line` and confirms it. Findings it can't confirm are dropped.
|
||||
4. Writes `query_tracker.md` at the project root.
|
||||
5. Replies in chat with a short summary and a link to the tracker.
|
||||
|
||||
## The 10 rules
|
||||
|
||||
| # | Rule | Severity |
|
||||
|---|---|---|
|
||||
| 1 | Formatting: uppercase keywords, one clause per line | STYLE |
|
||||
| 2 | No `SELECT *`; page unbounded lists with `TOP` / `OFFSET FETCH` | STYLE |
|
||||
| 3 | CTEs instead of nested subqueries (readability only) | STYLE |
|
||||
| 4 | Index-friendly WHERE: no function around a column | PERF |
|
||||
| 5 | `DISTINCT` must not hide a bad join; use `EXISTS` | WRONG-RESULT |
|
||||
| 6 | `COUNT(*)` for total rows, `COUNT(col)` only to skip NULLs | WRONG-RESULT |
|
||||
| 7 | Parameterized queries (`?`): no f-string values | HIGH |
|
||||
| 8 | Composite index column order matches the WHERE | PERF |
|
||||
| 9 | Set-based writes: `executemany`, no per-row loops, no N+1 | PERF |
|
||||
| 10 | `UNION ALL` when duplicates can't happen | STYLE |
|
||||
|
||||
## query_tracker.md
|
||||
|
||||
It opens with a 10-row summary table. After that comes one block per change, ordered HIGH, then WRONG-RESULT, then PERF, then STYLE:
|
||||
|
||||
````markdown
|
||||
### Q1. SQL injection in orders_for_customer (rule 7, HIGH)
|
||||
- [ ] Status: todo
|
||||
- Where: `orders_repo.py:17`
|
||||
- Problem: customer_id goes into the SQL through an f-string.
|
||||
- Before:
|
||||
```sql
|
||||
f"... WHERE CustomerId = {customer_id}"
|
||||
```
|
||||
- After:
|
||||
```sql
|
||||
"... WHERE CustomerId = ?", customer_id
|
||||
```
|
||||
- Check: a valid id returns the same row count as before.
|
||||
````
|
||||
|
||||
Running the skill again updates the tracker without overwriting it:
|
||||
- Existing Q numbers and their `[x]` marks are kept.
|
||||
- Items that are fixed are marked "resolved".
|
||||
- New findings get the next free Q number.
|
||||
|
||||
To apply a fix, tell Claude which item, for example "Q1 fix pannu".
|
||||
|
||||
## Safety
|
||||
|
||||
- It is read-only: it doesn't change code and doesn't connect to any database.
|
||||
- `query_tracker.md` is the only file it writes.
|
||||
- It doesn't guess table sizes. Findings that only matter on large tables say so.
|
||||
|
||||
## Tested
|
||||
|
||||
On 2026-10-21 the skill ran against two sample repos:
|
||||
|
||||
| Sample | Result |
|
||||
|---|---|
|
||||
| A repo with one planted bug per rule | Found all 10, each with a correct `file:line` |
|
||||
| A clean repo with traps: whitelisted f-string column names, `IN (?, ?)` placeholder lists, `COUNT(DISTINCT x)` | 0 false alarms |
|
||||
|
||||
The update-on-re-run behaviour has not been tested yet.
|
||||
@@ -0,0 +1,129 @@
|
||||
---
|
||||
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.
|
||||
Reference in new issue
Block a user