From 09e3f71bf6f9b018c0050ade8b0795f7294cd7eb Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Mon, 1 Jun 2026 22:53:17 +0200 Subject: [PATCH] fix(search_code): accept '&' in paths, warn on literal-'|' trap, report timing #272: '&' is neutralised by the single-/double-quoting of the grep/Select-String/ git commands, so it no longer fails validation. A relaxed validate_search_path_arg (cbm_validate_shell_arg minus '&') is used for search_code + detect_changes root_path/file_pattern; all other shell metacharacters stay rejected and base_branch keeps the strict check. #282: search_code defaulted regex=false, so 'foo|bar' matched the literal pipe and silently returned 0 results. The result now carries a warnings[] array that flags a literal '|' under regex=false (with the regex=true hint), so the trap is visible instead of looking like a legitimate no-match. (The related invalid-regex silent-empty case was already fixed in #283.) Also: every search_code result now reports elapsed_ms, and a perf warning fires when a search exceeds 5s (also logged as search.slow) so slow calls are visible. Tests for both issues + the multi-word/invalid-regex no-regression; generous search_code smoke coverage (basic+elapsed_ms, literal-| warning, '&' acceptance). Closes #272 Closes #282 --- scripts/smoke-test.sh | 15 +++++++++ src/mcp/mcp.c | 78 ++++++++++++++++++++++++++++++++++++++----- tests/test_mcp.c | 46 +++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 8 deletions(-) diff --git a/scripts/smoke-test.sh b/scripts/smoke-test.sh index d06ada816..568608dd5 100755 --- a/scripts/smoke-test.sh +++ b/scripts/smoke-test.sh @@ -318,6 +318,21 @@ if [ "$NCLUST" -lt 1 ]; then fi echo "OK: get_architecture returned $NCLUST community cluster(s)" +# 3g: search_code — basic search reports elapsed_ms + matches +SC=$(cli search_code "{\"project\":\"$PROJECT\",\"pattern\":\"cbm_\",\"mode\":\"compact\",\"limit\":5}") +echo "$SC" | python3 -c "import json,sys; d=json.loads(sys.stdin.read()); assert 'elapsed_ms' in d; print('OK: search_code elapsed_ms='+str(d['elapsed_ms'])+' total_grep_matches='+str(d.get('total_grep_matches')))" 2>/dev/null || { echo "FAIL: search_code basic / no elapsed_ms"; echo "$SC" | head -c 400; exit 1; } + +# 3g: search_code — literal '|' under regex=false must surface a warning (#282) +SCW=$(cli search_code "{\"project\":\"$PROJECT\",\"pattern\":\"cbm_init|cbm_nope\",\"regex\":false,\"limit\":5}") +echo "$SCW" | python3 -c "import json,sys; d=json.loads(sys.stdin.read()); w=' '.join(d.get('warnings',[])); assert 'regex=true' in w; print('OK: search_code literal-| warning surfaced')" 2>/dev/null || { echo "FAIL: search_code literal-| warning missing"; echo "$SCW" | head -c 400; exit 1; } + +# 3g: search_code — '&' in file_pattern accepted, not rejected as invalid (#272) +SCA=$(cli search_code "{\"project\":\"$PROJECT\",\"pattern\":\"cbm_\",\"file_pattern\":\"*R&D*.c\",\"limit\":5}") +case "$SCA" in + *"invalid characters"*) echo "FAIL: search_code rejected '&' in file_pattern"; echo "$SCA" | head -c 300; exit 1 ;; + *) echo "OK: search_code accepts '&' in file_pattern" ;; +esac + # 3e: delete_project cleanup cli delete_project "{\"project\":\"$PROJECT\"}" > /dev/null diff --git a/src/mcp/mcp.c b/src/mcp/mcp.c index fd69dda07..ae953358a 100644 --- a/src/mcp/mcp.c +++ b/src/mcp/mcp.c @@ -3167,8 +3167,9 @@ static yyjson_mut_val *build_dir_distribution(yyjson_mut_doc *doc, search_result /* Phase 4: assemble JSON output from search results */ static char *assemble_search_output(search_result_t *sr, int sr_count, grep_match_t *raw, int raw_count, int gm_count, int limit, int mode, - int context_lines, const char *root_path) { - enum { MODE_COMPACT = 0, MODE_FULL = 1, MODE_FILES = 2 }; + int context_lines, const char *root_path, + bool warn_literal_pipe, uint64_t elapsed_ms) { + enum { MODE_COMPACT = 0, MODE_FULL = 1, MODE_FILES = 2, SEARCH_SLOW_MS = 5000 }; yyjson_mut_doc *doc = yyjson_mut_doc_new(NULL); yyjson_mut_val *root_obj = yyjson_mut_obj(doc); @@ -3223,12 +3224,36 @@ static char *assemble_search_output(search_result_t *sr, int sr_count, grep_matc yyjson_mut_obj_add_int(doc, root_obj, "total_grep_matches", gm_count); yyjson_mut_obj_add_int(doc, root_obj, "total_results", sr_count); yyjson_mut_obj_add_int(doc, root_obj, "raw_match_count", raw_count); + yyjson_mut_obj_add_int(doc, root_obj, "elapsed_ms", (int)elapsed_ms); if (sr_count > 0 && gm_count > 0) { char ratio[CBM_SZ_32]; snprintf(ratio, sizeof(ratio), "%.1fx", (double)gm_count / (double)(sr_count + raw_count)); yyjson_mut_obj_add_strcpy(doc, root_obj, "dedup_ratio", ratio); } + /* Warnings: surface common foot-guns instead of leaving them silent. */ + yyjson_mut_val *warnings = yyjson_mut_arr(doc); + if (warn_literal_pipe) { + yyjson_mut_arr_add_strcpy( + doc, warnings, + "pattern contains '|' but regex=false, so it is matched literally (not as " + "alternation). Pass regex=true for 'foo|bar' to mean 'foo OR bar'."); + } + if (elapsed_ms >= SEARCH_SLOW_MS) { + char slow[CBM_SZ_128]; + snprintf(slow, sizeof(slow), + "search took %dms (>%ds); narrow file_pattern/path_filter or use a more " + "specific pattern", + (int)elapsed_ms, SEARCH_SLOW_MS / 1000); + yyjson_mut_arr_add_strcpy(doc, warnings, slow); + char ems[CBM_SZ_32]; + snprintf(ems, sizeof(ems), "%d", (int)elapsed_ms); + cbm_log_warn("search.slow", "elapsed_ms", ems); /* visibility in logs */ + } + if (yyjson_mut_arr_size(warnings) > 0) { + yyjson_mut_obj_add_val(doc, root_obj, "warnings", warnings); + } + char *json = yy_doc_to_str(doc); if (json) { sanitize_ascii(json); @@ -3460,11 +3485,43 @@ static int parse_search_mode(const char *mode_str) { } /* Validate shell-safe arguments for search. */ -static bool validate_search_args(const char *root_path, const char *file_pattern) { - if (!cbm_validate_shell_arg(root_path)) { +/* Search/grep paths and globs are ALWAYS single-quoted (POSIX sh) or + * double-/single-quoted (Windows cmd/PowerShell) on the command line, which + * neutralises '&' — a very common character in real paths (R&D, "Foo & Bar", + * OneDrive). Accept '&' here while still rejecting every metacharacter that + * could break out of the quoting (#272). */ +static bool validate_search_path_arg(const char *s) { + if (!s) { return false; } - if (file_pattern && !cbm_validate_shell_arg(file_pattern)) { + for (const char *p = s; *p; p++) { + switch (*p) { + case '\'': + case '"': + case ';': + case '|': + case '$': + case '`': + case '<': + case '>': + case '\n': + case '\r': +#ifndef _WIN32 + case '\\': +#endif + return false; + default: + break; + } + } + return true; +} + +static bool validate_search_args(const char *root_path, const char *file_pattern) { + if (!validate_search_path_arg(root_path)) { + return false; + } + if (file_pattern && !validate_search_path_arg(file_pattern)) { return false; } return true; @@ -3499,6 +3556,10 @@ static char *handle_search_code(cbm_mcp_server_t *srv, const char *args) { int limit = cbm_mcp_get_int_arg(args, "limit", MCP_DEFAULT_LIMIT); int context_lines = cbm_mcp_get_int_arg(args, "context", 0); bool use_regex = cbm_mcp_get_bool_arg(args, "regex"); + uint64_t search_t0 = cbm_now_ms(); + /* In literal (non-regex) mode a '|' is matched as a byte, not alternation — + * a common silent 0-match trap; flagged in the result warnings (#282). */ + bool pat_has_pipe = pattern && strchr(pattern, '|') != NULL; int mode = parse_search_mode(mode_str); free(mode_str); @@ -3708,8 +3769,9 @@ static char *handle_search_code(cbm_mcp_server_t *srv, const char *args) { /* ── Phase 4: Context assembly (extracted helper) ─────────── */ - char *result = assemble_search_output(sr, sr_count, raw, raw_count, gm_count, limit, mode, - context_lines, root_path); + char *result = + assemble_search_output(sr, sr_count, raw, raw_count, gm_count, limit, mode, context_lines, + root_path, pat_has_pipe && !use_regex, cbm_now_ms() - search_t0); free(gm); free(sr); free(raw); @@ -3773,7 +3835,7 @@ static char *handle_detect_changes(cbm_mcp_server_t *srv, const char *args) { return cbm_mcp_text_result("project not found", true); } - if (!cbm_validate_shell_arg(root_path)) { + if (!validate_search_path_arg(root_path)) { free(root_path); free(project); free(base_branch); diff --git a/tests/test_mcp.c b/tests/test_mcp.c index 43e2b7b22..61c71b897 100644 --- a/tests/test_mcp.c +++ b/tests/test_mcp.c @@ -772,6 +772,50 @@ TEST(search_code_invalid_regex_errors_issue283) { PASS(); } +/* issue #282: a literal '|' under regex=false is a silent 0-match trap. It must + * now be surfaced as a warning (and the result carries elapsed_ms). */ +TEST(search_code_literal_pipe_warns_issue282) { + char tmp[512]; + cbm_mcp_server_t *srv = setup_snippet_server(tmp, sizeof(tmp)); + ASSERT_NOT_NULL(srv); + + char *resp = + cbm_mcp_server_handle(srv, "{\"jsonrpc\":\"2.0\",\"id\":93,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"search_code\"," + "\"arguments\":{\"pattern\":\"HandleRequest|Nope\"," + "\"regex\":false,\"project\":\"test-project\"}}}"); + ASSERT_NOT_NULL(resp); + ASSERT_NOT_NULL(strstr(resp, "warnings")); /* surfaced, not silent */ + ASSERT_NOT_NULL(strstr(resp, "regex=true")); /* the hint names the fix */ + ASSERT_NOT_NULL(strstr(resp, "elapsed_ms")); /* timing is reported */ + free(resp); + + cleanup_snippet_dir(tmp); + cbm_mcp_server_free(srv); + PASS(); +} + +/* issue #272: '&' in a path / file_pattern is neutralised by the command's + * quoting and must no longer be rejected as "invalid characters". */ +TEST(search_code_ampersand_accepted_issue272) { + char tmp[512]; + cbm_mcp_server_t *srv = setup_snippet_server(tmp, sizeof(tmp)); + ASSERT_NOT_NULL(srv); + + char *resp = + cbm_mcp_server_handle(srv, "{\"jsonrpc\":\"2.0\",\"id\":94,\"method\":\"tools/call\"," + "\"params\":{\"name\":\"search_code\"," + "\"arguments\":{\"pattern\":\"HandleRequest\"," + "\"file_pattern\":\"*R&D*.go\",\"project\":\"test-project\"}}}"); + ASSERT_NOT_NULL(resp); + ASSERT_TRUE(strstr(resp, "invalid characters") == NULL); + free(resp); + + cleanup_snippet_dir(tmp); + cbm_mcp_server_free(srv); + PASS(); +} + TEST(tool_detect_changes_no_project) { cbm_mcp_server_t *srv = cbm_mcp_server_new(NULL); @@ -2013,6 +2057,8 @@ SUITE(mcp) { RUN_TEST(tool_search_code_no_project); RUN_TEST(search_code_multi_word); RUN_TEST(search_code_invalid_regex_errors_issue283); + RUN_TEST(search_code_literal_pipe_warns_issue282); + RUN_TEST(search_code_ampersand_accepted_issue272); RUN_TEST(tool_detect_changes_no_project); RUN_TEST(tool_manage_adr_no_project); RUN_TEST(tool_manage_adr_get_with_existing_adr);