schemadiff: export ListManagedTables so owners enumerate declared tables the way pull does

[Kiran01bm]

schemadiff.ListManagedTables exports the catalog query pull uses to enumerate a schema's tables, so owners detecting undeclared tables call the same enumeration instead of copying it.

Why

Undeclared-table detection stays with the owner on purpose (docs/capabilities.md), and the recipe there asked owners to run "the listing pull baselines a schema from" themselves, with the SQL inlined. Two hand-maintained copies of a fail-closed enumeration drift, and a false positive in the owner's copy blocks a table nobody touched. One exported function is the single source for what counts as a table pg-sprite manages, and a doc test keeps the recipe's copy identical to it.

What

• schemadiff.ListManagedTables(ctx, pool, schema) returns the sorted names of ordinary and partitioned tables (relkind r/p) in one schema. Partitions are excluded because their parent's PARTITION BY declares them; extension-owned tables are excluded because they have no file to write; views, materialized views, foreign tables and sequences are outside the declarative model.
• Decision reversed: the previous recipe excluded INHERITS children "because export refuses them anyway". They are now listed. An undeclared table is undeclared whether or not pull can write its file, so an INHERITS child with no desired file is a permanent finding for the owner until it is declared (through a hand-written file, since Render refuses inheritance) or dropped through a reviewed process. The listing is the candidate set pull enumerates, not the set of files it writes; refused shapes are reported per table.
• Every catalog relation, operator, and type in the query is pg_catalog-qualified (OPERATOR(pg_catalog.=), 'pg_catalog.pg_class'::pg_catalog.regclass). Under a user-first search_path, an unqualified relation or = would join nothing and list no tables; an unqualified regclass cast would stop matching the extension dependency and list extension members as undeclared. pull's schema-existence guard is hardened the same way.
• pull calls it; its private listTables is deleted.
• docs/capabilities.md cites the function, shows the hardened SQL, names both shadowing failure modes, and points at the model-limits paragraph instead of enumerating refusals; docs/schemabot-integration.md matches. introspectInTx documents why its own unqualified catalog queries are safe (it sets a search_path that omits pg_catalog, so the catalog is searched first).

Tests

• TestListManagedTables: ordinary, partitioned parent, INHERITS parent and child, unlogged listed; partition, view, materialized view, sequence not.
• TestListManagedTablesExcludesExtensionOwnedRelations: on a throwaway database, ALTER EXTENSION plpgsql ADD TABLE gives a table a real deptype = 'e' dependency; the listing leaves it out. Skips on SQLSTATE 42501 when an external PG_DSN role does not own the extension.
• TestListManagedTablesResistsCatalogShadowing: empty impostor pg_class/pg_namespace/pg_depend, always-false = over (oid,oid), (name,name), ("char","char"), and a regclass domain, all ahead of pg_catalog on the connection's search_path, with an extension member present; the listing is still exact. Every un-qualification of the query fails it.
• TestCapabilitiesDocShowsTheManagedTablesQuery: the recipe's fenced SQL block equals listManagedTablesSQL after normalizing comments, layout, and the 'app' literal.

Before / after

Before
┌────────────────┐   private listTables()   ┌──────────────┐
│ pull           │────────────────────────▶ │ pg_catalog   │
└────────────────┘                          └──────────────┘
┌────────────────┐   hand-copied SQL from   ┌──────────────┐
│ owner          │   docs/capabilities.md   │ pg_catalog   │   two copies, free to drift
└────────────────┘────────────────────────▶ └──────────────┘

After
┌────────────────┐
│ pull           │──┐
└────────────────┘  │   ┌────────────────────────────────┐   ┌──────────────┐
                    ├─▶ │ schemadiff.ListManagedTables   │─▶ │ pg_catalog   │
┌────────────────┐  │   │ (pg_catalog-qualified, sorted) │   └──────────────┘
│ owner          │──┘   └────────────────────────────────┘
└────────────────┘        one enumeration; the recipe's copy is pinned to it by a doc test

🤖 Created with Amp (Claude Opus 4.6).

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Reviewed 4fad2ff0 against merge base a8d171c9 (4 files, +165/−44). Exporting the enumeration is the right call and the query itself is a strict improvement on the private one it replaces. I verified the hardening is real rather than decorative, and mutation-tested the package (18 mutations evaluated, 10 killed / 8 survived, worktree clean) — two of the survivors matter.

The hardening works, and I checked it live. On a stock PostgreSQL 16 I built the shadowing attack the doc comment describes — a schema containing a table named pg_class plus an = operator over (oid, oid) that always returns false, with search_path = shadow, pg_catalog so pg_catalog is no longer implicitly first:

-- old private listTables SQL, hostile search_path:   (0 rows)
-- this PR's SQL, same session:                       ordinary

'pg_class'::regclass resolved to the decoy (oid 43034) while 'pg_catalog.pg_class'::pg_catalog.regclass resolved to 1259. Both halves of the qualification are load-bearing.

Also correct: ORDER BY c.relname is stable across locales because relname is name, which sorts in C collation — so the exact-slice assert.Equal cannot flake on a differently-configured server. The nil→make([]string, 0) change is deliberate and pinned (assert.NotNil in "empty schema"; the mutation back to var tables []string dies). The godoc's claim that "pull reports the refusal by table" is accurate — writePullText prints REFUSED <table>: <err>.

1. TestListManagedTablesExcludesExtensionOwnedRelations can drop plpgsql from the whole database, and nothing pins the guard that stops it

ALTER EXTENSION plpgsql ADD TABLE … makes the table an extension member. A member cannot be dropped on its own, so DROP SCHEMA … CASCADE — which is what testutil.NewSchema's cleanup runs — resolves the dependency the only way it can:

DROP TABLE t1.owned_by_ext;
  ERROR:  cannot drop table t1.owned_by_ext because extension plpgsql requires it
  HINT:   You can drop extension plpgsql instead.

DROP SCHEMA t1 CASCADE;
  NOTICE: drop cascades to extension plpgsql
  DROP SCHEMA

SELECT extname FROM pg_extension;   -- (0 rows)
DO $$ BEGIN END $$;                 -- ERROR: language "plpgsql" does not exist

The happy path is safe and the comment at :73-74 explains why: cleanups are LIFO, so the ALTER EXTENSION … DROP TABLE runs before the schema drop. I ran the test twice in a row on a fresh server and plpgsql survived both times. The problem is what that ordering is protecting, and how little holds it in place:

• Deleting the t.Cleanup block at managed_tables_integration_test.go:72-78 leaves the whole suite green while stripping plpgsql off the server. That was mutation m0: SURVIVED (0 failures), extensions after: NONE. A refactor that moves the membership release, reorders the cleanups, or drops it as redundant costs nothing in CI and destroys the database.
• CI is the exposed configuration, not the protected one. .github/workflows/ci.yml:108 states it: "Each job runs one long-lived server (make db-up + make test-db)". So the blast radius is not one throwaway schema, it is every remaining test in that job — and on a developer's make db-up server it persists across runs until someone works out that CREATE EXTENSION plpgsql is the repair. Any interruption that skips cleanups (-timeout panic, SIGINT) also leaves a schema behind that detonates whenever it is eventually dropped with CASCADE.

testutil already has the tool that removes the class rather than the instance: NewDatabase(t, serverURL) (internal/testutil/postgres.go), whose docstring is for exactly this — "a test that must observe an exact session set … gets a database of its own". Point this test at a throwaway database instead of a throwaway schema and the cascade cannot reach anything shared, the ALTER EXTENSION … DROP TABLE cleanup and its require.NoError become unnecessary, and m0 stops being a green mutation because there is nothing left to remove.

I found a long-lived local server already sitting in the post-cascade state (pg_extension empty, template1 still holding plpgsql), which is what sent me looking; I can't attribute it to a specific run, but it is the exact signature.

2. The search_path hardening — the PR's headline safety claim — has no test

Two mutations that revert it exactly, both green:

Mutation	Result
all 7 OPERATOR(pg_catalog.=) → = in listManagedTablesSQL	SURVIVED
'pg_catalog.pg_class'::pg_catalog.regclass → 'pg_class'::regclass	SURVIVED

Every semantic predicate is well pinned — dropping NOT c.relispartition dies in 3 tests, dropping the deptype = 'e' exclusion dies in the extension test, 'e'→'x' dies, collapsing the relkind disjunction either way dies, removing the schema filter dies in 6, removing or reversing ORDER BY dies. The qualification is the one property in the file with no test at all, and it is the property the PR body leads with.

The probe above is the missing test, and it fits the existing harness. On a database of its own (see finding 1), before the listing:

_, err = pool.Exec(t.Context(), fmt.Sprintf(`
    CREATE SCHEMA shadow;
    CREATE TABLE shadow.pg_class (oid oid, relname name, relnamespace oid,
                                  relkind "char", relispartition bool);
    CREATE FUNCTION shadow.always_false(oid, oid) RETURNS boolean
        AS $$ SELECT false $$ LANGUAGE sql IMMUTABLE;
    CREATE OPERATOR shadow.= (LEFTARG=oid, RIGHTARG=oid, FUNCTION=shadow.always_false);
    ALTER DATABASE %s SET search_path = shadow, pg_catalog;`, ...))

ALTER DATABASE … SET search_path is the part that makes this work through a *pgxpool.Pool: ListManagedTables takes the pool and picks whichever connection it likes, so a session-scoped SET on one connection proves nothing. Naming pg_catalog explicitly and second is also essential — left implicit it is searched first and no shadowing is possible, which is why this is not reachable from the tests as they stand. With that in place, a fresh pool listing the schema still returns the right names, and both mutations above die.

3. "a silent empty result" is one of the two failure modes, and the other one is the one this file says matters

managed_tables.go:39-41 (and docs/capabilities.md:339-340) justify the qualification as stopping a shadowed search_path from turning the listing "into a silent empty result." That is true of the operator shadow — my run A returned 0 rows. It is not what the regclass shadow does. With the operator left intact and only pg_class shadowed:

-- old SQL:            ordinary, owned_by_ext        <- extension member leaks in
-- this PR's SQL:      ordinary

Resolving 'pg_class'::regclass to a decoy makes d.classid compare against an oid nothing matches, so the NOT EXISTS is unconditionally true and every extension-owned table is listed. That is a false positive, not an empty result — and false positives are precisely the harm the PR body and capabilities.md:319-320 name ("every false positive blocks a table nobody touched"). Both hardenings are right; the sentence explaining them describes the mode that matters less to the caller. Naming both failure modes would make the comment carry its own justification.

4. Three error sites now return the same string, where the code being replaced returned three

managed_tables.go:45, :53 and :58 are byte-identical: fmt.Errorf("list managed tables in schema %q: %w", schema, err). The listTables this replaces distinguished them — "list tables in schema %s", "scan table in schema %s", "read tables in schema %s" (internal/cli/pull.go:82, :89, :94 on main). A query that never opened, a row that failed to decode, and a stream that broke mid-read are three different situations for whoever reads the error, and the wrap is where that distinction lived. Since the function is now a public entry point whose errors reach code that has no other context, this is a step backwards; keeping the three verbs costs nothing.

5. Smaller

• Removing the rows.Err() check (:57-59) and removing defer rows.Close() (:47) both survive. I'd call neither a defect — pgx surfaces most stream failures through Scan, and the connection returns to the pool on Next exhaustion — but they are the two remaining unpinned lines in the file, so they belong in the tally rather than being read as coverage.
• The multi-statement pool.Exec at :67-70 runs under the simple protocol's implicit transaction, so the ALTER EXTENSION cannot half-apply and leave a member table with no registered cleanup. Worth knowing, given finding 1 — it closes the one window that would otherwise sit between the ADD and the t.Cleanup registration.
• All 14 checks are green on this head across PostgreSQL 14–18, and the branch already carries main (4531008).

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Second pass — the registry, the docs, and the stack.

Invariants

No entry in docs/invariants.md is touched by this change. The registry governs execution safety — the checksum gate, lock windows, checkpoint and slot state, preflight refusals, control-plane discipline — and an enumeration helper plus the recipe that cites it sits outside all nine families. RF-2 is the nearest neighbour and is unrelated: it refuses a change to a table other tables reference, whereas ListManagedTables deliberately lists such a table and leaves it to the owner. The disposition here is "no entry applies", not "upholds", and nothing needs adding — the property this PR is really about (pg-sprite never plans or executes a DROP TABLE, so the set of tables is the owner's) is a scoping decision recorded in capabilities.md, and this change does not alter it.

1. The stated failure mode survives the fix: the recipe still carries its own copy of the SQL

The PR body's case is that "two hand-maintained copies of a fail-closed enumeration drift", and the evidence is strong — this PR is itself repairing that drift, since capabilities.md on main excluded INHERITS children through pg_inherits while listTables only ever excluded relispartition. But the fix removes one of the two copies and leaves the other in place: capabilities.md:322-337 still inlines the SQL, now as a hand-transcribed duplicate of listManagedTablesSQL, with nothing tying them together. Three mutations confirm it:

Mutation to docs/capabilities.md	Result
delete AND NOT c.relispartition (:330) — recreating the exact drift being fixed	SURVIVED
collapse the relkind disjunction to 'r' only (:328-329)	SURVIVED
unqualify all 7 OPERATOR(pg_catalog.=) back to =	SURVIVED

The third is the one that stings: an owner who copies the block gets the pre-hardening query back, and :339-340 will still be sitting underneath it telling them the qualification protects them.

This repo already reaches for a test when a doc and the code have to agree — the same reflex as the proof-type registry test in #91. Extracting the fenced sql block from this section and comparing it to listManagedTablesSQL (normalizing $1 against the 'app' literal and the leading indentation) makes the copy checked rather than hoped-for, and kills all three. Failing that, the honest alternative is to delete the block and let the recipe cite the function alone — the section already tells owners the enumeration is available to call, so the SQL is there for readers who want to see it rather than run it, and a comment saying so at least stops it from being copied.

2. The refusal list at :316-318 is a short version of the one at :132-134, in the same file

The new paragraph tells owners which listed tables pull cannot write a file for: "a partitioned parent, either side of INHERITS, an unlogged table, a table other tables reference" — four categories, phrased as the complete set. capabilities.md:132-134, unchanged by this PR, gives six: tables that are "partitioned (or are partitions), participate in classic table inheritance, own or are referenced by foreign keys, are unlogged, carry explicit collations, or take defaults from sequences they do not own."

Render agrees with the longer list. It has eight refusal paths (render.go:68-85, :123-130) and then a ninth gate at :110-112, where statement.ParseDesired rejects output a desired file will not accept — which is how a table with an outgoing REFERENCES is refused, as Render's own doc comment at :58 says. The omission is not academic: outgoing foreign keys are common enough that on a normal relational schema a large fraction of the candidate set is unrenderable, and an owner reading :316-318 will conclude their FK-owning tables pull cleanly. managed_tables.go:36-39 carries the same four-item list and reaches the audience least able to check it, since a caller of the exported function reads the godoc and never opens capabilities.md.

Either enumerate all six, or stop enumerating and point at the paragraph that already does — "a listed table whose shape export refuses (see the declarative door's model limits above)". The second is more robust, and is the same argument as finding 1: this is a list that has already drifted once in this file.

3. Including INHERITS children is a reversal of a recorded decision, not a correction of a typo

The body describes "the partition and inheritance notes corrected to match what the query does", which reads as the docs having simply fallen behind. What was there was a stated position with a stated reason: capabilities.md:313-314 on main excluded INHERITS children "because export refuses them anyway." The new text takes the opposite view — an unrenderable table "is still undeclared and still the owner's to resolve" — and that is the better position, because a table nobody can declare is exactly the thing an owner needs to know about. But it is a decision, and it has a consequence worth writing down: an owner already running the old recipe will see new, permanent findings for every INHERITS child in their schema, and there is no pull output that will ever clear them. One sentence in the body saying the exclusion was deliberate, is being reversed, and why, is the difference between a reviewer checking the change and a reviewer rediscovering the argument.

There is a smaller version of the same thing inside the new paragraph. It now explains why partitions and extension members are excluded but says nothing about the relkind filter, even though :340 (unchanged) is the sentence that covers it: "Views, materialized views, foreign tables, and sequences are outside the model and are not undeclared tables." A reader of the new prose alone cannot tell whether a foreign table's absence is intentional. In the godoc it is not covered anywhere, which is the version that matters — managed_tables.go:30-41 justifies two exclusions and is silent on the third.

4. The body describes a weaker test than the branch contains

"Extension exclusion is asserted on the SQL text because a stock server has no extension-owned tables to create." The head commit is titled "prove the extension exclusion live" and managed_tables_integration_test.go:61-84 does exactly that, against a real server, by making a table a member of plpgsql. The body is describing the first commit. Worth refreshing before this merges, both because the live assertion is the stronger claim and because the workaround it uses is the thing finding 1 in my other comment is about.

5. Stack and mechanics

• Branch already contains main (4531008), all 14 checks green on 4fad2ff0 across PostgreSQL 14–18. chatgpt-codex-connector posted twice but only to report its own usage limit, so there is no automated review on this head to reconcile against.
• internal/cli/pull.go loses 33 lines and gains one call, and the call site is pinned — swapping c.Schema for a literal dies in two pull tests. The private helper's deletion is complete; no other enumeration of "tables in a schema" remains in the tree (introspect.go and migrate.go both resolve a single named relation, which is a different question).
• One asymmetry left over from the move: introspectInTx (pkg/schemadiff/introspect.go:51-68) is what pull runs against every name this function returns, and it is unqualified throughout — plain pg_class/pg_namespace joins and a bare =. It is safe today because it sets search_path itself and leaves pg_catalog implicit, so pg_catalog is searched first; that is a different mechanism from the one this PR just adopted, reached by the opposite route. Not a defect and not this PR's job, but now that one half of the pair has been hardened explicitly it is worth a note at one of the two sites saying why the other does not need it — otherwise the next reader reasonably concludes the hardening was applied inconsistently.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving. Exporting the enumeration is right, the pg_catalog qualification is real hardening (I reproduced both shadowing attacks live against a stock server), and every semantic predicate in the query is well pinned. Findings are in the two comments above — the plpgsql cascade in the new extension test and the untested qualification are the two worth acting on before this lands.

This stamp was left by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/92, follow-up commit

Verdict: All findings from the scratch reviews at 83cc66ca, 4531008a, 4fad2ff0 and from review comment 5607854405 and review comment 5607855411 are addressed in f3da086e; two are recorded as decisions rather than changes.

#	Finding	Status	Explanation
S1-F1 (83cc66c)	ListManagedTables doc contract claimed it returns file-backed tables; it returns shapes Render refuses too.	Fixed (earlier)	Doc comment and docs/capabilities.md describe the candidate set pull enumerates, with refusals reported per table.
S1-F2 (83cc66c)	Extension exclusion asserted on SQL text only.	Fixed (earlier)	Replaced with a live ALTER EXTENSION plpgsql ADD TABLE test; now hardened further per C1-F1.
S2-F1 (4531008)	pull.go:48 schema-existence guard uses bare =.	Fixed	nspname OPERATOR(pg_catalog.=) $1.
S2-F2 (4531008)	docs/schemabot-integration.md:124 still lists INHERITS children as excluded.	Fixed	Exclusions now read "partitions and extension-owned tables".
S2-F3 (4531008)	No hostile search_path test.	Fixed	TestListManagedTablesResistsCatalogShadowing — see C1-F2.
S3-F1 (4fad2ff)	Extension test needs superuser and hard-fails under an external PG_DSN.	Fixed	addToExtensionOrSkip skips on SQLSTATE 42501 so the compose database and CI matrix still run it; docs/testing.md names extension ownership among the skip-on-refusal requirements.
C1-F1	ALTER EXTENSION plpgsql ADD TABLE in a throwaway schema; a dropped or reordered cleanup would DROP SCHEMA … CASCADE the shared server's plpgsql.	Fixed	Both extension tests run on testutil.NewDatabase; dropping that database is the only cleanup and the ALTER EXTENSION … DROP TABLE cleanup is gone.
C1-F2	Reverting OPERATOR(pg_catalog.=) or ::pg_catalog.regclass survives every test.	Fixed	New test installs empty shadow.pg_class/pg_namespace/pg_depend, always-false shadow.= over (oid,oid), (name,name), ("char","char"), and a shadow.regclass domain, connects with search_path = shadow, pg_catalog, and expects exactly ordinary, partitioned with an extension member excluded; every un-qualification mutant (8 sites) fails it.
C1-F3	Doc and godoc name only the empty-result failure mode; a shadowed regclass produces false positives.	Fixed	Godoc and docs/capabilities.md name both: unqualified relation/operator → empty listing; unqualified regclass cast → extension members listed as undeclared.
C1-F4	Three identical error strings.	Fixed	list / scan / read managed tables in schema %q.
C1-F5	rows.Err() / defer rows.Close() untested (tally).	No change	Noted as no defect in the comment; no test added.
C2-F1	docs/capabilities.md inlines a hand-copied SQL block that can drift.	Fixed	TestCapabilitiesDocShowsTheManagedTablesQuery extracts the section's fenced sql block and compares it, comments/layout/'app' normalized, to listManagedTablesSQL; every mutant of the constant fails it.
C2-F2	Four-item refusal list in godoc and recipe disagrees with the six-item model-limits paragraph and Render.	Fixed	Both stop enumerating and point at the model's limits (recipe links to "The two front doors").
C2-F3	Including INHERITS children reverses a recorded decision without saying so; relkind filter undocumented.	Fixed	PR body states the reversal and its consequence (an INHERITS child with no file is a permanent finding until declared or dropped); godoc names views, materialized views, foreign tables, and sequences as outside the model.
C2-F4	PR body says extension exclusion is asserted on SQL text.	Fixed	Body refreshed to the live tests.
C2-F5	introspectInTx catalog queries are unqualified; explain why that is safe.	Fixed	Comment on introspectInTx: its SET LOCAL search_path omits pg_catalog, so the catalog is searched first; ListManagedTables runs under the caller's session search_path and qualifies everything.
C2 invariants	No registry entry applies.	Agreed	No // INV: added; pkg/schemadiff is periphery.

Decisions to veto: (a) The shadowing test uses a per-connection search_path (BeforeConnect, the native_integration_test.go idiom) rather than ALTER DATABASE … SET search_path; it exercises the same resolution path with one fewer privileged statement. (b) The regclass impostor is a domain over oid, so an unqualified cast fails loudly on the catalog's name rather than matching nothing; the mutant still dies, and the false-positive mode is documented rather than reproduced, which would need a superuser-created base type. (c) Kept the SQL block in the recipe (owners not on Go can copy it) and pinned it with the doc test instead of deleting it.

Source: #92, review comments 5607854405 and 5607855411 at head f3da086e; scratch reviews pg-sprite-pr92-review.md (83cc66c), pg-sprite-pr92-review-4531008a.md, pg-sprite-pr92-review-4fad2ff0.md