sql-review · diff
git:20260701.a5145aa to git:20261005.8662b6e
60 added, 53 removed. Audit A to A.
---
name: sql-review
- description: Bootstrap or extend an app's sql_review/ directory — for each query in queries/*.sql, write a paste-and-runnable .review.sql (EXPLAIN + bounded row-count + freshness) and a README mapping each query to its upstream object. Read-only. Use when the user says "bootstrap sql_review", "make a SQL review directory", "fill the NOT covered yet gap", "extend the sql_review folder", or "/sql-review".
+ description: Prove an app's numbers against live Snowflake and leave a record a person signs. Builds or repairs the app's sql_review/ page files, checks objects, grants and DDL drift, runs every section as aggregates, has reviewer agents judge the logic, drops findings the evidence does not support, and writes the committed review log. Use when the user says "review the SQL", "are these numbers right", "trace the data", "audit the lineage", before a release or deploy, or from /review-app --sql. It spends warehouse credits, so run it when asked, not on your own initiative.
+ argument-hint: "<slug> [--page NN] [--offline] [--optimize] [--role ROLE] [--connection NAME]"
+ allowed-tools: [Bash, Read, Edit, Write, Glob, Grep, Task]
---
- # sql-review
-
- Author or extend `apps/<slug>/sql_review/` so every UI-feeding query has an audit-ready, paste-and-runnable companion plus a lineage README. **Read-only** — this never mutates Snowflake and never deploys. It is a generator: the qualitative judgment about whether the data is *right* lives in /deep-dive-data, the static code review in /review-app, and the deterministic ship gate in `streamsnow validate-app`.
-
- ## When to use this vs. its siblings
-
- - `/sql-review` (here) — you want *just* the paste-and-runnable companions + lineage index for an app. Mechanical, single-pass, no opinions.
- - /review-app and /deep-dive-data run this same bootstrap automatically when they detect a `sql_review/` gap, so you rarely need to invoke it by hand. Reach for it directly when you want the scaffolding without the surrounding multi-dimension review.
- - If you need a verdict on data correctness (real column sets, view-vs-base-table, predicate pushdown, cost), that is /deep-dive-data, not this skill.
-
- ## Steps
-
- 1. **Resolve `<slug>`.** If omitted, list `apps/*/` and ask which one. Confirm `apps/<slug>/queries/` exists. If there are no `queries/*.sql`, stop — there is nothing to review (see Edge cases for apps that inline all SQL).
- 2. **Compute the gap.** Enumerate sources with `ls apps/<slug>/queries/*.sql`, then existing companions with `ls apps/<slug>/sql_review/*.review.sql 2>/dev/null`. The queries with no matching `.review.sql` are the work. Leave existing companions untouched unless the user explicitly asks to refresh them.
- 3. **Read each gap query.** Parse its header block (Query / Feeds / Schemas / Params) and its body to learn the upstream `db.schema.object`(s), the bind params (`:1`, `:2`, …), and any `{TOKEN}` fragments the app substitutes at render time.
- 4. **Confirm a connection before any live check.** Run `streamsnow doctor` (or `snow connection list`). If none resolves, you can still write everything from static analysis — see step 7 — so this is a branch, not a hard stop.
- 5. **Write `apps/<slug>/sql_review/<name>.review.sql`** for each gap query — paste-and-runnable in Snowsight, three bounded read-only sections (see Companion shape). Substitute concrete sample literals for every `:N` bind and `{TOKEN}` so the file runs top-to-bottom as-is. Never leave a placeholder that errors on paste.
- 6. **Verify upstream lineage (connection present).** For each upstream object: confirm it resolves with a zero-row probe `snow sql -q "SELECT COUNT(*) FROM <db>.<schema>.<object> WHERE 1=0"`, and capture type + columns from `INFORMATION_SCHEMA.TABLES`/`COLUMNS`. Cap any sampling with `LIMIT`; never run an unbounded `SELECT *`. No DDL, no writes. Mark each row **verified**.
- 7. **Verify-or-defer (no connection).** With no connection, write the companions and README from static analysis only, mark every upstream lineage row **unverified**, and tell the user to re-run after `streamsnow configure` + `snow connection add`. Do not fabricate column lists.
- 8. **Write/refresh `apps/<slug>/sql_review/README.md`** as the full index: one row per query → upstream `db.schema.object` → feeds (from the header) → review-file name → lineage status (verified / unverified). Carry over rows for pre-existing companions so the README stays complete, not just the delta.
- 9. **Report the coverage delta:** queries total, companions now present, any still uncovered, and whether lineage was live-verified or static-only.
-
- ## Companion shape (`<name>.review.sql`)
+ # /sql-review
- Open with a comment naming the source query and its upstream object(s). Then three labelled, bounded, read-only sections — paste the whole file into a Snowsight worksheet and run top to bottom:
+ > **Repo overlay:** if `.streamsnow/overlays/sql-review.md` exists in this repo, read it first — committed, repo-specific additions/overrides ([_shared/overlays.md](../_shared/overlays.md)). Outside Claude Code, also read [_shared/other-agents.md](../_shared/other-agents.md).
- 1. **EXPLAIN** — `EXPLAIN USING TEXT <the query with sample literals substituted>;` to inspect pruning and join order without scanning rows.
- 2. **Row-count** — `SELECT COUNT(*) …` over the same predicate window to sanity-check cardinality. Keep the window narrow so it stays cheap.
- 3. **Freshness** — `SELECT MAX(<date/ts col>) …` per upstream object so the reviewer sees how current the data is.
+ Every fact comes from a `streamsnow sql-review` command as JSON with a stable `id`; agents only
+ judge, every finding cites those IDs, a verifier drops what the evidence does not support, and
+ `log` refuses anything uncited. Judgment, never a gate: `streamsnow validate-app` alone passes or
+ fails a ship. Run it on request and before a release or deploy.
- ## Decision guidance
+ ## Preflight
- - **Sample literals must satisfy the predicates.** A bind that feeds `WHERE created_at >= :1` needs a date that actually returns rows in the row-count section, or the reviewer can't tell a correct query from an empty one. Pick literals from the freshness window, not arbitrary constants.
- - **One companion per source query, not per upstream object.** A query that joins three tables gets one `.review.sql` with three freshness checks — mirror the query's own shape so the lineage is obvious.
- - **Pick the freshness column the query filters on.** If the query prunes on `event_date`, the `MAX()` should be on `event_date`, not on a load-timestamp column — that's the column whose currency actually affects the UI.
- - **Verified beats unverified, but unverified still ships value.** A static-only `sql_review/` with an honest "unverified" column is useful and auditable. Don't block on a missing connection; just be explicit about what wasn't checked.
+ 1. **Resolve the slug**; read the app's `AGENTS.md` (Data notes) and `REQUIREMENTS.md`.
+ 2. **Offline gate:** `streamsnow sql-review check <slug>`. No `index.yaml`, coverage gaps or
+ `marker` findings → build the index first, per [authoring.md](authoring.md). Drift →
+ `streamsnow sql-review generate <slug>`, committed on its own: the live commands refuse page
+ files that do not match what the app runs. `--offline` stops here and reports the coverage.
+ 3. **Connection:** `streamsnow doctor`. None → stop after step 2 and name the enabler
+ (`streamsnow configure`, then `snow connection add`); never invent results. The live commands
+ run as `snowflake.roles.ci_role` (the deployed app's role) with secondary roles off; if the
+ user does not hold it, ask which role to use and pass `--role`.
- ## Notes
+ ## Facts (JSON only)
- - **READ-ONLY always:** EXPLAIN, COUNT, MAX, `INFORMATION_SCHEMA`. Refuse to emit INSERT / UPDATE / MERGE / CREATE / DROP into a review file.
- - **Respect governance.** Companions read the same approved objects the query does — never reach past the schema allowlist (`governance.schema_allow` / `schema_deny` in `streamsnow.config.yaml`). If unsure a referenced object is allowed, run `streamsnow check schema-refs apps/<slug>`.
- - **`sql_review/` is review scaffolding, not app code.** It is not deployed and not loaded by the app's `sql_loader`. Editing it never changes what ships.
- - **Qualified names follow config.** Companions resolve against `governance.database` and the app's configured warehouse/role — refer to them generically (`<database>.<schema>.<object>`), never hardcode environment-specific values.
+ 4. `streamsnow sql-review probe <slug>`: objects exist, direct grants, DDL drift, every section
+ compiles with its columns. Keep the `run_id` it prints; pass `--run <run_id>` from here on.
+ 5. `streamsnow sql-review run <slug> --run <run_id>` (`--page NN` for one page): rows, totals,
+ an order-insensitive hash and timing per section, computed in Snowflake.
+ 6. Read these outputs, never result rows. Do not run sections yourself, and never call `snow sql`
+ directly: the commands guard every statement before it is sent.
- ## Edge cases
+ ## Judgment
- - **Empty `queries/` directory** (legacy app that inlines all SQL in Python). There's nothing to bootstrap. Stop and suggest the user externalize queries into `queries/*.sql` first, then re-run.
- - **Query has no `{TOKEN}` placeholders.** Fine — the rendered companion is byte-identical to the source SQL (no substitutions). The EXPLAIN/COUNT/freshness sections and the lineage README are still worth having; no special handling.
- - **Existing `sql_review/` plus newly added queries** (the common extend case). Only fill the gap. Don't regenerate companions the user already reviewed unless they ask.
- - **Auth expires mid-run.** The first failing `snow sql` probe returns non-zero. Stop live verification there, finish the remaining companions static-only, mark their rows **unverified**, and tell the user to re-run after `snow connection test` to upgrade them to verified.
- - **Object resolves but has zero rows in the predicate window.** That's a finding, not an error — note it in the README so a reviewer knows the UI feature may render empty. It is not this skill's job to fix it; hand to /deep-dive-data.
+ 7. **In parallel** (Claude Code: one message, several Task calls): agent
+ `streamsnow:sql-review-page` once per page, and `streamsnow:sql-review-object` once per object
+ in `sql_review/app_specific_reporting_objects/`. Without subagents, follow
+ [reviewers/page.md](reviewers/page.md) and [reviewers/object.md](reviewers/object.md) yourself,
+ one brief at a time. Each brief names its inputs, the one file it writes, and its result.
+ 8. **Optimizer** ([reviewers/optimizer.md](reviewers/optimizer.md)) for each section `run` marked
+ `slow` (over 10 s), or for all with `--optimize`. Every rewrite it proposes is backed by
+ `streamsnow sql-review bench <slug> --run <run_id> --metric NN#n --sql-file <candidate>` with
+ `equivalent: true`. It proposes diffs, never applies them, never suggests a bigger warehouse.
+ 9. **Verifier** ([reviewers/verifier.md](reviewers/verifier.md)), a fresh agent per page batch: it
+ re-reads every cited result, tries to refute each finding, and keeps or drops it with a reason.
- ## Troubleshooting
+ ## Log
- - **Probe returns "does not exist or not authorized."** Either the object is genuinely missing or the configured role lacks grants. Run `streamsnow check schema-refs apps/<slug>` to confirm the reference is allowed, then check the role's grants. Mark the row **unverified** until resolved — don't guess columns.
- - **`streamsnow doctor` passes but `snow sql` fails.** doctor checks local prerequisites, not live auth. Run `snow connection test` for the live check; refresh the connection if it's expired.
- - **Companion errors on paste** (`{TOKEN}` or `:N` left in). A placeholder slipped through step 5. Substitute a concrete sample literal that satisfies the predicate and re-verify the EXPLAIN parses.
+ 10. Merge the kept findings into `findings.json` in the run directory, shaped per
+ [findings.md](findings.md). Validate with `streamsnow sql-review log <slug> --run <run_id>
+ --findings <file> --dry-run`; a refusal names the finding and why. Fix the citation or drop
+ the finding; never invent evidence.
+ 11. Write it: the same command without `--dry-run`. It writes
+ `sql_review/review_log/YYYY-MM-DD_<shortsha>.md` and updates the README's latest review.
+ 12. `streamsnow sql-review check <slug>`, then commit the log and `sql_review/README.md` alone:
+ `chore(sql-review): <slug> review <YYYY-MM-DD>`. Fixes come later, in their own commits,
+ after the user agrees to each.
+ 13. **Report:** the log path, blocker / major / minor counts and the top findings. Tell the user
+ the sign-off block (Reviewer, Date, Decision) is theirs to fill in. Never fill it in.
- ## Hand-offs
+ ## Rules
- - /review-app and /deep-dive-data run this bootstrap automatically when they detect a gap; invoke /sql-review directly to fill the gap in isolation.
- - Lineage findings here feed /deep-dive-data's live-DB tracing — the qualitative judgment lives there, not in this generator. If a probe surfaces a real correctness concern, route it to /deep-dive-data, which emits `BLOCK`/`FLAG`/`NICE-TO-HAVE` findings that /apply-review and /auto-review-app consume.
- - /validate-app (`streamsnow validate-app <slug>`) remains the only deterministic ship gate; a populated `sql_review/` supports a review but does not replace that gate.
+ - No row-level data or small-group breakdowns in a finding, the log, or chat: pass or fail, row
+ counts, top-level totals.
+ - A human applies DDL. Propose it; deploy it only when the user explicitly says to.
+ - Data judgment follows [tracing.md](tracing.md): never claim upstream is broken without cited
+ evidence, and tell "missing" from "not visible to this role" apart.
+ - Exit codes: `1` means a check failed (report it as a fact); `2` means nothing ran (fix the
+ cause: a stale page file, the scaffold placeholder, a refused statement, no connection).
## Done when
- Every `queries/*.sql` has a paste-and-runnable READ-ONLY `<name>.review.sql` (EXPLAIN + bounded row-count + freshness, with concrete sample literals), `sql_review/README.md` maps each query to its upstream object with a verified / unverified status, and the coverage delta is reported.
+ The log is committed with every finding cited and verified, check is clean, and the user knows
+ the sign-off is theirs. With `--offline`: check is clean or its gaps are reported.