test(pg-compat): SP-1..SP-3 combined — PG protocol coverage, polyglot foundation, driver matrix (supersedes #5894, #5903, #5910) - #6020
Conversation
Design for a phased PostgreSQL protocol test program: - SP-1: TAP coverage gaps (auth matrix, data types, cursors, pool churn, LISTEN/NOTIFY rejection) in the existing harness, per-PR gating. - SP-2: polyglot test foundation (Toxiproxy + pytest runner + 4-target differential engine + pg_stat_statements routing oracle), nightly + pg-compat label. Informed by surveys of PgBouncer, pgcat, and pgdog test suites.
- New infras use dbdeployer (infra-dbdeployer-pgsql17-repl), matching the existing infra-dbdeployer-* convention; first dbdeployer PG infra. - Frame the initial phase as discovery (failure inventory, xfail catalogue), no expectation of 100% success; SP-2 CI is reporting-oriented. - Add backend-protocol mode (pgsql-use_native_backend_protocol off/on) as a first-class test axis, tracking native-backend PR #5882; differential harness grows to 6 targets (proxy-libpq / proxy-native / direct x text/binary). - Reframe LISTEN/NOTIFY as a per-mode contract test; NOTIFY forwarding is owned by #5882 (already ships pgsql-native_notify-t), not this spec.
Task-by-task TDD plan: extend pg_lite_client with MD5 + SCRAM-SHA-256 (reusing deps/libscram client funcs), then auth-method matrix, data-type/ binary matrix, server-side cursors, pool churn/session-isolation, and LISTEN/NOTIFY contract tests. Frontend auth driven via the integer pgsql-authentication_method variable (1=cleartext,2=md5,3=scram).
PR #5865 already adds pgsql-verifier_auth-t / -verifier_passthrough-t / pgsql_reconcile_unit-t covering the credential-storage x floor matrix, anti-enumeration, and backend pass-through via libpq (connect success/fail only, no queries, no wire challenge-type assertion). Reposition SP-1's auth-matrix test as the wire-level complement: assert the actual auth CHALLENGE type ProxySQL presents per floor (3/5/10) via a new pg_lite_client getLastAuthType() accessor, plus run a query to prove the session is usable. De-dup the storage-type/floor success/fail (owned by #5865). Add merge-order note and an optional #5865-gated no-downgrade wire assertion. MD5/SCRAM enabler tasks unchanged.
10-task plan: dbdeployer-PG validation spike + native fallback, primary+2 replica infra with pg_stat_statements, Toxiproxy sidecar, automatic pgsql_replication_hostgroups routing, in-container pytest harness with admin config-as-primitive, 6-target differential engine (proxy-libpq/ native x text/binary vs direct) + divergence self-check, pg_stat_statements routing oracle + write-pin self-check, shared behavior set + psycopg3 adapter, xfail catalogue, and nightly+label CI. Endpoints env-injected so the harness is decoupled from the topology decision.
Wire-level complement to PR #5865: assert the auth CHALLENGE type ProxySQL presents to the client (cleartext=3) and run a query to prove the session is usable — neither observable through libpq. Adds PgConnection::getLastAuthType() to pg_lite_client and the test scaffold (2 assertions); md5/scram land in the next tasks.
- try_frontend_login: consumeInputUntilReady() after execute() so it actually round-trips SELECT 1 (execute() only sends) — makes 'login + query succeed' honest; Tasks 2-3 reuse this helper. - BAIL_OUT if the admin auth-method SET/LOAD fails, so a config failure gives a clear diagnostic instead of a confusing wrong-challenge-type failure. Addresses both Minor findings from the Task 1 review; test still 2/2 green.
…age (Task 3 review)
Round-trips a representative literal per PG type (bool, int4, int8,
float8, numeric, text, bytea, uuid, timestamptz, jsonb, int4[], inet)
through the ProxySQL PG frontend via pg_lite_client's extended-protocol
API, asserting the DataRow value, the RowDescription type OID, and the
requested result-format code, in both text and binary result formats.
Adjustments vs the task brief:
- Makefile rule needs -lscram -lusual -Wl,--allow-multiple-definition:
pg_lite_client.cpp compiles doSASLAuth() unconditionally (not gated
behind an #ifdef), so those symbols are always referenced at link
time regardless of which auth method a given test exercises.
- run_case() binds via bindStatementEx() with an explicit empty
paramFormats array instead of bindStatementSingleFormat(). The
latter unconditionally sends a 1-element param-format array even
when there are 0 bind parameters, which trips a real ProxySQL bug:
PgSQL_Connection.cpp's stmt_execute_start() only expands
num_param_formats==1 into num_params when num_params > 1, so
num_param_formats==1 with 0 actual params falls into the
mismatch-error branch ("Invalid param format count: got 1,
expected 0"), even though the PG Bind message spec defines
num_param_formats==1 as applying to all parameters regardless of
their count (confirmed against PostgreSQL's own exec_bind_message,
which only errors when numPFormats > 1 && numPFormats != numParams).
Real clients never send a param-format array for a 0-param bind, so
the test now does the same; the divergence itself is out of scope
for this data-type matrix and is called out in the task report for
separate follow-up.
All 24 assertions (12 types x text+binary) pass against the sdd-pg1
debug ProxySQL build, confirming transparent OID/value/format-code
handling on the libpq backend path for this type set.
…t + payload (Task 4 review) The binary-format assertions were tautological: run_case parsed the RowDescription only up to the type OID (never the trailing per-column format-code field) and captured the DataRow value only for text format. Each binary assertion therefore checked just the OID (identical in text and binary) and 'a row exists' -- it would have passed unchanged even if ProxySQL had silently downgraded the requested binary result format to text, which is precisely the #5866 transparency class this is meant to catch. Rework run_case to read the full RowDescription field layout (the same layout readResult() uses, including the per-column format code it stores into columnFormat()) while additionally keeping the type OID -- which readResult()/PgResult discard, and which both assertions here need. The DataRow payload for column 0 is now captured for both formats. Assert: text : columnFormat==0 AND oid==expected AND value(text)==expected binary: columnFormat==1 (ProxySQL honored binary) AND oid==expected AND non-null; plus an end-to-end decode of the int4 case (payload is exactly 4 bytes, big-endian == 2147483647) to prove the bytes are real binary, not text mislabeled binary. Fix the assertion message to describe what is actually checked. Also drop the stray blank line after the new Makefile rule. 24/24 pass with real binary checks: payload sizes match the fixed-width binary encodings (int4=4, int8=8, uuid=16, timestamptz=8, bool=1 bytes) and differ from the text forms, and every column reports columnFormat==1.
…d reuse (Task 6 review)
…RAM Makefile flags (final review)
…ved-ports claim, final command block)
Fill in infra-dbdeployer-pgsql17-repl/bin/docker-proxy-post.bash (was a
no-op placeholder) and add its conf/proxysql/infra-config.sql: all three
PG backends load into the writer hostgroup addressed through Toxiproxy
(toxiproxy.${INFRA_ID}:6001/6002/6003), pgsql_replication_hostgroups
drives automatic rw-split via check_type='read_only', and the monitor
(monitor/monitor role, 1000ms read_only_interval) demotes replicas by
polling pg_is_in_recovery() through the proxy.
Uses ${INFRA_ID} rather than ${INFRA} for hostnames/comment-tagging:
${INFRA} is only reliably exported when this script runs via
docker-compose-init.bash, not via ensure-infras.bash's already-running
reconfigure path, which would otherwise template unresolvable
"toxiproxy." hostnames. docker-compose.yml already provisions the
toxiproxy.${INFRA_ID}/dbdeployer1.${INFRA_ID} aliases for this reason.
Verified end-to-end on infra sdd-sp2: applied via
test/infra/control/ensure-infras.bash, monitor demotes both replicas
within a few seconds (runtime_pgsql_servers: 1 writer + 2 readers,
pgsql_server_read_only_log clean), and frontend routing on port 6133
is correct (SELECT pg_is_in_recovery() -> reader, writes -> writer).
…TOP) Add -v ON_ERROR_STOP=1 to the psql invocation that applies infra-config.sql to the ProxySQL admin (PG protocol, port 6132). Without it psql prints SQL-level errors (bad token, constraint violation, duplicate rule_id) but continues and exits 0, so set -e never fires and the script's fail-non-zero contract was silently defeated -- connection failures were caught, SQL failures were not. Matches the precedent in this infra's docker/entrypoint.sh and infra-pgsql17-repl's init-replication.sh. Verified on sdd-sp2: a bogus statement piped with ON_ERROR_STOP=1 now aborts with exit 3 at the first error (without the flag the same input kept executing and exited 0), and a full ensure-infras.bash re-apply still exits 0 leaving runtime_pgsql_servers in the expected 1-writer/2-reader state.
…-polyglot-foundation # Conflicts: # .gitignore
…patibility, #5910 review) Gemini review claimed the javabuild stage (eclipse-temurin:21-jdk) compiles Behaviors.java at the default JDK 21 class-file version, while the final image's `default-jre-headless` on a Debian bookworm base is Java 17 -- a mismatch that would raise UnsupportedClassVersionError at runtime. Empirically verified before trusting the claim, since our in-image CI runs were already passing 4/4: - `docker run --rm --entrypoint java proxysql-pg-compat:latest -version` reports OpenJDK 21.0.11 (Debian build), not 17. - `docker run ... --entrypoint cat /etc/os-release` shows the final stage's `python:3.11-slim` base now resolves to Debian 13 (trixie), not bookworm (Debian 12) as the review assumed. Trixie's `default-jre-headless` is OpenJDK 21, which matches the JDK-21-compiled classes exactly -- so the 4/4 pass was real, not a fluke. - `behaviors-java connect` against the live sdd-sp2 backend and the full `tests/test_behaviors_java.py` suite both passed (4 passed) prior to this change too. Verdict: false positive for the CURRENT resolved base image -- the JDK version match is real, just incidental to `python:3.11-slim` having moved on to trixie. The underlying risk the review is pointing at is still legitimate: a floating base tag means the JRE version is not pinned, and a future re-resolution to an older Debian (or a deliberate downgrade) would reintroduce the exact mismatch described. Compiling with `javac --release 17` costs nothing and removes the dependency on which Debian release the base tag happens to resolve to. Rebuilt the image and re-ran tests/test_behaviors_java.py -v: 4 passed.
…-polyglot-foundation
…sertions Addresses CodeRabbit findings on #5894. pg_lite_client.cpp: the 4-byte authentication sub-type was re-read after readMessage() refilled the buffer without re-validating its length, so a truncated AuthenticationOk read past the end of the vector; the read also used an unaligned reinterpret_cast. Route every read through a single bounds-checked readAuthType(). The ErrorResponse path in the same function hand-walked a raw char* with no bounds check at all -- replaced with the already-bounds-checked extractErrorMessage(). pgsql-datatype_matrix-t: the row labelled "timestamptz" applied AT TIME ZONE 'UTC', which yields timestamp WITHOUT time zone (OID 1114), so timestamptz (1184) was never covered. Use a real timestamptz and pin the session to UTC so its text rendering stays deterministic. The binary half asserted only the OID, so a silent text fallback or a corrupted payload still passed; every case now carries the exact bytes PostgreSQL's *_send() emits and the payload is compared byte-for-byte. pgsql-auth_method_matrix-t: only the first pgsql-authentication_method switch checked its result. A failed SET/LOAD left the previous floor in effect, so later assertions silently tested the wrong method -- a "scram floor" check could actually exercise md5, and a wrong-password rejection could pass for the wrong reason. Every switch is now a checked precondition. pgsql-server_side_cursors-t: BEGIN/DECLARE/MOVE/CLOSE/COMMIT were fire-and-forget PQexec() calls that leaked their PGresult and hid failures; a failed DECLARE surfaced only as "FETCH 3 returns 3 rows" failing. Check and clear each result, and abort when a precondition fails.
…cess timeout, pytest CVE Addresses CodeRabbit findings on #5903 and #5910. diff.py: only-targets/skip-targets are documented as glob patterns, but were matched with exact set membership, so the documented "only-targets: proxy_native_*" matched no target and silently skipped the entire case -- a vacuous pass. Match with fnmatchcase; a name with no metacharacter still compares exactly. Pinned by a new infra-free unit test. transactions.py: if a statement raised between begin() and commit(), the finally block ran DROP TABLE on a session left in PostgreSQL's aborted- transaction state, so the drop failed, the table leaked, and the cleanup error masked the original failure. Roll back (best-effort) first, and close the connection even if the drop fails. _subproc.py: an uncaught subprocess.TimeoutExpired turned a hung driver into a pytest ERROR during teardown and discarded the partial output that identifies where it hung. Report it as a failure with the captured streams, decoding defensively since TimeoutExpired yields bytes even under text=True. The timeout is now overridable via PGCOMPAT_BEHAVIOR_TIMEOUT. requirements.txt: GHSA-6w46-j5rx-g56g (insecure tmpdir handling) affects every pytest before 9.0.3, so the "8.*" pin could not pick up the fix. The suite uses only stable APIs and the 3.11 base image satisfies pytest 9's Python >=3.10 requirement.
…sted creds Addresses CodeRabbit and Gemini findings on #5903 and #5910. ci-pg-compat.yml: the job builds and tests but never pushes, yet checkout persisted its write-all token into .git/config where every later step -- including the third-party driver images this suite builds and runs -- could read it. Set persist-credentials: false. docker-proxy-post.bash: the ProxySQL admin wait loop had no timeout. docker-compose-init.bash re-execs itself under `timeout`, but ensure-infras.bash's reconfigure path calls this script directly, where the loop would hang the run instead of failing it; it now gives up after PROXY_WAIT_SECONDS (default 120) and dumps container logs. The SQL template was expanded with eval-echo, running the whole file through the shell, and was read via a cwd-relative path; use envsubst with an explicit variable list and resolve the template relative to SCRIPT_DIR. Verified to produce byte-identical output to the previous eval for the current template. Dockerfile: verify the downloaded pgjdbc jar against a pinned SHA-256, so a substituted artifact fails the build. The recorded digest is that of the jar whose SHA-1 matches Maven Central's published digest (Central does not publish .sha256 for this artifact). go/java/node behaviors: the scaffold comments claimed only `connect` was implemented and the other three exited 2 as "not implemented", contradicting code where all four behaviors are complete. The Go errNotImplemented sentinel and the Node NotImplementedError class were never raised, so their dispatch branches were dead; removed along with the now-unused errors import.
Addresses CodeRabbit findings filed against the plan documents on #5894 and #5903. Each of these described behaviour the shipped code either implements differently or had to correct. SP-1 plan: the sketched timestamptz row used AT TIME ZONE 'UTC' (OID 1114, i.e. timestamp WITHOUT time zone) and the binary assertion checked only the OID. Both are corrected to match the implemented test, which uses a real timestamptz and compares the DataRow payload byte-for-byte. SP-2 plan: the env contract named a single PGCOMPAT_BACKEND_PORT, which cannot address the dbdeployer layout where all three nodes share one host and differ only by port. The implementation deliberately publishes a _HOST/_PORT pair per node (harness/targets.py records this explicitly); the plan and its sample snippets now say the same. The diff.py sketch also gains the fnmatch-based target filtering that the documented "only-targets: proxy_native_*" example requires.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
📝 WalkthroughWalkthroughAdds PostgreSQL protocol TAP coverage, PostgreSQL 17 replication infrastructure, a differential compatibility harness, behavior tests for five drivers, and scheduled or label-gated non-blocking CI with JUnit artifacts. ChangesPostgreSQL compatibility testing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 310e9c2c91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Build ProxySQL (debug, PROXYSQL31) | ||
| run: PROXYSQL31=1 make -j$(nproc) debug |
There was a problem hiding this comment.
On a fresh ubuntu-22.04 runner, this step produces only the ProxySQL binary, but the following ensure-infras.bash call reaches start-proxysql-isolated.bash, which runs both filesystem helpers and ProxySQL itself from the local-only tag proxysql-ci-base:latest. The repository's infra README says this image must be built locally, and the inspected ci-legacy-g4.yml workflow explicitly builds it before calling the same helper; without an equivalent build/tag step here, infrastructure startup fails before any pg-compat tests run.
Useful? React with 👍 / 👎.
| if only and not _matches_any(t.name, only): | ||
| continue |
There was a problem hiding this comment.
When a case uses the documented example -- only-targets: proxy_native_*, this condition excludes both direct_text and direct_binary. compare() then requires those format-matched baselines and reports them as unavailable, so the case fails regardless of whether the selected proxy targets are transparent. Include each selected proxy target's direct baseline implicitly, or otherwise prevent filtering out a required baseline.
Useful? React with 👍 / 👎.
| if (type != AUTH_TYPE || buffer.size() < 4 || | ||
| ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 11) { |
There was a problem hiding this comment.
On platforms or sanitizer builds that enforce C++ alignment and object-lifetime rules, dereferencing an int32_t* cast from vector<uint8_t>::data() is undefined behavior. The new readAuthType() helper was added specifically to avoid this, but the SCRAM path still performs the unsafe cast here and again for SASLFinal and AuthenticationOk; route all three reads through the helper so SCRAM authentication does not depend on byte-buffer alignment or aliasing behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
test/infra/infra-dbdeployer-pgsql17-repl/docker-compose-init.bash-132-144 (1)
132-144: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIncrease the dbdeployer readiness deadline.
Line 132 allows only 120 seconds.
docker/entrypoint.shcan use 270 seconds in its bounded readiness loops before it writes/tmp/dbdeployer_ready. The initializer can fail during a valid slow startup.Proposed fix
-MAX_WAIT=120 +MAX_WAIT="${DBDEPLOYER_READY_TIMEOUT:-480}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/infra/infra-dbdeployer-pgsql17-repl/docker-compose-init.bash` around lines 132 - 144, Increase MAX_WAIT in the readiness loop around the dbdeployer container check to 270 seconds, matching the bounded readiness period used by docker/entrypoint.sh. Preserve the existing timeout logging and polling behavior..github/workflows/CI-pg-compat.yml-43-45 (1)
43-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the caller-level
write-alland inherited secrets.The reusable job only needs
contents: readfor checkout and theupload-artifactaction has no repository write path from this workflow scope. If the reusable job also keepspermissions: write-all, reduce that too; otherwise future reusable-workflow changes still receive all caller secrets at call sites.Proposed change
- permissions: write-all + permissions: + contents: read uses: sysown/proxysql/.github/workflows/ci-pg-compat.yml@GH-Actions - secrets: inherit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/CI-pg-compat.yml around lines 43 - 45, Update the reusable workflow invocation in CI-pg-compat.yml to remove caller-level permissions: write-all and secrets: inherit, retaining only the minimum contents: read permission required for checkout. Also reduce any permissions: write-all declaration in the referenced reusable workflow if present.test/pg-compat/tests/test_differential_selfcheck.py-48-71 (1)
48-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not delete a rule that this test did not create.
Line 56 inserts a fixed global
rule_id. Lines 68-70 delete that ID even when the insert fails.If ProxySQL already has rule 90, this test deletes the existing routing rule during cleanup. Check that rule 90 is unused before insertion. Delete it only after a successful insertion. Alternatively, snapshot and restore the existing rule.
Proposed fix
def test_engine_detects_divergence(admin): + inserted = False try: + assert not admin.query( + f"SELECT 1 FROM pgsql_query_rules WHERE rule_id={SELFCHECK_RULE_ID}" + ), f"rule_id {SELFCHECK_RULE_ID} is already in use" admin.query( "INSERT INTO pgsql_query_rules " "(rule_id,active,match_pattern,replace_pattern,re_modifiers,apply) " f"VALUES ({SELFCHECK_RULE_ID},1,'SELECT 1 AS canary'," "'SELECT 2 AS canary','CASELESS',1)" ) + inserted = True admin.query("LOAD PGSQL QUERY RULES TO RUNTIME") ... finally: - admin.query( - f"DELETE FROM pgsql_query_rules WHERE rule_id={SELFCHECK_RULE_ID}" - ) - admin.query("LOAD PGSQL QUERY RULES TO RUNTIME") + if inserted: + admin.query( + f"DELETE FROM pgsql_query_rules WHERE rule_id={SELFCHECK_RULE_ID}" + ) + admin.query("LOAD PGSQL QUERY RULES TO RUNTIME")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pg-compat/tests/test_differential_selfcheck.py` around lines 48 - 71, The test_engine_detects_divergence cleanup unconditionally deletes the fixed SELFCHECK_RULE_ID, risking removal of a pre-existing rule when insertion fails. Update the test to verify the rule_id is unused before inserting and track successful insertion, then run the DELETE and runtime reload in finally only when this test created the rule; otherwise preserve the existing rule unchanged.test/pg-compat/drivers/go/go.mod-5-12 (1)
5-12: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBump
pgxtov5.9.2before applying thecryptobump.
github.com/jackc/pgx/v5v5.7.5 is still affected by the SQL-injection advisoryGHSA-j88v-2chj-qfwx/ CVE-2026-41889, andv5.7.6is not the patched release. The patch is inv5.9.2, so propose that version here instead. Keepgolang.org/x/cryptoatv0.45.0for the SSH-agent/SSH fixes, and rungo mod tidyafter the bump.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pg-compat/drivers/go/go.mod` around lines 5 - 12, Update the github.com/jackc/pgx/v5 dependency in the Go module from v5.7.5 to v5.9.2, while keeping golang.org/x/crypto at v0.45.0. Run go mod tidy afterward to refresh the module requirements and checksums.Source: Linters/SAST tools
docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md-23-25 (1)
23-25: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the required Docker build network setting in the shared runner.
Line [23] requires
--network=host, but the supplied SP-2run-pg-compat.bashusesdocker buildwithout that option. The multi-stage build downloads Go, pgjdbc, and npm dependencies, so local or CI builds can fail or behave differently. Add the option to the shared build command and verify the CI path, or remove this requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md` around lines 23 - 25, Update the shared run-pg-compat.bash Docker build command to include --network=host, preserving the existing multi-stage build arguments and CI behavior. Verify the runner’s CI build path uses this updated command rather than removing the documented network requirement.docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md-128-140 (1)
128-140: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail CI when an expected behavior program is absent.
run_behavior()skips missing binaries. A broken multi-stage copy or toolchain build can therefore produce green tests without running a driver. Use skips only for an explicit local partial-image mode. In CI, assert that all expected binaries exist and fail with an infrastructure error when one is missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md` around lines 128 - 140, Update run_behavior to fail with an infrastructure error when the expected program is missing instead of unconditionally calling pytest.skip. Allow the skip only when an explicit local partial-image mode is enabled, and preserve the existing subprocess behavior and failure handling for present binaries.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-839-847 (1)
839-847: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a state variable that ProxySQL restores and force backend reuse.
application_nameis ignored by ProxySQL in the cross-driver contract, and connection A remains open while B is created. The test can pass without checking backend-session cleanup. Use theTimeZone='Antarctica/Troll'probe, close A, then create B and assert a different value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 839 - 847, Update the run function’s session-isolation probe to set and verify TimeZone='Antarctica/Troll' instead of application_name, then close connection A before creating connection B to force backend reuse. Assert B reports a different TimeZone value, preserving the existing cleanup for both adapters.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-456-461 (1)
456-461: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore admin state in a
finallyblock.If the SET, LOAD, or assertion fails,
admin.restore(saved)is skipped. The test can leavepgsql-authentication_method=1active for later tests and create order-dependent failures. Wrap the mutation and assertion intry/finally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 456 - 461, Update test_admin_reconfig_roundtrip so the mutation, reload, query, and assertion execute inside a try block, with admin.restore(saved) in a finally block that always runs after the snapshot.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-819-826 (1)
819-826: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPin verification reads to the writer.
The cross-driver contract requires verification reads to run inside explicit transactions. These bare
SELECT count(*)statements can route to a replica before replication catches up and cause false failures. Wrap each verification read inBEGINandCOMMIT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 819 - 826, Update the transaction verification flow around the rollback and commit assertions so each SELECT count(*) read is executed inside an explicit transaction: begin before the read and commit afterward, while preserving the existing assertions and insert transaction behavior.docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md-861-871 (1)
861-871: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winForce backend reuse in the session-isolation test.
Connection A remains open while connection B is created, so ProxySQL can assign B a different backend. The test does not prove state restoration after backend reuse. Use the contract’s
TimeZone='Antarctica/Troll'probe, close A before opening B, and then assert that B does not observe the value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md` around lines 861 - 871, Update the session-isolation test around PGConnPtr a and PGConnPtr b to use the contract’s TimeZone='Antarctica/Troll' probe, close connection A before creating connection B, and assert that B’s SHOW TimeZone result does not retain that value. Preserve the existing scalar validation and make the sequencing force backend reuse.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-592-601 (1)
592-601: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle cases without a direct baseline.
only-targetsandskip-targetscan removedirect_textordirect_binary, butcompare()still compares proxy results withNone. A case such asonly-targets: proxy_native_*will be reported as divergent for structural reasons. Require a matching direct baseline or skip comparisons that have no baseline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 592 - 601, Update compare() so proxy results are only compared when their matching direct_text or direct_binary baseline exists; otherwise skip that proxy entry or require the baseline according to the intended validation behavior. Preserve divergence reporting when a baseline is present, avoiding comparisons against None caused by only-targets or skip-targets.docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md-852-859 (1)
852-859: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the pool-churn case exceed the configured limit.
The loop opens one connection, runs one query, and closes the connection before the next iteration. It never exceeds
pgsql-max_connections, never tests queuing or rejection, and cannot detect concurrent pool exhaustion. Set a small limit and hold concurrent connections open while asserting the documented pool behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md` around lines 852 - 859, Update the connection-storm case around mk() to configure a deliberately small pgsql-max_connections limit and hold multiple connections concurrently so the test exceeds that limit. Assert the documented queuing or rejection behavior while connections are retained, then release them and preserve validation that successful connections can execute SELECT 1 without pool leaks.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-231-239 (1)
231-239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not hard-code PostgreSQL port 5432 in the Toxiproxy bootstrap.
The plan supports dbdeployer ports
16710,16711, and16712at Lines [171]-[173], butmk()always uses$3:5432. The proxies will target the wrong upstreams in that topology. Pass the upstream port through the selected infrastructure contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 231 - 239, Update the Toxiproxy bootstrap’s mk function and its pg_primary, pg_replica1, and pg_replica2 calls to accept and use the upstream PostgreSQL port instead of hard-coding 5432. Reuse the selected infrastructure contract’s dbdeployer ports 16710, 16711, and 16712 when constructing each proxy upstream, and preserve the existing proxy names and listen ports.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-145-157 (1)
145-157: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftInitialize the monitor role on every PostgreSQL node.
The post-provision command targets only
${CONTAINER}and does not select a node or port. In the three-node fallback, it can createmonitorand the extensions on only one server, while ProxySQL monitors all three servers. Monitoring and automatic hostgroup movement will then fail on uninitialized nodes. Iterate over every node and verify the role and extension on each server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 145 - 157, Update the Step 4 post-provision instructions and script flow to initialize the monitor role and pg_stat_statements extension on every PostgreSQL node, not only the default ${CONTAINER} target. Iterate over the three-node fallback servers using each node’s connection target or port, then verify monitor authentication and SELECT * FROM pg_stat_statements LIMIT 1 succeeds on each node while preserving the existing postgres/testuser database coverage.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-874-882 (1)
874-882: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftApply the xfail mode to the tested target.
The catalogue stores
mode, but the collection hook applies one xfail marker to the whole differential test node. A native-only divergence can therefore xfail the libpq comparison too and hide regressions. Make the target or mode part of the test node ID, or apply xfail at the per-target comparison level.Also applies to: 904-911
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 874 - 882, Update the differential test collection and xfail application around the catalogue entries and per-test handling so each xfail marker is scoped to its declared mode, especially native-only entries. Make the collected node identity include the target/mode or apply the marker within the per-target comparison, ensuring libpq comparisons remain independently evaluated; preserve strict=false xpass reporting.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-57-59 (1)
57-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the repository path for the reusable workflow.
The plan lists
gh-actions-reusable/ci-pg-compat.yml, but the supplied workflow contract uses.github/workflows/gh-actions-reusable/ci-pg-compat.yml. If implementation follows the shorter path, GitHub will not load the reusable workflow from the expected location. Use the exact repository path consistently in the file structure, Task 10, and commit commands.Also applies to: 935-938
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 57 - 59, The plan references the reusable workflow with an incomplete path. Update every occurrence associated with Task 10, the file structure, and commit commands to use .github/workflows/gh-actions-reusable/ci-pg-compat.yml consistently, while preserving the caller workflow path.docs/superpowers/specs/2026-07-08-pgsql-protocol-testing-design.md-142-145 (1)
142-145: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAlign the documented differential target count.
Line [142] says
targets.pycreates four differential targets. Lines [174]-[185] define six targets: two backend modes, two result formats, and two direct baselines. This mismatch can cause the implementation to omit required comparisons. Change the directory layout and interface text to six targets, or define a different target contract consistently.Also applies to: 174-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-07-08-pgsql-protocol-testing-design.md` around lines 142 - 145, Align the differential target contract across the document: update the targets.py description and related interface text to consistently specify six targets, covering the two backend modes, two result formats, and two direct baselines defined in the target list at lines 174–185.docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md-84-89 (1)
84-89: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the
npm installfallback from the Node build.The Dockerfile labels this stage as installing against the lockfile, but
RUN npm ci --omit=dev || npm install --omit=devfalls back to a non-lockfile install whennpm cifails. Keepnpm ci --omit=devas the only install here and requiredrivers/node/package-lock.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md` around lines 84 - 89, Update the nodebuild stage’s dependency installation to run only npm ci --omit=dev, removing the npm install fallback, and require drivers/node/package-lock.json in the copied dependency inputs so lockfile-based installation is enforced.docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md-983-990 (1)
983-990: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd native backend coverage for Task 7 or narrow the task scope.
§3.5requirespgsql-listen_notify_contract-tto parameterizepgsql-use_native_backend_protocoland run xfail-tolerant native assertions because SP-1 must cover both backend modes. The current Task 7 only tests the libpq path and explicitly defers native assertions topgsql-native_notify-t, so the plan still lacks the promised native LISTEN/NOTIFY contract coverage. Add the native mode case here, or update the SP-1 scope if this task should remain libpq-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md` around lines 983 - 990, Update Task 7 and its `pgsql-listen_notify_contract-t` execution plan to include parameterized native-backend coverage with xfail-tolerant assertions, matching §3.5 and covering both backend modes; alternatively, explicitly narrow the SP-1 scope and remove the claim that this task provides complete LISTEN/NOTIFY contract coverage.docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md-560-564 (1)
560-564: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSet the session time zone before comparing
timestamptztext.The snippet says the session pins
TimeZone=UTC, butrun_casedoes not set it.timestamptztext output uses PostgreSQL’s session time zone, so this expected value is not portable. RunSET TIME ZONE 'UTC'on each connection before the test query, or compare a normalized value instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md` around lines 560 - 564, Update the run_case setup used by the timestamptz test to execute SET TIME ZONE 'UTC' on each connection before the test query, ensuring the expected text value is deterministic while preserving the existing OID and value assertions.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-705-709 (1)
705-709: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGrant
pg_stat_statements_reset()execution to the oracle role.
reset_all()connects astestuser, but PostgreSQL 17 restrictsSELECT pg_stat_statements_reset()by default to superusers. Add a targeted grant in the provisioned backends, for example
GRANT EXECUTE ON FUNCTION pg_stat_statements_reset(oid, oid, bigint, boolean) TO testuser;
so Task 7 does not fail on the first oracle reset.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 705 - 709, Update the backend provisioning setup used by reset_all to grant testuser EXECUTE on pg_stat_statements_reset(oid, oid, bigint, boolean). Keep the grant targeted to that function and ensure it is applied to every provisioned backend before reset_all connects as testuser.docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md-954-962 (1)
954-962: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReduce workflow token permissions.
permissions: write-allgrants all write scopes, while this job only needs checkout and report upload. Use explicit minimal permissions, addingcontents: readandactions: writeas required by caller/callee intersections foractions/checkoutandactions/upload-artifact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md` around lines 954 - 962, Replace the broad permissions setting on the pg-compat job with explicit minimal permissions: set contents to read and actions to write, while leaving the job conditions, reusable workflow reference, and inherited secrets unchanged.test/tap/tests/pgsql-pool_churn-t.cpp-145-150 (1)
145-150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe isolation assertion passes when connection B fails.
scalar()returns an empty string if the connection is down or the query fails. Line 149 only checksb_val != "Antarctica/Troll", so a failed connection B satisfies the assertion. The test then reports success while proving nothing about the backend session reset.A second point: line 145 closes A and line 147 opens B immediately. ProxySQL returns the backend to the free pool asynchronously. Wait for
ConnFreeto reach 1 before opening B, the same way the code drains to 0 at lines 132-136.🛠️ Proposed fix
a.reset(); // close A -> its single backend returns to the pool, session-dirtied + // The backend returns to the free pool asynchronously. Wait for it so B + // deterministically reuses it instead of racing the return. + for (int i = 0; i < 100; ++i) { // up to ~10s + if (admin_scalar(admin, "SELECT IFNULL(SUM(ConnFree),0) FROM stats_pgsql_connection_pool") == "1") break; + usleep(100 * 1000); + } + PGConnPtr b = mk(); // cap=1 + no spares => B must reuse A's dirtied backend std::string b_val = scalar(b.get(), "SHOW TimeZone"); - ok(b_val != "Antarctica/Troll", + bool b_connected = (b && PQstatus(b.get()) == CONNECTION_OK); + ok(b_connected && !b_val.empty() && b_val != "Antarctica/Troll", "connection B (forced to reuse A's backend) does NOT inherit A's session state (got '%s')", b_val.c_str()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pgsql-pool_churn-t.cpp` around lines 145 - 150, Update the test flow around the A-to-B handoff: after resetting connection A, wait until ConnFree reaches 1 using the existing polling pattern from lines 132-136 before creating B. Strengthen the assertion after scalar(b.get(), "SHOW TimeZone") to first require that B is connected and the query succeeded, then verify b_val is not "Antarctica/Troll", so query failure cannot pass the isolation check.
🟡 Minor comments (5)
test/infra/infra-dbdeployer-pgsql17-repl/docker-compose-destroy.bash-4-4 (1)
4-4: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winQuote the script directory expansion.
Line 4 splits or expands the path when the script directory contains spaces or glob characters. Quote the command substitution before calling
pushd.Proposed fix
-pushd $(dirname $0) &>/dev/null +pushd "$(dirname "$0")" &>/dev/null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/infra/infra-dbdeployer-pgsql17-repl/docker-compose-destroy.bash` at line 4, Quote the command substitution in the pushd invocation so the directory path returned by dirname "$0" remains a single argument, including when it contains spaces or glob characters.Source: Linters/SAST tools
test/infra/infra-dbdeployer-pgsql17-repl/conf/proxysql/infra-config.sql-12-26 (1)
12-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the template expansion documentation.
This template is expanded by allowlisted
envsubstintest/infra/infra-dbdeployer-pgsql17-repl/bin/docker-proxy-post.bashlines 43-50. It is not eval-expanded. Remove the staleevalwarning and document the five variables thatenvsubstsubstitutes.Proposed fix
--- NOTE: this template is eval-expanded by ./bin/docker-proxy-post.bash, which +-- NOTE: ./bin/docker-proxy-post.bash expands only INFRA_ID, INFRA, WHG, RHG, +-- and ROOT_PASSWORD with envsubst. ... --- CAUTION for future edits: this whole file is run through a shell eval to --- expand the template placeholders above, so any double quote character or --- any dollar-sign token in a comment (not just the intended placeholders) --- gets interpreted by that eval too. Keep comments free of both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/infra/infra-dbdeployer-pgsql17-repl/conf/proxysql/infra-config.sql` around lines 12 - 26, Update the opening documentation in the template to state that it is expanded by the allowlisted envsubst invocation in docker-proxy-post.bash, not shell eval. Remove the stale eval-specific cautions and document the five variables substituted by envsubst, preserving the existing INFRA_ID and invocation-path context where applicable.test/pg-compat/drivers/prisma/behaviors.mjs-290-300 (1)
290-300: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
process.exitcan truncate the stderr failure reason in both Node programs. Node writes to a pipe asynchronously. Both programs callprocess.exitimmediately after writing the error text to stderr, so the process can terminate before the write is flushed.test/pg-compat/tests/_subproc.pysurfaces that stderr text as the pytest failure message, so a truncated write hides the divergence the harness is meant to catalogue.
test/pg-compat/drivers/prisma/behaviors.mjs#L290-L300: replaceprocess.exit(1)andprocess.exit(0)inmain()withprocess.exitCode = 1(thenreturn) andprocess.exitCode = 0.test/pg-compat/drivers/node/behaviors.js#L289-L296: replaceprocess.exit(await dispatch(args[0]))withprocess.exitCode = await dispatch(args[0]), and keep the usage path'sprocess.exit(2)behind a flushed write or convert it toprocess.exitCodeplusreturn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pg-compat/drivers/prisma/behaviors.mjs` around lines 290 - 300, Update main() in test/pg-compat/drivers/prisma/behaviors.mjs at lines 290-300 to set process.exitCode instead of immediately calling process.exit after writing errors, returning from the failure branch and setting success to 0. Update test/pg-compat/drivers/node/behaviors.js at lines 289-296 to assign dispatch’s result to process.exitCode; also avoid immediate process.exit(2) after the usage write by using a flushed write or setting the exit code and returning.test/tap/tests/pgsql-listen_notify_contract-t.cpp-1-9 (1)
1-9: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInclude
<memory>forstd::unique_ptr.Line 9 uses
std::unique_ptr, but no include in this file declares it.pgsql-pool_churn-t.cppincludes<memory>for the samePGConnPtralias.pgsql-server_side_cursors-t.cppgets the declaration transitively throughpg_lite_client.h, which this file does not include. The build depends on a transitive include fromcommand_line.h,tap.h, orutils.h, which is not guaranteed across toolchains.🔧 Proposed fix
`#include` <string> `#include` <sstream> +#include <memory> `#include` "libpq-fe.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pgsql-listen_notify_contract-t.cpp` around lines 1 - 9, Update the includes in the test file to explicitly include the standard memory header before the PGConnPtr alias, ensuring std::unique_ptr is declared without relying on transitive includes.test/tap/tests/pgsql-auth_method_matrix-t.cpp-105-111 (1)
105-111: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSnapshot
pgsql-authentication_methodinstead of hardcoding the restore value.The restore writes the literal
3. The comment states that a wrong floor corrupts whichever test runs next, so the restore must return the exact value the infra seeded, not an assumed default.pgsql-pool_churn-talready applies this pattern formax_connectionsandpgsql-free_connections_pct.🛠️ Proposed snapshot and restore
MYSQL* admin = admin_connect(); if (!admin) BAIL_OUT("cannot reach admin"); + + // Snapshot the seeded floor so the restore below is exact. + int orig_method = 3; // documented default, used only if the read fails + if (mysql_query(admin, "SELECT variable_value FROM global_variables " + "WHERE variable_name='pgsql-authentication_method'") == 0) { + MYSQL_RES* res = mysql_store_result(admin); + if (res) { + MYSQL_ROW row = mysql_fetch_row(res); + if (row && row[0]) orig_method = atoi(row[0]); + mysql_free_result(res); + } + }- if (!set_frontend_auth_method(admin, 3)) { - diag("WARNING: failed to restore pgsql-authentication_method=3; " + if (!set_frontend_auth_method(admin, orig_method)) { + diag("WARNING: failed to restore pgsql-authentication_method=%d; " "the instance is left on a non-default auth floor"); }Add the
orig_methodargument to thediag()call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pgsql-auth_method_matrix-t.cpp` around lines 105 - 111, Snapshot the initial pgsql-authentication_method value before modifying it, store it in an orig_method variable, and restore that captured value instead of the hardcoded 3 in the cleanup path around set_frontend_auth_method. Update the failure diag call to include orig_method, following the snapshot-and-restore pattern used by pgsql-pool_churn-t.
🧹 Nitpick comments (6)
test/pg-compat/tests/_subproc.py (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive a clear error for a malformed timeout override.
If
PGCOMPAT_BEHAVIOR_TIMEOUTis set to a non-numeric value,int()raisesValueErrorinside every behavior test. The message does not name the variable, so the cause is hard to identify from the report.♻️ Proposed refactor
- timeout = int(os.environ.get("PGCOMPAT_BEHAVIOR_TIMEOUT", "120")) + raw_timeout = os.environ.get("PGCOMPAT_BEHAVIOR_TIMEOUT", "120") + try: + timeout = int(raw_timeout) + except ValueError: + pytest.fail(f"PGCOMPAT_BEHAVIOR_TIMEOUT is not an integer: {raw_timeout!r}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pg-compat/tests/_subproc.py` at line 12, The timeout initialization in _subproc.py should catch invalid PGCOMPAT_BEHAVIOR_TIMEOUT values and raise a clear error that names the environment variable and expected numeric format, while preserving the default and valid-value behavior..github/workflows/gh-actions-reusable/ci-pg-compat.yml (1)
43-43: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope the job permissions instead of
write-all.This job builds ProxySQL, runs third-party driver images, and uploads an artifact. It never pushes to the repository.
write-allgrants theGITHUB_TOKENwrite scope for packages, deployments, issues, and more, so any step in this job (including the driver containers) sees an over-privileged token.
actions/upload-artifact@v4needs no repository write scope, socontents: readis sufficient here. If the caller must keepwrite-allfor intersection reasons, the callee can still narrow its own scope.🔒 Proposed least-privilege change
- permissions: write-all + permissions: + contents: read🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/gh-actions-reusable/ci-pg-compat.yml at line 43, Replace the job-level permissions setting near the CI compatibility job with least-privilege permissions granting only contents read access. Keep artifact uploading and the existing build and driver-image steps working without retaining write-all or adding broader repository scopes.Source: Linters/SAST tools
docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md (1)
75-82: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify the downloaded pgjdbc artifact.
The Dockerfile pins the pgjdbc version but accepts any bytes returned by the remote URL. Add a SHA-256 checksum or trusted repository verification before
javacconsumes the JAR. This makes the driver build reproducible and detects unexpected artifact changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md` around lines 75 - 82, The Java build stage currently downloads the pinned pgjdbc JAR without validating its contents. Update the Dockerfile flow around PGJDBC_VERSION and the /pgjdbc.jar download to verify the artifact with a trusted SHA-256 checksum before javac consumes it, keeping the build fail-fast when verification fails.test/tap/tests/pgsql-datatype_matrix-t.cpp (1)
194-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the server error before returning false.
When the server returns
ERROR_RESPONSE,run_case()returns false with no diagnostic. Theok()line then printsoid=0 payload=and gives no cause. Add adiag()with the raw payload so CI failures are diagnosable.🩺 Proposed diagnostic
} else if (type == PgConnection::ERROR_RESPONSE) { + diag("%s fmt=%d: server returned ErrorResponse: %.*s", + c.label, (int)fmt, (int)buf.size(), + reinterpret_cast<const char*>(buf.data())); conn.disconnect(); return false; }The ErrorResponse payload contains embedded null bytes between fields, so the output is truncated at the first field. That is still more information than the current empty diagnostic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pgsql-datatype_matrix-t.cpp` around lines 194 - 197, The ERROR_RESPONSE branch in run_case() should emit a diagnostic with the raw response payload via diag() before disconnecting and returning false. Preserve the existing disconnect and failure flow, and ensure the payload is passed so CI output includes the server error details.test/tap/tests/pg_lite_client.cpp (2)
327-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared post-password result handling.
The cleartext branch and the MD5 branch repeat the same three steps: read the next message, throw on
ERROR_RESPONSE, and return onAuthenticationOk. A small helper removes the duplication and also removes the deep nesting that SonarCloud reports at lines 331, 339, 344, and 346.♻️ Proposed helper extraction
+ // Reads the server reply after a password/SASL response and reports whether + // authentication completed. Throws on ErrorResponse. + auto awaitAuthResult = [&]() -> bool { + readMessage(type, buffer); + if (type == ERROR_RESPONSE) + throw PgException("Authentication error: " + extractErrorMessage(buffer)); + return type == AUTH_TYPE && readAuthType(buffer) == 0; + };Then each branch becomes:
else if (authType == 3) { // Cleartext password sendPassword(password); - // After sending password, we need to wait for auth result - readMessage(type, buffer); - if (type == ERROR_RESPONSE) - throw PgException("Authentication error: " + extractErrorMessage(buffer)); - if (type == AUTH_TYPE) { - authType = readAuthType(buffer); - if (authType == 0) return; - } + if (awaitAuthResult()) return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pg_lite_client.cpp` around lines 327 - 350, Extract the duplicated post-password authentication handling from the cleartext and MD5 branches into a small helper near the surrounding authentication logic. The helper should call readMessage, throw PgException for ERROR_RESPONSE using extractErrorMessage, and return success when AUTH_TYPE resolves to AuthenticationOk; invoke it after sendPassword and sendMD5Password while preserving invalid-MD5 validation and existing authentication flow.Source: Linters/SAST tools
460-461: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
readAuthType()for the three SASL authentication-type reads.The comment at lines 303-306 states that every read of the authentication sub-type must go through
readAuthType(). These three sites bypass it and usentohl(*reinterpret_cast<int32_t*>(buffer.data())). That cast reads anint32_tthrough auint8_t*object, which violates strict aliasing, and it repeats the length check thatreadAuthType()already performs.♻️ Proposed change for each of the three sites
- if (type != AUTH_TYPE || buffer.size() < 4 || - ntohl(*reinterpret_cast<int32_t*>(buffer.data())) != 11) { + if (type != AUTH_TYPE || buffer.size() < 4 || readAuthType(buffer) != 11) { free(client_first); free_scram_state(st); throw PgException("expected AuthenticationSASLContinue(11)"); }Apply the same substitution for the
12check at lines 490-491 and the0check at lines 509-510.Also applies to: 490-491, 509-510
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pg_lite_client.cpp` around lines 460 - 461, Replace the direct ntohl(reinterpret_cast...) authentication subtype reads in the three SASL checks with readAuthType(), passing the expected values 11, 12, and 0 respectively. Remove the duplicated buffer-length checks from these conditions and rely on readAuthType() for validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md`:
- Around line 542-572: Unify the datatype test contract by extending Case with
the expected binary representation and defining Observed with every asserted
field: observed_value, observed_oid, col_format, raw_bytes, text_value, and
is_null. Update run_case and its implementation to return Observed data and
populate all fields for both text and binary formats, then align the test cases
and assertions with this single contract.
In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md`:
- Around line 559-564: The _parse function currently discards SQL chunks
beginning with metadata comments, producing empty statements. Remove recognized
metadata lines, including transactional metadata, before splitting SQL; preserve
the remaining SQL statements and parse transactional according to the case
contract alongside skip-targets and only-targets.
---
Major comments:
In @.github/workflows/CI-pg-compat.yml:
- Around line 43-45: Update the reusable workflow invocation in CI-pg-compat.yml
to remove caller-level permissions: write-all and secrets: inherit, retaining
only the minimum contents: read permission required for checkout. Also reduce
any permissions: write-all declaration in the referenced reusable workflow if
present.
In `@docs/superpowers/plans/2026-07-08-pgsql-sp1-tap-coverage-gaps.md`:
- Around line 861-871: Update the session-isolation test around PGConnPtr a and
PGConnPtr b to use the contract’s TimeZone='Antarctica/Troll' probe, close
connection A before creating connection B, and assert that B’s SHOW TimeZone
result does not retain that value. Preserve the existing scalar validation and
make the sequencing force backend reuse.
- Around line 852-859: Update the connection-storm case around mk() to configure
a deliberately small pgsql-max_connections limit and hold multiple connections
concurrently so the test exceeds that limit. Assert the documented queuing or
rejection behavior while connections are retained, then release them and
preserve validation that successful connections can execute SELECT 1 without
pool leaks.
- Around line 983-990: Update Task 7 and its `pgsql-listen_notify_contract-t`
execution plan to include parameterized native-backend coverage with
xfail-tolerant assertions, matching §3.5 and covering both backend modes;
alternatively, explicitly narrow the SP-1 scope and remove the claim that this
task provides complete LISTEN/NOTIFY contract coverage.
- Around line 560-564: Update the run_case setup used by the timestamptz test to
execute SET TIME ZONE 'UTC' on each connection before the test query, ensuring
the expected text value is deterministic while preserving the existing OID and
value assertions.
In `@docs/superpowers/plans/2026-07-08-pgsql-sp2-polyglot-foundation.md`:
- Around line 839-847: Update the run function’s session-isolation probe to set
and verify TimeZone='Antarctica/Troll' instead of application_name, then close
connection A before creating connection B to force backend reuse. Assert B
reports a different TimeZone value, preserving the existing cleanup for both
adapters.
- Around line 456-461: Update test_admin_reconfig_roundtrip so the mutation,
reload, query, and assertion execute inside a try block, with
admin.restore(saved) in a finally block that always runs after the snapshot.
- Around line 819-826: Update the transaction verification flow around the
rollback and commit assertions so each SELECT count(*) read is executed inside
an explicit transaction: begin before the read and commit afterward, while
preserving the existing assertions and insert transaction behavior.
- Around line 592-601: Update compare() so proxy results are only compared when
their matching direct_text or direct_binary baseline exists; otherwise skip that
proxy entry or require the baseline according to the intended validation
behavior. Preserve divergence reporting when a baseline is present, avoiding
comparisons against None caused by only-targets or skip-targets.
- Around line 231-239: Update the Toxiproxy bootstrap’s mk function and its
pg_primary, pg_replica1, and pg_replica2 calls to accept and use the upstream
PostgreSQL port instead of hard-coding 5432. Reuse the selected infrastructure
contract’s dbdeployer ports 16710, 16711, and 16712 when constructing each proxy
upstream, and preserve the existing proxy names and listen ports.
- Around line 145-157: Update the Step 4 post-provision instructions and script
flow to initialize the monitor role and pg_stat_statements extension on every
PostgreSQL node, not only the default ${CONTAINER} target. Iterate over the
three-node fallback servers using each node’s connection target or port, then
verify monitor authentication and SELECT * FROM pg_stat_statements LIMIT 1
succeeds on each node while preserving the existing postgres/testuser database
coverage.
- Around line 874-882: Update the differential test collection and xfail
application around the catalogue entries and per-test handling so each xfail
marker is scoped to its declared mode, especially native-only entries. Make the
collected node identity include the target/mode or apply the marker within the
per-target comparison, ensuring libpq comparisons remain independently
evaluated; preserve strict=false xpass reporting.
- Around line 57-59: The plan references the reusable workflow with an
incomplete path. Update every occurrence associated with Task 10, the file
structure, and commit commands to use
.github/workflows/gh-actions-reusable/ci-pg-compat.yml consistently, while
preserving the caller workflow path.
- Around line 705-709: Update the backend provisioning setup used by reset_all
to grant testuser EXECUTE on pg_stat_statements_reset(oid, oid, bigint,
boolean). Keep the grant targeted to that function and ensure it is applied to
every provisioned backend before reset_all connects as testuser.
- Around line 954-962: Replace the broad permissions setting on the pg-compat
job with explicit minimal permissions: set contents to read and actions to
write, while leaving the job conditions, reusable workflow reference, and
inherited secrets unchanged.
In `@docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md`:
- Around line 23-25: Update the shared run-pg-compat.bash Docker build command
to include --network=host, preserving the existing multi-stage build arguments
and CI behavior. Verify the runner’s CI build path uses this updated command
rather than removing the documented network requirement.
- Around line 128-140: Update run_behavior to fail with an infrastructure error
when the expected program is missing instead of unconditionally calling
pytest.skip. Allow the skip only when an explicit local partial-image mode is
enabled, and preserve the existing subprocess behavior and failure handling for
present binaries.
- Around line 84-89: Update the nodebuild stage’s dependency installation to run
only npm ci --omit=dev, removing the npm install fallback, and require
drivers/node/package-lock.json in the copied dependency inputs so lockfile-based
installation is enforced.
In `@docs/superpowers/specs/2026-07-08-pgsql-protocol-testing-design.md`:
- Around line 142-145: Align the differential target contract across the
document: update the targets.py description and related interface text to
consistently specify six targets, covering the two backend modes, two result
formats, and two direct baselines defined in the target list at lines 174–185.
In `@test/infra/infra-dbdeployer-pgsql17-repl/docker-compose-init.bash`:
- Around line 132-144: Increase MAX_WAIT in the readiness loop around the
dbdeployer container check to 270 seconds, matching the bounded readiness period
used by docker/entrypoint.sh. Preserve the existing timeout logging and polling
behavior.
In `@test/pg-compat/drivers/go/go.mod`:
- Around line 5-12: Update the github.com/jackc/pgx/v5 dependency in the Go
module from v5.7.5 to v5.9.2, while keeping golang.org/x/crypto at v0.45.0. Run
go mod tidy afterward to refresh the module requirements and checksums.
In `@test/pg-compat/tests/test_differential_selfcheck.py`:
- Around line 48-71: The test_engine_detects_divergence cleanup unconditionally
deletes the fixed SELFCHECK_RULE_ID, risking removal of a pre-existing rule when
insertion fails. Update the test to verify the rule_id is unused before
inserting and track successful insertion, then run the DELETE and runtime reload
in finally only when this test created the rule; otherwise preserve the existing
rule unchanged.
In `@test/tap/tests/pgsql-pool_churn-t.cpp`:
- Around line 145-150: Update the test flow around the A-to-B handoff: after
resetting connection A, wait until ConnFree reaches 1 using the existing polling
pattern from lines 132-136 before creating B. Strengthen the assertion after
scalar(b.get(), "SHOW TimeZone") to first require that B is connected and the
query succeeded, then verify b_val is not "Antarctica/Troll", so query failure
cannot pass the isolation check.
---
Minor comments:
In `@test/infra/infra-dbdeployer-pgsql17-repl/conf/proxysql/infra-config.sql`:
- Around line 12-26: Update the opening documentation in the template to state
that it is expanded by the allowlisted envsubst invocation in
docker-proxy-post.bash, not shell eval. Remove the stale eval-specific cautions
and document the five variables substituted by envsubst, preserving the existing
INFRA_ID and invocation-path context where applicable.
In `@test/infra/infra-dbdeployer-pgsql17-repl/docker-compose-destroy.bash`:
- Line 4: Quote the command substitution in the pushd invocation so the
directory path returned by dirname "$0" remains a single argument, including
when it contains spaces or glob characters.
In `@test/pg-compat/drivers/prisma/behaviors.mjs`:
- Around line 290-300: Update main() in
test/pg-compat/drivers/prisma/behaviors.mjs at lines 290-300 to set
process.exitCode instead of immediately calling process.exit after writing
errors, returning from the failure branch and setting success to 0. Update
test/pg-compat/drivers/node/behaviors.js at lines 289-296 to assign dispatch’s
result to process.exitCode; also avoid immediate process.exit(2) after the usage
write by using a flushed write or setting the exit code and returning.
In `@test/tap/tests/pgsql-auth_method_matrix-t.cpp`:
- Around line 105-111: Snapshot the initial pgsql-authentication_method value
before modifying it, store it in an orig_method variable, and restore that
captured value instead of the hardcoded 3 in the cleanup path around
set_frontend_auth_method. Update the failure diag call to include orig_method,
following the snapshot-and-restore pattern used by pgsql-pool_churn-t.
In `@test/tap/tests/pgsql-listen_notify_contract-t.cpp`:
- Around line 1-9: Update the includes in the test file to explicitly include
the standard memory header before the PGConnPtr alias, ensuring std::unique_ptr
is declared without relying on transitive includes.
---
Nitpick comments:
In @.github/workflows/gh-actions-reusable/ci-pg-compat.yml:
- Line 43: Replace the job-level permissions setting near the CI compatibility
job with least-privilege permissions granting only contents read access. Keep
artifact uploading and the existing build and driver-image steps working without
retaining write-all or adding broader repository scopes.
In `@docs/superpowers/plans/2026-07-08-pgsql-sp3-driver-matrix.md`:
- Around line 75-82: The Java build stage currently downloads the pinned pgjdbc
JAR without validating its contents. Update the Dockerfile flow around
PGJDBC_VERSION and the /pgjdbc.jar download to verify the artifact with a
trusted SHA-256 checksum before javac consumes it, keeping the build fail-fast
when verification fails.
In `@test/pg-compat/tests/_subproc.py`:
- Line 12: The timeout initialization in _subproc.py should catch invalid
PGCOMPAT_BEHAVIOR_TIMEOUT values and raise a clear error that names the
environment variable and expected numeric format, while preserving the default
and valid-value behavior.
In `@test/tap/tests/pg_lite_client.cpp`:
- Around line 327-350: Extract the duplicated post-password authentication
handling from the cleartext and MD5 branches into a small helper near the
surrounding authentication logic. The helper should call readMessage, throw
PgException for ERROR_RESPONSE using extractErrorMessage, and return success
when AUTH_TYPE resolves to AuthenticationOk; invoke it after sendPassword and
sendMD5Password while preserving invalid-MD5 validation and existing
authentication flow.
- Around line 460-461: Replace the direct ntohl(reinterpret_cast...)
authentication subtype reads in the three SASL checks with readAuthType(),
passing the expected values 11, 12, and 0 respectively. Remove the duplicated
buffer-length checks from these conditions and rely on readAuthType() for
validation.
In `@test/tap/tests/pgsql-datatype_matrix-t.cpp`:
- Around line 194-197: The ERROR_RESPONSE branch in run_case() should emit a
diagnostic with the raw response payload via diag() before disconnecting and
returning false. Preserve the existing disconnect and failure flow, and ensure
the payload is passed so CI output includes the server error details.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #6020 +/- ##
==========================================
+ Coverage 53.06% 53.14% +0.08%
==========================================
Files 478 483 +5
Lines 143736 144166 +430
Branches 36348 36445 +97
==========================================
+ Hits 76267 76616 +349
+ Misses 50559 50524 -35
- Partials 16910 17026 +116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The gate failed on "D Security Rating on New Code" (17 vulnerabilities). Sonar analyses only NEW code, which is why patterns shared with existing files surface here for the first time. Dockerfile (12 findings), all real supply-chain hardening: - curl now pins --proto/--proto-redir '=https', so the -L redirect chain cannot be downgraded to plaintext HTTP (S6506). - npm installs drop the `|| npm install` fallback and add --ignore-scripts (S6505, S8543). Both lockfiles are committed, so `npm ci` always succeeds; the fallback could only ever fire when the lockfile was missing -- that is, it resolved unlocked versions exactly when reproducibility mattered most. --ignore-scripts is safe for Prisma because the explicit `prisma generate` below does the work @prisma/client's postinstall would have done. - `npx prisma generate` becomes `npx --no-install ...`: bare npx silently fetches an unpinned package from the registry when the binary is missing locally, defeating the lockfile (S6505, S8543). - pip installs with --only-binary :all: so no package runs a setup.py at install time (S8541); requirements.txt moves from ranges to exact pins (S8544). --require-hashes is deliberately not used -- a full transitive hash lock is a larger change than this PR should carry. - `COPY . .` is replaced by explicit paths (S6470). It had been sweeping the whole build context into the image: docs, host-side scripts, already- compiled driver sources, local __pycache__ and anything untracked. - The image now runs as a non-root user (S6471). This suite exists to execute third-party driver code against a live backend, so running it unprivileged meaningfully bounds a compromised dependency. Suppressions, following conventions already established in this repo: - CI-pg-compat.yml: write-all / @gh-actions / secrets: inherit carry NOSONAR markers matching CI-set_parser_algorithm_3-g1.yml. All three are repo-wide caller conventions (68 of 69 callers), and the branch ref is required by the documented two-branch caller/reusable model -- a SHA pin would break it. - pg_lite_client.cpp: MD5 carries a NOSONAR in the style already used in lib/DNS_Cache.cpp. PostgreSQL's AuthenticationMD5Password defines the response as an MD5 construction on the wire, so a client exercising that auth path has no alternative digest to choose. Verified by building the image and running it: 35 tests collect cleanly, all four driver wrappers execute as the non-root user and honour the exit-2 CLI contract, the Prisma client and its debian-openssl-3.0.x query engine are generated despite --ignore-scripts, and the pinned packages install from wheels. The first build of the explicit-COPY change caught a real regression (drivers/python is imported in-process by tests/test_behaviors.py and must ship in the final image); that is fixed and re-verified here.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/tap/tests/pg_lite_client.cpp (1)
379-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
UPPER_SNAKE_CASEfor the hexadecimal digit constant.Line 387 declares the local static constant as
hx. Rename it toHEX_DIGITS. Preferconstexprso the declaration also expresses immutability.Proposed fix
-static const char* hx = "0123456789abcdef"; +static constexpr char HEX_DIGITS[] = "0123456789abcdef"; ... - out.push_back(hx[digest[i] >> 4]); - out.push_back(hx[digest[i] & 0x0f]); + out.push_back(HEX_DIGITS[digest[i] >> 4]); + out.push_back(HEX_DIGITS[digest[i] & 0x0f]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/pg_lite_client.cpp` around lines 379 - 386, In md5_hex, rename the local hexadecimal digit constant hx to HEX_DIGITS and declare it constexpr, preserving its existing value and usage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/CI-pg-compat.yml:
- Around line 43-45: Update the reusable workflow invocation in the CI pg
compatibility workflow: replace the mutable GH-Actions ref with a reviewed full
commit SHA, replace permissions: write-all with only the required permission
keys, and replace secrets: inherit with explicit named secrets required by
ci-pg-compat.yml. Preserve the scheduled and manual dispatch behavior.
In `@test/pg-compat/Dockerfile`:
- Around line 76-81: Update the dependency installation in the Dockerfile and
requirements workflow to use a complete transitive dependency lock with hashes,
then enable pip’s --require-hashes during installation. Replace the current
intentionally unhashed requirements approach while preserving binary-only
installation.
In `@test/pg-compat/requirements.txt`:
- Around line 1-4: Replace the direct-only pins in the requirements lock,
including lines 11–14, with a complete transitive dependency lock containing
every resolved package and artifact hashes for reproducible, validated installs.
Alternatively, revise the lock-description comments to accurately state that the
file contains direct pins only.
---
Nitpick comments:
In `@test/tap/tests/pg_lite_client.cpp`:
- Around line 379-386: In md5_hex, rename the local hexadecimal digit constant
hx to HEX_DIGITS and declare it constexpr, preserving its existing value and
usage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dff0550-02bd-4af6-a724-7ff6f04d3709
📒 Files selected for processing (4)
.github/workflows/CI-pg-compat.ymltest/pg-compat/Dockerfiletest/pg-compat/requirements.txttest/tap/tests/pg_lite_client.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: Gitar
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (2)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/pg_lite_client.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/pg_lite_client.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/pg_lite_client.cpp
🪛 ast-grep (0.45.0)
test/tap/tests/pg_lite_client.cpp
[warning] 385-385: This hashing algorithm is insecure. If this hash is used in a security context, such as password hashing, it should be converted to a stronger hashing algorithm.
Context: MD5(reinterpret_cast<const unsigned char*>(in.data()), in.size(), digest);
Note: [CWE-328] Use of Weak Hash.
(insecure-hash-cpp)
🪛 Checkov (3.3.9)
test/pg-compat/Dockerfile
[medium] 59-60: Basic Auth Credentials
(CKV_SECRET_4)
🪛 GitHub Check: SonarCloud Code Analysis
test/pg-compat/Dockerfile
[warning] 19-20: Replace this invocation of "curl" with the ADD instruction.
[warning] 81-81: Using dependencies without locking resolved versions is security-sensitive.
🪛 zizmor (1.29.0)
.github/workflows/CI-pg-compat.yml
[error] 43-43: overly broad permissions (excessive-permissions): uses write-all permissions
(excessive-permissions)
[warning] 44-44: secrets unconditionally inherited by called workflow (secrets-inherit): this reusable workflow
(secrets-inherit)
🔇 Additional comments (2)
test/pg-compat/Dockerfile (2)
17-20: LGTM!Also applies to: 29-35, 51-63
82-97: LGTM!Also applies to: 115-122
| permissions: write-all # NOSONAR githubactions:S8234 — see the note above; matches all other caller workflows | ||
| uses: sysown/proxysql/.github/workflows/ci-pg-compat.yml@GH-Actions # NOSONAR githubactions:S7637 — branch ref matches all other caller workflows | ||
| secrets: inherit # NOSONAR githubactions:S7635 — matches all other caller workflows |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
caller='.github/workflows/CI-pg-compat.yml'
callee='.github/workflows/gh-actions-reusable/ci-pg-compat.yml'
# Inspect the caller and reusable workflow capability requirements.
rg -n -C 3 'permissions:|secrets:|uses:|upload-artifact|GITHUB_TOKEN|secrets\.' \
"$caller" "$callee"Repository: sysown/proxysql
Length of output: 3760
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow event/header lines =="
sed -n '1,55p' .github/workflows/CI-pg-compat.yml
echo
echo "== reusable workflow headers and relevant capability usage =="
sed -n '1,130p' .github/work S7637 — branch ref matches all other caller workflows.Repository: sysown/proxysql
Length of output: 3222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
caller='.github/workflows/CI-pg-compat.yml'
callee='.github/workflows/gh-actions-reusable/ci-pg-compat.yml'
echo "== caller top =="
sed -n '1,60p' "$caller"
echo
echo "== reusable top and permissions/secrets/token access =="
sed -n '1,160p' "$callee" | awk '
/^ permissions:/ || /^ permissions:/ || /^permissions:/ || /^secrets:/ || /GITHUB_TOKEN|secrets\./ {print NR": "$0}
'Repository: sysown/proxysql
Length of output: 2732
🌐 Web query:
GitHub Actions reusable workflows permissions inherit secrets inherit writable-GITHUB_TOKEN permission
💡 Result:
In GitHub Actions, you can manage secrets and GITHUB_TOKEN permissions for reusable workflows using specific configurations to ensure secure and efficient automation. Secrets To pass secrets from a caller workflow to a reusable workflow, you can use the secrets: inherit keyword [1][2]. When you use this keyword, all secrets defined at the repository or organization level in the caller's environment become available to the called workflow [3]. This simplifies the process compared to passing secrets individually [2]. Note that environment secrets cannot be passed via workflow_call; if you define an environment in a job within the reusable workflow, that environment's secrets will be used instead [4][5]. GITHUB_TOKEN Permissions Permissions for the GITHUB_TOKEN are governed by the following rules: 1. Default Permissions: If you do not explicitly define the permissions key in the caller job, the called reusable workflow will operate with the default GITHUB_TOKEN permissions [6][7]. 2. Downgrading Only: Permissions for the GITHUB_TOKEN passed from a caller workflow to a reusable workflow can only be downgraded, never elevated [6][7]. For example, if a chain of workflows exists (A > B > C), workflow B and C cannot have higher permissions than workflow A [7][8]. 3. Explicit Configuration: It is considered a best practice to explicitly define the permissions key at the workflow or job level in the calling workflow to ensure the reusable workflow has only the minimum access required [3][7][9]. Summary You do not need an explicit inherit keyword for permissions as you do for secrets; permissions are naturally constrained by the caller [6][7]. By using secrets: inherit and explicitly setting restrictive permissions in your caller job, you can effectively control the access level of your reusable workflows [10][3].
Citations:
- 1: https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows
- 2: https://github.blog/changelog/2022-05-03-github-actions-simplify-using-secrets-with-reusable-workflows/
- 3: https://itnext.io/github-actions-working-with-reusable-workflows-625f79b0586f
- 4: https://github.com/github/docs/blob/main/content/actions/how-tos/reuse-automations/reuse-workflows.md
- 5: https://docs.github.com/en/enterprise-cloud@latest/actions/how-tos/reuse-automations/reuse-workflows
- 6: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/reusing-workflow-configurations.md
- 7: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations
- 8: https://docs.github.com/en/enterprise-server@3.13/actions/reference/reusable-workflows-reference
- 9: https://github.com/github/docs/blob/main/content/actions/tutorials/authenticate-with-github_token.md
- 10: https://exlogare.net/blog/github-actions-reusable-workflows-workflow-call
Reduce the reusable workflow trust boundary.
This caller exposes write-all GitHub permissions and all caller secrets to sysown/proxysql/.github/workflows/ci-pg-compat.yml@GH-Actions, which is a mutable branch ref. If an update to that branch is accepted, it can run with these capabilities from the scheduled and manual dispatch paths. Pin the workflow to a reviewed full commit SHA, define only the required permissions, and pass only the named secrets the reusable job needs.
🧰 Tools
🪛 zizmor (1.29.0)
[error] 43-43: overly broad permissions (excessive-permissions): uses write-all permissions
(excessive-permissions)
[warning] 44-44: secrets unconditionally inherited by called workflow (secrets-inherit): this reusable workflow
(secrets-inherit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/CI-pg-compat.yml around lines 43 - 45, Update the reusable
workflow invocation in the CI pg compatibility workflow: replace the mutable
GH-Actions ref with a reviewed full commit SHA, replace permissions: write-all
with only the required permission keys, and replace secrets: inherit with
explicit named secrets required by ci-pg-compat.yml. Preserve the scheduled and
manual dispatch behavior.
Source: Linters/SAST tools
| # --only-binary :all: installs from wheels exclusively, so no package gets to | ||
| # run a setup.py at install time; every pin in requirements.txt publishes a | ||
| # manylinux/pure-python wheel. --require-hashes is deliberately NOT used: it | ||
| # would need a full transitive hash lock (pip-compile), which is a bigger | ||
| # change than this PR should carry. | ||
| RUN pip install --no-cache-dir --only-binary :all: -r requirements.txt |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'Dockerfile$|requirements.*\.txt$|requirements.*\.lock$|pyproject.*\.toml$' . | sed 's#^\./##' | sort
echo
echo "== Dockerfile relevant sections =="
if [ -f test/pg-compat/Dockerfile ]; then
wc -l test/pg-compat/Dockerfile
sed -n '1,140p' test/pg-compat/Dockerfile | cat -n
else
echo "test/pg-compat/Dockerfile not found"
fi
echo
echo "== requirements files =="
for f in $(fd 'requirements.*(txt|in|locked|lock)$|pyproject.*\.toml$' test . 2>/dev/null | sort); do
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
done
echo
echo "== hash/lock references =="
rg -n "require-hashes|pip-compile|pip-tools|requirements.lock|requirements.*lock|only-binary" .Repository: sysown/proxysql
Length of output: 14521
🌐 Web query:
Python pip --only-binary :all: does it skip setup.py
💡 Result:
The pip option --only-binary:all: does not inherently "skip setup.py" in the sense of avoiding the package build process; rather, it instructs pip to only use pre-built binary distributions (wheels) and to forbid the use of source distributions (sdists) [1][2]. When you use --only-binary:all:, pip will only search for and install packages that have pre-built wheels available for your specific platform and Python environment [1][3]. If a package does not have a compatible wheel, pip will fail to install it because it is prohibited from downloading or building from a source distribution [1][4]. In modern pip, building a package from source—even when necessary—is handled by creating a wheel first (using PEP 517 build isolation) rather than directly executing the legacy setup.py install command [5][6]. Therefore, --only-binary:all: effectively prevents the entire source-building pipeline for that package by restricting pip to existing binaries [1][7]. If your intention is to ensure that a package is built from source locally, you should use --no-binary:all: instead [1][4].
Citations:
- 1: https://pip.pypa.io/en/stable/cli/pip_install.html
- 2: https://pip.pypa.io/en/stable/cli/pip_install/
- 3: https://pip.pypa.io/en/stable/cli/pip_download.html
- 4: https://pip.pypa.io/en/stable/cli/pip%5Finstall/
- 5: Deprecate
--no-binaryimplyingsetup.py installpypa/pip#11451 - 6: Deprecate legacy setup.py install when --no-binary is used pypa/pip#9422
- 7: https://pip.pypa.io/en/stable/development/architecture/upgrade-options.html
🌐 Web query:
Python pip install --only-binary :all: source distribution setup.py
💡 Result:
The pip option --only-binary:all: is a directive that instructs pip to use only pre-built binary distributions (wheels) for all packages and to refuse to install from source distributions (sdists) [1][2]. If a package does not have a binary distribution available for your environment, the installation will fail [1][3]. When you use --only-binary:all:, you explicitly prevent pip from using setup.py or building the package from source [1][2]. Key points: - Purpose: It ensures that no code is compiled or built locally during installation [1][4]. This can be used for security (to avoid running arbitrary code in setup.py) or for consistency (ensuring only pre-verified binaries are used) [5]. - Interaction with setup.py: Ordinarily, pip may invoke setup.py or use a build backend (like setuptools, flit, or poetry) to build a wheel from source [6]. Using --only-binary:all: disables this entire process for all packages [1][2]. - Inverse option: The opposite flag is --no-binary:all:, which forces pip to ignore all pre-built wheels and compile everything from source [1][3]. - Fallback/Preference: If you want to use binaries when available but fall back to source when necessary, you should use --prefer-binary instead, which instructs pip to prefer the newest available binary over a source distribution [7][4]. In summary, --only-binary:all: is a strict requirement that mandates the presence of a pre-built wheel, effectively bypassing any source-based build steps [1][2].
Citations:
- 1: https://pip.pypa.io/en/stable/cli/pip_install/
- 2: https://pip.pypa.io/en/latest/cli/pip%5Finstall/
- 3: https://pip.pypa.io/en/stable/cli/pip_install.html
- 4: https://pip.pypa.io/en/stable/development/architecture/upgrade-options.html
- 5: https://www.b-list.org/weblog/2023/dec/07/pip-install-safely/
- 6: https://github.com/pypa/pip/blob/d043e4bcce6456b73920bedf69b1a0a4942ceff8/docs/reference/pip_install.rst
- 7: https://stackoverflow.com/questions/61734206/how-can-i-use-prefer-binary-with-pip-in-python-3
🌐 Web query:
Python pip install --require-hashes option documented
💡 Result:
The --require-hashes option in pip is a command-line flag used to enforce Hash-checking Mode, which verifies that every downloaded package matches a specific hash provided in a requirements file [1][2]. This feature is designed to ensure repeatable installs and protect against potential compromises of PyPI or the HTTPS certificate chain, as well as guarding against packages changing without an incremented version number [3]. Key details about --require-hashes: Enforcement: When this option is used, pip requires a hash to check each requirement against [1][4]. If a hash is missing for any package, the installation will fail [2]. Automatic Behavior: This mode is enabled automatically if at least one requirement in your requirements file includes a --hash option [1][5]. Environment Variable: You can set the environment variable PIP_REQUIRE_HASHES to achieve the same effect as passing the flag on the command line [1][4]. Use Case: It is primarily intended for secure, automated server deployments where exact reproducibility is required [2][3]. Related Commands: - pip hash: Can be used to compute the hash of local package archives, which is useful when you need to provide multiple hashes for different package distributions (e.g., source vs. binary) [6]. - --no-require-hashes: An option available to explicitly disable the automatic enabling of hash checking, which can be useful when mixing hashed requirements with non-hashed sources like local directories or VCS URLs [2][5].
Citations:
- 1: https://pip.pypa.io/en/stable/cli/pip_install.html
- 2: https://pip.pypa.io/en/stable/topics/secure-installs/
- 3: https://pip.pypa.io/en/stable/topics/repeatable-installs.html
- 4: https://pip.pypa.io/en/latest/cli/pip%5Finstall/
- 5: https://pip.pypa.io/en/stable/topics/secure-installs.html
- 6: https://pip.pypa.io/en/stable/cli/pip_hash.html
Lock and hash the full Python dependency closure.
--only-binary :all: only prevents source distributions and wheel builds; it does not pin transitive versions or check downloaded archive hashes against committed values. Use a full hash lock and install with --require-hashes.
Proposed fix
-COPY requirements.txt .
-RUN pip install --no-cache-dir --only-binary :all: -r requirements.txt
+COPY requirements.lock .
+RUN pip install --no-cache-dir --only-binary :all: --require-hashes -r requirements.lock🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 81-81: Using dependencies without locking resolved versions is security-sensitive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/pg-compat/Dockerfile` around lines 76 - 81, Update the dependency
installation in the Dockerfile and requirements workflow to use a complete
transitive dependency lock with hashes, then enable pip’s --require-hashes
during installation. Replace the current intentionally unhashed requirements
approach while preserving binary-only installation.
Source: Linters/SAST tools
Clears the last SonarCloud vulnerability (docker:S8544, "using dependencies without locking resolved versions") on the pip install. requirements.txt now pins the full transitive tree at exact versions, each with the SHA-256 of every wheel pip may legitimately select on linux -- pure-python wheels plus manylinux x86_64 and aarch64 for the compiled packages, so an arm64 build stays reproducible rather than failing the hash check. The Dockerfile consumes it with --require-hashes alongside the existing --only-binary :all:, so the install aborts if any artifact does not match a recorded hash. The header records how to regenerate the file after a version bump, since it is generated rather than hand-edited, and keeps the note that pytest 9.0.3 is a security floor (GHSA-6w46-j5rx-g56g) rather than a routine pin. Verified by rebuilding the final stage from scratch: the hashed install succeeds, the four pinned packages report the intended versions, 35 tests collect, and all four driver wrappers still honour the exit-2 CLI contract.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/pg-compat/requirements.txt (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the direct-dependency note with the lock entries.
Line 14 lists
psycopg[binary], but this file pinspsycopg==3.2.13andpsycopg-binary==3.2.13as separate top-level requirements on Lines 20 and 22. Update the note to list the actual entries. Otherwise, future regeneration can omit or independently changepsycopg-binary.Proposed documentation fix
-# Direct requirements are psycopg[binary], asyncpg, pytest and tomli; the rest +# Direct requirements are psycopg, psycopg-binary, asyncpg, pytest and tomli; the rest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pg-compat/requirements.txt` at line 14, Update the direct-dependency note in requirements.txt to list psycopg and psycopg-binary separately, matching the pinned top-level entries, while preserving the other dependencies already named.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/pg-compat/requirements.txt`:
- Line 14: Update the direct-dependency note in requirements.txt to list psycopg
and psycopg-binary separately, matching the pinned top-level entries, while
preserving the other dependencies already named.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f90799a5-ae35-4dd6-a9a1-0402cb4017d1
📒 Files selected for processing (2)
test/pg-compat/Dockerfiletest/pg-compat/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- test/pg-compat/Dockerfile
📜 Review details
🔇 Additional comments (1)
test/pg-compat/requirements.txt (1)
1-13: LGTM!Also applies to: 15-19, 20-73
pg_lite_client.cpp: doSASLAuth() still read the 4-byte auth type through `ntohl(*reinterpret_cast<int32_t*>(buffer.data()))` in three places, bypassing the readAuthType() helper added precisely to avoid that unaligned/aliasing read. An oversight in the earlier hardening pass -- the SCRAM path is now routed through the helper too, and no such cast remains in the file. The `buffer.size() < 4` guards are kept ahead of the calls so each site still throws its own specific message rather than the helper's generic one. harness/diff.py: making only-targets/skip-targets actually honour globs exposed a second defect. compare() checks every proxy target against its format-matched direct baseline, so a filter naming only proxy targets -- the documented `only-targets: proxy_native_*` names no direct target at all -- removed the baselines and the case then failed with "baseline unavailable" no matter how transparent the proxy was. Before globs worked, that same filter matched nothing and skipped every target, i.e. it passed vacuously; the fix turned a silent non-test into a false failure. Baselines are now pulled back in after filtering, and the pairing rule lives in one shared baseline_name() used by both the filter and the assertion so they cannot drift apart. Pinned by a new infra-free unit test. ci-pg-compat.yml: add the GHCR login/pull step that retags ghcr.io/sysown/proxysql-ci-base:latest as proxysql-ci-base:latest before ensure-infras.bash runs. start-proxysql-isolated.bash runs ProxySQL from that local-only tag, which nothing on a fresh runner provides, so infra startup would have failed before a single pg-compat test executed. Mirrors ci-legacy-g4.yml, retry loop included, since both the login and the pull have been observed to fail transiently. The gap went unnoticed because the job only runs behind the 'pg-compat' label. Plan docs: the SP-1 plan was left internally inconsistent by the previous commit, which updated main() to the Observed/expected_binary_hex contract without updating Case, Observed or run_case. The contract is unified, and the case table is now a short excerpt that points at the shipped test as the source of truth instead of a full duplicate that drifts. The SP-2 plan still showed the _parse() that drops SQL following a metadata comment -- the bug the shipped _statements() documents fixing -- so it is synced.
|
CI failed: TAP test failure in the cluster simulation Galera test group (`test_cluster_sim_galera-t`), causing the CI job to fail.Overview1 test failure found across 1 analyzed log in the cluster simulation Galera test suite. FailuresGalera Cluster Simulation TAP Test Failure (confidence: high)
Summary
Code Review ✅ Approved 1 resolved / 1 findingsCombines the PostgreSQL protocol and compatibility test stack (SP-1 through SP-3) into a single branch with comprehensive driver matrix and polyglot foundation support, addressing the SCRAM auth-type reads finding. ✅ 1 resolved✅ Quality: SCRAM auth-type reads bypass the memcpy-safe readAuthType helper
Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Note on the
|
1 participant
Combines #5894 (SP-1), #5903 (SP-2) and #5910 (SP-3) into a single branch on top of current
v3.0, and fixes the review findings that accumulated on all three.Those three PRs are a stacked chain — 5894 →
v3.0, 5903 → 5894, 5910 → 5903 — so they could only ever merge in order, and only 5894 conflicted withv3.0. Rebasing and re-running CI on each in turn would have meant three full CI cycles for what is really one body of work. This branch takes the chain tip once, resolves the single conflict, and applies every outstanding review fix in one place.What this adds
The PostgreSQL protocol/compatibility test stack, in three layers:
pg_lite_client.pg-compatCI workflow.75 files, +7994/−21 against
v3.0. Nolib/,src/orinclude/changes — this is entirely test, infra, CI and docs.Merge conflict resolution
One conflict:
test/tap/tests/Makefile.v3.0had moved these targets to$(TAP_LDIR)/libtap$(SHLIB_EXT)and added apgsql-reg_test_5899_bind_zero_param_formats-ttarget; the PR side had added-lscram -lusualfor the now-SCRAM-capablepg_lite_client.cppbut hardcodedlibtap.soand-Wl,--allow-multiple-definition.Resolved by taking
v3.0's structure and adding the PR's link requirements — and usingv3.0's existing portable idioms rather than either side verbatim:$(SHLIB_EXT)and$(ALLOW_MULTI_DEF), both of which degrade correctly on Darwin where the hardcoded forms would not. The ten targets that linkpg_lite_client.cpp(includingreg_test_5899, which needs the SCRAM libs now that it shares that translation unit) are collapsed into onePG_LITE_CLIENT_TESTSstatic pattern rule, so the list can't drift out of sync again.Review fixes
Real defects, in code and in the plan docs that described them
pgsql-datatype_matrix-t: thetimestamptzcase never testedtimestamptz.AT TIME ZONE 'UTC'yieldstimestampwithout time zone, so the row asserted OID 1114 under the labeltimestamptz(1184). Now uses a realtimestamptz, with the session pinned to UTC so its text rendering stays deterministic.*_send()emits and theDataRowpayload is compared byte-for-byte.harness/diff.py:only-targets/skip-targetsglobs never matched. They are documented as glob patterns, but were matched with exact set membership — so the documentedonly-targets: proxy_native_*selected nothing and silently skipped the whole case, a vacuous pass. Now matched withfnmatchcase, pinned by a new infra-free unit test.pg_lite_client.cpp: out-of-bounds reads in the auth path. The 4-byte auth sub-type was re-read afterreadMessage()refilled the buffer without re-validating its length (and via an unalignedreinterpret_cast); theErrorResponsebranch hand-walked a rawchar*with no bounds check at all. Both now go through bounds-checked helpers.Robustness
pgsql-auth_method_matrix-t: only the firstpgsql-authentication_methodswitch checked its result. A failedSET/LOADleft the previous floor active, so a "scram floor" assertion could actually exercise md5 and a wrong-password check could pass for the wrong reason. Every switch is now a checked precondition.pgsql-server_side_cursors-t:BEGIN/DECLARE/MOVE/CLOSE/COMMITwere fire-and-forgetPQexec()calls that leaked theirPGresultand hid failures behind a misleading row-count assertion.tests/_subproc.py: an uncaughtsubprocess.TimeoutExpiredturned a hung driver into a pytest error during teardown, discarding the partial output that identifies where it hung.behaviors/transactions.py: if a statement raised mid-transaction, thefinallyblock'sDROP TABLEran on a session in PostgreSQL's aborted-transaction state — so the drop failed, the table leaked, and the cleanup error masked the original failure.Security / supply chain
ci-pg-compat.yml:persist-credentials: false. The job builds and tests but never pushes, yet itswrite-alltoken was persisted into.git/configwhere every later step — including the third-party driver images this suite builds and runs — could read it.Dockerfile: the pgjdbc jar is now verified against a pinned SHA-256. (Maven Central publishes.sha1but not.sha256for this artifact; the recorded digest is that of the jar whose SHA-1 matches the published one.)requirements.txt:pytestmoved off the8.*line. GHSA-6w46-j5rx-g56g (insecure tmpdir handling) affects every release before 9.0.3, so no8.*pin could pick up the fix. The suite uses only stable APIs and the 3.11 base image satisfies pytest 9's>=3.10requirement.docker-proxy-post.bash: the ProxySQL admin wait loop was unbounded (it now fails afterPROXY_WAIT_SECONDS, default 120, and dumps container logs), and the SQL template was expanded witheval-echo— running the whole file through the shell — from a cwd-relative path. Nowenvsubstwith an explicit variable list, resolved relative toSCRIPT_DIR. Verified to produce byte-identical output to the previousevalfor the current template.Cleanup
Stale "SP3-Task-1 scaffold / not implemented" comments in the Go, Java and Node behavior programs contradicted code where all four behaviors are complete. The Go
errNotImplementedsentinel and the NodeNotImplementedErrorclass were never raised, so their dispatch branches were dead; removed, along with the now-unusederrorsimport.Deliberately not changed
docker-compose-init.bash'sINFRA_IDfallback (basename $(dirname $(pwd)), which derives the wrong name and reads the caller's cwd rather than the script's). CodeRabbit flagged it on the new infra, and the criticism is correct — but this line is byte-identical across all 26docker-compose-init.bashfiles intest/infra/. It is a repo-wide convention that the new infra copied, not a defect introduced here. Fixing it in one file would leave it inconsistent with 25 siblings; fixing all 26 is a separate change. In practice the harness always exportsINFRA_ID, and adev-$USERsafety net follows. Worth a dedicated follow-up.Also noted but not changed: this workflow runs with
permissions: write-all, which is broader than a build-and-test job needs. Narrowing it risks breaking steps I can't exercise locally, so it is called out rather than guessed at.Verification
PROXYSQL31=1 make debug -j$(nproc)from a clean tree — succeeded.PROXYSQL31=1 make build_tap_test_debug -j$(nproc)— all twelve TAP binaries this branch adds or touches build and link, including all ten that linkpg_lite_client.cpp. This was the load-bearing check: the Makefile conflict is exactly the kind that only fails at link time.go vetclean andgofmtreports nothing;node --check;python -m compileallovertest/pg-compat/;bash -nover every changed shell script; YAML parse of the workflow.Not run locally: the pg-compat pytest suite and the PG TAP tests themselves, which need the dbdeployer/Toxiproxy/driver-image infra stood up. Those are for CI.
SonarCloud
#5894 and #5903 both failed the gate on "D Security Rating on New Code". Those analyses have since been purged (the API returns
Component ... of pull request '5894' not found), so the specific findings could not be recovered and fixed pre-emptively. A fresh analysis on this PR will produce actionable data; I'll address what it reports.One likely contributor worth flagging up front:
pg_lite_client.cppimplements PostgreSQL's MD5 authentication, which mandates MD5 and trips static-analysis weak-hash rules. That usage is protocol-required and confined to test code.Disposition of the original PRs
#5894, #5903 and #5910 stay open for now so their review threads remain live; they should be closed once this merges.
Summary by CodeRabbit
New Features
Bug Fixes
CI
Documentation