Static check that the update script covers this cycle's install-script changes

[jnasbyupgrade]

Add a static check that the current cycle's update script accounts for every change to the install script

bin/update_lint_textfirst splits two versioned install scripts into statements, diffs them, and verifies the update script between them accounts for the difference. No database, no SQL parser, no build — it reads three tracked files and exits in well under a second, so it rides CI's existing lint job as a step rather than a job. make update-lint is the same entry point a developer runs locally.

Closes the "nothing fails when you forget the accumulator" half of #93.

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: drop comments, collapse whitespace.
3. Multiset-diff. In NEW but not OLD is "added".
4. Every added statement must appear, normalized, somewhere in the normalized update script — substring against the whole file, so a copy wrapped in a DO block, a format() or an IF still matches.
5. Anything left over is a finding; exit non-zero.

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

Aimed at small, subtle diffs, not whole new objects. A changed function body, a changed GRANT, a tweaked view definition. An entire new function is hard to miss in a diff of the install script between two versions, so nothing here chases that case — and not chasing it is what keeps this small.

Default scope: the current cycle only

With no arguments it checks the only pair that is still editable — sql/cat_tools--<last-released>.sql.in → sql/cat_tools.sql.in, with sql/cat_tools--<last-released>--stable.sql.in as the accumulator. Both version numbers are derived (the current one from cat_tools.control, the previous from the single accumulator file named after it), so a release needs no edit here.

Released pairs are frozen. --versions OLD NEW runs one by hand; that is a way to validate this script, not a gate.

What it catches, on the real tree

Change a function body in sql/cat_tools.sql.in and forget the accumulator, against a copy of the actual tree:

sql/cat_tools.sql.in:380: added statement has no copy in sql/cat_tools--0.3.0--stable.sql.in
    SELECT __cat_tools.create_function( 'cat_tools.relation__kind' , ...

The finding names the file, the line and the statement, so "what did I forget" is answered by the output rather than by translating an object key back to a location.

What decides whether a case belongs here

The header leads its limitations with the rule, because it is what keeps this small:

Each case below is declined, not missing. The rule that decides what belongs here is: ask what the right way to verify THIS case is. Where text comparison is genuinely easy it is used; where it is awkward, that awkwardness is evidence the case belongs to a different mechanism — a runtime catalog comparison, or an explicit test — and not to a cleverer parser. Growing this script to reach one of them would cost far more than it bought.

Three cases come out of that rule with real code or a real exemption:

Enum labels are out of scope, because a better mechanism already covers them. The pgTAP suite runs the SAME expected output against a fresh install, an updated database and a pg_upgraded one (see CLAUDE.md's TEST_LOAD_SOURCE), so an install and an update script that disagree about an enum's labels fail one of those runs against the real catalog — stronger than anything a text check could assert, and it needs nothing from this script. A changed CREATE TYPE ... AS ENUM on a type the old version already had is therefore exempt rather than waived: declaring it out of scope and then failing on it every time would teach people to reach for a waiver. A brand-new enum type stays under the ordinary copy rule, since it copies into the update script verbatim like any other new object.

Scaffolding. Statements defining or dropping an object in the __cat_tools schema are exempt. That schema is created at the top of the install script and dropped at the bottom, so nothing in it survives CREATE EXTENSION and no divergence there can leave an updated database short an object — the update script's private copy of a helper is free to be bound to whatever names existed when it was written. This is a naming-convention exemption, not a per-release waiver, so it cannot rot. Only definitions are exempt, never calls: SELECT __cat_tools.create_function('cat_tools.foo', ...) creates a real object and is still checked.

ALTER DEFAULT PRIVILEGES. ADP is not retroactive, so copying it into the update script is not enough: every object of that category that already existed in OLD needs an explicit GRANT. Scoped to ADP statements that are newly added. --versions 0.2.1 0.2.2 reproduces the real historical bug — the five enum types created in 0.2.0/0.2.1 that never got GRANT USAGE. Narrow on purpose: only the IN SCHEMA <s> GRANT <p> ON TYPES TO <r> shape is recognized, so a FOR ROLE or schema-less global ADP raises nothing, and TABLES/FUNCTIONS/SEQUENCES would each need their own "what creates one of these" pattern. It reproduces a known bug shape; it is not a general ADP analysis, and widening it into one is exactly the growth the rule above argues against.

The escape hatch

-- update-lint: ok /REGEX/ reason

in the update script, because that is the file being reviewed and the file the exception is a property of. Two rules keep a waiver from outliving its reason:

• A waiver that matches nothing fails. Either the divergence is gone, or the statement changed underneath it into something nobody reviewed.
• A line opening with -- update-lint that does not parse is a hard error naming file, line and text — never a silent no-op. A missing reason or a mistyped keyword used to be dropped without a word, leaving the author staring at a finding they believed they had already waived.

Whitespace closes the regex, not the first /, so a regex holding an operator, a character class or a path does not truncate at it and silently waive something wider than written.

Intent: a handful per release on genuine exceptions, never one per statement.

Results

Live dev pair: clean, 0 findings. (It is currently empty — nothing has changed since 0.3.0.)

#96 (perm-normalize) as a preview — fetched read-only, not merged or rebased onto. This is the best available sample of what a real SQL-touching PR looks like under this check:

  old sql/cat_tools--0.3.0.sql.in (131 statements)
  new sql/cat_tools.sql.in (136 statements)
  added 5 (matched 5, scaffolding 0, enum 0, unmatched 0)
  OK

Zero false positives on its ~166 accumulator lines.

Historical pairs, as validation only — frozen, out of default scope, and deliberately not tuned for:

pair	added	matched	scaffolding	enum	unmatched	ADP
0.2.0→0.2.1	13	13	0	0	0	0
0.2.0→0.2.2	19	16	1	0	2	5
0.2.1→0.2.2	8	5	1	0	2	5
0.2.2→0.2.3	2	0	0	0	2	0
0.2.3→0.3.0	53	48	1	3	1	0

The five ADP findings are the real historical bug, not noise. The remaining unmatched are all the documented hand-reformatting class: a _cat_tools.column rebuild, cat_tools.trigger__parse, the two relation__kind/relation__relkind helper calls rewritten into literal CREATE OR REPLACE FUNCTION, and a bootstrap stub the update script defines in its final form instead. None is a missed update.

Documented rather than implemented

The header's limitations lead with the class that matters most and is genuinely out of reach:

This compares text, so a divergence that is invisible in the SQL but real in the catalog is invisible here. Demonstrated, not hypothetical: defect D1 in #93 had a fresh and an updated install agreeing on every ACL and every has_type_privilege answer while differing in five pg_init_privs rows, because ALTER DEFAULT PRIVILEGES grants implicitly and an implicit grant is never snapshotted — so the two produced different pg_dump output. bin/structural_diff called them identical too. That class needs a pg_init_privs or pg_dump comparison; nothing text-level can reach it.

Also documented rather than built:

• Enum labels, as above — the pgTAP suite is the right mechanism.
• Removals are never failed on. A DROP does not appear in an install script, so a removal can only be matched by inference.
• Any semantics-preserving edit that changes text is a false positive: reindenting, reordering two GRANTs, hand-rewriting a helper call as the CREATE OR REPLACE FUNCTION it expands to, adding an overload. The escape hatch is the answer; there is no cheaper one.
• Substring-against-the-whole-file is loose the other way. A statement copied into a comment or an IF false branch counts as matched.
• Whole new objects get no special help — by design, see above.

Size

	code	comment	blank	total
bin/update_lint_textfirst	257	230	54	541
bin/test/textfirst.t	146	39	25	210
(bin/update_lint from the closed #90, for reference)	1188	456	166	1810

Tests

prove bin/test/textfirst.t — 30 tests, one case per rule plus the three things only the real tree can prove: that the current development pair is clean, that the ADP rule reproduces the historical bug, and that preprocessing erases sql.mk's " VERSIONED FILE!" tag (one such marker sits inside __cat_tools.create_function()'s dollar-quoted body, where no comment strip can reach it, and left alone it makes every release's copy of that function differ from the base file's). Deliberately a handful, not a suite — a checker whose test suite dwarfs it has stopped being the cheap option.

CI wiring

Two steps appended to the existing lint job (ubuntu-latest, no container, checkout without submodules): prove bin/test/textfirst.t first, so a bug in the checker is reported as a bug in the checker rather than as a finding against the SQL, then make update-lint.

A step, not a job — all-checks-passed hard-fails when the workflow's job-key set differs from its needs:.

update-lint is deliberately not tied to lint as a prerequisite in either direction: lint.mk's vendored include is guarded on $(wildcard .git), so lint does not exist as a target in a released tarball and make lint fails loudly there. Naming it as a prerequisite would define it with no recipe and convert that loud failure into a silent pass.

[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: 8b3fb496-218c-4850-b265-2684807468b4

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.

[github-actions]

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

---

Review: PR #92 — text-first update-script checker (prototype, comparison against #90)

• Gather context (PR body, diff, CLAUDE.md conventions)
• Read bin/update_lint_textfirst and bin/test/textfirst.t in full (this revision, 538/198 lines — substantially larger than the version reviewed previously; enum-label handling is now implemented, where it was previously stubbed)
• Attempt to run prove bin/test/textfirst.t
• Review for bugs, edge cases, and design issues
• Post review feedback

Note on testing: as in the previous review, I could not get perl/prove approved to execute in this sandbox (every invocation, even perl -c, hit a pending-approval wall with no prompt surfaced to me). I instead hand-traced the two acceptance-test claims against the real files in the tree:

• --versions 0.2.1 0.2.2: confirmed sql/cat_tools--0.2.2.sql.in:23 has the exact ALTER DEFAULT PRIVILEGES IN SCHEMA cat_tools GRANT USAGE ON TYPES TO cat_tools__usage; shape adp_gaps matches, and confirmed sql/cat_tools--0.2.1--0.2.2.sql.in has no GRANT USAGE ON TYPE ... TO cat_tools__usage lines for constraint_type/procedure_type/relation_type/relation_relkind/object_type — so the five-type finding is real, not a fixture artifact.
• The now-implemented enum-label handling: verified relation_type, relation_relkind, and object_type gained labels between sql/cat_tools--0.2.3.sql.in and sql/cat_tools.sql.in, and that sql/cat_tools--0.2.3--0.3.0.sql.in does carry the matching ALTER TYPE ... ADD VALUE statements (lines 202-207, 642-643) — so enum_gaps should report these as covered, not as findings.
• bin/test/textfirst.t's Test::More tests => 27 plan matches the actual count of 27 is/like/unlike/is_deeply calls, including the 6-test SKIP block, so the file isn't structurally broken.

Given the PR's own framing (non-merge prototype for comparison with #90), these are notes for that comparison rather than blockers.

1. The PR description's numbers/"what it stubs" table looks stale relative to this revision's code

The body says enum values are still stubbed ("install says CREATE TYPE ... AS ENUM (...), update must say ALTER TYPE ... ADD VALUE... Needs a label-set diff") and lists 3 enum-related unmatched findings for 0.2.3→0.3.0. But this revision's enum_gaps (bin/update_lint_textfirst:308-329) implements exactly that label-set diff, and bin/test/textfirst.t:81-120 tests it directly. My manual trace above indicates the real 0.2.3→0.3.0 pair's three enum-gain cases are now matched via the ALTER TYPE ... ADD VALUE statements already present in the update script, not left unmatched. Since this PR exists specifically to be numerically compared against #90, worth updating the body's table/stub-list before anyone uses those numbers for the comparison.

2. Two findings from the previous review of this PR remain unaddressed in this revision (still present at current line numbers)

• Waiver regex breaks on a literal / in the pattern (bin/update_lint_textfirst:339): m{--\s*update-lint:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z} still has no escape mechanism, so a waiver regex containing / (an operator, a path literal, a character class) truncates silently.
• adp_gaps still recognizes only the IN SCHEMA ... GRANT ... ON TYPES TO ... shape (bin/update_lint_textfirst:372-376); FOR ROLE ... or omitted IN SCHEMA (global default privileges) would silently produce no finding rather than erroring.

Neither blocks the prototype's stated purpose (there's a documented escape hatch, and the tool is explicit that only the TYPES category is implemented), but flagging since they were raised before and the code shape is unchanged.

3. Minor, new-to-this-revision observations

• adp_gaps's "already existed" check re-derives its own notion of "in old" (%in_old at bin/update_lint_textfirst:367) via plain presence in @$old, rather than reusing the multiset-aware @added/@removed computed in main. If an ADP statement were ever duplicated (occurs once in OLD, twice in NEW), this presence check would treat it as "not new" even though one copy is. Purely theoretical for ADP statements in practice (they're unique per schema/role), so low priority.
• A waiver comment missing its mandatory reason parses as "not a waiver" rather than an error (bin/update_lint_textfirst:339, e.g. -- update-lint: ok /regex/ with nothing after). The header says "the reason is mandatory," but in practice this just means the line silently fails to register as a waiver — the underlying finding still fires (so it's not a silent bypass), but the author gets no specific feedback that their comment was malformed vs. simply absent. Consistent with the tool's general "fail loud on staleness, not on malformed input" posture, so a nit rather than a bug.

What looks solid on inspection

• The splitter (quote/dollar-quote/comment/;-boundary handling) and the multiset added/removed diff (%budget accounting in bin/update_lint_textfirst:480-484) are correct by trace, including the case where an unchanged CREATE TYPE (same labels) never reaches enum_gaps at all.
• enum_gaps's if (my $gaps = enum_gaps(...)) correctly treats an empty-but-defined arrayref as "handled, no gaps" rather than misreading it as false (Perl arrayrefs are always truthy) — this is the kind of thing that's easy to get wrong and it's right here.
• is_scaffolding's __\Q$ext\E\b anchor correctly rejects a differently-prefixed schema (e.g. a hypothetical __cat_tools_v2) while still matching schema-qualified names like __cat_tools.helper.
• CI wiring (.github/workflows/ci.yml, Makefile) looks consistent with the repo's CLAUDE.md build system notes: both sql/<ext>.control and the accumulator sql/cat_tools--<old>--stable.sql.in are tracked files present on a plain checkout, so make update-lint in the lint job (no pgxn-tools container, no build step) should work without a make invocation first.

Overall

Consistent with the PR's own "honest comparison" section: the core matcher and its one real check (ADP-on-preexisting-objects) hold up under manual trace against the real tree, and enum-label handling has moved from stub to implemented since the body was written. The two carried-over findings (waiver / escaping, single ADP shape) are pre-existing and non-blocking for a comparison prototype; the main actionable item here is refreshing the PR body's numbers/stub table so the #90 comparison it's built for stays accurate.

[github-actions]

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

---

Review: PR #92 — text-first update-script checker (prototype, comparison against #90)

• Gather context (PR body, diff, CLAUDE.md conventions)
• Read bin/update_lint_textfirst and bin/test/textfirst.t in full (this revision: 551/214 lines, commits through 52f2d94)
• Attempt to run prove bin/test/textfirst.t
• Review for bugs, edge cases, and design issues
• Post review feedback

Note on testing: as in the previous two reviews of this PR, perl/prove execution wasn't approved in this sandbox, so I hand-traced the logic against the real sql/cat_tools--*.sql.in pairs instead of running the suite. bin/test/textfirst.t's tests => 31 plan matches the actual count of is/like/unlike/is_deeply calls in the file (21 unconditional + 4 from the two-iteration typo loop + 6 in the SKIP block against the real tree).

1. The PR description contradicts what this revision actually does (new, not previously flagged)

The body states "Nothing here is wired into the Makefile or CI" — but the diff now includes:

• .github/workflows/ci.yml: two new steps in the lint job, prove bin/test/textfirst.t and make update-lint, with no continue-on-error — this is a real, hard-failing gate, not inert.
• Makefile: a new .PHONY: update-lint target running the checker for real.
• CLAUDE.md: a new bullet documenting make update-lint as an established, CI-enforced convention ("make update-lint checks that statically... and runs in CI's lint job"`), not a prototype note.

This is a meaningful gap between the PR's own framing ("PROTOTYPE, do not merge... nothing wired into CI") and its actual effect: merging this branch as-is would land a new mandatory CI gate and a permanent CLAUDE.md convention, not just a comparison artifact sitting inert next to #90. Worth reconciling the body (or the wiring) before anyone treats "do not merge" as low-risk to leave open/rebase-forward.

2. The numbers/"what it stubs" table is still stale (raised in the last two reviews, still unaddressed)

The body still lists enum-value handling as a stub ("Needs a label-set diff") and lists 3 enum-related unmatched findings for 0.2.3→0.3.0. This revision's enum_gaps (bin/update_lint_textfirst:313-334) fully implements that label-set diff and is directly tested (bin/test/textfirst.t:81-120). Since this PR exists specifically to be numerically compared against #90, the stale table undercounts this tool's real coverage.

3. Two findings from prior reviews remain unaddressed at current line numbers

• Waiver regex has no escape for a literal / in the pattern (bin/update_lint_textfirst:352): m{\A:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z} still truncates non-greedily at the first /, with no \/ escape. A waiver regex matching SQL text containing / (an operator, a character class, a path) silently compiles to something shorter/different than intended.
• adp_gaps only recognizes one ALTER DEFAULT PRIVILEGES shape (bin/update_lint_textfirst:385-389): IN SCHEMA ... GRANT ... ON TYPES TO .... FOR ROLE ... or an omitted IN SCHEMA (global default privileges) would silently produce no finding. The header now explicitly documents the PRIOR TO/REQUIRES preprocessing assumption (a genuine improvement since the first review), but still doesn't note this ADP shape assumption.

4. New minor observation: parse_waivers isn't quote/dollar-quote aware

parse_waivers (bin/update_lint_textfirst:340-359) scans $text line-by-line with a plain --\s*update-lint\b match, unlike split_statements's quote/comment/dollar-quote-aware scanner used everywhere else in the file. A -- update-lint...-looking substring inside a string literal or a dollar-quoted function body (e.g., documentation text describing this very tool) would be parsed as a real waiver line — and, if it doesn't happen to match the expected waiver grammar, would trigger a hard usage_error even though it's not actually a comment. Narrow in practice (nothing in the current tree triggers it), but worth noting since the rest of the tool is careful about exactly this distinction.

What looks solid on inspection

• The splitter (quote/dollar-quote/comment/;-boundary handling) and the multiset added/removed diff (bin/update_lint_textfirst:493-497) are correct by trace.
• The now-hard-error path for a malformed waiver (87dbb47) correctly covers both "no reason" and "mistyped keyword" cases, and is tested (bin/test/textfirst.t:171-183).
• Manually verified against the real tree: --versions 0.2.1 0.2.2 finds exactly the five enum types (constraint_type, procedure_type, relation_type, relation_relkind, object_type) with no GRANT USAGE, matching sql/cat_tools--0.2.1--0.2.2.sql.in's actual content; --versions 0.2.2 0.2.3 raises nothing since the ADP statement is unchanged between those installs; the 0.2.3→0.3.0 enum-label additions are matched via the ALTER TYPE ... ADD VALUE statements already present in that update script, not left unmatched.

Overall

Consistent with the PR's stated purpose (non-merge comparison prototype): the core matcher, the enum-label diff, and the one real ADP check hold up under manual trace against the real tree. The most actionable item is reconciling the PR body with what this revision actually does — both the stub/numbers table (stale across three revisions now) and, new this round, the "nothing wired into CI" claim that the diff itself contradicts.

[github-actions]

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

---

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

• Gather context (PR body, diff, CLAUDE.md conventions, prior review comments)
• Read bin/update_lint_textfirst and bin/test/textfirst.t (current revision, through 9f807c8)
• Diff against origin/master (.github/workflows/ci.yml, CLAUDE.md, Makefile)
• Review for bugs, edge cases, and design issues
• Post review feedback

Note on testing: as in every prior review of this PR, perl/prove execution wasn't approved in this sandbox, so I hand-traced the logic instead of running the suite. bin/test/textfirst.t's tests => 28 plan matches the actual count of is/like/unlike/is_deeply calls in the file (18 unconditional + 4 from the two-iteration typo loop + 6 in the SKIP block).

1. The PR description is now significantly stale — it describes machinery this revision removed

Commit 9f807c8 ("Drop enum label checking; the pgTAP suite verifies enums against the catalog") deleted enum_gaps entirely. The code no longer verifies that added enum labels have a matching ALTER TYPE ... ADD VALUE in the update script — any changed CREATE TYPE ... AS ENUM on a pre-existing type is now unconditionally exempted (bin/update_lint_textfirst:495-496), with the rationale (a good one) that bin/test_existing update-scenario in the test CI job already exercises the accumulator update path end-to-end and the pgTAP suite asserts enum labels against the real catalog for every PG version in the matrix.

But the PR body still describes this as one of "the three cases that needed real code" — "checked label by label," BEFORE/AFTER clauses, a "removed label" message — none of which exist anymore. It still lists per-pair enum finding counts in the results table and an "Enum label ORDER is not checked" bullet under "Documented rather than implemented." The size table (280/213/58/551 for the checker, 154/34/26/214 for the test file) and "31 tests" claim are also both stale: the files are actually 526 and 197 lines, and the test plan is 28.

Since this PR exists specifically to be a numeric point of comparison (against #90, and for reviewers judging it), it'd be worth refreshing the body to match what's actually implemented before this is used for that purpose. This has been flagged in each of the last two reviews for a smaller version of the same gap; it's now larger since a whole feature was removed.

2. Two findings from prior reviews remain unaddressed (posted inline again at current line numbers)

• Waiver regex has no escape for a literal / (bin/update_lint_textfirst:330) — a waiver regex containing / (an operator, a character class, a path) silently truncates instead of erroring.
• adp_gaps only recognizes the IN SCHEMA ... GRANT ... ON TYPES TO ... shape (bin/update_lint_textfirst:363-367) — FOR ROLE ... or global (no IN SCHEMA) default privileges would silently produce no finding.

3. New minor observation (posted inline)

• parse_waivers isn't quote/dollar-quote aware (bin/update_lint_textfirst:324), unlike the rest of the file's careful scanner. A -- update-lint-looking substring inside a string literal or dollar-quoted body would be parsed as a real waiver directive rather than inert text.

What looks solid on inspection

• Verified the accumulator resolution logic (current_version/current_pair, bin/update_lint_textfirst:389-422) against the actual tree: cat_tools.control's default_version = 'stable' correctly routes NEW to the base sql/cat_tools.sql.in and resolves the accumulator via sql/cat_tools--*--stable.sql.in, matching the one file on disk (sql/cat_tools--0.3.0--stable.sql.in).
• Confirmed .github/workflows/ci.yml's test job runs bin/test_existing update-scenario cat_tools_update 0.2.2 on every supported PG version for every non-docs-only PR — this is the real coverage the enum-removal rationale depends on, and it checks out.
• The splitter (quote/dollar-quote/comment/;-boundary handling) and the multiset added/removed diff (bin/update_lint_textfirst:471-475) are correct by trace.
• CLAUDE.md and Makefile changes are consistent with the code: the new make update-lint target and CI steps match what's described, and the "not tied to lint" rationale (tarball has no .git, so lint.mk's include is guarded) is accurate.

Overall

The core matcher, scaffolding exemption, and ADP check all hold up under manual trace. The main actionable item is the same one raised in the last two reviews, now bigger: the PR body needs a pass to match what this revision actually does, especially since its self-comparison numbers are the point of the exercise. The two carried-over code findings and the new one are non-blocking, consistent with this being an explicitly-scoped, escape-hatch-backed tool.

[github-actions]

The waiver regex has no escape mechanism for a literal / in the pattern:

unless $rest =~ m{\A:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z};

(.+?) is non-greedy, so it stops at the first / after the opening delimiter. A waiver meant to match SQL text containing a / (an operator, a path literal, a character class like [a/b]) silently truncates to a shorter/different regex instead of erroring — the failure mode is a confusing "stale waiver" or an unintended match, not a parse error. Given waivers are the escape hatch for every case this tool can't handle by copy-matching, this seems worth an escape (\/) or a delimiter unlikely to appear in SQL. (Raised in prior reviews of this PR; still present at the current line.)

[jnasbyupgrade]

Fixed in de8d51d: whitespace, not the first /, now closes the regex (/(.+?)/\s+), so a waiver pattern may contain a slash. bin/test/textfirst.t covers it — -- update-lint: ok /a/b/ division is fine waives SELECT a/b and keeps the whole reason intact.

🤖 Generated with Claude Code

[github-actions]

adp_gaps only recognizes one ALTER DEFAULT PRIVILEGES shape:

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, but Postgres also allows ALTER DEFAULT PRIVILEGES FOR ROLE ... and omitting IN SCHEMA entirely (global defaults). Either form silently fails to match and the check does nothing — no error, no finding — for what could still be a real "objects predate this ADP" gap. Since this is the one gap category implemented "for real," worth at least a comment noting the syntax assumption so a future author extending this (e.g. to add TABLES/FUNCTIONS coverage) doesn't assume all ADP shapes are handled. (Raised in prior reviews; still present.)

[jnasbyupgrade]

I think the right thing here is to detect use of unsupported cases and error. We don't need to boil the ocean, but we should still post lookouts for icebergs (or something...).

However, there's a bigger issue: unless I'm misunderstanding the perl regex, the regex is wrong because of the hard-coded spaces? Assuming I'm just mis-understanding I'd say it's worth a BRIEF comment on why this works.

[jnasbyupgrade]

Both done.

Unsupported cases now error. Anything starting ALTER DEFAULT PRIVILEGES that does not match the one understood form is reported as unrecognized ALTER DEFAULT PRIVILEGES form rather than falling through the old next unless — waivable like any other finding, so it is a lookout, not a wall. New case in bin/test/textfirst.t covers it with the FOR ROLE shape.

On the regex: no hard-coded spaces. /x makes literal whitespace in the pattern insignificant, so every gap between keywords is the explicit \s+ written next to it, and those match the single spaces split_statements already collapsed the statement down to. Added as a two-line comment at the regex.

🤖 Generated with Claude Code

[github-actions]

parse_waivers scans $text line-by-line with a plain --\s*update-lint\b match, unlike split_statements's quote/comment/dollar-quote-aware scanner used everywhere else in this file. A -- update-lint...-looking substring inside a string literal or a dollar-quoted function body (e.g. documentation text describing this tool, quoted inside a COMMENT ON FUNCTION or similar) would be parsed as a real waiver directive and, if it doesn't match the expected grammar, would trigger a hard usage_error even though it isn't actually a comment. Narrow in practice (nothing in the current tree triggers it), but worth noting since the rest of the tool is careful about exactly this distinction.

[jnasbyupgrade]

I think this is worth a comment as a known issue, but not a fix.

[jnasbyupgrade]

Comment only, no fix. The LIMITATIONS bullet on line-by-line waiver scanning now names the actual sharp edge — a -- update-lint inside a string or a dollar-quoted body that does not parse is a hard error naming a line that is not a comment at all — and says a second scanner costs more than the case is worth.

🤖 Generated with Claude Code

[github-actions]

Claude finished @jnasbyupgrade's task in 1m 44s —— View job

---

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

• Gather context (PR body, diff, CLAUDE.md conventions, prior review comments)
• Review the delta since the last review (commit de8d51d, "Let a waiver regex contain a slash; document two declined cases")
• Post review feedback

Note on testing: as in every prior review of this PR, perl execution wasn't approved in this sandbox, so I hand-traced the regex instead of running prove.

This commit's fix is real but not complete

de8d51d changes the waiver-parsing regex (bin/update_lint_textfirst:345) from

m{\A:\s*ok\s+/(.+?)/\s*(\S.*?)\s*\z}

to

m{\A:\s*ok\s+/(.+?)/\s+(\S.*?)\s*\z}

i.e. requiring mandatory whitespace (\s+, not \s*) between the closing / and the reason. This does fix the case raised in every prior review of this PR (and tested now at bin/test/textfirst.t:153-162, -- update-lint: ok /a/b/ division is fine): a / immediately followed by a non-whitespace character (part of the regex) forces backtracking past it, so the full intended regex a/b is captured rather than truncating to a.

However, tracing the same backtracking logic against a pattern where the embedded / is followed by a space — e.g. a waiver written as -- update-lint: ok /a/ b/ this is the reason (intended regex: a/ b) — shows the truncation bug still reproduces:

1. Non-greedy group 1 first tries a, matches the delimiter / at the next character.
2. \s+ now finds the following space and consumes it — satisfied.
3. Group 2 (\S.*?) then starts at b, and expands (backtracking) until \s*\z matches at end of string — which succeeds by swallowing b/ this is the reason entirely as the "reason".
4. Result: re="a" (silently truncated from the intended a/ b), why="b/ this is the reason" (silently absorbing part of the regex) — a full match from \A to \z, so no error is raised.

So the fix narrows the bug (a / followed immediately by non-whitespace is now safe) but doesn't close it for a regex that itself contains a / followed by whitespace (a plausible shape: an alternation, a comment/path fragment, etc.). This edge case isn't covered by the new test either. Given the PR's own escape-hatch design leans on this exact regex being trustworthy, it may be worth a follow-up (e.g. requiring the reason to start on non-whitespace and rejecting/warning if the regex source contains an unescaped /+whitespace sequence, or switching to an explicit escape like \/) — but this is a narrower residual case than what was originally flagged, not a regression.

Other previously-raised items

The two other findings from prior reviews (adp_gaps only recognizing the IN SCHEMA ... ON TYPES ADP shape; parse_waivers not being quote/dollar-quote aware) are, as of this commit, explicitly documented as declined-on-purpose in the header (bin/update_lint_textfirst:79-84 and :141-144) rather than left as silent gaps — consistent with the tool's stated design principle of declaring what's out of scope rather than silently missing it. Nothing to add there.

Overall

The slash-escaping fix is a solid, well-tested improvement over the previous revision and closes the common case. One narrower edge case (regex containing / immediately followed by whitespace) remains under the new mandatory-whitespace scheme and isn't covered by a test — worth a note for whoever picks this back up, not a blocker given this PR's own framing.

[jnasbyupgrade]

uh... lets create a generic script/Makefile/whatever for running all tests in bin/test/. I don't remember if we already had that or not. Basically, because othr tests make use of stuff in bin/, we should test bin/ before anything else.

[jnasbyupgrade]

Added make test-bin, which runs prove bin/test/ — the directory, so a new .t needs no CI edit — and made it the first step of the lint job, ahead of make lint and make update-lint. We did not already have one; CI named bin/test/textfirst.t directly.

The cross-job half of "before anything else" is not done. The heavy test job uses bin/test_existing and bin/structural_diff but only needs: [changes], so it still starts in parallel with lint. Making that ordering real means either a new job everything needs: (which also means editing all-checks-passed, since it hard-fails when the workflow's job-key set differs from its needs:) or adding lint to test's needs: and serializing the whole matrix behind the linter. Raising that separately rather than picking one here.

🤖 Generated with Claude Code

[jnasbyupgrade]

Not a fan of the name... how'd we end up with it?

[jnasbyupgrade]

How we got it: bin/update_lint was the semantic-parser version in #90. This one arrived alongside it as the text-first alternative, and the suffix is what told the two apart while both existed. #90 is closed and that file never reached master, so the suffix no longer distinguishes anything, and make update-lint is already the target name.

Leaving the actual rename to you rather than guessing at a name.

🤖 Generated with Claude Code

[jnasbyupgrade]

thought we decided NOT to drop comments? I don't see any value to it, esp. since many of these commands should literally be identical.

[jnasbyupgrade]

This and your note on normalized_blob (dedent instead of collapsing whitespace) are the same question — dropping comments and collapsing whitespace are one normalization step, and keeping comments only works if the whitespace handling changes with it. Not changing either piecemeal; raising the normalization as a whole separately rather than guessing which half you want.

🤖 Generated with Claude Code

[jnasbyupgrade]

Uh... not sure this is the right call. If we have a SED actually in effect in the base script, I'd be inclined to think it should ALSO be in effect in the relevant update scripts, which means there should be a make rule for processing them as well. If that's the case, we really shouldn't be duping that code here unless there's a really good reason to.

Also, given that those are not checked in (right?), I'd say we should actually be testing the base files. HOWEVER, given that the processed files are what actually run on the server, we should ALSO be testing those as well.

[jnasbyupgrade]

Two facts worth having before this gets decided, then I am leaving the decision to you:

1. There already is a make rule processing update scripts. sql.mk's sql/%.sql: sql/%.sql.in pattern rule matches them, and upgrade_scripts_out puts the results into DATA, so the SEDs are in effect there today — sql/cat_tools--0.2.3--0.3.0.sql.in carries one -- SED: marker and it does get resolved.
2. Correct, the .sql files are not checked in. The checker reads the .sql.in sources, which is what keeps it build-free and database-free — it runs in the lint job, not the pgxn-tools container. Also checking the processed files means depending on a build, and there is no single processed output to check: the version SEDs are MAJORVER-dependent, so the .sql differs per PostgreSQL major.

Raising the "test both" question separately rather than acting on it, together with your VERSIONED FILE! comment, which points the same direction.

🤖 Generated with Claude Code

[jnasbyupgrade]

This is a case where some higher-level thinking comes into play. The ONLY reason for those tags is to try and prevent someone from mistakenly looking at a generated file (because that's almost guaranteed to be a mistake). To achieve that, it's good if the tags appear every screen/page or so. BUT, if there's only ONE place where it's showing up in actual SQL then we really need to ask whether it's worth extra code to handle that. I lean towards the answer to that being "no".

[jnasbyupgrade]

Two readings of this and I do not want to guess which you mean:

1. Drop the (?:\s+VERSIONED\s+FILE!)? handling here. That one @generated@ inside __cat_tools.create_function()'s dollar-quoted body then differs between the base file and every versioned copy, so the cost is a false positive and a permanent waiver rather than saved code.
2. Drop the tagging itself from sql.mk's copy rule, so nothing needs handling on either side. That is the one that actually removes code, but it is a change to what the generated files say, not to this checker.

Raising it with your preprocessing-duplication comment rather than picking one.

🤖 Generated with Claude Code

[jnasbyupgrade]

ISTM we could do the same thing by just removing the appropriate amount of indentation from whichever version has more indentation... and I'm guessing that means we don't have to screw with comments.

[jnasbyupgrade]

Same question as your comment on comment-dropping at the header's step 2 — that is what dedenting would have to replace, so I am raising both together rather than reworking the core normalization on a guess.

One thing that bears on the choice: collapsing whitespace absorbs more than leading indentation. A copy in the update script is often re-wrapped across different line breaks as well as re-indented, and rule 4 matches the statement as a substring of the whole update file rather than statement-against-statement, so "remove the appropriate amount of indentation" would need a rule for how much, measured from where, and would still have to handle the line breaks not lining up.

🤖 Generated with Claude Code

[jnasbyupgrade]

I think this is worth a comment as a known issue, but not a fix.

[jnasbyupgrade]

I think the right thing here is to detect use of unsupported cases and error. We don't need to boil the ocean, but we should still post lookouts for icebergs (or something...).

However, there's a bigger issue: unless I'm misunderstanding the perl regex, the regex is wrong because of the hard-coded spaces? Assuming I'm just mis-understanding I'd say it's worth a BRIEF comment on why this works.

[jnasbyupgrade]

I'd say it's worth a comment (somewhere) that there can be multiple update scripts that result in the same version (which is why --*-- is needed).

[jnasbyupgrade]

Added at current_pair: a version can be reached from several others — this tree ships both 0.2.0--0.2.2 and 0.2.1--0.2.2 — so the target version alone does not name a file, hence the --*-- glob. The comment also says why exactly one is still expected for the current cycle, and that two would mean the pair being checked is ambiguous.

🤖 Generated with Claude Code

[jnasbyupgrade]

s/accumulator//. We should just refer to these as update scripts.

[jnasbyupgrade]

Done — dropped from CLAUDE.md, from bin/update_lint_textfirst's header and comments, and from the one test description that used it. They are just update scripts now.

🤖 Generated with Claude Code

[github-actions]

Claude finished @jnasbyupgrade's task in 7m 41s —— View job

---

Reviewing PR #92

• Gather context (PR body, diff, CLAUDE.md conventions, prior reviews)
• Run /code-review skill against this PR (in progress — background agents dispatched)
• Post summary