Rpa_Skills/skills/sql-review/SKILL.md

name, description

name

description

sql-review

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).

# 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.