chore(advise): run the integration suite in CI, and close the recorded follow-ups - #16
Merged
Merged
Conversation
`describe_rewrites` exists because neither thing dbt enrichment does shows up in a terminal row: an enriched proposal is byte-identical to the same proposal from a dbt-free run. It counted the ADV302 config rewrite and the generic non-index warning, but not the third outcome — a `DROP INDEX` on a dbt-managed relation, where the note says `dbt run` may recreate the index from the model's `indexes:` config. That branch is reachable on the adapter this module was written for, not a hypothetical: a Postgres run whose only proposals for dbt-managed relations are ADV002/ADV003 drops enriches every one of them, in `rationale` and in the `--ddl` note, and the terminal printed nothing. The operator applies the drop, dbt puts the index back, and the same proposal returns on the next run — the silently-reverting advice ADV302 exists to eliminate, pointing the other way. Counted off its own evidence flag and reported as its own clause rather than folded into the "cannot be expressed as dbt config" one: that clause tells the operator to expect a runnable statement not to last, while this one tells them to delete a config entry as well, which is the opposite action. This changes Postgres stderr deliberately. The previous round forbade that in order to prove an extraction had not altered behaviour; that constraint has served its purpose. No existing assertion was weakened — the two existing counts keep their exact wording, and the new test pins that the drop count discriminates this branch from both of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…kipping it ADV004 was the only index-creating rule that never called `_covered` at all. It could propose a partial index that an existing one already serves, and its rationale said nothing about the gap — a check that silently did not run, which is the one thing this rule set is built to refuse. Both options were honest; this takes the suppressing one, with disclosure where suppression is not supportable. Suppress on a plain index leading with the guarded column. `WHERE status = $1 AND shipped_at IS NOT NULL` is already served by a plain index on `(status, ...)` with the null check applied as a filter, so the partial index adds no access path — only size. Size is precisely what this tool cannot measure: nothing here knows what fraction of the table satisfies the guard, so "a fraction of the size" would be an assertion traded against a second index's write cost on every write. Nothing downstream would catch the pair either: ADV003's redundant-prefix check is restricted to plain indexes, so a partial index shadowed by a plain one is never flagged on any later run. Disclose, rather than suppress, where the check genuinely cannot answer: * `have_index_data=False` (catalog query denied) caps confidence at LOW and names the skipped check, exactly as ADV001/ADV007/ADV008 do. * An existing *partial* index leading with the same column is named. `_covered` excludes partial indexes deliberately, and for this rule that exclusion cuts the other way than for ADV001: such an index may be precisely this proposal already applied. Predicates are not compared — the existing WHERE clause is not parsed, and this proposal's guard is reconstructed from redacted usage — so it is reported as unknown, by name. * An expression index mentioning the column is named, the same gap the other three rules already disclose. Evidence gains `partial_indexes_not_compared` and `expression_indexes`. Deliberately not ADV001's `partial_indexes_skipped`: there the partial index is known not to cover an unfiltered lookup, here nobody compared the predicates, and `--json` renders evidence as bare k=v pairs with no per-rule text to distinguish them. This changes Postgres output deliberately: an ADV004 proposal covered by a plain index disappears, and the rationale gains sentences in three previously silent cases. No existing assertion was weakened — a surgical mutant that removes only the `_covered` call kills exactly one test (the new suppression test) and leaves the other 935 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ning codepoints Both `render_ddl` implementations guarded with `"\n" in ddl or "\r" in ddl`, while every place that actually splits the text — `_comment_lines`, `_is_fully_commented`, and the tests asserting no bare line is ever emitted — uses `splitlines()`. That splits on eight further codepoints: \v, \f, \x1c, \x1d, \x1e, \x85, U+2028 and U+2029. Postgres permits every one of them inside a quoted identifier, so an introspected name carrying one produced a second physical line in the generated file that the guard never examined: the NOT-RENDERED fallback was skipped and the tail of the statement was written out looking like a bare statement of its own — in the one file whose stated purpose is that nothing unintended is executable. Closed for both adapters by deriving the predicate from `splitlines()` itself (`_has_line_break`) rather than restating its character set. That keeps the guard and the splitting in lockstep by construction: there is no second list to forget, and a future CPython recognising one more boundary cannot reopen the hole. Empty text answers False — no lines is not a line break, which the bare `splitlines() != [text]` comparison gets wrong. Pinned one parametrized case per codepoint per adapter, not one case built from a string containing all eight: a guard handling six of them would still trip on such a string and pass. Each case also asserts its own premise (`len(ddl.splitlines()) == 2`), so a codepoint that turned out not to be a boundary would fail loudly instead of passing vacuously. Verified by mutation: restoring the old two-character expression at both call sites kills exactly 16 tests, 8 per adapter, one per codepoint, and reducing `_has_line_break` itself kills those 16 plus its own unit test — with the unmutated tree green. Behaviour is unchanged on both adapters for every identifier that does not contain one of the eight, so no existing test needed adjusting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rver answered The compose file published 55432, which an unrelated `postgres:16` container held on a development machine for the whole of this feature's work. When that happens `docker compose up` does not bind and does not fail: it reports success, the port keeps belonging to whatever bound it first, and every test in the package silently talks to a stranger's database. It surfaced as a password-authentication failure that the author and three reviewers all read as a code bug. Both halves of the fix, because either alone leaves it silent: * The published port is now 27432 — unregistered, outside the ephemeral range on Linux (32768+) and macOS (49152+) so no outbound socket can take it first, and not one of the ports Postgres tooling gravitates to. * `live_dsn` verifies it reached *this* suite's server instead of trusting the connection. Two cheap reads in one round trip: the database name, and `shared_preload_libraries` containing pg_stat_statements. A mismatch is a hard failure — a skip is how the original problem stayed invisible — naming a port collision as the likely cause and how to look for it, since compose's own output never mentions it. An unreachable port stays a skip: "no Docker" remains a supported state for the default suite. The second check is the one that matters. Measured against a plain `postgres:16` carrying the same database name, the old `SELECT 1` check passed, `CREATE EXTENSION pg_stat_statements` succeeded, and the first real failure came a hundred lines into the seed fixture as "pg_stat_statements must be loaded via shared_preload_libraries" — an error that says nothing about the port. Both failure modes were reproduced live against real containers and now fail at the fixture with the diagnosis. The messages are also credential-safe, since CI is about to print them: the DSN is described as `host:port/database` rather than echoed, and the driver's own text is passed through the project's own `scrub` — an authentication failure is the most common real connect failure and the one place a password can surface. A keyword-form DSN is described generically rather than dissected, because `urlparse` puts the whole string, password included, in `path`. The checks are module-level pure functions so the default suite can exercise the server-is-wrong path without Docker: a check that only runs when the server is right is a check nobody ever sees run. A drift test pins the fixture's port against the port compose actually publishes, since two files naming one port with nothing tying them together is how this becomes invisible again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 23 tests under tests/integration/ were deselected by default and ran only on a contributor's own machine. That gap was not theoretical: this feature's live suite was repeatedly the only thing that caught a bug 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 behind, and a workload statement that failed on the wire for every default run. Each was invisible to `pytest` and to review. A service container cannot be given a command: GitHub's `services:` schema has no `command`/`entrypoint` key, and `options` is passed to `docker create` before the image, so the compose file's `-c shared_preload_libraries=...` flags cannot be expressed there. The service therefore provides the lifecycle and the health check, and a step applies both settings with ALTER SYSTEM and restarts the container — a reload is not enough, since both are postmaster-level and `pg_reload_conf()` leaves them inactive with no error. One `psql -c` per statement, because psql wraps multiple `-c` options in a single implicit transaction and ALTER SYSTEM cannot run inside one. The step then asserts both settings took effect, so a failure names the cause instead of surfacing deep inside a seed fixture. The job fails if the suite skipped or executed nothing. That is the load-bearing part: every test in the package skips itself when no Postgres answers, and pytest exits 0 on a skip, so a service that never became ready, a wrong port, or a marker typo would otherwise produce a green job that ran nothing at all — the same failure the `no-extras` job refuses, for the same reason. The count comes from pytest's own JUnit XML rather than a regex over output written for humans. **The job itself is unverified: it cannot be run locally and will only be proven by a run on a PR.** What is verified is everything reachable without GitHub. ci.yml parses; the guard script is extracted and executed against real pytest JUnit reports, where it accepts a full run, rejects a run containing a skip, rejects a run that collected nothing, and explains a missing report instead of raising; the heredoc terminator is asserted to sit at column 0, since an indented one would make the shell read to EOF. The wiring most likely to break silently — port, database name, image, health check, both server settings, the restart, `-m integration`, `--strict-markers`, and the guard's `if: always()` — is pinned against the fixture's own constants, so CI and the compose file cannot drift apart. Six separate one-line mutations of ci.yml were each measured to kill exactly one test, and the right one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ey concern Neither is fixed, deliberately, and both are the kind of thing that gets lost in a branch summary rather than found again in the source. `_prepend_note` covers 2 of `_classify`'s 5 `note=` sites. The other three build their note from `_dbt_ddl_note` directly and would discard an existing note — but none is reachable today: all three are `CREATE INDEX` paths, and the only rule that sets a note before enrichment (Redshift's ADV105) emits no index DDL and so lands in the generic branch, which already routes through `_prepend_note`. Fixing them now would add three paths no test can reach. The generic non-index branch is exercised only from the Redshift side. It keys on the DDL prefix rather than the adapter, so it is engine-agnostic by construction, but a future Postgres rule emitting non-index DDL would land there with no test of its own. Writing one now would mean inventing a proposal shape no rule produces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The integration job failed on its first real run with `ERROR: unrecognized configuration parameter "pg_stat_statements.track"`. The cause is an ordering problem that cannot be ordered away: a GitHub service container takes no command (the schema has no `command` key, and `options` is passed to `docker create` before the image), so the two `-c` flags the compose file uses could not be expressed there. The workaround applied both settings with `ALTER SYSTEM` and one restart — but `pg_stat_statements.track` does not exist as a parameter until the library is loaded, which is what the restart was meant to accomplish. Setting them correctly would need two restarts. `docker run` takes the flags directly, and takes the *same* flags as tests/integration/docker-compose.yml, so the server CI tests against is now described once rather than twice. The cost is that nothing gates the job's steps on readiness, so the wait is explicit and the final `pg_isready` outside the retry loop is what turns a server that never came up into a failure rather than a skip. Rehearsed locally with the exact commands: the container starts, both settings read back as expected, and all 23 integration tests pass against port 27432. The structural pins move with it — `test_ci_integration_job.py` now reads the server step's shell text rather than a `services:` mapping, keeping every guarantee it had (image, port, database, readiness wait, both flags) and gaining one: that the flags appear as command flags, since that is the only form that works here. Worth recording that the guard step did its job on its first outing: it refused to let a run that never reached pytest look like a pass, and said so in one line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The follow-ups recorded across Batches 1–3b. Each was deferred with a stated reason; each reason has
lapsed. Committed separately.
1. CI now runs the integration suite
The 23 live tests were deselected by default and, until now, ran only on a contributor's machine.
That is not a theoretical gap — this feature's live suite repeatedly found bugs no fixture could reach:
pg_class.reltuples = -1on a never-analysed table read as "tiny table", silently suppressingevery proposal
$Nplaceholders, silently dropping a whole query groupAND s.toplevelfilter that made a hot function-wrapped query vanish while leavingconfidently-wrong advice in its place
Every one was invisible to
pytestand to review. The new job provisionspostgres:16as a servicecontainer and fails if zero integration tests run — a service that never became ready, or a marker
typo, would otherwise look exactly like a pass.
One wrinkle worth recording: a GitHub service container cannot be given a command, so
shared_preload_libraries=pg_stat_statementsandtrack=allare applied withALTER SYSTEMand acontainer restart, then verified before any test runs.
pg_reload_conf()is not enough — both arepostmaster-level and a reload leaves them inactive with no error.
2.
advisewas silent about a dbt rewrite it had performeddescribe_rewritesexists because dbt enrichment is invisible in a terminal row — an enriched proposalis byte-identical to the same proposal from a dbt-free run. It counted the ADV302 config rewrite and the
generic non-index warning, but not the
DROP INDEXbranch. So a Postgres run emitting onlyADV002/ADV003 proposals for dbt-managed relations did the enrichment and said nothing.
Now counted, with a third stderr clause: N index drop(s) target a dbt-managed relation — remove that
entry too or the next
dbt runrecreates it.3. ADV004 was the only index-creating rule that never checked coverage
Every other one calls
_coveredand, when the existing-index list can't be read, says so and capsconfidence. ADV004 (partial indexes) never called it at all, so it could propose an index an existing
one already covers, and its rationale said nothing about the gap — inconsistent with the discipline the
whole rule set turns on.
It now calls
_coveredand discloses what that cannot answer:path and the partial index buys only size — which this tool cannot measure, and which ADV003 would
never flag because its prefix check excludes partial indexes
_covered's exclusioncuts the other way here: it may be this very proposal already applied, and predicates are never
compared
Deliberate behaviour change: an ADV004 proposal covered by a plain index now disappears, three
previously-silent rationale cases now speak, and confidence drops to LOW when the index list was
unreadable. No test was weakened to accommodate it.
4. The "no executable-looking DDL line" guard had a hole
render_ddl's guard checked\nand\r, but the tests usesplitlines(), which splits on eightfurther codepoints (
\v,\f,\x1c–\x1e,\x85, U+2028, U+2029). An identifier containing onecould produce a line the guard never examined. Closed for both adapters, pinned per codepoint.
5. The integration port collided in practice
Host port 55432 was held by an unrelated container throughout this work, and when that happens
docker compose updoes not bind and the suite silently talks to whatever else is listening — whichsurfaced as a password-authentication failure that looked like a code bug and cost time for me and three
reviewers. Moved to 27432, and — the load-bearing half, since any port can collide — the fixture now
verifies it reached the database it expects, naming a port collision as the likely cause when it hasn't.
Recorded, not fixed
_prepend_notecovers 2 of 5note=sites in_classify; the other three are unreachable today (allCREATE INDEXpaths, and ADV105 lands in the already-covered generic branch).dbt.py's generic non-index fallback is exercised only from the Redshift side. A future non-indexPostgres rule would land in the same branch and deserves its own test then.
deferring it, and given every silent-suppression bug in this feature was found live, shipping an
adapter whose SQL has never executed would be the wrong trade.
Verification
980 passed, 23 deselected— zero skips, no extras, no Docker23 passedintegration against livepostgres:16module-scope import
ruff check,ruff format --check,mypy src/sqlqualitycleanThe new CI job is unverified until this PR runs it — that is what the checks below are for.
🤖 Generated with Claude Code