From c4ae9a88f8884ee5a155c8349ace9ea31a58007f Mon Sep 17 00:00:00 2001 From: Xuan-Son Nguyen Date: Fri, 10 Jul 2026 11:52:59 +0200 Subject: [PATCH] server: improve tools, remove apply_diff (#25498) * server: improve tools, remove apply_diff * improve edit tool * add tools_io abstraction * add tools_io_basic * fix build * move utils to class member * add const --- common/arg.cpp | 2 +- tools/server/server-tools.cpp | 1001 +++++++++++------ tools/server/server-tools.h | 6 +- tools/server/tests/unit/test_tools_builtin.py | 125 ++ tools/server/tests/utils.py | 3 + 5 files changed, 781 insertions(+), 356 deletions(-) create mode 100755 tools/server/tests/unit/test_tools_builtin.py diff --git a/common/arg.cpp b/common/arg.cpp index 8a78291658..6fb6a5700f 100644 --- a/common/arg.cpp +++ b/common/arg.cpp @@ -3036,7 +3036,7 @@ common_params_context common_params_parser_init(common_params & params, llama_ex {"--tools"}, "TOOL1,TOOL2,...", "experimental: whether to enable built-in tools for AI agents - do not enable in untrusted environments (default: no tools)\n" "specify \"all\" to enable all tools\n" - "available tools: read_file, file_glob_search, grep_search, exec_shell_command, write_file, edit_file, apply_diff, get_datetime", + "available tools: read_file, file_glob_search, grep_search, exec_shell_command, write_file, edit_file, get_datetime", [](common_params & params, const std::string & value) { params.server_tools = parse_csv_row(value); } diff --git a/tools/server/server-tools.cpp b/tools/server/server-tools.cpp index 790ed85a06..3f162a13e0 100644 --- a/tools/server/server-tools.cpp +++ b/tools/server/server-tools.cpp @@ -19,91 +19,7 @@ namespace fs = std::filesystem; // internal helpers // -static std::vector to_cstr_vec(const std::vector & v) { - std::vector r; - r.reserve(v.size() + 1); - for (const auto & s : v) { - r.push_back(const_cast(s.c_str())); - } - r.push_back(nullptr); - return r; -} - -struct run_proc_result { - std::string output; - int exit_code = -1; - bool timed_out = false; -}; - -static run_proc_result run_process( - const std::vector & args, - size_t max_output, - int timeout_secs) { - run_proc_result res; - - subprocess_s proc; - auto argv = to_cstr_vec(args); - - int options = subprocess_option_no_window - | subprocess_option_combined_stdout_stderr - | subprocess_option_inherit_environment - | subprocess_option_search_user_path; - - if (subprocess_create(argv.data(), options, &proc) != 0) { - res.output = "failed to spawn process"; - return res; - } - - std::atomic done{false}; - std::atomic timed_out{false}; - - std::thread timeout_thread([&]() { - auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(timeout_secs); - while (!done.load()) { - if (std::chrono::steady_clock::now() >= deadline) { - timed_out.store(true); - subprocess_terminate(&proc); - return; - } - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - } - }); - - FILE * f = subprocess_stdout(&proc); - std::string output; - bool truncated = false; - if (f) { - char buf[4096]; - while (fgets(buf, sizeof(buf), f) != nullptr) { - if (!truncated) { - size_t len = strlen(buf); - if (output.size() + len <= max_output) { - output.append(buf, len); - } else { - output.append(buf, max_output - output.size()); - truncated = true; - } - } - } - } - - done.store(true); - if (timeout_thread.joinable()) { - timeout_thread.join(); - } - - subprocess_join(&proc, &res.exit_code); - subprocess_destroy(&proc); - - res.output = output; - res.timed_out = timed_out.load(); - if (truncated) { - res.output += "\n[output truncated]"; - } - return res; -} - -json server_tool::to_json() { +json server_tool::to_json() const { return { {"display_name", display_name}, {"tool", name}, @@ -115,6 +31,228 @@ json server_tool::to_json() { }; } +static constexpr size_t SERVER_TOOL_GIT_LS_FILES_MAX_OUTPUT = 8 * 1024 * 1024; // 8 MB +static constexpr int SERVER_TOOL_GIT_LS_FILES_TIMEOUT = 15; // seconds + +class tools_io { +public: + struct exec_result { + std::string output; + int exit_code = -1; + bool timed_out = false; + }; + + virtual ~tools_io() = default; + + virtual bool is_directory(const std::string & path) const = 0; + virtual bool is_regular_file(const std::string & path) const = 0; + virtual bool file_size(const std::string & path, uintmax_t & out_size) const = 0; + virtual bool read_file(const std::string & path, std::string & out) const = 0; + virtual bool write_file(const std::string & path, const std::string & content) const = 0; + // paths relative to `base`, '/'-separated; sets `err` if `base` isn't a directory + virtual std::vector list_files(const std::string & base, std::string & err) const = 0; + virtual exec_result run(const std::vector & args, size_t max_output, int timeout_secs) const = 0; +}; + +class tools_io_basic : public tools_io { +public: + bool is_directory(const std::string & path) const override { + std::error_code ec; + return fs::is_directory(path, ec) && !ec; + } + + bool is_regular_file(const std::string & path) const override { + std::error_code ec; + return fs::is_regular_file(path, ec) && !ec; + } + + bool file_size(const std::string & path, uintmax_t & out_size) const override { + std::error_code ec; + out_size = fs::file_size(path, ec); + return !ec; + } + + bool read_file(const std::string & path, std::string & out) const override { + std::ifstream f(path, std::ios::binary); + if (!f) return false; + std::ostringstream ss; + ss << f.rdbuf(); + out = ss.str(); + return true; + } + + bool write_file(const std::string & path, const std::string & content) const override { + std::error_code ec; + fs::path fpath(path); + if (fpath.has_parent_path()) { + fs::create_directories(fpath.parent_path(), ec); + if (ec) return false; + } + std::ofstream f(path, std::ios::binary); + if (!f) return false; + f << content; + return (bool) f; + } + + std::vector list_files(const std::string & base, std::string & err) const override { + err.clear(); + if (!is_directory(base)) { + err = "path does not exist or is not a directory: " + base; + return {}; + } + + auto res = run( + {"git", "-C", base, "ls-files", "--cached", "--others", "--exclude-standard"}, + SERVER_TOOL_GIT_LS_FILES_MAX_OUTPUT, SERVER_TOOL_GIT_LS_FILES_TIMEOUT); + + if (res.exit_code == 0 && !res.timed_out) { + std::vector result; + std::istringstream iss(res.output); + std::string line; + while (std::getline(iss, line)) { + if (!line.empty() && line.back() == '\r') line.pop_back(); + if (line.empty()) continue; + std::replace(line.begin(), line.end(), '\\', '/'); + if (is_regular_file((fs::path(base) / line).string())) { + result.push_back(line); + } + } + return result; + } + + return list_files_fallback(base); + } + + exec_result run(const std::vector & args, size_t max_output, int timeout_secs) const override { + exec_result res; + + subprocess_s proc; + auto argv = to_cstr_vec(args); + + int options = subprocess_option_no_window + | subprocess_option_combined_stdout_stderr + | subprocess_option_inherit_environment + | subprocess_option_search_user_path; + + if (subprocess_create(argv.data(), options, &proc) != 0) { + res.output = "failed to spawn process"; + return res; + } + + std::atomic done{false}; + std::atomic timed_out{false}; + + std::thread timeout_thread([&]() { + auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(timeout_secs); + while (!done.load()) { + if (std::chrono::steady_clock::now() >= deadline) { + timed_out.store(true); + subprocess_terminate(&proc); + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + }); + + FILE * f = subprocess_stdout(&proc); + std::string output; + bool truncated = false; + if (f) { + char buf[4096]; + while (fgets(buf, sizeof(buf), f) != nullptr) { + if (!truncated) { + size_t len = strlen(buf); + if (output.size() + len <= max_output) { + output.append(buf, len); + } else { + output.append(buf, max_output - output.size()); + truncated = true; + } + } + } + } + + done.store(true); + if (timeout_thread.joinable()) { + timeout_thread.join(); + } + + subprocess_join(&proc, &res.exit_code); + subprocess_destroy(&proc); + + res.output = output; + res.timed_out = timed_out.load(); + if (truncated) { + res.output += "\n[output truncated]"; + } + return res; + } + +private: + static std::vector to_cstr_vec(const std::vector & v) { + std::vector r; + r.reserve(v.size() + 1); + for (const auto & s : v) { + r.push_back(const_cast(s.c_str())); + } + r.push_back(nullptr); + return r; + } + + static const std::unordered_set & junk_dir_names() { + static const std::unordered_set names = { + ".git", ".svn", ".hg", "node_modules", "__pycache__", + ".venv", "venv", "dist", "build", "target", ".cache", ".idea", ".vscode", + }; + return names; + } + + std::vector list_files_fallback(const std::string & base) const { + std::vector result; + std::error_code ec; + + std::vector> stack; + stack.emplace_back(fs::path(base), fs::path()); + + while (!stack.empty()) { + auto [dir, rel_dir] = stack.back(); + stack.pop_back(); + + for (const auto & entry : fs::directory_iterator(dir, fs::directory_options::skip_permission_denied, ec)) { + if (ec) break; + std::string fname = entry.path().filename().string(); + std::error_code tec; + if (entry.is_directory(tec)) { + if (junk_dir_names().count(fname) > 0) continue; + stack.emplace_back(entry.path(), rel_dir / fname); + } else if (entry.is_regular_file(tec)) { + std::string rel = (rel_dir / fname).string(); + std::replace(rel.begin(), rel.end(), '\\', '/'); + result.push_back(rel); + } + } + } + + return result; + } +}; + +static std::unique_ptr make_tools_io(const json & params) { + GGML_UNUSED(params); // TODO in follow-up PR + return std::make_unique(); +} + +// no '/' in pattern -> match basename at any depth; else match full relative path +static bool path_glob_match(const std::string & pattern, const std::string & rel_path) { + if (pattern.find('/') == std::string::npos) { + return glob_match(pattern, fs::path(rel_path).filename().string()); + } + if (pattern == "**" || pattern.rfind("**/", 0) == 0 || pattern.rfind('/', 0) == 0) { + return glob_match(pattern, rel_path); + } + return glob_match("**/" + pattern, rel_path); +} + // // read_file: read a file with optional line range and line-number prefix // @@ -128,7 +266,7 @@ struct server_tool_read_file : server_tool { permission_write = false; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { @@ -149,16 +287,17 @@ struct server_tool_read_file : server_tool { }; } - json invoke(json params) override { + json invoke(json params) const override { std::string path = params.at("path").get(); int start_line = json_value(params, "start_line", 1); int end_line = json_value(params, "end_line", -1); // -1 = no limit bool append_loc = json_value(params, "append_loc", false); - std::error_code ec; - uintmax_t file_size = fs::file_size(path, ec); - if (ec) { - return {{"error", "cannot stat file: " + ec.message()}}; + auto io = make_tools_io(params); + + uintmax_t file_size = 0; + if (!io->file_size(path, file_size)) { + return {{"error", "cannot stat file: " + path}}; } if (file_size > SERVER_TOOL_READ_FILE_MAX_SIZE && end_line == -1) { return {{"error", string_format( @@ -166,11 +305,12 @@ struct server_tool_read_file : server_tool { (size_t)file_size, SERVER_TOOL_READ_FILE_MAX_SIZE)}}; } - std::ifstream f(path); - if (!f) { + std::string content; + if (!io->read_file(path, content)) { return {{"error", "failed to open file: " + path}}; } + std::istringstream f(content); std::string result; std::string line; int lineno = 0; @@ -211,17 +351,23 @@ struct server_tool_file_glob_search : server_tool { permission_write = false; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { {"name", name}, - {"description", "Recursively search for files matching a glob pattern under a directory."}, + {"description", + "Recursively search for files matching a glob pattern under a directory. " + "Automatically skips files ignored by .gitignore (when the directory is inside a git repo) " + "and common junk directories (.git, node_modules, build, dist, etc.) otherwise. " + "A pattern with no '/' (e.g. \"*.cpp\") matches the file's basename at any depth. " + "A pattern containing '/' matches the full relative path; unless already anchored with " + "\"**/\" or a leading '/', it is automatically prefixed with \"**/\"."}, {"parameters", { {"type", "object"}, {"properties", { {"path", {{"type", "string"}, {"description", "Base directory to search in"}}}, - {"include", {{"type", "string"}, {"description", "Glob pattern for files to include (e.g. \"**/*.cpp\"). Default: **"}}}, + {"include", {{"type", "string"}, {"description", "Glob pattern for files to include (e.g. \"*.cpp\" or \"src/**/*.cpp\"). Default: **"}}}, {"exclude", {{"type", "string"}, {"description", "Glob pattern for files to exclude"}}}, }}, {"required", json::array({"path"})}, @@ -230,33 +376,39 @@ struct server_tool_file_glob_search : server_tool { }; } - json invoke(json params) override { + json invoke(json params) const override { std::string base = params.at("path").get(); std::string include = json_value(params, "include", std::string("**")); std::string exclude = json_value(params, "exclude", std::string("")); - std::ostringstream output_text; - size_t count = 0; - - std::error_code ec; - for (const auto & entry : fs::recursive_directory_iterator(base, - fs::directory_options::skip_permission_denied, ec)) { - if (!entry.is_regular_file()) continue; - - std::string rel = fs::relative(entry.path(), base, ec).string(); - if (ec) continue; - std::replace(rel.begin(), rel.end(), '\\', '/'); - - if (!glob_match(include, rel)) continue; - if (!exclude.empty() && glob_match(exclude, rel)) continue; - - output_text << entry.path().string() << "\n"; - if (++count >= SERVER_TOOL_FILE_SEARCH_MAX_RESULTS) { - break; - } + auto io = make_tools_io(params); + std::string err; + auto files = io->list_files(base, err); + if (!err.empty()) { + return {{"error", err}}; } - output_text << "\n---\nTotal matches: " << count << "\n"; + std::vector matches; + for (const auto & rel : files) { + if (!path_glob_match(include, rel)) continue; + if (!exclude.empty() && path_glob_match(exclude, rel)) continue; + matches.push_back(rel); + } + + size_t total = matches.size(); + size_t shown = std::min(total, SERVER_TOOL_FILE_SEARCH_MAX_RESULTS); + + std::ostringstream output_text; + for (size_t i = 0; i < shown; i++) { + output_text << matches[i] << "\n"; + } + + output_text << "\n---\nTotal matches: " << total << "\n"; + if (total > shown) { + output_text << string_format( + "[%zu results limit reached (%zu total matches). Refine the glob pattern to narrow the search.]\n", + shown, total); + } return {{"plain_text_response", output_text.str()}}; } @@ -275,20 +427,29 @@ struct server_tool_grep_search : server_tool { permission_write = false; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { {"name", name}, - {"description", "Search for a regex pattern in files under a path. Returns matching lines."}, + {"description", + "Search for a pattern in files under a path. Returns matching lines with file paths " + "(and, unless searching a single file, paths relative to the given directory). " + "Automatically skips files ignored by .gitignore (when the directory is inside a git repo) " + "and common junk directories (.git, node_modules, build, dist, etc.) otherwise. " + "include/exclude: a pattern with no '/' matches the basename at any depth; a pattern " + "containing '/' matches the full relative path (auto-anchored with \"**/\" unless already anchored)."}, {"parameters", { {"type", "object"}, {"properties", { {"path", {{"type", "string"}, {"description", "File or directory to search in"}}}, - {"pattern", {{"type", "string"}, {"description", "Regular expression pattern to search for"}}}, + {"pattern", {{"type", "string"}, {"description", "Pattern to search for (regular expression unless literal is true)"}}}, {"include", {{"type", "string"}, {"description", "Glob pattern to filter files (default: **)"}}}, {"exclude", {{"type", "string"}, {"description", "Glob pattern to exclude files"}}}, {"return_line_numbers", {{"type", "boolean"}, {"description", "If true, include line numbers in results"}}}, + {"literal", {{"type", "boolean"}, {"description", "Treat pattern as a literal string instead of a regular expression (default: false)"}}}, + {"ignore_case", {{"type", "boolean"}, {"description", "Case-insensitive search (default: false)"}}}, + {"context_lines", {{"type", "integer"}, {"description", "Number of lines of context to show before and after each match (default: 0)"}}}, }}, {"required", json::array({"path", "pattern"})}, }}, @@ -296,64 +457,109 @@ struct server_tool_grep_search : server_tool { }; } - json invoke(json params) override { - std::string path = params.at("path").get(); - std::string pat_str = params.at("pattern").get(); - std::string include = json_value(params, "include", std::string("**")); - std::string exclude = json_value(params, "exclude", std::string("")); - bool show_lineno = json_value(params, "return_line_numbers", false); + json invoke(json params) const override { + std::string path = params.at("path").get(); + std::string pat_str = params.at("pattern").get(); + std::string include = json_value(params, "include", std::string("**")); + std::string exclude = json_value(params, "exclude", std::string("")); + bool show_lineno = json_value(params, "return_line_numbers", false); + bool literal = json_value(params, "literal", false); + bool ignore_case = json_value(params, "ignore_case", false); + int ctx_lines = std::max(0, json_value(params, "context_lines", 0)); + + std::string pattern_src = pat_str; + if (literal) { + static const std::string specials = "\\^$.|?*+()[]{}"; + std::string escaped; + escaped.reserve(pat_str.size() * 2); + for (char c : pat_str) { + if (specials.find(c) != std::string::npos) escaped += '\\'; + escaped += c; + } + pattern_src = escaped; + } std::regex pattern; try { - pattern = std::regex(pat_str); + auto flags = std::regex::ECMAScript; + if (ignore_case) flags |= std::regex::icase; + pattern = std::regex(pattern_src, flags); } catch (const std::regex_error & e) { return {{"error", std::string("invalid regex: ") + e.what()}}; } - std::ostringstream output_text; - size_t total = 0; + auto io = make_tools_io(params); - auto search_file = [&](const fs::path & fpath) { - std::ifstream f(fpath); - if (!f) return; - std::string line; - int lineno = 0; - while (std::getline(f, line) && total < SERVER_TOOL_GREP_SEARCH_MAX_RESULTS) { - lineno++; - if (std::regex_search(line, pattern)) { - output_text << fpath.string() << ":"; - if (show_lineno) { - output_text << lineno << ":"; - } - output_text << line << "\n"; - total++; - } + // collect (absolute_path, display_path) pairs to search + std::vector> files; + + if (io->is_regular_file(path)) { + files.emplace_back(path, path); + } else if (io->is_directory(path)) { + std::string err; + auto candidates = io->list_files(path, err); + if (!err.empty()) { + return {{"error", err}}; } - }; - - std::error_code ec; - if (fs::is_regular_file(path, ec)) { - search_file(path); - } else if (fs::is_directory(path, ec)) { - for (const auto & entry : fs::recursive_directory_iterator(path, - fs::directory_options::skip_permission_denied, ec)) { - if (!entry.is_regular_file()) continue; - if (total >= SERVER_TOOL_GREP_SEARCH_MAX_RESULTS) break; - - std::string rel = fs::relative(entry.path(), path, ec).string(); - if (ec) continue; - std::replace(rel.begin(), rel.end(), '\\', '/'); - - if (!glob_match(include, rel)) continue; - if (!exclude.empty() && glob_match(exclude, rel)) continue; - - search_file(entry.path()); + for (const auto & rel : candidates) { + if (!path_glob_match(include, rel)) continue; + if (!exclude.empty() && path_glob_match(exclude, rel)) continue; + files.emplace_back((fs::path(path) / rel).string(), rel); } } else { return {{"error", "path does not exist: " + path}}; } - output_text << "\n\n---\nTotal matches: " << total << "\n"; + std::ostringstream output_text; + size_t total = 0; + bool limit_reached = false; + bool show_num = show_lineno || ctx_lines > 0; + + for (const auto & file_entry : files) { + if (limit_reached) break; + const std::string & fpath = file_entry.first; + const std::string & display_path = file_entry.second; + + std::string content; + if (!io->read_file(fpath, content)) continue; + std::vector lines; + { + std::istringstream f(content); + std::string line; + while (std::getline(f, line)) lines.push_back(line); + } + + for (size_t i = 0; i < lines.size(); i++) { + if (total >= SERVER_TOOL_GREP_SEARCH_MAX_RESULTS) { + limit_reached = true; + break; + } + if (!std::regex_search(lines[i], pattern)) continue; + + long ctx_start = ctx_lines > 0 ? std::max(0, (long) i - ctx_lines) : (long) i; + long ctx_end = ctx_lines > 0 ? std::min((long) lines.size() - 1, (long) i + ctx_lines) : (long) i; + + for (long j = ctx_start; j <= ctx_end; j++) { + bool is_match = (j == (long) i); + output_text << display_path << (is_match ? ':' : '-'); + if (show_num) { + output_text << (j + 1) << (is_match ? ':' : '-'); + } + output_text << lines[j] << "\n"; + } + if (ctx_lines > 0) { + output_text << "--\n"; + } + total++; + } + } + + output_text << "\n---\nTotal matches: " << total << "\n"; + if (limit_reached) { + output_text << string_format( + "[%zu matches limit reached. Narrow the path/pattern/include to see more.]\n", + SERVER_TOOL_GREP_SEARCH_MAX_RESULTS); + } return {{"plain_text_response", output_text.str()}}; } @@ -373,7 +579,7 @@ struct server_tool_exec_shell_command : server_tool { permission_write = true; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { @@ -392,7 +598,7 @@ struct server_tool_exec_shell_command : server_tool { }; } - json invoke(json params) override { + json invoke(json params) const override { std::string command = params.at("command").get(); int timeout = json_value(params, "timeout", 10); size_t max_output = (size_t) json_value(params, "max_output_size", (int) SERVER_TOOL_EXEC_SHELL_COMMAND_MAX_OUTPUT_SIZE); @@ -406,7 +612,8 @@ struct server_tool_exec_shell_command : server_tool { std::vector args = {"sh", "-c", command}; #endif - auto res = run_process(args, max_output, timeout); + auto io = make_tools_io(params); + auto res = io->run(args, max_output, timeout); std::string text_output = res.output; text_output += string_format("\n[exit code: %d]", res.exit_code); @@ -429,7 +636,7 @@ struct server_tool_write_file : server_tool { permission_write = true; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { @@ -447,25 +654,12 @@ struct server_tool_write_file : server_tool { }; } - json invoke(json params) override { + json invoke(json params) const override { std::string path = params.at("path").get(); std::string content = params.at("content").get(); - std::error_code ec; - fs::path fpath(path); - if (fpath.has_parent_path()) { - fs::create_directories(fpath.parent_path(), ec); - if (ec) { - return {{"error", "failed to create directories: " + ec.message()}}; - } - } - - std::ofstream f(path, std::ios::binary); - if (!f) { - return {{"error", "failed to open file for writing: " + path}}; - } - f << content; - if (!f) { + auto io = make_tools_io(params); + if (!io->write_file(path, content)) { return {{"error", "failed to write file: " + path}}; } @@ -474,7 +668,7 @@ struct server_tool_write_file : server_tool { }; // -// edit_file: edit file content via line-based changes +// edit_file: exact text replacement, one or more edits per call // struct server_tool_edit_file : server_tool { @@ -484,218 +678,322 @@ struct server_tool_edit_file : server_tool { permission_write = true; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { {"name", name}, {"description", - "Edit a file by applying a list of line-based changes. " - "Each change targets a 1-based inclusive line range and has a mode: " - "\"replace\" (replace lines with content), " - "\"delete\" (remove lines, content must be empty string), " - "\"append\" (insert content after line_end). " - "Set line_start to -1 to target the end of file (line_end is ignored in that case). " - "Changes must not overlap. They are applied in reverse line order automatically."}, + "Edit a file using exact text replacement. Each edits[].old_text must be unique in the file " + "and is matched against the original content, not incrementally. Merge nearby changes into " + "one edit instead of overlapping edits. Use write_file to replace the whole file."}, {"parameters", { {"type", "object"}, {"properties", { - {"path", {{"type", "string"}, {"description", "Path to the file to edit"}}}, - {"changes", { + {"path", {{"type", "string"}, {"description", "Path to the file to edit"}}}, + {"edits", { {"type", "array"}, - {"description", "List of changes to apply"}, + {"description", "One or more exact text replacements to apply"}, {"items", { {"type", "object"}, {"properties", { - {"mode", {{"type", "string"}, {"description", "\"replace\", \"delete\", or \"append\""}}}, - {"line_start", {{"type", "integer"}, {"description", "First line of the range (1-based); use -1 for end of file"}}}, - {"line_end", {{"type", "integer"}, {"description", "Last line of the range (1-based, inclusive); ignored when line_start is -1"}}}, - {"content", {{"type", "string"}, {"description", "Content to insert; must be empty string for delete mode"}}}, + {"old_text", {{"type", "string"}, {"description", "Exact text to find; must be unique in the file and must not overlap with other edits"}}}, + {"new_text", {{"type", "string"}, {"description", "Text to replace old_text with"}}}, }}, - {"required", json::array({"mode", "line_start", "line_end", "content"})}, + {"required", json::array({"old_text", "new_text"})}, }}, }}, }}, - {"required", json::array({"path", "changes"})}, + {"required", json::array({"path", "edits"})}, }}, }}, }; } - json invoke(json params) override { + json invoke(json params) const override { std::string path = params.at("path").get(); - const json & changes = params.at("changes"); + const json & edits_json = params.at("edits"); - if (!changes.is_array()) { - return {{"error", "\"changes\" must be an array"}}; + if (!edits_json.is_array() || edits_json.empty()) { + return {{"error", "\"edits\" must be a non-empty array"}}; } - // read file into lines - std::ifstream fin(path); - if (!fin) { + struct edit_req { + std::string old_text; + std::string new_text; + }; + std::vector edits; + edits.reserve(edits_json.size()); + for (const auto & e : edits_json) { + edit_req er; + er.old_text = e.at("old_text").get(); + er.new_text = e.at("new_text").get(); + if (er.old_text.empty()) { + return {{"error", string_format("edits[%zu].old_text must not be empty", edits.size())}}; + } + edits.push_back(std::move(er)); + } + + auto io = make_tools_io(params); + std::string original_content; + if (!io->read_file(path, original_content)) { return {{"error", "failed to open file: " + path}}; } - std::vector lines; - { - std::string line; - while (std::getline(fin, line)) { - lines.push_back(line); - } - } - fin.close(); - // validate and collect changes, then sort descending by line_start - struct change_entry { - std::string mode; - int line_start; // 1-based - int line_end; // 1-based inclusive - std::string content; - }; - std::vector entries; - entries.reserve(changes.size()); - - for (const auto & ch : changes) { - change_entry e; - e.mode = ch.at("mode").get(); - e.line_start = ch.at("line_start").get(); - e.line_end = ch.at("line_end").get(); - e.content = ch.at("content").get(); - - if (e.mode != "replace" && e.mode != "delete" && e.mode != "append") { - return {{"error", "invalid mode \"" + e.mode + "\"; must be replace, delete, or append"}}; + // does any old_text need fuzzy matching (no exact match found)? + bool any_fuzzy = false; + for (size_t i = 0; i < edits.size(); i++) { + if (original_content.find(edits[i].old_text) != std::string::npos) continue; + std::string fuzzy_content = normalize_for_fuzzy_match(original_content); + std::string fuzzy_old = normalize_for_fuzzy_match(edits[i].old_text); + if (fuzzy_content.find(fuzzy_old) == std::string::npos) { + return {{"error", string_format( + "could not find edits[%zu].old_text in %s, it must match the file's current content exactly", + i, path.c_str())}}; } - if (e.mode == "delete" && !e.content.empty()) { - return {{"error", "content must be empty string for delete mode"}}; - } - int n = (int) lines.size(); - if (e.line_start == -1) { - // -1 targets end of file -> valid for append only; line_end is ignored - if (e.mode != "append") { - return {{"error", "line_start -1 (end of file) is only valid for append mode"}}; - } - // append at end of file: insert position is the current line count - e.line_start = n; - e.line_end = n; - } else { - if (e.line_start < 1 || e.line_end < e.line_start) { - return {{"error", string_format("invalid line range [%d, %d]", e.line_start, e.line_end)}}; - } - if (e.line_end > n) { - return {{"error", string_format("line_end %d exceeds file length %d", e.line_end, n)}}; - } - } - entries.push_back(std::move(e)); + any_fuzzy = true; } - // sort descending so earlier-indexed changes don't shift later ones - std::sort(entries.begin(), entries.end(), [](const change_entry & a, const change_entry & b) { - return a.line_start > b.line_start; + std::string base_content = any_fuzzy ? normalize_for_fuzzy_match(original_content) : original_content; + + // uniqueness check always uses fuzzy-normalized text, so a whitespace-only duplicate still counts + std::vector matched; + matched.reserve(edits.size()); + for (size_t i = 0; i < edits.size(); i++) { + std::string needle = any_fuzzy ? normalize_for_fuzzy_match(edits[i].old_text) : edits[i].old_text; + size_t occurrences = count_occurrences( + normalize_for_fuzzy_match(original_content), + normalize_for_fuzzy_match(edits[i].old_text)); + if (occurrences > 1) { + return {{"error", string_format( + "found %zu occurrences of edits[%zu].old_text in %s, it must be unique", + occurrences, i, path.c_str())}}; + } + size_t idx = base_content.find(needle); + matched.push_back({i, idx, needle.size(), edits[i].new_text}); + } + + std::sort(matched.begin(), matched.end(), [](const matched_edit & a, const matched_edit & b) { + return a.match_index < b.match_index; }); - - // apply changes (0-based indices internally) - for (const auto & e : entries) { - int idx_start = e.line_start - 1; // 0-based - int idx_end = e.line_end - 1; // 0-based inclusive - - // split content into lines (preserve trailing newline awareness) - std::vector new_lines; - if (!e.content.empty()) { - std::istringstream ss(e.content); - std::string ln; - while (std::getline(ss, ln)) { - new_lines.push_back(ln); - } - // if content ends with \n, getline consumed it — no extra empty line needed - // if content does NOT end with \n, last line is still captured correctly - } - - if (e.mode == "replace") { - // erase [idx_start, idx_end] and insert new_lines - lines.erase(lines.begin() + idx_start, lines.begin() + idx_end + 1); - lines.insert(lines.begin() + idx_start, new_lines.begin(), new_lines.end()); - } else if (e.mode == "delete") { - lines.erase(lines.begin() + idx_start, lines.begin() + idx_end + 1); - } else { // append - // insert after idx_end; idx_end + 1 == lines.size() for end-of-file append - lines.insert(lines.begin() + (idx_end + 1), new_lines.begin(), new_lines.end()); + for (size_t i = 1; i < matched.size(); i++) { + if (matched[i - 1].match_index + matched[i - 1].match_length > matched[i].match_index) { + return {{"error", string_format( + "edits[%zu] and edits[%zu] overlap in %s; merge them into one edit or target disjoint regions", + matched[i - 1].edit_index, matched[i].edit_index, path.c_str())}}; } } - // write file back - std::ofstream fout(path, std::ios::binary); - if (!fout) { - return {{"error", "failed to open file for writing: " + path}}; + std::string new_content = any_fuzzy + ? apply_replacements_preserving_unchanged_lines(original_content, base_content, matched) + : apply_replacements(base_content, matched, 0); + + if (new_content == original_content) { + return {{"error", "no changes made: the replacement(s) produced identical content"}}; } - for (size_t i = 0; i < lines.size(); i++) { - fout << lines[i]; - if (i + 1 < lines.size()) { - fout << "\n"; - } - } - if (!lines.empty()) { - fout << "\n"; - } - if (!fout) { + + if (!io->write_file(path, new_content)) { return {{"error", "failed to write file: " + path}}; } - return {{"result", "file edited successfully"}, {"path", path}, {"lines", (int) lines.size()}}; - } -}; - -// -// apply_diff: apply a unified diff via git apply -// - -struct server_tool_apply_diff : server_tool { - server_tool_apply_diff() { - name = "apply_diff"; - display_name = "Apply diff"; - permission_write = true; + return {{"result", "file edited successfully"}, {"path", path}, {"edits_applied", (int) matched.size()}}; } - json get_definition() override { - return { - {"type", "function"}, - {"function", { - {"name", name}, - {"description", "Apply a unified diff to edit one or more files using git apply. Use this instead of edit_file when the changes are complex."}, - {"parameters", { - {"type", "object"}, - {"properties", { - {"diff", {{"type", "string"}, {"description", "Unified diff content in git diff format"}}}, - }}, - {"required", json::array({"diff"})}, - }}, - }}, - }; - } +private: + // strip trailing whitespace, normalize smart quotes/dashes/spaces to ASCII + static std::string normalize_line_for_fuzzy_match(const std::string & line) { + size_t end = line.size(); + while (end > 0 && (line[end - 1] == ' ' || line[end - 1] == '\t' || line[end - 1] == '\r')) { + end--; + } + std::string s = line.substr(0, end); - json invoke(json params) override { - std::string diff = params.at("diff").get(); - - // write diff to a temporary file - static std::atomic counter{0}; - std::string tmp_path = (fs::temp_directory_path() / - ("llama_patch_" + std::to_string(++counter) + ".patch")).string(); - - { - std::ofstream f(tmp_path, std::ios::binary); - if (!f) { - return {{"error", "failed to create temp patch file"}}; + auto replace_all = [](std::string & str, const std::string & from, const std::string & to) { + if (from.empty()) return; + size_t pos = 0; + while ((pos = str.find(from, pos)) != std::string::npos) { + str.replace(pos, from.size(), to); + pos += to.size(); + } + }; + + // smart single quotes -> ' + for (unsigned char b : {0x98, 0x99, 0x9A, 0x9B}) { + replace_all(s, std::string("\xE2\x80") + (char) b, "'"); + } + // smart double quotes -> " + for (unsigned char b : {0x9C, 0x9D, 0x9E, 0x9F}) { + replace_all(s, std::string("\xE2\x80") + (char) b, "\""); + } + // various dashes -> - + for (unsigned char b = 0x90; b <= 0x95; b++) { + replace_all(s, std::string("\xE2\x80") + (char) b, "-"); + } + replace_all(s, "\xE2\x88\x92", "-"); // minus sign + // special spaces -> ' ' + replace_all(s, "\xC2\xA0", " "); // no-break space + for (unsigned char b = 0x82; b <= 0x8A; b++) { + replace_all(s, std::string("\xE2\x80") + (char) b, " "); + } + replace_all(s, "\xE2\x80\xAF", " "); // narrow no-break space + replace_all(s, "\xE2\x81\x9F", " "); // medium mathematical space + replace_all(s, "\xE3\x80\x80", " "); // ideographic space + + return s; + } + + // applies the per-line transform above to every line; preserves line count/positions + static std::string normalize_for_fuzzy_match(const std::string & content) { + std::string result; + result.reserve(content.size()); + size_t start = 0; + while (true) { + size_t nl = content.find('\n', start); + bool is_last = nl == std::string::npos; + std::string line = is_last ? content.substr(start) : content.substr(start, nl - start); + result += normalize_line_for_fuzzy_match(line); + if (is_last) break; + result += '\n'; + start = nl + 1; + } + return result; + } + + // lines with trailing '\n' kept, so untouched ones can be reconstructed verbatim + static std::vector split_lines_with_endings(const std::string & content) { + std::vector lines; + size_t start = 0; + while (start < content.size()) { + size_t nl = content.find('\n', start); + if (nl == std::string::npos) { + lines.push_back(content.substr(start)); + break; + } + lines.push_back(content.substr(start, nl - start + 1)); + start = nl + 1; + } + return lines; + } + + struct line_span { + size_t start; + size_t end; + }; + + static std::vector get_line_spans(const std::string & content) { + std::vector spans; + size_t offset = 0; + for (const auto & line : split_lines_with_endings(content)) { + spans.push_back({offset, offset + line.size()}); + offset += line.size(); + } + return spans; + } + + // count non-overlapping occurrences of `needle` in `content` + static size_t count_occurrences(const std::string & content, const std::string & needle) { + if (needle.empty()) return 0; + size_t count = 0, pos = 0; + while ((pos = content.find(needle, pos)) != std::string::npos) { + count++; + pos += needle.size(); + } + return count; + } + + struct matched_edit { + size_t edit_index; + size_t match_index; // offset into the "base content" (see below) + size_t match_length; + std::string new_text; + }; + + // replacements must be sorted ascending by match_index and non-overlapping + static std::string apply_replacements( + const std::string & content, + const std::vector & replacements, + size_t offset) { + std::string result = content; + for (auto it = replacements.rbegin(); it != replacements.rend(); ++it) { + size_t local_index = it->match_index - offset; + result = result.substr(0, local_index) + it->new_text + result.substr(local_index + it->match_length); + } + return result; + } + + // widen a replacement's byte range to the line(s) of `lines` it touches + static bool get_replacement_line_range( + const std::vector & lines, + size_t match_index, size_t match_length, + size_t & out_start_line, size_t & out_end_line /* exclusive */) { + size_t replacement_start = match_index; + size_t replacement_end = match_index + match_length; + + size_t start_line = (size_t) -1; + for (size_t i = 0; i < lines.size(); i++) { + if (replacement_start >= lines[i].start && replacement_start < lines[i].end) { + start_line = i; + break; + } + } + if (start_line == (size_t) -1) return false; + + size_t end_line = start_line; + while (end_line < lines.size() && lines[end_line].end < replacement_end) { + end_line++; + } + if (end_line >= lines.size()) return false; + + out_start_line = start_line; + out_end_line = end_line + 1; + return true; + } + + // like apply_replacements, but untouched lines come from `original_content` + static std::string apply_replacements_preserving_unchanged_lines( + const std::string & original_content, + const std::string & base_content, + const std::vector & replacements /* ascending, non-overlapping */) { + auto original_lines = split_lines_with_endings(original_content); + auto base_lines = get_line_spans(base_content); + + struct group { + size_t start_line; + size_t end_line; // exclusive + std::vector reps; + }; + std::vector groups; + + for (const auto & rep : replacements) { + size_t start_line = 0, end_line = 0; + get_replacement_line_range(base_lines, rep.match_index, rep.match_length, start_line, end_line); + if (!groups.empty() && start_line < groups.back().end_line) { + groups.back().end_line = std::max(groups.back().end_line, end_line); + groups.back().reps.push_back(rep); + } else { + groups.push_back({start_line, end_line, {rep}}); } - f << diff; } - auto res = run_process({"git", "apply", tmp_path}, 4096, 10); + size_t original_line_index = 0; + std::string result; + for (auto & g : groups) { + for (size_t i = original_line_index; i < g.start_line; i++) { + result += original_lines[i]; + } - std::error_code ec; - fs::remove(tmp_path, ec); + size_t group_start_offset = base_lines[g.start_line].start; + size_t group_end_offset = base_lines[g.end_line - 1].end; + std::string slice = base_content.substr(group_start_offset, group_end_offset - group_start_offset); + result += apply_replacements(slice, g.reps, group_start_offset); - if (res.exit_code != 0) { - return {{"error", "git apply failed (exit " + std::to_string(res.exit_code) + "): " + res.output}}; + original_line_index = g.end_line; } - return {{"result", "patch applied successfully"}}; + for (size_t i = original_line_index; i < original_lines.size(); i++) { + result += original_lines[i]; + } + + return result; } }; @@ -710,7 +1008,7 @@ struct server_tool_get_datetime : server_tool { permission_write = false; } - json get_definition() override { + json get_definition() const override { return { {"type", "function"}, {"function", { @@ -720,7 +1018,7 @@ struct server_tool_get_datetime : server_tool { }; } - json invoke(json) override { + json invoke(json) const override { auto now = std::chrono::system_clock::now(); auto time = std::chrono::system_clock::to_time_t(now); @@ -740,7 +1038,6 @@ static std::vector> build_tools() { tools.push_back(std::make_unique()); tools.push_back(std::make_unique()); tools.push_back(std::make_unique()); - tools.push_back(std::make_unique()); tools.push_back(std::make_unique()); return tools; } diff --git a/tools/server/server-tools.h b/tools/server/server-tools.h index 444ef5f809..cc147379bf 100644 --- a/tools/server/server-tools.h +++ b/tools/server/server-tools.h @@ -9,10 +9,10 @@ struct server_tool { bool permission_write = false; virtual ~server_tool() = default; - virtual json get_definition() = 0; - virtual json invoke(json params) = 0; + virtual json get_definition() const = 0; + virtual json invoke(json params) const = 0; - json to_json(); + json to_json() const; }; struct server_tools { diff --git a/tools/server/tests/unit/test_tools_builtin.py b/tools/server/tests/unit/test_tools_builtin.py new file mode 100755 index 0000000000..d4fd5dc9b7 --- /dev/null +++ b/tools/server/tests/unit/test_tools_builtin.py @@ -0,0 +1,125 @@ +import os + +import pytest +from utils import * + +server: ServerProcess + +# project root, used as the search directory for grep_search/file_glob_search +PROJECT_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..", "..", "..")) + +# marker for the grep_search test to find in this file +GREP_MARKER = "llama_cpp_test_tools_builtin_marker_grep_search" + + +@pytest.fixture(autouse=True) +def create_server(): + global server + server = ServerPreset.router() + server.server_tools = "all" + + +def call_tool(name: str, params: dict) -> dict: + res = server.make_request("POST", "/tools", data={"tool": name, "params": params}) + assert res.status_code == 200, res.body + assert "error" not in res.body, res.body + return res.body + + +def call_tool_expect_error(name: str, params: dict) -> str: + res = server.make_request("POST", "/tools", data={"tool": name, "params": params}) + assert res.status_code == 200, res.body + assert "error" in res.body, res.body + return res.body["error"] + + +def test_tools_builtin_grep_search(): + global server + server.start() + + res = call_tool("grep_search", { + "path": PROJECT_ROOT, + "pattern": GREP_MARKER, + "include": "test_tools_builtin.py", # bare pattern -> matches basename at any depth + }) + text = res["plain_text_response"] + assert "test_tools_builtin.py" in text + assert GREP_MARKER in text + assert "Total matches: 1" in text + + +def test_tools_builtin_read_file(): + global server + server.start() + + this_file = os.path.join(PROJECT_ROOT, "tools", "server", "tests", "unit", "test_tools_builtin.py") + res = call_tool("read_file", {"path": this_file}) + text = res["plain_text_response"] + assert GREP_MARKER in text + assert "def test_tools_builtin_read_file" in text + + +def test_tools_builtin_write_then_edit_file(): + global server + server.start() + + log_path = os.path.join(PROJECT_ROOT, "test.log") + try: + write_res = call_tool("write_file", {"path": log_path, "content": "line1\nline2\nline3\n"}) + assert write_res["result"] == "file written successfully" + + read_before = call_tool("read_file", {"path": log_path}) + assert read_before["plain_text_response"] == "line1\nline2\nline3\n" + + edit_res = call_tool("edit_file", { + "path": log_path, + "edits": [ + {"old_text": "line2", "new_text": "line2-edited"}, + {"old_text": "line3\n", "new_text": "line3\nline4\n"}, + ], + }) + assert edit_res["result"] == "file edited successfully" + assert edit_res["edits_applied"] == 2 + + read_after = call_tool("read_file", {"path": log_path}) + assert read_after["plain_text_response"] == "line1\nline2-edited\nline3\nline4\n" + finally: + if os.path.exists(log_path): + os.remove(log_path) + + +def test_tools_builtin_edit_file_rejects_non_unique_old_text(): + global server + server.start() + + log_path = os.path.join(PROJECT_ROOT, "test.log") + try: + call_tool("write_file", {"path": log_path, "content": "dup\ndup\n"}) + err = call_tool_expect_error("edit_file", { + "path": log_path, + "edits": [{"old_text": "dup", "new_text": "changed"}], + }) + assert "unique" in err + finally: + if os.path.exists(log_path): + os.remove(log_path) + + +def test_tools_builtin_edit_file_rejects_overlapping_edits(): + global server + server.start() + + log_path = os.path.join(PROJECT_ROOT, "test.log") + try: + call_tool("write_file", {"path": log_path, "content": "line1\nline2\n"}) + err = call_tool_expect_error("edit_file", { + "path": log_path, + "edits": [ + {"old_text": "line1\nline2", "new_text": "a"}, + {"old_text": "line2", "new_text": "b"}, + ], + }) + assert "overlap" in err + finally: + if os.path.exists(log_path): + os.remove(log_path) diff --git a/tools/server/tests/utils.py b/tools/server/tests/utils.py index 67d7d20dbd..8c0de384f6 100644 --- a/tools/server/tests/utils.py +++ b/tools/server/tests/utils.py @@ -113,6 +113,7 @@ class ServerProcess: ui_mcp_proxy: bool = False backend_sampling: bool = False gcp_compat: bool = False + server_tools: str | None = None # session variables process: subprocess.Popen | None = None @@ -256,6 +257,8 @@ class ServerProcess: server_args.append("--no-cache-idle-slots") if self.ui_mcp_proxy: server_args.append("--ui-mcp-proxy") + if self.server_tools: + server_args.extend(["--tools", self.server_tools]) if self.backend_sampling: server_args.append("--backend_sampling") if self.gcp_compat: