Skip to content

Fix results ordering, control file whitespace, ENABLE_* override, debug levels#31

Merged
jnasbyupgrade merged 6 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix-pr69-issues
Jul 14, 2026
Merged

Fix results ordering, control file whitespace, ENABLE_* override, debug levels#31
jnasbyupgrade merged 6 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix-pr69-issues

Conversation

@jnasbyupgrade

@jnasbyupgrade jnasbyupgrade commented May 14, 2026

Copy link
Copy Markdown
Contributor

Related pgxntool-test PR: Postgres-Extensions/pgxntool-test#19

Summary

  • Fix results: prerequisite ordering: verify-results ran before make test, checking stale regression.diffs from a prior run; reordered to results: test verify-results
  • Fix parse_control_file trailing whitespace: ${value%% } only stripped one trailing space; now uses a regex to strip all trailing spaces and tabs (e.g. default_version = '1.0.0' # comment)
  • Add override to PGXNTOOL_ENABLE_TEST_BUILD/PGXNTOOL_ENABLE_TEST_INSTALL so command-line values are normalized by pgxntool_validate_yesno rather than silently ignored
  • Fix pgtle.sh debug levels: stray single-digit values (1–4) replaced with verbosity tiers (30/40/50) consistent with the lib.sh scheme
  • Rework the debug() level docs in lib.sh and .claude/development.md: levels encode verbosity/esotericness, not code nesting depth (a top-level line can be high if it's esoteric; loop-body detail is high because it's noisy). The 10/20/30/40/50 tiers are anchors, not strict multiples of 10 — any in-range value is fine, so fine-tuning needs no renumbering
  • Clarify CLAUDE.md audience: for extension developers using pgxntool, not contributors; redirect contributors to .claude/
  • Move developer guidelines (Makefile := rules, debug level rules) from CLAUDE.md into .claude/development.md, which also now prominently states that pgxntool changes must be made from a pgxntool-test checkout
  • Update HISTORY.asc with entries for the two user-visible fixes

@jnasbyupgrade

Copy link
Copy Markdown
Contributor Author

@claude please review this PR.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

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


Review of PR #31

  • Gather context (read diff, changed files)
  • Review base.mk changes (results ordering, ENABLE_* override)
  • Review lib.sh changes (debug level docs)
  • Review pgtle.sh changes (control file parsing, debug levels)
  • Review CLAUDE.md / .claude/development.md changes
  • Review HISTORY.asc changes
  • Post final review

Summary

Overall this is a solid, well-documented set of fixes. The two functional/user-visible fixes (results ordering, control-file whitespace trimming) are correct and the reasoning in the added comments checks out against Make/bash semantics. One inconsistency worth resolving before merge:

Finding: pgtle.sh debug levels don't actually follow the new lib.sh scheme

lib.sh:64-71 (and the mirrored .claude/development.md doc) now documents a nesting-depth-based scheme:

  • 1-9: high-level script context (top-level entry/exit, arguments)
  • 10-19: per-iteration detail inside loops
  • 20-29: nesting inside loop-level detail
  • higher ranges follow the same pattern

But the values this PR assigns in pgtle.sh don't follow that pattern — they're a straight substitution of the old tiered scheme (10=critical, 20=warning, 30=general/entry-exit, 40=verbose/loop, 50=max verbosity), just scaled from the stray 1-4 typos to 30/40/50:

  • pgtle.sh:159debug 30 "Global arrays initialized..." fires at global script scope (not even inside a function), yet is 30. Per the new scheme this is the textbook 1-9 case ("top-level entry/exit").
  • pgtle.sh:838debug 50 "generate_pgtle_sql: Processing upgrade file $i..." is the one call that's genuinely inside a loop, per-iteration — exactly the doc's 10-19 example ("entry (11)") — but it's assigned 50, the opposite end of the range.
  • None of the 17 debug calls in pgtle.sh use anything in 1-9 or 10-19, despite the file containing multiple loops (discover_sql_files, generate_pgtle_sql's upgrade-path loop).

Pre-existing usage in update-setup-files.sh:65,85,128 (debug 20/debug 30, flat, non-nested) shows the tiered convention is what's actually used elsewhere in the codebase too — so this may just mean the new doc describes an aspirational scheme nothing follows yet, rather than a bug unique to this PR. But as written, the PR body's claim ("range-based values ... per lib.sh convention") isn't quite accurate: the new lib.sh convention is about nesting depth, and these values don't reflect nesting depth. Worth either:

  1. Renumbering the pgtle.sh calls to reflect actual nesting (top-level ≈ 1-9, the one real loop iteration ≈ 10-19), or
  2. Adjusting the lib.sh/development.md doc to describe the tiered-verbosity scheme that's actually in use.

Fix this →

Other things checked, no issues found

  • base.mk:303-309 — the verify-results: test / results: verify-results dependency chain correctly forces serialization even under make -j; the recipe at base.mk:310-314 merges correctly with the prerequisite-only rule (standard Make behavior for multiple rules on the same target).
  • base.mk:96-98,140-142 — adding override to PGXNTOOL_ENABLE_TEST_BUILD/PGXNTOOL_ENABLE_TEST_INSTALL is correct and brings them in line with the pre-existing PGXNTOOL_ENABLE_VERIFY_RESULTS pattern (base.mk:176), which already used override := for the same reason.
  • pgtle.sh:466-474 — the new regex-based trailing-whitespace strip correctly handles multiple spaces/tabs before a comment, unlike the old ${value%% } (which only strips one literal space char). No regex-injection concern since value is the subject, not the pattern, in the [[ =~ ]] test.
  • .gitattributes already export-ignores .claude/ and *.md, so the new .claude/development.md won't leak into git subtree distributions.
  • CLAUDE.md/HISTORY.asc edits are clear and consistent with the rest of the docs.

I did not run make test (no live Postgres in this environment) — the two functional fixes were verified by code reading against Make/bash semantics rather than execution.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

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: Pro Plus

Run ID: 2c7d40a3-03e1-4da6-93a9-0d5512564106

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
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

jnasbyupgrade and others added 6 commits July 14, 2026 18:16
…ug levels

- Fix `results:` ordering: `verify-results` ran before `make test`, so it
  checked stale `regression.diffs`; reordered to `results: test verify-results`
- Fix `parse_control_file` trailing whitespace: `${value%% }` only removed one
  space; now uses regex to strip all trailing spaces and tabs
- Add `override` to `PGXNTOOL_ENABLE_TEST_BUILD`/`PGXNTOOL_ENABLE_TEST_INSTALL`
  so command-line values are normalized by `pgxntool_validate_yesno` rather
  than silently ignored
- Fix `pgtle.sh` debug levels: single-digit values (1–4) replaced with
  standard multiples-of-10 (30, 40, 50) per documented convention
- Add Makefile variable assignment and debug level rules to `CLAUDE.md`

Related changes in pgxntool-test:
- New test in `04-pgtle.bats`: verify `pgtle.sh` handles multiple spaces and
  tab before trailing comment in control files
- Updated `make-results.bats`: new test verifying `verify-results` blocks
  `make results` after a failing test run
- Updated `test-verify-results.bats`: reflect that `make results` now runs
  `make test` first

Co-Authored-By: Claude <noreply@anthropic.com>
Update the `debug()` comment in `lib.sh` to document the intended
range-based scheme: 1-9 for high-level script context, 10-19 for
per-loop-iteration detail, 20-29 for nesting inside that — with room
within each decade to fine-tune without renumbering. The old comment
described "multiples of 10" which was inaccurate and misleading.

Related changes in pgxntool-test:
- `helpers.bash`: test `debug()` now uses `$TESTDEBUG` instead of `$DEBUG`
- `SKILL.md`: added advisory debug level distribution check to commit workflow

Co-Authored-By: Claude <noreply@anthropic.com>
CLAUDE.md is for users of pgxntool. Developer-focused guidelines
(Makefile variable assignment rules, debug level scheme) belong in
.claude/ where they are visible to contributors but not surfaced to
extension developers reading the user docs.

Also updates the debug level rules to the range-based scheme documented
in lib.sh (1-9, 10-19, 20-29...) rather than the old multiples-of-10
description.

Co-Authored-By: Claude <noreply@anthropic.com>
CLAUDE.md is loaded by anyone who has pgxntool embedded in their project.
.claude/ is only relevant to people making changes to pgxntool itself.

- CLAUDE.md: rewrite "Scope" section to make clear it is for extension
  developers consuming pgxntool, not for pgxntool contributors
- .claude/development.md: add prominent "developers only" header and a
  "Work from pgxntool-test, Not Here" section explaining why changes must
  be made from a pgxntool-test checkout (full test infrastructure lives there)
- Fix test helper path reference: tests/lib/helpers.bash in pgxntool-test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
`results: test verify-results` lists both as independent prerequisites,
so `make -j` can run them concurrently. verify-results may finish before
test produces regression.diffs, find no failures, and allow the copy to
proceed — defeating the safeguard entirely.

Fix: add `verify-results: test` so the dependency chain is explicit. Make
guarantees prerequisite ordering only when there is an actual dependency,
not when targets are listed as siblings.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Level encodes how noisy/esoteric a message is (how far you'd crank DEBUG), not code nesting depth: a top-level line can be high if esoteric, and loop-body detail is usually high because it's noisy. Restores the verbosity tiers the code already follows, keeping the "anchors, not strict multiples" flexibility. Mirrored in .claude/development.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jnasbyupgrade added a commit to Postgres-Extensions/pgxntool-test that referenced this pull request Jul 14, 2026
Related pgxntool PR:
Postgres-Extensions/pgxntool#31

## Summary

Tests and template updates for pgxntool PR #31:

- `make-results.bats`: new test verifying `verify-results` blocks `make
results` when tests are failing (exercises the fixed `results: test
verify-results` ordering)
- `test-verify-results.bats`: realigned to the new `results: test`
ordering — `verify-results` now runs the suite, so tests that assumed
otherwise were updated/renamed, with clarifying comments
- New `04-pgtle.bats` test: `pgtle.sh` correctly handles multiple spaces
and a tab before a trailing comment in control files
- `template-multi-extension/ext_beta.control`: use multiple spaces
before trailing comment to exercise whitespace trimming
- `multi-extension.bats`: update comments for multiple-spaces testing
- Template test SQL: add `\set ECHO none` to
`base.sql`/`pgxntool-test.sql` so pg_regress output is stable across
psql versions, and regenerate the matching expected output — fixes a
latent template mismatch that #31's `verify-results: test` change
surfaced
- `test.md` agent: fix debug level description (the BATS `$TESTDEBUG`
1–5 scale is separate from `lib.sh`'s verbosity scale)

---------

Co-authored-by: Claude <noreply@anthropic.com>
@jnasbyupgrade jnasbyupgrade merged commit 9fbe6f4 into Postgres-Extensions:master Jul 14, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant