Add 32 unit test files (2963 assertions) and CI workflow improvements

[renecannao]

Summary

Massive expansion of unit test coverage for lib/, include/, and GenAI/MCP components.

• 32 new unit test files with 2,963 total assertions (up from 16 files / ~400 assertions)
• CI infrastructure improvements: groups.json validation, DEBUG auto-detection, TSDB auto-detection fix
• 2 new CI workflows: legacy-g4 and legacy-clickhouse-g1
• Bug fix: stale proxysql.db causing cluster sync test failures

Test Coverage Areas

Core Utilities

• gen_utils, proxysql_utils, MySQL_encode, proxy_protocol_info, sqlite3db
• c_tokenizer, pgsql_tokenizer (SQL digest/tokenization)
• PgSQL_Variables_Validator, PgSQL_Error_Helper, PgSQL_ExplicitTxnStateMgr
• ProxySQL_Config (validation + Write functions), ProxySQL_Statistics
• ProxySQL_Admin_Disk_Upgrade (schema migrations)
• Query_Processor (firewall whitelist + PgSQL command types)
• GTID_Server_Data helpers, ProxySQL_GloVars, log_utils
• proxysql_find_charset, ezOptionParser, proxysql_listen_validator

GenAI/MCP (PROXYSQLGENAI=1)

• Stats_Tool_Handler parsing, Query_Tool_Handler helpers
• MySQL_FTS string utilities, Anomaly_Detector (normalization + SQLi detection)
• LLM_Clients (retryability), MCP_Endpoint (auth + JSON-RPC)
• MCP_Thread + GenAI_Thread variable management (197 + 193 tests)
• Discovery_Schema (172 tests), MySQL_Catalog (89 tests)

CI Improvements

• check_groups.py — validates all compiled tests are in groups.json
• run-multi-group.bash calls check_groups.py at startup (exit 1 if missing)
• Makefile: auto-detect -DDEBUG and -DPROXYSQLTSDB from libproxysql.a symbols
• New workflows: CI-legacy-g4.yml, CI-legacy-clickhouse-g1.yml

Bug Fixes

• cluster sync test: remove stale proxysql.db before launching replicas
• namespace collision: wrap parse_int_or_zero in stats_utils namespace
• TSDB detection: search for init_tsdb_variables instead of nonexistent ProxySQL_TSDB

Findings

• GenAI_Thread.cpp: llm_cache_enabled variable listed but has no get/set handler
• PgSQL_Query_Processor.cpp: OPERATOR CLASS/FAMILY branches are dead code

Test plan

• All 48 unit tests pass locally (make debug in test/tap/tests/unit/)
• check_groups.py reports 0 missing tests
• CI run with legacy-g2, legacy-g4, basictests, legacy-clickhouse-g1

Summary by CodeRabbit

• New Features

  • Centralized Bearer-token validation and additional SQL/protocol validation helpers.
  • Pre-flight test-group completeness check to prevent CI runs when compiled tests are missing.
  • New reusable CI workflows for legacy and grouped test pipelines.

• Tests

  • Large expansion of unit and integration tests across query processing, tokenizers, auth, stats, GenAI, catalog, and CI plumbing.

• Chores

  • Testability refactors and loosened test-tool dependency constraints.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds reusable GitHub Actions workflows for legacy CI groups, exposes many internal helpers to unit tests (friend declarations and namespace helpers), centralizes Bearer-token validation, introduces a TAP-groups completeness checker, broadens helper visibility, and adds dozens of new unit tests plus supporting CI/test infra and dependency updates.

Changes

Cohort / File(s)	Summary
GitHub Actions: entry workflows ​.github/workflows/CI-legacy-clickhouse-g1.yml, ​.github/workflows/CI-legacy-g4.yml	New manual + workflow_run-triggered workflows with concurrency keyed by workflow+branch/ref; they call reusable workflows and pass the full github context as JSON.
GitHub Actions: reusable workflows ​.github/workflows/gh-actions-reusable/ci-legacy-clickhouse-g1.yml, ​.github/workflows/gh-actions-reusable/ci-legacy-g4.yml	New reusable CI workflows (workflow_call): compute SHA from trigger, sparse checkout, restore caches, build CI Docker image, start infra, run isolated tests, upload logs on failure, and update check status.
Headers: test access & helpers include/Anomaly_Detector.h, include/MCP_Endpoint.h, include/Query_Tool_Handler.h, include/Stats_Tool_Handler.h, include/proxysql_admin.h, include/proxysql_config.h	Added multiple friend declarations for test helpers and declared several free-function/namespace helper prototypes to expose internals for unit tests.
Library code: helper visibility & auth lib/LLM_Clients.cpp, lib/MCP_Endpoint.cpp, lib/Query_Tool_Handler.cpp, lib/Stats_Tool_Handler.cpp	Widened helper linkage (static→non-static), renamed LLM callback to LLM_WriteCallback, grouped stats helpers into stats_utils, and added MCP_JSONRPC_Resource::validate_bearer_token() used by authenticate_request().
Test infra: pre-run validation & scripts test/infra/control/run-multi-group.bash, test/tap/groups/check_groups.py	Added Python checker that compares groups.json to compiled TAP binaries; runner now aborts early when compiled binaries are present but missing from groups.json.
Test harness: build changes test/tap/tests/unit/Makefile	Expanded feature detection (TSDB/debug), added -DDEBUG conditional, extended UNIT_TESTS list, and added tap_noise_stubs.o/custom link rules for certain tests.
Tests: GenAI / MCP / LLM test/tap/tests/unit/genai_*, test/tap/tests/unit/genai_mcp_*, test/tap/tests/unit/genai_llm_clients_unit-t.cpp	Many new GenAI/MCP/LLM-related unit tests (anomaly detection, discovery schema, MCP endpoints/threads, LLM client helpers, FTS, catalog, parsing, threads/config).
Tests: Query/Protocol/Tokenizers test/tap/tests/unit/query_processor_*.cpp, test/tap/tests/unit/c_tokenizer_unit-t.cpp, test/tap/tests/unit/pgsql_*, test/tap/tests/unit/proxy_protocol_unit-t.cpp	New and expanded tests for tokenizers, PgSQL/MySQL protocol logic, query processor, firewall, transaction state, variable validators, and proxy-protocol parsing.
Tests: Config/Admin/DB/Utilities test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp, test/tap/tests/unit/config_*, test/tap/tests/unit/sqlite3db_unit-t.cpp, test/tap/tests/unit/*_unit-t.cpp	Added many unit tests covering admin disk upgrades, config validation/write, SQLite3DB, encodings, charset lookup, GTID utils, logging, listener validation, and assorted utilities.
CI/test runtime tweaks test/tap/tests/test_cluster_sync_mysql_servers-t.cpp	Pre-launch removal of persisted ProxySQL DB files to avoid loading stale per-run state before starting ProxySQL.
Test environment requirements test/scripts/bin/Pipfile, test/scripts/bin/requirements*.txt	Switched Python runtime requirement to 3 and loosened/pinned Python package constraints for test scripts (moved many == pins to >=).
Test grouping metadata test/tap/groups/groups.json	Registered many new unit and GenAI test group entries mapping tests to CI infra targets (unit-tests-g1, ai-g1, legacy groups, etc.).

Sequence Diagram(s)

sequenceDiagram
    participant CI_TRIGGER as CI-trigger
    participant GH as GitHub Actions
    participant LEGACY as CI-legacy workflow
    participant REUSABLE as Reusable workflow (sysown/proxysql@GH-Actions)
    CI_TRIGGER->>GH: emit workflow_run (in_progress)
    GH->>LEGACY: start workflow (workflow_run / workflow_dispatch)
    LEGACY->>GH: set concurrency group (workflow + head branch/ref)
    LEGACY->>REUSABLE: workflow_call (with input trigger = toJson(github))
    REUSABLE->>GH: inherit secrets, checkout at SHA, restore caches
    REUSABLE->>REUSABLE: build CI image, start infra, run isolated tests
    REUSABLE-->>GH: upload artifacts on failure
    REUSABLE-->>GH: update check status

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• AI and SQLite3: Fix SIGSEGV and retry logic bugs #5409: overlaps edits to lib/LLM_Clients.cpp (write-callback and retry helper visibility).
• Query Processor rule management unit tests (Phase 2.4) #5487: overlaps large query-processor and PgSQL unit-test additions.
• Run ProxySQL cluster inside single container + CI stability fixes #5528: related CI/test-infra changes adding legacy CI workflows and wiring.

Poem

🐰 I hopped through code with nimble paws,

Friend classes opened secret laws,
Tokens trimmed and headers bright,
CI springs up to run the night,
Tests parade — a carrot-clap applause!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 46.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: 32 new unit test files with 2963 assertions and CI workflow improvements are the primary focus of this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch v3.0-CodeCov0325

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly strengthens the project's testing framework by introducing a substantial number of new unit tests and refining existing CI processes. The primary goal is to enhance code reliability and maintainability through broader test coverage and more robust build validations, particularly for core utilities and GenAI/MCP functionalities.

Highlights

• Expanded Unit Test Coverage: Massively increased unit test coverage for core libraries, includes, and GenAI/MCP components, adding 32 new files and 2,963 assertions.
• CI Infrastructure Improvements: Enhanced CI workflows with groups.json validation, automatic detection of DEBUG and PROXYSQLTSDB flags, and introduced new CI-legacy-g4.yml and CI-legacy-clickhouse-g1.yml workflows.
• Bug Fixes: Addressed a bug causing cluster sync test failures due to stale proxysql.db files and resolved a namespace collision for parse_int_or_or_zero.
• TSDB Detection Fix: Corrected the TSDB auto-detection logic to accurately identify init_tsdb_variables instead of a non-existent ProxySQL_TSDB symbol.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Ignored Files

• Ignored by pattern: .github/workflows/** (4)
  • .github/workflows/CI-legacy-clickhouse-g1.yml
  • .github/workflows/CI-legacy-g4.yml
  • .github/workflows/gh-actions-reusable/ci-legacy-clickhouse-g1.yml
  • .github/workflows/gh-actions-reusable/ci-legacy-g4.yml

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[Copilot]

Pull request overview

Expands automated coverage across core ProxySQL utilities and GenAI/MCP components while tightening CI/test infrastructure checks (especially around TAP test grouping and feature auto-detection).

Changes:

• Adds a large set of new TAP unit tests covering core utility codepaths and GenAI/MCP helpers.
• Improves TAP/CI infrastructure: groups.json completeness validation, DEBUG/TSDB auto-detection in unit test build flags, and new legacy CI workflows.
• Fixes cluster sync test flakiness by removing stale persisted DB files before launching replicas; refactors/exports some GenAI helper functions for direct unit testing.

Reviewed changes

Copilot reviewed 50 out of 51 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/tap/tests/unit/sqlite3db_unit-t.cpp	New unit tests for SQLite3DB wrapper behaviors (execute/select/schema/build/locks).
test/tap/tests/unit/proxysql_utils_unit-t.cpp	New unit tests for proxysql_utils helpers (hex/unhex, escaping, splitting, checksums).
test/tap/tests/unit/proxy_protocol_unit-t.cpp	New unit tests for PROXY protocol header parsing + subnet/network matching.
test/tap/tests/unit/pgsql_variables_validator_unit-t.cpp	New unit tests for PgSQL variable validators (bool/float/string/mem/encoding/search_path).
test/tap/tests/unit/pgsql_tokenizer_unit-t.cpp	New unit tests for PgSQL digest/tokenizer behavior and edge cases.
test/tap/tests/unit/mysql_encode_unit-t.cpp	New unit tests for MySQL protocol encoding helpers (lenenc, CPY*, XOR, etc.).
test/tap/tests/unit/log_utils_unit-t.cpp	New unit tests for LogBuffer and sampling/flush helpers.
test/tap/tests/unit/gtid_utils_unit-t.cpp	New unit tests for GTID set/interval helpers + serialization.
test/tap/tests/unit/glovars_unit-t.cpp	New unit tests for GloVars defaults, checksum utilities, and CLI option parsing.
test/tap/tests/unit/genai_stats_parsing_unit-t.cpp	New GenAI-only unit tests for Stats tool parsing + percentile helpers.
test/tap/tests/unit/genai_query_handler_unit-t.cpp	New GenAI-only unit tests for Query tool helper functions and comment stripping.
test/tap/tests/unit/genai_mcp_thread_unit-t.cpp	New GenAI-only unit tests for MCP thread variables/validation/listing/locks.
test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp	New GenAI-only unit tests for MCP endpoint auth + JSON-RPC response/error formatting.
test/tap/tests/unit/genai_llm_clients_unit-t.cpp	New GenAI-only unit tests for retryability classification + curl write callback.
test/tap/tests/unit/genai_fts_string_unit-t.cpp	New GenAI-only unit tests for MySQL_FTS string sanitization/escaping/table naming.
test/tap/tests/unit/genai_anomaly_unit-t.cpp	New GenAI-only unit tests for Anomaly_Detector normalization and SQLi detection.
test/tap/tests/unit/gen_utils_unit-t.cpp	New unit tests for gen_utils wildcard matching, escaping, and file checks.
test/tap/tests/unit/charset_find_unit-t.cpp	New unit tests for charset/collation lookup helpers and edge cases.
test/tap/tests/unit/c_tokenizer_unit-t.cpp	New/expanded unit tests for MySQL tokenizer/digest/strip comment utilities.
test/tap/tests/unit/Makefile	Adds unit-test targets, TSDB/DEBUG symbol-based autodetect, and a standalone build rule for ezOptionParser tests.
test/tap/tests/test_cluster_sync_mysql_servers-t.cpp	Fixes test flakiness by deleting stale proxysql.db/proxysql_stats.db before replica startup.
test/tap/groups/check_groups.py	New script validating compiled TAP tests vs groups.json registration.
test/infra/control/run-multi-group.bash	Runs groups.json completeness validation before executing TAP groups.
lib/Stats_Tool_Handler.cpp	Moves parsing helpers into stats_utils namespace and exports them for testing.
lib/Query_Tool_Handler.cpp	Exposes several helper functions (previously static) for unit testing.
lib/MCP_Endpoint.cpp	Extracts Bearer-token validation into a pure helper for unit testing/reuse.
lib/LLM_Clients.cpp	Renames/exports curl write callback; exports retryability helper for unit testing.
include/proxysql_config.h	Adds friend test helper access for config unit tests.
include/proxysql_admin.h	Adds friend test access for disk-upgrade test helper.
include/Stats_Tool_Handler.h	Declares stats_utils::* helpers + exposes percentile/interval helpers for tests.
include/Query_Tool_Handler.h	Adds friend test class; declares exported helper functions for unit tests.
include/MCP_Endpoint.h	Adds friend test class + declares validate_bearer_token().
include/Anomaly_Detector.h	Adds friend test helper to access private methods in unit tests.
.github/workflows/gh-actions-reusable/ci-legacy-g4.yml	New reusable workflow to run legacy-g4 group in CI.
.github/workflows/gh-actions-reusable/ci-legacy-clickhouse-g1.yml	New reusable workflow to run legacy-clickhouse-g1 group in CI.
.github/workflows/CI-legacy-g4.yml	New top-level workflow wiring to reusable legacy-g4 workflow with concurrency.
.github/workflows/CI-legacy-clickhouse-g1.yml	New top-level workflow wiring to reusable legacy-clickhouse-g1 workflow with concurrency.

Comments suppressed due to low confidence (1)

lib/LLM_Clients.cpp:147

• These helper functions were made non-static to support unit tests, but is_retryable_error is a very generic symbol name and is now exported from the library without any namespacing/header declaration. This increases the risk of future link-time symbol collisions and makes the API surface unclear. Consider moving these helpers into a dedicated namespace (or a *_utils-prefixed name) and exposing them via an internal/test header, rather than exporting generic global symbols.

size_t LLM_WriteCallback(void* contents, size_t size, size_t nmemb, void* userp) {
	size_t totalSize = size * nmemb;
	std::string* response = static_cast<std::string*>(userp);
	response->append(static_cast<char*>(contents), totalSize);
	return totalSize;
}

// ============================================================================
// Retry Logic Helper Functions
// ============================================================================

/**
 * @brief Check if an error is retryable based on HTTP status code
 *
 * Determines whether a failed LLM API call should be retried based on:
 * - HTTP status codes (408 timeout, 429 rate limit, 5xx server errors)
 * - CURL error codes (network failures, timeouts)
 *
 * @param http_status_code HTTP status code from response
 * @param curl_code libcurl error code
 * @return true if error is retryable, false otherwise
 */
bool is_retryable_error(int http_status_code, CURLcode curl_code) {
	// Retry on specific HTTP status codes
	if (http_status_code == 408 ||           // Request Timeout
	    http_status_code == 429 ||           // Too Many Requests (rate limit)
	    http_status_code == 500 ||           // Internal Server Error
	    http_status_code == 502 ||           // Bad Gateway
	    http_status_code == 503 ||           // Service Unavailable
	    http_status_code == 504) {           // Gateway Timeout
		return true;
	}

	// Retry on specific curl errors (network issues, timeouts)
	if (curl_code == CURLE_OPERATION_TIMEDOUT ||
	    curl_code == CURLE_COULDNT_CONNECT ||
	    curl_code == CURLE_READ_ERROR ||
	    curl_code == CURLE_RECV_ERROR) {
		return true;
	}

	return false;
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Same issue as above: zeroing handler.variables and handler.status_variables with memset() leaks the constructor's strdup("") allocations for the auth fields and obscures default initialization. Prefer not to memset() these members in the test setup.

[Copilot]

Same issue as above: these memset() calls overwrite MCP_Threads_Handler's allocated auth strings (initialized via strdup("") in the constructor), causing memory leaks and potentially confusing sanitizer runs. Prefer leaving the constructor defaults intact or explicitly resetting only the necessary fields.

[Copilot]

result is checked for null with ok(result != nullptr, ...), but the next assertion unconditionally dereferences it (result->rows_count). If execute_statement() ever returns null (e.g., schema/setup failure), this test will segfault instead of reporting a test failure. Guard all result->... accesses behind if (result) (or fold the check into the same ok() expression and early-return) so the test fails cleanly.

[Copilot]

MCP_Threads_Handler's constructor allocates endpoint auth strings with strdup("") (see lib/MCP_Thread.cpp). These memset() calls overwrite those pointers without freeing them, which can show up as leaks under sanitizers and also hides the actual default initialization behavior you're trying to test. Prefer relying on the constructor defaults (already "no auth"), or explicitly free/reset just the specific fields you need instead of zeroing the whole struct.

[Copilot]

Same issue as above: memset(&handler.variables, 0, ...) overwrites constructor-owned strdup("") pointers in MCP_Threads_Handler::variables without freeing them first. Remove these memset() calls and rely on the constructor defaults (already empty auth strings) to avoid leaks/UB under sanitizers.

[gemini-code-assist]

Code Review

This pull request significantly expands the unit testing coverage and infrastructure. It exposes several internal helper functions across various lib/ files for direct unit testing, particularly for new GenAI features and existing PostgreSQL components. A large number of new unit test files have been added to cover diverse functionalities, from core ProxySQL operations to specific PostgreSQL and GenAI-related modules. The testing framework is further enhanced by a new Python script that verifies the completeness of the test suite registration and modifications to the Makefile to integrate these new tests. Additionally, stale database files are now explicitly removed in a cluster synchronization test to ensure cleaner test environments.

[coderabbitai]

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

lib/Stats_Tool_Handler.cpp (1)

124-125: ⚠️ Potential issue | 🟠 Major

Normalize bracketed IPv6 literals before returning host.

parse_server_filter("[2001:db8::1]:6033", ...) currently returns host == "[2001:db8::1]". The downstream equality checks use raw srv_host values, so IPv6 backends can never match server filters in show_connections, show_free_connections, and show_connection_history.

🛠️ Proposed fix

 	host = server_filter.substr(0, colon);
+	if (host.size() >= 2 && host.front() == '[' && host.back() == ']') {
+		host = host.substr(1, host.size() - 2);
+	}
 	std::string port_str = server_filter.substr(colon + 1);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Stats_Tool_Handler.cpp` around lines 124 - 125, parse_server_filter
currently returns bracketed IPv6 hosts (e.g. "[2001:db8::1]") which prevents
matches; after extracting host (the variable host from server_filter) detect if
host begins with '[' and ends with ']' and strip the surrounding brackets so the
function returns the unbracketed IPv6 literal. Implement this normalization in
parse_server_filter (operate on host after host = server_filter.substr(...)) and
ensure it only strips when both leading '[' and trailing ']' are present.

lib/Query_Tool_Handler.cpp (1)

183-205: ⚠️ Potential issue | 🟠 Major

json_int() silently truncates float values instead of using default_val.

The is_number() check matches both integers and floats. If malformed MCP input provides a float like 3.14, the code calls val.get<int>(), which silently truncates to 3 instead of falling back to default_val. Use is_number_integer() to restrict to integer values only:

int json_int(const json& j, const std::string& key, int default_val) {
 	if (j.contains(key) && !j[key].is_null()) {
 		const json& val = j[key];
-		// If it's already a number, return it
-		if (val.is_number()) {
-			return val.get<int>();
+		// If it's already an integer, return it
+		if (val.is_number_integer()) {
+			return val.get<int>();
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Query_Tool_Handler.cpp` around lines 183 - 205, The json_int function
currently treats any JSON number as an integer and calls val.get<int>(), which
truncates floats; change the numeric check in json_int (function name: json_int,
symbol: val.is_number()) to only accept integers (use val.is_number_integer() or
the equivalent is_number_integer()/is_number_unsigned()/is_number_integer()
combo) so that floating-point inputs (e.g., 3.14) fall through and return
default_val; keep the boolean branch (val.is_boolean()) and string parsing with
stoi as-is, but ensure numeric strings with decimals still fall back to
default_val.

🧹 Nitpick comments (8)

test/tap/tests/unit/proxysql_utils_unit-t.cpp (1)

113-126: Add zero-dimension contract tests for generate_multi_rows_query().

Current tests only lock behavior for positive dimensions. Please add explicit assertions for (rows=0, params>0), (rows>0, params=0), and (0,0) to prevent regressions when this helper is refactored.

Also applies to: 150-172

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/proxysql_utils_unit-t.cpp` around lines 113 - 126, Add
unit tests in test_generate_multi_rows_query and test_generate_multi_rows_single
(or new test functions) to cover zero-dimension cases for
generate_multi_rows_query: assert that generate_multi_rows_query(0, N) returns
an empty string and contains no '?' for any N>0, that
generate_multi_rows_query(M, 0) returns an empty string and contains no '?' for
any M>0, and that generate_multi_rows_query(0,0) returns an empty string with no
placeholders; ensure each assertion uses ok(...) with a clear message so
regressions are caught when this helper is refactored.

test/tap/tests/unit/charset_find_unit-t.cpp (1)

128-147: Restore global collation setting to keep tests isolated

test_find_charset_name_with_collation() leaves mysql_thread___default_variables[SQL_COLLATION_CONNECTION] modified, which can create order-dependent behavior for future additions. Save and restore it inside this helper.

Proposed fix

 static void test_find_charset_name_with_collation() {
+	char *saved_collation =
+		mysql_thread___default_variables[SQL_COLLATION_CONNECTION];
+
 	// When default_collation is set, proxysql_find_charset_name should
 	// prefer the matching collation entry
 	mysql_thread___default_variables[SQL_COLLATION_CONNECTION] =
 		(char *)"utf8_bin";
@@
 	ok(c != NULL && strcmp(c->csname, "utf8") == 0,
 		"name('utf8') with non-matching collation falls back to first match");
+
+	mysql_thread___default_variables[SQL_COLLATION_CONNECTION] = saved_collation;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/charset_find_unit-t.cpp` around lines 128 - 147, The test
test_find_charset_name_with_collation modifies
mysql_thread___default_variables[SQL_COLLATION_CONNECTION] and doesn't restore
it, causing test-order dependencies; fix by saving the original value into a
local variable (e.g. prev_collation) before changing it and restore
mysql_thread___default_variables[SQL_COLLATION_CONNECTION] = prev_collation at
the end of test_find_charset_name_with_collation (after all assertions) so the
global collation is returned to its prior state.

test/tap/tests/unit/sqlite3db_unit-t.cpp (1)

25-35: Avoid sharing one mutable SQLite3DB across the whole suite.

Several cases only pass because earlier tests created tables and inserted rows into the global db. That makes failures cascade and prevents isolated execution. Recreate/reset the in-memory database per test, or at least per logical group.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/sqlite3db_unit-t.cpp` around lines 25 - 35, The test
suite currently shares a single mutable SQLite3DB instance via the static db and
setup_db/teardown_db, causing state leakage between tests; change to create a
fresh SQLite3DB for each test (or logical group) by removing the shared static
db and making setup_db instantiate and return a new SQLite3DB (or register it as
a per-test fixture), ensure open((char*)":memory:", ...) is called for each new
instance, and ensure teardown_db deletes only that instance so tables/data do
not persist across tests; reference symbols: SQLite3DB, db, setup_db,
teardown_db.

test/tap/tests/unit/pgsql_error_helper_unit-t.cpp (1)

152-153: Reset info between branch cases.

This test reuses one PgSQL_ErrorInfo across dozens of fill_error_info() calls, but several branches only assert type. That lets stale category or severity values from the previous case hide a partial-overwrite bug in fill_error_info().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/pgsql_error_helper_unit-t.cpp` around lines 152 - 153,
The test test_fill_error_class_branches reuses a single PgSQL_ErrorInfo instance
across many fill_error_info() calls which allows stale fields (like category or
severity) to persist and mask partial-overwrite bugs; fix by
resetting/reinitializing the PgSQL_ErrorInfo (e.g., recreate or zero-initialize
the info object) before each branch/call to fill_error_info() so assertions on
type cannot be satisfied by leftover category/severity values.

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp (1)

64-176: Add one case for unauthenticated config.ping.

These auth tests stop at validate_bearer_token(), so a regression in the actual request-auth path could still break the ping exception without failing this suite.

Based on learnings, the ping method at the config endpoint should not require the Authorization: Bearer header.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp` around lines 64 - 176, Add
a unit that verifies the config.ping JSON-RPC method succeeds without an
Authorization header: in the same test file (near the validate_bearer_token
tests) add a test (e.g., test_config_ping_unauthenticated) that constructs a
JSON-RPC request for method "config.ping" with no Authorization value,
dispatches it through the same request-handling path used by the other endpoint
tests, and assert the response is successful (not an auth error). Ensure the new
test uses the same helpers/dispatcher as the suite so it exercises the real
request-auth path rather than only validate_bearer_token().

test/tap/tests/unit/ezoption_parser_unit-t.cpp (1)

27-28: Note: Missing test harness includes for standalone header test.

This test file omits test_globals.h and test_init.h which are normally required for unit tests. This appears intentional since ezOptionParser is a standalone header-only library with no ProxySQL dependencies.

If this is the intended pattern for testing external/standalone headers, consider documenting this exception in the test directory or Makefile comments.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/ezoption_parser_unit-t.cpp` around lines 27 - 28, The
test file currently only includes "tap.h" and "ezOptionParser.hpp" which omits
the usual test harness headers; either add the standard unit-test harness
includes ("test_globals.h" and "test_init.h") to this test so it follows the
same setup as other unit tests, or if the omission is intentional for standalone
header testing, add a brief comment in the test file or the tests/Makefile
documenting this exception (reference the includes "tap.h" and
"ezOptionParser.hpp" to locate the file).

test/tap/tests/unit/genai_mysql_catalog_unit-t.cpp (2)

670-684: Comment mentions unicode but test doesn't include unicode characters.

The comment on line 673 states "JSON with quotes, backslashes, unicode" but the test string contains only ASCII characters with escaped quotes. Consider adding actual unicode to match the documented test scope.

♻️ Optional: Add unicode to match comment

 	// JSON with quotes, backslashes, unicode
-	std::string special_doc = "{\"desc\":\"It's a \\\"test\\\" with 'quotes'\"}";
+	std::string special_doc = "{\"desc\":\"It's a \\\"test\\\" with 'quotes' and unicode: café ñ 日本語\"}";
 	int rc = cat->upsert("db", "table", "special", special_doc);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/genai_mysql_catalog_unit-t.cpp` around lines 670 - 684,
The test_special_characters_in_document test claims to include unicode but the
special_doc string lacks non-ASCII characters; update the special_doc value used
by cat->upsert and the subsequent compare to include one or more actual Unicode
codepoints (e.g., emoji or accented letters) while preserving the existing
quotes/backslashes escapes so the stored/retrieved doc still equals special_doc;
ensure the variable name special_doc and the ok(doc == special_doc, ...)
assertion remain unchanged so the equality check validates the Unicode
round-trip.

---

460-467: Filter validation is vacuously true when search returns no results.

If results is empty, the loop doesn't execute and all_sales remains true, causing the test to pass without validating any actual filtering. Compare with test_search_with_tags_filter (line 511) which explicitly handles the empty case with bool has_billing = results.empty();.

This may be intentional given the FTS5 environment variability comments, but consider making this explicit for consistency:

♻️ Optional: Make empty-results handling explicit

 	ok(results.is_array(), "search with schema filter returns array");
 	// All results should be from sales schema
-	bool all_sales = true;
+	bool all_sales = results.empty();  // vacuously true if FTS5 returns no results
 	for (const auto& entry : results) {
 		if (entry["schema"].get<std::string>() != "sales") {
 			all_sales = false;
 			break;
 		}
+		all_sales = true;  // at least one valid result found
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/genai_mysql_catalog_unit-t.cpp` around lines 460 - 467,
The current check using all_sales is vacuously true when results is empty;
update the test to explicitly handle the empty-results case (mirror the approach
in test_search_with_tags_filter): if results.empty() assert that behavior with a
clear message (e.g. "search returned no results due to FTS5 variability"),
otherwise run the existing loop over results to compute all_sales and assert
ok(all_sales, ...); refer to the variables/results loop and
test_search_with_tags_filter when making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@include/Stats_Tool_Handler.h`:
- Around line 13-26: The header exposes uint64_t in
stats_utils::parse_digest_filter but doesn't include <cstdint>; make the header
self-contained by adding `#include` <cstdint> at the top (so uint64_t is defined)
while keeping the namespace and function declarations (parse_digest_filter and
parse_server_filter) unchanged; no other API changes are needed.

In `@lib/MCP_Endpoint.cpp`:
- Around line 35-37: The authorization header check using bearer_prefix and
auth_header calls auth_header.compare(...) which is case-sensitive and will
reject valid schemes like "bearer" or "BEARER"; change this to a
case-insensitive check (for example use strncasecmp on auth_header.c_str() vs
bearer_prefix.c_str(), or normalize a substring of auth_header to lowercase and
then compare to "bearer ") inside the same block so valid case variants pass and
other logic remains unchanged; update the check around bearer_prefix and
auth_header (where compare() is used) to perform the case-insensitive comparison
and preserve the length check.

In `@test/tap/groups/check_groups.py`:
- Around line 28-49: The current scanning logic only iterates immediate entries
in test_dirs and misses binaries in nested subdirectories; replace the
os.listdir loop for each directory in test_dirs with a recursive walk (os.walk)
starting from each base (e.g., the existing entries in test_dirs or simply
tap_root/tests and tap_root/tests_with_deps) and for each file path apply the
same checks (entry name endswith "-t", os.path.isfile(path), os.access(path,
os.X_OK), and _is_elf(path)) before adding to the compiled set; update the loop
that currently references compiled and _is_elf so it aggregates results from the
recursive traversal rather than only top-level entries.

In `@test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp`:
- Around line 53-66: The test currently uses calloc + reinterpret_cast to create
admin (ProxySQL_Admin) which bypasses constructors; replace that with proper
object construction (e.g., admin = new ProxySQL_Admin(); or placement new into
appropriately typed storage) and remove the free(admin) in the destructor,
instead use delete admin; ensure the created SQLite3DB is still assigned to
admin->configdb and cleanup deletes admin->configdb and then delete admin so
both constructor and destructor run for ProxySQL_Admin.

In `@test/tap/tests/unit/charset_find_unit-t.cpp`:
- Around line 270-274: The test harness should stop if initialization fails: in
main() after calling test_init_minimal() and storing its return in rc,
immediately short-circuit (e.g., return rc or call exit/skip) when rc != 0
instead of continuing; ensure you do this before any further test code (the
existing ok(rc == 0, ...) assertion can remain but follow it with an early
return/exit to avoid touching uninitialized globals from test_init_minimal().

In `@test/tap/tests/unit/genai_discovery_schema_unit-t.cpp`:
- Around line 1093-1095: The TAP plan count is wrong: in main() the call to
plan(172) does not match the 171 explicit ok(...) assertions in the
PROXYSQLGENAI branch; update the plan to the correct count (change plan(172) to
plan(171)) or recompute and set plan(...) to the actual number of tests produced
by this test file (inspect the PROXYSQLGENAI branch assertions and adjust the
plan call in main() accordingly, near test_init_minimal()).
- Around line 519-528: The test test_log_rag_search_fts creates two
Discovery_Schema instances and double-calls init on ds even though ds2 is used
for log_rag_search_fts; remove the unused ds2 and the extra init call: use a
single Discovery_Schema returned from create_test_schema(), do not call init()
again (since create_test_schema already initializes it), call
ds->log_rag_search_fts("billing tables", 5, "{\"type\":\"table\"}"), assert rc
== 0 with ok(), and delete the single ds at the end to avoid double
initialization and unused variable.
- Around line 54-62: The helper create_test_schema must fail fast on
Discovery_Schema::init() failure instead of returning nullptr; update
create_test_schema to detect rc != 0, delete ds, emit a TAP-friendly diagnostic
or error (so test runner reports a failure) and immediately abort/exit the test
run (e.g., call exit(1)) so callers that dereference the result do not segfault;
modify the logic inside create_test_schema (the function name and its use of
Discovery_Schema::init) accordingly.

In `@test/tap/tests/unit/genai_mysql_catalog_unit-t.cpp`:
- Around line 80-98: The tests call create_test_catalog() and use ok(cat !=
nullptr, ...) but continue even if cat is nullptr, causing null dereferences;
update each test function (e.g., test_upsert_and_get, test_upsert_update,
test_get_not_found, test_schema_isolation, test_empty_schema_global,
test_remove_existing, test_remove_nonexistent, test_remove_with_schema_filter,
test_remove_empty_schema, all test_list_*, test_search_*, test_merge_*,
test_multiple_kinds_same_key, test_special_characters_in_document) to check the
returned pointer and return early when create_test_catalog() yields nullptr
(i.e., after ok(...), if cat == nullptr then return) before calling methods like
cat->upsert, cat->get, cat->remove, cat->list, etc., to avoid null pointer
dereference.

In `@test/tap/tests/unit/genai_query_handler_unit-t.cpp`:
- Around line 24-29: Remove the fragile private->public macro hack in the test
file: delete the three macro lines that `#define` private public, `#define`
protected public, and the corresponding `#undefs`, since Query_Tool_Handler.h
already exposes internals to tests via the friend declaration (friend class
QueryToolHandlerUnitTest). Leave the include of "Query_Tool_Handler.h" and rely
on the existing friend access so strip_leading_comments and other private
methods can be tested without altering visibility.
- Around line 17-29: The TAP/proxysql harness headers (tap.h, test_globals.h,
test_init.h, proxysql.h) must be included unconditionally so that functions like
plan(), ok(), and exit_status() are available even when PROXYSQLGENAI is not
defined; move those include lines above the `#ifdef` PROXYSQLGENAI guard. Also
remove the unnecessary macro hack (`#define` private public / `#define` protected
public) around Query_Tool_Handler.h and rely on the existing friend declaration
(QueryToolHandlerUnitTest) in Query_Tool_Handler.h to access private members
instead.

In `@test/tap/tests/unit/listen_validator_unit-t.cpp`:
- Around line 649-661: The test plan's comment count for the
validate_module_listener_conflicts block is off-by-one — update the plan in the
plan(...) call so the comment showing "14 // validate_module_listener_conflicts"
reflects the actual 15 assertions, or adjust the tests to match 14; specifically
either change the comment to "15 // validate_module_listener_conflicts" in the
plan invocation or remove/merge one assertion from the validate_* tests (e.g.,
functions test_validate_no_conflicts, test_validate_cross_module_conflict,
test_validate_same_module_no_conflict, test_validate_null_listeners,
test_validate_empty_listeners, test_validate_null_string_literal,
test_validate_invalid_listeners_skipped,
test_validate_multiple_listeners_per_module,
test_validate_multiple_listeners_with_conflict,
test_validate_unix_socket_conflict, test_validate_empty_modules) so the counts
match.

In `@test/tap/tests/unit/mysql_encode_unit-t.cpp`:
- Around line 59-64: The test currently only checks MSB zeroing and non-zero
lower bytes for CPY3 in test_cpy3(), which won't catch byte-order errors; change
the assertions to assert equality against the exact little-endian value
0x00030201U returned by CPY3(data) (use CPY3(data) == 0x00030201U or equivalent
test helper) so the test verifies the exact 3-byte little-endian result and that
the MSB is zero.

In `@test/tap/tests/unit/pgsql_txn_state_unit-t.cpp`:
- Around line 523-526: The test incorrectly expects one savepoint after calling
mgr->handle_transaction("ROLLBACK TO SAVEPOINT sp2"); update the assertion to
expect two savepoints (sp1 and sp2 remain) and any related checks that assume
only one savepoint; specifically modify the ok(mgr->get_savepoint_count() == 1,
...) to assert == 2 so the test reflects that ROLLBACK TO preserves the target
savepoint (sp2) and earlier ones (e.g., sp1) while removing later ones (e.g.,
sp3).

In `@test/tap/tests/unit/query_processor_unit-t.cpp`:
- Around line 1360-1363: The empty SQLi fixture in the test uses SQLite3_result
with only a "fingerprint" column which doesn't match the real
firewall_whitelist_sqli_fingerprints schema expected by load_firewall(); update
the fixture created via SQLite3_result in query_processor_unit-t.cpp to include
both columns (e.g., an "active" column type matching the loader's expectation
and the "fingerprint" column) in the correct order and populate rows accordingly
so the test mirrors the real table schema used by load_firewall().

In `@test/tap/tests/unit/statistics_unit-t.cpp`:
- Around line 47-69: The test suite is leaking timer state because a single
ProxySQL_Statistics instance (stats) is shared across cases; update the test
fixture so timer tests start from a fresh state by recreating or explicitly
resetting the fixture before each timer test group: either add a teardown +
re-run setup_stats() to delete and reallocate stats (and reassign
GloVars.statsdb_disk) or call a reset method on ProxySQL_Statistics if one
exists (e.g., stats->resetTimers() or similar) prior to advancing timers in the
MySQL and TSDB timer tests so each group begins from zero.
- Around line 791-837: Wrap the TSDB-related test calls (e.g.
test_init_creates_tsdb_tables, test_set_variable_*, test_tsdb_* timers,
test_insert_and_query_tsdb_metric*, test_insert_and_query_backend_health,
test_get_tsdb_status, test_tsdb_downsample_*, test_tsdb_retention_* ) in a
runtime guard that checks the test fixture's statsdb pointer
(stats->statsdb_disk) and skips these tests when stats->statsdb_disk is null;
specifically, before invoking that block, add a condition like "if
(stats->statsdb_disk != nullptr) { ... } else { skip/emit message }" so the TSDB
tests do not run when statsdb failed to initialize. Ensure you reference and use
the existing stats object (stats->statsdb_disk) so behavior matches
test_constructor_creates_databases detection.

---

Outside diff comments:
In `@lib/Query_Tool_Handler.cpp`:
- Around line 183-205: The json_int function currently treats any JSON number as
an integer and calls val.get<int>(), which truncates floats; change the numeric
check in json_int (function name: json_int, symbol: val.is_number()) to only
accept integers (use val.is_number_integer() or the equivalent
is_number_integer()/is_number_unsigned()/is_number_integer() combo) so that
floating-point inputs (e.g., 3.14) fall through and return default_val; keep the
boolean branch (val.is_boolean()) and string parsing with stoi as-is, but ensure
numeric strings with decimals still fall back to default_val.

In `@lib/Stats_Tool_Handler.cpp`:
- Around line 124-125: parse_server_filter currently returns bracketed IPv6
hosts (e.g. "[2001:db8::1]") which prevents matches; after extracting host (the
variable host from server_filter) detect if host begins with '[' and ends with
']' and strip the surrounding brackets so the function returns the unbracketed
IPv6 literal. Implement this normalization in parse_server_filter (operate on
host after host = server_filter.substr(...)) and ensure it only strips when both
leading '[' and trailing ']' are present.

---

Nitpick comments:
In `@test/tap/tests/unit/charset_find_unit-t.cpp`:
- Around line 128-147: The test test_find_charset_name_with_collation modifies
mysql_thread___default_variables[SQL_COLLATION_CONNECTION] and doesn't restore
it, causing test-order dependencies; fix by saving the original value into a
local variable (e.g. prev_collation) before changing it and restore
mysql_thread___default_variables[SQL_COLLATION_CONNECTION] = prev_collation at
the end of test_find_charset_name_with_collation (after all assertions) so the
global collation is returned to its prior state.

In `@test/tap/tests/unit/ezoption_parser_unit-t.cpp`:
- Around line 27-28: The test file currently only includes "tap.h" and
"ezOptionParser.hpp" which omits the usual test harness headers; either add the
standard unit-test harness includes ("test_globals.h" and "test_init.h") to this
test so it follows the same setup as other unit tests, or if the omission is
intentional for standalone header testing, add a brief comment in the test file
or the tests/Makefile documenting this exception (reference the includes "tap.h"
and "ezOptionParser.hpp" to locate the file).

In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp`:
- Around line 64-176: Add a unit that verifies the config.ping JSON-RPC method
succeeds without an Authorization header: in the same test file (near the
validate_bearer_token tests) add a test (e.g., test_config_ping_unauthenticated)
that constructs a JSON-RPC request for method "config.ping" with no
Authorization value, dispatches it through the same request-handling path used
by the other endpoint tests, and assert the response is successful (not an auth
error). Ensure the new test uses the same helpers/dispatcher as the suite so it
exercises the real request-auth path rather than only validate_bearer_token().

In `@test/tap/tests/unit/genai_mysql_catalog_unit-t.cpp`:
- Around line 670-684: The test_special_characters_in_document test claims to
include unicode but the special_doc string lacks non-ASCII characters; update
the special_doc value used by cat->upsert and the subsequent compare to include
one or more actual Unicode codepoints (e.g., emoji or accented letters) while
preserving the existing quotes/backslashes escapes so the stored/retrieved doc
still equals special_doc; ensure the variable name special_doc and the ok(doc ==
special_doc, ...) assertion remain unchanged so the equality check validates the
Unicode round-trip.
- Around line 460-467: The current check using all_sales is vacuously true when
results is empty; update the test to explicitly handle the empty-results case
(mirror the approach in test_search_with_tags_filter): if results.empty() assert
that behavior with a clear message (e.g. "search returned no results due to FTS5
variability"), otherwise run the existing loop over results to compute all_sales
and assert ok(all_sales, ...); refer to the variables/results loop and
test_search_with_tags_filter when making the change.

In `@test/tap/tests/unit/pgsql_error_helper_unit-t.cpp`:
- Around line 152-153: The test test_fill_error_class_branches reuses a single
PgSQL_ErrorInfo instance across many fill_error_info() calls which allows stale
fields (like category or severity) to persist and mask partial-overwrite bugs;
fix by resetting/reinitializing the PgSQL_ErrorInfo (e.g., recreate or
zero-initialize the info object) before each branch/call to fill_error_info() so
assertions on type cannot be satisfied by leftover category/severity values.

In `@test/tap/tests/unit/proxysql_utils_unit-t.cpp`:
- Around line 113-126: Add unit tests in test_generate_multi_rows_query and
test_generate_multi_rows_single (or new test functions) to cover zero-dimension
cases for generate_multi_rows_query: assert that generate_multi_rows_query(0, N)
returns an empty string and contains no '?' for any N>0, that
generate_multi_rows_query(M, 0) returns an empty string and contains no '?' for
any M>0, and that generate_multi_rows_query(0,0) returns an empty string with no
placeholders; ensure each assertion uses ok(...) with a clear message so
regressions are caught when this helper is refactored.

In `@test/tap/tests/unit/sqlite3db_unit-t.cpp`:
- Around line 25-35: The test suite currently shares a single mutable SQLite3DB
instance via the static db and setup_db/teardown_db, causing state leakage
between tests; change to create a fresh SQLite3DB for each test (or logical
group) by removing the shared static db and making setup_db instantiate and
return a new SQLite3DB (or register it as a per-test fixture), ensure
open((char*)":memory:", ...) is called for each new instance, and ensure
teardown_db deletes only that instance so tables/data do not persist across
tests; reference symbols: SQLite3DB, db, setup_db, teardown_db.

🪄 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: 1df5d559-7b8f-41df-82db-d82af15f7f6f

📥 Commits

Reviewing files that changed from the base of the PR and between 2350632 and cde4cc7.

📒 Files selected for processing (51)

• .github/workflows/CI-legacy-clickhouse-g1.yml
• .github/workflows/CI-legacy-g4.yml
• .github/workflows/gh-actions-reusable/ci-legacy-clickhouse-g1.yml
• .github/workflows/gh-actions-reusable/ci-legacy-g4.yml
• include/Anomaly_Detector.h
• include/MCP_Endpoint.h
• include/Query_Tool_Handler.h
• include/Stats_Tool_Handler.h
• include/proxysql_admin.h
• include/proxysql_config.h
• lib/LLM_Clients.cpp
• lib/MCP_Endpoint.cpp
• lib/Query_Tool_Handler.cpp
• lib/Stats_Tool_Handler.cpp
• test/infra/control/run-multi-group.bash
• test/tap/groups/check_groups.py
• test/tap/groups/groups.json
• test/tap/tests/test_cluster_sync_mysql_servers-t.cpp
• test/tap/tests/unit/Makefile
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
• test/tap/tests/unit/c_tokenizer_unit-t.cpp
• test/tap/tests/unit/charset_find_unit-t.cpp
• test/tap/tests/unit/config_validation_unit-t.cpp
• test/tap/tests/unit/config_write_unit-t.cpp
• test/tap/tests/unit/ezoption_parser_unit-t.cpp
• test/tap/tests/unit/gen_utils_unit-t.cpp
• test/tap/tests/unit/genai_anomaly_unit-t.cpp
• test/tap/tests/unit/genai_discovery_schema_unit-t.cpp
• test/tap/tests/unit/genai_fts_string_unit-t.cpp
• test/tap/tests/unit/genai_llm_clients_unit-t.cpp
• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/genai_mcp_thread_unit-t.cpp
• test/tap/tests/unit/genai_mysql_catalog_unit-t.cpp
• test/tap/tests/unit/genai_query_handler_unit-t.cpp
• test/tap/tests/unit/genai_stats_parsing_unit-t.cpp
• test/tap/tests/unit/genai_thread_unit-t.cpp
• test/tap/tests/unit/glovars_unit-t.cpp
• test/tap/tests/unit/gtid_utils_unit-t.cpp
• test/tap/tests/unit/listen_validator_unit-t.cpp
• test/tap/tests/unit/log_utils_unit-t.cpp
• test/tap/tests/unit/mysql_encode_unit-t.cpp
• test/tap/tests/unit/pgsql_error_helper_unit-t.cpp
• test/tap/tests/unit/pgsql_tokenizer_unit-t.cpp
• test/tap/tests/unit/pgsql_txn_state_unit-t.cpp
• test/tap/tests/unit/pgsql_variables_validator_unit-t.cpp
• test/tap/tests/unit/proxy_protocol_unit-t.cpp
• test/tap/tests/unit/proxysql_utils_unit-t.cpp
• test/tap/tests/unit/query_processor_firewall_unit-t.cpp
• test/tap/tests/unit/query_processor_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/statistics_unit-t.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In C++, is it well-defined to allocate raw zeroed storage with calloc, cast it to a non-trivial class type, skip the constructor, and then call non-static member functions on it?

💡 Result:

No, it is not well-defined. For a non-trivial class type (one with a non-trivial constructor), casting the pointer returned by calloc to the class type and calling non-static member functions on it without first starting the object's lifetime (e.g., via placement new or std::start_lifetime_as in C++23 for eligible types) results in undefined behavior. This is because referring to any non-static member or calling a non-static member function before the constructor begins execution is UB ([class.cdtor]/1). Although calloc implicitly creates objects of implicit-lifetime types (C++20+, [intro.object]/10, [c.malloc]/5), a non-trivial class type is not an implicit-lifetime type unless it meets specific criteria like being an aggregate or having a trivial eligible constructor and trivial destructor ([basic.types.general], [class.prop]). Non-trivial constructors make the type non-implicit-lifetime, so no object lifetime starts automatically, leading to UB when accessing members or calling functions.

Citations:

• 1: https://stackoverflow.com/questions/37644977/is-circumventing-a-class-constructor-legal-or-does-it-result-in-undefined-behav
• 2: https://stackoverflow.com/questions/58407561/using-calloced-memory-as-a-typed-array-without-ub
• 3: https://stackoverflow.com/questions/47203580/calloc-vs-new-for-complicated-structs-in-various-compilers
• 4: https://timsong-cpp.github.io/cppwp/n4868/c.malloc
• 5: https://timsong-cpp.github.io/cppwp/basic.life
• 6: https://eel.is/c++draft/obj.lifetime
• 7: https://en.cppreference.com/w/cpp/language/lifetime
• 8: https://cpp-lang.net/docs/named_req/implicitlifetimetype/
• 9: https://stackoverflow.com/questions/38807403/using-malloc-realloc-for-array-of-classes-structs-including-std-vector
• 10: https://stackoverflow.com/questions/15943278/calloc-malloc-vs-new-ing-a-struct-that-has-an-unordered-map-inside
• 11: https://stackoverflow.com/questions/63926529/calling-constructor-of-class-member-when-using-malloc
• 12: https://timsong-cpp.github.io/cppwp/class.cdtor
• 13: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0593r6.html
• 14: https://stackoverflow.com/questions/75709434/an-aggregate-is-implicit-lifetime-doesnt-seem-right

---

Replace calloc/reinterpret_cast with proper object construction.

Calling non-static member functions on calloc-allocated memory without invoking a constructor is undefined behavior in C++. For ProxySQL_Admin (a non-trivial class), no object lifetime begins with calloc alone; the constructor must be invoked via placement new or normal allocation. Use RAII-compliant allocation (e.g., placement new on stack storage or standard new) instead.

🧰 Tools

🪛 Cppcheck (2.20.0)

[warning] 58-58: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp` around lines 53 - 66, The
test currently uses calloc + reinterpret_cast to create admin (ProxySQL_Admin)
which bypasses constructors; replace that with proper object construction (e.g.,
admin = new ProxySQL_Admin(); or placement new into appropriately typed storage)
and remove the free(admin) in the destructor, instead use delete admin; ensure
the created SQLite3DB is still assigned to admin->configdb and cleanup deletes
admin->configdb and then delete admin so both constructor and destructor run for
ProxySQL_Admin.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard initialization failure before running the suite

If test_init_minimal() fails at Line 272, the rest of the test body still runs and can touch uninitialized globals. Short-circuit immediately on failure.

Proposed fix

 int main() {
-	plan(62);
 	int rc = test_init_minimal();
-	ok(rc == 0, "test_init_minimal() succeeds");
+	if (rc != 0) {
+		plan(1);
+		ok(false, "test_init_minimal() succeeds");
+		return exit_status();
+	}
+	plan(62);
+	ok(true, "test_init_minimal() succeeds");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/charset_find_unit-t.cpp` around lines 270 - 274, The test
harness should stop if initialization fails: in main() after calling
test_init_minimal() and storing its return in rc, immediately short-circuit
(e.g., return rc or call exit/skip) when rc != 0 instead of continuing; ensure
you do this before any further test code (the existing ok(rc == 0, ...)
assertion can remain but follow it with an early return/exit to avoid touching
uninitialized globals from test_init_minimal().

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Assert CPY3()'s exact little-endian value.

These checks still pass if CPY3() reads the three bytes in the wrong order. Compare against the expected 0x00030201U so the test actually validates the contract described in the comment.

Proposed fix

 static void test_cpy3() {
     unsigned char data[] = {0x01, 0x02, 0x03, 0xFF};
     unsigned int result = CPY3(data);
-    // CPY3 reads 3 bytes little-endian and masks MSB to 0
-    ok((result & 0xFF000000) == 0, "CPY3: MSB is zeroed");
-    ok((result & 0x00FFFFFF) != 0, "CPY3: lower 3 bytes are non-zero");
+    // CPY3 reads 3 bytes little-endian and masks MSB to 0
+    ok(result == 0x00030201U, "CPY3: reads 3 bytes little-endian");
+    ok((result & 0xFF000000U) == 0, "CPY3: MSB is zeroed");
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	static void test_cpy3() {
	unsigned char data[] = {0x01, 0x02, 0x03, 0xFF};
	unsigned int result = CPY3(data);
	// CPY3 reads 3 bytes little-endian and masks MSB to 0
	ok((result & 0xFF000000) == 0, "CPY3: MSB is zeroed");
	ok((result & 0x00FFFFFF) != 0, "CPY3: lower 3 bytes are non-zero");
	static void test_cpy3() {
	unsigned char data[] = {0x01, 0x02, 0x03, 0xFF};
	unsigned int result = CPY3(data);
	// CPY3 reads 3 bytes little-endian and masks MSB to 0
	ok(result == 0x00030201U, "CPY3: reads 3 bytes little-endian");
	ok((result & 0xFF000000U) == 0, "CPY3: MSB is zeroed");
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/mysql_encode_unit-t.cpp` around lines 59 - 64, The test
currently only checks MSB zeroing and non-zero lower bytes for CPY3 in
test_cpy3(), which won't catch byte-order errors; change the assertions to
assert equality against the exact little-endian value 0x00030201U returned by
CPY3(data) (use CPY3(data) == 0x00030201U or equivalent test helper) so the test
verifies the exact 3-byte little-endian result and that the MSB is zero.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In PostgreSQL, after SAVEPOINT sp1; SAVEPOINT sp2; SAVEPOINT sp3; ROLLBACK TO SAVEPOINT sp2;, should sp2still exist, or are only savepoints created beforesp2 kept?

💡 Result:

After SAVEPOINT sp1; SAVEPOINT sp2; SAVEPOINT sp3; ROLLBACK TO SAVEPOINT sp2;, the savepoint sp2 still exists and remains valid. However, sp3 (and any savepoints created after sp2) are implicitly destroyed. Only savepoints created before sp2 (like sp1) and sp2 itself are kept.

Citations:

• 1: https://www.postgresql.org/docs/current/sql-rollback-to.html
• 2: https://www.postgresql.org/docs/current/sql-savepoint.html
• 3: https://www.postgresql.org/docs/current/tutorial-transactions.html

---

🏁 Script executed:

# First, let me check the test file size and structure
wc -l test/tap/tests/unit/pgsql_txn_state_unit-t.cpp

Repository: sysown/proxysql

Length of output: 109

---

🏁 Script executed:

# Now let me examine the context around lines 523-526 to see the full test scenario
# This will show how sp1, sp2, sp3 are created and what the test expects
sed -n '480,530p' test/tap/tests/unit/pgsql_txn_state_unit-t.cpp

Repository: sysown/proxysql

Length of output: 2300

---

Fix savepoint count expectation after ROLLBACK TO.

ROLLBACK TO SAVEPOINT sp2 should preserve sp2 itself along with savepoints created before it (like sp1), removing only savepoints created after sp2 (like sp3). The current test expects count 1, which bakes incorrect behavior into the suite.

Proposed fix

-    // Rollback to sp2 — this should remove sp2 and sp3 state, keeping sp1
+    // Rollback to sp2 — this should remove savepoints after sp2, keeping sp1 and sp2
     bool r1 = mgr->handle_transaction("ROLLBACK TO SAVEPOINT sp2");
     ok(r1 == true, "statemgr: ROLLBACK TO sp2 returns true");
-    ok(mgr->get_savepoint_count() == 1, "statemgr: one savepoint after ROLLBACK TO sp2");
+    ok(mgr->get_savepoint_count() == 2, "statemgr: sp1 and sp2 remain after ROLLBACK TO sp2");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/pgsql_txn_state_unit-t.cpp` around lines 523 - 526, The
test incorrectly expects one savepoint after calling
mgr->handle_transaction("ROLLBACK TO SAVEPOINT sp2"); update the assertion to
expect two savepoints (sp1 and sp2 remain) and any related checks that assume
only one savepoint; specifically modify the ok(mgr->get_savepoint_count() == 1,
...) to assert == 2 so the test reflects that ROLLBACK TO preserves the target
savepoint (sp2) and earlier ones (e.g., sp1) while removing later ones (e.g.,
sp3).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Make the empty SQLi fixture match the real schema.

load_firewall() expects the SQLi whitelist resultset to mirror firewall_whitelist_sqli_fingerprints (active, fingerprint). This one-column fixture only passes because the test never exercises fingerprint lookups, so it can miss loader/schema regressions.

🧪 Fixture fix

-	SQLite3_result *sqli = new SQLite3_result(1);
-	sqli->add_column_definition(SQLITE_TEXT, "fingerprint");
+	SQLite3_result *sqli = new SQLite3_result(2);
+	sqli->add_column_definition(SQLITE_TEXT, "active");
+	sqli->add_column_definition(SQLITE_TEXT, "fingerprint");

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Empty sqli fingerprints
	SQLite3_result *sqli = new SQLite3_result(1);
	sqli->add_column_definition(SQLITE_TEXT, "fingerprint");
	// Empty sqli fingerprints
	SQLite3_result *sqli = new SQLite3_result(2);
	sqli->add_column_definition(SQLITE_TEXT, "active");
	sqli->add_column_definition(SQLITE_TEXT, "fingerprint");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/query_processor_unit-t.cpp` around lines 1360 - 1363, The
empty SQLi fixture in the test uses SQLite3_result with only a "fingerprint"
column which doesn't match the real firewall_whitelist_sqli_fingerprints schema
expected by load_firewall(); update the fixture created via SQLite3_result in
query_processor_unit-t.cpp to include both columns (e.g., an "active" column
type matching the loader's expectation and the "fingerprint" column) in the
correct order and populate rows accordingly so the test mirrors the real table
schema used by load_firewall().

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Reset the fixture between timer tests.

This file shares one ProxySQL_Statistics instance across the whole suite, so timer state leaks between cases. After Line 199 advances the MySQL connections timer, Line 208 no longer starts from a zero timer, and the same pattern shows up in the TSDB timer checks. Recreate or explicitly reset the fixture before each timer test group.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/statistics_unit-t.cpp` around lines 47 - 69, The test
suite is leaking timer state because a single ProxySQL_Statistics instance
(stats) is shared across cases; update the test fixture so timer tests start
from a fresh state by recreating or explicitly resetting the fixture before each
timer test group: either add a teardown + re-run setup_stats() to delete and
reallocate stats (and reassign GloVars.statsdb_disk) or call a reset method on
ProxySQL_Statistics if one exists (e.g., stats->resetTimers() or similar) prior
to advancing timers in the MySQL and TSDB timer tests so each group begins from
zero.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Skip the DB-backed TSDB tests when statsdb_disk is null.

test_constructor_creates_databases() already detects the “clean rebuild needed” path, but the TSDB table/metric/backend/status/downsample tests still run unconditionally here. If stats->statsdb_disk failed to initialize, those paths turn a diagnosable setup problem into a null-dereference crash instead of a skip.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/statistics_unit-t.cpp` around lines 791 - 837, Wrap the
TSDB-related test calls (e.g. test_init_creates_tsdb_tables,
test_set_variable_*, test_tsdb_* timers, test_insert_and_query_tsdb_metric*,
test_insert_and_query_backend_health, test_get_tsdb_status,
test_tsdb_downsample_*, test_tsdb_retention_* ) in a runtime guard that checks
the test fixture's statsdb pointer (stats->statsdb_disk) and skips these tests
when stats->statsdb_disk is null; specifically, before invoking that block, add
a condition like "if (stats->statsdb_disk != nullptr) { ... } else { skip/emit
message }" so the TSDB tests do not run when statsdb failed to initialize.
Ensure you reference and use the existing stats object (stats->statsdb_disk) so
behavior matches test_constructor_creates_databases detection.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/scripts/bin/requirements.txt (1)

1-11: Consider the reproducibility trade-off of switching from pinned to minimum versions.

Changing from == (pinned) to >= (minimum version) for 10 dependencies eliminates build reproducibility. While this allows automatic security updates, it can introduce unexpected behavior changes in CI/test environments when transitive dependencies pull in newer versions.

Consider one of these approaches:

• Use a lock file (e.g., requirements.lock, Pipfile.lock) to maintain reproducibility while allowing controlled updates
• Pin to specific versions in CI while using >= for local development
• Document the testing/validation strategy for dependency updates

Note: The PR summary mentions removing a stale Pipfile.lock, which would have provided this pinning.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/scripts/bin/requirements.txt` around lines 1 - 11, The requirements.txt
change replaced pinned (==) versions with minimum (>=) constraints for the
listed packages in test/scripts/bin/requirements.txt, which breaks build
reproducibility; revert to a reproducible approach by either (a) restoring exact
pins for the listed packages (e.g., use == for certifi, chardet, idna,
packaging, PyMySQL, pyparsing, PyYAML, requests, six, structlog, urllib3) or (b)
reintroducing a lockfile (e.g., requirements.lock or Pipfile.lock) and update CI
to install from that lockfile while keeping >= for local dev; also update the PR
to include the chosen lockfile or document the chosen policy for dependency
updates and CI pinning so CI/test environments remain deterministic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/scripts/bin/requirements.txt`:
- Line 7: requirements.txt currently pins PyYAML>=5.4.1 which conflicts with
requirements-ubuntu24.txt's PyYAML>=6.0; update the PyYAML entry in
requirements.txt (the "PyYAML" requirement line) to match the stricter
constraint (use PyYAML>=6.0) so both files use the same minimum version.

---

Nitpick comments:
In `@test/scripts/bin/requirements.txt`:
- Around line 1-11: The requirements.txt change replaced pinned (==) versions
with minimum (>=) constraints for the listed packages in
test/scripts/bin/requirements.txt, which breaks build reproducibility; revert to
a reproducible approach by either (a) restoring exact pins for the listed
packages (e.g., use == for certifi, chardet, idna, packaging, PyMySQL,
pyparsing, PyYAML, requests, six, structlog, urllib3) or (b) reintroducing a
lockfile (e.g., requirements.lock or Pipfile.lock) and update CI to install from
that lockfile while keeping >= for local dev; also update the PR to include the
chosen lockfile or document the chosen policy for dependency updates and CI
pinning so CI/test environments remain deterministic.

🪄 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: 89aea15d-187e-49b2-bc62-16e942b5e4ca

📥 Commits

Reviewing files that changed from the base of the PR and between cde4cc7 and 0441d50.

⛔ Files ignored due to path filters (1)

• test/scripts/bin/Pipfile.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• test/scripts/bin/Pipfile
• test/scripts/bin/requirements-ubuntu24.txt
• test/scripts/bin/requirements.txt

✅ Files skipped from review due to trivial changes (2)

• test/scripts/bin/Pipfile
• test/scripts/bin/requirements-ubuntu24.txt

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)

• GitHub Check: CI-3p-sqlalchemy / test (mysql8.0,pymysql)
• GitHub Check: CI-3p-postgresql / test (pgsql16,REL_16_4)
• GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.0,mysql)
• GitHub Check: CI-3p-laravel-framework / test (mariadb10.6,mariadb)
• GitHub Check: CI-3p-mariadb-connector-c / test (mariadb10.11,8.0)
• GitHub Check: CI-3p-php-pdo-mysql / test (mysql8.0,mariadb)
• GitHub Check: CI-3p-postgresql / test (pgsql17,REL_16_4)
• GitHub Check: CI-3p-php-pdo-mysql / test (mysql8.4,mysql)
• GitHub Check: CI-3p-pgjdbc / test (pgsql16,REL42.7.7)
• GitHub Check: CI-3p-php-pdo-pgsql / test (pgsql17,pgsql)
• GitHub Check: CI-CodeQL / analyze (cpp)
• GitHub Check: CI-3p-postgresql / test (pgsql17,REL_17_0)
• GitHub Check: CI-3p-sqlalchemy / test (pgsql17,psycopg2)
• GitHub Check: CI-3p-sqlalchemy / test (pgsql16,psycopg2)
• GitHub Check: CI-3p-mysql-connector-j / test (mysql8.4,8.0.33)
• GitHub Check: CI-3p-pgjdbc / test (pgsql17,REL42.7.7)
• GitHub Check: CI-3p-php-pdo-mysql / test (mysql9.0,mariadb)
• GitHub Check: CI-3p-postgresql / test (pgsql16,REL_17_0)
• GitHub Check: CI-3p-php-pdo-pgsql / test (pgsql16,pgsql)
• GitHub Check: claude-review

🔇 Additional comments (2)

test/scripts/bin/requirements.txt (2)

11-11: No compatibility concerns. Requests 2.33.0 explicitly supports urllib3 2.x versions (requires urllib3 >=1.26, <3), so urllib3 2.6.3 is fully compatible and no action is needed.

---

5-5: > Likely an incorrect or invalid review comment.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Inconsistent PyYAML minimum version across requirements files.

requirements.txt specifies PyYAML>=5.4.1, but requirements-ubuntu24.txt:7 specifies PyYAML>=6.0. This inconsistency can lead to environment-specific behavior differences between local development and Ubuntu 24 CI environments.

Consider aligning both files to use the same minimum version (preferably >=6.0 to match the more restrictive constraint).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/scripts/bin/requirements.txt` at line 7, requirements.txt currently pins
PyYAML>=5.4.1 which conflicts with requirements-ubuntu24.txt's PyYAML>=6.0;
update the PyYAML entry in requirements.txt (the "PyYAML" requirement line) to
match the stricter constraint (use PyYAML>=6.0) so both files use the same
minimum version.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp (2)

53-73: ⚠️ Potential issue | 🟠 Major

Avoid UB from calling methods on a non-constructed ProxySQL_Admin.

This still relies on calloc + reinterpret_cast and then invokes non-static members without starting object lifetime. That is undefined behavior in C++, even in tests.

In C++17, is it valid to call non-static member functions on memory allocated with calloc and cast to a class pointer, without running the class constructor first?

As per coding guidelines, "**/*.cpp: Use RAII for resource management and jemalloc for memory allocation".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp` around lines 53 - 73, The
test currently causes UB by calloc+reinterpret_cast to ProxySQL_Admin and
calling its members; instead, replace that with a test-only POD shim (e.g.,
struct TestProxyAdmin { SQLite3DB* configdb; };) and use RAII/new-delete (or
std::unique_ptr) to manage it in TestDiskUpgrade, updating the TestDiskUpgrade
constructor/destructor to allocate TestProxyAdmin with new and set its configdb
= db_, and delete it in the destructor; do not instantiate ProxySQL_Admin via
raw calloc/reinterpret_cast—if you must use the real class and its constructor
is safe to run in tests, allocate it with new (or placement-new to call
constructor) rather than calloc; also follow allocation guidelines (use operator
new / jemalloc as per project policy) and replace free with delete.

---

60-64: ⚠️ Potential issue | 🟡 Minor

Add failure guards for calloc and SQLite open before dereferencing.

admin and db_ are used immediately with no allocation/open-result checks. On failure, this can crash the test binary before assertions run.

Suggested hardening patch

 	void *mem = calloc(1, sizeof(ProxySQL_Admin));
-	admin = reinterpret_cast<ProxySQL_Admin*>(mem);
+	if (!mem) {
+		admin = nullptr;
+		return;
+	}
+	admin = reinterpret_cast<ProxySQL_Admin*>(mem);
 	SQLite3DB *db_ = new SQLite3DB();
-	db_->open((char *)":memory:", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX);
-	admin->configdb = db_;
+	if (!db_ || db_->open((char *)":memory:", SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX) != 0) {
+		delete db_;
+		free(admin);
+		admin = nullptr;
+		return;
+	}
+	admin->configdb = db_;
 }
 
 ~TestDiskUpgrade() {
-	if (admin->configdb) {
+	if (admin && admin->configdb) {
 		delete admin->configdb;
 		admin->configdb = nullptr;
 	}
-	free(admin);
+	if (admin) free(admin);
 }

Also applies to: 67-72

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp` around lines 60 - 64, The
code dereferences the result of calloc for ProxySQL_Admin (mem/admin) and the
result of SQLite3DB::open (db_) without checking for failures; add guards
immediately after calloc and after db_->open to detect allocation or open
errors, free/delete any partially allocated resources (free(mem) or delete db_)
and fail the test/return cleanly instead of continuing; update the blocks that
set admin = reinterpret_cast<ProxySQL_Admin*>(mem) and admin->configdb = db_ to
only run after those checks, referencing ProxySQL_Admin, calloc, admin,
SQLite3DB, db_, and SQLite3DB::open to locate the code to change.

🧹 Nitpick comments (1)

test/tap/tests/unit/sqlite3db_unit-t.cpp (1)

42-58: Consider verifying INSERT success for clearer failure diagnosis.

The INSERT statements on lines 43-44 don't check the return value of execute(). If they fail, the test would report "COUNT(*) != 2" rather than the actual root cause. This is a minor concern for test maintainability.

💡 Optional: Check execute() return values

 static void test_execute_insert_select() {
-    db->execute("INSERT INTO test_t (id, name) VALUES (1, 'alice')");
-    db->execute("INSERT INTO test_t (id, name) VALUES (2, 'bob')");
+    bool rc1 = db->execute("INSERT INTO test_t (id, name) VALUES (1, 'alice')");
+    bool rc2 = db->execute("INSERT INTO test_t (id, name) VALUES (2, 'bob')");
+    ok(rc1 && rc2, "execute: INSERT statements succeed");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/sqlite3db_unit-t.cpp` around lines 42 - 58, The test
test_execute_insert_select() currently calls db->execute(...) for the two
INSERTs without checking their return status, so if an INSERT fails the later
COUNT check misleads; update the test to capture and assert the return value (or
error) from db->execute for each INSERT (e.g., check the boolean/return code
from execute and call ok(...) or fail early) before running
db->execute_statement("SELECT COUNT(*)...") so failures in db->execute are
reported immediately and clearly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp`:
- Around line 97-102: The test test_malformed_bearer_prefix_lowercase
contradicts the implementation of MCP_JSONRPC_Resource::validate_bearer_token
which uses strncasecmp() (case-insensitive per RFC 7235); update the test to
expect true instead of false: change the ok(...) assertion in
test_malformed_bearer_prefix_lowercase to assert true for input "bearer mytoken"
and adjust the test message to reflect that lowercase 'bearer' prefix is
accepted.

---

Duplicate comments:
In `@test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp`:
- Around line 53-73: The test currently causes UB by calloc+reinterpret_cast to
ProxySQL_Admin and calling its members; instead, replace that with a test-only
POD shim (e.g., struct TestProxyAdmin { SQLite3DB* configdb; };) and use
RAII/new-delete (or std::unique_ptr) to manage it in TestDiskUpgrade, updating
the TestDiskUpgrade constructor/destructor to allocate TestProxyAdmin with new
and set its configdb = db_, and delete it in the destructor; do not instantiate
ProxySQL_Admin via raw calloc/reinterpret_cast—if you must use the real class
and its constructor is safe to run in tests, allocate it with new (or
placement-new to call constructor) rather than calloc; also follow allocation
guidelines (use operator new / jemalloc as per project policy) and replace free
with delete.
- Around line 60-64: The code dereferences the result of calloc for
ProxySQL_Admin (mem/admin) and the result of SQLite3DB::open (db_) without
checking for failures; add guards immediately after calloc and after db_->open
to detect allocation or open errors, free/delete any partially allocated
resources (free(mem) or delete db_) and fail the test/return cleanly instead of
continuing; update the blocks that set admin =
reinterpret_cast<ProxySQL_Admin*>(mem) and admin->configdb = db_ to only run
after those checks, referencing ProxySQL_Admin, calloc, admin, SQLite3DB, db_,
and SQLite3DB::open to locate the code to change.

---

Nitpick comments:
In `@test/tap/tests/unit/sqlite3db_unit-t.cpp`:
- Around line 42-58: The test test_execute_insert_select() currently calls
db->execute(...) for the two INSERTs without checking their return status, so if
an INSERT fails the later COUNT check misleads; update the test to capture and
assert the return value (or error) from db->execute for each INSERT (e.g., check
the boolean/return code from execute and call ok(...) or fail early) before
running db->execute_statement("SELECT COUNT(*)...") so failures in db->execute
are reported immediately and clearly.

🪄 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: d9f5d52f-98ec-4377-8a5c-8f516b59f4cf

📥 Commits

Reviewing files that changed from the base of the PR and between 4fb7aac and acaa100.

📒 Files selected for processing (6)

• include/Stats_Tool_Handler.h
• lib/MCP_Endpoint.cpp
• test/tap/groups/check_groups.py
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp
• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• lib/MCP_Endpoint.cpp
• test/tap/groups/check_groups.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: CI-builds / builds (debian12,-dbg)
• GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
• GitHub Check: CI-builds / builds (ubuntu22,-tap)
• GitHub Check: run / trigger
• GitHub Check: claude-review

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{cpp,h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via #ifdef PROXYSQLGENAI, #ifdef PROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters

Files:

• include/Stats_Tool_Handler.h
• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

include/**/*.{h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

Include guards must follow the pattern #ifndef _CLASS*_H

Files:

• include/Stats_Tool_Handler.h

**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Use RAII for resource management and jemalloc for memory allocation

Files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

test/tap/tests/unit/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness

Files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

🧠 Learnings (7)

📓 Common learnings

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness

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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.

📚 Learning: 2026-03-22T14:38:16.093Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags

Applied to files:

• include/Stats_Tool_Handler.h
• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

📚 Learning: 2026-03-22T14:38:16.093Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

📚 Learning: 2026-02-13T09:29:39.713Z

Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

📚 Learning: 2026-03-22T14:38:16.093Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

📚 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/unit/genai_mcp_endpoint_unit-t.cpp
• test/tap/tests/unit/sqlite3db_unit-t.cpp
• test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

📚 Learning: 2026-01-20T07:40:34.938Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

🪛 Clang (14.0.6)

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

[error] 13-13: 'tap.h' file not found

(clang-diagnostic-error)

test/tap/tests/unit/sqlite3db_unit-t.cpp

[error] 12-12: 'tap.h' file not found

(clang-diagnostic-error)

test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

[error] 25-25: 'tap.h' file not found

(clang-diagnostic-error)

🪛 Cppcheck (2.20.0)

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

test/tap/tests/unit/sqlite3db_unit-t.cpp

[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp

[error] 149-149: Common realloc mistake

(memleakOnRealloc)

---

[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 64-64: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

🔇 Additional comments (15)

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp (6)

1-24: LGTM!

The file header, documentation, and includes are well-structured. The #ifdef PROXYSQLGENAI guard correctly wraps the MCP-specific includes. As per coding guidelines, the test properly includes test_globals.h and test_init.h.

---

31-62: LGTM!

The MCP_Endpoint_Test friend accessor class cleanly exposes private methods for unit testing. The static helper pattern is appropriate and aligns with the friend class MCP_Endpoint_Test; declaration in include/MCP_Endpoint.h.

---

104-176: LGTM!

The remaining bearer token validation tests comprehensively cover edge cases including:

• Missing space after "Bearer"
• Wrong authentication scheme
• Empty/whitespace-only tokens
• Whitespace trimming behavior
• Case-sensitive token comparison
• Long tokens

All test expectations align with the implementation logic.

---

182-222: LGTM!

The JSON-RPC response tests properly validate:

• JSON-RPC 2.0 version field
• ID preservation for string, numeric, and omission for null IDs
• Result content serialization

Good coverage of the JSON-RPC 2.0 specification requirements.

---

228-282: LGTM!

The JSON-RPC error tests comprehensively cover:

• Standard JSON-RPC 2.0 error codes (-32700, -32601, -32600)
• Custom application error codes (-32001)
• ID handling in error responses (string, numeric, null/omitted)
• Error object structure validation

---

286-326: LGTM!

The main function correctly:

• Plans 36 assertions matching the actual test count (15 bearer + 9 response + 12 error)
• Uses test_init_minimal() and test_cleanup_minimal() per test harness requirements
• Provides graceful skip when PROXYSQLGENAI is not enabled
• Returns exit_status() per TAP convention

include/Stats_Tool_Handler.h (3)

6-6: Nice fix: explicit <cstdint> include.

This makes the header self-contained for uint64_t usage in public declarations.

---

14-40: stats_utils extraction is clean and consistent.

Declarations match the implementation namespace and improve symbol isolation for parsing helpers.

---

258-280: Public exposure of utility methods looks intentional and appropriate.

Making get_interval_config(...) and calculate_percentile(...) public aligns with testability goals and is consistent with this PR’s expanded unit testing scope.

test/tap/tests/unit/admin_disk_upgrade_unit-t.cpp (1)

118-543: Nice coverage breadth for upgrade/no-op/data-preservation paths.

The test matrix is solid: versioned upgrades, no-op cases, rename checks, and multi-row invariants are all exercised with clear assertions.

Also applies to: 549-587

test/tap/tests/unit/sqlite3db_unit-t.cpp (5)

12-35: Test harness and setup look good.

Proper inclusion of test_globals.h and test_init.h as required, and clean setup/teardown pattern. The static analysis warning about null pointer after new (line 31) is a false positive since C++ new throws std::bad_alloc on failure rather than returning nullptr.

Minor robustness suggestion: consider verifying db->open() succeeded before proceeding, though for :memory: this is unlikely to fail.

---

83-106: Good edge case coverage; const-correctness is a known API limitation.

The (char*) casts from string literals are const-incorrect but appear necessary due to the SQLite3DB API signatures. The static analysis warning on line 86 is a false positive—these are casts on string literals, not memory allocations. Test coverage for match/mismatch/nonexistent scenarios is thorough.

---

108-159: Comprehensive coverage of table build operations.

Good test design covering both drop=false and drop=true paths for build_table(), and verifying idempotency of check_and_build_table(). The unchecked execute() calls on lines 127 and 152 follow the same pattern noted earlier.

---

165-194: Proper null guards and reasonable lock sanity tests.

The test_sqlite3_row_get_size correctly guards against null results before accessing row data. The lock tests appropriately verify no crash/deadlock without overcomplicating with concurrency verification.

---

196-221: Well-structured main with correct assertion count.

The plan(21) matches the actual assertion count across all test functions. Proper use of test_init_minimal() and test_cleanup_minimal() per the unit test harness requirements. Based on learnings, unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h — this file is compliant.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp (1)

31-31: Rename test helper class to match repository prefix convention.

MCP_Endpoint_Test does not match the required protocol prefix set for C++ class names in this repo. Consider renaming this helper (and its friend declaration) to a ProxySQL_... class name.

As per coding guidelines, **/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp` at line 31, Rename the
test helper class MCP_Endpoint_Test to use the repository's protocol prefix
(e.g., ProxySQL_MCP_Endpoint_Test) and update any corresponding friend
declarations and usages to the new name; specifically change the class
declaration named MCP_Endpoint_Test and any "friend" lines or test references
that mention MCP_Endpoint_Test to the new ProxySQL_* PascalCase identifier so
the class name follows the MySQL_/PgSQL_/ProxySQL_ prefix convention.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp`:
- Around line 210-221: The tests test_jsonrpc_response_with_null_id and the
similar block around the later assertions assume the "id" key is omitted when id
is null, but MCP_Endpoint_Test::call_create_jsonrpc_response (and the underlying
helper that sets response["id"] = id) currently serializes "id": null; update
the tests to expect the presence of "id" with a null value instead of absence —
e.g., assert parsed.contains("id") and parsed["id"].is_null() (or equivalent)
for both test_jsonrpc_response_with_null_id and the other test block that checks
null id behavior so they match the helper output.
- Around line 133-153: The two unit tests test_token_with_leading_whitespace and
test_token_with_trailing_whitespace assume validate_bearer_token
(MCP_JSONRPC_Resource::validate_bearer_token) trims internal whitespace inside
the token substring, but the implementation only trims the overall header and
then takes trimmed.substr(7) without removing interior leading/trailing spaces;
update those tests to expect the preserved spaces (e.g., for "Bearer   mytoken"
expect "  mytoken" and for "Bearer mytoken   " expect "mytoken   ") so the
assertions match the current behavior of validate_bearer_token.

---

Nitpick comments:
In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp`:
- Line 31: Rename the test helper class MCP_Endpoint_Test to use the
repository's protocol prefix (e.g., ProxySQL_MCP_Endpoint_Test) and update any
corresponding friend declarations and usages to the new name; specifically
change the class declaration named MCP_Endpoint_Test and any "friend" lines or
test references that mention MCP_Endpoint_Test to the new ProxySQL_* PascalCase
identifier so the class name follows the MySQL_/PgSQL_/ProxySQL_ prefix
convention.

🪄 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: ef39c0a1-8c28-417c-b6f7-e53fe3982e8c

📥 Commits

Reviewing files that changed from the base of the PR and between acaa100 and 6d6015a.

📒 Files selected for processing (1)

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

📜 Review details

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{cpp,h,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{cpp,h,hpp}: Class names must use PascalCase with protocol prefixes (MySQL_, PgSQL_, ProxySQL_)
Member variables must use snake_case
Constants and macros must use UPPER_SNAKE_CASE
C++17 is required; use conditional compilation via #ifdef PROXYSQLGENAI, #ifdef PROXYSQL31, etc. for feature flags
Use pthread mutexes for synchronization and std::atomic<> for counters

Files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Use RAII for resource management and jemalloc for memory allocation

Files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

test/tap/tests/unit/**/*.cpp

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness

Files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

🧠 Learnings (6)

📚 Learning: 2026-03-22T14:38:16.093Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests in test/tap/tests/unit/ must use test_globals.h and test_init.h and link against libproxysql.a via the custom test harness

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

📚 Learning: 2026-02-13T09:29:39.713Z

Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5372
File: test/tap/tap/mcp_client.cpp:355-385
Timestamp: 2026-02-13T09:29:39.713Z
Learning: In ProxySQL MCP implementation (test/tap/tap/mcp_client.cpp), the `check_server()` method uses the ping endpoint which is designed to work without authentication. The `ping` method at the `config` endpoint should not require the `Authorization: Bearer` header, unlike tool invocation endpoints which do require authentication when `auth_token_` is set.

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

📚 Learning: 2026-03-22T14:38:16.093Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef` PROXYSQLGENAI, `#ifdef` PROXYSQL31, etc. for feature flags

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

📚 Learning: 2026-03-22T14:38:16.093Z

Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-22T14:38:16.093Z
Learning: Applies to test/tap/tests/{test_*,*-t}.cpp : Test files in test/tap/tests/ must follow the naming pattern test_*.cpp or *-t.cpp

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

📚 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/unit/genai_mcp_endpoint_unit-t.cpp

📚 Learning: 2026-01-20T07:40:34.938Z

Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).

Applied to files:

• test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

🪛 Clang (14.0.6)

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

[error] 13-13: 'tap.h' file not found

(clang-diagnostic-error)

🪛 Cppcheck (2.20.0)

test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp

[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

---

[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference

(nullPointerOutOfMemory)

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Whitespace-after-Bearer expectations are inverted in two tests.

On Lines 133-153, validate_bearer_token() currently does not trim whitespace inside the token substring (trimmed.substr(7)), so these two assertions should fail against lib/MCP_Endpoint.cpp.

Proposed fix (align tests with current implementation)

 static void test_token_with_leading_whitespace() {
-	// Token with leading spaces should be trimmed and still match
+	// Extra spaces after "Bearer " are part of the token and should fail
 	bool result = MCP_JSONRPC_Resource::validate_bearer_token(
 		"Bearer   mytoken", "mytoken"
 	);
-	ok(result == true, "validate_bearer_token: leading whitespace trimmed, token matches");
+	ok(result == false, "validate_bearer_token: token with extra leading spaces rejected");
 }
@@
 static void test_token_with_surrounding_whitespace() {
 	bool result = MCP_JSONRPC_Resource::validate_bearer_token(
 		"Bearer  \t mytoken \t  ", "mytoken"
 	);
-	ok(result == true, "validate_bearer_token: surrounding whitespace trimmed, token matches");
+	ok(result == false, "validate_bearer_token: internal token whitespace rejected");
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp` around lines 133 - 153,
The two unit tests test_token_with_leading_whitespace and
test_token_with_trailing_whitespace assume validate_bearer_token
(MCP_JSONRPC_Resource::validate_bearer_token) trims internal whitespace inside
the token substring, but the implementation only trims the overall header and
then takes trimmed.substr(7) without removing interior leading/trailing spaces;
update those tests to expect the preserved spaces (e.g., for "Bearer   mytoken"
expect "  mytoken" and for "Bearer mytoken   " expect "mytoken   ") so the
assertions match the current behavior of validate_bearer_token.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

null ID assertions conflict with current JSON-RPC helper behavior.

On Lines 210-221 and Lines 257-269, tests assert "id" is omitted, but both helper implementations set response["id"] = id unconditionally, which serializes as "id": null.

Proposed fix (align tests with helper output)

 static void test_jsonrpc_response_with_null_id() {
@@
 	json parsed = json::parse(result);
 	ok(parsed["jsonrpc"] == "2.0", "create_jsonrpc_response: jsonrpc version is 2.0 (null id)");
-	ok(!parsed.contains("id"), "create_jsonrpc_response: null id is omitted from response");
+	ok(parsed.contains("id") && parsed["id"].is_null(),
+		"create_jsonrpc_response: null id is present and set to null");
 	ok(parsed["result"]["ok"] == true, "create_jsonrpc_response: result ok is true");
 }
@@
 static void test_jsonrpc_error_null_id() {
@@
 	json parsed = json::parse(result);
 	ok(parsed["jsonrpc"] == "2.0", "create_jsonrpc_error: jsonrpc 2.0 with null id");
 	ok(parsed["error"]["code"] == -32600, "create_jsonrpc_error: error code is -32600");
-	ok(!parsed.contains("id"), "create_jsonrpc_error: null id is omitted from error response");
+	ok(parsed.contains("id") && parsed["id"].is_null(),
+		"create_jsonrpc_error: null id is present and set to null");
 }

Also applies to: 257-269

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/tap/tests/unit/genai_mcp_endpoint_unit-t.cpp` around lines 210 - 221,
The tests test_jsonrpc_response_with_null_id and the similar block around the
later assertions assume the "id" key is omitted when id is null, but
MCP_Endpoint_Test::call_create_jsonrpc_response (and the underlying helper that
sets response["id"] = id) currently serializes "id": null; update the tests to
expect the presence of "id" with a null value instead of absence — e.g., assert
parsed.contains("id") and parsed["id"].is_null() (or equivalent) for both
test_jsonrpc_response_with_null_id and the other test block that checks null id
behavior so they match the helper output.

[sonarqubecloud]

Quality Gate failed

Failed conditions
60 Security Hotspots
E Security Rating on New Code (required ≥ A)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE