⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.79%. Comparing base (8885726) to head (050380b).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main     #714   +/-   ##
=======================================
  Coverage   68.79%   68.79%           
=======================================
  Files          46       46           
  Lines       30992    30992           
=======================================
  Hits        21318    21318           
  Misses       9674     9674           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

REV Review — PR #714: test(ci): add SSL mode connection tests (D1-D6)

Summary

Adds Section D SSL/TLS tests (D1-D6) to the connection compatibility suite, with a self-signed TLS postgres container launched in CI. Coverage is solid for the happy path and known-broken cases are honestly documented as SKIP with issue references; a few correctness and robustness issues need attention before merge.

Blocking findings

BUG — D4 will false-pass when ssl=off postgres is absent from CI env

test_ssl_require_fail (D4) connects to TEST_PGHOST/TEST_PGPORT — the existing plain postgres service. In the current CI job those variables are only set in the env: block of the Run connection tests step, but the step name in the diff shows the env vars were already there for the pre-existing tests. The real risk: if the plain server happens to have ssl=on (which it does not today, but is not enforced), sslmode=require will succeed, exit_code will be 0, and the test reports FAIL — correct. However if TEST_PGHOST is unset and falls back to empty string, rpg may refuse to connect for a different reason (bad hostname), exit non-zero, but the stderr output will not mention "SSL" or "TLS", causing the second assert to fail with a misleading FAIL: D4 … exited N but no SSL/TLS mention. The test does not guard against TEST_PGHOST being unset before entering test_ssl_section. The outer guard only checks TEST_PG_TLS_PORT — it does not validate that the plain-server env vars are present. If someone runs the script locally without TEST_PGHOST set, set -u will abort the entire script at D4, not just skip the section.

• File: tests/compat/test-connections.sh, around line ~420 (D4 function)
• Fix: add a guard at the top of test_ssl_require_fail (or in test_ssl_section) that skips D4 when TEST_PGHOST is empty, analogous to the TEST_PG_TLS_PORT check.

BUG — TLS container startup race: pg_isready loop does not fail CI on timeout

.github/workflows/checks.yml, "Start TLS postgres" step:

for i in $(seq 1 20); do
  docker exec pg-tls pg_isready -U postgres && break || sleep 1
done

If postgres never becomes ready the loop finishes silently with exit code 0 (the last command is sleep 1, which succeeds). The subsequent "Run connection tests" step then runs against a container that is not ready and produces confusing SSL errors instead of a clear "container never started" message. Fix: add a check after the loop, e.g.:

docker exec pg-tls pg_isready -U postgres || { echo "pg-tls never became ready" >&2; exit 1; }

Potential issues

MEDIUM — (( FAIL++ )) || true pattern is fragile

This pattern is pre-existing in the file but the new Section D code copies it throughout. With set -Eeuo pipefail, (( FAIL++ )) evaluates to exit code 1 when FAIL is 0 before the increment (arithmetic expressions return 1 when the result is 0). The || true suppresses that. However if FAIL is somehow unset, set -u will abort — but FAIL is initialised at the top so this is unlikely. The pattern is fragile and not idiomatic; FAIL=$(( FAIL + 1 )) is safer and passes ShellCheck cleanly.

MEDIUM — Self-signed cert ownership path in Docker entrypoint

The cert is copied into /tmp/pg.crt and /tmp/pg.key inside the container. Postgres typically refuses to start if the data directory or cert is world-readable. The chmod 600 /tmp/pg.key is present but /tmp/pg.crt is left at default permissions. While Postgres only checks key permissions, future changes to the entrypoint or base image could break this silently. More robustly, use /var/lib/postgresql/ (already owned by postgres) instead of /tmp/.

LOW — D2 sslmode=prefer does not verify SSL was actually used

D2 only checks exit code 0. On a TLS server with sslmode=prefer, SSL should be negotiated, but the test does not confirm ssl=t via pg_stat_ssl. This means D2 cannot catch a regression where prefer silently falls back to plaintext. Low severity given D3 already covers sslmode=require with the ssl column check, but a pg_stat_ssl assertion here would close the gap.

LOW — Trailing whitespace added

tests/compat/test-connections.sh, the echo "=== Results: …" line has trailing whitespace added by this PR (visible in the diff as a space before the closing "). Minor, but fails git diff --check.

LOW — Em dash in comment uses hyphen

# (e) Environment variables only - no CLI connection flags
# instead: set PGDATABASE=wrongdb but pass -d <real-db> - connection

Both changed to - from —. The postgres-ai writing rules specify em dash with spaces (word — word). The hyphens were intentional (they replace the original —) — but the original used — and the PR downgrades them. Not a blocker, just inconsistent with org style.

Test coverage

Section D adds 6 test cases covering the full sslmode spectrum:

• D1 (disable): connects, verifies ssl=f via pg_stat_ssl — good positive + assertion
• D2 (prefer): exit-code-only check — passes but weak (see potential issues above)
• D3 (require vs TLS server): connects, verifies ssl=t — this is the regression test for feat(ssl): fully support sslmode=require with proper error handling #710/fix(ssl): implement sslmode=require without cert verification #711, well done
• D4 (require vs plain server): verifies non-zero exit + SSL/TLS mention in stderr — solid negative test
• D5 (verify-ca): explicit SKIP with issue reference (fix(ssl): sslmode=verify-ca and verify-full fail with UnknownIssuer when server does not send full chain #712) — acceptable
• D6 (verify-full): same SKIP — acceptable

CI integration: the TLS container is started before the test step and the env vars are correctly injected. The skip-when-unset guard in test_ssl_section ensures the tests are non-breaking for local runs without a TLS server.

Missing scenarios worth tracking (not blocking):

• sslmode=allow is not covered (not a regression risk, but part of the full matrix)
• No test that verifies PGSSLCERT/PGSSLKEY env vars are forwarded (client cert auth)
• No test for TLS container startup failure reporting (covered above as blocking)

Overall coverage for the stated scope (D1-D6) is adequate, with D3 being the most important test and correctly implemented.

Verdict

REQUEST_CHANGES

Two blocking issues need fixes: (1) D4's missing guard against unset TEST_PGHOST under set -u, and (2) the silent swallow of the pg_isready readiness loop failure in CI. Both are one-liners to fix. D2's weak assertion and the trailing whitespace are nice-to-haves.