WIP-Fixes: Add/Improve TAP tests for MCP query_rules/digests#5321
WIP-Fixes: Add/Improve TAP tests for MCP query_rules/digests#5321renecannao merged 5 commits intov4.0from
Conversation
Since 'proxy_sqlite3_open_v2' is now being set by default due to the new
dedicated TU ('proxy_sqlite3_symbols.cpp'). 'LoadPlugins' might not be
called, not properly setting 'SQLITE_CONFIG_URI' and invalidating many
configuration that are currently specified in the conn URI.
|
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. 📝 WalkthroughWalkthroughThis pull request establishes a comprehensive test infrastructure framework for MCP (MySQL Configuration Protocol) rules in ProxySQL. It introduces centralized test helper utilities, configuration automation, test orchestration, and refactors eight existing test phase scripts to use shared infrastructure while adding a new Phase 9 test phase for OK Message evaluation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 @JavierJF, 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 significantly enhances the testing infrastructure for ProxySQL's Management Console Protocol (MCP) query rules and digests by introducing a new, structured TAP test suite. Concurrently, it resolves a critical bug related to SQLite configuration that was hindering the proper functioning of the MCP server, ensuring more robust and reliable operation of query rule management. 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
|
There was a problem hiding this comment.
Code Review
This PR introduces TAP tests and fixes for MCP query rules and digests, along with a fix for 'SQLITE_CONFIG_URI' not being set on 'LoadPlugins'. The changes include adding new test scripts, renaming existing test files, and modifying the src/main.cpp file to configure SQLITE_CONFIG_URI. Several test scripts were added and modified to improve test coverage for MCP query rules.
This reverts commit df22161.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive TAP test coverage for the MCP (Model Context Protocol) query rules and digests functionality in ProxySQL. The changes introduce a well-structured test suite with 9 phases covering CRUD operations, LOAD/SAVE commands, runtime behavior, statistics, digests, and rule evaluation for different actions (block, rewrite, timeout, OK message).
Changes:
- Added main TAP test runner (
test_mcp_query_rules-t) with environment configuration file - Created 9 phase-specific test scripts covering different aspects of MCP query rules
- Introduced shared helper functions (
mcp_test_helpers.sh) to eliminate code duplication - Added MCP configuration script (
configure_mcp.sh) for test setup - Updated test plan documentation with corrected test ordering
- Removed duplicate SSL certificate files from repository root (copies still exist in test/tap/)
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/tap/tests/test_mcp_query_rules-t | Main TAP test runner orchestrating all phase tests with preflight checks |
| test/tap/tests/test_mcp_query_rules-t.env | Environment variable configuration for test credentials and connection details |
| test/tap/tests/mcp_rules_testing/mcp_test_helpers.sh | Centralized helper functions for MySQL/ProxySQL/MCP operations with proper password handling |
| test/tap/tests/mcp_rules_testing/configure_mcp.sh | MCP server configuration and setup script with HTTPS/HTTP support |
| test/tap/tests/mcp_rules_testing/test_phase*.sh | Nine phase-specific test scripts refactored to use shared helpers |
| test/tap/tests/mcp_rules_testing/template_header.sh | Template for test script headers ensuring consistency |
| test/tap/tests/mcp_rules_testing/claude-test-plan.md | Updated test plan with corrected test sequencing |
| test/tap/tests/mcp_rules_testing/rules/block_rule.sql | Example SQL for creating block rules |
| proxysql-*.pem | Removed duplicate auto-generated test certificates from root |
Comments suppressed due to low confidence (1)
test/tap/tests/mcp_rules_testing/test_mcp_query_rules_block.sh:417
- The password parameter is being set to the username instead of the password. The command should use
$TAP_MYSQLPASSWORDinstead of$TAP_MYSQLUSERNAMEfor the-pflag.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,377 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
The shebang inconsistency should be corrected. The main test runner uses #!/usr/bin/env bash (line 1 of test_mcp_query_rules-t), but most phase test scripts and helper scripts use #!/bin/bash. For better portability and consistency with the main test runner, all bash scripts should use #!/usr/bin/env bash.
| #!/bin/bash | |
| #!/usr/bin/env bash |
| source $(basename $(dirname $0)).env 2>/dev/null | ||
| source $(basename $0 | sed 's/.sh//').env 2>/dev/null |
There was a problem hiding this comment.
The command substitutions should be quoted to prevent word splitting and glob expansion. Lines 29-30 should use "$(basename "$(dirname "$0")")" and "$(basename "$0" | sed 's/.sh//')" to properly handle filenames with spaces or special characters.
| source $(basename $(dirname $0)).env 2>/dev/null | |
| source $(basename $0 | sed 's/.sh//').env 2>/dev/null | |
| source "$(basename "$(dirname "$0")")".env 2>/dev/null | |
| source "$(basename "$0" | sed 's/.sh//')".env 2>/dev/null |
| # T9.1: Query with OK_msg - Health check test | ||
| run_test "T9.1: Query with OK_msg - health_check returns status message" \ | ||
| test_returns_okmsg "run_sql_readonly" "SELECT * FROM health_check;" "Status: OK" | ||
|
|
||
| # T9.2: Verify success response contains OK_msg | ||
| run_test "T9.2: Verify success response contains OK_msg - STATUS query" \ | ||
| test_returns_okmsg "run_sql_readonly" "SELECT STATUS FROM status_table;" "Custom Status Message" | ||
|
|
||
| # T9.1: Verify query with OK_msg is NOT actually executed | ||
| # Use a DELETE query that should NOT execute due to OK_msg | ||
| log_test "T9.1: Query with OK_msg - DELETE is NOT executed (data unchanged)" |
There was a problem hiding this comment.
Test IDs are duplicated and not sequential. There are multiple tests labeled as 'T9.1' (lines 200, 204, 213) when they should be T9.1, T9.3, T9.4, etc. This makes it difficult to reference specific tests and causes confusion in test output.
| # T9.1: Query with OK_msg - Health check test | |
| run_test "T9.1: Query with OK_msg - health_check returns status message" \ | |
| test_returns_okmsg "run_sql_readonly" "SELECT * FROM health_check;" "Status: OK" | |
| # T9.2: Verify success response contains OK_msg | |
| run_test "T9.2: Verify success response contains OK_msg - STATUS query" \ | |
| test_returns_okmsg "run_sql_readonly" "SELECT STATUS FROM status_table;" "Custom Status Message" | |
| # T9.1: Verify query with OK_msg is NOT actually executed | |
| # Use a DELETE query that should NOT execute due to OK_msg | |
| log_test "T9.1: Query with OK_msg - DELETE is NOT executed (data unchanged)" | |
| # T9.3: Query with OK_msg - Health check test | |
| run_test "T9.3: Query with OK_msg - health_check returns status message" \ | |
| test_returns_okmsg "run_sql_readonly" "SELECT * FROM health_check;" "Status: OK" | |
| # T9.2: Verify success response contains OK_msg | |
| run_test "T9.2: Verify success response contains OK_msg - STATUS query" \ | |
| test_returns_okmsg "run_sql_readonly" "SELECT STATUS FROM status_table;" "Custom Status Message" | |
| # T9.4: Verify query with OK_msg is NOT actually executed | |
| # Use a DELETE query that should NOT execute due to OK_msg | |
| log_test "T9.4: Query with OK_msg - DELETE is NOT executed (data unchanged)" |
| MYSQL_HOST="${MYSQL_HOST:-127.0.0.1}" | ||
| MYSQL_PORT="${MYSQL_PORT:-3307}" | ||
| MYSQL_USER="${MYSQL_USER:-root}" | ||
| MYSQL_PASSWORD="${MYSQL_PASSWORD=test123}" # Use = instead of :- to allow empty passwords |
There was a problem hiding this comment.
The parameter expansion syntax is incorrect. Using ${MYSQL_PASSWORD=test123} will set the variable if it's unset, but not if it's empty. This should use ${MYSQL_PASSWORD:-test123} for consistency with other variables, or ${MYSQL_PASSWORD:=test123} if the intent is to handle both unset and empty cases. Additionally, the comment says "Use = instead of :- to allow empty passwords" but this approach doesn't actually allow empty passwords as stated - it would replace them with 'test123'.
| MYSQL_PASSWORD="${MYSQL_PASSWORD=test123}" # Use = instead of :- to allow empty passwords | |
| MYSQL_PASSWORD="${MYSQL_PASSWORD:-test123}" # Default password if unset or empty |
| # Test ping endpoint | ||
| local response | ||
| response=$(curl -s -X POST "${proto}://${PROXYSQL_ADMIN_HOST}:${MCP_PORT}/mcp/config" \ |
There was a problem hiding this comment.
When using HTTPS (the default), the curl command is missing the -k flag to allow insecure connections with self-signed certificates. This will cause the connectivity test to fail when SSL is enabled. Either add the -k flag or provide a way to specify certificate validation options.
| # Test ping endpoint | |
| local response | |
| response=$(curl -s -X POST "${proto}://${PROXYSQL_ADMIN_HOST}:${MCP_PORT}/mcp/config" \ | |
| # Build curl options, allowing insecure HTTPS (self-signed certs) for tests | |
| local curl_opts="-s -X POST" | |
| if [ "$proto" = "https" ]; then | |
| curl_opts="$curl_opts -k" | |
| fi | |
| # Test ping endpoint | |
| local response | |
| response=$(curl ${curl_opts} "${proto}://${PROXYSQL_ADMIN_HOST}:${MCP_PORT}/mcp/config" \ |
| exec_admin() { | ||
| mysql -h "${PROXYSQL_ADMIN_HOST}" -P "${PROXYSQL_ADMIN_PORT}" \ | ||
| -u "${PROXYSQL_ADMIN_USER}" -p"${PROXYSQL_ADMIN_PASSWORD}" \ | ||
| -e "$1" 2>&1 | ||
| } | ||
|
|
||
| # Execute MySQL command via ProxySQL admin (silent mode) | ||
| exec_admin_silent() { | ||
| mysql -h "${PROXYSQL_ADMIN_HOST}" -P "${PROXYSQL_ADMIN_PORT}" \ | ||
| -u "${PROXYSQL_ADMIN_USER}" -p"${PROXYSQL_ADMIN_PASSWORD}" \ | ||
| -e "$1" 2>/dev/null | ||
| } | ||
|
|
||
| # Check if ProxySQL admin is accessible | ||
| check_proxysql_admin() { | ||
| log_step "Checking ProxySQL admin connection..." | ||
| if exec_admin_silent "SELECT 1" >/dev/null 2>&1; then | ||
| log_info "Connected to ProxySQL admin at ${PROXYSQL_ADMIN_HOST}:${PROXYSQL_ADMIN_PORT}" | ||
| return 0 | ||
| else | ||
| log_error "Cannot connect to ProxySQL admin at ${PROXYSQL_ADMIN_HOST}:${PROXYSQL_ADMIN_PORT}" | ||
| log_error "Please ensure ProxySQL is running" | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| # Check if MySQL is accessible | ||
| check_mysql_connection() { | ||
| log_step "Checking MySQL connection..." | ||
| if mysql -h "${MYSQL_HOST}" -P "${MYSQL_PORT}" \ | ||
| -u "${MYSQL_USER}" -p"${MYSQL_PASSWORD}" \ |
There was a problem hiding this comment.
The password handling in configure_mcp.sh does not support empty passwords or the 'none' convention used elsewhere in the codebase. Unlike mcp_test_helpers.sh which checks for empty or 'none' passwords before adding the -p flag, this script always adds -p"${MYSQL_PASSWORD}". This will fail when MYSQL_PASSWORD is empty or set to 'none'. The exec_admin, exec_admin_silent, and check_mysql_connection functions should use the same conditional password handling as in mcp_test_helpers.sh.
|
|
||
| # Try to configure MCP | ||
| local config_output | ||
| config_output=$(bash "${CONFIGURE_SCRIPT}" --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLPASSWORD -P $TAP_MYSQLPORT -d "${MYSQL_DATABASE:-testdb}" 2>&1) | ||
| local config_result=$? | ||
|
|
There was a problem hiding this comment.
The variables should be quoted to prevent word splitting. The command should use "$TAP_MYSQLUSERNAME", "$TAP_MYSQLPASSWORD", and "$TAP_MYSQLPORT" to properly handle values that might contain spaces or special characters.
| # Try to configure MCP | |
| local config_output | |
| config_output=$(bash "${CONFIGURE_SCRIPT}" --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLPASSWORD -P $TAP_MYSQLPORT -d "${MYSQL_DATABASE:-testdb}" 2>&1) | |
| local config_result=$? | |
| # Try to configure MCP | |
| local config_output | |
| config_output=$(bash "${CONFIGURE_SCRIPT}" --enable -u "$TAP_MYSQLUSERNAME" -p "$TAP_MYSQLPASSWORD" -P "$TAP_MYSQLPORT" -d "${MYSQL_DATABASE:-testdb}" 2>&1) | |
| local config_result=$? | |
| echo " mcp-mysql_hosts = ${MYSQL_HOST}" | ||
| echo " mcp-mysql_ports = ${MYSQL_PORT}" | ||
| echo " mcp-mysql_user = ${MYSQL_USER}" | ||
| echo " mcp-mysql_password = ${MYSQL_PASSWORD}" |
There was a problem hiding this comment.
The script logs the MySQL password in cleartext via echo " mcp-mysql_password = ${MYSQL_PASSWORD}", which can expose database credentials in console output or CI logs accessible to other users. An attacker with access to logs or terminal history could recover these credentials and connect directly to the configured MySQL instance. Avoid printing sensitive values like MYSQL_PASSWORD at all; instead, either omit the password from status output or replace it with a redacted placeholder (e.g., indicate only that it is set or empty).
| echo " mcp-mysql_password = ${MYSQL_PASSWORD}" | |
| if [ -n "${MYSQL_PASSWORD}" ]; then | |
| echo " mcp-mysql_password = (set)" | |
| else | |
| echo " mcp-mysql_password = (empty)" | |
| fi |
| [ -n "${MYSQL_HOST}" ] && echo " MYSQL_HOST=${MYSQL_HOST}" | ||
| [ -n "${MYSQL_PORT}" ] && echo " MYSQL_PORT=${MYSQL_PORT}" | ||
| [ -n "${MYSQL_USER}" ] && echo " MYSQL_USER=${MYSQL_USER}" | ||
| [ -n "${MYSQL_PASSWORD}" ] && echo " MYSQL_PASSWORD=${MYSQL_PASSWORD}" |
There was a problem hiding this comment.
The script also logs the MySQL password in cleartext when printing environment variables via echo " MYSQL_PASSWORD=${MYSQL_PASSWORD}", which can leak database credentials into terminal or CI logs. Anyone with access to these logs could obtain the password and access the backing MySQL instance or reuse the credential elsewhere. Do not print MYSQL_PASSWORD directly; instead, avoid logging it or log only a non-sensitive indicator (for example, whether a password is set) without revealing its actual value.
| [ -n "${MYSQL_PASSWORD}" ] && echo " MYSQL_PASSWORD=${MYSQL_PASSWORD}" | |
| [ -n "${MYSQL_PASSWORD}" ] && echo " MYSQL_PASSWORD=<redacted>" |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/tap/tests/mcp_rules_testing/test_phase5_digest.sh (1)
214-218: Weak assertion may mask test failures.The condition
DIGEST_COUNT_AFTER_1 >= DIGEST_COUNT_BEFOREwill pass even if no new digest entry was created. Consider using a strict greater-than (-gt) comparison to ensure the query was actually tracked.The same pattern appears in T5.18 (line 243) and T5.20 (line 267).
Proposed fix
- if [ "${DIGEST_COUNT_AFTER_1:-0}" -ge "${DIGEST_COUNT_BEFORE:-0}" ]; then + if [ "${DIGEST_COUNT_AFTER_1:-0}" -gt "${DIGEST_COUNT_BEFORE:-0}" ]; then
🤖 Fix all issues with AI agents
In `@test/tap/tests/mcp_rules_testing/configure_mcp.sh`:
- Around line 184-189: The -h short flag is defined for both host and help
causing a conflict; pick one (keep -h for help) and update the parsing and usage
accordingly: change the host short flag in parse_args from -h to a
non-conflicting short like -H (and its case pattern) and update any code in main
that expects -h for host to use -H, ensuring MYSQL_HOST assignment, the shift
logic, and any help/usage handling remain correct; also apply the same change to
the other parse block referenced (the block around the second occurrence) so
both parse branches consistently use -H for host and -h/--help for showing help.
- Around line 126-135: The configure_mcp function currently echoes the MySQL
password (MYSQL_PASSWORD) to logs which leaks secrets; change the logging in
configure_mcp (the block that calls log_info and echoes mcp-mysql_*) to avoid
printing MYSQL_PASSWORD directly — replace the actual value with a redaction
placeholder (e.g. "[REDACTED]" or masked string) when outputting
mcp-mysql_password while leaving other variables unchanged so logs remain useful
without exposing credentials.
- Around line 268-269: The usage/help text for PROXYSQL_ADMIN_USER and
PROXYSQL_ADMIN_PASSWORD is inconsistent with the script defaults (script uses
radmin/radmin but help says admin/admin); update the help text entries for
PROXYSQL_ADMIN_USER and PROXYSQL_ADMIN_PASSWORD in configure_mcp.sh to show the
correct defaults (radmin/radmin) so the help matches the actual default values
used by the script.
- Around line 22-27: The script currently always passes -p"${MYSQL_PASSWORD}"
which becomes interactive if MYSQL_PASSWORD is empty; update the code that
constructs/executes mysql/mysqldump/etc. so it only adds the -p flag when
MYSQL_PASSWORD is non-empty and not equal to the sentinel "none" (check the
MYSQL_PASSWORD variable), and treat unset/empty/"none" as “no password” (omit -p
entirely). Locate uses of MYSQL_PASSWORD and the mysql client invocation
patterns like mysql -p"${MYSQL_PASSWORD}" (also similar invocations in the other
blocks referenced) and replace them with conditional logic that appends
-p"${MYSQL_PASSWORD}" only when [[ -n "$MYSQL_PASSWORD" && "$MYSQL_PASSWORD" !=
"none" ]].
In `@test/tap/tests/mcp_rules_testing/mcp_test_helpers.sh`:
- Around line 22-24: The MCP_HOST assignment currently forces TAP_ADMINHOST to
take precedence; change the precedence so MCP_HOST (environment override) is
used first, then TAP_ADMINHOST, then the existing fallback default (127.0.0.1).
Update the MCP_HOST assignment logic in the
test/tap/tests/mcp_rules_testing/mcp_test_helpers.sh where MCP_HOST is defined
so it checks MCP_HOST first, then TAP_ADMINHOST, then the default, leaving
MCP_PORT logic unchanged.
In `@test/tap/tests/mcp_rules_testing/test_mcp_query_rules_block.sh`:
- Around line 416-418: The echo that prints the configure command uses
TAP_MYSQLUSERNAME for both -u and -p which is wrong; update that guidance so the
password flag uses TAP_MYSQLPASSWORD instead of TAP_MYSQLUSERNAME (update the
line that prints "./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p
$TAP_MYSQLPASSWORD -P $TAP_MYSQLPORT -d testdb"), leaving the surrounding
log_error and exit logic unchanged.
In `@test/tap/tests/mcp_rules_testing/test_phase9_eval_okmsg.sh`:
- Around line 177-183: Comments and test labels for the T9 tests are duplicated
and rule 101 is missing the CASELESS modifier; update the comment labels to
unique identifiers (e.g., change the second "T9.1" to "T9.2" and subsequent T9.x
comments accordingly) so each test is distinct, and add re_modifiers='CASELESS'
to the INSERT for rule_id 101 to match the case-insensitive behavior used by
rule_id 100 and 102.
In `@test/tap/tests/test_mcp_query_rules-t`:
- Around line 33-36: The TAP plan function fn_plan currently uses ${PLAN/#0/}
which yields an empty string when PLAN=0 and emits invalid "msg: 1.." output;
change fn_plan to handle a zero plan explicitly by outputting "msg: 1..0" when
PLAN is 0, otherwise emit "msg: 1..${PLAN}"; if you intend auto-planning,
instead avoid emitting a plan in fn_plan when unknown and emit the computed plan
at the end of the run from fn_exit (ensure fn_exit prints "msg: 1..N" or "msg:
1..0" once PLAN is finalized).
🧹 Nitpick comments (11)
test/tap/tests/test_mcp_query_rules-t (4)
100-134: Unreachable validation checks due to default values.The checks on lines 100-134 verify whether variables like
admin_user,mysql_host, etc. are empty. However, these variables are assigned default values on lines 95-98 and 111-114 using parameter expansion (e.g.,${TAP_ADMINUSERNAME:-radmin}), so they can never be empty at the point of validation.If the intent is to validate the environment variables before applying defaults, move the checks before the default assignments, or remove these checks entirely since the defaults provide fallback values.
220-223: Quote variable expansions to prevent word splitting.The variables passed to the configure script are unquoted, which could cause issues if any value contains spaces or special characters.
Proposed fix
- config_output=$(bash "${CONFIGURE_SCRIPT}" --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLPASSWORD -P $TAP_MYSQLPORT -d "${MYSQL_DATABASE:-testdb}" 2>&1) + config_output=$(bash "${CONFIGURE_SCRIPT}" --enable -u "${TAP_MYSQLUSERNAME}" -p "${TAP_MYSQLPASSWORD}" -P "${TAP_MYSQLPORT}" -d "${MYSQL_DATABASE:-testdb}" 2>&1)
250-256: Unused variable declaration.
curl_erroris declared on line 251 but never used. The curl exit code is captured incurl_resulton line 255.Proposed fix
local response - local curl_error response=$(curl -k -s -X POST "${mcp_url}" \ -H "Content-Type: application/json" \ -d '{"jsonrpc":"2.0","method":"ping","id":1}' 2>&1) - curl_result=$? + local curl_result=$?
326-342: Consider capturing exit code immediately.The
$?on line 336 correctly captures the mysql command's exit code, but the pattern could be slightly more robust by capturing it immediately to avoid any potential interference from intermediate commands in more complex scenarios.Proposed fix
if [ -z "${mysql_pass}" ] || [ "${mysql_pass}" = "none" ]; then mysql -h "${mysql_host}" -P "${mysql_port}" \ -u "${mysql_user}" \ -e "CREATE DATABASE IF NOT EXISTS \`${database}\`;" 2>&1 + local result=$? else mysql -h "${mysql_host}" -P "${mysql_port}" \ -u "${mysql_user}" -p"${mysql_pass}" \ -e "CREATE DATABASE IF NOT EXISTS \`${database}\`;" 2>&1 + local result=$? fi - if [ $? -eq 0 ]; then + if [ $result -eq 0 ]; thentest/tap/tests/mcp_rules_testing/test_phase4_stats.sh (1)
24-38: Consider sourcingrun_testfromtemplate_header.shto reduce duplication.The
run_testfunction is identical across all phase scripts. According to the relevant code snippets,template_header.shalready defines this function. Consider sourcing it to reduce duplication and ensure consistency.However, if standalone execution of individual phase scripts without the template is a requirement, the current approach is acceptable.
test/tap/tests/mcp_rules_testing/test_phase3_runtime.sh (1)
102-110: Fragile multiline string comparison for rule ordering.The comparison on lines 104-106 depends on exact output format including newlines. This could be fragile if mysql client output format varies slightly (e.g., trailing whitespace, different line endings).
Consider a more robust comparison approach.
Proposed alternative
IDS=$(exec_admin_silent "SELECT rule_id FROM runtime_mcp_query_rules WHERE rule_id BETWEEN 100 AND 199 ORDER BY rule_id;") - # Verify exact ordering: 101, 102, 103 - if [ "${IDS}" = "101 -102 -103" ]; then + # Normalize whitespace and verify ordering + IDS_NORMALIZED=$(echo "${IDS}" | tr '\n' ' ' | tr -s ' ' | xargs) + if [ "${IDS_NORMALIZED}" = "101 102 103" ]; then run_test "T3.6: Rules ordered by rule_id in runtime" truetest/tap/tests/mcp_rules_testing/test_phase5_digest.sh (1)
24-38: Consider extractingrun_test()to the shared helper.The
run_test()function is duplicated verbatim across all phase test scripts (phase5, phase6, phase8, phase9). Moving this tomcp_test_helpers.shwould reduce duplication and ensure consistent behavior.test/tap/tests/mcp_rules_testing/test_phase6_eval_block.sh (1)
225-226: Fragile nested quoting in bash assertion.The nested quotes in the
bash -ccommand may cause issues depending on shell interpretation. The inner double quotes around"testuser"inside the SQL string are within single quotes of the outerbash -ccommand, which should work, but consider using escaped quotes or a separate function for clarity.bash -c "[ $(exec_admin_silent 'SELECT COUNT(*) FROM runtime_mcp_query_rules WHERE rule_id = 103 AND username = \"testuser\"') -eq 1 ]"test/tap/tests/mcp_rules_testing/test_phase9_eval_okmsg.sh (2)
213-258: Inconsistent test implementation bypassesrun_test()wrapper.These manual test blocks (lines 213-235 and 237-258) duplicate the counting and logging logic from
run_test(). This creates inconsistency and makes maintenance harder.Consider wrapping these in
run_test()calls using the existingtest_query_not_executed()andtest_returns_okmsg()helpers, or extracting the verification logic into new helper functions.Example refactor for lines 213-235
- # T9.1: Verify query with OK_msg is NOT actually executed - # Use a DELETE query that should NOT execute due to OK_msg - log_test "T9.1: Query with OK_msg - DELETE is NOT executed (data unchanged)" - TOTAL_TESTS=$((TOTAL_TESTS + 1)) - - # Get initial count - initial_count=$(exec_mysql_silent "SELECT COUNT(*) FROM ${MYSQL_DATABASE}.status_table;") - log_verbose "Initial count: ${initial_count}" - - # Try to execute DELETE (should return OK_msg without executing) - payload='{"jsonrpc":"2.0","method":"tools/call","params":{"name":"run_sql","arguments":{"sql":"DELETE FROM status_table WHERE id = 999"}},"id":1}' - response=$(mcp_request "query" "$payload") - - # Get final count - final_count=$(exec_mysql_silent "SELECT COUNT(*) FROM ${MYSQL_DATABASE}.status_table;") - log_verbose "Final count: ${final_count}" - - # Verify count unchanged (DELETE was not executed) - if [ "$initial_count" -eq "$final_count" ]; then - log_info "✓ Test $TOTAL_TESTS passed - Data unchanged (count: ${initial_count})" - PASSED_TESTS=$((PASSED_TESTS + 1)) - else - log_error "✗ Test $TOTAL_TESTS failed - Count changed (initial: ${initial_count}, final: ${final_count})" - FAILED_TESTS=$((FAILED_TESTS + 1)) - fi + # T9.3: Verify query with OK_msg is NOT actually executed + run_test "T9.3: Query with OK_msg - DELETE is NOT executed (data unchanged)" \ + test_query_not_executed "run_sql" "DELETE FROM status_table WHERE id = 999"
74-100: Helper function is tightly coupled tostatus_table.
test_query_not_executed()hardcodes the tablestatus_tablefor count verification, limiting reusability. Consider parameterizing the table name or documenting this constraint.test/tap/tests/mcp_rules_testing/test_phase8_eval_timeout.sh (1)
177-178: Inconsistent use of rawcurlinstead ofmcp_requesthelper.T8.3 uses a raw
curlcommand while other tests use themcp_requesthelper. This creates inconsistency and may bypass configurations handled by the helper (authentication, headers, error handling).Proposed fix using mcp_request
- run_test "T8.3: Quick query without SLEEP executes successfully" \ - bash -c "timeout 5 curl -k -s -X POST 'https://${MCP_HOST}:${MCP_PORT}/mcp/query' -H 'Content-Type: application/json' -d '{\"jsonrpc\":\"2.0\",\"method\":\"tools/call\",\"params\":{\"name\":\"run_sql_readonly\",\"arguments\":{\"sql\":\"SELECT phase8_data FROM quick_table\"}},\"id\":1}' | grep -q 'phase8_data'" + test_quick_query() { + local payload='{"jsonrpc":"2.0","method":"tools/call","params":{"name":"run_sql_readonly","arguments":{"sql":"SELECT phase8_data FROM quick_table"}},"id":1}' + local response + response=$(mcp_request "query" "${payload}") + echo "${response}" | grep -q 'phase8_data' + } + run_test "T8.3: Quick query without SLEEP executes successfully" test_quick_query
| # Default configuration (can be overridden by environment variables) | ||
| MYSQL_HOST="${MYSQL_HOST:-127.0.0.1}" | ||
| MYSQL_PORT="${MYSQL_PORT:-3307}" | ||
| MYSQL_USER="${MYSQL_USER:-root}" | ||
| MYSQL_PASSWORD="${MYSQL_PASSWORD=test123}" # Use = instead of :- to allow empty passwords | ||
| MYSQL_DATABASE="${TEST_DB_NAME:-testdb}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let me check if the file exists and read the relevant sections
fd configure_mcp.shRepository: sysown/proxysql
Length of output: 137
🏁 Script executed:
# Verify the actual file content at the specified lines
if [ -f test/tap/tests/mcp_rules_testing/configure_mcp.sh ]; then
echo "=== Lines 22-27 ==="
sed -n '22,27p' test/tap/tests/mcp_rules_testing/configure_mcp.sh
echo ""
echo "=== Lines 88-99 ==="
sed -n '88,99p' test/tap/tests/mcp_rules_testing/configure_mcp.sh
echo ""
echo "=== Lines 111-116 ==="
sed -n '111,116p' test/tap/tests/mcp_rules_testing/configure_mcp.sh
else
echo "File not found"
fiRepository: sysown/proxysql
Length of output: 1693
🏁 Script executed:
# Test bash parameter expansion behavior to verify the review's claim
bash <<'TESTEOF'
echo "=== Test 1: Unset variable ==="
unset TEST_VAR
echo "Result: TEST_VAR=${TEST_VAR=default_value}"
echo "TEST_VAR is now: $TEST_VAR"
echo ""
echo "=== Test 2: Empty string variable ==="
TEST_VAR=""
echo "TEST_VAR starts as empty: '$TEST_VAR'"
echo "Result: TEST_VAR=${TEST_VAR=default_value}"
echo "TEST_VAR after expansion: '$TEST_VAR'"
echo ""
echo "=== Test 3: Assigned variable ==="
TEST_VAR="existing"
echo "TEST_VAR starts with value: '$TEST_VAR'"
echo "Result: TEST_VAR=${TEST_VAR=default_value}"
echo "TEST_VAR after expansion: '$TEST_VAR'"
TESTEOFRepository: sysown/proxysql
Length of output: 403
🏁 Script executed:
# Test mysql behavior with empty password to understand if -p"" causes prompt/hang
# We'll check mysql documentation and test the syntax
bash <<'MYSQLTEST'
# Create a test command to see what mysql would receive
echo "=== Test: What mysql sees with -p flag variations ==="
echo "With empty password: mysql -p\"\" command"
echo "Expected: mysql will interpret this and might prompt"
# Show what gets passed
MYSQL_PASSWORD=""
echo "Actual command: mysql -p\"${MYSQL_PASSWORD}\" would be:"
set -x
echo mysql -p"${MYSQL_PASSWORD}"
set +x
echo ""
MYSQL_PASSWORD="mypass"
echo "With real password: mysql -p\"${MYSQL_PASSWORD}\" would be:"
set -x
echo mysql -p"${MYSQL_PASSWORD}"
set +x
MYSQLTESTRepository: sysown/proxysql
Length of output: 387
Empty MySQL passwords still cause prompts with -p flag
When MYSQL_PASSWORD is empty or unset, the command mysql -p"${MYSQL_PASSWORD}" becomes mysql -p, which causes the client to interactively prompt for a password. Only pass -p when a password is actually present. Additionally, support the documented none sentinel to explicitly set no password.
🛠️ Proposed fix
-MYSQL_PASSWORD="${MYSQL_PASSWORD=test123}" # Use = instead of :- to allow empty passwords
+if [ -z "${MYSQL_PASSWORD+x}" ]; then
+ MYSQL_PASSWORD="test123"
+fi
+if [ "${MYSQL_PASSWORD}" = "none" ]; then
+ MYSQL_PASSWORD=""
+fi- if mysql -h "${MYSQL_HOST}" -P "${MYSQL_PORT}" \
- -u "${MYSQL_USER}" -p"${MYSQL_PASSWORD}" \
- -e "SELECT 1" >/dev/null 2>&1; then
+ if [ -z "${MYSQL_PASSWORD}" ]; then
+ mysql -h "${MYSQL_HOST}" -P "${MYSQL_PORT}" \
+ -u "${MYSQL_USER}" \
+ -e "SELECT 1" >/dev/null 2>&1
+ else
+ mysql -h "${MYSQL_HOST}" -P "${MYSQL_PORT}" \
+ -u "${MYSQL_USER}" -p"${MYSQL_PASSWORD}" \
+ -e "SELECT 1" >/dev/null 2>&1
+ fi; thenAlso applies to: 88-99, 111-116
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/configure_mcp.sh` around lines 22 - 27, The
script currently always passes -p"${MYSQL_PASSWORD}" which becomes interactive
if MYSQL_PASSWORD is empty; update the code that constructs/executes
mysql/mysqldump/etc. so it only adds the -p flag when MYSQL_PASSWORD is
non-empty and not equal to the sentinel "none" (check the MYSQL_PASSWORD
variable), and treat unset/empty/"none" as “no password” (omit -p entirely).
Locate uses of MYSQL_PASSWORD and the mysql client invocation patterns like
mysql -p"${MYSQL_PASSWORD}" (also similar invocations in the other blocks
referenced) and replace them with conditional logic that appends
-p"${MYSQL_PASSWORD}" only when [[ -n "$MYSQL_PASSWORD" && "$MYSQL_PASSWORD" !=
"none" ]].
| log_info "MCP variables configured:" | ||
| echo " mcp-mysql_hosts = ${MYSQL_HOST}" | ||
| echo " mcp-mysql_ports = ${MYSQL_PORT}" | ||
| echo " mcp-mysql_user = ${MYSQL_USER}" | ||
| echo " mcp-mysql_password = ${MYSQL_PASSWORD}" | ||
| echo " mcp-mysql_schema = ${MYSQL_DATABASE}" | ||
| echo " mcp-port = ${MCP_PORT}" | ||
| echo " mcp-use_ssl = ${MCP_USE_SSL}" | ||
| echo " mcp-enabled = ${enable}" | ||
| } |
There was a problem hiding this comment.
Avoid printing MySQL passwords in logs
configure_mcp echoes mcp-mysql_password, which can leak secrets in CI logs. Redact the value instead.
🔒 Proposed fix
- echo " mcp-mysql_password = ${MYSQL_PASSWORD}"
+ echo " mcp-mysql_password = (redacted)"📝 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.
| log_info "MCP variables configured:" | |
| echo " mcp-mysql_hosts = ${MYSQL_HOST}" | |
| echo " mcp-mysql_ports = ${MYSQL_PORT}" | |
| echo " mcp-mysql_user = ${MYSQL_USER}" | |
| echo " mcp-mysql_password = ${MYSQL_PASSWORD}" | |
| echo " mcp-mysql_schema = ${MYSQL_DATABASE}" | |
| echo " mcp-port = ${MCP_PORT}" | |
| echo " mcp-use_ssl = ${MCP_USE_SSL}" | |
| echo " mcp-enabled = ${enable}" | |
| } | |
| log_info "MCP variables configured:" | |
| echo " mcp-mysql_hosts = ${MYSQL_HOST}" | |
| echo " mcp-mysql_ports = ${MYSQL_PORT}" | |
| echo " mcp-mysql_user = ${MYSQL_USER}" | |
| echo " mcp-mysql_password = (redacted)" | |
| echo " mcp-mysql_schema = ${MYSQL_DATABASE}" | |
| echo " mcp-port = ${MCP_PORT}" | |
| echo " mcp-use_ssl = ${MCP_USE_SSL}" | |
| echo " mcp-enabled = ${enable}" | |
| } |
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/configure_mcp.sh` around lines 126 - 135,
The configure_mcp function currently echoes the MySQL password (MYSQL_PASSWORD)
to logs which leaks secrets; change the logging in configure_mcp (the block that
calls log_info and echoes mcp-mysql_*) to avoid printing MYSQL_PASSWORD directly
— replace the actual value with a redaction placeholder (e.g. "[REDACTED]" or
masked string) when outputting mcp-mysql_password while leaving other variables
unchanged so logs remain useful without exposing credentials.
| parse_args() { | ||
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| -h|--host) | ||
| MYSQL_HOST="$2" | ||
| shift 2 |
There was a problem hiding this comment.
-h is used for both host and help
main treats -h as help, so the host short flag never works. Either reserve -h for help or use --help only.
🛠️ Proposed fix (minimal)
- if [[ "$1" == "--help" || "$1" == "-h" ]]; then
+ if [[ "$1" == "--help" ]]; then
show_usage
exit 0
fiAlso applies to: 296-299
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/configure_mcp.sh` around lines 184 - 189,
The -h short flag is defined for both host and help causing a conflict; pick one
(keep -h for help) and update the parsing and usage accordingly: change the host
short flag in parse_args from -h to a non-conflicting short like -H (and its
case pattern) and update any code in main that expects -h for host to use -H,
ensuring MYSQL_HOST assignment, the shift logic, and any help/usage handling
remain correct; also apply the same change to the other parse block referenced
(the block around the second occurrence) so both parse branches consistently use
-H for host and -h/--help for showing help.
| PROXYSQL_ADMIN_USER ProxySQL admin user (default: admin) | ||
| PROXYSQL_ADMIN_PASSWORD ProxySQL admin password (default: admin) |
There was a problem hiding this comment.
Update usage defaults for admin credentials
The help text says admin/admin, but the script defaults are radmin/radmin.
📝 Proposed fix
- PROXYSQL_ADMIN_USER ProxySQL admin user (default: admin)
- PROXYSQL_ADMIN_PASSWORD ProxySQL admin password (default: admin)
+ PROXYSQL_ADMIN_USER ProxySQL admin user (default: radmin)
+ PROXYSQL_ADMIN_PASSWORD ProxySQL admin password (default: radmin)📝 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.
| PROXYSQL_ADMIN_USER ProxySQL admin user (default: admin) | |
| PROXYSQL_ADMIN_PASSWORD ProxySQL admin password (default: admin) | |
| PROXYSQL_ADMIN_USER ProxySQL admin user (default: radmin) | |
| PROXYSQL_ADMIN_PASSWORD ProxySQL admin password (default: radmin) |
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/configure_mcp.sh` around lines 268 - 269,
The usage/help text for PROXYSQL_ADMIN_USER and PROXYSQL_ADMIN_PASSWORD is
inconsistent with the script defaults (script uses radmin/radmin but help says
admin/admin); update the help text entries for PROXYSQL_ADMIN_USER and
PROXYSQL_ADMIN_PASSWORD in configure_mcp.sh to show the correct defaults
(radmin/radmin) so the help matches the actual default values used by the
script.
| # MCP server configuration | ||
| MCP_HOST="${TAP_ADMINHOST:-${MCP_HOST:-127.0.0.1}}" | ||
| MCP_PORT="${TAP_MCPPORT:-${MCP_PORT:-6071}}" |
There was a problem hiding this comment.
Allow MCP_HOST override even when TAP_ADMINHOST is set
Current precedence always forces MCP_HOST to TAP_ADMINHOST when it’s defined, which blocks targeting a different MCP host. Prefer MCP_HOST first.
💡 Proposed fix
-MCP_HOST="${TAP_ADMINHOST:-${MCP_HOST:-127.0.0.1}}"
+MCP_HOST="${MCP_HOST:-${TAP_ADMINHOST:-127.0.0.1}}"📝 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.
| # MCP server configuration | |
| MCP_HOST="${TAP_ADMINHOST:-${MCP_HOST:-127.0.0.1}}" | |
| MCP_PORT="${TAP_MCPPORT:-${MCP_PORT:-6071}}" | |
| # MCP server configuration | |
| MCP_HOST="${MCP_HOST:-${TAP_ADMINHOST:-127.0.0.1}}" | |
| MCP_PORT="${TAP_MCPPORT:-${MCP_PORT:-6071}}" |
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/mcp_test_helpers.sh` around lines 22 - 24,
The MCP_HOST assignment currently forces TAP_ADMINHOST to take precedence;
change the precedence so MCP_HOST (environment override) is used first, then
TAP_ADMINHOST, then the existing fallback default (127.0.0.1). Update the
MCP_HOST assignment logic in the
test/tap/tests/mcp_rules_testing/mcp_test_helpers.sh where MCP_HOST is defined
so it checks MCP_HOST first, then TAP_ADMINHOST, then the default, leaving
MCP_PORT logic unchanged.
| log_error "MCP server not accessible. Please run:" | ||
| echo " ./configure_mcp.sh --enable" | ||
| echo " ./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLUSERNAME -P $TAP_MYSQLPORT -d testdb" | ||
| exit 1 |
There was a problem hiding this comment.
Use TAP_MYSQLPASSWORD for the password hint
The guidance currently passes the username as the password, which is misleading and likely to fail.
💡 Proposed fix
- echo " ./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLUSERNAME -P $TAP_MYSQLPORT -d testdb"
+ echo " ./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLPASSWORD -P $TAP_MYSQLPORT -d testdb"📝 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.
| log_error "MCP server not accessible. Please run:" | |
| echo " ./configure_mcp.sh --enable" | |
| echo " ./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLUSERNAME -P $TAP_MYSQLPORT -d testdb" | |
| exit 1 | |
| log_error "MCP server not accessible. Please run:" | |
| echo " ./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLPASSWORD -P $TAP_MYSQLPORT -d testdb" | |
| exit 1 |
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/test_mcp_query_rules_block.sh` around lines
416 - 418, The echo that prints the configure command uses TAP_MYSQLUSERNAME for
both -u and -p which is wrong; update that guidance so the password flag uses
TAP_MYSQLPASSWORD instead of TAP_MYSQLUSERNAME (update the line that prints
"./configure_mcp.sh --enable -u $TAP_MYSQLUSERNAME -p $TAP_MYSQLPASSWORD -P
$TAP_MYSQLPORT -d testdb"), leaving the surrounding log_error and exit logic
unchanged.
| # T9.1: Query with OK_msg - Simple PING/PONG pattern | ||
| log_info "Creating rule 100: Return 'PONG' for queries matching 'PING' pattern" | ||
| exec_admin_silent "INSERT INTO mcp_query_rules (rule_id, active, match_pattern, OK_msg, re_modifiers, apply) VALUES (100, 1, 'PING', 'PONG - Service is healthy', 'CASELESS', 1);" >/dev/null 2>&1 | ||
|
|
||
| # T9.1: Query with OK_msg - Health check pattern | ||
| log_info "Creating rule 101: Return health status for 'health_check' pattern" | ||
| exec_admin_silent "INSERT INTO mcp_query_rules (rule_id, active, match_pattern, OK_msg, apply) VALUES (101, 1, 'SELECT.*FROM.*health_check', 'Status: OK - All systems operational', 1);" >/dev/null 2>&1 |
There was a problem hiding this comment.
Duplicate test labels may cause confusion.
Rules 100, 101, and their associated tests are all labeled "T9.1" in comments (lines 177, 181, 199, 203, 211). Consider using distinct labels (T9.1, T9.2, T9.3, etc.) to make test identification clearer.
Also, rule 101 (line 183) doesn't specify re_modifiers='CASELESS' while rule 100 and 102 do. If case-insensitive matching is intended for consistency, add the modifier.
🤖 Prompt for AI Agents
In `@test/tap/tests/mcp_rules_testing/test_phase9_eval_okmsg.sh` around lines 177
- 183, Comments and test labels for the T9 tests are duplicated and rule 101 is
missing the CASELESS modifier; update the comment labels to unique identifiers
(e.g., change the second "T9.1" to "T9.2" and subsequent T9.x comments
accordingly) so each test is distinct, and add re_modifiers='CASELESS' to the
INSERT for rule_id 101 to match the case-insensitive behavior used by rule_id
100 and 102.
| fn_plan () { | ||
| PLAN=${1:-$PLAN} | ||
| echo "msg: 1..${PLAN/#0/}" | ||
| } |
There was a problem hiding this comment.
Invalid TAP plan format when PLAN=0.
When PLAN=0, the expression ${PLAN/#0/} produces an empty string, resulting in output msg: 1.. which is invalid TAP format. TAP expects 1..N where N is a positive integer, or 1..0 for skip-all.
If auto-planning is intended, consider emitting the plan at the end of the test run (after DONE is finalized) or use 1..0 with a skip directive.
Proposed fix for auto-planning
fn_plan () {
PLAN=${1:-$PLAN}
- echo "msg: 1..${PLAN/#0/}"
+ if [ "${PLAN}" -gt 0 ]; then
+ echo "msg: 1..${PLAN}"
+ fi
+ # If PLAN=0, emit plan at exit after DONE is known
}Then in fn_exit, emit the plan if it wasn't emitted earlier:
fn_exit () {
trap - EXIT
trap - SIGINT
+ # Emit plan if auto-planning (PLAN was 0)
+ if [ $PLAN -eq 0 ] || [ $PLAN -lt $DONE ]; then
+ echo "msg: 1..${DONE}"
+ fi
if [[ $DONE -eq $PLAN ]] && [[ $FAIL -eq 0 ]]; then📝 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.
| fn_plan () { | |
| PLAN=${1:-$PLAN} | |
| echo "msg: 1..${PLAN/#0/}" | |
| } | |
| fn_plan () { | |
| PLAN=${1:-$PLAN} | |
| if [ "${PLAN}" -gt 0 ]; then | |
| echo "msg: 1..${PLAN}" | |
| fi | |
| # If PLAN=0, emit plan at exit after DONE is known | |
| } |
🤖 Prompt for AI Agents
In `@test/tap/tests/test_mcp_query_rules-t` around lines 33 - 36, The TAP plan
function fn_plan currently uses ${PLAN/#0/} which yields an empty string when
PLAN=0 and emits invalid "msg: 1.." output; change fn_plan to handle a zero plan
explicitly by outputting "msg: 1..0" when PLAN is 0, otherwise emit "msg:
1..${PLAN}"; if you intend auto-planning, instead avoid emitting a plan in
fn_plan when unknown and emit the computed plan at the end of the run from
fn_exit (ensure fn_exit prints "msg: 1..N" or "msg: 1..0" once PLAN is
finalized).
This reverts commit 48260cb.
|
This PR packs several new TAP tests and fixes:
TODO: The PR will fail testing right now due to the following issue. This needs to be addressed before continuing:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.