Add config query, protobuf vendoring, and RAG source fetch#5752
Conversation
Ignore the extracted protobuf source tree under deps/protobuf so the repository only tracks the vendored source archive. This keeps the new protobuf 3.21.12 dependency self-contained for mysqlx builds on Linux and macOS without polluting git status after the archive is unpacked by the deps Makefile.
Add end-user documentation for the /mcp/config query tool, including the SQL allowlist/denylist model, the single-statement requirement, and the structured response fields returned by the handler. Expand the TAP coverage to verify allowed SELECT/WITH/UPDATE paths, explicit rejection of PRAGMA/DDL/ATTACH, and false-positive resistance when forbidden tokens appear inside strings or comments. This keeps the config endpoint useful for controlled admin writes while preventing multi-statement and dangerous SQL execution.
Replace the rag.fetch_from_source stub with a real backend read path for MySQL and PostgreSQL sources. The handler now looks up document metadata from the RAG vector database, resolves the configured source backend, validates identifiers, builds a safe source query, and returns a per-document result row that includes either the authoritative source row or a fetch error. Unsupported backend types are rejected explicitly, and the source query is capped to a single-row result. Add a focused unit test that seeds the RAG metadata tables, exercises the tool entry point, and verifies backend failure propagation plus missing-document handling without requiring a live external database. Also document /mcp/rag and the fetch semantics in the MCP architecture guide.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVendored Protobuf build and .gitignore rule added; plugin build/link and test wiring updated to use vendored Protobuf. Added constrained ChangesProtobuf vendoring & build/link wiring
GenAI Config Query tool & RAG source refetch
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Handler as Config_Tool_Handler
participant AdminDB as ProxySQL Admin (SQLite)
Client->>Handler: POST { tool: "query", sql: "..." }
Handler->>Handler: validate_config_sql(sql)
Handler->>AdminDB: execute_statement(sql) (under sql_query_global_mutex)
AdminDB-->>Handler: resultset / error
Handler-->>Client: JSON { sql, rows_affected, row_count, columns, rows }
sequenceDiagram
participant Client as MCP Client
participant RAG as /mcp/rag Handler
participant Catalog as Local RAG catalog (SQLite)
participant Backend as External DB (MySQL/Postgres)
Client->>RAG: POST { tool: "rag.fetch_from_source", doc_ids: [...] }
loop per doc_id
RAG->>Catalog: fetch_rag_source_config(doc_id)
Catalog-->>RAG: RagSourceConfig (backend_type, pk_json, conn info)
RAG->>Backend: SELECT ... WHERE pk_predicate (validated/quoted)
Backend-->>RAG: row / error
RAG-->>Client: append per-doc row { doc_id, row | error }
end
RAG-->>Client: final envelope { rows, truncated?, stats }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a server-enforced SQL query tool to the /mcp/config endpoint, allowing constrained DML and SELECT operations against the ProxySQL admin database. It also implements the rag.fetch_from_source tool in the /mcp/rag endpoint, enabling retrieval of authoritative rows from MySQL and PostgreSQL backends. Additionally, Protobuf 3.21.12 is now vendored to remove external dependencies, and several fixes were applied for macOS compatibility. Feedback focuses on critical security concerns regarding unrestricted write access to sensitive admin tables and potential SQL injection vulnerabilities in the RAG and config query paths. There are also concerns regarding resource management, specifically potential mutex deadlocks and memory leaks during exception handling, as well as performance inefficiencies in the RAG connection logic.
| json Config_Tool_Handler::handle_query(const std::string& sql) { | ||
| if (!GloAdmin || !GloAdmin->admindb) { | ||
| return create_error_response("ProxySQL Admin database not available"); | ||
| } | ||
|
|
||
| const std::string validation_error = validate_config_sql(GloAdmin->admindb, sql); | ||
| if (!validation_error.empty()) { | ||
| return create_error_response(validation_error); | ||
| } | ||
|
|
||
| if (pthread_mutex_lock(&GloAdmin->sql_query_global_mutex) != 0) { | ||
| return create_error_response("Failed to lock sql_query_global_mutex"); | ||
| } | ||
|
|
||
| char* error = NULL; | ||
| int cols = 0; | ||
| int affected_rows = 0; | ||
| SQLite3_result* resultset = NULL; | ||
| GloAdmin->admindb->execute_statement(sql.c_str(), &error, &cols, &affected_rows, &resultset); | ||
|
|
||
| int unlock_rc = pthread_mutex_unlock(&GloAdmin->sql_query_global_mutex); | ||
| if (unlock_rc != 0) { | ||
| if (error) { | ||
| std::string err_msg = error; | ||
| free(error); | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response(err_msg + "; also failed to unlock sql_query_global_mutex"); | ||
| } | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response("Failed to unlock sql_query_global_mutex"); | ||
| } | ||
|
|
||
| if (error) { | ||
| std::string err_msg = error; | ||
| free(error); | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response(err_msg); | ||
| } | ||
|
|
||
| json payload; | ||
| payload["sql"] = sql; | ||
| payload["rows_affected"] = affected_rows; | ||
| payload["row_count"] = resultset ? resultset->rows_count : 0; | ||
| payload["columns"] = json::array(); | ||
| if (resultset) { | ||
| for (const auto* column : resultset->column_definition) { | ||
| payload["columns"].push_back(column ? column->name : ""); | ||
| } | ||
| payload["rows"] = resultset_to_json(resultset, cols); | ||
| delete resultset; | ||
| } else { | ||
| payload["rows"] = json::array(); | ||
| } | ||
|
|
||
| return create_success_response(payload); | ||
| } |
There was a problem hiding this comment.
The query tool allows unrestricted UPDATE, DELETE, INSERT, and REPLACE operations on any table in the ProxySQL admin database. This is a significant security risk as an MCP client (or an LLM acting on its behalf) could modify sensitive tables like users, mysql_users, or rag_sources to escalate privileges or disable security features. Consider implementing a table allowlist or restricting write operations to non-sensitive configuration tables.
| if (pthread_mutex_lock(&GloAdmin->sql_query_global_mutex) != 0) { | ||
| return create_error_response("Failed to lock sql_query_global_mutex"); | ||
| } | ||
|
|
||
| char* error = NULL; | ||
| int cols = 0; | ||
| int affected_rows = 0; | ||
| SQLite3_result* resultset = NULL; | ||
| GloAdmin->admindb->execute_statement(sql.c_str(), &error, &cols, &affected_rows, &resultset); | ||
|
|
||
| int unlock_rc = pthread_mutex_unlock(&GloAdmin->sql_query_global_mutex); |
There was a problem hiding this comment.
Similar to the handler_lock, GloAdmin->sql_query_global_mutex is locked manually and could be leaked if an exception occurs during execute_statement. Since this function is called within a try-catch block in execute_tool, an exception here will cause a permanent deadlock on the global admin database mutex.
| std::string escaped_doc_id = doc_id; | ||
| size_t pos = 0; | ||
| while ((pos = escaped_doc_id.find("'", pos)) != std::string::npos) { | ||
| escaped_doc_id.replace(pos, 1, "''"); | ||
| pos += 2; | ||
| } | ||
|
|
||
| std::string sql = | ||
| "SELECT d.source_id, s.name, s.backend_type, s.backend_host, s.backend_port, " | ||
| "s.backend_user, s.backend_pass, s.backend_db, s.table_name, s.pk_column, s.where_sql, d.pk_json " | ||
| "FROM rag_documents d " | ||
| "JOIN rag_sources s ON s.source_id = d.source_id " | ||
| "WHERE d.doc_id = '" + escaped_doc_id + "' AND d.deleted = 0 AND s.enabled = 1"; |
There was a problem hiding this comment.
Manual SQL escaping for doc_id using string replacement is fragile and prone to SQL injection vulnerabilities. You should use the execute_parameterized_query function already defined in this class (line 724) to safely pass doc_id as a parameter.
|
|
||
| std::string sql = "SELECT " + select_list + " FROM " + quote_mysql_identifier(cfg.table_name) + " WHERE " + where_clause; | ||
| if (!cfg.where_sql.empty()) { | ||
| sql += " AND (" + cfg.where_sql + ")"; |
There was a problem hiding this comment.
The cfg.where_sql value is concatenated directly into the SQL query. While this value comes from the admin database, it can be modified by the query tool in Config_Tool_Handler. This allows a potential SQL injection on the backend database if an attacker can manipulate the rag_sources table. Ensure that where_sql is either strictly validated or that the query tool is restricted from modifying sensitive RAG metadata.
| payload["rows"] = resultset_to_json(resultset, cols); | ||
| delete resultset; |
There was a problem hiding this comment.
If resultset_to_json throws an exception, the resultset pointer will be leaked. Ensure that resultset is deleted in case of an exception.
| for (const auto& doc_id : doc_ids) { | ||
| if (rows.size() >= static_cast<size_t>(max_rows) || total_bytes >= max_bytes) { | ||
| truncated = true; | ||
| break; | ||
| } | ||
|
|
||
| RagSourceConfig cfg; | ||
| std::string error; | ||
| if (!fetch_rag_source_config(vector_db, doc_id, cfg, error)) { | ||
| json result_row; | ||
| result_row["doc_id"] = doc_id; | ||
| result_row["source_name"] = source_name; | ||
|
|
||
| // Parse pk_json to get the primary key value | ||
| try { | ||
| json pk_data = json::parse(pk_json); | ||
| json row_data = json::object(); | ||
|
|
||
| // If specific columns are requested, only include those | ||
| if (!columns.empty()) { | ||
| for (const std::string& col : columns) { | ||
| // For demo purposes, we'll just echo back some mock data | ||
| if (col == "Id" && pk_data.contains("Id")) { | ||
| row_data["Id"] = pk_data["Id"]; | ||
| } else if (col == pk_column) { | ||
| // This would be the actual primary key value | ||
| row_data[col] = "mock_value"; | ||
| } else { | ||
| // For other columns, provide mock data | ||
| row_data[col] = "mock_" + col + "_value"; | ||
| } | ||
| } | ||
| } else { | ||
| // If no columns specified, include basic info | ||
| row_data["Id"] = pk_data.contains("Id") ? pk_data["Id"] : json(0); | ||
| row_data[pk_column] = "mock_pk_value"; | ||
| } | ||
| result_row["error"] = error; | ||
| result_row["source_name"] = ""; | ||
| std::string row_str = result_row.dump(); | ||
| if (total_bytes + static_cast<int>(row_str.length()) > max_bytes) { | ||
| truncated = true; | ||
| break; | ||
| } | ||
| total_bytes += static_cast<int>(row_str.length()); | ||
| rows.push_back(result_row); | ||
| continue; | ||
| } | ||
|
|
||
| result_row["row"] = row_data; | ||
| json fetched_rows = json::array(); | ||
| bool ok = false; | ||
| if (cfg.backend_type == "mysql") { | ||
| ok = fetch_rag_rows_mysql(cfg, doc_id, columns, fetched_rows, error); | ||
| } else if (cfg.backend_type == "pgsql" || cfg.backend_type == "postgres" || cfg.backend_type == "postgresql") { | ||
| ok = fetch_rag_rows_pgsql(cfg, doc_id, columns, fetched_rows, error); | ||
| } else { | ||
| error = "Unsupported backend type: " + cfg.backend_type; | ||
| } | ||
|
|
||
| // Check size limits | ||
| std::string row_str = result_row.dump(); | ||
| if (total_bytes + static_cast<int>(row_str.length()) > max_bytes) { | ||
| truncated = true; | ||
| break; | ||
| } | ||
| json result_row; | ||
| result_row["doc_id"] = doc_id; | ||
| result_row["source_id"] = cfg.source_id; | ||
| result_row["source_name"] = cfg.source_name; | ||
| result_row["backend_type"] = cfg.backend_type; | ||
|
|
||
| total_bytes += static_cast<int>(row_str.length()); | ||
| rows.push_back(result_row); | ||
| } catch (...) { | ||
| // Skip malformed pk_json | ||
| continue; | ||
| } | ||
| } else if (rows.size() >= static_cast<size_t>(max_rows) || total_bytes >= max_bytes) { | ||
| if (!ok) { | ||
| result_row["error"] = error; | ||
| } else { | ||
| result_row["row"] = fetched_rows[0]; | ||
| } | ||
|
|
||
| std::string row_str = result_row.dump(); | ||
| if (total_bytes + static_cast<int>(row_str.length()) > max_bytes) { | ||
| truncated = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| delete doc_result; | ||
| total_bytes += static_cast<int>(row_str.length()); | ||
| rows.push_back(result_row); | ||
| } |
There was a problem hiding this comment.
The rag.fetch_from_source implementation opens and closes a new database connection for every single doc_id requested. This is extremely inefficient for large batches of documents, especially when multiple documents belong to the same source. Consider grouping doc_ids by source and reusing connections, or implementing a connection cache.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
deps/Makefile (1)
49-51: 💤 Low valueOptional: fold the new
protobufblock into the existingPROXYSQL40=1cascade.There are now two adjacent
ifeq ($(PROXYSQL40),1)blocks — one forprotobufhere and one forsqlite-vecat lines 57-59 (with thePROXYSQLCLICKHOUSEblock in between). Coalescing them keeps the v4.0-gated targets in one place and avoids future drift.♻️ Proposed grouping
-ifeq ($(PROXYSQL40),1) - targets += protobuf -endif - ifeq ($(PROXYSQLCLICKHOUSE),1) targets += clickhouse-cpp endif ifeq ($(PROXYSQL40),1) + targets += protobuf targets += sqlite-vec endif🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deps/Makefile` around lines 49 - 51, There are two adjacent conditionals checking PROXYSQL40 (one adding target "protobuf" and another adding "sqlite-vec") separated by a PROXYSQLCLICKHOUSE block; merge them by removing the separate ifeq ($(PROXYSQL40),1) that adds "protobuf" and instead add "protobuf" to the same targets += line or block that currently adds "sqlite-vec" under the ifeq ($(PROXYSQL40),1) clause so both protobuf and sqlite-vec are declared inside a single PROXYSQL40 gated block (leave PROXYSQLCLICKHOUSE untouched).doc/MCP/Architecture.md (1)
260-285: 💤 Low valueOptional: keep the component diagram in sync.
The newly documented
/mcp/ragendpoint is not represented in the ASCII component diagram earlier in this file (which still shows config/stats/query/admin/cache/ai handlers only). Adding arag THlane would make the architecture overview match the prose.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/MCP/Architecture.md` around lines 260 - 285, The component diagram at the top of the document is out of sync with the new /mcp/rag endpoint: update the ASCII component diagram to add a "rag TH" lane (or similar labeled lane) and include the rag tools (rag.search_fts, rag.search_vector, rag.search_hybrid, rag.get_chunks, rag.get_docs, rag.fetch_from_source, rag.admin.stats) so the diagram visually represents the RAG retrieval handler described in the `/mcp/rag` section; place the lane alongside the existing config/stats/query/admin/cache/ai handlers and ensure the label and arrows reflect authentication (mcp-rag_endpoint_auth) and interactions described in the prose.test/tap/tests/unit/genai_config_query_unit-t.cpp (1)
30-43: 💤 Low value
calloc+reinterpret_cast<ProxySQL_Admin*>is fragile if the class gains non-POD members.The fixture skips
ProxySQL_Admin's constructors, which works today only because the test exercises justsql_query_global_mutex(manuallypthread_mutex_init'd) andadmindb. If anyone adds a non-trivial member (std::string, std::vector, virtual table, etc.) toProxySQL_Admin, this test will silently produce undefined behavior. A short comment here flagging the assumption — or moving to a small testable subset / friend hook — would help future maintenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/unit/genai_config_query_unit-t.cpp` around lines 30 - 43, The test's AdminFixture currently allocates ProxySQL_Admin via calloc and reinterpret_cast which bypasses constructors and is fragile if ProxySQL_Admin later gains non-POD members; change the fixture to construct a real ProxySQL_Admin (or use placement-new on properly aligned storage) instead of calloc so constructors run and initialize members like admindb, and keep explicit pthread_mutex_init only if sql_query_global_mutex is still a plain pthread_mutex; alternatively add a clear comment in AdminFixture noting the intentional bypass and the requirement that ProxySQL_Admin remain POD for this test, referencing AdminFixture, ProxySQL_Admin, sql_query_global_mutex, admindb and GloAdmin so future maintainers see the assumption.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deps/Makefile`:
- Around line 182-198: The protobuf build step for target $(PROTOBUF_LIB) sets
many CMake flags but doesn't pin CMAKE_INSTALL_LIBDIR, so protobuf may install
into lib64 and the expected $(PROTOBUF_INSTALL_DIR)/lib/libprotobuf.a path will
be missing; modify the CC=${CC} CXX=${CXX} cmake invocation in the
$(PROTOBUF_LIB) recipe (the cmake -S . -B build ... line) to add
-DCMAKE_INSTALL_LIBDIR=lib so installed libraries land in
$(PROTOBUF_INSTALL_DIR)/lib and downstream rules (e.g., plugins/mysqlx/Makefile)
find libprotobuf.a.
In `@plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp`:
- Around line 463-524: The handle_query function currently locks
GloAdmin->sql_query_global_mutex manually and may leak the lock if
execute_statement or subsequent JSON/string operations throw; replace the manual
pthread_mutex_lock/unlock pair with an RAII guard (e.g., a small
pthread_lock_guard class or std::unique_lock wrapper) that locks the mutex at
entry and always unlocks on scope exit around the call to
GloAdmin->admindb->execute_statement and the subsequent error/result handling;
ensure error, resultset cleanup (free(error), delete resultset) still happens
before returning and that create_error_response/create_success_response paths
remain unchanged, referencing Config_Tool_Handler::handle_query,
GloAdmin->sql_query_global_mutex, execute_statement, and resultset.
- Around line 485-505: Some code paths free an error string allocated with
strdup() (from SQLite3DB::execute_statement) using the SQLite allocator
proxy_sqlite3_free, which is wrong; update the error deallocation in
RAG_Tool_Handler (the blocks around where error is assigned/checked and freed,
look for uses of (*proxy_sqlite3_free)(error)) and in MySQL_FTS (similar
error-handling blocks) to call free(error) instead of
(*proxy_sqlite3_free)(error), and ensure any surrounding cleanup logic (deleting
resultset or returning create_error_response) remains unchanged.
In `@plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp`:
- Around line 1118-1122: The concatenation of cfg.where_sql directly into the
SELECT creates an untrusted SQL injection surface; update the builder (the code
that constructs sql using cfg.where_sql in RAG_Tool_Handler.cpp around the
SELECT/WHERE construction and the analogous PG block) to (1) add a clear comment
that rag_sources.where_sql is a trusted catalog value and is a trust boundary,
(2) validate cfg.where_sql at load time or before use to enforce it is a single
boolean expression — reject or fail if it contains characters/sequences that can
terminate or inject statements such as ';', '--', '/*', '*/', newlines, or
multiple top-level statements, and (3) if validation fails, refuse to use the
fragment (e.g., error out or ignore it) rather than concatenating; reference
cfg.where_sql and quote_mysql_identifier in your changes and apply the same
validation and comment to the PostgreSQL construction at the other location.
- Around line 73-317: These file-scope helper functions
(is_valid_sql_identifier, quote_mysql_identifier, quote_pgsql_identifier,
escape_mysql_literal, escape_pgsql_literal, json_scalar_to_sql_literal_mysql,
json_scalar_to_sql_literal_pgsql, append_source_pk_predicates_mysql,
append_source_pk_predicates_pgsql, mysql_result_to_json, pgsql_result_to_json)
must have internal linkage to avoid ODR/linker collisions; wrap their
definitions in the same anonymous namespace used later in the file (or
alternatively mark each function static) so they are file-local and remain
discoverable by name in the diff (e.g., enclose the above function definitions
in "namespace { ... }"). Ensure you move only the helper definitions (not their
callers) into that anonymous namespace.
- Around line 1029-1040: The code frees sqlite_error with free(sqlite_error) but
earlier (and elsewhere) the code uses the SQLite-proxy allocator via
proxy_sqlite3_free; change the free call in the error path inside the block
(where sqlite_error is checked before setting error) to call the proxy free
function (e.g. (*proxy_sqlite3_free)(sqlite_error) or
proxy_sqlite3_free(sqlite_error)) instead, keeping the rest of the control flow
in the same block (variables: sqlite_error, execute_statement,
proxy_sqlite3_free, result).
In `@plugins/mysqlx/Makefile`:
- Around line 30-34: The PROTOBUF_LIB path in this Makefile assumes protobuf
installed to $(PROTOBUF_INSTALL_DIR)/lib but some distros install to lib64; fix
the root cause by updating deps/Makefile's CMake configure step to force a
consistent install lib dir (add -DCMAKE_INSTALL_LIBDIR=lib when configuring
protobuf/builds) so that PROTOBUF_INSTALL_DIR, PROTOBUF_INCLUDE_DIR and
PROTOBUF_LIB resolve correctly across platforms and the $(PLUGIN_SO) link
prerequisite is satisfied.
In `@test/tap/tests/unit/genai_config_query_unit-t.cpp`:
- Around line 73-144: The test's TAP plan is incorrect: update the plan() call
in main() from plan(30) to plan(31) so the declared number of assertions matches
the actual assertions (including those from expect_query_value); locate the
plan(...) invocation at the top of main and change the literal 30 to 31.
---
Nitpick comments:
In `@deps/Makefile`:
- Around line 49-51: There are two adjacent conditionals checking PROXYSQL40
(one adding target "protobuf" and another adding "sqlite-vec") separated by a
PROXYSQLCLICKHOUSE block; merge them by removing the separate ifeq
($(PROXYSQL40),1) that adds "protobuf" and instead add "protobuf" to the same
targets += line or block that currently adds "sqlite-vec" under the ifeq
($(PROXYSQL40),1) clause so both protobuf and sqlite-vec are declared inside a
single PROXYSQL40 gated block (leave PROXYSQLCLICKHOUSE untouched).
In `@doc/MCP/Architecture.md`:
- Around line 260-285: The component diagram at the top of the document is out
of sync with the new /mcp/rag endpoint: update the ASCII component diagram to
add a "rag TH" lane (or similar labeled lane) and include the rag tools
(rag.search_fts, rag.search_vector, rag.search_hybrid, rag.get_chunks,
rag.get_docs, rag.fetch_from_source, rag.admin.stats) so the diagram visually
represents the RAG retrieval handler described in the `/mcp/rag` section; place
the lane alongside the existing config/stats/query/admin/cache/ai handlers and
ensure the label and arrows reflect authentication (mcp-rag_endpoint_auth) and
interactions described in the prose.
In `@test/tap/tests/unit/genai_config_query_unit-t.cpp`:
- Around line 30-43: The test's AdminFixture currently allocates ProxySQL_Admin
via calloc and reinterpret_cast which bypasses constructors and is fragile if
ProxySQL_Admin later gains non-POD members; change the fixture to construct a
real ProxySQL_Admin (or use placement-new on properly aligned storage) instead
of calloc so constructors run and initialize members like admindb, and keep
explicit pthread_mutex_init only if sql_query_global_mutex is still a plain
pthread_mutex; alternatively add a clear comment in AdminFixture noting the
intentional bypass and the requirement that ProxySQL_Admin remain POD for this
test, referencing AdminFixture, ProxySQL_Admin, sql_query_global_mutex, admindb
and GloAdmin so future maintainers see the assumption.
🪄 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: 0745e755-690d-4f4d-a814-e1ec4536b8ba
⛔ Files ignored due to path filters (1)
deps/protobuf/protobuf-3.21.12.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (15)
.gitignoredeps/Makefiledoc/MCP/Architecture.mddocs/superpowers/plans/2026-05-04-config-query-tool.mddocs/superpowers/plans/2026-05-04-protobuf-vendor.mdplugins/genai/Makefileplugins/genai/include/Config_Tool_Handler.hplugins/genai/src/tool_handlers/Config_Tool_Handler.cppplugins/genai/src/tool_handlers/RAG_Tool_Handler.cppplugins/mysqlx/Makefileplugins/mysqlx/src/mysqlx_connection.cppsrc/Makefiletest/tap/tests/unit/Makefiletest/tap/tests/unit/genai_config_query_unit-t.cpptest/tap/tests/unit/genai_rag_fetch_from_source_unit-t.cpp
📜 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). (4)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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 naming convention
Constants and macros must use UPPER_SNAKE_CASE naming convention
Use conditional compilation via#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB,#ifdef PROXYSQLCLICKHOUSEfor feature tier gating in core code
Files:
plugins/genai/include/Config_Tool_Handler.hplugins/mysqlx/src/mysqlx_connection.cpptest/tap/tests/unit/genai_rag_fetch_from_source_unit-t.cppplugins/genai/src/tool_handlers/Config_Tool_Handler.cpptest/tap/tests/unit/genai_config_query_unit-t.cppplugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp
**/*.{h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Include guards in header files must use
#ifndef __CLASS_*_Hpattern
Files:
plugins/genai/include/Config_Tool_Handler.h
plugins/genai/**
📄 CodeRabbit inference engine (CLAUDE.md)
GenAI, MCP, RAG, and LLM features must be implemented in
plugins/genai/and loaded as a.soat runtime viaplugins = (genai)configuration; do not gate core code withPROXYSQLGENAI
Files:
plugins/genai/include/Config_Tool_Handler.hplugins/genai/Makefileplugins/genai/src/tool_handlers/Config_Tool_Handler.cppplugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp
test/tap/tests/unit/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests must live in
test/tap/tests/unit/and link againstlibproxysql.avia the custom test harness usingtest_globals.handtest_init.h
Files:
test/tap/tests/unit/genai_rag_fetch_from_source_unit-t.cpptest/tap/tests/unit/genai_config_query_unit-t.cpp
{lib/proxy_sqlite3_symbols.cpp,lib/Makefile,src/Makefile}
📄 CodeRabbit inference engine (CLAUDE.md)
The sqlite-vec hook (
proxy_sqlite3_vec_initinlib/proxy_sqlite3_symbols.cppandvec.olink inlib/Makefileandsrc/Makefile) may be gated byPROXYSQLGENAIas the only exception to keeping genai code out of core
Files:
src/Makefile
🧠 Learnings (6)
📚 Learning: 2026-03-26T16:38:58.553Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5548
File: lib/mysql_connection.cpp:1837-1843
Timestamp: 2026-03-26T16:38:58.553Z
Learning: In `lib/mysql_connection.cpp`, when reviewing `SHOW WARNINGS` handling, treat the digest source as an intentional design choice: both `update_warning_count_from_connection()` and the `add_eof()` call under `ASYNC_USE_RESULT_CONT` detect warnings using `myds->sess->CurrentQuery.QueryParserArgs.digest_text` (comment-stripped digest text). This is expected to fail/behave differently when `mysql-query_digests_keep_comment=1` (digest_text includes comments) or when `mysql-query_digests=0` (digest_text unavailable). Do not require a change unless the regression test coverage is expanded (noting `reg_test_5306-show_warnings_with_comment-t` explicitly excludes these configurations as an accepted limitation).
Applied to files:
.gitignore
📚 Learning: 2026-04-11T13:17:55.508Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.508Z
Learning: When using GitHub-flavored Markdown headings, be aware that an em-dash surrounded by spaces (written as ` — `) affects the generated anchor/slug: GitHub replaces spaces with hyphens and removes non-alphanumeric punctuation, which can produce double hyphens (e.g., `## Foo — bar` → anchor `#foo--bar`, not `#foo-bar`). If you reference these anchors (e.g., internal links), ensure the expected slug matches this behavior.
Applied to files:
docs/superpowers/plans/2026-05-04-protobuf-vendor.mddoc/MCP/Architecture.mddocs/superpowers/plans/2026-05-04-config-query-tool.md
📚 Learning: 2026-04-11T13:17:55.509Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:17:55.509Z
Learning: When reviewing GitHub-flavored Markdown links/anchors, remember that heading-to-anchor slug generation treats spaces as hyphens and removes punctuation. If a heading contains an em-dash surrounded by spaces (e.g. ` — `), the slugs can legitimately include a double hyphen where the two surrounding space-runs become `-` on either side of the removed em-dash (e.g. `...vocabulary--read...`). Do not flag double-hyphens in anchor links for em-dash-containing headings as errors; they reflect GitHub’s correct slug behavior.
Applied to files:
docs/superpowers/plans/2026-05-04-protobuf-vendor.mddoc/MCP/Architecture.mddocs/superpowers/plans/2026-05-04-config-query-tool.md
📚 Learning: 2026-04-11T13:16:05.854Z
Learnt from: renecannao
Repo: sysown/proxysql PR: 5607
File: doc/GH-Actions/README.md:13-18
Timestamp: 2026-04-11T13:16:05.854Z
Learning: When validating GitHub-rendered Markdown in this repository (e.g., links that use heading anchors), account for GitHub slug behavior for headings containing an em-dash (—) surrounded by spaces: GitHub strips the em-dash and converts each surrounding space into a hyphen independently, which can produce a double hyphen (--) in the generated anchor. Therefore, do NOT flag as broken links any anchors whose expected slug contains a double hyphen specifically attributable to an em-dash surrounded by spaces in the source heading. (Example: `...vocabulary — read...` -> `...vocabulary--read...`.)
Applied to files:
doc/MCP/Architecture.md
📚 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_rag_fetch_from_source_unit-t.cpptest/tap/tests/unit/genai_config_query_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/genai_rag_fetch_from_source_unit-t.cpptest/tap/tests/unit/genai_config_query_unit-t.cpp
🪛 checkmake (0.3.2)
deps/Makefile
[warning] 182-182: Target body for "$(PROTOBUF_LIB)" exceeds allowed length of 5 lines (14).
(maxbodylength)
[warning] 198-198: Target "protobuf" should be declared PHONY.
(phonydeclared)
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/genai_config_query_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: 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] 35-35: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 36-36: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 37-37: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 40-40: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 GitHub Check: SonarCloud Code Analysis
test/tap/tests/unit/genai_rag_fetch_from_source_unit-t.cpp
[failure] 28-28: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 26-26: Customize this struct's copy constructor to participate in resource management. Customize or delete its copy assignment operator. Also consider whether move operations should be customized.
[failure] 29-29: C-style cast removing const qualification from the type of a pointer may lead to an undefined behaviour.
[failure] 82-82: Rewrite the code so that you no longer need this "delete".
plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp
[failure] 122-122: Call a function from instead of a function from <ctype.h>.
[failure] 69-69: Call a function from instead of a function from <ctype.h>.
[failure] 136-136: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 150-150: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 117-117: Explicitly capture "token" in a non-transient lambda.
[failure] 31-31: Call a function from instead of a function from <ctype.h>.
[failure] 158-158: Refactor this code to not nest more than 3 if|for|do|while|switch statements.
[failure] 87-87: Refactor this function to reduce its Cognitive Complexity from 62 to the 25 allowed.
[failure] 518-518: Rewrite the code so that you no longer need this "delete".
[failure] 487-487: Remove this use of "free".
[failure] 503-503: Rewrite the code so that you no longer need this "delete".
[failure] 501-501: Remove this use of "free".
[failure] 494-494: Rewrite the code so that you no longer need this "delete".
test/tap/tests/unit/genai_config_query_unit-t.cpp
[failure] 31-31: Remove this use of "calloc".
[failure] 35-35: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 48-48: Rewrite the code so that you no longer need this "delete".
[failure] 36-36: C-style cast removing const qualification from the type of a pointer may lead to an undefined behaviour.
[failure] 52-52: Remove this use of "free".
plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp
[failure] 82-82: Call a function from instead of a function from <ctype.h>.
[failure] 77-77: Call a function from instead of a function from <ctype.h>.
[failure] 1045-1045: Rewrite the code so that you no longer need this "delete".
[failure] 1037-1037: Rewrite the code so that you no longer need this "delete".
[failure] 1005-1005: Refactor this function to reduce its Cognitive Complexity from 26 to the 25 allowed.
[failure] 1032-1032: Remove this use of "free".
[failure] 1067-1067: Rewrite the code so that you no longer need this "delete".
[failure] 1053-1053: Call a function from instead of a function from <ctype.h>.
🪛 LanguageTool
docs/superpowers/plans/2026-05-04-config-query-tool.md
[grammar] ~60-~60: Ensure spelling is correct
Context: ...->execute_statement(...)- convert any resultset withMCP_Tool_Handler::resultset_to_js...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.1)
docs/superpowers/plans/2026-05-04-protobuf-vendor.md
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
docs/superpowers/plans/2026-05-04-config-query-tool.md
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (13)
plugins/genai/Makefile (1)
75-78: LGTM — Darwin-onlydynamic_lookupmirrorsplugins/mysqlx/Makefile.
PLUGIN_LDFLAGSis empty on Linux (no behavior change) and only adds-Wl,-undefined,dynamic_lookupon macOS, where the plugin must be allowed to resolve symbols provided by the host process at load time. Consistent with the equivalent change inplugins/mysqlx/Makefile.Also applies to: 125-125
plugins/mysqlx/src/mysqlx_connection.cpp (1)
86-94: LGTM — portable non-blocking setup with correct cleanup on failure.Switching from
SOCK_NONBLOCK(Linux/glibc extension, not portable to macOS) to asocket()+fcntl(F_GETFL/F_SETFL, O_NONBLOCK)pair is the right call for the macOS build that this PR is enabling. Short-circuit evaluation inflags < 0 || fcntl(...) < 0correctly skips theF_SETFLcall whenF_GETFLfailed, and the error branch properlyclose()sfd_, resets it to-1, and setsERROR_STATEbefore returning, so callers can't observe a half-initialized descriptor..gitignore (1)
224-224: LGTM — matches the existingdeps/<lib>/<lib>-*/convention.Pattern is consistent with the surrounding
deps/*entries (e.g.deps/zstd/zstd-*/,deps/curl/curl-*/) and correctly preserves the trackedprotobuf-3.21.12.tar.gzarchive while ignoring the unpacked source tree produced bydeps/Makefile.plugins/mysqlx/Makefile (1)
134-136: LGTM — link order is correct for staticlibprotobuf.a.
$^expands to$(OBJS) $(PROTO_OBJS) $(PROTOBUF_LIB), so the protobuf archive sits after every object that references its symbols — important because the linker resolves archive members lazily by reference order. The trailing$(ZSTD_AR) $(LZ4_AR)are independent and ordering against them is irrelevant. macOS gets-Wl,-undefined,dynamic_lookupviaPLUGIN_LDFLAGS, matching the genai plugin.src/Makefile (1)
169-169: LGTM.Adding
libsqlparser.ato the DarwinLIBPROXYSQLARmirrors the-lsqlparseralready present inSTATICMYLIBSfor Linux and aligns with the matching unit-test Makefile change, so the link set is consistent across platforms.plugins/genai/include/Config_Tool_Handler.h (1)
54-59: Declaration matches the implementation.The new
handle_querydeclaration is consistent with the dispatcher inexecute_tooland the implementation inConfig_Tool_Handler.cpp.doc/MCP/Architecture.md (1)
142-148: Documentation faithfully matches the implemented policy.The query policy and JSON response shape (
sql,rows_affected,row_count,columns,rows) accurately mirror whatConfig_Tool_Handler::handle_queryproduces.test/tap/tests/unit/genai_rag_fetch_from_source_unit-t.cpp (2)
97-99: 💤 Low value
set_vector_dbis a documented public API method, not a test-only addition.The method is properly defined as a public inline setter in
plugins/genai/include/RAG_Tool_Handler.h(line 499) and is already used in production code (plugins/genai/src/AI_Features_Manager.cpp, line 387). No action needed.
122-125: ⚡ Quick winThe assertion on
"mysql_real_connect failed"is actually stable because this string is hardcoded in ProxySQL's own code (backend_client.cpp), not generated by libmysqlclient. Across different libmysqlclient versions, the prefix remains constant; only the appendedmysql_error()output varies (which the test does not assert). No changes needed.plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp (1)
511-521: 💤 Low value
resultset_to_jsonreturns only a rows array—no structural inconsistency.The function
resultset_to_json(defined inMCP_Tool_Handler.cpp) returns a JSON array of row objects, as documented: "@return JSON array of row objects (empty array when resultset is NULL or has no rows)." Buildingpayload["columns"]explicitly and assigningpayload["rows"]to the result ofresultset_to_json(resultset, cols)produces the correct response shape with separatecolumnsandrowskeys. No issue.test/tap/tests/unit/Makefile (3)
146-146: LGTM — DarwinLIBPROXYSQLAR_FULLupdated consistently.Replacing
$(SQLITE3_LDIR)/sqlite3.owith$(PARSERSQL_LDIR)/libsqlparser.aon macOS mirrors the parallel change insrc/Makefiledescribed in the PR. The Linux path already carries-lsqlparservia-Wl,-BstaticinMYLIBS, so this brings the two platforms in sync.
399-401: LGTM — new test targets correctly gated behindPROXYSQL40.Both names match their explicit build rules added below, and the placement inside
ifeq ($(PROXYSQL40),1)is consistent with all other genai/plugin targets in that block.
744-752: LGTM — new build rules follow the establishedGENAI_ALL_SRCSpattern exactly.The dependency sets and link flags are identical to
genai_stats_parsing_unit-t/genai_query_handler_unit-t. Both new tool-handler source files (Config_Tool_Handler.cpp,RAG_Tool_Handler.cpp) live inplugins/genai/src/tool_handlers/and will be captured by the existingGENAI_ALL_SRCSwildcard. The explicit rules appear before the%-tpattern rule, so GNU Make will correctly prefer them.
| json Config_Tool_Handler::handle_query(const std::string& sql) { | ||
| if (!GloAdmin || !GloAdmin->admindb) { | ||
| return create_error_response("ProxySQL Admin database not available"); | ||
| } | ||
|
|
||
| const std::string validation_error = validate_config_sql(GloAdmin->admindb, sql); | ||
| if (!validation_error.empty()) { | ||
| return create_error_response(validation_error); | ||
| } | ||
|
|
||
| if (pthread_mutex_lock(&GloAdmin->sql_query_global_mutex) != 0) { | ||
| return create_error_response("Failed to lock sql_query_global_mutex"); | ||
| } | ||
|
|
||
| char* error = NULL; | ||
| int cols = 0; | ||
| int affected_rows = 0; | ||
| SQLite3_result* resultset = NULL; | ||
| GloAdmin->admindb->execute_statement(sql.c_str(), &error, &cols, &affected_rows, &resultset); | ||
|
|
||
| int unlock_rc = pthread_mutex_unlock(&GloAdmin->sql_query_global_mutex); | ||
| if (unlock_rc != 0) { | ||
| if (error) { | ||
| std::string err_msg = error; | ||
| free(error); | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response(err_msg + "; also failed to unlock sql_query_global_mutex"); | ||
| } | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response("Failed to unlock sql_query_global_mutex"); | ||
| } | ||
|
|
||
| if (error) { | ||
| std::string err_msg = error; | ||
| free(error); | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response(err_msg); | ||
| } | ||
|
|
||
| json payload; | ||
| payload["sql"] = sql; | ||
| payload["rows_affected"] = affected_rows; | ||
| payload["row_count"] = resultset ? resultset->rows_count : 0; | ||
| payload["columns"] = json::array(); | ||
| if (resultset) { | ||
| for (const auto* column : resultset->column_definition) { | ||
| payload["columns"].push_back(column ? column->name : ""); | ||
| } | ||
| payload["rows"] = resultset_to_json(resultset, cols); | ||
| delete resultset; | ||
| } else { | ||
| payload["rows"] = json::array(); | ||
| } | ||
|
|
||
| return create_success_response(payload); | ||
| } |
There was a problem hiding this comment.
Manual lock/unlock around execute_statement can leak the admin global mutex on exception.
pthread_mutex_lock(&GloAdmin->sql_query_global_mutex) is paired with a manual pthread_mutex_unlock after the call. If execute_statement (or the json/string operations between lock and unlock) throws, the outer try/catch in execute_tool will swallow the exception, but the global admin mutex is never released — every subsequent admin DB operation will deadlock. Use an RAII guard so the unlock is exception-safe.
🔒 Suggested RAII fix
- if (pthread_mutex_lock(&GloAdmin->sql_query_global_mutex) != 0) {
- return create_error_response("Failed to lock sql_query_global_mutex");
- }
-
- char* error = NULL;
- int cols = 0;
- int affected_rows = 0;
- SQLite3_result* resultset = NULL;
- GloAdmin->admindb->execute_statement(sql.c_str(), &error, &cols, &affected_rows, &resultset);
-
- int unlock_rc = pthread_mutex_unlock(&GloAdmin->sql_query_global_mutex);
- if (unlock_rc != 0) {
- if (error) {
- std::string err_msg = error;
- free(error);
- if (resultset) {
- delete resultset;
- }
- return create_error_response(err_msg + "; also failed to unlock sql_query_global_mutex");
- }
- if (resultset) {
- delete resultset;
- }
- return create_error_response("Failed to unlock sql_query_global_mutex");
- }
+ char* error = NULL;
+ int cols = 0;
+ int affected_rows = 0;
+ SQLite3_result* resultset = NULL;
+ {
+ std::lock_guard<std::mutex> _lk(/* wrap pthread_mutex via std::mutex adapter, or */
+ /* use std::unique_lock with a pthread RAII helper */);
+ GloAdmin->admindb->execute_statement(sql.c_str(), &error, &cols, &affected_rows, &resultset);
+ }Since sql_query_global_mutex is a pthread_mutex_t, a small RAII wrapper (pthread_lock_guard) over pthread_mutex_lock/pthread_mutex_unlock — or moving to std::mutex — would do.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 518-518: Rewrite the code so that you no longer need this "delete".
[failure] 487-487: Remove this use of "free".
[failure] 503-503: Rewrite the code so that you no longer need this "delete".
[failure] 501-501: Remove this use of "free".
[failure] 494-494: Rewrite the code so that you no longer need this "delete".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp` around lines 463 -
524, The handle_query function currently locks GloAdmin->sql_query_global_mutex
manually and may leak the lock if execute_statement or subsequent JSON/string
operations throw; replace the manual pthread_mutex_lock/unlock pair with an RAII
guard (e.g., a small pthread_lock_guard class or std::unique_lock wrapper) that
locks the mutex at entry and always unlocks on scope exit around the call to
GloAdmin->admindb->execute_statement and the subsequent error/result handling;
ensure error, resultset cleanup (free(error), delete resultset) still happens
before returning and that create_error_response/create_success_response paths
remain unchanged, referencing Config_Tool_Handler::handle_query,
GloAdmin->sql_query_global_mutex, execute_statement, and resultset.
| if (error) { | ||
| std::string err_msg = error; | ||
| free(error); | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response(err_msg + "; also failed to unlock sql_query_global_mutex"); | ||
| } | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response("Failed to unlock sql_query_global_mutex"); | ||
| } | ||
|
|
||
| if (error) { | ||
| std::string err_msg = error; | ||
| free(error); | ||
| if (resultset) { | ||
| delete resultset; | ||
| } | ||
| return create_error_response(err_msg); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how SQLite3DB::execute_statement allocates the error string
# and how callers free it across the codebase.
# 1) Locate the implementation of execute_statement
ast-grep --pattern $'$_ SQLite3DB::execute_statement($$$) {
$$$
}'
# 2) Find all freeing patterns near execute_statement results
rg -nP -C3 '\bexecute_statement\s*\(' --type=cpp
# 3) See if errmsg comes via sqlite3_exec / proxy_sqlite3_exec with the standard sqlite3_malloc'd buffer
rg -nP -C3 '(proxy_)?sqlite3_(exec|errmsg|malloc|free)\s*\(' --type=cpp -g 'lib/**' -g 'plugins/**'Repository: sysown/proxysql
Length of output: 50372
🏁 Script executed:
# Find exact code in Config_Tool_Handler.cpp lines 485-505
cat -n plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp | sed -n '485,505p'Repository: sysown/proxysql
Length of output: 681
🏁 Script executed:
# Look for RAG_Tool_Handler.cpp and check line 702 area
find . -name "RAG_Tool_Handler.cpp" -type fRepository: sysown/proxysql
Length of output: 114
🏁 Script executed:
# Search for proxy_sqlite3_free usage pattern
rg -n "proxy_sqlite3_free" --type=cpp | head -20Repository: sysown/proxysql
Length of output: 1044
🏁 Script executed:
# Find the execute_statement implementation to understand the allocator
rg -n "execute_statement.*{" --type=cpp -A 20 --max-count=3 | grep -A 20 "^src/"Repository: sysown/proxysql
Length of output: 163
🏁 Script executed:
# Search for execute_statement implementation definition
rg -n "SQLite3DB::execute_statement" --type=cpp -A 25 | head -80Repository: sysown/proxysql
Length of output: 4788
🏁 Script executed:
# Specifically check RAG_Tool_Handler.cpp around line 702
cat -n plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp | sed -n '695,710p'Repository: sysown/proxysql
Length of output: 534
🏁 Script executed:
# Check sqlite3db.cpp to understand how error is allocated
rg -n "sqlite3_errmsg\|sqlite3_malloc\|sqlite3_exec" lib/sqlite3db.cpp | head -30Repository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Find all error freeing patterns in genai plugins to see broader pattern
rg -n "free\(error\)\|proxy_sqlite3_free.*error" plugins/genai/ --type=cppRepository: sysown/proxysql
Length of output: 41
🏁 Script executed:
# Check if there are other instances like RAG_Tool_Handler with mismatched allocator
rg -B 5 "\(\*proxy_sqlite3_free\)\(error\)" plugins/genai/ --type=cpp | head -60Repository: sysown/proxysql
Length of output: 1937
🏁 Script executed:
# Verify the strdup allocator once more to be absolutely certain
rg -B 2 -A 1 "strdup\(\(\*proxy_sqlite3_errmsg\)" lib/sqlite3db.cppRepository: sysown/proxysql
Length of output: 1444
Config_Tool_Handler.cpp uses the correct allocator; the issue exists in RAG_Tool_Handler.cpp and MySQL_FTS.cpp instead.
The error pointer from SQLite3DB::execute_statement() is allocated via strdup() (lib/sqlite3db.cpp lines 337, 345, 401), which uses the C standard library allocator. Therefore it must be freed with free(), not sqlite3_free().
Config_Tool_Handler.cpp (lines 485, 501) correctly uses free(error). However, RAG_Tool_Handler.cpp (lines 702, 775) and MySQL_FTS.cpp (lines 583, 659) incorrectly use (*proxy_sqlite3_free)(error) on strdup-allocated pointers, which is undefined behavior and risks crashes or memory corruption.
Update RAG_Tool_Handler.cpp and MySQL_FTS.cpp to use free(error) instead of (*proxy_sqlite3_free)(error).
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 487-487: Remove this use of "free".
[failure] 503-503: Rewrite the code so that you no longer need this "delete".
[failure] 501-501: Remove this use of "free".
[failure] 494-494: Rewrite the code so that you no longer need this "delete".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/genai/src/tool_handlers/Config_Tool_Handler.cpp` around lines 485 -
505, Some code paths free an error string allocated with strdup() (from
SQLite3DB::execute_statement) using the SQLite allocator proxy_sqlite3_free,
which is wrong; update the error deallocation in RAG_Tool_Handler (the blocks
around where error is assigned/checked and freed, look for uses of
(*proxy_sqlite3_free)(error)) and in MySQL_FTS (similar error-handling blocks)
to call free(error) instead of (*proxy_sqlite3_free)(error), and ensure any
surrounding cleanup logic (deleting resultset or returning
create_error_response) remains unchanged.
| bool is_valid_sql_identifier(const std::string& identifier) { | ||
| if (identifier.empty()) { | ||
| return false; | ||
| } | ||
| if (!(std::isalpha(static_cast<unsigned char>(identifier[0])) || identifier[0] == '_')) { | ||
| return false; | ||
| } | ||
| for (size_t i = 1; i < identifier.size(); ++i) { | ||
| const unsigned char c = static_cast<unsigned char>(identifier[i]); | ||
| if (!(std::isalnum(c) || identifier[i] == '_' || identifier[i] == '$')) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| std::string quote_mysql_identifier(const std::string& identifier) { | ||
| std::string out = "`"; | ||
| for (char c : identifier) { | ||
| if (c == '`') { | ||
| out += "``"; | ||
| } else { | ||
| out.push_back(c); | ||
| } | ||
| } | ||
| out += "`"; | ||
| return out; | ||
| } | ||
|
|
||
| std::string quote_pgsql_identifier(const std::string& identifier) { | ||
| std::string out = "\""; | ||
| for (char c : identifier) { | ||
| if (c == '"') { | ||
| out += "\"\""; | ||
| } else { | ||
| out.push_back(c); | ||
| } | ||
| } | ||
| out += "\""; | ||
| return out; | ||
| } | ||
|
|
||
| std::string escape_mysql_literal(MYSQL* conn, const std::string& value) { | ||
| std::string escaped; | ||
| escaped.resize(value.size() * 2 + 1); | ||
| unsigned long len = mysql_real_escape_string(conn, &escaped[0], value.c_str(), static_cast<unsigned long>(value.size())); | ||
| escaped.resize(len); | ||
| return "'" + escaped + "'"; | ||
| } | ||
|
|
||
| std::string escape_pgsql_literal(PGconn* conn, const std::string& value, std::string& error) { | ||
| char* escaped = PQescapeLiteral(conn, value.c_str(), value.size()); | ||
| if (!escaped) { | ||
| error = "Failed to escape PostgreSQL literal"; | ||
| return ""; | ||
| } | ||
| std::string out(escaped); | ||
| PQfreemem(escaped); | ||
| return out; | ||
| } | ||
|
|
||
| std::string json_scalar_to_sql_literal_mysql(MYSQL* conn, const json& value, std::string& error) { | ||
| if (value.is_null()) { | ||
| return "NULL"; | ||
| } | ||
| if (value.is_string()) { | ||
| return escape_mysql_literal(conn, value.get<std::string>()); | ||
| } | ||
| if (value.is_boolean()) { | ||
| return value.get<bool>() ? "1" : "0"; | ||
| } | ||
| if (value.is_number()) { | ||
| return value.dump(); | ||
| } | ||
| error = "Primary key value must be a scalar"; | ||
| return ""; | ||
| } | ||
|
|
||
| std::string json_scalar_to_sql_literal_pgsql(PGconn* conn, const json& value, std::string& error) { | ||
| if (value.is_null()) { | ||
| return "NULL"; | ||
| } | ||
| if (value.is_string()) { | ||
| std::string escaped = escape_pgsql_literal(conn, value.get<std::string>(), error); | ||
| if (!error.empty()) { | ||
| return ""; | ||
| } | ||
| return escaped; | ||
| } | ||
| if (value.is_boolean()) { | ||
| return value.get<bool>() ? "TRUE" : "FALSE"; | ||
| } | ||
| if (value.is_number()) { | ||
| return value.dump(); | ||
| } | ||
| error = "Primary key value must be a scalar"; | ||
| return ""; | ||
| } | ||
|
|
||
| bool append_source_pk_predicates_mysql( | ||
| MYSQL* conn, | ||
| const json& pk_json, | ||
| const std::string& pk_column, | ||
| std::string& where_clause, | ||
| std::string& error | ||
| ) { | ||
| where_clause.clear(); | ||
| if (pk_json.is_object()) { | ||
| if (pk_json.empty()) { | ||
| error = "Source primary key JSON is empty"; | ||
| return false; | ||
| } | ||
| for (auto it = pk_json.begin(); it != pk_json.end(); ++it) { | ||
| const std::string& key = it.key(); | ||
| if (!is_valid_sql_identifier(key)) { | ||
| error = "Primary key column contains unsafe characters: " + key; | ||
| return false; | ||
| } | ||
| std::string literal = json_scalar_to_sql_literal_mysql(conn, it.value(), error); | ||
| if (!error.empty()) { | ||
| return false; | ||
| } | ||
| if (!where_clause.empty()) { | ||
| where_clause += " AND "; | ||
| } | ||
| where_clause += quote_mysql_identifier(key) + " = " + literal; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| if (!is_valid_sql_identifier(pk_column)) { | ||
| error = "Primary key column contains unsafe characters: " + pk_column; | ||
| return false; | ||
| } | ||
| std::string literal = json_scalar_to_sql_literal_mysql(conn, pk_json, error); | ||
| if (!error.empty()) { | ||
| return false; | ||
| } | ||
| where_clause = quote_mysql_identifier(pk_column) + " = " + literal; | ||
| return true; | ||
| } | ||
|
|
||
| bool append_source_pk_predicates_pgsql( | ||
| PGconn* conn, | ||
| const json& pk_json, | ||
| const std::string& pk_column, | ||
| std::string& where_clause, | ||
| std::string& error | ||
| ) { | ||
| where_clause.clear(); | ||
| if (pk_json.is_object()) { | ||
| if (pk_json.empty()) { | ||
| error = "Source primary key JSON is empty"; | ||
| return false; | ||
| } | ||
| for (auto it = pk_json.begin(); it != pk_json.end(); ++it) { | ||
| const std::string& key = it.key(); | ||
| if (!is_valid_sql_identifier(key)) { | ||
| error = "Primary key column contains unsafe characters: " + key; | ||
| return false; | ||
| } | ||
| std::string literal = json_scalar_to_sql_literal_pgsql(conn, it.value(), error); | ||
| if (!error.empty()) { | ||
| return false; | ||
| } | ||
| if (!where_clause.empty()) { | ||
| where_clause += " AND "; | ||
| } | ||
| where_clause += quote_pgsql_identifier(key) + " = " + literal; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| if (!is_valid_sql_identifier(pk_column)) { | ||
| error = "Primary key column contains unsafe characters: " + pk_column; | ||
| return false; | ||
| } | ||
| std::string literal = json_scalar_to_sql_literal_pgsql(conn, pk_json, error); | ||
| if (!error.empty()) { | ||
| return false; | ||
| } | ||
| where_clause = quote_pgsql_identifier(pk_column) + " = " + literal; | ||
| return true; | ||
| } | ||
|
|
||
| json mysql_result_to_json(MYSQL_RES* res) { | ||
| json out; | ||
| out["columns"] = json::array(); | ||
| out["rows"] = json::array(); | ||
|
|
||
| if (!res) { | ||
| return out; | ||
| } | ||
|
|
||
| MYSQL_FIELD* fields = mysql_fetch_fields(res); | ||
| const unsigned int num_fields = mysql_num_fields(res); | ||
| std::vector<std::string> column_names; | ||
| column_names.reserve(num_fields); | ||
| for (unsigned int i = 0; i < num_fields; ++i) { | ||
| const char* name = fields && fields[i].name ? fields[i].name : "unknown_field"; | ||
| column_names.emplace_back(name); | ||
| out["columns"].push_back(name); | ||
| } | ||
|
|
||
| MYSQL_ROW row; | ||
| while ((row = mysql_fetch_row(res))) { | ||
| json row_obj = json::object(); | ||
| for (unsigned int i = 0; i < num_fields; ++i) { | ||
| row_obj[column_names[i]] = row[i] ? row[i] : ""; | ||
| } | ||
| out["rows"].push_back(row_obj); | ||
| } | ||
|
|
||
| return out; | ||
| } | ||
|
|
||
| json pgsql_result_to_json(PGresult* res) { | ||
| json out; | ||
| out["columns"] = json::array(); | ||
| out["rows"] = json::array(); | ||
|
|
||
| if (!res) { | ||
| return out; | ||
| } | ||
|
|
||
| const int num_fields = PQnfields(res); | ||
| const int num_rows = PQntuples(res); | ||
| std::vector<std::string> column_names; | ||
| column_names.reserve(num_fields); | ||
| for (int i = 0; i < num_fields; ++i) { | ||
| const char* name = PQfname(res, i); | ||
| column_names.emplace_back(name ? name : "unknown_field"); | ||
| out["columns"].push_back(name ? name : "unknown_field"); | ||
| } | ||
|
|
||
| for (int r = 0; r < num_rows; ++r) { | ||
| json row_obj = json::object(); | ||
| for (int c = 0; c < num_fields; ++c) { | ||
| row_obj[column_names[c]] = PQgetisnull(res, r, c) ? "" : PQgetvalue(res, r, c); | ||
| } | ||
| out["rows"].push_back(row_obj); | ||
| } | ||
|
|
||
| return out; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Wrap file-scope helpers in an anonymous namespace (or mark static).
is_valid_sql_identifier, quote_mysql_identifier, quote_pgsql_identifier, escape_mysql_literal, escape_pgsql_literal, json_scalar_to_sql_literal_*, append_source_pk_predicates_*, mysql_result_to_json, and pgsql_result_to_json are all at file scope with external linkage. The lower block (lines 988–1232) already uses an anonymous namespace for the same reason; these earlier helpers should match. As-is, any other translation unit defining a function with one of these common names (is_valid_sql_identifier, mysql_result_to_json, …) will cause an ODR violation or a silent symbol clash at link time.
♻️ Move helpers into the anonymous namespace
+namespace {
+
bool is_valid_sql_identifier(const std::string& identifier) {
...
json pgsql_result_to_json(PGresult* res) {
...
}
+
+} // namespace📝 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.
| bool is_valid_sql_identifier(const std::string& identifier) { | |
| if (identifier.empty()) { | |
| return false; | |
| } | |
| if (!(std::isalpha(static_cast<unsigned char>(identifier[0])) || identifier[0] == '_')) { | |
| return false; | |
| } | |
| for (size_t i = 1; i < identifier.size(); ++i) { | |
| const unsigned char c = static_cast<unsigned char>(identifier[i]); | |
| if (!(std::isalnum(c) || identifier[i] == '_' || identifier[i] == '$')) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| std::string quote_mysql_identifier(const std::string& identifier) { | |
| std::string out = "`"; | |
| for (char c : identifier) { | |
| if (c == '`') { | |
| out += "``"; | |
| } else { | |
| out.push_back(c); | |
| } | |
| } | |
| out += "`"; | |
| return out; | |
| } | |
| std::string quote_pgsql_identifier(const std::string& identifier) { | |
| std::string out = "\""; | |
| for (char c : identifier) { | |
| if (c == '"') { | |
| out += "\"\""; | |
| } else { | |
| out.push_back(c); | |
| } | |
| } | |
| out += "\""; | |
| return out; | |
| } | |
| std::string escape_mysql_literal(MYSQL* conn, const std::string& value) { | |
| std::string escaped; | |
| escaped.resize(value.size() * 2 + 1); | |
| unsigned long len = mysql_real_escape_string(conn, &escaped[0], value.c_str(), static_cast<unsigned long>(value.size())); | |
| escaped.resize(len); | |
| return "'" + escaped + "'"; | |
| } | |
| std::string escape_pgsql_literal(PGconn* conn, const std::string& value, std::string& error) { | |
| char* escaped = PQescapeLiteral(conn, value.c_str(), value.size()); | |
| if (!escaped) { | |
| error = "Failed to escape PostgreSQL literal"; | |
| return ""; | |
| } | |
| std::string out(escaped); | |
| PQfreemem(escaped); | |
| return out; | |
| } | |
| std::string json_scalar_to_sql_literal_mysql(MYSQL* conn, const json& value, std::string& error) { | |
| if (value.is_null()) { | |
| return "NULL"; | |
| } | |
| if (value.is_string()) { | |
| return escape_mysql_literal(conn, value.get<std::string>()); | |
| } | |
| if (value.is_boolean()) { | |
| return value.get<bool>() ? "1" : "0"; | |
| } | |
| if (value.is_number()) { | |
| return value.dump(); | |
| } | |
| error = "Primary key value must be a scalar"; | |
| return ""; | |
| } | |
| std::string json_scalar_to_sql_literal_pgsql(PGconn* conn, const json& value, std::string& error) { | |
| if (value.is_null()) { | |
| return "NULL"; | |
| } | |
| if (value.is_string()) { | |
| std::string escaped = escape_pgsql_literal(conn, value.get<std::string>(), error); | |
| if (!error.empty()) { | |
| return ""; | |
| } | |
| return escaped; | |
| } | |
| if (value.is_boolean()) { | |
| return value.get<bool>() ? "TRUE" : "FALSE"; | |
| } | |
| if (value.is_number()) { | |
| return value.dump(); | |
| } | |
| error = "Primary key value must be a scalar"; | |
| return ""; | |
| } | |
| bool append_source_pk_predicates_mysql( | |
| MYSQL* conn, | |
| const json& pk_json, | |
| const std::string& pk_column, | |
| std::string& where_clause, | |
| std::string& error | |
| ) { | |
| where_clause.clear(); | |
| if (pk_json.is_object()) { | |
| if (pk_json.empty()) { | |
| error = "Source primary key JSON is empty"; | |
| return false; | |
| } | |
| for (auto it = pk_json.begin(); it != pk_json.end(); ++it) { | |
| const std::string& key = it.key(); | |
| if (!is_valid_sql_identifier(key)) { | |
| error = "Primary key column contains unsafe characters: " + key; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_mysql(conn, it.value(), error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| if (!where_clause.empty()) { | |
| where_clause += " AND "; | |
| } | |
| where_clause += quote_mysql_identifier(key) + " = " + literal; | |
| } | |
| return true; | |
| } | |
| if (!is_valid_sql_identifier(pk_column)) { | |
| error = "Primary key column contains unsafe characters: " + pk_column; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_mysql(conn, pk_json, error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| where_clause = quote_mysql_identifier(pk_column) + " = " + literal; | |
| return true; | |
| } | |
| bool append_source_pk_predicates_pgsql( | |
| PGconn* conn, | |
| const json& pk_json, | |
| const std::string& pk_column, | |
| std::string& where_clause, | |
| std::string& error | |
| ) { | |
| where_clause.clear(); | |
| if (pk_json.is_object()) { | |
| if (pk_json.empty()) { | |
| error = "Source primary key JSON is empty"; | |
| return false; | |
| } | |
| for (auto it = pk_json.begin(); it != pk_json.end(); ++it) { | |
| const std::string& key = it.key(); | |
| if (!is_valid_sql_identifier(key)) { | |
| error = "Primary key column contains unsafe characters: " + key; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_pgsql(conn, it.value(), error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| if (!where_clause.empty()) { | |
| where_clause += " AND "; | |
| } | |
| where_clause += quote_pgsql_identifier(key) + " = " + literal; | |
| } | |
| return true; | |
| } | |
| if (!is_valid_sql_identifier(pk_column)) { | |
| error = "Primary key column contains unsafe characters: " + pk_column; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_pgsql(conn, pk_json, error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| where_clause = quote_pgsql_identifier(pk_column) + " = " + literal; | |
| return true; | |
| } | |
| json mysql_result_to_json(MYSQL_RES* res) { | |
| json out; | |
| out["columns"] = json::array(); | |
| out["rows"] = json::array(); | |
| if (!res) { | |
| return out; | |
| } | |
| MYSQL_FIELD* fields = mysql_fetch_fields(res); | |
| const unsigned int num_fields = mysql_num_fields(res); | |
| std::vector<std::string> column_names; | |
| column_names.reserve(num_fields); | |
| for (unsigned int i = 0; i < num_fields; ++i) { | |
| const char* name = fields && fields[i].name ? fields[i].name : "unknown_field"; | |
| column_names.emplace_back(name); | |
| out["columns"].push_back(name); | |
| } | |
| MYSQL_ROW row; | |
| while ((row = mysql_fetch_row(res))) { | |
| json row_obj = json::object(); | |
| for (unsigned int i = 0; i < num_fields; ++i) { | |
| row_obj[column_names[i]] = row[i] ? row[i] : ""; | |
| } | |
| out["rows"].push_back(row_obj); | |
| } | |
| return out; | |
| } | |
| json pgsql_result_to_json(PGresult* res) { | |
| json out; | |
| out["columns"] = json::array(); | |
| out["rows"] = json::array(); | |
| if (!res) { | |
| return out; | |
| } | |
| const int num_fields = PQnfields(res); | |
| const int num_rows = PQntuples(res); | |
| std::vector<std::string> column_names; | |
| column_names.reserve(num_fields); | |
| for (int i = 0; i < num_fields; ++i) { | |
| const char* name = PQfname(res, i); | |
| column_names.emplace_back(name ? name : "unknown_field"); | |
| out["columns"].push_back(name ? name : "unknown_field"); | |
| } | |
| for (int r = 0; r < num_rows; ++r) { | |
| json row_obj = json::object(); | |
| for (int c = 0; c < num_fields; ++c) { | |
| row_obj[column_names[c]] = PQgetisnull(res, r, c) ? "" : PQgetvalue(res, r, c); | |
| } | |
| out["rows"].push_back(row_obj); | |
| } | |
| return out; | |
| } | |
| namespace { | |
| bool is_valid_sql_identifier(const std::string& identifier) { | |
| if (identifier.empty()) { | |
| return false; | |
| } | |
| if (!(std::isalpha(static_cast<unsigned char>(identifier[0])) || identifier[0] == '_')) { | |
| return false; | |
| } | |
| for (size_t i = 1; i < identifier.size(); ++i) { | |
| const unsigned char c = static_cast<unsigned char>(identifier[i]); | |
| if (!(std::isalnum(c) || identifier[i] == '_' || identifier[i] == '$')) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| std::string quote_mysql_identifier(const std::string& identifier) { | |
| std::string out = "`"; | |
| for (char c : identifier) { | |
| if (c == '`') { | |
| out += "``"; | |
| } else { | |
| out.push_back(c); | |
| } | |
| } | |
| out += "`"; | |
| return out; | |
| } | |
| std::string quote_pgsql_identifier(const std::string& identifier) { | |
| std::string out = "\""; | |
| for (char c : identifier) { | |
| if (c == '"') { | |
| out += "\"\""; | |
| } else { | |
| out.push_back(c); | |
| } | |
| } | |
| out += "\""; | |
| return out; | |
| } | |
| std::string escape_mysql_literal(MYSQL* conn, const std::string& value) { | |
| std::string escaped; | |
| escaped.resize(value.size() * 2 + 1); | |
| unsigned long len = mysql_real_escape_string(conn, &escaped[0], value.c_str(), static_cast<unsigned long>(value.size())); | |
| escaped.resize(len); | |
| return "'" + escaped + "'"; | |
| } | |
| std::string escape_pgsql_literal(PGconn* conn, const std::string& value, std::string& error) { | |
| char* escaped = PQescapeLiteral(conn, value.c_str(), value.size()); | |
| if (!escaped) { | |
| error = "Failed to escape PostgreSQL literal"; | |
| return ""; | |
| } | |
| std::string out(escaped); | |
| PQfreemem(escaped); | |
| return out; | |
| } | |
| std::string json_scalar_to_sql_literal_mysql(MYSQL* conn, const json& value, std::string& error) { | |
| if (value.is_null()) { | |
| return "NULL"; | |
| } | |
| if (value.is_string()) { | |
| return escape_mysql_literal(conn, value.get<std::string>()); | |
| } | |
| if (value.is_boolean()) { | |
| return value.get<bool>() ? "1" : "0"; | |
| } | |
| if (value.is_number()) { | |
| return value.dump(); | |
| } | |
| error = "Primary key value must be a scalar"; | |
| return ""; | |
| } | |
| std::string json_scalar_to_sql_literal_pgsql(PGconn* conn, const json& value, std::string& error) { | |
| if (value.is_null()) { | |
| return "NULL"; | |
| } | |
| if (value.is_string()) { | |
| std::string escaped = escape_pgsql_literal(conn, value.get<std::string>(), error); | |
| if (!error.empty()) { | |
| return ""; | |
| } | |
| return escaped; | |
| } | |
| if (value.is_boolean()) { | |
| return value.get<bool>() ? "TRUE" : "FALSE"; | |
| } | |
| if (value.is_number()) { | |
| return value.dump(); | |
| } | |
| error = "Primary key value must be a scalar"; | |
| return ""; | |
| } | |
| bool append_source_pk_predicates_mysql( | |
| MYSQL* conn, | |
| const json& pk_json, | |
| const std::string& pk_column, | |
| std::string& where_clause, | |
| std::string& error | |
| ) { | |
| where_clause.clear(); | |
| if (pk_json.is_object()) { | |
| if (pk_json.empty()) { | |
| error = "Source primary key JSON is empty"; | |
| return false; | |
| } | |
| for (auto it = pk_json.begin(); it != pk_json.end(); ++it) { | |
| const std::string& key = it.key(); | |
| if (!is_valid_sql_identifier(key)) { | |
| error = "Primary key column contains unsafe characters: " + key; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_mysql(conn, it.value(), error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| if (!where_clause.empty()) { | |
| where_clause += " AND "; | |
| } | |
| where_clause += quote_mysql_identifier(key) + " = " + literal; | |
| } | |
| return true; | |
| } | |
| if (!is_valid_sql_identifier(pk_column)) { | |
| error = "Primary key column contains unsafe characters: " + pk_column; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_mysql(conn, pk_json, error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| where_clause = quote_mysql_identifier(pk_column) + " = " + literal; | |
| return true; | |
| } | |
| bool append_source_pk_predicates_pgsql( | |
| PGconn* conn, | |
| const json& pk_json, | |
| const std::string& pk_column, | |
| std::string& where_clause, | |
| std::string& error | |
| ) { | |
| where_clause.clear(); | |
| if (pk_json.is_object()) { | |
| if (pk_json.empty()) { | |
| error = "Source primary key JSON is empty"; | |
| return false; | |
| } | |
| for (auto it = pk_json.begin(); it != pk_json.end(); ++it) { | |
| const std::string& key = it.key(); | |
| if (!is_valid_sql_identifier(key)) { | |
| error = "Primary key column contains unsafe characters: " + key; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_pgsql(conn, it.value(), error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| if (!where_clause.empty()) { | |
| where_clause += " AND "; | |
| } | |
| where_clause += quote_pgsql_identifier(key) + " = " + literal; | |
| } | |
| return true; | |
| } | |
| if (!is_valid_sql_identifier(pk_column)) { | |
| error = "Primary key column contains unsafe characters: " + pk_column; | |
| return false; | |
| } | |
| std::string literal = json_scalar_to_sql_literal_pgsql(conn, pk_json, error); | |
| if (!error.empty()) { | |
| return false; | |
| } | |
| where_clause = quote_pgsql_identifier(pk_column) + " = " + literal; | |
| return true; | |
| } | |
| json mysql_result_to_json(MYSQL_RES* res) { | |
| json out; | |
| out["columns"] = json::array(); | |
| out["rows"] = json::array(); | |
| if (!res) { | |
| return out; | |
| } | |
| MYSQL_FIELD* fields = mysql_fetch_fields(res); | |
| const unsigned int num_fields = mysql_num_fields(res); | |
| std::vector<std::string> column_names; | |
| column_names.reserve(num_fields); | |
| for (unsigned int i = 0; i < num_fields; ++i) { | |
| const char* name = fields && fields[i].name ? fields[i].name : "unknown_field"; | |
| column_names.emplace_back(name); | |
| out["columns"].push_back(name); | |
| } | |
| MYSQL_ROW row; | |
| while ((row = mysql_fetch_row(res))) { | |
| json row_obj = json::object(); | |
| for (unsigned int i = 0; i < num_fields; ++i) { | |
| row_obj[column_names[i]] = row[i] ? row[i] : ""; | |
| } | |
| out["rows"].push_back(row_obj); | |
| } | |
| return out; | |
| } | |
| json pgsql_result_to_json(PGresult* res) { | |
| json out; | |
| out["columns"] = json::array(); | |
| out["rows"] = json::array(); | |
| if (!res) { | |
| return out; | |
| } | |
| const int num_fields = PQnfields(res); | |
| const int num_rows = PQntuples(res); | |
| std::vector<std::string> column_names; | |
| column_names.reserve(num_fields); | |
| for (int i = 0; i < num_fields; ++i) { | |
| const char* name = PQfname(res, i); | |
| column_names.emplace_back(name ? name : "unknown_field"); | |
| out["columns"].push_back(name ? name : "unknown_field"); | |
| } | |
| for (int r = 0; r < num_rows; ++r) { | |
| json row_obj = json::object(); | |
| for (int c = 0; c < num_fields; ++c) { | |
| row_obj[column_names[c]] = PQgetisnull(res, r, c) ? "" : PQgetvalue(res, r, c); | |
| } | |
| out["rows"].push_back(row_obj); | |
| } | |
| return out; | |
| } | |
| } // namespace |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 82-82: Call a function from instead of a function from <ctype.h>.
[failure] 77-77: Call a function from instead of a function from <ctype.h>.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp` around lines 73 - 317,
These file-scope helper functions (is_valid_sql_identifier,
quote_mysql_identifier, quote_pgsql_identifier, escape_mysql_literal,
escape_pgsql_literal, json_scalar_to_sql_literal_mysql,
json_scalar_to_sql_literal_pgsql, append_source_pk_predicates_mysql,
append_source_pk_predicates_pgsql, mysql_result_to_json, pgsql_result_to_json)
must have internal linkage to avoid ODR/linker collisions; wrap their
definitions in the same anonymous namespace used later in the file (or
alternatively mark each function static) so they are file-local and remain
discoverable by name in the diff (e.g., enclose the above function definitions
in "namespace { ... }"). Ensure you move only the helper definitions (not their
callers) into that anonymous namespace.
| if (!result || result->rows.empty()) { | ||
| if (sqlite_error) { | ||
| error = sqlite_error; | ||
| free(sqlite_error); | ||
| } else { | ||
| error = "Document not found or source disabled: " + doc_id; | ||
| } | ||
| if (result) { | ||
| delete result; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm execute_statement's error allocator and check for inconsistent free patterns.
ast-grep --pattern $'$_ SQLite3DB::execute_statement($$$) {
$$$
}'
rg -nP -C2 '\bexecute_statement\s*\([^;]*&error' --type=cpp
rg -nP -C1 '(proxy_sqlite3_free|\bfree)\s*\(\s*(error|sqlite_error)\s*\)' --type=cppRepository: sysown/proxysql
Length of output: 50372
🏁 Script executed:
# Read the specific lines around 698 and 1029-1040 in RAG_Tool_Handler.cpp
head -n 710 plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp | tail -n 30
echo "---"
head -n 1045 plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp | tail -n 30Repository: sysown/proxysql
Length of output: 1768
🏁 Script executed:
# Search for proxy_sqlite3_free usage to understand the custom allocator pattern
rg -n "proxy_sqlite3_free" --type=cpp | head -20Repository: sysown/proxysql
Length of output: 1044
🏁 Script executed:
# Look at how error is freed in different contexts within RAG_Tool_Handler specifically
rg -n "(proxy_sqlite3_free|free)\(" plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp | head -20Repository: sysown/proxysql
Length of output: 177
Use proxy_sqlite3_free() to free sqlite_error for consistency.
Line 702 (near the execute_statement call at line 698) correctly frees the error with (*proxy_sqlite3_free)(error). However, at line 1032 within the same file, free(sqlite_error) is used to free an error from the same execute_statement() call. This allocator mismatch can cause undefined behavior if a custom SQLite allocator is configured. Align both sites with the established pattern in the codebase.
Suggested change
- if (sqlite_error) {
- error = sqlite_error;
- free(sqlite_error);
- } else {
+ if (sqlite_error) {
+ error = sqlite_error;
+ (*proxy_sqlite3_free)(sqlite_error);
+ } else {
error = "Document not found or source disabled: " + doc_id;
}📝 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.
| if (!result || result->rows.empty()) { | |
| if (sqlite_error) { | |
| error = sqlite_error; | |
| free(sqlite_error); | |
| } else { | |
| error = "Document not found or source disabled: " + doc_id; | |
| } | |
| if (result) { | |
| delete result; | |
| } | |
| return false; | |
| } | |
| if (!result || result->rows.empty()) { | |
| if (sqlite_error) { | |
| error = sqlite_error; | |
| (*proxy_sqlite3_free)(sqlite_error); | |
| } else { | |
| error = "Document not found or source disabled: " + doc_id; | |
| } | |
| if (result) { | |
| delete result; | |
| } | |
| return false; | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 1037-1037: Rewrite the code so that you no longer need this "delete".
[failure] 1032-1032: Remove this use of "free".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp` around lines 1029 -
1040, The code frees sqlite_error with free(sqlite_error) but earlier (and
elsewhere) the code uses the SQLite-proxy allocator via proxy_sqlite3_free;
change the free call in the error path inside the block (where sqlite_error is
checked before setting error) to call the proxy free function (e.g.
(*proxy_sqlite3_free)(sqlite_error) or proxy_sqlite3_free(sqlite_error))
instead, keeping the rest of the control flow in the same block (variables:
sqlite_error, execute_statement, proxy_sqlite3_free, result).
| std::string sql = "SELECT " + select_list + " FROM " + quote_mysql_identifier(cfg.table_name) + " WHERE " + where_clause; | ||
| if (!cfg.where_sql.empty()) { | ||
| sql += " AND (" + cfg.where_sql + ")"; | ||
| } | ||
| sql += " LIMIT 2"; |
There was a problem hiding this comment.
Document/limit the trust boundary on cfg.where_sql.
cfg.where_sql is concatenated verbatim into the source SELECT (same at lines 1194–1198 for PG). All other inputs in this builder go through identifier validation or proper escaping, so this is the one free-form SQL fragment in the path. Even though rag_sources.where_sql is operator-controlled (loaded from the local rag catalog), an SQL injection into that catalog or an operator typo can now reach the upstream MySQL/PostgreSQL with admin privileges of the configured backend_user. At minimum, please add a comment documenting that the catalog is trusted and require it to be a single boolean expression; ideally validate that it does not contain ;, comments, or statement-terminators on load.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/genai/src/tool_handlers/RAG_Tool_Handler.cpp` around lines 1118 -
1122, The concatenation of cfg.where_sql directly into the SELECT creates an
untrusted SQL injection surface; update the builder (the code that constructs
sql using cfg.where_sql in RAG_Tool_Handler.cpp around the SELECT/WHERE
construction and the analogous PG block) to (1) add a clear comment that
rag_sources.where_sql is a trusted catalog value and is a trust boundary, (2)
validate cfg.where_sql at load time or before use to enforce it is a single
boolean expression — reject or fail if it contains characters/sequences that can
terminate or inject statements such as ';', '--', '/*', '*/', newlines, or
multiple top-level statements, and (3) if validation fails, refuse to use the
fragment (e.g., error out or ignore it) rather than concatenating; reference
cfg.where_sql and quote_mysql_identifier in your changes and apply the same
validation and comment to the PostgreSQL construction at the other location.
| PROTOBUF_VERSION := 3.21.12 | ||
| PROTOBUF_SRC_DIR := $(PROXYSQL_PATH)/deps/protobuf/protobuf-$(PROTOBUF_VERSION) | ||
| PROTOBUF_INSTALL_DIR := $(PROTOBUF_SRC_DIR)/install | ||
| PROTOBUF_INCLUDE_DIR := $(PROTOBUF_INSTALL_DIR)/include | ||
| PROTOBUF_LIB := $(PROTOBUF_INSTALL_DIR)/lib/libprotobuf.a |
There was a problem hiding this comment.
Downstream of the lib vs lib64 issue raised in deps/Makefile.
PROTOBUF_LIB here mirrors the path computed in deps/Makefile and inherits the same assumption that the protobuf install lands under …/install/lib/. On RHEL/CentOS/Fedora x86_64 the install directory will be …/install/lib64/ and the prerequisite $(PROTOBUF_LIB) will never be satisfied, breaking the $(PLUGIN_SO) link rule (line 134).
Root-cause fix is in deps/Makefile (passing -DCMAKE_INSTALL_LIBDIR=lib to the CMake configure step). Once that lands, the path defined here is consistent on all supported distros and no change is needed in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/mysqlx/Makefile` around lines 30 - 34, The PROTOBUF_LIB path in this
Makefile assumes protobuf installed to $(PROTOBUF_INSTALL_DIR)/lib but some
distros install to lib64; fix the root cause by updating deps/Makefile's CMake
configure step to force a consistent install lib dir (add
-DCMAKE_INSTALL_LIBDIR=lib when configuring protobuf/builds) so that
PROTOBUF_INSTALL_DIR, PROTOBUF_INCLUDE_DIR and PROTOBUF_LIB resolve correctly
across platforms and the $(PLUGIN_SO) link prerequisite is satisfied.
Add genai_config_query_unit-t and genai_rag_fetch_from_source_unit-t to groups.json under ai-g1 (requires genai plugin) with @proxysql_min_version:4.0 constraint, matching the pattern of other genai feature unit tests (discovery_schema, fts_string, etc.).
CI-unit-tests-asan-coverage failed with "30 tests planned but 31 executed" The helper expect_query_value() emits 5 ok() calls and is invoked twice (10 assertions), plus 21 inline ok() calls in main(), for 31 assertions total. plan(30) -> plan(31) so the harness exit status reflects the real assertion count.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deps/Makefile (1)
182-196:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
-DCMAKE_INSTALL_LIBDIR=libto the protobuf CMake configuration.The recipe hardcodes the artifact path as
$(PROTOBUF_INSTALL_DIR)/lib/libprotobuf.a(line 180), but the CMake configuration does not pinCMAKE_INSTALL_LIBDIR. Protobuf 3.21.12's CMake includesGNUInstallDirs, which defaultsLIBDIRtolib64on 64-bit RHEL, CentOS, and Fedora systems. This causes the install to write to.../install/lib64/libprotobuf.a, breaking the build on those distros since the expected target never exists at the hardcodedlib/path.🛠 Proposed fix
cd protobuf/protobuf-$(PROTOBUF_VERSION) && CC=${CC} CXX=${CXX} cmake -S . -B build \ -DCMAKE_BUILD_TYPE=RelWithDebInfo \ -DCMAKE_INSTALL_PREFIX=$(PROTOBUF_INSTALL_DIR) \ + -DCMAKE_INSTALL_LIBDIR=lib \ -DCMAKE_POSITION_INDEPENDENT_CODE=ON \ -Dprotobuf_BUILD_TESTS=OFF \ -Dprotobuf_BUILD_CONFORMANCE=OFF \ -Dprotobuf_BUILD_EXAMPLES=OFF \ -Dprotobuf_BUILD_PROTOC_BINARIES=OFF \ -Dprotobuf_BUILD_LIBPROTOC=OFF \ -Dprotobuf_BUILD_SHARED_LIBS=OFF \ -Dprotobuf_WITH_ZLIB=OFF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deps/Makefile` around lines 182 - 196, The protobuf CMake invocation in the $(PROTOBUF_LIB) recipe doesn't set CMAKE_INSTALL_LIBDIR, causing installs on some distros to go to lib64 and mismatch the hardcoded artifact path; update the cmake call inside the protobuf/protobuf-$(PROTOBUF_VERSION) step (the CC=${CC} CXX=${CXX} cmake -S . -B build ...) to include -DCMAKE_INSTALL_LIBDIR=lib so the installed libraries land under $(PROTOBUF_INSTALL_DIR)/lib/ as expected, ensuring the subsequent ${MAKE} -C build install produces $(PROTOBUF_INSTALL_DIR)/lib/libprotobuf.a.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deps/Makefile`:
- Line 198: Add the protobuf target to the Makefile's phony declarations so make
treats it as an alias rather than a filename: update the .PHONY list (or add a
new .PHONY line) to include the target name "protobuf" to mirror how other phony
targets like "default", "cleanpart", and "cleanall" are declared; ensure the
line references the exact target symbol protobuf so make won't look for a file
named "protobuf".
---
Duplicate comments:
In `@deps/Makefile`:
- Around line 182-196: The protobuf CMake invocation in the $(PROTOBUF_LIB)
recipe doesn't set CMAKE_INSTALL_LIBDIR, causing installs on some distros to go
to lib64 and mismatch the hardcoded artifact path; update the cmake call inside
the protobuf/protobuf-$(PROTOBUF_VERSION) step (the CC=${CC} CXX=${CXX} cmake -S
. -B build ...) to include -DCMAKE_INSTALL_LIBDIR=lib so the installed libraries
land under $(PROTOBUF_INSTALL_DIR)/lib/ as expected, ensuring the subsequent
${MAKE} -C build install produces $(PROTOBUF_INSTALL_DIR)/lib/libprotobuf.a.
🪄 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: 0cff0768-c948-4ea1-9256-eb142a4cad78
📒 Files selected for processing (2)
deps/Makefiletest/tap/groups/groups.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tap/groups/groups.json
📜 Review details
🧰 Additional context used
🪛 checkmake (0.3.2)
deps/Makefile
[warning] 198-198: Target "protobuf" should be declared PHONY.
(phonydeclared)
[warning] 182-182: Target body for "$(PROTOBUF_LIB)" exceeds allowed length of 5 lines (14).
(maxbodylength)
🔇 Additional comments (4)
deps/Makefile (4)
49-51: LGTM!
177-180: LGTM!
430-430: LGTM!
457-457: LGTM!
| -Dprotobuf_WITH_ZLIB=OFF | ||
| cd protobuf/protobuf-$(PROTOBUF_VERSION) && CC=${CC} CXX=${CXX} ${MAKE} -C build install | ||
|
|
||
| protobuf: $(PROTOBUF_LIB) |
There was a problem hiding this comment.
Declare the protobuf target as .PHONY.
The protobuf target is a phony target (an alias, not a file), and should be declared as such to prevent make from checking for a file named "protobuf" in the current directory. This follows the pattern used for other phony targets in this Makefile (e.g., default at line 62, cleanpart at line 431, cleanall at line 460).
🛠 Proposed fix
protobuf: $(PROTOBUF_LIB)
+.PHONY: protobuf📝 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.
| protobuf: $(PROTOBUF_LIB) | |
| protobuf: $(PROTOBUF_LIB) | |
| .PHONY: protobuf |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 198-198: Target "protobuf" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deps/Makefile` at line 198, Add the protobuf target to the Makefile's phony
declarations so make treats it as an alias rather than a filename: update the
.PHONY list (or add a new .PHONY line) to include the target name "protobuf" to
mirror how other phony targets like "default", "cleanpart", and "cleanall" are
declared; ensure the line references the exact target symbol protobuf so make
won't look for a file named "protobuf".
|
…verage is cancelled The previous pattern opened a LouisBrunner check_run with status=in_progress at job start and closed it via check_id at job end with if: always(). When the runner is killed mid-job (concurrency:cancel-in-progress, OOM, infra shutdown), the closing step never executes and the check_run is left in in_progress forever, blocking the PR rollup. Collapse to a single LouisBrunner call at the end: pass name: instead of check_id: so the action creates the check_run with a terminal conclusion in one shot. If the job is cancelled before the closing step runs, no check_run is ever created (PR rollup shows "missing" — recoverable via re-run) instead of "stuck" — unrecoverable without GitHub App auth. Refs: stuck check_run 77381366265 on PR sysown#5752 (sha cf0b1d3) from 2026-05-22 12:39 UTC. Same pattern exists across ~163 other CI-* workflows; fixing only the workflow with the observed incident here. Sweep of the remaining files belongs in a separate PR.
1 participant
Summary
This branch bundles the current GenAI and build-system work on top of
v3.0.What changed
/mcp/config.query, a server-side SQL execution tool for the admin DB with explicit statement/token restrictions and structured results.deps/and links mysqlx against that local archive.rag.fetch_from_sourcepath in the GenAI MCP stack so RAG documents can be refetched from the source DB instead of returning stubbed results.Verification
PROXYSQL40=1 make debug -j24PROXYSQL40=1 make build_tap_test_debug -j24Notes
v3.0.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores