diff --git a/include/repo_tools.hpp b/include/repo_tools.hpp index 0ef13dc..b20a8fb 100644 --- a/include/repo_tools.hpp +++ b/include/repo_tools.hpp @@ -39,5 +39,13 @@ nlohmann::json git_show(const std::string& workspace_abs, size_t context_lines, size_t output_limit = 0); +nlohmann::json git_add(const std::string& workspace_abs, + const std::vector& pathspecs, + size_t output_limit = 0); + +nlohmann::json git_commit(const std::string& workspace_abs, + const std::string& message, + size_t output_limit = 0); + void set_rg_binary_for_testing(const std::string& path); void clear_rg_binary_for_testing(); diff --git a/include/tool_registry.hpp b/include/tool_registry.hpp index f6a2d09..465ce68 100644 --- a/include/tool_registry.hpp +++ b/include/tool_registry.hpp @@ -25,6 +25,8 @@ struct ToolDescriptor { std::string name; std::string description; ToolCategory category = ToolCategory::ReadOnly; + bool mutates_repository_state = false; + bool can_execute_repo_controlled_code = false; bool requires_approval = false; nlohmann::json json_schema = nlohmann::json::object(); size_t max_output_bytes = 0; diff --git a/src/agent_tools.cpp b/src/agent_tools.cpp index 80326d4..8bfeb16 100644 --- a/src/agent_tools.cpp +++ b/src/agent_tools.cpp @@ -113,6 +113,14 @@ bool optional_bool_arg(const ToolCall& cmd, const char* key, bool default_value return value.get(); } +std::string require_non_empty_string_arg(const ToolCall& cmd, const char* key, const char* tool_name) { + const std::string value = require_string_arg(cmd, key, tool_name); + if (value.empty()) { + throw std::runtime_error("Argument '" + std::string(key) + "' for " + tool_name + " must not be empty."); + } + return value; +} + size_t parse_bounded_output_bytes_arg(const ToolCall& cmd, const char* key, size_t default_value) { const size_t parsed = optional_size_arg(cmd, key, default_value); if (parsed == 0 || parsed > kMaxBuildToolOutputBytes) { @@ -228,6 +236,8 @@ ToolRegistry build_default_tool_registry() { .name = "read_file_safe", .description = "Reads the complete contents of a workspace file.", .category = ToolCategory::ReadOnly, + .mutates_repository_state = false, + .can_execute_repo_controlled_code = false, .requires_approval = false, .json_schema = make_parameters_schema({ {"path", { @@ -252,6 +262,8 @@ ToolRegistry build_default_tool_registry() { .name = "write_file_safe", .description = "Writes string content to a workspace file, overwriting existing content.", .category = ToolCategory::Mutating, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = false, .requires_approval = true, .json_schema = make_parameters_schema({ {"path", { @@ -279,6 +291,8 @@ ToolRegistry build_default_tool_registry() { .name = "bash_execute_safe", .description = "Executes a bounded bash command inside the workspace.", .category = ToolCategory::Execution, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = true, .requires_approval = true, .json_schema = make_parameters_schema({ {"command", { @@ -311,6 +325,8 @@ ToolRegistry build_default_tool_registry() { .name = "build_project_safe", .description = "Runs ./build.sh in debug or release mode with bounded retained output.", .category = ToolCategory::Execution, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = true, .requires_approval = true, .json_schema = make_parameters_schema({ {"build_mode", { @@ -360,6 +376,8 @@ ToolRegistry build_default_tool_registry() { .name = "test_project_safe", .description = "Runs ./build.sh test with bounded retained output and a small ctest summary.", .category = ToolCategory::Execution, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = true, .requires_approval = true, .json_schema = make_parameters_schema({ {"timeout_ms", { @@ -407,6 +425,8 @@ ToolRegistry build_default_tool_registry() { .name = "list_files_bounded", .description = "Lists workspace files with optional directory and extension filters.", .category = ToolCategory::ReadOnly, + .mutates_repository_state = false, + .can_execute_repo_controlled_code = false, .requires_approval = false, .json_schema = make_parameters_schema({ {"directory", { @@ -438,6 +458,8 @@ ToolRegistry build_default_tool_registry() { .name = "rg_search", .description = "Searches the workspace with ripgrep and returns structured matches.", .category = ToolCategory::ReadOnly, + .mutates_repository_state = false, + .can_execute_repo_controlled_code = false, .requires_approval = false, .json_schema = make_parameters_schema({ {"query", { @@ -473,6 +495,8 @@ ToolRegistry build_default_tool_registry() { .name = "git_status", .description = "Returns the current git working tree status for the workspace.", .category = ToolCategory::ReadOnly, + .mutates_repository_state = false, + .can_execute_repo_controlled_code = false, .requires_approval = false, .json_schema = make_parameters_schema(nlohmann::json::object()), .max_output_bytes = kMaxRepoOutputBytes, @@ -481,6 +505,47 @@ ToolRegistry build_default_tool_registry() { } }); + register_or_throw(®istry, ToolDescriptor{ + .name = "git_add", + .description = "Stages explicitly listed git pathspecs into the index.", + .category = ToolCategory::Mutating, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = true, + .requires_approval = true, + .json_schema = make_parameters_schema({ + {"pathspecs", { + {"type", "array"}, + {"items", {{"type", "string"}}}, + {"description", "Explicit git pathspecs to stage. Must contain at least one entry."} + }} + }, {"pathspecs"}), + .max_output_bytes = kMaxRepoOutputBytes, + .execute = [](const ToolCall& cmd, const AgentConfig& config, size_t output_limit) { + const std::vector pathspecs = optional_string_array_arg_strict(cmd, "pathspecs"); + return git_add(config.workspace_abs, pathspecs, output_limit); + } + }); + + register_or_throw(®istry, ToolDescriptor{ + .name = "git_commit", + .description = "Creates a git commit from the currently staged index changes.", + .category = ToolCategory::Mutating, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = true, + .requires_approval = true, + .json_schema = make_parameters_schema({ + {"message", { + {"type", "string"}, + {"description", "Commit message for the staged index changes."} + }} + }, {"message"}), + .max_output_bytes = kMaxRepoOutputBytes, + .execute = [](const ToolCall& cmd, const AgentConfig& config, size_t output_limit) { + const std::string message = require_string_arg(cmd, "message", "git_commit"); + return git_commit(config.workspace_abs, message, output_limit); + } + }); + register_or_throw(®istry, ToolDescriptor{ .name = "apply_patch", .description = "Applies an exact-text replacement patch to a workspace file. " @@ -488,6 +553,8 @@ ToolRegistry build_default_tool_registry() { "old_text must appear exactly once in the file. " "An explicit empty new_text is valid and deletes the matched text.", .category = ToolCategory::Mutating, + .mutates_repository_state = true, + .can_execute_repo_controlled_code = false, .requires_approval = true, .json_schema = nlohmann::json{ {"type", "object"}, @@ -678,6 +745,8 @@ ToolRegistry build_default_tool_registry() { "By default shows unstaged changes; set cached=true for staged changes. " "Optionally filter by pathspecs and control hunk context with context_lines (default 3).", .category = ToolCategory::ReadOnly, + .mutates_repository_state = false, + .can_execute_repo_controlled_code = false, .requires_approval = false, .json_schema = make_parameters_schema({ {"cached", { @@ -716,6 +785,8 @@ ToolRegistry build_default_tool_registry() { "rev is required. Optionally disable patch or stat output, filter by pathspecs, " "and control diff context with context_lines (default 3).", .category = ToolCategory::ReadOnly, + .mutates_repository_state = false, + .can_execute_repo_controlled_code = false, .requires_approval = false, .json_schema = make_parameters_schema({ {"rev", { diff --git a/src/repo_tools.cpp b/src/repo_tools.cpp index 1146203..c633c28 100644 --- a/src/repo_tools.cpp +++ b/src/repo_tools.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -39,6 +40,48 @@ struct ChildProcess { int stderr_fd = -1; }; +struct GitTextCommandResult { + bool launched = false; + std::string stdout_text; + std::string stderr_text; + int exit_code = -1; + bool truncated = false; + bool killed_early = false; + std::string err; +}; + +nlohmann::json make_git_add_error_result(std::string stdout_text, + std::string stderr_text, + int exit_code, + bool truncated, + std::string error) { + return { + {"ok", false}, + {"stdout", std::move(stdout_text)}, + {"stderr", std::move(stderr_text)}, + {"exit_code", exit_code}, + {"truncated", truncated}, + {"error", std::move(error)} + }; +} + +nlohmann::json make_git_commit_error_result(std::string stdout_text, + std::string stderr_text, + int exit_code, + std::string commit_sha, + bool truncated, + std::string error) { + return { + {"ok", false}, + {"stdout", std::move(stdout_text)}, + {"stderr", std::move(stderr_text)}, + {"exit_code", exit_code}, + {"commit_sha", std::move(commit_sha)}, + {"truncated", truncated}, + {"error", std::move(error)} + }; +} + void close_fd(int fd) { if (fd >= 0) { close(fd); @@ -156,18 +199,43 @@ bool flush_excess_stdout_buffer(std::string* stdout_buffer, return true; } -std::string normalize_git_repo_error(std::string failure) { - failure = trim_line_endings(std::move(failure)); - std::string folded = failure; - std::transform(folded.begin(), folded.end(), folded.begin(), [](unsigned char ch) { +std::string ascii_lower(std::string text) { + std::transform(text.begin(), text.end(), text.begin(), [](unsigned char ch) { return static_cast(std::tolower(ch)); }); + return text; +} + +std::string normalize_git_repo_error(std::string failure) { + failure = trim_line_endings(std::move(failure)); + const std::string folded = ascii_lower(failure); if (folded.find("not a git repository") != std::string::npos) { return "Current workspace is not a git repository."; } return failure; } +std::string normalize_git_commit_error(const std::string& stdout_text, + const std::string& stderr_text) { + std::string combined; + if (!stdout_text.empty()) { + combined += stdout_text; + } + if (!stderr_text.empty()) { + if (!combined.empty()) { + combined.push_back('\n'); + } + combined += stderr_text; + } + combined = trim_line_endings(std::move(combined)); + const std::string folded = ascii_lower(combined); + if (folded.find("nothing to commit") != std::string::npos || + folded.find("no changes added to commit") != std::string::npos) { + return "No staged changes to commit."; + } + return normalize_git_repo_error(combined); +} + size_t clamp_git_context_lines(size_t context_lines) { return std::min(context_lines, kMaxGitContextLines); } @@ -527,6 +595,7 @@ bool stream_command_stdout_lines(const std::string& executable, std::string* stderr_text, int* exit_code, bool* killed_early, + bool* stderr_truncated, std::string* err, size_t output_limit = 0) { ChildProcess child; @@ -537,6 +606,7 @@ bool stream_command_stdout_lines(const std::string& executable, if (stderr_text) stderr_text->clear(); if (exit_code) *exit_code = -1; if (killed_early) *killed_early = false; + if (stderr_truncated) *stderr_truncated = false; std::string stdout_buffer; char buffer[4096]; @@ -626,6 +696,11 @@ bool stream_command_stdout_lines(const std::string& executable, if (stderr_text && stderr_text->size() < kMaxCommandStderrBytes) { const size_t keep = std::min(static_cast(n), kMaxCommandStderrBytes - stderr_text->size()); stderr_text->append(buffer, keep); + if (keep < static_cast(n) && stderr_truncated) { + *stderr_truncated = true; + } + } else if (stderr_truncated) { + *stderr_truncated = true; } continue; } @@ -820,6 +895,365 @@ bool stream_command_stdout_records(const std::string& executable, return err == nullptr || err->empty(); } +GitTextCommandResult run_git_text_command(const std::string& git_binary, + const std::vector& args, + const std::string& workspace_abs, + size_t output_limit) { + GitTextCommandResult result; + bool stderr_truncated = false; + auto on_line = [&](const std::string& line) -> bool { + return append_bounded_output_line(&result.stdout_text, line, output_limit, &result.truncated); + }; + + result.launched = stream_command_stdout_lines(git_binary, args, workspace_abs, + on_line, &result.stderr_text, &result.exit_code, + &result.killed_early, &stderr_truncated, + &result.err, output_limit); + if (!result.stdout_text.empty() && result.stdout_text.back() == '\n') { + result.stdout_text.pop_back(); + } + result.stderr_text = trim_line_endings(std::move(result.stderr_text)); + result.truncated = result.truncated || stderr_truncated; + return result; +} + +bool starts_with_path_component(const std::string& path, const std::string& prefix) { + if (prefix.empty()) { + return true; + } + return path == prefix || path.starts_with(prefix + "/"); +} + +bool resolve_workspace_relative_git_path(const std::string& workspace_abs, + const std::string& input, + std::string* normalized_rel, + std::string* err) { + if (input.empty()) { + if (err) *err = "Argument 'pathspecs' for git_add must not contain empty path strings."; + return false; + } + if (input.front() == ':') { + if (err) *err = "Git pathspec magic is not supported for git_add."; + return false; + } + + AgentConfig cfg; + cfg.workspace_abs = workspace_abs; + + std::string safe_abs; + std::string resolve_err; + if (!workspace_resolve(cfg, input, &safe_abs, &resolve_err)) { + if (err) *err = "Path must stay within the workspace: " + resolve_err; + return false; + } + + const fs::path rel_path = fs::path(safe_abs).lexically_relative(fs::path(workspace_abs)); + if (rel_path.empty()) { + if (normalized_rel) *normalized_rel = "."; + } else { + if (normalized_rel) *normalized_rel = rel_path.generic_string(); + } + return true; +} + +bool get_repo_root_and_workspace_prefix(const std::string& git_binary, + const std::string& workspace_abs, + std::string* repo_root, + std::string* workspace_prefix, + std::string* err) { + const std::vector args = {git_binary, "rev-parse", "--show-toplevel"}; + GitTextCommandResult result = run_git_text_command(git_binary, args, workspace_abs, 0); + if (!result.launched) { + if (err) *err = result.err.empty() ? result.stderr_text : result.err; + return false; + } + if (result.exit_code != 0) { + if (err) *err = normalize_git_repo_error(result.stderr_text); + return false; + } + + const fs::path repo_root_path = fs::path(trim_line_endings(result.stdout_text)).lexically_normal(); + const fs::path workspace_path = fs::path(workspace_abs).lexically_normal(); + const fs::path relative = workspace_path.lexically_relative(repo_root_path); + const std::string relative_text = relative.generic_string(); + if (relative.empty() && workspace_path != repo_root_path) { + if (err) *err = "Current workspace is not inside the git repository root."; + return false; + } + if (relative_text == ".." || relative_text.starts_with("../")) { + if (err) *err = "Current workspace is not inside the git repository root."; + return false; + } + + if (repo_root) *repo_root = repo_root_path.string(); + if (workspace_prefix) *workspace_prefix = (relative_text == "." ? "" : relative_text); + return true; +} + +bool list_staged_paths(const std::string& git_binary, + const std::string& workspace_abs, + std::vector* staged_paths, + std::string* stderr_text, + int* exit_code, + std::string* err) { + const std::vector args = { + git_binary, + "diff", + "--cached", + "--name-status", + "-z", + "--" + }; + + if (staged_paths) { + staged_paths->clear(); + } + + bool killed_early = false; + std::vector records; + const bool launched = stream_command_stdout_records( + git_binary, + args, + workspace_abs, + '\0', + [&](const std::string& record) -> bool { + records.push_back(record); + return true; + }, + stderr_text, + exit_code, + &killed_early, + err); + if (!launched) { + return false; + } + + for (size_t i = 0; i < records.size(); ++i) { + const std::string& status = records[i]; + if (status.empty()) { + continue; + } + + const auto push_path = [&](const std::string& path_text) { + if (!path_text.empty() && staged_paths) { + staged_paths->push_back(fs::path(path_text).lexically_normal().generic_string()); + } + }; + + const char code = status[0]; + if ((code == 'R' || code == 'C') && status.size() >= 2) { + if (i + 2 >= records.size()) { + if (err) *err = "Failed to parse staged rename/copy paths before git commit."; + return false; + } + push_path(records[i + 1]); + push_path(records[i + 2]); + i += 2; + continue; + } + + if (i + 1 >= records.size()) { + if (err) *err = "Failed to parse staged paths before git commit."; + return false; + } + push_path(records[i + 1]); + i += 1; + } + + return true; +} + +bool resolve_head_sha(const std::string& git_binary, + const std::string& workspace_abs, + std::string* head_sha, + std::string* err) { + const std::vector args = {git_binary, "rev-parse", "HEAD"}; + GitTextCommandResult result = run_git_text_command(git_binary, args, workspace_abs, 0); + if (!result.launched) { + if (err) *err = result.err.empty() ? result.stderr_text : result.err; + return false; + } + if (result.exit_code != 0) { + const std::string normalized = normalize_git_repo_error(result.stderr_text); + const std::string folded = ascii_lower(normalized); + if (folded.find("unknown revision or path not in the working tree") != std::string::npos || + folded.find("ambiguous argument 'head'") != std::string::npos || + folded.find("needed a single revision") != std::string::npos) { + if (head_sha) head_sha->clear(); + return true; + } + if (err) *err = normalized; + return false; + } + + if (head_sha) { + *head_sha = trim_line_endings(result.stdout_text); + } + return true; +} + +bool list_head_reflog_shas(const std::string& git_binary, + const std::string& workspace_abs, + std::vector* shas, + std::string* err) { + const std::vector args = {git_binary, "reflog", "--format=%H", "HEAD"}; + GitTextCommandResult result = run_git_text_command(git_binary, args, workspace_abs, 0); + if (!result.launched) { + if (err) *err = result.err.empty() ? result.stderr_text : result.err; + return false; + } + if (result.exit_code != 0) { + const std::string normalized = normalize_git_repo_error(result.stderr_text); + const std::string folded = ascii_lower(normalized); + if (folded.find("unknown revision or path not in the working tree") != std::string::npos || + folded.find("ambiguous argument 'head'") != std::string::npos || + folded.find("needed a single revision") != std::string::npos || + folded.find("your current branch 'main' does not have any commits yet") != std::string::npos || + folded.find("your current branch does not have any commits yet") != std::string::npos) { + if (shas) shas->clear(); + return true; + } + if (err) *err = normalized; + return false; + } + + if (shas) { + shas->clear(); + std::istringstream lines(result.stdout_text); + for (std::string line; std::getline(lines, line);) { + line = trim_line_endings(std::move(line)); + if (!line.empty()) { + shas->push_back(line); + } + } + } + return true; +} + +struct GitCommitObservedState { + std::string head_sha; + std::vector reflog_shas; +}; + +bool capture_git_commit_observed_state(const std::string& git_binary, + const std::string& workspace_abs, + const std::string& head_failure_message, + const std::string& reflog_failure_message, + GitCommitObservedState* state, + std::string* err) { + if (!state) { + if (err) *err = "Internal error: missing git commit observed state output."; + return false; + } + + state->head_sha.clear(); + state->reflog_shas.clear(); + + std::string head_err; + if (!resolve_head_sha(git_binary, workspace_abs, &state->head_sha, &head_err)) { + if (err) *err = head_err.empty() ? head_failure_message : head_err; + return false; + } + + std::string reflog_err; + if (!list_head_reflog_shas(git_binary, workspace_abs, &state->reflog_shas, &reflog_err)) { + if (err) *err = reflog_err.empty() ? reflog_failure_message : reflog_err; + return false; + } + + return true; +} + +std::string first_new_commit_from_reflog(const std::vector& before, + const std::vector& after) { + if (after.size() < before.size()) { + return ""; + } + + const size_t new_count = after.size() - before.size(); + for (size_t i = 0; i < before.size(); ++i) { + if (after[new_count + i] != before[i]) { + return ""; + } + } + + if (new_count == 0) { + return ""; + } + return after[new_count - 1]; +} + +bool resolve_created_commit_sha_from_ancestry(const std::string& git_binary, + const std::string& workspace_abs, + const std::string& head_before, + const std::string& head_after, + std::string* commit_sha, + std::string* err) { + if (commit_sha) { + commit_sha->clear(); + } + if (head_after.empty()) { + return true; + } + + std::vector args = {git_binary, "rev-list", "--reverse"}; + if (head_before.empty()) { + args.push_back(head_after); + } else { + args.push_back("--ancestry-path"); + args.push_back(head_before + ".." + head_after); + } + + GitTextCommandResult result = run_git_text_command(git_binary, args, workspace_abs, 0); + if (!result.launched) { + if (err) *err = result.err.empty() ? result.stderr_text : result.err; + return false; + } + if (result.exit_code != 0) { + if (err) *err = normalize_git_repo_error(result.stderr_text); + return false; + } + + std::istringstream lines(result.stdout_text); + std::string first_commit; + for (std::string line; std::getline(lines, line);) { + line = trim_line_endings(std::move(line)); + if (!line.empty()) { + first_commit = line; + break; + } + } + + if (commit_sha) { + *commit_sha = first_commit; + } + return true; +} + +bool resolve_created_commit_sha(const std::string& git_binary, + const std::string& workspace_abs, + const std::string& head_before, + const std::string& head_after, + const std::vector& reflog_before, + const std::vector& reflog_after, + std::string* commit_sha, + std::string* err) { + if (commit_sha) { + commit_sha->clear(); + } + + const std::string from_reflog = first_new_commit_from_reflog(reflog_before, reflog_after); + if (!from_reflog.empty()) { + if (commit_sha) { + *commit_sha = from_reflog; + } + return true; + } + + return resolve_created_commit_sha_from_ancestry( + git_binary, workspace_abs, head_before, head_after, commit_sha, err); +} + std::string join_relative_path(const std::string& base, const std::string& child) { if (base.empty()) { return fs::path(child).lexically_normal().generic_string(); @@ -1130,7 +1564,8 @@ nlohmann::json rg_search(const std::string& workspace_abs, return true; }; - if (!stream_command_stdout_lines(rg_binary, args, abs_directory, on_line, &stderr_text, &exit_code, &killed_early, &err)) { + if (!stream_command_stdout_lines(rg_binary, args, abs_directory, on_line, &stderr_text, &exit_code, &killed_early, + nullptr, &err)) { if (parse_failed) { err = parse_error; } else if (err.empty()) { @@ -1410,7 +1845,7 @@ nlohmann::json git_diff(const std::string& workspace_abs, if (!stream_command_stdout_lines(git_binary, args, workspace_abs, on_line, &stderr_text, &exit_code, - &killed_early, &err, output_limit)) { + &killed_early, nullptr, &err, output_limit)) { if (err.empty()) { err = trim_line_endings(stderr_text); } @@ -1530,7 +1965,7 @@ nlohmann::json git_show(const std::string& workspace_abs, if (!stream_command_stdout_lines(git_binary, args, workspace_abs, on_line, &stderr_text, &exit_code, - &killed_early, &err, output_limit)) { + &killed_early, nullptr, &err, output_limit)) { if (err.empty()) { err = trim_line_endings(stderr_text); } @@ -1574,3 +2009,231 @@ nlohmann::json git_show(const std::string& workspace_abs, {"error", ""} }; } + +nlohmann::json git_add(const std::string& workspace_abs, + const std::vector& pathspecs, + size_t output_limit) { + if (pathspecs.empty()) { + return make_git_add_error_result( + "", "", -1, false, "Argument 'pathspecs' for git_add must contain at least one pathspec."); + } + + const std::string git_binary = find_executable_in_path("git"); + if (git_binary.empty()) { + return make_git_add_error_result( + "", "", -1, false, "git is not installed or not executable in this environment."); + } + + std::vector safe_paths; + safe_paths.reserve(pathspecs.size()); + for (const auto& pathspec : pathspecs) { + std::string normalized_rel; + std::string validate_err; + if (!resolve_workspace_relative_git_path(workspace_abs, pathspec, &normalized_rel, &validate_err)) { + return make_git_add_error_result("", "", -1, false, std::move(validate_err)); + } + safe_paths.push_back(std::move(normalized_rel)); + } + + std::vector args = {git_binary, "add", "--"}; + args.insert(args.end(), safe_paths.begin(), safe_paths.end()); + + GitTextCommandResult command = run_git_text_command(git_binary, args, workspace_abs, output_limit); + if (!command.launched) { + if (command.err.empty()) { + command.err = command.stderr_text; + } + return make_git_add_error_result( + "", command.stderr_text, command.exit_code, command.truncated, + command.err.empty() ? "git add failed." : command.err); + } + + if (command.killed_early && command.truncated) { + return make_git_add_error_result(command.stdout_text, + command.stderr_text, + -1, + command.truncated, + "git add output exceeded the configured limit before the command completed."); + } + + if (command.exit_code != 0) { + const std::string failure = normalize_git_repo_error(command.stderr_text); + return make_git_add_error_result(command.stdout_text, + command.stderr_text, + command.exit_code, + command.truncated, + failure.empty() ? "git add exited with code " + std::to_string(command.exit_code) + : failure); + } + + return { + {"ok", true}, + {"stdout", command.stdout_text}, + {"stderr", command.stderr_text}, + {"exit_code", command.exit_code}, + {"truncated", command.truncated}, + {"error", ""} + }; +} + +nlohmann::json git_commit(const std::string& workspace_abs, + const std::string& message, + size_t output_limit) { + if (message.empty()) { + return make_git_commit_error_result( + "", "", -1, "", false, "Argument 'message' for git_commit must not be empty."); + } + + const std::string git_binary = find_executable_in_path("git"); + if (git_binary.empty()) { + return make_git_commit_error_result( + "", "", -1, "", false, "git is not installed or not executable in this environment."); + } + + std::string repo_root; + std::string workspace_prefix; + std::string repo_err; + if (!get_repo_root_and_workspace_prefix(git_binary, workspace_abs, &repo_root, &workspace_prefix, &repo_err)) { + return make_git_commit_error_result( + "", "", -1, "", false, repo_err.empty() ? "Failed to resolve git repository root." : repo_err); + } + + std::vector staged_paths; + std::string staged_stderr; + int staged_exit_code = -1; + std::string staged_err; + if (!list_staged_paths(git_binary, workspace_abs, &staged_paths, &staged_stderr, &staged_exit_code, &staged_err)) { + return make_git_commit_error_result("", + trim_line_endings(staged_stderr), + staged_exit_code, + "", + false, + staged_err.empty() ? "Failed to inspect staged paths before git commit." + : staged_err); + } + if (staged_exit_code != 0) { + const std::string failure = normalize_git_repo_error(staged_stderr); + return make_git_commit_error_result("", + trim_line_endings(staged_stderr), + staged_exit_code, + "", + false, + failure.empty() ? "Failed to inspect staged paths before git commit." + : failure); + } + for (const auto& staged_path : staged_paths) { + if (!starts_with_path_component(staged_path, workspace_prefix)) { + return make_git_commit_error_result( + "", "", -1, "", false, "Refusing to commit staged path outside the workspace: " + staged_path); + } + } + + GitCommitObservedState before_state; + std::string before_state_err; + if (!capture_git_commit_observed_state(git_binary, + workspace_abs, + "Failed to resolve HEAD before git commit.", + "Failed to inspect HEAD reflog before git commit.", + &before_state, + &before_state_err)) { + return make_git_commit_error_result("", "", -1, "", false, before_state_err); + } + + std::vector args = {git_binary}; + if (!workspace_prefix.empty()) { + args.push_back("-c"); + args.push_back("core.hooksPath=/dev/null"); + } + args.insert(args.end(), { + "commit", + "-m", message + }); + + GitTextCommandResult command = run_git_text_command(git_binary, args, workspace_abs, output_limit); + if (!command.launched) { + if (command.err.empty()) { + command.err = command.stderr_text; + } + return make_git_commit_error_result( + "", command.stderr_text, command.exit_code, "", command.truncated, + command.err.empty() ? "git commit failed." : command.err); + } + + GitCommitObservedState after_state; + std::string after_state_err; + if (!capture_git_commit_observed_state(git_binary, + workspace_abs, + "Failed to resolve HEAD after git commit.", + "Failed to inspect HEAD reflog after git commit.", + &after_state, + &after_state_err)) { + return make_git_commit_error_result( + command.stdout_text, command.stderr_text, -1, "", command.truncated, after_state_err); + } + + std::string created_commit_sha; + std::string created_commit_err; + if (!resolve_created_commit_sha(git_binary, + workspace_abs, + before_state.head_sha, + after_state.head_sha, + before_state.reflog_shas, + after_state.reflog_shas, + &created_commit_sha, + &created_commit_err)) { + return make_git_commit_error_result( + command.stdout_text, + command.stderr_text, + -1, + "", + command.truncated, + created_commit_err.empty() ? "Failed to determine which commit git commit created." : created_commit_err); + } + + const bool head_advanced = !created_commit_sha.empty(); + if (command.killed_early && command.truncated) { + if (head_advanced) { + return { + {"ok", true}, + {"stdout", command.stdout_text}, + {"stderr", command.stderr_text}, + {"exit_code", 0}, + {"commit_sha", created_commit_sha}, + {"truncated", command.truncated}, + {"error", ""} + }; + } + return make_git_commit_error_result(command.stdout_text, + command.stderr_text, + -1, + "", + command.truncated, + "git commit output exceeded the configured limit before the command completed."); + } + + if (command.exit_code != 0) { + const std::string failure = normalize_git_commit_error(command.stdout_text, command.stderr_text); + return make_git_commit_error_result(command.stdout_text, + command.stderr_text, + command.exit_code, + "", + command.truncated, + failure.empty() ? "git commit exited with code " + std::to_string(command.exit_code) + : failure); + } + + if (!head_advanced) { + return make_git_commit_error_result( + command.stdout_text, command.stderr_text, -1, "", command.truncated, "git commit did not create a new HEAD commit."); + } + + return { + {"ok", true}, + {"stdout", command.stdout_text}, + {"stderr", command.stderr_text}, + {"exit_code", command.exit_code}, + {"commit_sha", created_commit_sha}, + {"truncated", command.truncated}, + {"error", ""} + }; +} diff --git a/src/tool_registry.cpp b/src/tool_registry.cpp index 79002c3..bb5a3a5 100644 --- a/src/tool_registry.cpp +++ b/src/tool_registry.cpp @@ -12,17 +12,38 @@ nlohmann::json make_registry_error(const std::string& message) { }; } -nlohmann::json make_approval_blocked_result(const ToolDescriptor& descriptor) { +nlohmann::json missing_approvals_for(const ToolDescriptor& descriptor, const AgentConfig& config) { + nlohmann::json missing = nlohmann::json::array(); + if (descriptor.mutates_repository_state && !config.allow_mutating_tools) { + missing.push_back("mutating"); + } + if (descriptor.can_execute_repo_controlled_code && !config.allow_execution_tools) { + missing.push_back("execution"); + } + return missing; +} + +nlohmann::json make_approval_blocked_result(const ToolDescriptor& descriptor, const AgentConfig& config) { + const bool requires_mutating = descriptor.mutates_repository_state; + const bool requires_execution = descriptor.can_execute_repo_controlled_code; + const nlohmann::json missing = missing_approvals_for(descriptor, config); + std::string error = "Tool execution requires approval under the current policy."; - switch (descriptor.category) { - case ToolCategory::Mutating: - error = "Mutating tool execution is blocked by default. Enable allow_mutating_tools, set NCA_ALLOW_MUTATING_TOOLS=1, or pass --allow-mutating-tools."; - break; - case ToolCategory::Execution: - error = "Execution tool use is blocked by default. Enable allow_execution_tools, set NCA_ALLOW_EXECUTION_TOOLS=1, or pass --allow-execution-tools."; - break; - case ToolCategory::ReadOnly: - break; + if (requires_mutating && requires_execution && + missing == nlohmann::json::array({"mutating", "execution"})) { + error = + "Tool execution is blocked by default because this tool both mutates repository state and can " + "execute repository-controlled code. Enable allow_mutating_tools, set NCA_ALLOW_MUTATING_TOOLS=1, " + "or pass --allow-mutating-tools, and enable allow_execution_tools, set " + "NCA_ALLOW_EXECUTION_TOOLS=1, or pass --allow-execution-tools."; + } else if (requires_mutating && !config.allow_mutating_tools) { + error = + "Mutating tool execution is blocked by default. Enable allow_mutating_tools, set " + "NCA_ALLOW_MUTATING_TOOLS=1, or pass --allow-mutating-tools."; + } else if (requires_execution && !config.allow_execution_tools) { + error = + "Execution tool use is blocked by default. Enable allow_execution_tools, set " + "NCA_ALLOW_EXECUTION_TOOLS=1, or pass --allow-execution-tools."; } return nlohmann::json{ @@ -31,6 +52,9 @@ nlohmann::json make_approval_blocked_result(const ToolDescriptor& descriptor) { {"tool", descriptor.name}, {"category", tool_category_to_string(descriptor.category)}, {"requires_approval", descriptor.requires_approval}, + {"requires_mutating_approval", requires_mutating}, + {"requires_execution_approval", requires_execution}, + {"missing_approvals", missing}, {"error", error} }; } @@ -50,15 +74,7 @@ bool tool_execution_allowed(const ToolDescriptor& descriptor, const AgentConfig& return true; } - switch (descriptor.category) { - case ToolCategory::ReadOnly: - return true; - case ToolCategory::Mutating: - return config.allow_mutating_tools; - case ToolCategory::Execution: - return config.allow_execution_tools; - } - return false; + return missing_approvals_for(descriptor, config).empty(); } } // namespace @@ -89,7 +105,7 @@ bool ToolRegistry::register_tool(ToolDescriptor descriptor, std::string* err) { return false; } - if (descriptor.category == ToolCategory::ReadOnly) { + if (!descriptor.mutates_repository_state && !descriptor.can_execute_repo_controlled_code) { descriptor.requires_approval = false; } @@ -115,7 +131,7 @@ std::string ToolRegistry::execute(const ToolCall& call, const AgentConfig& confi const size_t output_limit = resolve_effective_output_limit(descriptor->max_output_bytes, config.max_tool_output_bytes); if (!tool_execution_allowed(*descriptor, config)) { - return make_approval_blocked_result(*descriptor).dump(); + return make_approval_blocked_result(*descriptor, config).dump(); } try { diff --git a/tests/test_agent_loop_limits.cpp b/tests/test_agent_loop_limits.cpp index 137ccb7..009c23f 100644 --- a/tests/test_agent_loop_limits.cpp +++ b/tests/test_agent_loop_limits.cpp @@ -93,6 +93,7 @@ class AgentLoopLimitsTest : public ::testing::Test { TEST_F(AgentLoopLimitsTest, MaxTurnsHalt) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; config.max_tool_calls_per_turn = 5; @@ -127,6 +128,7 @@ TEST_F(AgentLoopLimitsTest, MaxTurnsHalt) { TEST_F(AgentLoopLimitsTest, MaxToolCallsPerTurn) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 10; config.max_tool_calls_per_turn = 2; // Limit 2 @@ -155,6 +157,7 @@ TEST_F(AgentLoopLimitsTest, MaxToolCallsPerTurn) { TEST_F(AgentLoopLimitsTest, MaxTotalToolCalls) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 10; config.max_tool_calls_per_turn = 5; @@ -354,6 +357,7 @@ TEST_F(AgentLoopLimitsTest, BuildFailureContinuesWithInspectionGuidance) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; @@ -394,6 +398,7 @@ TEST_F(AgentLoopLimitsTest, TestFailureContinuesWithFailedTestsGuidance) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; @@ -437,6 +442,7 @@ TEST_F(AgentLoopLimitsTest, TestFailureGuidanceEllipsizesAfterVisibleFailedTests AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; @@ -475,6 +481,7 @@ TEST_F(AgentLoopLimitsTest, TestFailureWithoutParsedFailedTestsOmitsMalformedFra AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; @@ -514,6 +521,7 @@ TEST_F(AgentLoopLimitsTest, BuildTimeoutContinuesWithRetryGuidance) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; @@ -678,6 +686,7 @@ TEST_F(AgentLoopLimitsTest, RepeatedBlockedFailureStopsAfterRetryBudget) { TEST_F(AgentLoopLimitsTest, UnsupportedStructuredFailureRemainsFatal) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; @@ -860,6 +869,7 @@ TEST_F(AgentLoopLimitsTest, GuidanceMessageStaysSmallAndActionable) { AgentConfig config; config.workspace_abs = test_workspace; + config.allow_mutating_tools = true; config.allow_execution_tools = true; config.max_turns = 3; diff --git a/tests/test_build_test_tools.cpp b/tests/test_build_test_tools.cpp index e2a3d7a..c2bab5a 100644 --- a/tests/test_build_test_tools.cpp +++ b/tests/test_build_test_tools.cpp @@ -74,6 +74,7 @@ class BuildTestToolsTest : public ::testing::Test { AgentConfig approved_config() const { AgentConfig config; config.workspace_abs = workspace_; + config.allow_mutating_tools = true; config.allow_execution_tools = true; return config; } diff --git a/tests/test_repo_tools.cpp b/tests/test_repo_tools.cpp index fd5ef52..d733c64 100644 --- a/tests/test_repo_tools.cpp +++ b/tests/test_repo_tools.cpp @@ -403,6 +403,567 @@ TEST_F(RepoToolsTest, GitStatusExecutorEnforcesOutputLimit) { EXPECT_EQ(result["returned"].get(), result["entries"].size()); } +TEST_F(RepoToolsTest, GitAddRejectsEmptyPathspecs) { + const auto result = git_add(test_workspace, {}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["exit_code"].get(), -1); + EXPECT_NE(result["error"].get().find("pathspec"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitAddRejectsEmptyPathStringElements) { + const auto result = git_add(test_workspace, {""}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["exit_code"].get(), -1); + EXPECT_EQ(result["error"].get(), + "Argument 'pathspecs' for git_add must not contain empty path strings."); +} + +TEST_F(RepoToolsTest, GitAddFailsGracefullyOutsideGitRepo) { + const auto result = git_add(test_workspace, {"tracked.txt"}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("git repository"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitAddStagesOnlyRequestedPathspecs) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + create_file("only.txt", "only\n"); + create_file("other.txt", "other\n"); + + const auto add_result = git_add(test_workspace, {"only.txt"}); + ASSERT_TRUE(add_result["ok"].get()) << add_result.dump(); + + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && ! git diff --cached --quiet -- only.txt"), 0); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git diff --cached --quiet -- other.txt"), 0); +} + +TEST_F(RepoToolsTest, GitAddHandlesDashPrefixedFilename) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + create_file("-dash.txt", "dash\n"); + + const auto result = git_add(test_workspace, {"-dash.txt"}); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && ! git diff --cached --quiet -- -- '-dash.txt'"), 0); +} + +TEST_F(RepoToolsTest, GitAddRejectsParentEscapePathspec) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("root.txt", "root\n"); + + const auto result = git_add((fs::path(test_workspace) / "subdir").string(), {"../root.txt"}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("workspace"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitAddRejectsAbsolutePath) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + create_file("tracked.txt", "tracked\n"); + + const auto result = git_add(test_workspace, {(fs::path(test_workspace) / "tracked.txt").string()}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("workspace"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitAddRejectsPathspecMagic) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + create_file("tracked.txt", "tracked\n"); + + const auto result = git_add(test_workspace, {":(top)tracked.txt"}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("pathspec magic"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitAddRejectsRepoRootPathspecMagic) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("outside.txt", "outside\n"); + + const auto result = git_add((fs::path(test_workspace) / "subdir").string(), {":/outside.txt"}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("pathspec magic"), std::string::npos); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git diff --cached --quiet -- outside.txt"), 0); +} + +TEST_F(RepoToolsTest, GitAddRejectsShortFormPathspecMagic) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + create_file("inside.txt", "inside\n"); + + const auto result = git_add(test_workspace, {":!inside.txt"}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("pathspec magic"), std::string::npos); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git diff --cached --quiet -- inside.txt"), 0); +} + +TEST_F(RepoToolsTest, GitAddRejectsBareColonPathspecMagic) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + create_file("inside.txt", "inside\n"); + + const auto result = git_add(test_workspace, {":"}); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("pathspec magic"), std::string::npos); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git diff --cached --quiet -- inside.txt"), 0); +} + +TEST_F(RepoToolsTest, GitCommitRejectsEmptyMessage) { + const auto result = git_commit(test_workspace, ""); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["exit_code"].get(), -1); + EXPECT_NE(result["error"].get().find("message"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitCommitFailsGracefullyOutsideGitRepo) { + const auto result = git_commit(test_workspace, "msg"); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("git repository"), std::string::npos); +} + +TEST_F(RepoToolsTest, GitCommitWithoutStagedChangesReturnsReadableError) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1"), 0); + create_file("tracked.txt", "changed but unstaged\n"); + + const auto result = git_commit(test_workspace, "second"); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("staged"), std::string::npos); + EXPECT_TRUE(!result["stdout"].get().empty() || + !result["stderr"].get().empty()); +} + +TEST_F(RepoToolsTest, GitCommitCreatesCommitFromStagedIndex) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > staged.txt &&" + " printf 'keep\\n' > unstaged.txt &&" + " git add staged.txt unstaged.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1"), 0); + + create_file("staged.txt", "staged change\n"); + create_file("unstaged.txt", "worktree only\n"); + ASSERT_TRUE(git_add(test_workspace, {"staged.txt"})["ok"].get()); + + const auto result = git_commit(test_workspace, "second"); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + + ASSERT_EQ(run_bash("cd '" + test_workspace + + "' && test \"$(git show HEAD:staged.txt)\" = \"staged change\""), 0); + ASSERT_EQ(run_bash("cd '" + test_workspace + + "' && test \"$(git show HEAD:unstaged.txt)\" = \"keep\""), 0); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && ! git diff --quiet -- unstaged.txt"), 0); +} + +TEST_F(RepoToolsTest, GitCommitReturnsCommitSha) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + create_file("tracked.txt", "hello\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const auto result = git_commit(test_workspace, "init"); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + ASSERT_TRUE(result.contains("commit_sha")); + const std::string sha = result["commit_sha"].get(); + EXPECT_FALSE(sha.empty()); + + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git rev-parse HEAD > head.txt"), 0); + std::ifstream in(fs::path(test_workspace) / "head.txt"); + std::string head; + std::getline(in, head); + EXPECT_EQ(sha, head); +} + +TEST_F(RepoToolsTest, GitCommitPreservesExplicitHashPrefixedMessageLines) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + create_file("tracked.txt", "hello\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + ASSERT_TRUE(git_commit(test_workspace, "init")["ok"].get()); + + create_file("tracked.txt", "next\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const std::string message = "subject\n#kept\nbody"; + const auto result = git_commit(test_workspace, message); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + + ASSERT_EQ(run_bash("cd '" + test_workspace + + "' && git log -1 --format=%B > actual_message.txt &&" + " grep -Fx 'subject' actual_message.txt >/dev/null &&" + " grep -Fx '#kept' actual_message.txt >/dev/null &&" + " grep -Fx 'body' actual_message.txt >/dev/null"), 0); +} + +TEST_F(RepoToolsTest, GitCommitRejectsStagedPathsOutsideWorkspace) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("subdir/inside.txt", "inside\n"); + create_file("outside.txt", "outside\n"); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git add subdir/inside.txt outside.txt >/dev/null 2>&1"), 0); + ASSERT_NE(run_bash("cd '" + test_workspace + "' && git rev-parse HEAD >/dev/null 2>&1"), 0); + + const auto result = git_commit((fs::path(test_workspace) / "subdir").string(), "blocked"); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("outside the workspace"), std::string::npos); + ASSERT_NE(run_bash("cd '" + test_workspace + "' && git rev-parse HEAD >/dev/null 2>&1"), 0); +} + +TEST_F(RepoToolsTest, GitCommitSucceedsWhenAllStagedPathsStayInsideWorkspace) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("subdir/inside.txt", "inside\n"); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git add subdir/inside.txt >/dev/null 2>&1"), 0); + + const auto result = git_commit((fs::path(test_workspace) / "subdir").string(), "inside only"); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + ASSERT_EQ(run_bash("cd '" + test_workspace + + "' && test \"$(git show HEAD:subdir/inside.txt)\" = \"inside\""), 0); +} + +TEST_F(RepoToolsTest, GitCommitInRepoRootStillWorks) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + create_file("tracked.txt", "hello\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const auto result = git_commit(test_workspace, "root commit"); + ASSERT_TRUE(result["ok"].get()) << result.dump(); +} + +TEST_F(RepoToolsTest, GitCommitRejectsCrossBoundaryRenameIntoWorkspace) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("outside.txt", "outside\n"); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git add outside.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1 &&" + " git mv outside.txt subdir/inside.txt"), 0); + + const auto before = git_show(test_workspace, "HEAD", false, false, {}, 3); + ASSERT_TRUE(before["ok"].get()) << before.dump(); + + const auto result = git_commit((fs::path(test_workspace) / "subdir").string(), "rename blocked"); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("outside the workspace"), std::string::npos); + + const auto after = git_show(test_workspace, "HEAD", false, false, {}, 3); + ASSERT_TRUE(after["ok"].get()) << after.dump(); + EXPECT_EQ(before["stdout"], after["stdout"]); +} + +TEST_F(RepoToolsTest, GitCommitRejectsCrossBoundaryRenameOutOfWorkspace) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("subdir/inside.txt", "inside\n"); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git add subdir/inside.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1 &&" + " git mv subdir/inside.txt outside.txt"), 0); + + const auto before = git_show(test_workspace, "HEAD", false, false, {}, 3); + ASSERT_TRUE(before["ok"].get()) << before.dump(); + + const auto result = git_commit((fs::path(test_workspace) / "subdir").string(), "rename blocked"); + EXPECT_FALSE(result["ok"].get()); + EXPECT_NE(result["error"].get().find("outside the workspace"), std::string::npos); + + const auto after = git_show(test_workspace, "HEAD", false, false, {}, 3); + ASSERT_TRUE(after["ok"].get()) << after.dump(); + EXPECT_EQ(before["stdout"], after["stdout"]); +} + +TEST_F(RepoToolsTest, GitAddFailurePropagatesTruncatedFlag) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1"), 0); + const fs::path filter_script = fs::path(test_workspace) / "broken-clean.sh"; + { + std::ofstream out(filter_script); + out << "#!/bin/sh\n"; + out << "i=0\n"; + out << "while [ $i -lt 2000 ]; do printf 'clean-filter-line-%03d\\n' \"$i\" 1>&2; i=$((i+1)); done\n"; + out << "exit 1\n"; + } + fs::permissions(filter_script, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + create_file(".gitattributes", "*.txt filter=broken\n"); + create_file("tracked.txt", "hello\n"); + ASSERT_EQ(run_bash("cd '" + test_workspace + + "' && git config filter.broken.clean '" + filter_script.string() + "' &&" + " git config filter.broken.required true"), 0); + + const auto result = git_add(test_workspace, {"tracked.txt"}, 40); + EXPECT_FALSE(result["ok"].get()); + EXPECT_TRUE(result["truncated"].get()) << result.dump(); +} + +TEST_F(RepoToolsTest, GitAddKilledEarlyByOutputLimitIsReportedAsFailure) { + const fs::path custom_bin = fs::path(test_workspace) / "custom-bin"; + fs::create_directories(custom_bin); + const fs::path fake_git = custom_bin / "git"; + { + std::ofstream out(fake_git); + out << "#!/bin/sh\n"; + out << "i=0\n"; + out << "while [ $i -lt 200000 ]; do printf 'fake-git-add-line-%06d\\n' \"$i\"; i=$((i+1)); done\n"; + out << "exit 0\n"; + } + fs::permissions(fake_git, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + const char* old_path_cstr = std::getenv("PATH"); + const std::string old_path = old_path_cstr ? old_path_cstr : ""; + ScopedEnvVar scoped_path("PATH", custom_bin.string() + ":" + old_path); + + const auto result = git_add(test_workspace, {"tracked.txt"}, 40); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_TRUE(result["truncated"].get()) << result.dump(); + EXPECT_EQ(result["error"].get(), + "git add output exceeded the configured limit before the command completed."); +} + +TEST_F(RepoToolsTest, GitCommitFailurePropagatesTruncatedFlag) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'hello\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1"), 0); + const fs::path hook_dir = fs::path(test_workspace) / ".git" / "hooks"; + const fs::path hook = hook_dir / "commit-msg"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "i=0\n"; + out << "while [ $i -lt 2000 ]; do printf 'hook-output-line-%03d\\n' \"$i\"; i=$((i+1)); done\n"; + out << "exit 1\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + const auto result = git_commit(test_workspace, "hook fail", 40); + EXPECT_FALSE(result["ok"].get()); + EXPECT_TRUE(result["truncated"].get()) << result.dump(); +} + +TEST_F(RepoToolsTest, GitCommitKilledEarlyByOutputLimitIsReportedAsFailure) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1"), 0); + create_file("tracked.txt", "changed\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const fs::path hook = fs::path(test_workspace) / ".git" / "hooks" / "pre-commit"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "i=0\n"; + out << "while [ $i -lt 200000 ]; do printf 'hook-output-line-%06d\\n' \"$i\"; i=$((i+1)); done\n"; + out << "exit 1\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git rev-parse HEAD > before_head.txt"), 0); + const auto result = git_commit(test_workspace, "killed", 40); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_TRUE(result["truncated"].get()) << result.dump(); + EXPECT_TRUE(result["commit_sha"].get().empty()) << result.dump(); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git rev-parse HEAD > after_head.txt"), 0); + std::ifstream before_in(fs::path(test_workspace) / "before_head.txt"); + std::ifstream after_in(fs::path(test_workspace) / "after_head.txt"); + std::string before_head; + std::string after_head; + std::getline(before_in, before_head); + std::getline(after_in, after_head); + EXPECT_EQ(after_head, before_head) << result.dump(); +} + +TEST_F(RepoToolsTest, GitCommitDoesNotReturnOldHeadWhenCommandWasKilledEarly) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1 &&" + " git rev-parse HEAD > before_head.txt"), 0); + create_file("tracked.txt", "changed\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const fs::path hook = fs::path(test_workspace) / ".git" / "hooks" / "pre-commit"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "i=0\n"; + out << "while [ $i -lt 200000 ]; do printf 'hook-output-line-%06d\\n' \"$i\"; i=$((i+1)); done\n"; + out << "exit 0\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + const auto result = git_commit(test_workspace, "killed", 40); + EXPECT_TRUE(result["truncated"].get()) << result.dump(); + std::ifstream before_in(fs::path(test_workspace) / "before_head.txt"); + std::string before_head; + std::getline(before_in, before_head); + EXPECT_NE(result["commit_sha"].get(), before_head) << result.dump(); +} + +TEST_F(RepoToolsTest, GitCommitStderrOnlyFailureDoesNotPrefixErrorWithNewline) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'hello\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1"), 0); + const fs::path hook = fs::path(test_workspace) / ".git" / "hooks" / "commit-msg"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "printf 'stderr-only-hook-failure\\n' 1>&2\n"; + out << "exit 1\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + const auto result = git_commit(test_workspace, "stderr only"); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["error"].get(), "stderr-only-hook-failure"); +} + +TEST_F(RepoToolsTest, GitCommitIgnoresHooksWhenEnforcingWorkspaceOnlyCommits) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + fs::create_directories(fs::path(test_workspace) / "subdir"); + create_file("subdir/inside.txt", "inside\n"); + create_file("outside.txt", "outside\n"); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git add subdir/inside.txt >/dev/null 2>&1"), 0); + + const fs::path hook = fs::path(test_workspace) / ".git" / "hooks" / "pre-commit"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "printf 'mutated by hook\\n' > outside.txt\n"; + out << "git add outside.txt\n"; + out << "exit 0\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + const auto result = git_commit((fs::path(test_workspace) / "subdir").string(), "inside only"); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + ASSERT_EQ(run_bash("cd '" + test_workspace + + "' && test \"$(git show HEAD:subdir/inside.txt)\" = \"inside\""), 0); + ASSERT_NE(run_bash("cd '" + test_workspace + "' && git show HEAD:outside.txt >/dev/null 2>&1"), 0); +} + +TEST_F(RepoToolsTest, GitCommitRespectsRepositorySigningPolicy) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " git config commit.gpgSign true &&" + " git config gpg.program /definitely/missing/gpg"), 0); + create_file("tracked.txt", "hello\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const auto result = git_commit(test_workspace, "signed commit"); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_TRUE(result["commit_sha"].get().empty()) << result.dump(); + EXPECT_NE(result["error"].get().find("gpg"), std::string::npos) << result.dump(); + ASSERT_NE(run_bash("cd '" + test_workspace + "' && git rev-parse HEAD >/dev/null 2>&1"), 0); +} + +TEST_F(RepoToolsTest, GitCommitKilledEarlyAfterHeadAdvanceReturnsSuccess) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1 &&" + " git rev-parse HEAD > before_head.txt"), 0); + create_file("tracked.txt", "changed\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const fs::path hook = fs::path(test_workspace) / ".git" / "hooks" / "post-commit"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "i=0\n"; + out << "while [ $i -lt 200000 ]; do printf 'post-commit-line-%06d\\n' \"$i\"; i=$((i+1)); done\n"; + out << "exit 0\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + const auto result = git_commit(test_workspace, "post noisy", 40); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + EXPECT_TRUE(result["truncated"].get()) << result.dump(); + EXPECT_FALSE(result["commit_sha"].get().empty()) << result.dump(); + + std::ifstream before_in(fs::path(test_workspace) / "before_head.txt"); + std::string before_head; + std::getline(before_in, before_head); + EXPECT_NE(result["commit_sha"].get(), before_head) << result.dump(); +} + +TEST_F(RepoToolsTest, GitCommitReturnsCreatedCommitWhenPostCommitHookAdvancesHead) { + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1"), 0); + create_file("tracked.txt", "changed\n"); + ASSERT_TRUE(git_add(test_workspace, {"tracked.txt"})["ok"].get()); + + const fs::path hook = fs::path(test_workspace) / ".git" / "hooks" / "post-commit"; + { + std::ofstream out(hook); + out << "#!/bin/sh\n"; + out << "if [ \"${NANO_EXTRA_POST_COMMIT:-}\" = \"1\" ]; then\n"; + out << " exit 0\n"; + out << "fi\n"; + out << "export NANO_EXTRA_POST_COMMIT=1\n"; + out << "printf 'from-hook\\n' > hook.txt\n"; + out << "git add hook.txt >/dev/null 2>&1 || exit 1\n"; + out << "git commit -m hook-generated >/dev/null 2>&1 || exit 1\n"; + out << "exit 0\n"; + } + fs::permissions(hook, + fs::perms::owner_exec | fs::perms::owner_read | fs::perms::owner_write | + fs::perms::group_exec | fs::perms::group_read | + fs::perms::others_exec | fs::perms::others_read); + + const auto result = git_commit(test_workspace, "main commit"); + ASSERT_TRUE(result["ok"].get()) << result.dump(); + const std::string commit_sha = result["commit_sha"].get(); + ASSERT_FALSE(commit_sha.empty()) << result.dump(); + + std::ifstream head_in(fs::path(test_workspace) / ".git" / "HEAD"); + (void)head_in; + const std::string final_head_cmd = "cd '" + test_workspace + "' && git rev-parse HEAD > final_head.txt"; + ASSERT_EQ(run_bash(final_head_cmd), 0); + std::ifstream final_head_in(fs::path(test_workspace) / "final_head.txt"); + std::string final_head; + std::getline(final_head_in, final_head); + EXPECT_NE(commit_sha, final_head) << result.dump(); + + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && test \"$(git show " + commit_sha + ":tracked.txt)\" = \"changed\""), 0); + ASSERT_NE(run_bash("cd '" + test_workspace + "' && git show " + commit_sha + ":hook.txt >/dev/null 2>&1"), 0); + ASSERT_EQ(run_bash("cd '" + test_workspace + "' && test \"$(git show HEAD:hook.txt)\" = \"from-hook\""), 0); +} + // --------------------------------------------------------------------------- // GitDiffTest: git_diff tool // --------------------------------------------------------------------------- diff --git a/tests/test_schema_and_args_tolerance.cpp b/tests/test_schema_and_args_tolerance.cpp index ca4a372..2b92310 100644 --- a/tests/test_schema_and_args_tolerance.cpp +++ b/tests/test_schema_and_args_tolerance.cpp @@ -34,7 +34,7 @@ int run_bash(const std::string& command) { TEST(SchemaAndArgsToleranceTest, GetSchemaMatchesCurrentTools) { json schema = get_agent_tools_schema(); - EXPECT_EQ(schema.size(), 11u); + EXPECT_EQ(schema.size(), 13u); bool has_read = false; bool has_write = false; @@ -47,6 +47,8 @@ TEST(SchemaAndArgsToleranceTest, GetSchemaMatchesCurrentTools) { bool has_apply_patch = false; bool has_git_diff = false; bool has_git_show = false; + bool has_git_add = false; + bool has_git_commit = false; for (const auto& tool : schema) { std::string name = tool["function"]["name"]; if (name == "read_file_safe") has_read = true; @@ -60,6 +62,8 @@ TEST(SchemaAndArgsToleranceTest, GetSchemaMatchesCurrentTools) { if (name == "apply_patch") has_apply_patch = true; if (name == "git_diff") has_git_diff = true; if (name == "git_show") has_git_show = true; + if (name == "git_add") has_git_add = true; + if (name == "git_commit") has_git_commit = true; } EXPECT_TRUE(has_read); EXPECT_TRUE(has_write); @@ -72,11 +76,14 @@ TEST(SchemaAndArgsToleranceTest, GetSchemaMatchesCurrentTools) { EXPECT_TRUE(has_apply_patch); EXPECT_TRUE(has_git_diff); EXPECT_TRUE(has_git_show); + EXPECT_TRUE(has_git_add); + EXPECT_TRUE(has_git_commit); } TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsOutOfRangeInteger) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -94,6 +101,7 @@ TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsOutOfRangeInteger) { TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsOutOfRangeString) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -111,6 +119,7 @@ TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsOutOfRangeString) { TEST(SchemaAndArgsToleranceTest, BashTimeoutToleratesStrings) { AgentConfig config; config.workspace_abs = "."; // Allow mock safe dir + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -130,6 +139,7 @@ TEST(SchemaAndArgsToleranceTest, BashTimeoutToleratesStrings) { TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsUnsignedOverflow) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -146,6 +156,7 @@ TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsUnsignedOverflow) { TEST(SchemaAndArgsToleranceTest, BashTimeoutRejectsStringOverflow) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -295,6 +306,116 @@ TEST(SchemaAndArgsToleranceTest, BashExecuteSafeBlockedWithoutApprovalHasNoSideE std::filesystem::remove_all(test_workspace); } +TEST(SchemaAndArgsToleranceTest, BashExecuteSafeRequiresMutatingApprovalEvenIfExecutionAllowed) { + const auto test_workspace = + (std::filesystem::temp_directory_path() / + ("nano_bash_mutating_req_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(test_workspace); + std::filesystem::create_directories(test_workspace); + + AgentConfig config; + config.workspace_abs = test_workspace; + config.allow_execution_tools = true; + + ToolCall bash_call; + bash_call.name = "bash_execute_safe"; + bash_call.arguments = { + {"command", "printf 'hi' > blocked.txt"} + }; + + const json result = json::parse(execute_tool(bash_call, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "execution"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"mutating"})); + EXPECT_FALSE(std::filesystem::exists(std::filesystem::path(test_workspace) / "blocked.txt")); + + std::filesystem::remove_all(test_workspace); +} + +TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRequiresMutatingApprovalEvenIfExecutionAllowed) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_build_mutating_req_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + { + std::ofstream out(std::filesystem::path(ws) / "build.sh"); + out << "#!/bin/sh\n"; + out << "printf 'build blocked\\n' > build-side-effect.txt\n"; + out << "exit 0\n"; + } + std::filesystem::permissions(std::filesystem::path(ws) / "build.sh", + std::filesystem::perms::owner_exec | std::filesystem::perms::owner_read | + std::filesystem::perms::owner_write | std::filesystem::perms::group_exec | + std::filesystem::perms::group_read | std::filesystem::perms::others_exec | + std::filesystem::perms::others_read); + + AgentConfig config; + config.workspace_abs = ws; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "build_project_safe"; + tc.arguments = json::object(); + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "execution"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"mutating"})); + EXPECT_FALSE(std::filesystem::exists(std::filesystem::path(ws) / "build-side-effect.txt")); + + std::filesystem::remove_all(ws); +} + +TEST(SchemaAndArgsToleranceTest, TestProjectSafeRequiresMutatingApprovalEvenIfExecutionAllowed) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_test_mutating_req_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + { + std::ofstream out(std::filesystem::path(ws) / "build.sh"); + out << "#!/bin/sh\n"; + out << "printf 'test blocked\\n' > test-side-effect.txt\n"; + out << "exit 0\n"; + } + std::filesystem::permissions(std::filesystem::path(ws) / "build.sh", + std::filesystem::perms::owner_exec | std::filesystem::perms::owner_read | + std::filesystem::perms::owner_write | std::filesystem::perms::group_exec | + std::filesystem::perms::group_read | std::filesystem::perms::others_exec | + std::filesystem::perms::others_read); + + AgentConfig config; + config.workspace_abs = ws; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "test_project_safe"; + tc.arguments = json::object(); + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "execution"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"mutating"})); + EXPECT_FALSE(std::filesystem::exists(std::filesystem::path(ws) / "test-side-effect.txt")); + + std::filesystem::remove_all(ws); +} + TEST(SchemaAndArgsToleranceTest, WriteFileSafeBlockedWithoutMutatingApprovalHasNoSideEffects) { const auto test_workspace = (std::filesystem::temp_directory_path() / @@ -357,6 +478,204 @@ TEST(SchemaAndArgsToleranceTest, ApplyPatchBlockedWithoutMutatingApprovalHasNoSi std::filesystem::remove_all(test_workspace); } +TEST(SchemaAndArgsToleranceTest, GitAddBlockedWithoutMutatingApprovalHasNoSideEffects) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_git_add_blocked_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + { + std::ofstream out(std::filesystem::path(ws) / "blocked.txt"); + out << "blocked\n"; + } + + AgentConfig config; + config.workspace_abs = ws; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "git_add"; + tc.arguments = {{"pathspecs", json::array({"blocked.txt"})}}; + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"mutating"})); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git diff --cached --quiet -- blocked.txt"), 0) + << "blocked git_add must not stage anything"; + + std::filesystem::remove_all(ws); +} + +TEST(SchemaAndArgsToleranceTest, GitAddRequiresExecutionApprovalEvenIfMutatingAllowed) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_git_add_exec_req_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + { + std::ofstream out(std::filesystem::path(ws) / "blocked.txt"); + out << "blocked\n"; + } + + AgentConfig config; + config.workspace_abs = ws; + config.allow_mutating_tools = true; + config.allow_execution_tools = false; + + ToolCall tc; + tc.name = "git_add"; + tc.arguments = {{"pathspecs", json::array({"blocked.txt"})}}; + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"execution"})); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git diff --cached --quiet -- blocked.txt"), 0) + << "blocked git_add must not stage anything"; + + std::filesystem::remove_all(ws); +} + +TEST(SchemaAndArgsToleranceTest, GitCommitBlockedWithoutMutatingApprovalHasNoSideEffects) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_git_commit_blocked_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1 &&" + " printf 'next\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1"), 0); + + AgentConfig config; + config.workspace_abs = ws; + config.allow_execution_tools = true; + + const std::string before_head_cmd = + "cd '" + ws + "' && git rev-parse HEAD > head_before.txt"; + ASSERT_EQ(run_bash(before_head_cmd), 0); + + ToolCall tc; + tc.name = "git_commit"; + tc.arguments = {{"message", "blocked commit"}}; + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"mutating"})); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git rev-parse HEAD > head_after.txt"), 0); + std::ifstream before(std::filesystem::path(ws) / "head_before.txt"); + std::ifstream after(std::filesystem::path(ws) / "head_after.txt"); + std::string before_sha; + std::string after_sha; + std::getline(before, before_sha); + std::getline(after, after_sha); + EXPECT_EQ(before_sha, after_sha) << "blocked git_commit must not create a commit"; + + ASSERT_EQ(run_bash("cd '" + ws + "' && ! git diff --cached --quiet -- tracked.txt"), 0) + << "blocked git_commit must leave staged changes untouched"; + + std::filesystem::remove_all(ws); +} + +TEST(SchemaAndArgsToleranceTest, GitCommitRequiresMutatingApprovalEvenIfExecutionAllowed) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_git_commit_mutating_req_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'hello\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1"), 0); + + AgentConfig config; + config.workspace_abs = ws; + config.allow_mutating_tools = false; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "git_commit"; + tc.arguments = {{"message", "blocked commit"}}; + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"mutating"})); + + std::filesystem::remove_all(ws); +} + +TEST(SchemaAndArgsToleranceTest, GitCommitRequiresExecutionApprovalEvenIfMutatingAllowed) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_git_commit_exec_req_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T &&" + " printf 'base\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1 &&" + " git commit -m init >/dev/null 2>&1 &&" + " printf 'next\\n' > tracked.txt &&" + " git add tracked.txt >/dev/null 2>&1"), 0); + + AgentConfig config; + config.workspace_abs = ws; + config.allow_mutating_tools = true; + config.allow_execution_tools = false; + + ToolCall tc; + tc.name = "git_commit"; + tc.arguments = {{"message", "blocked commit"}}; + + const json result = json::parse(execute_tool(tc, config)); + EXPECT_FALSE(result["ok"].get()) << result.dump(); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], json::array({"execution"})); + + ASSERT_EQ(run_bash("cd '" + ws + "' && ! git diff --cached --quiet -- tracked.txt"), 0) + << "blocked git_commit must leave staged changes untouched"; + + std::filesystem::remove_all(ws); +} + TEST(SchemaAndArgsToleranceTest, SchemaIncludesBuildAndTestSafeTools) { json schema = get_agent_tools_schema(); json build_params; @@ -388,6 +707,7 @@ TEST(SchemaAndArgsToleranceTest, SchemaIncludesBuildAndTestSafeTools) { TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsInvalidBuildMode) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -401,6 +721,7 @@ TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsInvalidBuildMode) { TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsInvalidCleanFirstType) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -414,6 +735,7 @@ TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsInvalidCleanFirstType) { TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsUnsupportedTarget) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -427,6 +749,7 @@ TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsUnsupportedTarget) { TEST(SchemaAndArgsToleranceTest, TestProjectSafeRejectsUnsupportedFilter) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -440,6 +763,7 @@ TEST(SchemaAndArgsToleranceTest, TestProjectSafeRejectsUnsupportedFilter) { TEST(SchemaAndArgsToleranceTest, TestProjectSafeRejectsEnsureDebugBuild) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -453,6 +777,7 @@ TEST(SchemaAndArgsToleranceTest, TestProjectSafeRejectsEnsureDebugBuild) { TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsTimeoutOverflow) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -466,6 +791,7 @@ TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsTimeoutOverflow) { TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsHugeOutputLimit) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -479,6 +805,7 @@ TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsHugeOutputLimit) { TEST(SchemaAndArgsToleranceTest, BuildProjectSafeRejectsZeroOutputLimit) { AgentConfig config; config.workspace_abs = "."; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -511,6 +838,7 @@ TEST(SchemaAndArgsToleranceTest, BuildProjectSafeDispatchReturnsStructuredJson) AgentConfig config; config.workspace_abs = ws; + config.allow_mutating_tools = true; config.allow_execution_tools = true; ToolCall tc; @@ -895,6 +1223,30 @@ TEST(SchemaAndArgsToleranceTest, SchemaIncludesGitDiffAndGitShow) { EXPECT_EQ(git_show_params["required"], json::array({"rev"})); } +TEST(SchemaAndArgsToleranceTest, SchemaIncludesGitAddAndGitCommit) { + json schema = get_agent_tools_schema(); + json git_add_params; + json git_commit_params; + + for (const auto& tool : schema) { + const std::string name = tool["function"]["name"]; + if (name == "git_add") { + git_add_params = tool["function"]["parameters"]; + } + if (name == "git_commit") { + git_commit_params = tool["function"]["parameters"]; + } + } + + ASSERT_TRUE(git_add_params.is_object()); + ASSERT_TRUE(git_commit_params.is_object()); + EXPECT_EQ(git_add_params["properties"]["pathspecs"]["type"], "array"); + EXPECT_EQ(git_add_params["properties"]["pathspecs"]["items"]["type"], "string"); + EXPECT_EQ(git_add_params["required"], json::array({"pathspecs"})); + EXPECT_EQ(git_commit_params["properties"]["message"]["type"], "string"); + EXPECT_EQ(git_commit_params["required"], json::array({"message"})); +} + TEST(SchemaAndArgsToleranceTest, ExecuteToolDispatchesGitDiffAndGitShow) { const auto ws = (std::filesystem::temp_directory_path() / @@ -933,6 +1285,83 @@ TEST(SchemaAndArgsToleranceTest, ExecuteToolDispatchesGitDiffAndGitShow) { std::filesystem::remove_all(ws); } +TEST(SchemaAndArgsToleranceTest, ExecuteToolDispatchesGitAddAndGitCommit) { + const auto ws = + (std::filesystem::temp_directory_path() / + ("nano_schema_gitpackage_" + std::to_string(getpid()))) + .string(); + std::filesystem::remove_all(ws); + std::filesystem::create_directories(ws); + + ASSERT_EQ(run_bash("cd '" + ws + "' && git init -b main >/dev/null 2>&1 &&" + " git config user.email t@t.com && git config user.name T"), 0); + { + std::ofstream out(std::filesystem::path(ws) / "tracked.txt"); + out << "hello\n"; + } + + AgentConfig config; + config.workspace_abs = ws; + config.allow_mutating_tools = true; + config.allow_execution_tools = true; + + ToolCall add_call; + add_call.name = "git_add"; + add_call.arguments = {{"pathspecs", json::array({"tracked.txt"})}}; + const json add_json = json::parse(execute_tool(add_call, config)); + EXPECT_TRUE(add_json.contains("ok")) << add_json.dump(); + + ToolCall commit_call; + commit_call.name = "git_commit"; + commit_call.arguments = {{"message", "init"}}; + const json commit_json = json::parse(execute_tool(commit_call, config)); + EXPECT_TRUE(commit_json.contains("ok")) << commit_json.dump(); + + std::filesystem::remove_all(ws); +} + +TEST(SchemaAndArgsToleranceTest, GitAddRejectsNonArrayPathspecs) { + AgentConfig config; + config.workspace_abs = "."; + config.allow_mutating_tools = true; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "git_add"; + tc.arguments = {{"pathspecs", "not-an-array"}}; + + const std::string res = execute_tool(tc, config); + EXPECT_NE(res.find("Argument 'pathspecs' must be an array of strings."), std::string::npos); +} + +TEST(SchemaAndArgsToleranceTest, GitAddRejectsEmptyPathspecsAtRuntime) { + AgentConfig config; + config.workspace_abs = "."; + config.allow_mutating_tools = true; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "git_add"; + tc.arguments = {{"pathspecs", json::array()}}; + + const std::string res = execute_tool(tc, config); + EXPECT_NE(res.find("must contain at least one pathspec"), std::string::npos); +} + +TEST(SchemaAndArgsToleranceTest, GitCommitRejectsEmptyMessageAtRuntime) { + AgentConfig config; + config.workspace_abs = "."; + config.allow_mutating_tools = true; + config.allow_execution_tools = true; + + ToolCall tc; + tc.name = "git_commit"; + tc.arguments = {{"message", ""}}; + + const std::string res = execute_tool(tc, config); + EXPECT_NE(res.find("Argument 'message' for git_commit must not be empty."), std::string::npos); +} + TEST(SchemaAndArgsToleranceTest, GitDiffClampsHugeContextLines) { AgentConfig config; const std::string ws = diff --git a/tests/test_tool_registry.cpp b/tests/test_tool_registry.cpp index 85d3cb5..05c37a1 100644 --- a/tests/test_tool_registry.cpp +++ b/tests/test_tool_registry.cpp @@ -4,23 +4,33 @@ #include -TEST(ToolRegistryTest, RegistersAndFindsToolsByName) { - ToolRegistry registry; +namespace { +ToolDescriptor make_descriptor(const std::string& name, + ToolCategory category, + bool mutates_repository_state, + bool can_execute_repo_controlled_code, + bool requires_approval = true) { ToolDescriptor descriptor; - descriptor.name = "alpha"; - descriptor.description = "alpha tool"; - descriptor.category = ToolCategory::ReadOnly; - descriptor.requires_approval = false; - descriptor.json_schema = { - {"type", "object"}, - {"properties", nlohmann::json::object()}, - {"required", nlohmann::json::array()} - }; - descriptor.max_output_bytes = 128; + descriptor.name = name; + descriptor.description = name + " tool"; + descriptor.category = category; + descriptor.mutates_repository_state = mutates_repository_state; + descriptor.can_execute_repo_controlled_code = can_execute_repo_controlled_code; + descriptor.requires_approval = requires_approval; + descriptor.json_schema = {{"type", "object"}}; descriptor.execute = [](const ToolCall&, const AgentConfig&, size_t) { return nlohmann::json{{"ok", true}}; }; + return descriptor; +} + +} // namespace + +TEST(ToolRegistryTest, RegistersAndFindsToolsByName) { + ToolRegistry registry; + ToolDescriptor descriptor = make_descriptor("alpha", ToolCategory::ReadOnly, false, false, false); + descriptor.max_output_bytes = 128; std::string err; EXPECT_TRUE(registry.register_tool(descriptor, &err)) << err; @@ -30,23 +40,14 @@ TEST(ToolRegistryTest, RegistersAndFindsToolsByName) { EXPECT_EQ(found->name, "alpha"); EXPECT_EQ(found->description, "alpha tool"); EXPECT_EQ(found->category, ToolCategory::ReadOnly); + EXPECT_FALSE(found->mutates_repository_state); + EXPECT_FALSE(found->can_execute_repo_controlled_code); EXPECT_FALSE(found->requires_approval); } TEST(ToolRegistryTest, RejectsDuplicateRegistration) { ToolRegistry registry; - - ToolDescriptor descriptor; - descriptor.name = "duplicate"; - descriptor.description = "tool"; - descriptor.json_schema = { - {"type", "object"}, - {"properties", nlohmann::json::object()}, - {"required", nlohmann::json::array()} - }; - descriptor.execute = [](const ToolCall&, const AgentConfig&, size_t) { - return nlohmann::json{{"ok", true}}; - }; + ToolDescriptor descriptor = make_descriptor("duplicate", ToolCategory::ReadOnly, false, false, false); std::string err; EXPECT_TRUE(registry.register_tool(descriptor, &err)) << err; @@ -56,22 +57,8 @@ TEST(ToolRegistryTest, RejectsDuplicateRegistration) { TEST(ToolRegistryTest, SchemaPreservesRegistrationOrder) { ToolRegistry registry; - - ToolDescriptor first; - first.name = "first"; - first.description = "first tool"; - first.json_schema = { - {"type", "object"}, - {"properties", nlohmann::json::object()}, - {"required", nlohmann::json::array()} - }; - first.execute = [](const ToolCall&, const AgentConfig&, size_t) { - return nlohmann::json{{"ok", true}}; - }; - - ToolDescriptor second = first; - second.name = "second"; - second.description = "second tool"; + ToolDescriptor first = make_descriptor("first", ToolCategory::ReadOnly, false, false, false); + ToolDescriptor second = make_descriptor("second", ToolCategory::ReadOnly, false, false, false); std::string err; EXPECT_TRUE(registry.register_tool(first, &err)) << err; @@ -88,12 +75,7 @@ TEST(ToolRegistryTest, ReadOnlyToolExecutesWithoutApproval) { ToolRegistry registry; bool called = false; - ToolDescriptor descriptor; - descriptor.name = "read"; - descriptor.description = "read tool"; - descriptor.category = ToolCategory::ReadOnly; - descriptor.requires_approval = false; - descriptor.json_schema = {{"type", "object"}}; + ToolDescriptor descriptor = make_descriptor("read", ToolCategory::ReadOnly, false, false, false); descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { called = true; return nlohmann::json{{"ok", true}, {"value", 1}}; @@ -111,18 +93,9 @@ TEST(ToolRegistryTest, ReadOnlyToolExecutesWithoutApproval) { EXPECT_TRUE(result["ok"].get()); } -TEST(ToolRegistryTest, ReadOnlyToolApprovalFlagIsNormalizedOffAtRegistration) { +TEST(ToolRegistryTest, RiskFreeToolApprovalFlagIsNormalizedOffAtRegistration) { ToolRegistry registry; - - ToolDescriptor descriptor; - descriptor.name = "read"; - descriptor.description = "read tool"; - descriptor.category = ToolCategory::ReadOnly; - descriptor.requires_approval = true; - descriptor.json_schema = {{"type", "object"}}; - descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { - return nlohmann::json{{"ok", true}}; - }; + ToolDescriptor descriptor = make_descriptor("read", ToolCategory::ReadOnly, false, false, true); std::string err; ASSERT_TRUE(registry.register_tool(descriptor, &err)) << err; @@ -133,16 +106,11 @@ TEST(ToolRegistryTest, ReadOnlyToolApprovalFlagIsNormalizedOffAtRegistration) { EXPECT_FALSE(stored->requires_approval); } -TEST(ToolRegistryTest, MutatingToolBlockedWithoutApproval) { +TEST(ToolRegistryTest, MutatingOnlyToolBlockedWithoutApproval) { ToolRegistry registry; bool called = false; - ToolDescriptor descriptor; - descriptor.name = "write"; - descriptor.description = "write tool"; - descriptor.category = ToolCategory::Mutating; - descriptor.requires_approval = true; - descriptor.json_schema = {{"type", "object"}}; + ToolDescriptor descriptor = make_descriptor("write", ToolCategory::Mutating, true, false); descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { called = true; return nlohmann::json{{"ok", true}}; @@ -160,21 +128,17 @@ TEST(ToolRegistryTest, MutatingToolBlockedWithoutApproval) { EXPECT_FALSE(result["ok"].get()); EXPECT_EQ(result["status"], "blocked"); EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_FALSE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], nlohmann::json::array({"mutating"})); EXPECT_NE(result["error"].get().find("allow_mutating_tools"), std::string::npos); - EXPECT_NE(result["error"].get().find("NCA_ALLOW_MUTATING_TOOLS"), std::string::npos); - EXPECT_NE(result["error"].get().find("--allow-mutating-tools"), std::string::npos); } -TEST(ToolRegistryTest, ExecutionToolBlockedWithoutApproval) { +TEST(ToolRegistryTest, ExecutionOnlyToolBlockedWithoutApproval) { ToolRegistry registry; bool called = false; - ToolDescriptor descriptor; - descriptor.name = "exec"; - descriptor.description = "exec tool"; - descriptor.category = ToolCategory::Execution; - descriptor.requires_approval = true; - descriptor.json_schema = {{"type", "object"}}; + ToolDescriptor descriptor = make_descriptor("exec", ToolCategory::Execution, false, true); descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { called = true; return nlohmann::json{{"ok", true}}; @@ -192,21 +156,94 @@ TEST(ToolRegistryTest, ExecutionToolBlockedWithoutApproval) { EXPECT_FALSE(result["ok"].get()); EXPECT_EQ(result["status"], "blocked"); EXPECT_EQ(result["category"], "execution"); + EXPECT_FALSE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], nlohmann::json::array({"execution"})); EXPECT_NE(result["error"].get().find("allow_execution_tools"), std::string::npos); - EXPECT_NE(result["error"].get().find("NCA_ALLOW_EXECUTION_TOOLS"), std::string::npos); - EXPECT_NE(result["error"].get().find("--allow-execution-tools"), std::string::npos); } -TEST(ToolRegistryTest, MutatingToolExecutesWhenApproved) { +TEST(ToolRegistryTest, DualRiskToolBlockedWhenBothApprovalsAreMissing) { ToolRegistry registry; bool called = false; - ToolDescriptor descriptor; - descriptor.name = "write"; - descriptor.description = "write tool"; - descriptor.category = ToolCategory::Mutating; - descriptor.requires_approval = true; - descriptor.json_schema = {{"type", "object"}}; + ToolDescriptor descriptor = make_descriptor("git_commit", ToolCategory::Mutating, true, true); + descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { + called = true; + return nlohmann::json{{"ok", true}}; + }; + + std::string err; + ASSERT_TRUE(registry.register_tool(descriptor, &err)) << err; + + ToolCall tc; + tc.name = "git_commit"; + AgentConfig config; + + const auto result = nlohmann::json::parse(registry.execute(tc, config)); + EXPECT_FALSE(called); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["status"], "blocked"); + EXPECT_EQ(result["category"], "mutating"); + EXPECT_TRUE(result["requires_mutating_approval"].get()); + EXPECT_TRUE(result["requires_execution_approval"].get()); + EXPECT_EQ(result["missing_approvals"], nlohmann::json::array({"mutating", "execution"})); + EXPECT_NE(result["error"].get().find("allow_mutating_tools"), std::string::npos); + EXPECT_NE(result["error"].get().find("allow_execution_tools"), std::string::npos); +} + +TEST(ToolRegistryTest, DualRiskToolBlockedWhenExecutionApprovalIsMissing) { + ToolRegistry registry; + bool called = false; + + ToolDescriptor descriptor = make_descriptor("git_commit", ToolCategory::Mutating, true, true); + descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { + called = true; + return nlohmann::json{{"ok", true}}; + }; + + std::string err; + ASSERT_TRUE(registry.register_tool(descriptor, &err)) << err; + + ToolCall tc; + tc.name = "git_commit"; + AgentConfig config; + config.allow_mutating_tools = true; + + const auto result = nlohmann::json::parse(registry.execute(tc, config)); + EXPECT_FALSE(called); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["missing_approvals"], nlohmann::json::array({"execution"})); +} + +TEST(ToolRegistryTest, DualRiskToolBlockedWhenMutatingApprovalIsMissing) { + ToolRegistry registry; + bool called = false; + + ToolDescriptor descriptor = make_descriptor("git_commit", ToolCategory::Mutating, true, true); + descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { + called = true; + return nlohmann::json{{"ok", true}}; + }; + + std::string err; + ASSERT_TRUE(registry.register_tool(descriptor, &err)) << err; + + ToolCall tc; + tc.name = "git_commit"; + AgentConfig config; + config.allow_execution_tools = true; + + const auto result = nlohmann::json::parse(registry.execute(tc, config)); + EXPECT_FALSE(called); + EXPECT_FALSE(result["ok"].get()); + EXPECT_EQ(result["missing_approvals"], nlohmann::json::array({"mutating"})); +} + +TEST(ToolRegistryTest, MutatingOnlyToolExecutesWhenApproved) { + ToolRegistry registry; + bool called = false; + + ToolDescriptor descriptor = make_descriptor("write", ToolCategory::Mutating, true, false); descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { called = true; return nlohmann::json{{"ok", true}}; @@ -225,16 +262,11 @@ TEST(ToolRegistryTest, MutatingToolExecutesWhenApproved) { EXPECT_TRUE(result["ok"].get()); } -TEST(ToolRegistryTest, ExecutionToolExecutesWhenApproved) { +TEST(ToolRegistryTest, ExecutionOnlyToolExecutesWhenApproved) { ToolRegistry registry; bool called = false; - ToolDescriptor descriptor; - descriptor.name = "exec"; - descriptor.description = "exec tool"; - descriptor.category = ToolCategory::Execution; - descriptor.requires_approval = true; - descriptor.json_schema = {{"type", "object"}}; + ToolDescriptor descriptor = make_descriptor("exec", ToolCategory::Execution, false, true); descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { called = true; return nlohmann::json{{"ok", true}}; @@ -253,6 +285,30 @@ TEST(ToolRegistryTest, ExecutionToolExecutesWhenApproved) { EXPECT_TRUE(result["ok"].get()); } +TEST(ToolRegistryTest, DualRiskToolExecutesOnlyWhenBothApprovalsAreEnabled) { + ToolRegistry registry; + bool called = false; + + ToolDescriptor descriptor = make_descriptor("git_commit", ToolCategory::Mutating, true, true); + descriptor.execute = [&](const ToolCall&, const AgentConfig&, size_t) { + called = true; + return nlohmann::json{{"ok", true}}; + }; + + std::string err; + ASSERT_TRUE(registry.register_tool(descriptor, &err)) << err; + + ToolCall tc; + tc.name = "git_commit"; + AgentConfig config; + config.allow_mutating_tools = true; + config.allow_execution_tools = true; + + const auto result = nlohmann::json::parse(registry.execute(tc, config)); + EXPECT_TRUE(called); + EXPECT_TRUE(result["ok"].get()); +} + TEST(ToolRegistryTest, UnregisteredToolStillReturnsRegistryError) { ToolRegistry registry; ToolCall tc;