diff --git a/README.md b/README.md index de70f25..a997d32 100644 --- a/README.md +++ b/README.md @@ -7,5 +7,6 @@ Claude skills for RPA projects. Each folder in `skills/` is one skill: copy it i | `rpa-run-tab` | Adds a Tebra-style Run tab (Trigger button, Excel input, progress, live log, checkpoints) to a desktop PyQt6 bot in about 5 minutes. | | `rpa-dashboard-no-ai` | Builds a full dashboard (run panel, overview, report, optional checkpoints, admin and login) for a bot with no AI. Desktop or web. | | `rpa-dashboard-ai` | The same dashboard, plus confidence bands, review-and-approve and a cost/ROI page, for a bot that uses an LLM or OCR. Install it together with `rpa-dashboard-no-ai`. | +| `sql-review` | Reviews SQL code (SQL Server + pyodbc) against 10 rules through a subagent and writes every issue with a before/after fix plan to `query_tracker.md`. Read-only. | Run all tests: `uv run --with pytest --with playwright pytest tests` diff --git a/skills/sql-review/README.md b/skills/sql-review/README.md new file mode 100644 index 0000000..6fdff2b --- /dev/null +++ b/skills/sql-review/README.md @@ -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. diff --git a/skills/sql-review/SKILL.md b/skills/sql-review/SKILL.md new file mode 100644 index 0000000..13138ba --- /dev/null +++ b/skills/sql-review/SKILL.md @@ -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: + +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: | 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: +problem: +before: +after: +check: +``` + +## 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: · Scope: · DB: +Progress: 0/ done + +## Summary + +| # | Rule | Verdict | Evidence | +|---|---|---|---| +| 1 | Formatting | OK/PARTLY/ISSUE/N/A | file:line | +...one row per rule, 10 rows... + +## Plan + +### Q1. (rule , ) +- [ ] Status: todo +- Where: `file:line` +- Problem: +- Before: + ```sql + + ``` +- After: + ```sql + + ``` +- Check: +```` + +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 )`. +- Append new findings with the next free Q number. +- Update the Reviewed date, the Progress count and the Summary table.