chore(advise): run the integration suite in CI, and close the recorded follow-ups

.github/workflows/ci.yml

@@ -59,6 +59,121 @@ jobs:
             exit 1
           fi
 
+  # The 23 tests under tests/integration/ are deselected by default and, until this job
+  # existed, ran only on a contributor's own machine. That is not a theoretical gap: this
+  # feature's live suite repeatedly found bugs no fixture could reach — a `reltuples = -1`
+  # sentinel that read as "tiny table" and silently suppressed every proposal, redaction
+  # dismembering `$N` placeholders, a `toplevel` filter that made a hot function-wrapped query
+  # vanish while leaving confidently-wrong advice in its place, and a workload statement that
+  # failed on the wire for every default run. Each was invisible to `pytest` and to review.
+  integration:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: astral-sh/setup-uv@v5
+        with:
+          python-version: "3.12"
+      # Started with `docker run` rather than a `services:` container, and that is the whole
+      # reason this step exists in this shape. `pg_stat_statements` must be loaded by the
+      # postmaster at startup, but a service container cannot be given a command — GitHub's
+      # `services:` schema has no `command` key, and `options` is passed to `docker create`
+      # before the image, so the `-c shared_preload_libraries=...` flags cannot be expressed
+      # there at all.
+      #
+      # The first attempt worked around that with `ALTER SYSTEM` plus a restart, and it failed
+      # on the real runner: `ALTER SYSTEM SET pg_stat_statements.track` is rejected with
+      # "unrecognized configuration parameter" because that setting does not exist until the
+      # library is loaded, which is precisely what the restart was meant to achieve. Ordering
+      # the two calls correctly would need two restarts. Running the container directly is
+      # simpler, and — more importantly — takes the same flags as
+      # tests/integration/docker-compose.yml, so the server this job tests against is
+      # configured from one description rather than two that can drift.
+      - name: Start Postgres with pg_stat_statements preloaded
+        run: |
+          set -euo pipefail
+          docker run -d --name sqlquality-ci-pg \
+            -e POSTGRES_PASSWORD=sqlquality \
+            -e POSTGRES_DB=sqlquality_test \
+            -p 27432:5432 \
+            postgres:16 \
+            postgres -c shared_preload_libraries=pg_stat_statements \
+                     -c pg_stat_statements.track=all
+          # No health check gates a plain `docker run`, so wait explicitly. Failing here rather
+          # than letting the suite skip is the point: a server that never came up must not read
+          # as a pass, which is what the final step enforces.
+          for _ in $(seq 1 45); do
+            if docker exec sqlquality-ci-pg pg_isready -U postgres -d sqlquality_test; then
+              break
+            fi
+            sleep 2
+          done
+          docker exec sqlquality-ci-pg pg_isready -U postgres -d sqlquality_test
+          # Prove both settings took effect before any test runs, so a misconfiguration names
+          # itself here instead of surfacing as an error deep inside a fixture.
+          docker exec sqlquality-ci-pg psql -U postgres -d sqlquality_test -tAc \
+            "SELECT current_setting('shared_preload_libraries')" | grep -q pg_stat_statements
+          test "$(docker exec sqlquality-ci-pg psql -U postgres -d sqlquality_test -tAc \
+            "SELECT current_setting('pg_stat_statements.track')")" = "all"
+      # The `postgres` extra, since psycopg is what the live tests connect with. Not
+      # `--all-extras`: nothing here needs the llm extra.
+      - run: uv sync --extra postgres
+      - name: Run the integration suite
+        env:
+          # The fixture's own default DSN is this exact string, but it is set explicitly so a
+          # change to either side is a visible change here rather than a silent switch to
+          # whatever the fixture happens to default to.
+          SQLQUALITY_TEST_DSN: postgresql://postgres:sqlquality@127.0.0.1:27432/sqlquality_test
+        # `--strict-markers` catches a typo'd `integration` marker, which would otherwise
+        # select nothing. The junit XML is what the next step reads: pytest's own
+        # machine-readable count, not a regex over text meant for humans.
+        run: uv run pytest -m integration -q -rs --strict-markers --junitxml=integration.xml
+      # `pytest` exits 0 on skips, and every one of these tests skips itself when the server is
+      # unreachable — so a service that never became ready, a wrong port, or a marker typo would
+      # otherwise produce a green run that executed nothing at all. Same discipline as the
+      # `no-extras` job above, which refuses a skip for the same reason: an invariant that is
+      # only ever asserted by a test that did not run is not asserted.
+      #
+      # `if: always()` so a failing suite still reports *why* — a run that skipped everything
+      # and a run that genuinely failed need different fixes.
+      - name: Fail if the integration suite skipped or ran nothing
+        if: always()
+        run: |
+          uv run python - <<'PY'
+          import pathlib
+          import sys
+          import xml.etree.ElementTree as ET
+
+          report = pathlib.Path("integration.xml")
+          if not report.exists():
+              # Reachable because this step is `if: always()`: the pytest step can die before
+              # writing a report. Say so plainly rather than raising a traceback over it.
+              sys.exit(
+                  "::error::integration.xml was never written, so the previous step did not "
+                  "get as far as running tests. Its own log has the reason."
+              )
+          root = ET.parse(report).getroot()
+          suites = root.findall("testsuite") or ([root] if root.tag == "testsuite" else [])
+          if not suites:
+              sys.exit("::error::integration.xml has no testsuite element; pytest wrote nothing")
+          total = sum(int(s.get("tests", 0)) for s in suites)
+          skipped = sum(int(s.get("skipped", 0)) for s in suites)
+          failures = sum(int(s.get("failures", 0)) + int(s.get("errors", 0)) for s in suites)
+          executed = total - skipped
+          print(f"collected={total} executed={executed} skipped={skipped} failed={failures}")
+          if skipped:
+              sys.exit(
+                  f"::error::{skipped} integration test(s) skipped. These tests skip themselves "
+                  "when no Postgres answers, so a skip here means the service container never "
+                  "became ready, the published port does not match SQLQUALITY_TEST_DSN, or the "
+                  "server is not the one this suite expects. A skip must not read as a pass."
+              )
+          if executed == 0:
+              sys.exit(
+                  "::error::zero integration tests ran. Either the `integration` marker no "
+                  "longer selects them or the package collected nothing."
+              )
+          PY
+
   # `uv sync` installs what uv.lock pins, so every job above tests exactly one point in the
   # dependency ranges pyproject declares. A user running `pip install sqlquality` gets the
   # *newest* release satisfying those ranges instead — and that difference has already shipped

CHANGELOG.md

@@ -88,12 +88,50 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- CI now runs the 23 live-Postgres integration tests, in a job with a `postgres:16` service
+  container that has `pg_stat_statements` preloaded. Until now they ran only on a contributor's
+  own machine, while this feature's live suite was repeatedly the only thing that caught a
+  whole class of bug — a `reltuples = -1` sentinel that suppressed every proposal, redaction
+  dismembering `$N` placeholders, a `toplevel` filter that produced confidently-wrong advice,
+  and a workload statement that failed on the wire for every default run. The job fails if the
+  suite skipped or executed nothing, since every one of those tests skips itself when no server
+  answers and `pytest` exits 0 on a skip.
+- The integration compose file publishes host port 27432 instead of 55432, which collided with
+  an unrelated container in practice — and because `docker compose up` neither binds nor fails
+  in that case, the suite silently talked to whatever else was listening. The fixture now also
+  verifies which server answered (database name, and `pg_stat_statements` in
+  `shared_preload_libraries`) and fails naming a port collision as the likely cause, rather
+  than trusting that a successful connection reached the right database.
+- `--ddl`'s guarantee that every line of the generated script is either an intended statement
+  or a `--` comment now holds for all ten codepoints `str.splitlines()` treats as a line
+  boundary, on both the Postgres and Redshift renderers. The guard tested only `\n` and `\r`,
+  while everything that splits the text uses `splitlines()`, so an introspected identifier
+  containing `\v`, `\f`, `\x1c`, `\x1d`, `\x1e`, `\x85`, `U+2028` or `U+2029` — all legal
+  inside a quoted Postgres identifier — produced a second physical line the guard never
+  examined, and the tail of the statement was emitted looking like a bare statement of its own.
+- ADV004 (partial index) now consults the existing-index list, which it was alone among the
+  index-creating rules in never doing. A plain index leading with the guarded column now
+  suppresses the proposal — that index already serves the lookup, and the partial index's only
+  advantage is a size saving this tool cannot measure against a second index's write cost (and
+  which ADV003 would never flag as redundant, since its prefix check is restricted to plain
+  indexes). Where a check genuinely could not run it is now stated rather than skipped: an
+  unreadable existing-index list caps confidence at LOW and says so, and an existing *partial*
+  or expression index that leads with the same column is named, since sqlquality does not
+  compare index predicates and so cannot tell whether the proposal is already applied. New
+  evidence keys `partial_indexes_not_compared` and `expression_indexes`; deliberately not
+  ADV001's `partial_indexes_skipped`, which records a different fact.
 - dbt enrichment now discloses itself in the terminal on **every** engine. The stderr
   disclosure line counted only ADV302's config-block rewrite, which no Redshift proposal can
   reach (nothing Redshift emits is a `CREATE INDEX`), so a `--project-dir` run on Redshift
   warned in `rationale` and in the `--ddl` note that `dbt run` may undo an hours-long
   full-table rewrite while the terminal row stayed byte-identical to a dbt-free run. Any
-  proposal whose DDL cannot be expressed as dbt config is now counted and reported too.
+  proposal whose DDL cannot be expressed as dbt config is now counted and reported too, as is
+  an index *drop* (ADV002, ADV003) on a dbt-managed relation — the case reachable on Postgres,
+  where a run proposing only drops enriched every one of them and still said nothing, so an
+  operator applied a drop that the next `dbt run` recreated from the model's `indexes:` config.
+  Each of the three outcomes is reported as its own clause, because each calls for a different
+  action: paste a config block, expect a runnable statement not to survive the next rebuild, or
+  delete a config entry as well as running the drop.
 - `IS NOT NULL` predicates were classified as `IS NULL` when sqlglot 30.13 or newer was
   installed, because that release moved the negation from a wrapping `Not` node onto a
   `negate` flag on the `Is` node itself. Both encodings are now read. This was not cosmetic:

CONTRIBUTING.md

@@ -62,4 +62,11 @@ docker compose -f tests/integration/docker-compose.yml down
 
 These are deselected by default, so `uv run pytest` stays green without Docker. Point them
 at your own server with `SQLQUALITY_TEST_DSN`. They need the `postgres` extra
-(`uv sync --extra postgres`).
+(`uv sync --extra postgres`), and a server with `pg_stat_statements` in
+`shared_preload_libraries` — `CREATE EXTENSION` alone is not enough, which is why the compose
+file passes it as a server flag.
+
+The compose file publishes host port **27432**. If something else already holds it, `docker
+compose up` neither binds nor fails, and the suite would talk to whatever is listening — so
+the fixture checks which server answered and fails with that diagnosis rather than producing
+a puzzle. Free the port (`docker ps --filter publish=27432`) or set `SQLQUALITY_TEST_DSN`.

README.md

@@ -410,7 +410,7 @@ statement is not valid SQL to copy out and run.
 | ADV001 | Composite index candidate: hot equality columns, then one range/sort column, arity ≤ 3, and only columns some single query group filters on *together* | cost share, NDV, row estimate, joint co-occurring fingerprint count, absence of a covering index |
 | ADV002 | Drop an index with zero recorded scans (excludes unique/primary-key indexes) | scans since last stats reset, size |
 | ADV003 | Drop an index whose column list is a strict prefix of a wider index | both column lists |
-| ADV004 | Partial index: a hot equality column guarded by a hot, co-occurring `IS [NOT] NULL` check | cost share, co-occurring fingerprint count |
+| ADV004 | Partial index: a hot equality column guarded by a hot, co-occurring `IS [NOT] NULL` check | cost share, co-occurring fingerprint count, absence of a plain index leading with the guarded column |
 | ADV005 | Non-sargable predicate — a cast/function on a column, or a leading-wildcard `LIKE` | cost share |
 | 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 |