feat(advise): optional dbt enrichment — stop proposing DDL that dbt destroys

CHANGELOG.md

@@ -35,6 +35,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   could no longer satisfy the hot query's `ORDER BY`.
 - ADV003 is scoped to the tables the workload was observed using, like ADV002 — it no longer
   proposes `DROP INDEX` for a relation the run never analysed.
+- Optional dbt enrichment for `advise`: `--project-dir` (reads
+  `<project-dir>/target/manifest.json`) or `--manifest <path>` layers dbt model metadata
+  onto the same analysis, and every manifest-free `advise` invocation is proven
+  byte-identical (stdout, markdown, DDL and stderr) to a run with no dbt support at all.
+  **ADV302** rewrites an index-creating proposal for a `table`-, `incremental`- or
+  `materialized_view`-materialized dbt model into a commented `indexes:` config block
+  instead of raw DDL, because a normal `dbt run` (or `--full-refresh`) drops and rebuilds
+  those relations and the DDL would not survive it; on a `view` the *DDL* is dropped and
+  explained while the proposal stays at LOW confidence, since a view has no storage to index
+  but "this index cannot apply here" is still the finding. ADV302 is a rewrite, not a
+  proposal code: the original rule keeps its code, confidence and cost share, so no proposal
+  ever carries `code: "ADV302"` — filter on `evidence.dbt_index_config` instead — and
+  `advise` prints a stderr line saying how many proposals it rewrote, since the terminal
+  table row is otherwise unchanged. Several index proposals for one model merge into a single
+  `indexes:` block, because dbt reads one `indexes` key per config and two blocks pasted into
+  one config are a duplicate YAML key whose loser is silently discarded. Wherever ADV302
+  declines and leaves executable DDL in place (a partial index, an unrecognised
+  materialization, no plain column list, a non-btree access method), the warning is written
+  into the `--ddl` script above the statement, not only into the rationale. A `DROP INDEX`
+  proposal (ADV002, ADV003) on a dbt-managed relation is the same hazard pointing the other
+  way — if the index is declared in the model's `indexes:` config, `dbt run` recreates it and
+  the drop silently reverts — so those keep their DDL and gain a warning, in the rationale and
+  in the DDL script, that the config entry has to be removed too. A manifest whose
+  `adapter_type` is neither postgres nor redshift (or is absent), or whose schema is not v12,
+  is disclosed on stderr: a foreign adapter means dbt is not building the relations `advise`
+  introspected at all, so every match is a name coincidence. **ADV301** proposes
+  materializing a `view`-backed model that carries a hot share of workload cost, capped at
+  MEDIUM. **ADV303** flags a dbt model within reach of the manifest that the analyzed
+  workload never touched and that no other model, snapshot or dbt exposure declares as a
+  consumer, capped at LOW. Matching a model to a relation is on the exact `(schema, table)`
+  pair with no bare-name fallback, since a dbt project's target schema routinely differs
+  from the schema being introspected; a relation two different models both claim is
+  dropped from matching (not guessed at) and counted in the CLI disclosure and the JSON
+  payload's `dbt.dropped_collisions`.
 
 ### Fixed
 

README.md

@@ -268,8 +268,9 @@ optimizations — indexes to add, indexes to drop, partial indexes, non-sargable
 predicates, and hot `SELECT *`. Output is an advisory report plus a DDL file for you to
 review. **`advise` never writes to your database and never executes DDL.**
 
-Only **Postgres** is implemented today; Redshift, Snowflake and dbt enrichment are
-designed but not built — see [Limitations](#limitations).
+Only **Postgres** is implemented today; Redshift and Snowflake are designed but not built
+— see [Limitations](#limitations). An optional dbt manifest enriches the same analysis —
+see [dbt enrichment](#dbt-enrichment-optional) below.
 
 ```console
 $ sqlquality advise --dsn postgresql://readonly@db.internal/analytics
@@ -312,10 +313,12 @@ missing driver degrades with an install hint instead of a traceback.
 | `--profile` | — | dbt profile name, read from `profiles.yml`. |
 | `--target` | — | dbt target within the profile. |
 | `--profiles-dir` | `~/.dbt` | Directory holding `profiles.yml`. |
+| `--project-dir` | — | dbt project dir; reads `target/manifest.json` to enrich proposals (optional). See [dbt enrichment](#dbt-enrichment-optional). |
+| `--manifest` | — | Path to a dbt `manifest.json`. Overrides `--project-dir`. |
 | `--schema` | `public` | Schema to introspect. Repeat for several: `--schema public --schema sales`. See Limitations for the ambiguity caveat. |
 | `--since` | — | Window, e.g. `7d`. **Not honored on Postgres** — see Prerequisites below. |
 | `--limit` | `500` | Max query-history rows to read. |
-| `--min-cost-share` | `0.01` | Suppress proposals below this share of workload cost. Applies to the **cost-weighted** rules (ADV001, ADV004, ADV005, ADV006, ADV007, ADV008); the index-hygiene rules **ADV002 and ADV003 carry no cost evidence and are always reported**, whatever the threshold. |
+| `--min-cost-share` | `0.01` | Suppress proposals below this share of workload cost. Applies to the **cost-weighted** rules (ADV001, ADV004, ADV005, ADV006, ADV007, ADV008, ADV301 — the last only with `--project-dir`/`--manifest`); the index-hygiene rules **ADV002 and ADV003**, and **ADV303** (its evidence is absence, not cost, so there is no share to threshold), carry no cost evidence and are reported whatever the threshold. ADV303 has its own non-threshold suppression: it emits nothing at all when no query usage could be extracted, since then every model would look untouched by definition. |
 | `--keep-literals` | off | Do **not** redact literal values from query text. |
 | `--timeout` | `30` | Statement timeout in seconds (rejected outside 1–3600). |
 | `--dry-run` | off | Print every statement the adapter would issue, then exit 0 **without connecting**. |
@@ -386,6 +389,19 @@ statement is not valid SQL to copy out and run.
 | ADV006 | Hot `SELECT *` on a wide table (≥15 columns) | cost share, column count |
 | ADV007 | Add index on a hot join key with no existing index leading with it | cost share, NDV, row estimate, absence of a covering index |
 | ADV008 | Composite index for a hot `GROUP BY`, column order inferred from cost, capped at MEDIUM | cost share, row estimate, absence of a covering index |
+| ADV301¹ | Materialize a `view`-backed dbt model that carries a hot share of workload cost, capped at MEDIUM | cost share, dbt model |
+| ADV303¹ | A dbt model within reach of the manifest that the analyzed workload never touched and no other model, snapshot or exposure declares as a consumer, capped at LOW | dbt model |
+
+¹ Only fires with `--project-dir` or `--manifest` loaded — see [dbt enrichment](#dbt-enrichment-optional).
+
+**ADV302 is not in that table, because it is not a proposal code.** It is a *rewrite*
+applied to another rule's proposal — ADV001, ADV004, ADV007 or ADV008 keeps its own code,
+confidence and cost share, and only its `ddl` and `rationale` change. So no proposal ever
+carries `code: "ADV302"`, and a `--json` consumer filtering on that code sees zero rows on
+every run; filter on `evidence.dbt_index_config == true` instead (present, and `true`, only
+on a proposal whose DDL was replaced by a dbt config block). The terminal table shows the
+original rule's row unchanged, so `advise` prints a line on stderr saying how many proposals
+ADV302 rewrote. See [dbt enrichment](#dbt-enrichment-optional).
 
 **Confidence model**, mechanical rather than judgment-based:
 
@@ -455,7 +471,8 @@ DROP INDEX "public"."idx_orders_customer_ref";
 ```
 
 `--json` emits the same evidence as a structured payload (`analyzed`, `degraded`,
-`engine`, `proposals`, `redacted`, `skipped`, `window`). This is the first proposal from
+`engine`, `proposals`, `redacted`, `skipped`, `window`, plus `dbt` when — and only when — a
+manifest was loaded). This is the first proposal from
 the run above — the real payload lists all five under `proposals`:
 
 ```console
@@ -543,6 +560,105 @@ exposes only rows for tables the current role owns or can select from — a role
 table access silently sees no statistics
 ```
 
+#### dbt enrichment (optional)
+
+Passing `--project-dir` (reads `<project-dir>/target/manifest.json`) or `--manifest
+<path>` layers dbt model metadata onto the same analysis. Neither is required: every
+`advise` invocation without one behaves exactly as documented above, and that no-manifest
+path is proven byte-identical (stdout, markdown, DDL and stderr) to a run with no dbt
+support at all — dbt is enrichment layered on top of an engine-agnostic core, never a
+requirement of it.
+
+**Why ADV302 exists.** The rules above propose DDL from query cost and catalog metadata
+with no idea whether the table they're indexing is dbt-managed — and if it is, that
+matters. dbt's `table` materialization drops and recreates its relation on *every*
+`dbt run`, so a raw `CREATE INDEX` applied once is silently gone the next time dbt runs.
+`incremental` differs only in degree: a normal run keeps the relation, but
+`dbt run --full-refresh` rebuilds it the same way. `materialized_view` behaves like
+`incremental` — refreshed in place on a normal run, rebuilt on `--full-refresh` or a config
+change dbt can't apply in place. A plain `view` has no storage of its own at all, so it
+cannot carry an index. Confidently advising DDL that a routine `dbt run` silently erases is
+worse than advising nothing, which is what **ADV302** exists to prevent: with a manifest
+loaded, an index-creating proposal for a `table`-, `incremental`- or
+`materialized_view`-materialized relation is rewritten into a commented dbt `indexes:`
+config block you paste into that model's own config instead of DDL you'd apply once and
+lose; on a `view` the **DDL** is dropped and explained instead (there is no relation to
+index) while the proposal itself stays, downgraded to LOW — "this index cannot apply here"
+is the finding; on any other or absent materialization the DDL is left untouched, since
+unrecognised is not the same as known-safe. A partial (`WHERE`-restricted) index has no
+config-block equivalent — dbt's `indexes` config carries no predicate — so that proposal is
+disclosed as not expressible rather than silently dropping the predicate.
+
+**One model, one `indexes:` block.** dbt reads a single `indexes` key per model config, so
+when a run recommends several indexes for the same model they are merged into one block,
+carried by the highest-ranked of those proposals; each of the others points at it by code
+instead of emitting a block of its own. Two standalone blocks pasted under one `config:` are
+a duplicate YAML mapping key, and PyYAML — dbt's own parser — resolves that by silently
+keeping one and discarding the other recommended index, with no error.
+
+**Whenever a statement is left executable for a dbt-managed relation** — the partial-index,
+unrecognised-materialization, no-column-list and non-btree paths above — the warning is
+written into the `--ddl` script itself, as comment lines directly above the statement, not
+only into the `rationale`. The DDL script carries no rationales, and it is the artifact a
+human actually applies.
+
+**A `DROP INDEX` proposal on a dbt-managed relation is the same hazard pointing the other
+way.** ADV002 and ADV003 read the catalog, not the manifest, so they will propose dropping an
+index that the model's `indexes:` config still declares — and the next `dbt run` puts it
+straight back, after which the tool proposes the same drop again. Those proposals keep their
+DDL (dropping a genuinely unused index is still right, and dbt's `indexes` config cannot
+express a removal) and gain a warning, in the rationale *and* in the `--ddl` file, that the
+config entry has to be removed as well or the drop will not stick.
+
+**`advise` checks the manifest against the connection**, the same two checks `check` makes on
+the same file: it warns when the manifest is not a v12 schema, and when its `adapter_type` is
+neither `postgres` nor `redshift`. The second matters more than the missing `indexes:` config
+key would suggest: a Snowflake or BigQuery manifest paired with a Postgres connection means
+dbt is not building the relations `advise` just introspected *at all*, so every match is a
+name coincidence and all three dbt rules are wrong — ADV302's premise that a `dbt run`
+rebuilds the relation included. A manifest recording **no** `adapter_type` warns too, since
+`dbt compile` always writes one and the honest statement is that the pairing could not be
+checked. `advise` warns rather than suppressing: the mismatch is something to fix in your
+invocation, and dropping all dbt output silently would hide it.
+
+**The block is rebuilt from the proposal's column list, not from its DDL**, and always as
+`type: btree`. That is faithful for every rule shipping today — each emits a plain btree over
+a column list with no `USING`, expression, `DESC`/`NULLS` or opclass — and a proposal naming a
+non-btree access method declines the rewrite rather than being flattened into a btree.
+Ordering, opclasses and expression indexes are *not* detected: a future rule emitting one
+would need this reconstruction extended alongside it.
+
+Two more proposals only fire with a manifest loaded — see the proposal table above for
+ADV301 and ADV303. Both are capped below HIGH, for the same reason ADV302's rewrite trusts
+the manifest as of whenever `dbt compile` last ran: a model's materialization or its
+consumers can change without a fresh compile, so a stale manifest degrades to a wrong (but
+traceable — the disclosed materialization or lack of a consumer names why) recommendation
+rather than a silent one.
+
+**Matching is exact, deliberately.** A model's `relation_name` is dropped down to its
+`(schema, table)` pair (dbt writes a `catalog.schema.table` name; the database part is
+discarded, since `advise` connects to one database at a time) and matched against the
+relation each proposal already carries — **there is no bare-table-name fallback**. A dbt
+project's target schema (`dev`, `main`, a CI schema, ...) routinely differs from the schema
+`advise` introspects in production, so matching on the table name alone would risk
+attributing a production table's proposal to an unrelated development model — and ADV302
+would then rewrite that table's DDL on the strength of a wrong guess. If two *different*
+models both build the same `(schema, table)` pair (legitimate when a project targets more
+than one database), `advise` cannot tell which is live: that relation is dropped from
+matching entirely — not guessed at — and counted, both in the CLI's `dbt enrichment from
+...` disclosure line and in the JSON payload's `dbt.dropped_collisions`.
+
+```console
+$ sqlquality advise --dsn postgresql://readonly@db.internal/analytics --project-dir ./my_dbt_project
+engine: postgres (credentials from --dsn)
+dbt enrichment from my_dbt_project/target/manifest.json (42 model(s))
+...
+```
+
+A manifest that is missing, unreadable or malformed degrades to "no enrichment" plus a
+line on stderr — `advise` never aborts an otherwise-successful run over an optional input,
+since by the time the manifest loads the whole catalog analysis has already run.
+
 ### check (the CI gate)
 
 Scores each changed model on both a candidate and a baseline dbt manifest, and gates
@@ -897,6 +1013,28 @@ LLM suggestions unavailable: The 'anthropic' package is required for AnthropicPr
   Qualify the table in the query, or run `advise` once per `--schema`, to recover it.
   Generated DDL is qualified with the schema it was read from, so it does not depend on the
   applying session's `search_path`.
-- **Redshift, Snowflake and dbt enrichment are designed but not implemented.** `advise`
-  supports Postgres only today; passing another `--engine` fails with a clear error
-  rather than silently degrading.
+- **Redshift and Snowflake are designed but not implemented.** `advise` supports Postgres
+  only today; passing another `--engine` fails with a clear error rather than silently
+  degrading. Optional dbt enrichment (`--project-dir`/`--manifest`, see
+  [dbt enrichment](#dbt-enrichment-optional)) is implemented for Postgres.
+- **dbt enrichment trusts the manifest as of its last `dbt compile`.** ADV302 rewrites DDL
+  based on a model's materialization as the manifest records it; a materialization changed
+  without a fresh `dbt compile` produces a stale — but traceable, since the disclosed
+  materialization names its own source — rewrite. Nothing verifies the manifest against
+  the live relation.
+- **A manifest for another warehouse is warned about, not rejected.** `advise` connects to
+  Postgres; a manifest whose `adapter_type` is something else (or absent) gets a stderr
+  warning and enrichment still runs, so a project whose manifest and target database disagree
+  gets dbt proposals built on `(schema, table)` name coincidences. dbt's `indexes` model
+  config is likewise implemented by the postgres and redshift adapters only, and ADV302's
+  rewrite is not translated per adapter.
+- **ADV302 reconstructs the index from the proposal's column list.** The emitted block is
+  always `type: btree` over that column list; column *ordering* is preserved but opclasses,
+  `DESC`/`NULLS` and expression indexes are not expressible, and a non-btree access method
+  declines the rewrite rather than being silently flattened. That last check is textual (no
+  rule records an access method in evidence), so a column whose *name* contains a `USING`
+  clause declines a rewrite that would have been fine — the safe direction.
+- **ADV303 only looks at a model's immediate consumers.** A dead model feeding another dead
+  model is not reported until the downstream one is gone, so a fully dead chain unwinds one
+  model per run, from its leaf. Conservative by construction: it never flags a model that
+  something declares a dependency on.