/review-my-query

name: review-my-query
description: Use when a finished thing — a source, a result, code, or the record — is about to be trusted or consumed; the gate fires before the work leans on it. The code behind a number - a SQL query, view, or stored procedure, a dbt or semantic model, a measure, calc group, or RLS rule - needs reviewing for correctness before it ships or gets defended. Reviews the code as text against the locked definition, hunting the bugs that ship the wrong number: fan-out joins, grain and dedup, NULL and filter-context, timezone and late-arriving data, SCD, RLS leakage. Detects: "review my query/model", "check this SQL/proc/measure", "is this query right", "will this give the right number", "what's wrong with this query", "does this match the definition", "why don't these two numbers match" (with a query in hand). Within this family: the result the code produced is audit-my-experiment or audit-my-forecast; the whole record is kb-reconcile. It never runs the code and never writes the corrected production query.
allowed-tools: Read, Write

review-my-query

The colleague who reads your query before it ships: checks it against the definition you locked, hunts the bugs that quietly ship the wrong number, and hands you the findings, not a rewrite.

When to use

Fire when there's a piece of analytics code in hand — a SQL query/view/proc, a dbt or semantic model, a measure, calc group, or RLS rule — and the question is "is this right / will this give the right number" before it ships or gets defended. Triggers: "review my query", "check this SQL/proc/measure", "does this match the definition", "what's wrong with this", "why don't these two numbers match" (with the code in hand). Do NOT fire to orient on an unfamiliar estate (that's `groundwork`), to pin a metric's definition (`kpi-contract`), or to rehearse defending a finished number (`defend-my-number`). This reviews the code that computes a number; it does not orient, define, or rehearse. **This vs. its neighbors:** fire when the *code is suspect and the definition is trusted* — check the query/model against the locked contract. If the *definition itself* may be stale or wrong about the world (the code may be flawless), that's `audit-my-assumptions`.

The trap this exists to beat

Asked to "review this query," a capable assistant does the analytical part well: it spots the bugs and even checks the code against the contract fork by fork. Then it does the wrong thing with what it found. Its instinct is to **rewrite the query for you** — hand back a corrected `CREATE VIEW`, sometimes two versions — built on column and table names it **guessed** because it never saw the schema. That is a different job. It authors your production artifact (over the read-only line), does the work that's yours, and teaches you nothing about where the number was wrong. This skill **reviews**: it locates the defect, names the failure mode, ties it to the definition it breaks, grades it by whether it ships a wrong number, and points the fix *direction* — then leaves a committable review. It does not rewrite your query, and it does not invent the schema to do so. A second, quieter failure it avoids: **over-blocking** — grading every schema-uncertainty as Blocking to look thorough, so a conformant query comes back as a wall of red that trains the room to ignore your Blockings. It grades by what is *established*, reserves Blocking for a wrong number it can actually show, and lets clean code come back clean.

The loop

1. **Set the target + harvest the contract** — take the object under review as text. If a `knowledge-base/` exists, read `kpi-contract.md` (the definition to conform to — the sharpest anchor), `purpose.md`, `data-quality.md` / `notes.md` (known caveats), `landscape.md` (lineage). No contract? Review against the user's stated intent and **flag that the forks were never pinned** (hand to `kpi-contract`) — a missing contract is a finding, not a pass. 2. **Read the code as text** — build a model of what it computes: grain, join cardinality, filters and where they apply, time handling, nulls, set ops. For anything you can't see (a table's schema, what feeds it, what a constant means), **ask the user** — never connect to find out, never assume silently. 3. **Run the engine (two layers)** — see `references/failure-modes.md`:

• **Conformance:** walk each pinned fork of the contract (base, window, grain, inclusions/exclusions, time semantics, null handling, source-of-record, late-data rule); every departure is a finding.
• **Failure-mode taxonomy:** hunt the classic defects — grain/cardinality (incl. non-additive re-aggregation), filter/context, NULL/type, time, set logic, dimensional/SCD, security/scope, determinism.

4. **Grade each finding by ship-impact — and call it Blocking only when the wrong number is ESTABLISHED from what's in hand:**

• **Blocking** — read against the contract and standard SQL semantics, the code ships a wrong number you can show *without assuming an unknown*: it computes a different thing than the contract pins (counts logos where MRR is pinned), omits a contract-required transformation (no timezone conversion where Pacific is pinned), or has a logic error visible in the code itself (the cohort is built wrong). If there is **no plausible reading of the unseen schema under which the code is correct**, it is Blocking.
• **Latent / verify** — real ship-impact that is **conditional**: either correct-today-will-break (edge / SCD / late data / a filter that will rot), *or* a potential defect that hinges on a fact you cannot see (a table's grain, a column's type/nullability, whether a `status` value includes trials). State the assumption, the one discriminating check, and what it becomes if confirmed ("→ Blocking if `billing_daily` isn't unique per account-day"). It is a resolve-before-ship item — but you do **not** grade it Blocking on an assumption.
• **Advisory** — correct and robust, but unclear / unma