Add RAG ingestion with vector embeddings#5318
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a RAG ingestion system that synchronizes MySQL data to SQLite with optional embedding generation, incremental sync tracking, and vector extension support. Updates catalog search to use FTS5 with BM25 ranking. Adds build orchestration for RAG components and comprehensive end-to-end test workflow. Changes
Sequence DiagramsequenceDiagram
participant RAG as RAG Ingest
participant MySQL as MySQL<br/>(Source)
participant Chunk as Chunker
participant Embed as Embedding<br/>Provider
participant SQLite as SQLite<br/>(rag_vec_chunks)
RAG->>SQLite: Load sync cursor
RAG->>MySQL: SELECT with incremental filter
MySQL-->>RAG: Row batch
RAG->>Chunk: Create document chunks
Chunk-->>RAG: Chunk list
RAG->>Embed: embed(chunks)
alt OpenAI Provider
Embed->>Embed: Build HTTP request
Embed->>Embed: Call curl OpenAI API
Embed-->>Embed: Parse embeddings
else Stub Provider
Embed-->>Embed: Generate pseudo embeddings
end
Embed-->>RAG: Vector embeddings
RAG->>SQLite: INSERT rag_vec_chunks<br/>(doc_id, chunk_text, embedding)
RAG->>SQLite: UPDATE sync cursor
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Summary of ChangesHello @rahim-kanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new RAG ingestion mechanism, enabling the system to ingest documents, generate vector embeddings, and create a full-text search index. This enhancement significantly improves the system's ability to perform semantic search and retrieve relevant information from large document collections. The implementation includes support for incremental synchronization, chunking, and various embedding providers, along with a comprehensive test suite. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 RAG (Retrieval-Augmented Generation) ingester PoC, including changes to the Makefile to build the RAG ingester, a new Makefile and source file (rag_ingest.cpp) in the RAG_POC directory, sample SQL files for MySQL and SQLite, and a test script (test_rag_ingest.sh). The code changes in rag_ingest.cpp involve including necessary headers, defining functions for SQLite interaction, JSON parsing, and embedding handling (including stub and OpenAI embedding providers), and implementing the core ingestion logic with watermark support. Review comments highlight the importance of using $(CXXFLAGS) directly in the Makefile, potential portability issues with the MySQL header path, the need for libcrypt availability for the sha256_crypt_r weak alias, potential performance improvements by batching embedding inputs, providing more explicit error feedback for missing query parameters in catalog searches, verifying the ranking behavior of the bm25 function, and simplifying parameter binding in the MySQL_Catalog::search function by removing unnecessary variables. Additionally, the cleanup of mysql_tool_handler in the destructor and the correct implementation of escape_sql lambda for preventing SQL injection are noted as crucial improvements.
| auto escape_sql = [](const std::string& str) -> std::string { | ||
| std::string result; | ||
| result.reserve(str.length() * 2); // Reserve space for potential escaping | ||
| for (char c : str) { | ||
| if (c == '\'') { | ||
| result += '\''; // Escape single quote by doubling it | ||
| } | ||
| result += c; | ||
| } | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
The escape_sql lambda is correctly implemented to prevent SQL injection for FTS5 MATCH clauses. This is a critical security improvement, ensuring that user-provided queries are safely handled before being incorporated into the SQL statement.
| // Clean up MySQL Tool Handler | ||
| if (handler->mysql_tool_handler) { | ||
| proxy_info("Cleaning up MySQL Tool Handler...\n"); | ||
| delete handler->mysql_tool_handler; | ||
| handler->mysql_tool_handler = NULL; |
There was a problem hiding this comment.
Adding cleanup for mysql_tool_handler in the destructor is a crucial improvement for resource management, preventing potential memory leaks when the ProxySQL_MCP_Server is destroyed.
| # RAG ingester (PoC) | ||
| .PHONY: rag_ingest | ||
| rag_ingest: build_deps | ||
| cd RAG_POC && ${MAKE} CC=${CC} CXX=${CXX} OPTZ="${O2} -ggdb" CXXFLAGS="${O2} -ggdb" |
There was a problem hiding this comment.
Consider using $(CXXFLAGS) directly here instead of re-specifying OPTZ and CXXFLAGS with "${O2} -ggdb". This would make the build process more consistent with the overall Makefile's philosophy of setting CXXFLAGS once and reusing it, and easier to change build flags globally.
For example, if CXXFLAGS is already defined as "-std=c++17 -O2 -ggdb", then CXXFLAGS="${O2} -ggdb" would override it unnecessarily or potentially lead to unexpected behavior if O2 is later changed.
cd RAG_POC && ${MAKE} CC=${CC} CXX=${CXX} CXXFLAGS="${CXXFLAGS}"
|
|
||
| #include <sqlite3.h> | ||
| #include <mysql/mysql.h> | ||
| #include <mysql.h> |
There was a problem hiding this comment.
Changing from <mysql/mysql.h> to <mysql.h> might cause portability issues on systems where the MySQL client library headers are not directly in the default include path but rather in a subdirectory like mysql/. It's generally safer to use the more specific path if it's known to work across different environments or ensure the build system correctly adds the parent directory to the include paths.
| extern "C" __attribute__((weak)) char *sha256_crypt_r( | ||
| const char *key, | ||
| const char *salt, | ||
| char *buffer, | ||
| int buflen) { | ||
| if (!key || !salt || !buffer || buflen <= 0) { | ||
| return nullptr; | ||
| } | ||
| struct crypt_data data; | ||
| std::memset(&data, 0, sizeof(data)); | ||
| char *res = crypt_r(key, salt, &data); | ||
| if (!res) { | ||
| return nullptr; | ||
| } | ||
| size_t len = std::strlen(res); | ||
| if (len + 1 > static_cast<size_t>(buflen)) { | ||
| return nullptr; | ||
| } | ||
| std::memcpy(buffer, res, len + 1); | ||
| return buffer; | ||
| } |
There was a problem hiding this comment.
The sha256_crypt_r weak alias is a good workaround for potential library inconsistencies. However, it's worth noting that crypt_r is part of libcrypt, which might not be available on all systems or might require specific linking flags. This could lead to build issues on some platforms. Ensure the build environment (e.g., Docker images, CI/CD) consistently provides libcrypt or a compatible alternative.
| std::vector<std::string> batch_inputs = {emb_input}; | ||
| std::vector<std::vector<float>> vecs = embedder->embed(batch_inputs, ecfg.dim); | ||
| sqlite_insert_vec(ss, vecs[0], chunk_id, doc.doc_id, src.source_id, now_epoch); |
There was a problem hiding this comment.
Currently, embedder->embed is called for each chunk individually, passing a batch_inputs vector with a single element. The EmbeddingConfig includes a batch_size parameter, suggesting that the embedding provider might support batching multiple inputs for efficiency. Consider refactoring the ingestion loop to collect multiple emb_input strings and call embedder->embed once per batch, especially if ecfg.batch_size is greater than 1. This could significantly improve performance when processing many small chunks.
| if (query.empty()) { | ||
| proxy_error("Catalog search requires a query parameter\n"); | ||
| return "[]"; |
There was a problem hiding this comment.
The proxy_error message Catalog search requires a query parameter is good for indicating a missing required parameter. However, returning an empty JSON array "[]" might mask the error from the caller. Consider throwing an exception or returning a JSON object with an error message to provide more explicit feedback to the client about the failed request.
|
|
||
| sql << " ORDER BY updated_at DESC LIMIT ? OFFSET ?"; | ||
| // Order by relevance (BM25) and recency | ||
| sql << " ORDER BY bm25(f) ASC, c.updated_at DESC LIMIT ? OFFSET ?"; |
There was a problem hiding this comment.
The ORDER BY bm25(f) ASC, c.updated_at DESC clause is a good choice for hybrid relevance and recency. However, bm25 scores are typically lower for more relevant results, so ASC ordering would prioritize more relevant results. This seems correct for FTS, but ensure this aligns with the desired ranking behavior.
| if (!schema.empty()) { | ||
| (*proxy_sqlite3_bind_text)(stmt, param_idx++, schema.c_str(), -1, SQLITE_TRANSIENT); |
There was a problem hiding this comment.
The schema_pattern and kind_pattern variables are no longer needed since the schema and kind parameters are directly bound without wildcard wrapping. The tags_pattern still requires wrapping with % for LIKE operator.
| if (!schema.empty()) { | |
| (*proxy_sqlite3_bind_text)(stmt, param_idx++, schema.c_str(), -1, SQLITE_TRANSIENT); | |
| if (!schema.empty()) { | |
| (*proxy_sqlite3_bind_text)(stmt, param_idx++, schema.c_str(), -1, SQLITE_TRANSIENT); | |
| } | |
| if (!kind.empty()) { | |
| (*proxy_sqlite3_bind_text)(stmt, param_idx++, kind.c_str(), -1, SQLITE_TRANSIENT); | |
| } | |
| if (!tags.empty()) { |
- Fix Makefile: Use $(CXXFLAGS) directly for consistency with build philosophy - Fix MySQL_Catalog: Return proper error JSON instead of empty array on missing query
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
RAG_POC/rag_ingest.cpp (2)
993-1005: Potential NULL pointer dereference fromsqlite3_column_text.If any of these columns contain NULL values,
sqlite3_column_textreturns NULL, and assigning NULL tostd::stringis undefined behavior. Thewhere_sqlcolumn usesCOALESCE, but others don't.🐛 Suggested fix
- s.name = (const char*)sqlite3_column_text(st, 1); + s.name = str_or_empty((const char*)sqlite3_column_text(st, 1)); s.enabled = sqlite3_column_int(st, 2); - s.backend_type = (const char*)sqlite3_column_text(st, 3); - s.host = (const char*)sqlite3_column_text(st, 4); + s.backend_type = str_or_empty((const char*)sqlite3_column_text(st, 3)); + s.host = str_or_empty((const char*)sqlite3_column_text(st, 4)); s.port = sqlite3_column_int(st, 5); - s.user = (const char*)sqlite3_column_text(st, 6); - s.pass = (const char*)sqlite3_column_text(st, 7); - s.db = (const char*)sqlite3_column_text(st, 8); + s.user = str_or_empty((const char*)sqlite3_column_text(st, 6)); + s.pass = str_or_empty((const char*)sqlite3_column_text(st, 7)); + s.db = str_or_empty((const char*)sqlite3_column_text(st, 8)); - s.table_name = (const char*)sqlite3_column_text(st, 9); - s.pk_column = (const char*)sqlite3_column_text(st, 10); + s.table_name = str_or_empty((const char*)sqlite3_column_text(st, 9)); + s.pk_column = str_or_empty((const char*)sqlite3_column_text(st, 10));
1361-1368: Missingcurl_global_cleanupon error exit path.When the transaction fails and the function returns at line 1364,
curl_global_cleanup()is not called. While this is a minor issue for a CLI tool (resources are freed on process exit), it's good practice to ensure proper cleanup.♻️ Suggested fix
} else { sqlite_exec(db, "ROLLBACK;"); sqlite3_close(db); + curl_global_cleanup(); return 1; }lib/MySQL_Catalog.cpp (1)
387-388: Guard NULL tags/links to avoidstd::string(nullptr)crash.
tagsandlinksare nullable columns. If a row has NULL,sqlite3_column_textreturns nullptr and constructingstd::stringis undefined behavior. Add a null guard or COALESCE.🐛 Suggested fix (null-safe extraction)
- entry["tags"] = std::string((const char*)(*proxy_sqlite3_column_text)(stmt, 4)); - entry["links"] = std::string((const char*)(*proxy_sqlite3_column_text)(stmt, 5)); + const char* tags_str = (const char*)(*proxy_sqlite3_column_text)(stmt, 4); + entry["tags"] = tags_str ? std::string(tags_str) : nullptr; + const char* links_str = (const char*)(*proxy_sqlite3_column_text)(stmt, 5); + entry["links"] = links_str ? std::string(links_str) : nullptr;
🤖 Fix all issues with AI agents
In `@RAG_POC/Makefile`:
- Line 16: The Makefile's LIBS variable is missing the libcurl linker flag
causing link errors when rag_ingest.cpp (which includes <curl/curl.h> and uses
libcurl for the OpenAI embedding provider) is built; update the LIBS variable to
add -lcurl (e.g., append -lcurl to LIBS) so the linker includes libcurl when
linking targets that depend on rag_ingest.cpp.
In `@RAG_POC/rag_ingest.cpp`:
- Around line 1214-1226: The code uses std::stoll on strings validated by
is_integer_string but doesn't guard against std::out_of_range; wrap both places
where std::stoll is called (the initial assignment that sets max_num and the
comparison path that computes nv) in try/catch blocks catching std::out_of_range
(and std::invalid_argument for safety), and on catch switch to the
string-comparison branch by setting max_numeric = false and max_str = v (and
preserve max_set = true) so huge integers won't throw and will be compared
lexicographically instead; update both uses (the assignment setting max_num and
the nv comparison) to follow this pattern.
In `@RAG_POC/test_rag_ingest.sh`:
- Around line 109-122: The functions import_mysql_seed and run_mysql_sql
currently pass the password via -p"${MYSQL_PASS}", which leaks credentials;
change both to avoid the -p flag and instead supply the password via the
MYSQL_PWD environment variable or a temporary defaults file: invoke the mysql
binary with MYSQL_PWD="${MYSQL_PASS}" "${MYSQL_BIN}" -h"${MYSQL_HOST}"
-P"${MYSQL_PORT}" -u"${MYSQL_USER}" ... (or create a secure
--defaults-extra-file and point mysql at it), and ensure you unset or clear
MYSQL_PWD immediately after the command to avoid lingering secrets in the
environment or logs.
- Around line 31-36: The script always exports OPENAI_API_BASE and
OPENAI_API_KEY which forces the OpenAI-compatible path to run with placeholder
credentials; update test_rag_ingest.sh to stop unconditionally exporting
OPENAI_API_BASE and OPENAI_API_KEY and instead gate those exports behind an
explicit opt-in (e.g. check an ENABLE_OPENAI_TEST or similar env var) or keep
them commented out by default so Phase 5 only runs when the user opt-ins; ensure
references to OPENAI_API_BASE, OPENAI_API_KEY (and optionally
OPENAI_MODEL/EMBEDDING_PROVIDER) are only set when the opt-in flag is true.
♻️ Duplicate comments (1)
RAG_POC/rag_ingest.cpp (1)
1270-1275: Embedding calls are not batched despitebatch_sizeconfig.The
EmbeddingConfig.batch_sizeparameter is parsed but unused. Each chunk triggers a separate API call, which is inefficient for external providers like OpenAI. Consider collecting chunks and callingembedder->embedin batches.
🧹 Nitpick comments (6)
RAG_POC/rag_ingest.cpp (3)
173-181: Consider using parameterized queries instead of manual escaping.The
sql_escape_single_quotesfunction only escapes single quotes by doubling them. While the values come from trusted sources (sync cursor stored in SQLite), this escaping approach doesn't handle all edge cases (e.g., backslash escapes in certain MySQL modes).For more robust SQL construction, consider using MySQL prepared statements for the incremental filter instead of string concatenation.
Also applies to: 626-633
936-939: Wrap JSON parsing in try-catch to provide clearer error messages.If the embedding API returns malformed JSON or an unexpected response format,
json::parsewill throw a generic exception. Wrapping this in a try-catch would allow providing a more descriptive error message including the response status and partial body for debugging.♻️ Suggested improvement
- json resp = json::parse(buf.data); + json resp; + try { + resp = json::parse(buf.data); + } catch (const json::parse_error& e) { + throw std::runtime_error("Failed to parse embedding response: " + std::string(e.what())); + }
1249-1253:updated_atis always set to 0 in vector chunks.As noted in the comments, this stores a placeholder value. For production use, consider querying
SELECT unixepoch()once at the start of ingestion and reusing that value for all inserts, ensuring consistent timestamps within a single run.RAG_POC/Makefile (1)
21-21: Consider adding atestphony target.Static analysis suggests adding a
testtarget for consistency. For a PoC Makefile, this is optional but useful if you want to integrate withmake testworkflows.RAG_POC/test_rag_ingest.sh (2)
59-107: Escape JSON/where_sql before injecting into SQL.
chunking_json_value,embedding_json_value, andwhere_sqlare interpolated into SQL literals. A single quote in any override will break the statement and can make the test brittle. Consider escaping single quotes before injecting.♻️ Proposed fix (escape single quotes)
apply_schema_and_source() { local db="$1" local where_sql="$2" local load_schema="$3" local chunking_json_override="${4:-}" local embedding_json_override="${5:-}" local schema_override_path="${6:-}" + local sql_escape + sql_escape() { printf "%s" "$1" | sed "s/'/''/g"; } local schema_cmd="" if [[ "${load_schema}" == "true" ]]; then @@ - local embedding_json_value='{"enabled":false}' + local embedding_json_value='{"enabled":false}' if [[ -n "${embedding_json_override}" ]]; then embedding_json_value="${embedding_json_override}" fi echo "==> embedding_json: ${embedding_json_value}" + local chunking_json_sql + local embedding_json_sql + local where_sql_sql + chunking_json_sql="$(sql_escape "${chunking_json_value}")" + embedding_json_sql="$(sql_escape "${embedding_json_value}")" + where_sql_sql="$(sql_escape "${where_sql}")" + "${SQLITE_BIN}" "${db}" <<SQL .load ${VEC_EXT} @@ - UPDATE rag_sources - SET chunking_json='${chunking_json_value}' + UPDATE rag_sources + SET chunking_json='${chunking_json_sql}' WHERE source_id=1; UPDATE rag_sources -SET embedding_json='${embedding_json_value}' +SET embedding_json='${embedding_json_sql}' WHERE source_id=1; UPDATE rag_sources -SET where_sql='${where_sql}' +SET where_sql='${where_sql_sql}' WHERE source_id=1; SQL }
357-359: Sed replacement is brittle against schema formatting changes.The substitution assumes an exact string (
embedding float[1536]). If spacing or the default dimension changes, the replacement silently fails. Consider a more flexible pattern.♻️ Proposed fix (regex-based)
- sed "s/embedding float\[1536\]/embedding float[${OPENAI_EMBEDDING_DIM}]/" "${ROOT_DIR}/schema.sql" > "${OPENAI_SCHEMA_TMP}" + sed -E "s/embedding[[:space:]]+float\\[[0-9]+\\]/embedding float[${OPENAI_EMBEDDING_DIM}]/" "${ROOT_DIR}/schema.sql" > "${OPENAI_SCHEMA_TMP}"
|
|
||
| SQLITE3_OBJ := $(ROOT_DIR)/deps/sqlite3/sqlite-amalgamation-3500400/sqlite3.o | ||
|
|
||
| LIBS := -lmariadbclient -lssl -lcrypto -lcrypt -ldl -lpthread |
There was a problem hiding this comment.
Missing -lcurl in LIBS.
The rag_ingest.cpp file includes <curl/curl.h> and uses libcurl for the OpenAI embedding provider. The -lcurl library flag is missing from the LIBS variable, which will cause linker errors when embedding is enabled.
🐛 Fix
-LIBS := -lmariadbclient -lssl -lcrypto -lcrypt -ldl -lpthread
+LIBS := -lmariadbclient -lssl -lcrypto -lcrypt -lcurl -ldl -lpthread📝 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.
| LIBS := -lmariadbclient -lssl -lcrypto -lcrypt -ldl -lpthread | |
| LIBS := -lmariadbclient -lssl -lcrypto -lcrypt -lcurl -ldl -lpthread |
🤖 Prompt for AI Agents
In `@RAG_POC/Makefile` at line 16, The Makefile's LIBS variable is missing the
libcurl linker flag causing link errors when rag_ingest.cpp (which includes
<curl/curl.h> and uses libcurl for the OpenAI embedding provider) is built;
update the LIBS variable to add -lcurl (e.g., append -lcurl to LIBS) so the
linker includes libcurl when linking targets that depend on rag_ingest.cpp.
| if (cursor.numeric || is_integer_string(v)) { | ||
| max_numeric = true; | ||
| max_num = std::stoll(v); | ||
| } else { | ||
| max_numeric = false; | ||
| max_str = v; | ||
| } | ||
| max_set = true; | ||
| } else if (max_numeric) { | ||
| if (is_integer_string(v)) { | ||
| std::int64_t nv = std::stoll(v); | ||
| if (nv > max_num) max_num = nv; | ||
| } |
There was a problem hiding this comment.
std::stoll can throw std::out_of_range for values exceeding int64 limits.
While is_integer_string validates the format, it doesn't check if the value fits in an int64_t. If the watermark column contains very large numeric strings, std::stoll will throw an exception that isn't caught here, causing the ingestion to abort.
♻️ Suggested improvement
if (cursor.numeric || is_integer_string(v)) {
+ try {
max_numeric = true;
max_num = std::stoll(v);
+ } catch (const std::out_of_range&) {
+ // Fall back to string comparison for very large values
+ max_numeric = false;
+ max_str = v;
+ }
} else {🤖 Prompt for AI Agents
In `@RAG_POC/rag_ingest.cpp` around lines 1214 - 1226, The code uses std::stoll on
strings validated by is_integer_string but doesn't guard against
std::out_of_range; wrap both places where std::stoll is called (the initial
assignment that sets max_num and the comparison path that computes nv) in
try/catch blocks catching std::out_of_range (and std::invalid_argument for
safety), and on catch switch to the string-comparison branch by setting
max_numeric = false and max_str = v (and preserve max_set = true) so huge
integers won't throw and will be compared lexicographically instead; update both
uses (the assignment setting max_num and the nv comparison) to follow this
pattern.
| # Uncomment to test OpenAI-compatible embeddings | ||
| # export EMBEDDING_PROVIDER=openai | ||
| # export EMBEDDING_DIM=1536 | ||
| export OPENAI_API_BASE="https://api.synthetic.new/openai/v1" | ||
| export OPENAI_API_KEY="your_api_key_here" | ||
| # export OPENAI_MODEL="hf:nomic-ai/nomic-embed-text-v1.5" |
There was a problem hiding this comment.
OpenAI test is always enabled due to unconditional exports.
These exports make the Phase 5 guard always true, so the script will attempt an OpenAI-compatible call with placeholder credentials. This is likely to fail in CI and may unintentionally hit external services. Keep these commented or gate them behind an explicit opt-in flag.
🐛 Proposed fix (opt-in only)
-# export EMBEDDING_PROVIDER=openai
-# export EMBEDDING_DIM=1536
-export OPENAI_API_BASE="https://api.synthetic.new/openai/v1"
-export OPENAI_API_KEY="your_api_key_here"
-# export OPENAI_MODEL="hf:nomic-ai/nomic-embed-text-v1.5"
+# export EMBEDDING_PROVIDER=openai
+# export EMBEDDING_DIM=1536
+# export OPENAI_API_BASE="https://api.synthetic.new/openai/v1"
+# export OPENAI_API_KEY="your_api_key_here"
+# export OPENAI_MODEL="hf:nomic-ai/nomic-embed-text-v1.5"🤖 Prompt for AI Agents
In `@RAG_POC/test_rag_ingest.sh` around lines 31 - 36, The script always exports
OPENAI_API_BASE and OPENAI_API_KEY which forces the OpenAI-compatible path to
run with placeholder credentials; update test_rag_ingest.sh to stop
unconditionally exporting OPENAI_API_BASE and OPENAI_API_KEY and instead gate
those exports behind an explicit opt-in (e.g. check an ENABLE_OPENAI_TEST or
similar env var) or keep them commented out by default so Phase 5 only runs when
the user opt-ins; ensure references to OPENAI_API_BASE, OPENAI_API_KEY (and
optionally OPENAI_MODEL/EMBEDDING_PROVIDER) are only set when the opt-in flag is
true.
| import_mysql_seed() { | ||
| "${MYSQL_BIN}" \ | ||
| -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \ | ||
| -u"${MYSQL_USER}" -p"${MYSQL_PASS}" \ | ||
| < "${ROOT_DIR}/sample_mysql.sql" | ||
| } | ||
|
|
||
| run_mysql_sql() { | ||
| local sql="$1" | ||
| "${MYSQL_BIN}" \ | ||
| -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \ | ||
| -u"${MYSQL_USER}" -p"${MYSQL_PASS}" \ | ||
| -e "${sql}" | ||
| } |
There was a problem hiding this comment.
Avoid passing MySQL passwords on the command line.
-p"${MYSQL_PASS}" exposes credentials via process listings and CI logs. Prefer MYSQL_PWD (or a defaults file) to avoid leakage.
🔐 Proposed fix (MYSQL_PWD)
-import_mysql_seed() {
- "${MYSQL_BIN}" \
+import_mysql_seed() {
+ MYSQL_PWD="${MYSQL_PASS}" "${MYSQL_BIN}" \
-h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \
- -u"${MYSQL_USER}" -p"${MYSQL_PASS}" \
+ -u"${MYSQL_USER}" \
< "${ROOT_DIR}/sample_mysql.sql"
}
run_mysql_sql() {
local sql="$1"
- "${MYSQL_BIN}" \
+ MYSQL_PWD="${MYSQL_PASS}" "${MYSQL_BIN}" \
-h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \
- -u"${MYSQL_USER}" -p"${MYSQL_PASS}" \
+ -u"${MYSQL_USER}" \
-e "${sql}"
}📝 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.
| import_mysql_seed() { | |
| "${MYSQL_BIN}" \ | |
| -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \ | |
| -u"${MYSQL_USER}" -p"${MYSQL_PASS}" \ | |
| < "${ROOT_DIR}/sample_mysql.sql" | |
| } | |
| run_mysql_sql() { | |
| local sql="$1" | |
| "${MYSQL_BIN}" \ | |
| -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \ | |
| -u"${MYSQL_USER}" -p"${MYSQL_PASS}" \ | |
| -e "${sql}" | |
| } | |
| import_mysql_seed() { | |
| MYSQL_PWD="${MYSQL_PASS}" "${MYSQL_BIN}" \ | |
| -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \ | |
| -u"${MYSQL_USER}" \ | |
| < "${ROOT_DIR}/sample_mysql.sql" | |
| } | |
| run_mysql_sql() { | |
| local sql="$1" | |
| MYSQL_PWD="${MYSQL_PASS}" "${MYSQL_BIN}" \ | |
| -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" \ | |
| -u"${MYSQL_USER}" \ | |
| -e "${sql}" | |
| } |
🤖 Prompt for AI Agents
In `@RAG_POC/test_rag_ingest.sh` around lines 109 - 122, The functions
import_mysql_seed and run_mysql_sql currently pass the password via
-p"${MYSQL_PASS}", which leaks credentials; change both to avoid the -p flag and
instead supply the password via the MYSQL_PWD environment variable or a
temporary defaults file: invoke the mysql binary with MYSQL_PWD="${MYSQL_PASS}"
"${MYSQL_BIN}" -h"${MYSQL_HOST}" -P"${MYSQL_PORT}" -u"${MYSQL_USER}" ... (or
create a secure --defaults-extra-file and point mysql at it), and ensure you
unset or clear MYSQL_PWD immediately after the command to avoid lingering
secrets in the environment or logs.
Merge changes from PR #5318 (RAG ingestion feature) into the new development branch v4.0_rag_ingest_2. Changes include: - RAG ingestion tool (rag_ingest) with chunking and embeddings - MySQL_Catalog fixes for NULL pointer handling - MCP server updates for RAG tools - Comprehensive documentation and test scripts
|
Implements RAG document ingestion with:
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.