PgSQL native backend protocol: replace libpq on the data path (connect/auth/TLS, simple query, COPY, extended query via stmt pipeline, Describe cache) - #5882
Open
renecannao wants to merge 89 commits into
Conversation
…ing + unit tests Add PgSQL_Backend_Msg_Framer: a pure wire-message framer for the native PostgreSQL backend protocol. It accepts fed bytes (possibly partial) and yields complete messages (type byte + 4-byte big-endian length-prefixed body), signaling FRAME_NEED_MORE on incomplete trailing bytes and FRAME_ERROR on a malformed length. Header stays light (cstdint/cstddef only). Destructor frees the realloc'd buffer to avoid a leak on long-lived connections. Wire it into libproxysql.a via _OBJ_CXX in lib/Makefile. Also fix a pre-existing duplicate vec.o on the unit-test link line: the test/tap/tests/unit/Makefile appended SQLITE3_LDIR/vec.o to STATIC_LIBS in two separate PROXYSQL40 blocks, producing ~98 duplicate-symbol errors that broke every unit test under PROXYSQL40. Keep the single append after the autodetection block.
…hecks, noncopyable)
…proof-correctness test
…nc_connect assert) + timeout teardown [Task 1.6a]
… verification) [Task 1.6b]
…nding Docker) [Task 1.8]
…hrough results) [Task 1.6c / Phase 2 core]
The native simple-query path appended a NUL to query.length bytes, but the client-query callers (async_query with pgsql_real_query.QuerySize) pass a length that already includes the trailing NUL, producing a malformed double-NUL Query body. PostgreSQL rejects it with 08P01 'invalid message format', breaking the backend connection. Normalize to the SQL up to the first NUL (bounded by query.length) plus a single terminator, matching PQsendQuery semantics. The strlen()-based callers (async_send_simple_command/init_connect) are unaffected.
…nnection is_connection_in_reusable_state() called PQtransactionStatus(pgsql_conn) directly; in native mode pgsql_conn is NULL so libpq returns PQTRANS_UNKNOWN, making the session treat a normal backend query error (ErrorResponse + ReadyForQuery, the connection is still idle/reusable) as a broken connection and retry instead of forwarding the error (with its SQLSTATE) to the client. Derive the transaction status from the natively-tracked ReadyForQuery byte in native mode.
…nd-point,, when cbind is set
… both offered and TLS
New TAP test pgsql-native_cancel-t drives an identical client-visible query
cancellation in both libpq and native backend modes and asserts the outcomes
match. For each mode it starts a long SELECT pg_sleep(30) through ProxySQL,
fires a frontend CancelRequest via PQcancel(), and asserts:
- the query aborts with SQLSTATE 57014 (canceling statement due to user
request) — the empirical libpq-mode bar,
- the cancel takes effect promptly (well under the 30s sleep),
- the client session stays usable afterward (SELECT 1),
- the backend query is actually gone (checked on a DIRECT backend connection
via pg_stat_activity),
- the native phase truly exercised the native path (no libpq fallback), and
- libpq vs native produce the identical outcome.
The libpq phase runs first as the differential bar; the native phase must
match it. Registered in groups.json alongside the sibling native tests
(legacy-g1 and the mysql-* variant groups). 10/10 assertions pass.
Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…ership double-free in auth unit test A1 (HIGH, production): the rc0 epilogue called clear_named_portals() before RequestEnd()/LogQuery(), freeing the Bind-message packet that CurrentQuery.extended_query_info.stmt_client_name still pointed into; PgSQL_Event::write_query_format_2_json then read the freed pointer when eventslog format=2 (JSON) is enabled. Defer the destructive clear_named_portals() call until after RequestEnd() runs (which nulls stmt_client_name via CurrentQuery.end() only after logging), while computing sticky_backend_connection with the same pre-clear-equivalent value it used before, so pinning behavior is unchanged. Audited every other clear_named_portals()/reset() call site for the same hazard; none are affected (RequestEnd already ran first, or the current command's CurrentQuery never references the registry being cleared). A2 (test-only): pgsql_backend_auth-t's loopback-TLS fixtures (cases 11/12) shared one sbio/cbio pair across both SSL objects via SSL_set_bio(), which consumes one reference per BIO role; the second SSL_free() therefore double-freed. Add BIO_up_ref() on each BIO before the second SSL_set_bio(), ported from the wt-asan worktree fix. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…sertion in cancel test Review hardening on the native CancelRequest work (41df3ca/ad4312c72): 1. pg_native_send_cancel_request: the blocking connect() was unbounded — a black-holed backend would park the detached kill thread for the kernel's full connect timeout (~2min). Now: non-blocking connect + poll(POLLOUT) with a 5s bound + SO_ERROR check, then blocking send bounded with SO_SNDTIMEO. Primitive stays self-contained; fd closed on all paths. 2. pgsql-native_cancel-t: the native phase previously proved native-path engagement only by absence of the libpq-fallback tripwire. It now also POSITIVELY asserts the raw-CancelRequest branch ran, via a single-pass scan for the "Canceled query (native) on ... successfully" log line that only that branch emits (combined scan with the fallback regex so the two checks don't consume each other's lines), folded into the case result. 3. pgsql-native_cancel-t: documented why the direct pg_stat_activity check on saved[0] is sufficient (single-backend infra; hostgroups 0/1 point at the same host:port). Also softened the TLS limitation comment per review: PostgreSQL processes CancelRequest at the startup-packet layer before SSL negotiation and pg_hba matching, so a plaintext cancel commonly succeeds even against hostssl-only backends; a refusal is still handled gracefully (error + counter, query runs to completion — same as a lost PQcancel). Verified: make debug clean; container restarted; pgsql-native_cancel-t 10/10 (RC 0) with native_cancel_logged=1 in the native phase. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…t-owner teardown path, eventslog on)
New case PORTAL_STMT_LAST_OWNER: Parse s1 -> Bind p1(s1) -> Close('S', s1)
-> Execute(p1) -> Sync. Closing the statement while its portal lives makes
the portal's registry entry the LAST shared_ptr owner of the statement info;
PostgreSQL keeps the portal executable (both legs return the row), and the
Sync's implicit-txn teardown drops that final reference in the rc0 epilogue
— the exact ordering d561b76 fixed (clear_named_portals() deferred until
after RequestEnd()/LogQuery() has read stmt_client_name/digest for the
eventslog). Runs with the eventslog verified active (infra default
default_log=1/format=2; forced+restored otherwise) so the format=2 JSON
writer actually performs the read the UAF hit. The differential alone can
pass on a lucky heap (the UAF historically fired only under ASAN), so the
case additionally asserts ProxySQL_Uptime stayed monotonic across the case
(no crash/angel restart) and its primary value — making this path exist for
future ASAN runs — is documented in the script comment.
pgsql-native_portals-t: 13/13 ok, RC 0
(case detail: backend='1[]2[]3[]D[..9]C[SELECT 1]Z{I}', native=yes,
uptime_monotonic=349->349, eventslog default_log=1 format=2).
Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…MIT warning storm)
The native path fed PgSQL_ExplicitTxnStateMgr TWICE per query: once from the
native ReadyForQuery ('Z') handler in add_native_backend_message() (added by
e9428cb when native completion bypassed the shared handler epilogue) and once
from PgSQL_Session::handler()'s post-RunQuery rc0 epilogue (the libpq path's
single hook, which native completion also returns through after the extended-
query stmt-pipeline refactor). The first call registered/cleared the txn; the
second re-ran start_transaction()/commit() on the now-updated state and tripped
its 'already/no transaction in progress' warning branch -- once per transaction,
for simple AND extended-protocol BEGIN/COMMIT alike (pgbench -M prepared's per-
COMMIT warning storm, 3.22M lines / ~900MB in a 60s bench run; 0 in libpq mode).
Remove the redundant 'Z'-handler call; handler() owns the single registration for
both modes. handler() fires for every native completion path (simple query,
extended sync-terminated Execute, and extended flush-terminated Execute -- the
last never reaches the 'Z' handler), so no case is left uncovered. libpq behavior
is unchanged. native_txn_status capture and buffer flush in the 'Z' handler stay.
Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
Extend pgsql-native_transactions-t with extended-protocol BEGIN/COMMIT: E0-E2 via PQexecParams (unnamed extended portal), E3-E4 via PQprepare+PQexecPrepared (the exact pgbench -M prepared shape; E4 = 3 cycles). Fold a positive-absence assertion into every case's result_match: scan_native_window() requires ZERO 'no/already transaction in progress' warnings in the native-run log window (applies to the 15 simple cases too -- the double-registration bug hit them as well). Two log-scrape correctness fixes needed for the assertion to bite: clear the stream eofbit left by drainLogToNow() before scanning (same fix wait_for_log_match documents), and match the case-correct substring -- the log emits capital 'There', RE2 is case-sensitive. Verified: against a server with the bug re-introduced all 20 cases fail (native_txn_warnings=2/6/1); against the fixed server 21/21 green with 0 warnings emitted. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
…(§16-§17) Hard-won during the pgsql native-backend work: debug-only harness, shared lib/obj between flavors, file-bind-mounted binary (restart after rebuild, never rebuild mid-run), INFRA type-vs-id confusion, INFRA_ID collisions, docker-start-skips-provisioning. Claude-Session: https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/PgSQL_Protocol.cpp (2)
2818-2830: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep CommandComplete row-count parsing bounded.
If a malformed
CommandCompletepayload omits the terminating NUL,strtoull()can read pastpayload_len. Parse the trailing digits manually or requiretaglen < payload_lenbefore using C-string APIs.Proposed fix
if (start < end) { // We have a trailing number; this is the affected-rows count. - affected_rows = strtoull((const char*)(payload + start), NULL, 10); + uint64_t parsed = 0; + for (uint32_t j = start; j < end; j++) { + parsed = parsed * 10 + (uint64_t)(payload[j] - '0'); + } + affected_rows = parsed; }🤖 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 `@lib/PgSQL_Protocol.cpp` around lines 2818 - 2830, The CommandComplete row-count parsing in PgSQL_Protocol.cpp is still using a C-string conversion on an untrusted payload slice, which can read past payload_len when the NUL terminator is missing. Update the affected-rows extraction logic in the CommandComplete handling block to stay bounded by payload_len, either by manually accumulating the trailing digits from payload[start..end) or by only calling strtoull after confirming the terminator is within bounds and the slice is safely NUL-terminated.
2747-2756: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard message-size arithmetic before reserving/copying.
payload_len + 4and1 + 4 + payload_lencan wrap for malformed backend frames, leading to undersized allocation followed bymemcpy()of the originalpayload_len.Proposed fix
unsigned int PgSQL_Query_Result::add_native_backend_message(char type, const unsigned char* payload, uint32_t payload_len) { // Reconstruct the raw client-wire message: type(1) + be32 length(4) + payload. // The length field is (payload_len + 4) per the PostgreSQL wire protocol (it // counts itself but not the type byte). + if (payload_len > UINT32_MAX - 4 || payload_len > UINT_MAX - 5) { + result_packet_type |= PGSQL_QUERY_RESULT_ERROR; + return 0; + } const unsigned int size = 1 + 4 + payload_len; const uint32_t wire_len = (uint32_t)(payload_len + 4);🤖 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 `@lib/PgSQL_Protocol.cpp` around lines 2747 - 2756, Guard the message-size arithmetic in PgSQL_Protocol before calling buffer_reserve_space and l_alloc: in the packet-building path around payload_len, verify that payload_len is small enough that both payload_len + 4 and 1 + 4 + payload_len cannot overflow. If the size is invalid, reject the frame or fail early before any allocation/copy, and keep the check close to the code that computes wire_len, size, and uses memcpy so the standalone packet path cannot allocate too small a buffer.
🤖 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 `@test/tap/tests/pgsql-native_transactions-t.cpp`:
- Line 270: The test file currently defines NativeLogScan multiple times,
causing a redefinition compile error. Remove the duplicate struct declaration
and keep only one NativeLogScan definition near the related native transaction
scan test setup, then update any references in the surrounding test code to use
that single definition.
---
Outside diff comments:
In `@lib/PgSQL_Protocol.cpp`:
- Around line 2818-2830: The CommandComplete row-count parsing in
PgSQL_Protocol.cpp is still using a C-string conversion on an untrusted payload
slice, which can read past payload_len when the NUL terminator is missing.
Update the affected-rows extraction logic in the CommandComplete handling block
to stay bounded by payload_len, either by manually accumulating the trailing
digits from payload[start..end) or by only calling strtoull after confirming the
terminator is within bounds and the slice is safely NUL-terminated.
- Around line 2747-2756: Guard the message-size arithmetic in PgSQL_Protocol
before calling buffer_reserve_space and l_alloc: in the packet-building path
around payload_len, verify that payload_len is small enough that both
payload_len + 4 and 1 + 4 + payload_len cannot overflow. If the size is invalid,
reject the frame or fail early before any allocation/copy, and keep the check
close to the code that computes wire_len, size, and uses memcpy so the
standalone packet path cannot allocate too small a buffer.
🪄 Autofix (Beta)
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
Run ID: c4c8f9a1-399e-4705-8a9d-324b79a17541
📒 Files selected for processing (3)
doc/agents/common-mistakes.mdlib/PgSQL_Protocol.cpptest/tap/tests/pgsql-native_transactions-t.cpp
📜 Review details
🧰 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/pgsql-native_transactions-t.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/pgsql-native_transactions-t.cpplib/PgSQL_Protocol.cpp
🧠 Learnings (4)
📚 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/pgsql-native_transactions-t.cpp
📚 Learning: 2026-04-11T13:16:05.854Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:16:05.854Z
Learning: When validating GitHub-rendered Markdown in this repository (e.g., links that use heading anchors), account for GitHub slug behavior for headings containing an em-dash (—) surrounded by spaces: GitHub strips the em-dash and converts each surrounding space into a hyphen independently, which can produce a double hyphen (--) in the generated anchor. Therefore, do NOT flag as broken links any anchors whose expected slug contains a double hyphen specifically attributable to an em-dash surrounded by spaces in the source heading. (Example: `...vocabulary — read...` -> `...vocabulary--read...`.)
Applied to files:
doc/agents/common-mistakes.md
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
doc/agents/common-mistakes.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
doc/agents/common-mistakes.md
🪛 LanguageTool
doc/agents/common-mistakes.md
[style] ~306-~306: Consider using a different adjective to strengthen your wording.
Context: ...h run-tests-isolated.bash without the full group.
(FULL_ENTIRE)
🔇 Additional comments (3)
doc/agents/common-mistakes.md (1)
275-292: LGTM!Also applies to: 293-306
test/tap/tests/pgsql-native_transactions-t.cpp (1)
172-240: LGTM!Also applies to: 254-269, 271-295, 328-350, 363-400, 402-526, 565-565
lib/PgSQL_Protocol.cpp (1)
2760-2817: LGTM!Also applies to: 2831-2907
Contributor
Author
Hardening round: cancellation, sanitizer pass, and the promised benchmarkNative query cancellation implemented ( ASAN pass over the whole native suite (dedicated worktree, full sanitizer build): 10 TAP tests + 12 unit binaries green under ASAN; the branch's manual-memory surfaces (portal registry, raw captures, describe cache, framer, builders) produced zero sanitizer records — except one real find: a use-after-free between named-portal teardown and the event logger ( Benchmarking found a third bug before producing numbers: native mode registered explicit transactions twice (per-ReadyForQuery handler + shared epilogue), tripping a per-transaction "no transaction in progress" warning — 3.2M log lines / ~900MB during a single pgbench run. Fixed by removing the duplicate call, with a reviewer-verified invariant proof that the epilogue covers every native completion path ( Benchmark (release build, host-run proxysql, dedicated postgres:16 backend, 54×60s interleaved runs, 3 passes/cell; native-vs-libpq, median tps / proxy CPU):
The read-path gains at equal-or-lower proxy CPU are consistent with the double-copy elimination this PR exists for. The tpcb@c32 cell (fully write-saturated through one backend) is within noise across passes; flagged honestly rather than averaged away. Full methodology + limitations in the branch session reports ( Filed along the way: #5896 (connect-path debug assert on unresolvable host), #5897 (pre-existing native SET-tracking gaps), #5904/#5905/#5906 (concurrent soak, TLS corpus, fake-server robustness harness — the queued testing follow-ups). Agent-facing infra hazard catalog added to Current test matrix at HEAD |
…backend-protocol # Conflicts: # lib/PgSQL_Session.cpp
…mits
The proxy_debug() macro is gated on the admin-debug master switch
(GloVars.global.gdbg, include/proxysql_debug.h): unless admin-debug='true',
every proxy_debug() call is a runtime no-op and no MOD# line is ever written
to the foreground/teed proxysql.log.
The docker-pgsql16-single infra never provisioned admin-debug — unlike every
MySQL infra, whose docker-proxy-post.bash applies conf/proxysql/infra-config.sql
(SET admin-debug='true'; admin-debug_output=2; debug_levels verbosity=7). So
debug-level markers scraped by pgsql-native_prepared-t P25/P26 ("Describe
served from metadata cache", emitted at proxy_debug(PROXY_DEBUG_MYSQL_COM,5))
could never appear -> cache_hits_2nd_mode=0.
Mirror the MySQL convention in config.sql (applied by docker-proxy-post.bash):
enable admin-debug, keep debug_output=2 (debug DB only, no stderr flood), and
set module verbosity=7 (except pkt_array/net). Tests raise debug_output to 3
for their scrape phase via DebugLogScope.
Not a v3.0 code regression: all debug-propagation code (debug.cpp,
proxysql_debug.h, main.cpp gdbg default, ProxySQL_Admin.cpp gdbg/set_variable)
is byte-identical across 181e87c..89ed5b6; the FlushVariableStats admin
refactor is purely additive and debug output works end-to-end once admin-debug
is enabled. This is a latent infra provisioning gap this infra always had.
|
plisandro
pushed a commit
to plisandro/proxysql
that referenced
this pull request
Aug 10, 2026
- 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 sysown#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 sysown#5882 (already ships pgsql-native_notify-t), not this spec.
plisandro
pushed a commit
to plisandro/proxysql
that referenced
this pull request
Aug 10, 2026
…consistency hardening (final review) - diff.py: snapshot/restore pgsql-use_native_backend_protocol around the target loop in _run() (shared by run_case/run_case_sql) so a native-mode toggle never leaks into later cases once PR sysown#5882 lands the variable; a pure no-op today since the variable is absent. - conftest.py: pin client_encoding=UTF8 on the proxy DSN, matching targets.py and drivers/python/adapter.py. - behaviors/{connect,prepared,session_isolation}.py: wrap bodies in try/finally so connections close even on assert failure; make PsycopgAdapter.close() idempotent since session_isolation.py's finally may close an already-closed connection. - behaviors/transactions.py: fix stale comment pointing at a nonexistent harness/oracle.py; oracle_w lives in tests/test_routing_oracle.py.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5882 +/- ##
==========================================
- Coverage 60.80% 59.78% -1.03%
==========================================
Files 638 635 -3
Lines 180671 178098 -2573
Branches 45653 45136 -517
==========================================
- Hits 109860 106476 -3384
- Misses 48193 49483 +1290
+ Partials 22618 22139 -479
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:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 52 files
Not reviewed (too large): lib/PgSQL_Connection.cpp (~2,345 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deps/libscram/src/scram.c">
<violation number="1" location="deps/libscram/src/scram.c:514">
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:133">
P2: The pinned `expected_c_b64` literal in test 14 is invalid: it base64-decodes to the mangled 44-byte blob `p@es-server-end-point, \0...`, not to base64 of the gs2 header + SHA-256 digest (correct value begins `cD10bHMtc2VydmVyLWVuZC1wb2ludCws...`). A developer trusting this literal — or recomputing it per the plan's fallback instruction using the incorrect 22-byte header — gets a wrong assertion that masks rather than detects the channel-binding bug. Fix the header length first (see related finding), then pin the correct literal: base64 of the 24-byte header + 32-zero digest = `cD10bHMtc2VydmVyLWVuZC1wb2ludCws` + 56 `A`s.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:202">
P2: Test 15 uses `read_client_first_message` to validate the cbind path, but the vendored libscram server-side reader rejects cbind flag 'p' (`case 'p': ... "client requires SCRAM channel binding, but it is not supported"` and returns false). The plan never modifies that reader, so `parsed` is always false and the final assertion `ok(parsed && cbind_flag == 'p')` can never pass — the test is guaranteed to fail, contradicting the plan's "all 15 tests ok" expectation and the "round-trip through the independent libscram server-side verifier" claim. If the reader were ever patched to accept 'p', the `build_client_final_message(client, nullptr, server_first, nullptr, 0, 0)` call would then dereference NULL `credentials` in `calculate_client_proof` (`credentials->has_scram_keys` on a nullptr, and `credentials->passwd`), crashing the test. Test 15 needs a real server-side verifier path (or to be dropped) rather than this 'p'-rejecting parse smoke.</violation>
<violation number="3" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:334">
P1: Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</violation>
<violation number="4" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:526">
P2: Tests 8 and 9 build a dummy SSL with `SSL_CTX_use_certificate` + `SSL_new` and expect `pg_tls_server_end_point` (via `SSL_get_peer_certificate`) to return the cert digest. `SSL_get_peer_certificate` returns the peer's certificate only after a completed TLS handshake; a client SSL that never handshakes has no peer cert, so it returns NULL and `pg_tls_server_end_point` returns -1. Both tests would fail on `rc >= 0` and cannot validate the digest logic against a fake, non-handshaked SSL. The digest helper should be tested against the X509 directly (or behind a real handshake fixture), not through SSL_get_peer_certificate on a dummy SSL.</violation>
<violation number="5" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:787">
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</violation>
</file>
<file name="lib/PgSQL_Backend_Auth.cpp">
<violation number="1" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</violation>
<violation number="2" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a backend offers SCRAM-SHA-256-PLUS over TLS, `use_scram_plus` is true and `pg_scram_client_first(native_scram, true)` returns nullptr from the `if (channel_binding) return nullptr;` guard, so the native connection fails with 'SCRAM client-first failed' instead of completing channel-bound auth. libscram's `build_client_first_message` already emits the `p=tls-server-end-point,,` header based on the cbind input installed by `pg_scram_set_cbind`, so the `channel_binding` bool is redundant and the early return is harmful. Remove the guard and drive the header from the cbind state.</violation>
<violation number="3" location="lib/PgSQL_Backend_Auth.cpp:157">
P2: When a backend password is 2047 bytes or longer, this copy truncates it silently before SCRAM derives the proof. Preserve the complete password for SCRAM, or detect this case and route the connection through the libpq fallback instead of attempting authentication with a different secret.</violation>
</file>
<file name="include/PgSQL_Backend_Protocol.h">
<violation number="1" location="include/PgSQL_Backend_Protocol.h:106">
P1: When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</violation>
</file>
<file name="include/PgSQL_PreparedStatement.h">
<violation number="1" location="include/PgSQL_PreparedStatement.h:35">
P2: After a Describe cache is populated, prepared-statement memory usage under-reports the cache's payload allocations. Add the cached string capacities to `total_mem_usage` or otherwise include them in `get_memory_usage()` so the admin metric remains accurate.</violation>
<violation number="2" location="include/PgSQL_PreparedStatement.h:88">
P1: When a client changes `search_path` or uses a schema/temp object that changes name resolution, this global set-once cache can return the previous session's `RowDescription` without contacting PostgreSQL. The documented DDL-staleness trade-off does not cover these session-state changes; include the relevant state in the cache identity or invalidate/bypass the cache whenever it changes.</violation>
</file>
<file name="lib/PgSQL_Backend_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Backend_Protocol.cpp:13">
P1: When receive boundaries repeatedly leave a partial trailing backend frame, `PgSQL_Backend_Msg_Framer` retains consumed prefixes and grows based on total received bytes, not buffered bytes. Compact `buf + pos` before reallocating so long-running streamed results cannot consume unbounded memory despite each message staying below `PGSQL_MAX_BACKEND_MSG_LEN`.</violation>
</file>
<file name="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:62">
P1: The buffer sizes and unit-test expectations derived from the wrong 22-byte prefix must also change: §3.2 says out_cap >= "22 + 64 = 86" and "22 + 32 = 54", and §3.9 allocates `unsigned char cbind_input[86]`. With the correct 24-byte prefix, SHA-512-signed certs need 24+64=88 bytes, so the 86-byte buffer is too small: `pg_scram_build_cbind_input_tls_server_end_point` returns -1 and the code hits `assert(0)`/teardown, so a valid -PLUS connection with a SHA-512-signed backend cert cannot complete. Tests 11/12 (54/86-byte buffers) and Test 13's `22+digest_len` are likewise off by two.</violation>
<violation number="2" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P1: The gs2-cbind prefix `"p=tls-server-end-point,,"` is 24 bytes (p= + 20-byte `tls-server-end-point` + two commas), not 22. §3.2's `memcpy(out, "p=tls-server-end-point,,", 22)` copies only 22 of the 24 bytes, dropping the final comma, so the `c=` value in client-final becomes `p=tls-server-end-point,<digest>` (one comma short). The server computes its expected channel-binding data as base64("p=tls-server-end-point,," || digest), so this forces a c=/proof mismatch and every -PLUS attempt fails and falls back to libpq. Fix the constant to 24 and update the buffer sizing and tests that derive from it.</violation>
<violation number="3" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P2: The cbind prefix length is wrong throughout this spec. `"p=tls-server-end-point,,"` is 24 bytes, not 22. The memcpy in §3.2 copies only 22 bytes of the literal (dropping the final comma), producing a malformed SCRAM channel-binding header that will fail server-side `c=`/proof verification and force a libpq fallback whenever `-PLUS` is selected. The derived buffer sizes are also wrong: `22 + 64 = 86` should be `24 + 64 = 88`, so the §6 claim that an 86-byte buffer covers the 64-byte (SHA-512) worst case is incorrect — an implementer following the doc would under-size `cbind_input[86]` (the §3.9 call site). Update every length/constant in one pass: 22→24 for the prefix, 54→56 and 86→88 for the buffer sizes and Test 11/12 pinned lengths.</violation>
</file>
<file name="common_mk/openssl_flags.mk">
<violation number="1" location="common_mk/openssl_flags.mk:43">
P2: This file is explicitly documented as a local build workaround that must not be committed. Remove the added OpenSSL-selection changes from the PR so shared builds do not inherit this machine-dependent library selection.</violation>
</file>
<file name="include/PgSQL_Connection.h">
<violation number="1" location="include/PgSQL_Connection.h:499">
P2: For PostgreSQL 10 and newer, this produces a different value from `PQserverVersion` (`16.2` becomes `160200` instead of `160002`). Preserve the pre-10 three-component encoding only for major versions below 10.</violation>
</file>
<file name="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md">
<violation number="1" location="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md:275">
P1: The CopyFail safety net in Task 2 Step 2 sends CopyFail via `native_send_or_buffer(PG_Native_Conn_St::DONE)` and then immediately `continue`s into the read loop. On a non-blocking partial write, the CopyFail bytes remain queued in `native_outbuf`, and the next `FRAME_NEED_MORE` sets `async_exit_status` to PG_EVENT_READ, overwriting the pending POLLOUT; the backend is still waiting for the CopyFail and will never send the ErrorResponse/ReadyForQuery the drive is reading for, so the connection hangs. The already-implemented Sync-injection recovery in `native_fetch_result_cont` handles exactly this case by returning after the send when `async_exit_status == PG_EVENT_WRITE || !native_outbuf.empty() || !native_ssl_outbuf.empty()` (lib/PgSQL_Connection.cpp:2887-2894). Mirror that guard for the CopyFail send.</violation>
</file>
<file name="lib/PgSQL_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Protocol.cpp:2831">
P2: When a native simple query contains `UPDATE ...; SELECT ...`, this leaves `affected_rows` set to the UPDATE count because the later SELECT command is skipped after tuple data appears. Track command boundaries and report affected rows for the final statement, matching libpq's per-result handling.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:318">
P2: The framer accepts any backend-supplied message length without an upper bound. next() only rejects msglen < 4; for any larger value it returns FRAME_NEED_MORE while feed() keeps reallocating the internal buffer to fit every byte the peer sends. A broken or compromised backend that declares a multi-GB length (uint32, up to ~4GB) then streams bytes makes the connection grow its buffer without bound — a memory-exhaustion/DoS vector on the backend-facing decoder. Cap msglen at a reasonable maximum and return FRAME_ERROR once it is exceeded.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:322">
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</violation>
</file>
<file name="lib/PgSQL_Session.cpp">
<violation number="1" location="lib/PgSQL_Session.cpp:3666">
P2: PROCESSING_STMT_CLOSE's rc0 epilogue erases the registry entry (destroying its unique_ptr<PgSQL_Bind_Message> and releasing the shared_ptr stmt) before RequestEnd()/CurrentQuery.end() runs. handle_post_sync_close_message set extended_query_info.stmt_client_name and stmt_client_portal_name to point into that freed bind_msg / the cleared string, so they dangle for the remainder of the frame. This contradicts the A1 fix pattern documented in the same cycle boundary (clear_named_portals() must be deferred until after RequestEnd() because the event log reads those pointers). Evict the entry only after RequestEnd(), or drop the assignment of stmt_client_name/stmt_client_portal_name from freed storage.</violation>
<violation number="2" location="lib/PgSQL_Session.cpp:7442">
P2: When a named Bind returns `rc == -1`, `reset_extended_query_frame()` and `RequestEnd()` leave this active pending entry intact; only success or session reset releases it. Clear `pending_named_bind` on the error path after `RequestEnd()` so failed binds do not retain the raw packet and statement reference.</violation>
</file>
<file name="test/tap/tests/pgsql-native_notify-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_notify-t.cpp:254">
P2: The `if (!result_match && lp_nvs.size() == nt_nvs.size()) result_match = true;` override masks genuine payload/channel byte mismatches, not just the known both-zero case. When both paths receive the expected count but with different payload bytes or channel names, the payload loop sets `result_match = false`, then this override re-enables it, so the differential test reports a pass for exactly the byte-parity regression it exists to catch. The comment says the override is for when "either both 0 or both correct," but the size-only condition also passes "both wrong the same way." Restrict the override to the both-dropped-same-count case (sizes equal but different from `n_notifies`); never suppress a payload mismatch.</violation>
</file>
<file name="test/tap/tests/pgsql-native_stress-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_stress-t.cpp:155">
P2: `plan(4)` assumes exactly one record per scenario plus the coverage summary, but the S0 (and other) branches record an extra `OpRecord` on failure paths: for example when the libpq connection fails to open, S0 records "libpq conn failed" (result_match=false) and then the native branch records a second record, so S0 alone can emit two lines. Combined with S1/S2 and the summary that exceeds the planned 4 tests, producing a TAP "planned 4 but ran more" error. Make the record emission one-per-scenario regardless of the failure path (e.g. build the digest and fell_back flags first, then record once), or update the plan.</violation>
<violation number="2" location="test/tap/tests/pgsql-native_stress-t.cpp:196">
P2: In S0 both digests are built as `lp_dig += std::to_string(i) + ":ok|"` / `nt_dig += ... ":ok|"` with no dependence on the PREPARE/EXECUTE/DEALLOCATE results (each `PQexec` return is discarded via `PQclear`). The digests are therefore structurally identical, so `result_match = (lp_dig == nt_dig)` is always true and the S0 `ok()` assertion can never fail. Worse, `if (!lp)` / `if (!nt)` only test the raw PGconn pointer, not `PQstatus(...) != CONNECTION_OK` (the sibling pgsql-native_auth_differential-t.cpp checks both), so a native connection that fails to authenticate still yields a full ":ok|" digest and is reported as full native parity and coverage. This makes the S0 case give false assurance exactly where the PR claims it verifies the prepared-statement cycle. Check the connection status and make the digest reflect each statement's result so a native failure actually breaks `result_match`.</violation>
</file>
<file name="test/tap/tests/pgsql-native_prepared-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_prepared-t.cpp:390">
P2: The `ExtQCase` fields `expect_error`, `expect_sqlstate`, `describe_after_bind`, and `close_portal` are set by the P10–P20 cases but never read anywhere: `run_extq_cycle()`/`run_extq()` only consume `stmt_name`, `query`, `param_types`, `bind_steps`, and `close_stmt`. The documented "assert exact SQLSTATE on error" (e.g. P16=42601, P17=22012) never runs, so the error-path cases are guarded only by libpq-vs-native byte equality, which cannot catch both sides reporting the same wrong SQLSTATE — unlike the midframe case, which does assert `sqlstate=42601` explicitly. Either enforce `expect_sqlstate` in `run_extq_cycle()` or drop the misleading fields and case args.</violation>
</file>
<file name="lib/PgSQL_HostGroups_Manager.cpp">
<violation number="1" location="lib/PgSQL_HostGroups_Manager.cpp:3083">
P2: Native free-connection stats expose the raw ReadyForQuery byte (`I`/`T`/`E`), unlike the existing descriptive `transaction_status` values. Use `get_pg_transaction_status_str()` so native and libpq stats preserve the same output contract.</violation>
</file>
<file name="test/tap/tests/pgsql-native_cancel-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_cancel-t.cpp:176">
P2: `drainLogToNow()` does not advance the log stream, so the phase isolation it is meant to provide never happens. It calls `get_matching_lines(f_proxysql_log, "__no_such_marker_line__")`, but `get_matching_lines` (tap/utils.cpp) reads to EOF and then, because this regex never matches, executes `f_stream.seekg(init_pos)` — rewinding to the position at the start of the call. Net effect: the get-pointer is unchanged. Consequently `scanNativePhaseLog()` in the native phase starts at the offset set by `open_file_and_seek_end` and scans this test's own libpq-phase logs in addition to the native phase, so the tripwire/positive-evidence scan is not restricted to the native phase as the comments here and the combined-scan reasoning claim. Rewrite `drainLogToNow()` to read and discard lines in a forward loop (leaving the pointer at EOF) instead of calling `get_matching_lines`; otherwise the fallback/`Canceled query (native)` checks can observe stale pre-native-phase content.</violation>
</file>
<file name="test/tap/tests/pgsql-native_transactions-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_transactions-t.cpp:17">
P2: This test is registered to run in CI group `legacy-g1` (test/tap/groups/groups.json line 188), but the file's own header comment states that T1/T3/T5/T6/T7/T11/T13/T14 "report a real divergence and emit 'not ok'" because the native `PgSQL_ExplicitTxnStateMgr` is not kept in sync. Each not-ok is asserted via `cov.emit_tap()` `ok(r.result_match, ...)`, so a single divergent case fails the whole test and thus the legacy-g1 suite. This contradicts the PR's claim that legacy-g1 is green. Either the txn-tracking bugs are still present (the test will fail CI on every run) or they were fixed and these comments/assertions are stale. Resolve which is true: fix the native path so all cases pass, or handle the known-failing cases (e.g. xfail/skip) before landing, and remove the stale Known-Issues notes if they no longer apply.</violation>
</file>
<file name="test/tap/tests/pgsql-native_auth_differential-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_auth_differential-t.cpp:331">
P2: The first regex alternative and the header claim a query-path fallback message "native_mode requested but unimplemented at this stage; falling back to libpq" emitted by PgSQL_Connection::query_cont/fetch_result_cont. That string does not exist anywhere in the current tree — the only fallback log line is "native backend auth capability gap (%s) ... falling back to libpq" at lib/PgSQL_Connection.cpp:1482 (native_capability_gap). Since the native query path is now fully wired in this PR and logs no fallback, the "used native path" assertion would silently pass even if a query-path fallback were reintroduced with a different (or no) message. Drop the dead alternative, or make the capability-gap check the sole signal, and correct the header so the assertion's guarantee matches reality.</violation>
</file>
<file name="test/tap/tests/pgsql-native_streaming-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_streaming-t.cpp:90">
P2: This line allocates an EVP_MD_CTX with EVP_MD_CTX_new() only to evaluate a always-true ternary that yields "", and never frees that context — a leak and a no-op. col_hashes is fully overwritten later in the finalize loop, so the whole statement is dead. Remove it and initialize the vector directly (e.g. `fp.col_hashes.assign(fp.ncols, "");`).</violation>
</file>
<file name="lib/PgSQL_Logger.cpp">
<violation number="1" location="lib/PgSQL_Logger.cpp:1039">
P2: When a named-portal Close is logged, this new case derives `query_digest` from parser state that the Close processing does not populate. The event can therefore carry a previous or zero digest; set the Close digest explicitly, typically to zero, before constructing `PgSQL_Event`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:396">
P2: `run_case` returns early (without calling `cov.record`) when the libpq control or native connection fails to open. `main` still expects 15 case records plus the summary (plan(16)), so each early-returned case silently shrinks the ok-count and the TAP run fails with a "planned 16 but ran N" mismatch, plus the failure is un-attributed. Record a failing OpRecord (result_match=false, native_path_used=false, detail=connect error) before every early return so the plan count stays stable and diagnostics point at the failed case.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:1195">
P2: Task 4's extended-query runner feeds `PQsendPrepare` a `const char* paramTypes[16]` filled with parameter-type *names* as C strings, but libpq's `PQsendPrepare(PGconn*, const char*, const char*, int, const Oid*)` takes a `const Oid*` array of numeric type OIDs. This won't compile, and even if coerced, type-name strings are not OIDs (see lib/PgSQL_Connection.cpp:3543 which passes `parse_param_types.data()` where `Parse_Param_Types` is a vector of Oid). Convert the parameter types to `Oid` values (with text/binary awareness via `PQexecParams`-style `uint`/`Oid` array) before calling PQsendPrepare.</violation>
</file>
<file name="test/tap/tests/unit/Makefile">
<violation number="1" location="test/tap/tests/unit/Makefile:406">
P2: `pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</violation>
</file>
<file name="lib/PgSQL_PreparedStatement.cpp">
<violation number="1" location="lib/PgSQL_PreparedStatement.cpp:110">
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</violation>
</file>
<file name="test/tap/tests/pg_lite_client.cpp">
<violation number="1" location="test/tap/tests/pg_lite_client.cpp:330">
P3: The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</violation>
</file>
<file name="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md:218">
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * type. The PostgreSQL convention is an empty SCRAM username (the | ||
| * real username travels in the StartupMessage), so the header is | ||
| * "p=tls-server-end-point,,". */ | ||
| snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce); |
There was a problem hiding this comment.
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling snprintf.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deps/libscram/src/scram.c, line 514:
<comment>When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</comment>
<file context>
@@ -503,7 +506,15 @@ char *build_client_first_message(ScramState *scram_state)
+ * type. The PostgreSQL convention is an empty SCRAM username (the
+ * real username travels in the StartupMessage), so the header is
+ * "p=tls-server-end-point,,". */
+ snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce);
+ } else {
+ snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);
</file context>
|
|
||
| - [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)** | ||
|
|
||
| The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header). |
There was a problem hiding this comment.
P1: Task 3 changes the gs2 header to the 22-byte p=tls-server-end-point,, prefix but leaves client_first_message_bare = strdup(result + 3), which strips only 3 bytes. With cbind set, the stored bare form becomes tls-server-end-point,,n=,r=... instead of the required n=,r=.... Both calculate_client_proof and verify_server_signature fold client_first_message_bare into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 334:
<comment>Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+- [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)**
+
+The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header).
+
+- [ ] **Step 2: Change the gs2 header to honor cbind**
</file context>
|
|
||
| ```cpp | ||
| static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,"; | ||
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; |
There was a problem hiding this comment.
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22 and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's cbind_input[86] claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 787:
<comment>The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+```cpp
+static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,";
+static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22;
+
+int pg_scram_build_cbind_input_tls_server_end_point(
</file context>
Suggested change
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; | |
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 24; // "p=tls-server-end-point,," is 24 bytes (RFC 5802) |
| if (s == nullptr || s->st == nullptr) return nullptr; | ||
| // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does | ||
| // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header). | ||
| if (channel_binding) return nullptr; |
There was a problem hiding this comment.
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, pg_scram_client_first always returns nullptr despite the cbind input already being configured. Honor the configured cbind state for the true case so native SCRAM-PLUS can send its p=tls-server-end-point,, client-first message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Backend_Auth.cpp, line 125:
<comment>When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</comment>
<file context>
@@ -0,0 +1,226 @@
+ if (s == nullptr || s->st == nullptr) return nullptr;
+ // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does
+ // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header).
+ if (channel_binding) return nullptr;
+ scram_reset_error();
+ // libscram emits "n,,n=,r=<nonce>" and stashes client_nonce / client_first_message_bare
</file context>
| // gs2 header is "n,," (no channel binding) and the username field is empty ("n="), | ||
| // matching the PostgreSQL convention where the real username travels in the startup | ||
| // packet. Returns the owned message string, or nullptr on error (see scram_error()). | ||
| // channel_binding=true is not supported by this task and returns nullptr. |
There was a problem hiding this comment.
P1: When a TLS backend offers SCRAM-SHA-256-PLUS, this API contract makes native authentication fail because native_drive_auth calls it with channel_binding=true and treats nullptr as an authentication failure. Implement the channel-bound client-first path before selecting -PLUS, or stop selecting -PLUS and fall back before invoking this API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/PgSQL_Backend_Protocol.h, line 106:
<comment>When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</comment>
<file context>
@@ -0,0 +1,180 @@
+// gs2 header is "n,," (no channel binding) and the username field is empty ("n="),
+// matching the PostgreSQL convention where the real username travels in the startup
+// packet. Returns the owned message string, or nullptr on error (see scram_error()).
+// channel_binding=true is not supported by this task and returns nullptr.
+const char* pg_scram_client_first(PgSQL_Scram_State* s, bool channel_binding);
+
</file context>
| UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ | ||
| protocol_unit-t auth_unit-t connection_pool_unit-t \ | ||
| rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ | ||
| pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \ |
There was a problem hiding this comment.
P2: pgsql_backend_extq-t and pgsql_stmt_meta_cache-t are added to UNIT_TESTS here, but only pgsql_backend_auth-t and pgsql_backend_framing-t were registered in groups.json under unit-tests-g1. Since run-tests-isolated.bash discovers a group's tests from groups.json, the unit-tests-g1 TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/Makefile, line 406:
<comment>`pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</comment>
<file context>
@@ -404,7 +403,9 @@ $(LIBPROXYSQLAR): FORCE
UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \
protocol_unit-t auth_unit-t connection_pool_unit-t \
rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \
+ pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \
pgsql_command_complete_unit-t \
+ pgsql_stmt_meta_cache-t \
</file context>
| size_t total = 1 + msglen; // type byte + length-prefixed body | ||
| if (len - pos < total) return FRAME_NEED_MORE; | ||
| out.type = (char)buf[pos]; | ||
| out.payload = buf + pos + 5; |
There was a problem hiding this comment.
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md, line 322:
<comment>PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</comment>
<file context>
@@ -0,0 +1,798 @@
+ size_t total = 1 + msglen; // type byte + length-prefixed body
+ if (len - pos < total) return FRAME_NEED_MORE;
+ out.type = (char)buf[pos];
+ out.payload = buf + pos + 5;
+ out.payload_len = msglen - 4;
+ pos += total;
</file context>
| // Set-once: install only while the slot is still empty. On success the slot now | ||
| // owns `candidate`. On failure another publish already won, so free our copy — | ||
| // the caller must not touch `candidate` after this returns either way. | ||
| if (describe_cache.compare_exchange_strong(expected, candidate, |
There was a problem hiding this comment.
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_PreparedStatement.cpp, line 110:
<comment>After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</comment>
<file context>
@@ -98,6 +98,21 @@ PgSQL_STMT_Global_info::~PgSQL_STMT_Global_info() {
+ // Set-once: install only while the slot is still empty. On success the slot now
+ // owns `candidate`. On failure another publish already won, so free our copy —
+ // the caller must not touch `candidate` after this returns either way.
+ if (describe_cache.compare_exchange_strong(expected, candidate,
+ std::memory_order_acq_rel, std::memory_order_acquire)) {
+ return true;
</file context>
| // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 -> | ||
| // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake | ||
| // failure cannot leak the libscram state. | ||
| PgSQL_Scram_State* scram = nullptr; |
There was a problem hiding this comment.
P3: The SCRAM guard added here is dead code and its comment is misleading. scram (a ProxySQL PgSQL_Scram_State*) is initialized to nullptr and never assigned anywhere in handleAuthentication, and doSASLAuth creates its own unrelated libscram state (ScramState* st = scram_state_init()), which it already frees manually on every exit path. So the guard's ~ScramGuard() body if (*s) pg_scram_free(*s) never executes (*s is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references pg_scram_free (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the pg_scram_* symbols, and PG_LITE_CLIENT_SCRAM is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole #ifdef PG_LITE_CLIENT_SCRAM ... #endif block (and, if desired, the now-unused PgSQL_Backend_Protocol.h include).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pg_lite_client.cpp, line 330:
<comment>The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</comment>
<file context>
@@ -315,6 +323,17 @@ void PgConnection::handleAuthentication(const std::string& password) {
+ // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 ->
+ // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake
+ // failure cannot leak the libscram state.
+ PgSQL_Scram_State* scram = nullptr;
+ struct ScramGuard {
+ PgSQL_Scram_State** s;
</file context>
| server-version-dependent strings. | ||
| - **Corpus.** Scalar/row/empty/error results; every data type in text and binary format; | ||
| multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without | ||
| channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer |
There was a problem hiding this comment.
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md, line 218:
<comment>§7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</comment>
<file context>
@@ -0,0 +1,253 @@
+ server-version-dependent strings.
+- **Corpus.** Scalar/row/empty/error results; every data type in text and binary format;
+ multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without
+ channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer
+ framing); mid-session `SET client_encoding`. Error cases compare parsed `ErrorResponse`
+ fields.
</file context>
There was a problem hiding this comment.
40 issues found across 52 files
Not reviewed (too large): lib/PgSQL_Connection.cpp (~2,345 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="deps/libscram/src/scram.c">
<violation number="1" location="deps/libscram/src/scram.c:514">
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:133">
P2: The pinned `expected_c_b64` literal in test 14 is invalid: it base64-decodes to the mangled 44-byte blob `p@es-server-end-point, \0...`, not to base64 of the gs2 header + SHA-256 digest (correct value begins `cD10bHMtc2VydmVyLWVuZC1wb2ludCws...`). A developer trusting this literal — or recomputing it per the plan's fallback instruction using the incorrect 22-byte header — gets a wrong assertion that masks rather than detects the channel-binding bug. Fix the header length first (see related finding), then pin the correct literal: base64 of the 24-byte header + 32-zero digest = `cD10bHMtc2VydmVyLWVuZC1wb2ludCws` + 56 `A`s.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:202">
P2: Test 15 uses `read_client_first_message` to validate the cbind path, but the vendored libscram server-side reader rejects cbind flag 'p' (`case 'p': ... "client requires SCRAM channel binding, but it is not supported"` and returns false). The plan never modifies that reader, so `parsed` is always false and the final assertion `ok(parsed && cbind_flag == 'p')` can never pass — the test is guaranteed to fail, contradicting the plan's "all 15 tests ok" expectation and the "round-trip through the independent libscram server-side verifier" claim. If the reader were ever patched to accept 'p', the `build_client_final_message(client, nullptr, server_first, nullptr, 0, 0)` call would then dereference NULL `credentials` in `calculate_client_proof` (`credentials->has_scram_keys` on a nullptr, and `credentials->passwd`), crashing the test. Test 15 needs a real server-side verifier path (or to be dropped) rather than this 'p'-rejecting parse smoke.</violation>
<violation number="3" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:334">
P1: Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</violation>
<violation number="4" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:526">
P2: Tests 8 and 9 build a dummy SSL with `SSL_CTX_use_certificate` + `SSL_new` and expect `pg_tls_server_end_point` (via `SSL_get_peer_certificate`) to return the cert digest. `SSL_get_peer_certificate` returns the peer's certificate only after a completed TLS handshake; a client SSL that never handshakes has no peer cert, so it returns NULL and `pg_tls_server_end_point` returns -1. Both tests would fail on `rc >= 0` and cannot validate the digest logic against a fake, non-handshaked SSL. The digest helper should be tested against the X509 directly (or behind a real handshake fixture), not through SSL_get_peer_certificate on a dummy SSL.</violation>
<violation number="5" location="docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md:787">
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</violation>
</file>
<file name="lib/PgSQL_Backend_Auth.cpp">
<violation number="1" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</violation>
<violation number="2" location="lib/PgSQL_Backend_Auth.cpp:125">
P1: When a backend offers SCRAM-SHA-256-PLUS over TLS, `use_scram_plus` is true and `pg_scram_client_first(native_scram, true)` returns nullptr from the `if (channel_binding) return nullptr;` guard, so the native connection fails with 'SCRAM client-first failed' instead of completing channel-bound auth. libscram's `build_client_first_message` already emits the `p=tls-server-end-point,,` header based on the cbind input installed by `pg_scram_set_cbind`, so the `channel_binding` bool is redundant and the early return is harmful. Remove the guard and drive the header from the cbind state.</violation>
<violation number="3" location="lib/PgSQL_Backend_Auth.cpp:157">
P2: When a backend password is 2047 bytes or longer, this copy truncates it silently before SCRAM derives the proof. Preserve the complete password for SCRAM, or detect this case and route the connection through the libpq fallback instead of attempting authentication with a different secret.</violation>
</file>
<file name="include/PgSQL_Backend_Protocol.h">
<violation number="1" location="include/PgSQL_Backend_Protocol.h:106">
P1: When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</violation>
</file>
<file name="include/PgSQL_PreparedStatement.h">
<violation number="1" location="include/PgSQL_PreparedStatement.h:35">
P2: After a Describe cache is populated, prepared-statement memory usage under-reports the cache's payload allocations. Add the cached string capacities to `total_mem_usage` or otherwise include them in `get_memory_usage()` so the admin metric remains accurate.</violation>
<violation number="2" location="include/PgSQL_PreparedStatement.h:88">
P1: When a client changes `search_path` or uses a schema/temp object that changes name resolution, this global set-once cache can return the previous session's `RowDescription` without contacting PostgreSQL. The documented DDL-staleness trade-off does not cover these session-state changes; include the relevant state in the cache identity or invalidate/bypass the cache whenever it changes.</violation>
</file>
<file name="lib/PgSQL_Backend_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Backend_Protocol.cpp:13">
P1: When receive boundaries repeatedly leave a partial trailing backend frame, `PgSQL_Backend_Msg_Framer` retains consumed prefixes and grows based on total received bytes, not buffered bytes. Compact `buf + pos` before reallocating so long-running streamed results cannot consume unbounded memory despite each message staying below `PGSQL_MAX_BACKEND_MSG_LEN`.</violation>
</file>
<file name="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:62">
P1: The buffer sizes and unit-test expectations derived from the wrong 22-byte prefix must also change: §3.2 says out_cap >= "22 + 64 = 86" and "22 + 32 = 54", and §3.9 allocates `unsigned char cbind_input[86]`. With the correct 24-byte prefix, SHA-512-signed certs need 24+64=88 bytes, so the 86-byte buffer is too small: `pg_scram_build_cbind_input_tls_server_end_point` returns -1 and the code hits `assert(0)`/teardown, so a valid -PLUS connection with a SHA-512-signed backend cert cannot complete. Tests 11/12 (54/86-byte buffers) and Test 13's `22+digest_len` are likewise off by two.</violation>
<violation number="2" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P1: The gs2-cbind prefix `"p=tls-server-end-point,,"` is 24 bytes (p= + 20-byte `tls-server-end-point` + two commas), not 22. §3.2's `memcpy(out, "p=tls-server-end-point,,", 22)` copies only 22 of the 24 bytes, dropping the final comma, so the `c=` value in client-final becomes `p=tls-server-end-point,<digest>` (one comma short). The server computes its expected channel-binding data as base64("p=tls-server-end-point,," || digest), so this forces a c=/proof mismatch and every -PLUS attempt fails and falls back to libpq. Fix the constant to 24 and update the buffer sizing and tests that derive from it.</violation>
<violation number="3" location="docs/superpowers/specs/2026-06-14-pgsql-native-scram-plus-design.md:71">
P2: The cbind prefix length is wrong throughout this spec. `"p=tls-server-end-point,,"` is 24 bytes, not 22. The memcpy in §3.2 copies only 22 bytes of the literal (dropping the final comma), producing a malformed SCRAM channel-binding header that will fail server-side `c=`/proof verification and force a libpq fallback whenever `-PLUS` is selected. The derived buffer sizes are also wrong: `22 + 64 = 86` should be `24 + 64 = 88`, so the §6 claim that an 86-byte buffer covers the 64-byte (SHA-512) worst case is incorrect — an implementer following the doc would under-size `cbind_input[86]` (the §3.9 call site). Update every length/constant in one pass: 22→24 for the prefix, 54→56 and 86→88 for the buffer sizes and Test 11/12 pinned lengths.</violation>
</file>
<file name="common_mk/openssl_flags.mk">
<violation number="1" location="common_mk/openssl_flags.mk:43">
P2: This file is explicitly documented as a local build workaround that must not be committed. Remove the added OpenSSL-selection changes from the PR so shared builds do not inherit this machine-dependent library selection.</violation>
</file>
<file name="include/PgSQL_Connection.h">
<violation number="1" location="include/PgSQL_Connection.h:499">
P2: For PostgreSQL 10 and newer, this produces a different value from `PQserverVersion` (`16.2` becomes `160200` instead of `160002`). Preserve the pre-10 three-component encoding only for major versions below 10.</violation>
</file>
<file name="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md">
<violation number="1" location="docs/superpowers/plans/2026-07-07-pgsql-native-copy-harden-extq-wiring.md:275">
P1: The CopyFail safety net in Task 2 Step 2 sends CopyFail via `native_send_or_buffer(PG_Native_Conn_St::DONE)` and then immediately `continue`s into the read loop. On a non-blocking partial write, the CopyFail bytes remain queued in `native_outbuf`, and the next `FRAME_NEED_MORE` sets `async_exit_status` to PG_EVENT_READ, overwriting the pending POLLOUT; the backend is still waiting for the CopyFail and will never send the ErrorResponse/ReadyForQuery the drive is reading for, so the connection hangs. The already-implemented Sync-injection recovery in `native_fetch_result_cont` handles exactly this case by returning after the send when `async_exit_status == PG_EVENT_WRITE || !native_outbuf.empty() || !native_ssl_outbuf.empty()` (lib/PgSQL_Connection.cpp:2887-2894). Mirror that guard for the CopyFail send.</violation>
</file>
<file name="lib/PgSQL_Protocol.cpp">
<violation number="1" location="lib/PgSQL_Protocol.cpp:2831">
P2: When a native simple query contains `UPDATE ...; SELECT ...`, this leaves `affected_rows` set to the UPDATE count because the later SELECT command is skipped after tuple data appears. Track command boundaries and report affected rows for the final statement, matching libpq's per-result handling.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:318">
P2: The framer accepts any backend-supplied message length without an upper bound. next() only rejects msglen < 4; for any larger value it returns FRAME_NEED_MORE while feed() keeps reallocating the internal buffer to fit every byte the peer sends. A broken or compromised backend that declares a multi-GB length (uint32, up to ~4GB) then streams bytes makes the connection grow its buffer without bound — a memory-exhaustion/DoS vector on the backend-facing decoder. Cap msglen at a reasonable maximum and return FRAME_ERROR once it is exceeded.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md:322">
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</violation>
</file>
<file name="lib/PgSQL_Session.cpp">
<violation number="1" location="lib/PgSQL_Session.cpp:3666">
P2: PROCESSING_STMT_CLOSE's rc0 epilogue erases the registry entry (destroying its unique_ptr<PgSQL_Bind_Message> and releasing the shared_ptr stmt) before RequestEnd()/CurrentQuery.end() runs. handle_post_sync_close_message set extended_query_info.stmt_client_name and stmt_client_portal_name to point into that freed bind_msg / the cleared string, so they dangle for the remainder of the frame. This contradicts the A1 fix pattern documented in the same cycle boundary (clear_named_portals() must be deferred until after RequestEnd() because the event log reads those pointers). Evict the entry only after RequestEnd(), or drop the assignment of stmt_client_name/stmt_client_portal_name from freed storage.</violation>
<violation number="2" location="lib/PgSQL_Session.cpp:7442">
P2: When a named Bind returns `rc == -1`, `reset_extended_query_frame()` and `RequestEnd()` leave this active pending entry intact; only success or session reset releases it. Clear `pending_named_bind` on the error path after `RequestEnd()` so failed binds do not retain the raw packet and statement reference.</violation>
</file>
<file name="test/tap/tests/pgsql-native_notify-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_notify-t.cpp:254">
P2: The `if (!result_match && lp_nvs.size() == nt_nvs.size()) result_match = true;` override masks genuine payload/channel byte mismatches, not just the known both-zero case. When both paths receive the expected count but with different payload bytes or channel names, the payload loop sets `result_match = false`, then this override re-enables it, so the differential test reports a pass for exactly the byte-parity regression it exists to catch. The comment says the override is for when "either both 0 or both correct," but the size-only condition also passes "both wrong the same way." Restrict the override to the both-dropped-same-count case (sizes equal but different from `n_notifies`); never suppress a payload mismatch.</violation>
</file>
<file name="test/tap/tests/pgsql-native_stress-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_stress-t.cpp:155">
P2: `plan(4)` assumes exactly one record per scenario plus the coverage summary, but the S0 (and other) branches record an extra `OpRecord` on failure paths: for example when the libpq connection fails to open, S0 records "libpq conn failed" (result_match=false) and then the native branch records a second record, so S0 alone can emit two lines. Combined with S1/S2 and the summary that exceeds the planned 4 tests, producing a TAP "planned 4 but ran more" error. Make the record emission one-per-scenario regardless of the failure path (e.g. build the digest and fell_back flags first, then record once), or update the plan.</violation>
<violation number="2" location="test/tap/tests/pgsql-native_stress-t.cpp:196">
P2: In S0 both digests are built as `lp_dig += std::to_string(i) + ":ok|"` / `nt_dig += ... ":ok|"` with no dependence on the PREPARE/EXECUTE/DEALLOCATE results (each `PQexec` return is discarded via `PQclear`). The digests are therefore structurally identical, so `result_match = (lp_dig == nt_dig)` is always true and the S0 `ok()` assertion can never fail. Worse, `if (!lp)` / `if (!nt)` only test the raw PGconn pointer, not `PQstatus(...) != CONNECTION_OK` (the sibling pgsql-native_auth_differential-t.cpp checks both), so a native connection that fails to authenticate still yields a full ":ok|" digest and is reported as full native parity and coverage. This makes the S0 case give false assurance exactly where the PR claims it verifies the prepared-statement cycle. Check the connection status and make the digest reflect each statement's result so a native failure actually breaks `result_match`.</violation>
</file>
<file name="test/tap/tests/pgsql-native_prepared-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_prepared-t.cpp:390">
P2: The `ExtQCase` fields `expect_error`, `expect_sqlstate`, `describe_after_bind`, and `close_portal` are set by the P10–P20 cases but never read anywhere: `run_extq_cycle()`/`run_extq()` only consume `stmt_name`, `query`, `param_types`, `bind_steps`, and `close_stmt`. The documented "assert exact SQLSTATE on error" (e.g. P16=42601, P17=22012) never runs, so the error-path cases are guarded only by libpq-vs-native byte equality, which cannot catch both sides reporting the same wrong SQLSTATE — unlike the midframe case, which does assert `sqlstate=42601` explicitly. Either enforce `expect_sqlstate` in `run_extq_cycle()` or drop the misleading fields and case args.</violation>
</file>
<file name="lib/PgSQL_HostGroups_Manager.cpp">
<violation number="1" location="lib/PgSQL_HostGroups_Manager.cpp:3083">
P2: Native free-connection stats expose the raw ReadyForQuery byte (`I`/`T`/`E`), unlike the existing descriptive `transaction_status` values. Use `get_pg_transaction_status_str()` so native and libpq stats preserve the same output contract.</violation>
</file>
<file name="test/tap/tests/pgsql-native_cancel-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_cancel-t.cpp:176">
P2: `drainLogToNow()` does not advance the log stream, so the phase isolation it is meant to provide never happens. It calls `get_matching_lines(f_proxysql_log, "__no_such_marker_line__")`, but `get_matching_lines` (tap/utils.cpp) reads to EOF and then, because this regex never matches, executes `f_stream.seekg(init_pos)` — rewinding to the position at the start of the call. Net effect: the get-pointer is unchanged. Consequently `scanNativePhaseLog()` in the native phase starts at the offset set by `open_file_and_seek_end` and scans this test's own libpq-phase logs in addition to the native phase, so the tripwire/positive-evidence scan is not restricted to the native phase as the comments here and the combined-scan reasoning claim. Rewrite `drainLogToNow()` to read and discard lines in a forward loop (leaving the pointer at EOF) instead of calling `get_matching_lines`; otherwise the fallback/`Canceled query (native)` checks can observe stale pre-native-phase content.</violation>
</file>
<file name="test/tap/tests/pgsql-native_transactions-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_transactions-t.cpp:17">
P2: This test is registered to run in CI group `legacy-g1` (test/tap/groups/groups.json line 188), but the file's own header comment states that T1/T3/T5/T6/T7/T11/T13/T14 "report a real divergence and emit 'not ok'" because the native `PgSQL_ExplicitTxnStateMgr` is not kept in sync. Each not-ok is asserted via `cov.emit_tap()` `ok(r.result_match, ...)`, so a single divergent case fails the whole test and thus the legacy-g1 suite. This contradicts the PR's claim that legacy-g1 is green. Either the txn-tracking bugs are still present (the test will fail CI on every run) or they were fixed and these comments/assertions are stale. Resolve which is true: fix the native path so all cases pass, or handle the known-failing cases (e.g. xfail/skip) before landing, and remove the stale Known-Issues notes if they no longer apply.</violation>
</file>
<file name="test/tap/tests/pgsql-native_auth_differential-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_auth_differential-t.cpp:331">
P2: The first regex alternative and the header claim a query-path fallback message "native_mode requested but unimplemented at this stage; falling back to libpq" emitted by PgSQL_Connection::query_cont/fetch_result_cont. That string does not exist anywhere in the current tree — the only fallback log line is "native backend auth capability gap (%s) ... falling back to libpq" at lib/PgSQL_Connection.cpp:1482 (native_capability_gap). Since the native query path is now fully wired in this PR and logs no fallback, the "used native path" assertion would silently pass even if a query-path fallback were reintroduced with a different (or no) message. Drop the dead alternative, or make the capability-gap check the sole signal, and correct the header so the assertion's guarantee matches reality.</violation>
</file>
<file name="test/tap/tests/pgsql-native_streaming-t.cpp">
<violation number="1" location="test/tap/tests/pgsql-native_streaming-t.cpp:90">
P2: This line allocates an EVP_MD_CTX with EVP_MD_CTX_new() only to evaluate a always-true ternary that yields "", and never frees that context — a leak and a no-op. col_hashes is fully overwritten later in the finalize loop, so the whole statement is dead. Remove it and initialize the vector directly (e.g. `fp.col_hashes.assign(fp.ncols, "");`).</violation>
</file>
<file name="lib/PgSQL_Logger.cpp">
<violation number="1" location="lib/PgSQL_Logger.cpp:1039">
P2: When a named-portal Close is logged, this new case derives `query_digest` from parser state that the Close processing does not populate. The event can therefore carry a previous or zero digest; set the Close digest explicitly, typically to zero, before constructing `PgSQL_Event`.</violation>
</file>
<file name="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md">
<violation number="1" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:396">
P2: `run_case` returns early (without calling `cov.record`) when the libpq control or native connection fails to open. `main` still expects 15 case records plus the summary (plan(16)), so each early-returned case silently shrinks the ok-count and the TAP run fails with a "planned 16 but ran N" mismatch, plus the failure is un-attributed. Record a failing OpRecord (result_match=false, native_path_used=false, detail=connect error) before every early return so the plan count stays stable and diagnostics point at the failed case.</violation>
<violation number="2" location="docs/superpowers/plans/2026-06-14-pgsql-native-txn-copy-prepared-pr1.md:1195">
P2: Task 4's extended-query runner feeds `PQsendPrepare` a `const char* paramTypes[16]` filled with parameter-type *names* as C strings, but libpq's `PQsendPrepare(PGconn*, const char*, const char*, int, const Oid*)` takes a `const Oid*` array of numeric type OIDs. This won't compile, and even if coerced, type-name strings are not OIDs (see lib/PgSQL_Connection.cpp:3543 which passes `parse_param_types.data()` where `Parse_Param_Types` is a vector of Oid). Convert the parameter types to `Oid` values (with text/binary awareness via `PQexecParams`-style `uint`/`Oid` array) before calling PQsendPrepare.</violation>
</file>
<file name="test/tap/tests/unit/Makefile">
<violation number="1" location="test/tap/tests/unit/Makefile:406">
P2: `pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</violation>
</file>
<file name="lib/PgSQL_PreparedStatement.cpp">
<violation number="1" location="lib/PgSQL_PreparedStatement.cpp:110">
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</violation>
</file>
<file name="test/tap/tests/pg_lite_client.cpp">
<violation number="1" location="test/tap/tests/pg_lite_client.cpp:330">
P3: The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</violation>
</file>
<file name="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md">
<violation number="1" location="docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md:218">
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * type. The PostgreSQL convention is an empty SCRAM username (the | ||
| * real username travels in the StartupMessage), so the header is | ||
| * "p=tls-server-end-point,,". */ | ||
| snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce); |
There was a problem hiding this comment.
P1: When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling snprintf.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deps/libscram/src/scram.c, line 514:
<comment>When channel binding is enabled, this call writes into a buffer sized for plain SCRAM and truncates the client-first nonce. Size the result from the complete channel-bound format before calling `snprintf`.</comment>
<file context>
@@ -503,7 +506,15 @@ char *build_client_first_message(ScramState *scram_state)
+ * type. The PostgreSQL convention is an empty SCRAM username (the
+ * real username travels in the StartupMessage), so the header is
+ * "p=tls-server-end-point,,". */
+ snprintf(result, len, "p=tls-server-end-point,,n=,r=%s", scram_state->client_nonce);
+ } else {
+ snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);
</file context>
|
|
||
| - [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)** | ||
|
|
||
| The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header). |
There was a problem hiding this comment.
P1: Task 3 changes the gs2 header to the 22-byte p=tls-server-end-point,, prefix but leaves client_first_message_bare = strdup(result + 3), which strips only 3 bytes. With cbind set, the stored bare form becomes tls-server-end-point,,n=,r=... instead of the required n=,r=.... Both calculate_client_proof and verify_server_signature fold client_first_message_bare into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 334:
<comment>Task 3 changes the gs2 header to the 22-byte `p=tls-server-end-point,,` prefix but leaves `client_first_message_bare = strdup(result + 3)`, which strips only 3 bytes. With cbind set, the stored bare form becomes `tls-server-end-point,,n=,r=...` instead of the required `n=,r=...`. Both `calculate_client_proof` and `verify_server_signature` fold `client_first_message_bare` into the AuthMessage HMAC, while the server derives its bare form by stripping the full gs2 header after parsing the client-first. The resulting proof/signature inputs won't match, so SCRAM-SHA-256-PLUS native auth would fail at runtime. None of the planned tests catch it because test 14 overrides bare manually. Fix Task 3 to also strip the cbind header length when cbind is set.</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+- [ ] **Step 1: Read the existing `build_client_first_message` body (lines 481–521)**
+
+The function emits `n,,n=,r=<nonce>` at line 506 (`snprintf(result, len, "n,,n=,r=%s", scram_state->client_nonce);`). It also sets `client_first_message_bare = strdup(result + 3);` at line 508 (the bare form drops the `n,,` gs2 header).
+
+- [ ] **Step 2: Change the gs2 header to honor cbind**
</file context>
|
|
||
| ```cpp | ||
| static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,"; | ||
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; |
There was a problem hiding this comment.
P1: The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22 and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's cbind_input[86] claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-14-pgsql-native-scram-plus.md, line 787:
<comment>The gs2 channel-binding header "p=tls-server-end-point,," is 24 bytes, not 22. The plan's `PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22` and all 22-based sizing (cbind-input 54 instead of 56 bytes for SHA-256, Task 8's `cbind_input[86]` claimed sufficient for SHA-512's 24+64=88) are wrong, and Task 3's own test 13 already asserts the correct 24-byte length. Following the 22-byte constant drops the trailing ",," from cbind-input, producing an invalid SCRAM-SHA-256-PLUS that Postgres rejects. Use 24 for the header length and update every dependent size (54→56, 86→88, buffer capacity comments).</comment>
<file context>
@@ -0,0 +1,1144 @@
+
+```cpp
+static const char* const PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT = "p=tls-server-end-point,,";
+static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22;
+
+int pg_scram_build_cbind_input_tls_server_end_point(
</file context>
Suggested change
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 22; | |
| static const size_t PGSQL_CBIND_HEADER_TLS_SERVER_END_POINT_LEN = 24; // "p=tls-server-end-point,," is 24 bytes (RFC 5802) |
| if (s == nullptr || s->st == nullptr) return nullptr; | ||
| // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does | ||
| // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header). | ||
| if (channel_binding) return nullptr; |
There was a problem hiding this comment.
P1: When a TLS backend advertises SCRAM-SHA-256-PLUS, pg_scram_client_first always returns nullptr despite the cbind input already being configured. Honor the configured cbind state for the true case so native SCRAM-PLUS can send its p=tls-server-end-point,, client-first message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_Backend_Auth.cpp, line 125:
<comment>When a TLS backend advertises SCRAM-SHA-256-PLUS, `pg_scram_client_first` always returns `nullptr` despite the cbind input already being configured. Honor the configured cbind state for the `true` case so native SCRAM-PLUS can send its `p=tls-server-end-point,,` client-first message.</comment>
<file context>
@@ -0,0 +1,226 @@
+ if (s == nullptr || s->st == nullptr) return nullptr;
+ // Channel binding ('p'/'y' gs2 flag) is a separate task; this wrapper only does
+ // plain SCRAM-SHA-256 with gs2 flag 'n' ("n,," header).
+ if (channel_binding) return nullptr;
+ scram_reset_error();
+ // libscram emits "n,,n=,r=<nonce>" and stashes client_nonce / client_first_message_bare
</file context>
| // gs2 header is "n,," (no channel binding) and the username field is empty ("n="), | ||
| // matching the PostgreSQL convention where the real username travels in the startup | ||
| // packet. Returns the owned message string, or nullptr on error (see scram_error()). | ||
| // channel_binding=true is not supported by this task and returns nullptr. |
There was a problem hiding this comment.
P1: When a TLS backend offers SCRAM-SHA-256-PLUS, this API contract makes native authentication fail because native_drive_auth calls it with channel_binding=true and treats nullptr as an authentication failure. Implement the channel-bound client-first path before selecting -PLUS, or stop selecting -PLUS and fall back before invoking this API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At include/PgSQL_Backend_Protocol.h, line 106:
<comment>When a TLS backend offers `SCRAM-SHA-256-PLUS`, this API contract makes native authentication fail because `native_drive_auth` calls it with `channel_binding=true` and treats `nullptr` as an authentication failure. Implement the channel-bound client-first path before selecting `-PLUS`, or stop selecting `-PLUS` and fall back before invoking this API.</comment>
<file context>
@@ -0,0 +1,180 @@
+// gs2 header is "n,," (no channel binding) and the username field is empty ("n="),
+// matching the PostgreSQL convention where the real username travels in the startup
+// packet. Returns the owned message string, or nullptr on error (see scram_error()).
+// channel_binding=true is not supported by this task and returns nullptr.
+const char* pg_scram_client_first(PgSQL_Scram_State* s, bool channel_binding);
+
</file context>
| UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \ | ||
| protocol_unit-t auth_unit-t connection_pool_unit-t \ | ||
| rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \ | ||
| pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \ |
There was a problem hiding this comment.
P2: pgsql_backend_extq-t and pgsql_stmt_meta_cache-t are added to UNIT_TESTS here, but only pgsql_backend_auth-t and pgsql_backend_framing-t were registered in groups.json under unit-tests-g1. Since run-tests-isolated.bash discovers a group's tests from groups.json, the unit-tests-g1 TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/unit/Makefile, line 406:
<comment>`pgsql_backend_extq-t` and `pgsql_stmt_meta_cache-t` are added to `UNIT_TESTS` here, but only `pgsql_backend_auth-t` and `pgsql_backend_framing-t` were registered in `groups.json` under `unit-tests-g1`. Since `run-tests-isolated.bash` discovers a group's tests from `groups.json`, the `unit-tests-g1` TAP job will build but never run these two tests (only the ASAN-coverage workflow picks them up by listing the directory). The broken-extended-query and Describe-cache coverage this PR claims would silently be absent from the unit-tests-g1 run.</comment>
<file context>
@@ -404,7 +403,9 @@ $(LIBPROXYSQLAR): FORCE
UNIT_TESTS := smoke_test-t query_cache_unit-t query_processor_unit-t \
protocol_unit-t auth_unit-t connection_pool_unit-t \
rule_matching_unit-t hostgroups_unit-t monitor_health_unit-t \
+ pgsql_backend_framing-t pgsql_backend_auth-t pgsql_backend_extq-t \
pgsql_command_complete_unit-t \
+ pgsql_stmt_meta_cache-t \
</file context>
| size_t total = 1 + msglen; // type byte + length-prefixed body | ||
| if (len - pos < total) return FRAME_NEED_MORE; | ||
| out.type = (char)buf[pos]; | ||
| out.payload = buf + pos + 5; |
There was a problem hiding this comment.
P2: PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/plans/2026-06-11-pgsql-native-protocol-phase-0-1.md, line 322:
<comment>PgSQL_Backend_Msg::payload points into the framer's internal buffer, which is realloc'd on feed() and cleared once drained. Any consumer retaining m.payload across a subsequent feed()/next() reads dangling or clobbered data. The Task 1.6 post-auth handlers cache data (ParameterStatus name/value map, SCRAM server-first/server-final strings) and must copy out of the payload before more bytes are fed; the plan does not state this, so callers risk storing dangling pointers into bp's buffer. Document that returned payloads are valid only until the next feed(), and duplicate cached ParameterStatus and SCRAM inputs.</comment>
<file context>
@@ -0,0 +1,798 @@
+ size_t total = 1 + msglen; // type byte + length-prefixed body
+ if (len - pos < total) return FRAME_NEED_MORE;
+ out.type = (char)buf[pos];
+ out.payload = buf + pos + 5;
+ out.payload_len = msglen - 4;
+ pos += total;
</file context>
| // Set-once: install only while the slot is still empty. On success the slot now | ||
| // owns `candidate`. On failure another publish already won, so free our copy — | ||
| // the caller must not touch `candidate` after this returns either way. | ||
| if (describe_cache.compare_exchange_strong(expected, candidate, |
There was a problem hiding this comment.
P3: After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/PgSQL_PreparedStatement.cpp, line 110:
<comment>After a Describe cache is published, prepared-statement metadata memory statistics underreport the cache object and its payloads. Account for the cache allocation and stored payload sizes in the metadata-memory calculation.</comment>
<file context>
@@ -98,6 +98,21 @@ PgSQL_STMT_Global_info::~PgSQL_STMT_Global_info() {
+ // Set-once: install only while the slot is still empty. On success the slot now
+ // owns `candidate`. On failure another publish already won, so free our copy —
+ // the caller must not touch `candidate` after this returns either way.
+ if (describe_cache.compare_exchange_strong(expected, candidate,
+ std::memory_order_acq_rel, std::memory_order_acquire)) {
+ return true;
</file context>
| // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 -> | ||
| // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake | ||
| // failure cannot leak the libscram state. | ||
| PgSQL_Scram_State* scram = nullptr; |
There was a problem hiding this comment.
P3: The SCRAM guard added here is dead code and its comment is misleading. scram (a ProxySQL PgSQL_Scram_State*) is initialized to nullptr and never assigned anywhere in handleAuthentication, and doSASLAuth creates its own unrelated libscram state (ScramState* st = scram_state_init()), which it already frees manually on every exit path. So the guard's ~ScramGuard() body if (*s) pg_scram_free(*s) never executes (*s is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references pg_scram_free (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the pg_scram_* symbols, and PG_LITE_CLIENT_SCRAM is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole #ifdef PG_LITE_CLIENT_SCRAM ... #endif block (and, if desired, the now-unused PgSQL_Backend_Protocol.h include).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/tap/tests/pg_lite_client.cpp, line 330:
<comment>The SCRAM guard added here is dead code and its comment is misleading. `scram` (a ProxySQL `PgSQL_Scram_State*`) is initialized to nullptr and never assigned anywhere in `handleAuthentication`, and `doSASLAuth` creates its own unrelated libscram state (`ScramState* st = scram_state_init()`), which it already frees manually on every exit path. So the guard's `~ScramGuard()` body `if (*s) pg_scram_free(*s)` never executes (`*s` is always nullptr) and provides no RAII leak protection, despite the comment claiming the "SCRAM state ... RAII-freed on every exit path." The block also references `pg_scram_free` (a libproxysql.a symbol) in a test client whose own include comment says tests sharing this file should not pull the `pg_scram_*` symbols, and `PG_LITE_CLIENT_SCRAM` is never defined anywhere in the build tree, so the block is never even compiled. Remove the whole `#ifdef PG_LITE_CLIENT_SCRAM ... #endif` block (and, if desired, the now-unused `PgSQL_Backend_Protocol.h` include).</comment>
<file context>
@@ -315,6 +323,17 @@ void PgConnection::handleAuthentication(const std::string& password) {
+ // The SCRAM state persists across the multi-round SASL handshake (10 -> 11 ->
+ // 12 -> 0). RAII-freed on every exit path (throw or return) so a mid-handshake
+ // failure cannot leak the libscram state.
+ PgSQL_Scram_State* scram = nullptr;
+ struct ScramGuard {
+ PgSQL_Scram_State** s;
</file context>
| server-version-dependent strings. | ||
| - **Corpus.** Scalar/row/empty/error results; every data type in text and binary format; | ||
| multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without | ||
| channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer |
There was a problem hiding this comment.
P3: §7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/superpowers/specs/2026-06-11-pgsql-native-protocol-design.md, line 218:
<comment>§7's differential-test corpus lists "multi-round SCRAM (with and without channel binding)" as a test case, but the entire design defers channel binding: §2 defers SCRAM-SHA-256-PLUS, §4 selects plain SCRAM-SHA-256 whenever the plain mechanism is offered and routes -PLUS-only servers to the libpq fallback at connect time. Because the native path never performs channel binding, a "with channel binding" case exercises only the libpq branch and cannot be differentially compared against the native path. Drop "with and without channel binding" from the corpus, or reword it to reflect that -PLUS-only servers only exercise the fallback.</comment>
<file context>
@@ -0,0 +1,253 @@
+ server-version-dependent strings.
+- **Corpus.** Scalar/row/empty/error results; every data type in text and binary format;
+ multi-statement queries; COPY in/out; `NOTIFY`; multi-round SCRAM (with and without
+ channel binding), md5, cleartext; TLS on/off; large result sets (multi-buffer
+ framing); mid-session `SET client_encoding`. Error cases compare parsed `ErrorResponse`
+ fields.
</file context>
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsReplaces libpq with a native PostgreSQL backend wire-protocol implementation covering connection, authentication, COPY, simple queries, and extended queries. Consider addressing the minor audit log timing issue in the certificate authentication path where AUTH_OK is recorded before client welcome failure. 💡 Quality: AUTH_OK audit logged before welcome_client failure in cert path📄 lib/PgSQL_Session.cpp:4275-4285 📄 lib/PgSQL_Session.cpp:4236-4245 In the cert-auth branch the AUTH_OK audit entry is emitted (line ~4277) before welcome_client() is called, so when welcome_client() now returns false and the session is rejected as *wrong_pass, an AUTH_OK is still recorded for a connection that never succeeded. The other new branch (line ~4237) correctly moved log_audit_entry inside the success case. Move the log_audit_entry call into the 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
Resolve conflicts for PR 5882: - test/tap/tests/unit/Makefile: keep vec.o single-append NOTE (both sides removed the duplicate first vec.o block; HEAD's explanatory comment kept)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replaces libpq on the ProxySQL → PostgreSQL backend data path with a native wire-protocol implementation, behind the runtime flag
pgsql-use_native_backend_protocol(default off; libpq stays compiled in as fallback and as the differential-test oracle). Monitor and plugins keep using libpq.Design specs and implementation plans are in
docs/superpowers/specs/anddocs/superpowers/plans/(2026-06-11 through 2026-07-07).Highlights
COPY ... TO STDOUTstreams natively;COPY ... FROM STDINkeeps the session fast-forward route (byte-equal, zero-copy); a CopyFail safety net turns any unexpected CopyInResponse on the native drive into a clean error instead of a protocol hang.local_stmtsclient registry, per-backend statement reuse and implicit re-Parse are all retained — only the wire layer is swapped (typed Parse/Bind/Describe/Execute/Close/Flush/Sync builders; per-step drain with ack filtering that preserves ProxySQL's BindComplete/CloseComplete/ParseComplete synthesis; pipeline-abort recovery that injects a Sync on mid-frame errors).PgSQL_STMT_Global_info(set-once, atomic publish): repeat Describes are served without a backend round trip, in both backend modes.SQL3_Free_Connections, native query errors misclassified as broken connections, a bare-ack assert crash, a CopyFail partial-send hang.Testing
Differential testing against the libpq path as oracle (a divergence is a hard failure):
pgsql-native_auth_differential,query_differential(16/16),streaming,transactions(16/16),copy(15/15, with truthful per-route coverage reporting),prepared(27/27 strict — no escape hatches; all EXT_* operations native and byte-equal, incl. named statements + DEALLOCATE, mid-frame error recovery positively asserted, cross-mode Describe-cache parity in both directions),notify,stress(200× PREPARE/SELECT/txn across the pool).legacy-g1group: all pgsql tests green; the 10 MySQL-side failures were individually root-caused as unrelated to this branch (PgSQL-only diff; mostly sharedtest.sbtest1contamination between tests — triage notes available, tracking issues to follow).Known limitations / follow-ups
-PLUS-only-server edge cases fall back to libpq at connect time (logged once per backend).https://claude.ai/code/session_015yDEBKWSWyDYq9MFxS69p7
Summary by CodeRabbit
tls-server-end-point), with graceful fallback when not available.