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

[hanslemm]

Batch 3a of 3. Optional dbt enrichment for advise — and primarily a correctness fix, not a
feature: advise currently proposes DDL that dbt destroys.

Scope note: Batch 3 was originally Redshift + Snowflake + dbt. Two of those three cannot be verified
against a live engine (Redshift's svv_*/sys_* catalogs don't exist in Postgres; Snowflake needs a
real account), while every silent-suppression bug in this feature has been found live and none by
a fixture. So dbt — fully verifiable offline — went first. Redshift is Batch 3b; Snowflake waits for
an account rather than shipping an adapter whose SQL has never executed.

The correctness fix

dbt's table materialization drops and recreates the relation on every dbt run. So the
CREATE INDEX advise emits today for a dbt-managed table is destroyed by the next build — the tool
confidently advises something that silently reverts.

ADV302 rewrites those proposals into a commented indexes: config block instead. The three
materializations genuinely differ and distinguishing them is the rule:

materialized	raw CREATE INDEX	what ADV302 says
table	dropped every run	express as indexes: config
incremental	survives a run, lost on --full-refresh	express as config so a full refresh keeps it
materialized_view	rebuilt	express as config
view	cannot exist	not applicable; a view has no storage to index
anything else	unknown	say so, leave the DDL alone

ADV004's partial index is disclosed as not expressible rather than silently rewritten — dbt's
indexes config has no predicate field, so emitting one would lose the WHERE and turn a correct
proposal into a wrong one.

DROP INDEX on a dbt-managed relation is the mirror case, and it is handled: if the index is declared
in that model's config, the next dbt run recreates it, so the DDL file says to remove the config
entry too — otherwise the drop doesn't stick and gets proposed again next run.

Two rules that need the model graph

• ADV301 — materialize a view-backed model carrying a hot share of cost. Capped MEDIUM with no
  HIGH branch: the build-vs-read trade depends on a schedule query history cannot show. Verified
  unreachable by construction across a sweep of materializations × cost shares (including inf/nan).
• ADV303 — a model the workload never touched and that no model, snapshot or dbt exposure
  declares as a consumer. Capped LOW because the evidence is absence; states both the window and
  --limit caveats. Emits nothing on an empty workload — you cannot conclude a model is unused from a
  workload that observed nothing.

dbt stays optional, and that is measured

No adapter imports the dbt module; enrichment is one call above the adapter layer, so it will apply to
Batch 3b's Redshift adapter unchanged. A run with no manifest is byte-identical to main
across stdout, JSON, markdown, DDL and exit code — verified three separate times against a main
worktree, and now guarded by a regression test that reddens on an injected leak into any of the four
surfaces. The "dbt" payload key is omitted rather than set to null precisely so that identity is
literal rather than true-with-an-asterisk.

Model matching is on the exact (schema, table) pair with no bare-name fallback: a dbt target
schema routinely differs from the schema being introspected, and a name-only match would attribute a
production table to an unrelated dev model — then rewrite its DDL on the strength of it. A relation two
models both claim is dropped from matching and counted.

What the reviews found

Each task was reviewed and fixed; the whole-branch review then found 15 more, as it has on all three
predecessor branches.

The headline one: the entire feature could be disconnected from the CLI with CI green. Replacing
the enrich_proposals call with pass left all 665 tests passing, because the only check was an
integration test CI never runs. Closed at all four wiring sites, not just the one it was found at.

Two that were dangerous in the way this tool cares about: two config blocks for one model silently
lost a recommended index (duplicate YAML key — PyYAML keeps the last), proven live through the real
CLI; and the DDL file could hold a config block explaining that raw DDL is destroyed, with a bare
executable CREATE INDEX … WHERE …; on that same dbt table below it, because the warning lived only in
the rationale.

The relation parser was rewritten from a regex to a hand-written scanner after diagnosing that
re.findall silently skips characters it doesn't match — which is how every malformed name became a
confident wrong answer rather than a rejection. It is now total over str: verified across 16,105
adversarial inputs, never raising, always Relation | None.

Also fixed: load_dbt_context raised on wrong-typed JSON (a manifest that is a list, or has
nodes: null) and exited 1 with a traceback after all the catalog work; 42 of 92 mutations survived
the first fix wave, including five outright vacuous tests; and the _is_fully_commented guard on "no
emitted DDL line is executable-looking" had no test at all — all→any passed 665 tests while
emitting a bare DROP TABLE users;.

One mutation was disputed and the dispute upheld: .match() → .search() is an equivalent mutant,
since the regex is ^-anchored without re.MULTILINE. Confirmed across 1,483 inputs, including the
counterfactual that adding MULTILINE would distinguish them.

Limitation this branch acquired, and documents

advise now checks the manifest's dbt_schema_version and adapter_type, as check already did. A
foreign adapter_type means dbt is not building the introspected relations at all, so every match is a
name coincidence and all three dbt rules are wrong — it warns rather than silently advising. An absent
adapter_type warns too: dbt compile always writes one, so absence means a hand-written or truncated
manifest, and staying silent would make silence mean either "consistent" or "unchecked".

Verification

• 725 passed, 15 deselected — zero skips; no extras, no Docker
• 15 passed integration against live postgres:16
• ruff check, ruff format --check, mypy src/sqlquality clean

Follow-ups, not in this PR

• CI still never runs the 15 integration tests (no Postgres service in ci.yml). No longer
  load-bearing for ADV302 after the wiring guards, but worth a service container.
• ADV004 never discloses that it skipped coverage checks — carried from Batch 2.
• The integration compose binds host port 55432, which collides with another project's container on at
  least one dev machine; when that happens docker compose up doesn't bind and the suite talks to
  whatever else is listening.

🤖 Generated with Claude Code