PROTOTYPE (do not merge): text-first update-script checker, for comparison with #90

[jnasbyupgrade]

This is a deliberately incomplete prototype, opened only for comparison against #90. It is not a merge candidate and can be closed without merging. Nothing here is wired into the Makefile or CI; several real cases are stubbed, and it has known false positives.

The principle

Never build a model of what an object is. Compare statement text.

1. Split both install scripts into top-level statements (quote-, comment- and dollar-quote-aware, so a ; inside any of those is not a boundary).
2. Normalize each statement: drop comments, collapse whitespace.
3. Multiset-diff. In NEW but not OLD is "added"; in OLD but not NEW is "removed".
4. Every added statement must appear, normalized, somewhere in the normalized update script — substring against the whole file, not statement against statement, so a copy wrapped in a DO block, a format() or an IF still matches.
5. Report anything unmatched; exit non-zero.

This works because update scripts are overwhelmingly copy-paste from the install script. The interesting content of the tool is therefore not the matcher — it is the list of cases where a copy is impossible, plus the escape hatch for everything else.

Numbers, on every real pair in the tree

pair	added	matched	unmatched	ADP findings	what the unmatched are
0.2.0→0.2.1	13	13	0	0	—
0.2.0→0.2.2	19	16	3	5	scaffolding __cat_tools.omit_column, _cat_tools.column rebuild, cat_tools.trigger__parse
0.2.1→0.2.2	8	5	3	5	same three
0.2.2→0.2.3	2	0	2	0	the two relation__kind/relation__relkind helper calls the author deliberately reformatted into hand-written CREATE OR REPLACE FUNCTION
0.2.3→0.3.0	53	48	5	0	2 scaffolding (create_function, routine__parse_arg_types_text), 3 CREATE TYPE ... AS ENUM that gained labels
dev pair (0.3.0→ current)	1	0	1	0	the __cat_tools.create_function scaffolding copy, bound to an older helper name

Removed statements are counted and listed but not failed on — a DROP never appears in an install script, so a removal can never be matched by copy.

Every unmatched item above falls into a category the header names: scaffolding bound to old names, enum-value additions, or deliberate hand-reformatting. None of them is a missed update; all six pairs are real, and the false-positive rate is 14/96 added statements.

Size

	code	comment	blank	total
bin/update_lint_textfirst	204	128	35	367
bin/test/textfirst.t	83	16	15	114
(bin/update_lint from #90, for reference)	1188	456	166	1810

What it implements for real

ALTER DEFAULT PRIVILEGES. ADP is not retroactive, so a copy of the ADP statement in the update script is not enough. When NEW's install adds an ADP that OLD's did not have, every object of that category that already existed in OLD needs an explicit GRANT in the update script. --versions 0.2.1 0.2.2 flags exactly the five enum types (constraint_type, procedure_type, relation_type, relation_relkind, object_type) created in 0.2.0/0.2.1 that never got GRANT USAGE — the real historical bug. Scoped to ADP statements that are newly added, which is why 0.2.2→0.2.3 raises nothing. Only the TYPES category is written.

The escape hatch, which is central to the design and not an afterthought:

-- update-lint: ok /REGEX/ reason

in the update script. Any finding whose text matches is suppressed and the reason is printed instead; the reason is mandatory; waivers that match nothing are reported so they cannot rot. They live in the update script because that is the file being reviewed and the file the exception is a property of. Intent: a handful per release on genuine exceptions, never one per statement.

What it stubs

Each is documented in the header and is a false positive today, waivable by hand:

• Enum values — install says CREATE TYPE ... AS ENUM ('a','b','c'), update must say ALTER TYPE ... ADD VALUE 'c'. Needs a label-set diff.
• New columns — install states the final CREATE TABLE list, update needs ALTER TABLE ... ADD COLUMN.
• Changed view column list — install uses CREATE OR REPLACE VIEW, update must DROP VIEW + CREATE VIEW.
• Removals — as above, never checked.

Known false-positive sources

• Scaffolding bound to old names. The update script's private copy of __cat_tools.create_function() calls a differently-named helper than the install's, so the text legitimately differs. This fires on the dev pair today.
• Any hand-reformatting. 0.2.2→0.2.3 reports 2 of 2 added unmatched even though the update is correct. Text-first cannot distinguish that from a real omission; the escape hatch is the only answer.
• Function overloads, argument reordering, and any other semantics-preserving edit are invisible in the same way.

Honest comparison with #90

Where this shape is better:

• ~6x less code, and the code that remains is one idea rather than an object model with per-kind extraction rules. The splitter is ~55 lines and is the only part that has to be right.
• No parse fidelity to maintain. Add bin/update_lint: static check that the update script covers the object diff #90 must learn every new DDL shape the extension ever uses; this one never needs to.
• Findings quote the statement, so "what did I forget" is answered by the output rather than by translating an object key back to a line.
• The escape hatch is honest. Waivers make the exceptions explicit, reviewable in the diff, and reported when stale — where Add bin/update_lint: static check that the update script covers the object diff #90's coverage rules encode the same judgements invisibly inside the tool.

Where it is worse, and I think these matter:

• Higher false-positive rate on the cases that actually recur. Enum-label additions and scaffolding renames are not exotic — they show up in 3 of 6 pairs here — and each one costs a waiver. Add bin/update_lint: static check that the update script covers the object diff #90 gets them right with no annotation. A checker whose steady state is "add two waivers per release" trains people to add waivers.
• A waiver is a bigger hammer than it looks. /CREATE TYPE cat_tools.object_type/ waives that whole statement forever, including a future label added and forgotten. Add bin/update_lint: static check that the update script covers the object diff #90's enum handling stays live.
• Text equality is the wrong equivalence relation. Reindenting an install statement, or reordering two GRANTs, breaks a match that is semantically fine. That is a maintenance tax paid on every SQL edit, whereas Add bin/update_lint: static check that the update script covers the object diff #90 pays it only when a new DDL shape appears.
• Substring-against-the-whole-file is loose in the other direction too. A statement copied into a comment, or into a format() template that is never executed, matches. So does a copy inside an IF false branch.

My read: this is a good sketch of the floor — if #90 did not exist, 204 lines buying 85% coverage would be an easy yes. Against #90 it is a downgrade, because the gap between them is exactly the recurring cases (enum labels, ACLs on pre-existing objects, scaffolding), and the escape hatch converts those from "handled" into "annotated by hand every release". The two ideas are not exclusive, though: the ADP check here is text-first and cost ~30 lines, and the statement-text matcher would make #90's findings considerably easier to read.

Tests

prove bin/test/textfirst.t — 12 tests: the splitter's four ; hiding places, substring match through a DO block, an unmatched statement failing, a waiver suppressing and an unused waiver being reported, and the five-enum-type ADP result on the real 0.2.1→0.2.2 pair. Deliberately a handful, not a suite.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: aa65d5a2-c089-4c28-a796-3fa56759d64f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Claude finished @jnasbyupgrade's task in 3m 40s —— View job

---

Review: PR #92 — text-first update-script checker (prototype)

• Gather context (PR body, diff, CLAUDE.md conventions)
• Read bin/update_lint_textfirst and bin/test/textfirst.t
• Attempt to run prove bin/test/textfirst.t
• Review for bugs, edge cases, and design issues
• Post review feedback

Note: I was unable to execute perl bin/test/textfirst.t / prove in this sandboxed environment — the tool-approval prompt for running perl wasn't available to me here, so I could not confirm the test suite passes. I did trace the logic by hand against the real sql/cat_tools--0.2.1.sql.in / --0.2.1--0.2.2.sql.in pair the ADP test asserts against, and the shapes line up (see below). If CI can run prove bin/test/textfirst.t, that's a stronger signal than my read.

This is explicitly a non-merge comparison prototype, so I focused on things the PR description/header doesn't already disclose, rather than re-listing the stubs and false positives it's upfront about.

Findings

1. Waiver regex parsing breaks on a literal / in the pattern (bin/update_lint_textfirst:231)

next unless $line =~ m{--\s*update-lint:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z};

The non-greedy (.+?) stops at the first / after the opening one, with no escape mechanism. A waiver meant to match SQL text containing a / (e.g. an operator, a path literal, or a character class like [a/b]) will silently truncate: the regex compiles to something shorter/different than intended, and the mismatch surfaces only as a confusing "UNUSED WAIVER" (or, worse, an unintended match on unrelated text) rather than a parse error. Given the design leans on waivers as the escape hatch for every stubbed case, this seems worth hardening (e.g. support \/ escaping, or use a delimiter unlikely to appear in SQL).

2. adp_gaps's regex only recognizes one ALTER DEFAULT PRIVILEGES shape (bin/update_lint_textfirst:266-269)

next unless $st->{norm} =~ /\A
    ALTER \s+ DEFAULT \s+ PRIVILEGES \s+
    IN \s+ SCHEMA \s+ (\S+) \s+
    GRANT \s+ (.+?) \s+ ON \s+ TYPES \s+ TO \s+ (\S+)
/xi;

This matches the exact form used today (confirmed against sql/cat_tools--0.2.2.sql.in:23), but Postgres also allows ALTER DEFAULT PRIVILEGES FOR ROLE ... IN SCHEMA ... and omitting IN SCHEMA entirely (global defaults). Either form would silently fail to match and the check would just do nothing — no error, no finding — for what could still be a real "objects predate this ADP" gap. Since this is the one gap the prototype implements "for real" (per the PR body), it's worth at least a comment noting the syntax assumption, so a future author extending it doesn't assume all ADP shapes are covered.

3. preprocess()'s version-marker handling is asymmetric between REQUIRES and PRIOR TO (bin/update_lint_textfirst:105-110)

$text =~ s/^(.*)-- SED: PRIOR TO ([^!\n]*)!/-- Not used prior to $2: $1/gm;

REQUIRES N! markers are never touched (always left active), and PRIOR TO N! markers are unconditionally commented out. Cross-checked against sql.mk:190-193: this is correct only because every marker currently in the tree targets a PG version below today's support floor (PG12+, per CLAUDE.md) — there's no live case where a REQUIRES branch should be disabled today. That assumption is stated in the comment above the sub, so it's not hidden, but it means the tool would silently do the wrong thing (leave a not-yet-satisfied REQUIRES branch active) the day a marker's threshold is raised above the current floor, rather than erroring. Might be worth a runtime assertion (e.g. "no REQUIRES marker may exceed the support floor") so drift here fails loudly instead of producing a quietly-wrong comparison.

Smaller observations

• adp_gaps re-derives "is this statement new" via a plain presence check (%in_old) rather than reusing the multiset-aware @added computed in main. Harmless in practice since ADP statements are effectively unique per schema/role, but it's a second, slightly different notion of "added" living side by side with the first.
• The splitter, quote/dollar-quote/comment handling, and multiset added/removed logic all look correct on inspection, including the line-number bookkeeping across single-line comments (verified by tracing the \n handling rather than assuming it).

Given the PR's own framing (prototype for comparison, not a merge candidate), none of the above blocks anything — they're notes for if/when this direction gets carried forward rather than #90's.