mirror of
https://github.com/DeusData/codebase-memory-mcp.git
synced 2026-11-04 10:48:20 +00:00
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
This commit is contained in:
1 parent
d87cffe168
commit
09e3f71bf6
3 files changed
+131
-8
No files matched your search
@@ -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
|
||||
|
||||
|
||||
+70
-8
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user