feat: add git add and commit repo tools#18
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the agent's capabilities by integrating fundamental Git version control operations, Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, enhancing the repository manipulation capabilities. The changes are well-structured, including new helper functions for command execution and error normalization, along with comprehensive tests for the new functionality. My review identified a minor issue in the error handling logic for both new tools, where the output truncation status was not correctly reported in failure scenarios. Overall, this is a solid contribution.
src/repo_tools.cpp
Outdated
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", command.exit_code}, | ||
| {"truncated", false}, |
There was a problem hiding this comment.
The truncated flag is hardcoded to false in this error path. However, the command's output could have been truncated before it exited with an error. To ensure the returned status is accurate, command.truncated should be used here.
| {"truncated", false}, | |
| {"truncated", command.truncated}, |
src/repo_tools.cpp
Outdated
| {"stderr", command.stderr_text}, | ||
| {"exit_code", command.exit_code}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, |
There was a problem hiding this comment.
The truncated flag is hardcoded to false in this error path. However, the command's output could have been truncated before it exited with an error. To ensure the returned status is accurate, command.truncated should be used here. This is consistent with how truncation is handled in other parts of the tool, for example when a rev-parse fails later in this function.
{"truncated![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fa0e0f667
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .name = "git_commit", | ||
| .description = "Creates a git commit from the currently staged index changes.", | ||
| .category = ToolCategory::Mutating, | ||
| .requires_approval = true, |
There was a problem hiding this comment.
git_commit as an execution-capable tool
git_commit is only approval-gated as a mutating tool here, but src/repo_tools.cpp invokes a raw git commit, which executes repository hooks. I verified locally that a .git/hooks/pre-commit script runs through this path. In any repo that carries hooks (or sets core.hooksPath), a caller with allow_mutating_tools=true but allow_execution_tools=false can still run arbitrary shell code, which bypasses the current execution approval boundary.
Useful? React with 👍 / 👎.
| std::vector<std::string> args = {git_binary, "add", "--"}; | ||
| args.insert(args.end(), pathspecs.begin(), pathspecs.end()); | ||
|
|
||
| GitTextCommandResult command = run_git_text_command(git_binary, args, workspace_abs, output_limit); |
There was a problem hiding this comment.
This forwards caller-supplied pathspecs straight to git add without any workspace_abs confinement. When the configured workspace is a subdirectory of a larger repo, Git will happily accept ../... here; I verified with cd sub && git add -- ../root.txt, which stages root.txt from the parent directory. That means this new tool can mutate files outside the agent's configured workspace boundary, unlike the other mutating file tools.
Useful? React with 👍 / 👎.
| "commit", | ||
| "--cleanup=strip", | ||
| "-m", message |
There was a problem hiding this comment.
This commits the entire staged index from workspace_abs, not just files under the configured workspace tree. If the agent is launched in repo/subdir and the user already has ../root.txt staged, running this command from subdir still creates a commit for root.txt (I verified that locally with cd sub && git commit -m init). So even without calling git_add, the new tool can create commits that touch files outside the declared workspace boundary.
Useful? React with 👍 / 👎.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, enhancing the agent's capabilities for repository interaction. The changes are well-implemented with a strong focus on security, including robust path validation to prevent operations outside the workspace and safe command execution. The new tools are accompanied by a comprehensive set of tests covering functionality, edge cases, and security aspects. My review found a minor issue in how stdout and stderr are combined for error reporting, which could lead to a formatting issue in the error message. Overall, this is a high-quality contribution that significantly expands the agent's toolset.
src/repo_tools.cpp
Outdated
|
|
||
| std::string normalize_git_commit_error(const std::string& stdout_text, | ||
| const std::string& stderr_text) { | ||
| const std::string combined = trim_line_endings(stdout_text + "\n" + stderr_text); |
There was a problem hiding this comment.
The current method of combining stdout_text and stderr_text can result in a combined string with a leading newline if stdout_text is empty. This could affect the formatting of the final error message. A more robust approach would be to conditionally add the newline separator.
| const std::string combined = trim_line_endings(stdout_text + "\n" + stderr_text); | |
| const std::string combined = trim_line_endings(stdout_text.empty() ? stderr_text : stdout_text + "\n" + stderr_text); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff2147f096
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/repo_tools.cpp
Outdated
| if (input.rfind(":(", 0) == 0) { | ||
| if (err) *err = "Git pathspec magic is not supported for git_add."; | ||
| return false; |
There was a problem hiding this comment.
When the workspace is a subdirectory of a repository, :/foo is another Git pathspec-magic form that resolves from the repository root. resolve_workspace_relative_git_path() only rejects inputs starting with :(, so git_add() will accept :/outside.txt and stage files outside workspace_abs. That bypasses the workspace boundary this new tool is supposed to enforce for any caller that can choose the pathspec string.
Useful? React with 👍 / 👎.
| "diff", | ||
| "--cached", | ||
| "--name-only", | ||
| "-z", | ||
| "--" |
There was a problem hiding this comment.
This preflight uses git diff --cached --name-only -z, which only reports the destination side of a staged rename/copy. In a subdirectory workspace, a rename like outside.txt -> subdir/inside.txt therefore appears in-scope, the later prefix check passes, and git_commit() records a change to outside.txt anyway. That breaks the new “commit only files inside the workspace” guarantee for cross-boundary renames or copies.
Useful? React with 👍 / 👎.
src/repo_tools.cpp
Outdated
| if (result.killed_early && result.truncated) { | ||
| result.exit_code = 0; | ||
| } |
There was a problem hiding this comment.
If git commit writes more than output_limit to stdout (for example, a verbose failing hook), run_git_text_command() kills the child and then rewrites the signal exit status to 0. In a repository that already has a HEAD, git_commit() will then treat the command as successful and return the old HEAD as commit_sha, even though no new commit was created. A killed/truncated command needs to stay failed here.
Useful? React with 👍 / 👎.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds git_add and git_commit tools. The implementation includes security checks to confine operations to the workspace and handles various git command scenarios. The changes are accompanied by a comprehensive set of tests. My review identified a potential issue in git_add where a command terminated due to excessive output might be incorrectly reported as successful. I have provided a specific comment with a suggested fix for this.
| {"error", command.err.empty() ? "git add failed." : command.err} | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
The git_add function is missing a check to handle cases where the command is killed early due to exceeding the output limit. This could lead to the function incorrectly reporting success. A similar check exists in git_commit which should be replicated here to ensure consistent error handling.
It would also be beneficial to add a test case similar to GitCommitKilledEarlyByOutputLimitIsReportedAsFailure for git_add to cover this scenario.
if (command.killed_early && command.truncated) {
return {
{"ok", false},
{"stdout", command.stdout_text},
{"stderr", command.stderr_text},
{"exit_code", -1},
{"truncated", command.truncated},
{"error", "git add output exceeded the configured limit before the command completed."}
};
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d74bab4d46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/repo_tools.cpp
Outdated
| git_binary, | ||
| "-c", "commit.gpgSign=false", | ||
| "commit", | ||
| "--cleanup=strip", | ||
| "-m", message |
There was a problem hiding this comment.
The staged_paths scan above only validates the index before the commit starts, but this argv still lets Git run hooks. git commit -h says --no-verify bypasses pre-commit and commit-msg, and githooks(5) says prepare-commit-msg is not suppressed by that flag at all. In a repo with a hook that runs git add outside.txt, a commit launched from subdir/ can still include files outside workspace_prefix after the safety check has passed, so the new workspace boundary guarantee is bypassable.
Useful? React with 👍 / 👎.
src/repo_tools.cpp
Outdated
| if (command.killed_early && command.truncated) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, |
There was a problem hiding this comment.
This branch assumes a killed command means no commit was created, but githooks(5) says post-commit runs after a commit is made and cannot affect the outcome. If a noisy post-commit hook pushes stdout over output_limit, run_git_text_command sets killed_early and we return ok=false with an empty commit_sha even though HEAD has already advanced. An agent will likely retry in that repo and create duplicate commits.
Useful? React with 👍 / 👎.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, enhancing the agent's capabilities for version control. The implementation is very thorough, with robust argument validation, security checks to prevent actions outside the workspace, and comprehensive test coverage. The changes are well-structured and significantly improve the agent's functionality.
My review includes a few suggestions to improve consistency in tool categorization, enhance correctness in command execution handling, and refactor some duplicated code for better maintainability. Overall, this is an excellent contribution.
src/repo_tools.cpp
Outdated
| if (result.killed_early && result.truncated) { | ||
| result.exit_code = 0; | ||
| } |
There was a problem hiding this comment.
The function run_git_text_command sets exit_code = 0 if the process was killed early due to output limits. This can mask a real failure and makes the exit code unreliable for callers. The callers (git_add and git_commit) have their own logic to handle this specific scenario, and they have more context to decide whether it's a success or failure. For example, git_add treats this as an error, while git_commit checks if HEAD advanced. Forcing the exit code to 0 here is misleading and removes important information for the caller. The killed_early flag is sufficient for the caller to implement custom logic.
| register_or_throw(®istry, ToolDescriptor{ | ||
| .name = "git_add", | ||
| .description = "Stages explicitly listed git pathspecs into the index.", | ||
| .category = ToolCategory::Mutating, |
There was a problem hiding this comment.
The git_add tool is categorized as ToolCategory::Mutating, while git_commit is ToolCategory::Execution. Both tools execute an external git command and modify the repository state. For consistency with git_commit, bash_execute_safe, and other tools that run external processes, git_add should also be categorized as ToolCategory::Execution. This ensures that its use is governed by the allow_execution_tools configuration, providing a consistent security and approval model for all tools that execute external commands.
| .category = ToolCategory::Mutating, | |
| .category = ToolCategory::Execution, |
src/repo_tools.cpp
Outdated
| std::string folded = normalized; | ||
| std::transform(folded.begin(), folded.end(), folded.begin(), [](unsigned char ch) { | ||
| return static_cast<char>(std::tolower(ch)); | ||
| }); |
There was a problem hiding this comment.
This lowercase conversion logic is duplicated from normalize_git_commit_error and normalize_git_repo_error. To improve code reuse and maintainability, consider extracting this into a helper function within the anonymous namespace, like to_lower(std::string s), and using it in all three places.
src/repo_tools.cpp
Outdated
| if (message.empty()) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", "Argument 'message' for git_commit must not be empty."} | ||
| }; |
There was a problem hiding this comment.
This function has many similar blocks for creating error responses. To reduce repetition and improve readability, you could create a helper function to generate these error JSON objects. A similar approach could be applied to git_add as well.
For example, you could have a helper:
nlohmann::json make_git_commit_error(const std::string& error, int exit_code = -1, bool truncated = false, const std::string& stdout_text = "", const std::string& stderr_text = "") {
return {
{"ok", false},
{"stdout", stdout_text},
{"stderr", stderr_text},
{"exit_code", exit_code},
{"commit_sha", ""},
{"truncated", truncated},
{"error", error}
};
}Then this block could be simplified to:
if (message.empty()) {
return make_git_commit_error("Argument 'message' for git_commit must not be empty.");
}| std::filesystem::remove_all(ws); | ||
| } | ||
|
|
||
| TEST(SchemaAndArgsToleranceTest, GitCommitBlockedWithoutMutatingApprovalHasNoSideEffects) { |
There was a problem hiding this comment.
The name of this test is a bit misleading. Since git_commit is categorized as ToolCategory::Execution, it is blocked by allow_execution_tools=false (which is the default in a blank AgentConfig), not allow_mutating_tools. While the test is correct in its assertion, a more accurate name would prevent confusion, for example GitCommitBlockedWithoutApprovalHasNoSideEffects.
| TEST(SchemaAndArgsToleranceTest, GitCommitBlockedWithoutExecutionApprovalHasNoSideEffects) { | ||
| const auto ws = | ||
| (std::filesystem::temp_directory_path() / | ||
| ("nano_git_commit_exec_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_mutating_tools = true; | ||
| config.allow_execution_tools = false; | ||
|
|
||
| ASSERT_EQ(run_bash("cd '" + ws + "' && git rev-parse HEAD > head_before.txt"), 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<bool>()) << result.dump(); | ||
| EXPECT_EQ(result["status"], "blocked"); | ||
| EXPECT_EQ(result["category"], "execution"); | ||
|
|
||
| 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); | ||
|
|
||
| std::filesystem::remove_all(ws); | ||
| } |
There was a problem hiding this comment.
This test, GitCommitBlockedWithoutExecutionApprovalHasNoSideEffects, appears to be identical to GitCommitRequiresExecutionApprovalEvenIfMutatingAllowed (lines 444-472). They both set up the same configuration (allow_mutating_tools = true, allow_execution_tools = false) and test that git_commit is blocked. Consider removing one of them to avoid test duplication.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5927460d8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/agent_tools.cpp
Outdated
| register_or_throw(®istry, ToolDescriptor{ | ||
| .name = "git_commit", | ||
| .description = "Creates a git commit from the currently staged index changes.", | ||
| .category = ToolCategory::Execution, |
There was a problem hiding this comment.
git_commit behind mutating approval
git_commit writes repository state, but registering it as ToolCategory::Execution means ToolRegistry::tool_execution_allowed() only checks allow_execution_tools. In deployments that enable execution tools for build/test commands while intentionally keeping mutating tools disabled, this new tool can still create commits and bypass the mutation brake entirely. Because the category model is single-valued today, this needs to stay under the mutating gate (or explicitly require both approvals) to preserve the existing safety policy.
Useful? React with 👍 / 👎.
src/repo_tools.cpp
Outdated
|
|
||
| std::vector<std::string> args = { | ||
| git_binary, | ||
| "-c", "commit.gpgSign=false", |
There was a problem hiding this comment.
For every commit, this now forces commit.gpgSign=false, which changes the behavior of repositories that rely on signed commits. The git-commit docs describe commit.gpgSign/--no-gpg-sign as overriding a configuration that forces commits to be signed, so in repos with signing enabled or enforced downstream this tool will silently create unsigned commits that later fail push/merge policy checks. That only affects signing-enabled setups, but there it produces commits that are invalid for the repo’s normal workflow.
Useful? React with 👍 / 👎.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, enhancing the agent's capabilities for git operations. The implementation is robust, with a strong focus on security, particularly around path validation and workspace boundaries. The accompanying tests are comprehensive and cover a wide range of scenarios, including edge cases and failure modes. My feedback includes a few suggestions to reduce code duplication and improve consistency in argument validation. Overall, this is a well-executed feature addition.
src/agent_tools.cpp
Outdated
| if (pathspecs.empty()) { | ||
| throw std::runtime_error("Argument 'pathspecs' for git_add must contain at least one pathspec."); | ||
| } |
There was a problem hiding this comment.
This check for empty pathspecs is redundant because the git_add implementation in src/repo_tools.cpp already performs this validation and returns a structured JSON error. To keep validation centralized and avoid duplication, this check can be removed. The corresponding test in test_schema_and_args_tolerance.cpp would need a minor adjustment to check for the JSON error instead of the runtime error message.
src/agent_tools.cpp
Outdated
| }, {"message"}), | ||
| .max_output_bytes = kMaxRepoOutputBytes, | ||
| .execute = [](const ToolCall& cmd, const AgentConfig& config, size_t output_limit) { | ||
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
There was a problem hiding this comment.
The git_commit implementation in src/repo_tools.cpp already validates that the message is not empty. Using require_non_empty_string_arg here creates a redundant check. For consistency and to centralize validation, consider using require_string_arg and letting the git_commit function handle the empty check.
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_string_arg(cmd, "message", "git_commit"); |
src/repo_tools.cpp
Outdated
| std::string folded = normalized; | ||
| std::transform(folded.begin(), folded.end(), folded.begin(), [](unsigned char ch) { | ||
| return static_cast<char>(std::tolower(ch)); | ||
| }); |
There was a problem hiding this comment.
This block of code duplicates the functionality of the ascii_lower helper function, which was introduced earlier in this file. To improve code reuse and maintainability, you can replace these lines with a single call to ascii_lower.
| std::string folded = normalized; | |
| std::transform(folded.begin(), folded.end(), folded.begin(), [](unsigned char ch) { | |
| return static_cast<char>(std::tolower(ch)); | |
| }); | |
| const std::string folded = ascii_lower(normalized); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab94f3c53e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/repo_tools.cpp
Outdated
| if (input.rfind(":(", 0) == 0) { | ||
| if (err) *err = "Git pathspec magic is not supported for git_add."; | ||
| return false; | ||
| } | ||
| if (input.rfind(":/", 0) == 0) { |
There was a problem hiding this comment.
resolve_workspace_relative_git_path() only rejects the long-form magic prefixes :( and :/, but Git treats any leading : as special pathspec syntax. Inputs like :!inside.txt, :^inside.txt, or bare : therefore pass validation and are interpreted by git add -- <pathspec> as exclude/no-pathspec magic instead of literal files. In a nested workspace this lets git_add stage repo-root files outside the workspace (for example, :!inside.txt from subdir/ stages unrelated root files), which breaks the tool's explicit-path/workspace-only guarantee and can flow straight into git_commit.
Useful? React with 👍 / 👎.
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, significantly enhancing the agent's capabilities for interacting with git repositories. The implementation is robust, with strong security measures like path validation to prevent access outside the workspace and disabling git hooks to avoid unintended side effects. The test coverage is excellent, with thorough unit and integration tests for the new functionality, including various edge cases and failure modes. I have a couple of minor suggestions to improve consistency in argument validation.
| }, {"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"); |
There was a problem hiding this comment.
For consistency and to fail earlier, you should use the require_non_empty_string_arg helper function that was introduced in this same pull request. This ensures that the validation for a non-empty commit message happens at the argument parsing layer, which is a better practice than relying on the downstream git_commit function to perform this check.
| const std::string message = require_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
src/repo_tools.cpp
Outdated
| std::string* normalized_rel, | ||
| std::string* err) { | ||
| if (input.empty()) { | ||
| if (err) *err = "Argument 'pathspecs' for git_add must not contain empty paths."; |
There was a problem hiding this comment.
The error message here could be more precise. It's triggered when an individual path string within the pathspecs array is empty, but the message says pathspecs must not contain empty paths, which could be misinterpreted as the array itself being empty. A clearer message would be: "Argument 'pathspecs' for git_add must not contain empty path strings."
| if (err) *err = "Argument 'pathspecs' for git_add must not contain empty paths."; | |
| if (err) *err = "Argument 'pathspecs' for git_add must not contain empty path strings."; |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, enhancing the agent's capabilities to interact with git repositories. The changes include new helper functions for running git commands, parsing their output, and ensuring operations are securely scoped to the workspace. The accompanying tests are thorough, covering a wide range of scenarios including security checks and edge cases. My review identified a couple of issues with prefix checking in helper functions due to incorrect use of rfind, and a minor consistency improvement in argument validation.
src/repo_tools.cpp
Outdated
| if (prefix.empty()) { | ||
| return true; | ||
| } | ||
| return path == prefix || path.rfind(prefix + "/", 0) == 0; |
There was a problem hiding this comment.
The use of rfind with a starting position of 0 is likely a bug. path.rfind(prefix + "/", 0) will only search for the substring within the first character of path, which will almost always fail. To check if path starts with prefix + "/", you should use path.find(prefix + "/", 0) == 0. If your project uses C++20, path.starts_with(prefix + "/") would be even more idiomatic.
| return path == prefix || path.rfind(prefix + "/", 0) == 0; | |
| return path == prefix || path.find(prefix + "/", 0) == 0; |
src/repo_tools.cpp
Outdated
| if (err) *err = "Current workspace is not inside the git repository root."; | ||
| return false; | ||
| } | ||
| if (relative_text == ".." || relative_text.rfind("../", 0) == 0) { |
There was a problem hiding this comment.
Similar to the issue in starts_with_path_component, the use of rfind here to check for a prefix is incorrect. It should be replaced with find to correctly check if relative_text starts with "../".
| if (relative_text == ".." || relative_text.rfind("../", 0) == 0) { | |
| if (relative_text == ".." || relative_text.find("../", 0) == 0) { |
| }, {"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"); |
There was a problem hiding this comment.
For consistency and to perform argument validation at the earliest opportunity, consider using the require_non_empty_string_arg helper function you've added in this PR. While git_commit also checks for an empty message, using the specific helper here makes the argument requirements clearer at the tool registration level.
| const std::string message = require_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit as new agent tools, which is a great feature enhancement. The implementation is robust, with a strong focus on security, including path validation to prevent directory traversal and disabling git hooks when operating in subdirectories. The addition of comprehensive unit tests covering various edge cases, security aspects, and argument validation is commendable. Overall, this is a high-quality contribution. I have one minor suggestion to improve code consistency.
| }, {"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"); |
There was a problem hiding this comment.
For consistency and to fail earlier, please use the newly introduced require_non_empty_string_arg helper function here to validate the message argument. This ensures that an empty commit message is rejected at the argument parsing stage.
| const std::string message = require_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b4ddf62f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| register_or_throw(®istry, ToolDescriptor{ | ||
| .name = "git_add", | ||
| .description = "Stages explicitly listed git pathspecs into the index.", | ||
| .category = ToolCategory::Mutating, |
There was a problem hiding this comment.
Registering git_add as ToolCategory::Mutating means src/tool_registry.cpp:53-59 will allow it whenever allow_mutating_tools is true, even if execution tools are blocked. That is an execution-policy bypass: git_add() shells out to git add, and the repo already exercises repo-defined clean filters in tests/test_repo_tools.cpp:658-680; the official gitattributes docs also say clean/process filters run on check-in (https://git-scm.com/docs/gitattributes.html). In an untrusted repo, staging a file can therefore run arbitrary commands despite allow_execution_tools=false.
Useful? React with 👍 / 👎.
| register_or_throw(®istry, ToolDescriptor{ | ||
| .name = "git_commit", | ||
| .description = "Creates a git commit from the currently staged index changes.", | ||
| .category = ToolCategory::Mutating, |
There was a problem hiding this comment.
Marking git_commit as mutating-only bypasses the separate execution gate for repo-controlled hooks and signing helpers. src/tool_registry.cpp:53-59 checks only allow_mutating_tools for this category, tests/test_schema_and_args_tolerance.cpp:474-505 even asserts git_commit succeeds with allow_execution_tools=false, and git commit is documented to run commit hooks (https://git-scm.com/docs/git-commit.html#_hooks); tests/test_repo_tools.cpp:851-863 also shows a repo-configured gpg.program is invoked. A malicious repository can therefore execute commands via pre-commit/commit-msg/signing while the policy is supposedly forbidding execution.
Useful? React with 👍 / 👎.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, which is a significant feature enhancement. The implementation is very thorough, with a strong focus on security, such as validating paths to stay within the workspace, handling git hooks safely, and providing clear error messages. The accompanying refactoring of the tool approval mechanism to be more granular is also a great improvement. The test coverage for the new functionality is comprehensive and well-written. I've found a couple of areas for improvement: one is a minor consistency issue in argument parsing, and the other is a potential bug in parsing git's output for renamed/copied files which could have security implications.
| }; | ||
|
|
||
| const char code = status[0]; | ||
| if ((code == 'R' || code == 'C') && status.size() >= 2) { |
There was a problem hiding this comment.
The condition status.size() >= 2 seems incorrect here. When using git diff --name-status -z, the status for renames (R) and copies (C) is just a single character, not followed by a score (e.g., R100). This condition will be false for renames and copies, causing them to be parsed incorrectly, which will lead to an error. This could affect the git_commit tool's ability to correctly validate staged paths.
| if ((code == 'R' || code == 'C') && status.size() >= 2) { | |
| if (code == 'R' || code == 'C') { |
| }, {"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"); |
There was a problem hiding this comment.
For consistency and to centralize argument validation, consider using the require_non_empty_string_arg helper function here, which you've just introduced. The git_commit implementation currently performs its own check for an empty message, but using the helper would make the intent clearer at the argument parsing stage and avoid redundant checks.
| const std::string message = require_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 876b095356
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "commit", | ||
| "--cleanup=strip", | ||
| "-m", message |
There was a problem hiding this comment.
--cleanup=strip from explicit message commits
git commit -h describes --cleanup as stripping spaces and #comments, so forcing that mode here changes or rejects valid message inputs. A subject like #123 fix now aborts with an empty-message error, and any #... lines in the body are silently dropped, even though plain git commit -m ... would preserve them. Because this tool accepts an explicit message string, it should not run an extra cleanup pass that rewrites caller-provided content.
Useful? React with 👍 / 👎.
| if (!workspace_prefix.empty()) { | ||
| args.push_back("-c"); | ||
| args.push_back("core.hooksPath=/dev/null"); | ||
| } |
There was a problem hiding this comment.
When workspace_abs is a subdirectory, this adds -c core.hooksPath=/dev/null, which bypasses every hook, not just index-mutating ones. In repos that enforce commit-msg or prepare-commit-msg policies, git_commit will now create commits from nested workspaces that normal git commit -m ... would reject or rewrite, so the new tool silently sidesteps repository commit validation in exactly the subdir case it is trying to support.
Useful? React with 👍 / 👎.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
The pull request introduces new git_add and git_commit tools, enhancing the agent's Git repository management capabilities. It also refines the tool approval mechanism by adding mutates_repository_state and can_execute_repo_controlled_code flags to ToolDescriptor, enabling more granular control over tool execution based on their potential impact. The review comments suggest improvements such as simplifying redundant stderr_truncated flag checks, clarifying conditional logic for error message generation, and making the requires_approval check more explicit during tool registration.
| if (keep < static_cast<size_t>(n) && stderr_truncated) { | ||
| *stderr_truncated = true; | ||
| } | ||
| } else if (stderr_truncated) { | ||
| *stderr_truncated = true; | ||
| } |
There was a problem hiding this comment.
medium: The stderr_truncated flag is checked twice. Consider simplifying the logic to avoid redundancy.
if (stderr_text && stderr_text->size() < kMaxCommandStderrBytes) {
const size_t keep = std::min(static_cast<size_t>(n), kMaxCommandStderrBytes - stderr_text->size());
stderr_text->append(buffer, keep);
if (keep < static_cast<size_t>(n)) { // Check if truncation occurred during append
*stderr_truncated = true;
}
} else if (stderr_truncated) {
*stderr_truncated = true;
}| 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."; |
There was a problem hiding this comment.
medium: The conditional logic for setting the error message is a bit complex. Consider simplifying it by using a helper function or a more straightforward conditional structure.
std::string error = "Tool execution requires approval under the current policy.";
if (requires_mutating && requires_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) {
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) {
error =
"Execution tool use is blocked by default. Enable allow_execution_tools, set "
"NCA_ALLOW_EXECUTION_TOOLS=1, or pass --allow-execution-tools.";
}| if (!descriptor.mutates_repository_state && !descriptor.can_execute_repo_controlled_code) { | ||
| descriptor.requires_approval = false; | ||
| } |
There was a problem hiding this comment.
medium: This check could be more explicit by checking descriptor.category != ToolCategory::Mutating && descriptor.category != ToolCategory::Execution.
if (descriptor.category != ToolCategory::Mutating && descriptor.category != ToolCategory::Execution) {
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cd3e5447d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| args.insert(args.end(), { | ||
| "commit", | ||
| "-m", message |
There was a problem hiding this comment.
git_commit() builds the command as git commit -m <message> here, which means Git still applies the repository's commit.cleanup and comment-char rules to the caller-supplied text. In repos that set commit.cleanup=strip (or customize core.commentChar), lines in the provided message that begin with that comment character are silently removed, so the new tool can create a different commit message than the agent requested.
Useful? React with 👍 / 👎.
| if (input.front() == ':') { | ||
| if (err) *err = "Git pathspec magic is not supported for git_add."; | ||
| return false; |
There was a problem hiding this comment.
Rejecting every path whose first character is : makes the wrapper unable to stage legal POSIX filenames like :foo.txt. Git only accepts those through a literal pathspec such as :(literal):foo.txt, so with this check in place any repository containing a colon-prefixed file becomes impossible to stage through git_add.
Useful? React with 👍 / 👎.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| throw std::runtime_error("Argument '" + std::string(key) + "' for " + tool_name + " must not be empty."); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
New helper function is defined but never called
Low Severity
require_non_empty_string_arg is added in this PR but never called anywhere in the codebase. The git_commit executor at line 544 uses require_string_arg instead, relying on the git_commit() function in repo_tools.cpp to perform its own empty-message check. This is dead code that adds maintenance burden without providing value.
Additional Locations (1)
|
/cursor review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f996d6f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/agent_tools.cpp
Outdated
| .mutates_repository_state = false, | ||
| .can_execute_repo_controlled_code = true, |
There was a problem hiding this comment.
Under the new mutates_repository_state/can_execute_repo_controlled_code split, ToolRegistry only asks for allow_mutating_tools when mutates_repository_state is true (src/tool_registry.cpp:15-23). Marking bash_execute_safe as non-mutating means a config with allow_execution_tools=true and allow_mutating_tools=false can still run rm, git checkout, or overwrite tracked files through this tool; the same metadata is used for build_project_safe and test_project_safe below. That effectively bypasses the mutating approval bit for the tools most capable of changing the repository.
Useful? React with 👍 / 👎.
src/repo_tools.cpp
Outdated
| std::string head_after; | ||
| std::string head_after_err; | ||
| if (!resolve_head_sha(git_binary, workspace_abs, &head_after, &head_after_err)) { |
There was a problem hiding this comment.
git_commit derives both head_advanced and the returned commit_sha by calling rev-parse HEAD after git commit exits. Per githooks, post-commit runs after a commit is made, so a repo with a post-commit hook that amends, creates a follow-up commit, or checks out another ref will change HEAD before this read happens. In that case the tool returns the wrong SHA, or can even report git commit did not create a new HEAD commit although the requested commit was created.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable new git_add and git_commit tools, significantly enhancing the agent's capabilities for repository manipulation. The refactoring of the tool approval policy to be more granular with mutates_repository_state and can_execute_repo_controlled_code flags is a great improvement for security and control. The accompanying tests are thorough and cover many important edge cases and security considerations.
I have a couple of suggestions to improve consistency and maintainability. One is to use a newly added helper for argument validation in the git_commit tool registration. The other is to consider refactoring the very long git_commit implementation into smaller, more focused functions.
| }, {"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"); |
There was a problem hiding this comment.
For consistency and to fail early, consider using the new require_non_empty_string_arg helper function here. This function was added in this PR and seems perfect for ensuring the commit message is not empty at the argument parsing stage, rather than inside the git_commit implementation.
| const std::string message = require_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
| nlohmann::json git_commit(const std::string& workspace_abs, | ||
| const std::string& message, | ||
| size_t output_limit) { | ||
| if (message.empty()) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", "Argument 'message' for git_commit must not be empty."} | ||
| }; | ||
| } | ||
|
|
||
| const std::string git_binary = find_executable_in_path("git"); | ||
| if (git_binary.empty()) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", "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 { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", repo_err.empty() ? "Failed to resolve git repository root." : repo_err} | ||
| }; | ||
| } | ||
|
|
||
| std::vector<std::string> 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 { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", trim_line_endings(staged_stderr)}, | ||
| {"exit_code", staged_exit_code}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", 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 { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", trim_line_endings(staged_stderr)}, | ||
| {"exit_code", staged_exit_code}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", 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 { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", "Refusing to commit staged path outside the workspace: " + staged_path} | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| std::string head_before; | ||
| std::string head_before_err; | ||
| if (!resolve_head_sha(git_binary, workspace_abs, &head_before, &head_before_err)) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", head_before_err.empty() ? "Failed to resolve HEAD before git commit." : head_before_err} | ||
| }; | ||
| } | ||
|
|
||
| std::vector<std::string> reflog_before; | ||
| std::string reflog_before_err; | ||
| if (!list_head_reflog_shas(git_binary, workspace_abs, &reflog_before, &reflog_before_err)) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", ""}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", false}, | ||
| {"error", reflog_before_err.empty() ? "Failed to inspect HEAD reflog before git commit." : reflog_before_err} | ||
| }; | ||
| } | ||
|
|
||
| std::vector<std::string> 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 { | ||
| {"ok", false}, | ||
| {"stdout", ""}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", command.exit_code}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", command.err.empty() ? "git commit failed." : command.err} | ||
| }; | ||
| } | ||
|
|
||
| std::string head_after; | ||
| std::string head_after_err; | ||
| if (!resolve_head_sha(git_binary, workspace_abs, &head_after, &head_after_err)) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", head_after_err.empty() ? "Failed to resolve HEAD after git commit." : head_after_err} | ||
| }; | ||
| } | ||
|
|
||
| std::vector<std::string> reflog_after; | ||
| std::string reflog_after_err; | ||
| if (!list_head_reflog_shas(git_binary, workspace_abs, &reflog_after, &reflog_after_err)) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", reflog_after_err.empty() ? "Failed to inspect HEAD reflog after git commit." : reflog_after_err} | ||
| }; | ||
| } | ||
|
|
||
| std::string created_commit_sha; | ||
| std::string created_commit_err; | ||
| if (!resolve_created_commit_sha(git_binary, | ||
| workspace_abs, | ||
| head_before, | ||
| head_after, | ||
| reflog_before, | ||
| reflog_after, | ||
| &created_commit_sha, | ||
| &created_commit_err)) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", 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 { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", "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 { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", command.exit_code}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", failure.empty() ? "git commit exited with code " + std::to_string(command.exit_code) : failure} | ||
| }; | ||
| } | ||
|
|
||
| if (!head_advanced) { | ||
| return { | ||
| {"ok", false}, | ||
| {"stdout", command.stdout_text}, | ||
| {"stderr", command.stderr_text}, | ||
| {"exit_code", -1}, | ||
| {"commit_sha", ""}, | ||
| {"truncated", command.truncated}, | ||
| {"error", "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", ""} | ||
| }; | ||
| } |
There was a problem hiding this comment.
The git_commit function is quite long (over 200 lines) and has many responsibilities, such as argument validation, pre-commit checks, command execution, and post-commit processing. To improve readability and maintainability, consider refactoring it into several smaller, more focused helper functions. For example, you could have separate functions for:
- Pre-commit validation (checking staged files, repo state).
- Capturing state before and after the commit.
- Executing the
git commitcommand. - Resolving the created commit SHA and handling results.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces git_add and git_commit tools, along with a significant and well-executed refactoring of the tool approval mechanism to be more granular. The changes are extensive, with a strong focus on security and robustness, demonstrated by comprehensive new logic for path validation, hook management, and commit state verification. The accompanying tests are exceptionally thorough, covering a wide range of functional, security, and edge-case scenarios. My review includes a couple of minor suggestions to improve code consistency and reduce boilerplate, but overall this is a high-quality contribution.
| }, {"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"); |
There was a problem hiding this comment.
For consistency with the newly introduced require_non_empty_string_arg function and to enforce argument validation at the earliest point, consider using it here instead of require_string_arg. The git_commit implementation in repo_tools.cpp also performs this check, but centralizing this validation in the argument parsing layer improves maintainability.
| const std::string message = require_string_arg(cmd, "message", "git_commit"); | |
| const std::string message = require_non_empty_string_arg(cmd, "message", "git_commit"); |
| nlohmann::json git_add(const std::string& workspace_abs, | ||
| const std::vector<std::string>& pathspecs, | ||
| size_t output_limit) { |
There was a problem hiding this comment.
This function, as well as git_commit, contains significant boilerplate for constructing JSON responses, especially for error cases. This repetition makes the functions long and harder to maintain.
To improve this, you could introduce a helper function to generate these JSON responses. For example:
namespace {
nlohmann::json make_git_tool_error(const std::string& error,
int exit_code = -1,
bool truncated = false,
const std::string& stdout_text = "",
const std::string& stderr_text = "",
bool for_commit = false) {
nlohmann::json res = {
{"ok", false},
{"stdout", stdout_text},
{"stderr", stderr_text},
{"exit_code", exit_code},
{"truncated", truncated},
{"error", error}
};
if (for_commit) {
res["commit_sha"] = "";
}
return res;
}
} // namespaceUsing this helper would significantly reduce code duplication in both git_add and git_commit, making them more concise and readable.
- refactor(tool_registry): add explicit mutation and repo-controlled execution risk flags to tool descriptors - refactor(tool_registry): gate tool execution by combined approval requirements instead of single-category branching - feat(tool_registry): return blocked approval metadata for mutation, execution, and combined missing approvals while keeping category as a compatibility label - test(tool_registry): rewrite approval policy coverage around capability combinations instead of single category assumptions
- feat(repo_tools): add bounded git_add and git_commit helpers with workspace confinement and structured results - fix(repo_tools): harden git_add path validation, output-limit failures, and explicit path-only staging behavior - fix(repo_tools): harden git_commit staged-path checks, hook-aware commit_sha detection, and explicit message handling - refactor(repo_tools): extract small local helpers for git_commit state observation and git_add/git_commit error result construction - refactor(agent_tools): register git_add and git_commit and align tool risk metadata with the current approval model - test(repo_tools): cover git_add and git_commit success paths, confinement checks, hook behavior, truncation handling, and message preservation - test(agent_tools): expand schema and integration coverage for git_add and git_commit dispatch and approval behavior - test(build): align build, test, and agent loop fixtures with the current dual-risk approval settings
LeeLaugh-jason
force-pushed
the
gitaddcom
branch
from
efb6bed to
ecebc5e
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
This closes the remaining Phase 1 packaging-flow gap by adding explicit
git_addandgit_committools on top of the existing repo tools stack.What Changed
git_addhelper with explicit--pathspec separation and structured git outputgit_commithelper for staged-index-only commits with returnedcommit_shaagent_toolsas mutating, approval-gated tools with minimal schemasTesting
ASAN_OPTIONS=detect_leaks=0 LSAN_OPTIONS=detect_leaks=0 ./build.sh test --filter test_repo_tools,test_schema_and_args_toleranceNote
Medium Risk
Adds new approval-gated tools that can stage and commit changes and refactors policy enforcement to consider both mutation and repo-controlled execution risk. While guarded and well-tested, it expands the tool surface area that can change repository state and run git hooks/filters if misclassified.
Overview
Adds new
git_addandgit_committools to the agent tool registry, enabling staging explicit pathspecs and creating commits from the staged index while returning structured results (includingcommit_sha).Refines tool safety metadata by splitting approval requirements into
mutates_repository_statevscan_execute_repo_controlled_code, updates registry blocking responses to report missing approvals, and extends repo command execution plumbing to better track truncation/killed-early scenarios. Comprehensive tests cover schema updates, approval blocking/no-side-effects, pathspec validation/escape prevention, and commit safety checks (workspace-bound staged paths, hooks behavior, and output-limit handling).Written by Cursor Bugbot for commit 5cd3e54. This will update automatically on new commits. Configure here.