Add approval gate for file mutations - #62
Conversation
![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 a file edit policy gate to control file mutations independently of shell execution, adding new configuration options, a MutationPreview struct, and updating the tool execution pipeline to request user approval for file edits. The reviewer feedback highlights several important improvements: bypassing the sub-agent's tool allowlist for system-initiated ask_user calls to prevent execution failures, making the policy block error message context-aware for unattended sessions, and avoiding redundant user approval prompts by returning early when edit or write operations are no-ops.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let reply = match tools | ||
| .execute_tool_scoped( | ||
| "ask_user", | ||
| ask_payload, | ||
| allow.as_deref(), | ||
| is_subagent, | ||
| ) |
There was a problem hiding this comment.
When a sub-agent is configured with a restricted tool allowlist (e.g., only allowing write_file or edit_file), the internal policy gate's attempt to prompt the user via ask_user will fail if ask_user is not explicitly included in the sub-agent's allowlist. Since this ask_user call is system-initiated and not directly invoked by the sub-agent's LLM, it should bypass the sub-agent's tool allowlist. Passing None instead of allow.as_deref() resolves this issue.
| let reply = match tools | |
| .execute_tool_scoped( | |
| "ask_user", | |
| ask_payload, | |
| allow.as_deref(), | |
| is_subagent, | |
| ) | |
| let reply = match tools | |
| .execute_tool_scoped( | |
| "ask_user", | |
| ask_payload, | |
| None, | |
| is_subagent, | |
| ) |
There was a problem hiding this comment.
Fixed in ae22461. The system-initiated ask_user now passes None so it bypasses the sub-agent allowlist. Applied the same fix to the shell policy gate, which had the identical latent bug (a sub-agent allowlisted to {exec} would have failed the shell approval prompt the same way). Regression test: scoped_tools_tests::system_initiated_ask_user_bypasses_subagent_allowlist.
| ShellPolicyMode::Deny => { | ||
| return ToolExecutionFinished::Completed(Err( | ||
| "File edit blocked by policy: plan mode active — finalize or apply the plan first." | ||
| .to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
The error message hardcodes 'plan mode active — finalize or apply the plan first.' as the reason for the blocked edit. However, under the new unattended edit policy, edits in unattended sessions are blocked by default (Deny). Showing a 'plan mode active' error in a non-plan unattended session can be highly misleading to both users and the agent. Providing a context-aware error message based on runtime.unattended_session improves clarity.
ShellPolicyMode::Deny => {
let err_msg = if runtime.unattended_session {
"File edit blocked by policy: unattended edit mode is active."
} else {
"File edit blocked by policy: plan mode active — finalize or apply the plan first."
};
return ToolExecutionFinished::Completed(Err(err_msg.to_string()));
}There was a problem hiding this comment.
Fixed in ae22461. Extracted edit_policy_block_reason(unattended_session) so the message is context-aware: 'unattended edit mode is active' for unattended default-deny vs 'plan mode active' for interactive plan mode. Unit test: code_exec_gate_tests::edit_block_reason_distinguishes_unattended_and_plan_mode.
| let old_text = args | ||
| .get("old_text") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or("Missing 'old_text' argument")?; | ||
| let new_text = args | ||
| .get("new_text") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or("Missing 'new_text' argument")?; | ||
| let replace_all = args | ||
| .get("replace_all") | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
If old_text and new_text are identical, the edit is a no-op. Currently, preview_mutation will still generate a preview with an empty diff, causing the agent to prompt the user for approval of a no-op edit, only for the tool to eventually return an error when executed. Checking if old_text == new_text early and returning Ok(None) avoids unnecessary user prompts and improves the user experience.
| let old_text = args | |
| .get("old_text") | |
| .and_then(|v| v.as_str()) | |
| .ok_or("Missing 'old_text' argument")?; | |
| let new_text = args | |
| .get("new_text") | |
| .and_then(|v| v.as_str()) | |
| .ok_or("Missing 'new_text' argument")?; | |
| let replace_all = args | |
| .get("replace_all") | |
| .and_then(|v| v.as_bool()) | |
| .unwrap_or(false); | |
| let old_text = args | |
| .get("old_text") | |
| .and_then(|v| v.as_str()) | |
| .ok_or("Missing 'old_text' argument")?; | |
| let new_text = args | |
| .get("new_text") | |
| .and_then(|v| v.as_str()) | |
| .ok_or("Missing 'new_text' argument")?; | |
| if old_text == new_text { | |
| return Ok(None); | |
| } | |
| let replace_all = args | |
| .get("replace_all") | |
| .and_then(|v| v.as_bool()) | |
| .unwrap_or(false); |
There was a problem hiding this comment.
Fixed in ae22461. EditFileTool::preview_mutation now returns Ok(None) when old_text == new_text, skipping the prompt. execute() still surfaces its existing 'identical' error so the model learns the edit was a no-op. Test: mutation_preview_tests::edit_preview_skips_noop_when_old_equals_new.
| let before = match fs::read_to_string(&actual_path) { | ||
| Ok(content) => Some(content), | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, | ||
| Err(error) => return Err(format!("Could not preview write target: {error}")), | ||
| }; |
There was a problem hiding this comment.
If the file already exists and its current content is identical to the new content being written, the write is a no-op. Currently, preview_mutation will still generate a preview with an empty diff, prompting the user to approve a write that doesn't change anything. Checking if the content is already identical and returning Ok(None) avoids unnecessary approval prompts.
let before = match fs::read_to_string(&actual_path) {
Ok(current_content) => {
if current_content == content {
return Ok(None);
}
Some(current_content)
}
Err(error) if error.kind() == std::io::ErrorKind::NotFound => None,
Err(error) => return Err(format!("Could not preview write target: {error}")),
};There was a problem hiding this comment.
Fixed in ae22461. WriteFileTool::preview_mutation returns Ok(None) when the existing file content is identical to the new content. Test: mutation_preview_tests::write_preview_skips_noop_when_content_unchanged.
Implements the merged context, MCP, workspace automation, and agent-control improvements. Includes the edit-approval IsanAgent dependency from altaidevorg/isanagent#62.
- Bypass the sub-agent tool allowlist for system-initiated ask_user prompts
in both the edit and shell policy gates. A restricted sub-agent allowlist
(e.g. {write_file} / {exec}) that omits ask_user previously caused the
approval prompt itself to fail.
- Make the edit-policy Deny message context-aware (unattended vs plan mode)
instead of hardcoding 'plan mode active', which is misleading for the
unattended default-deny.
- Skip the approval prompt for no-op edits (old_text == new_text) and no-op
writes (content identical to the existing file).
- Add regression tests covering each fix.
Addresses altaidevorg#62 review comments.
|
All 4 review comments addressed in
Added 4 regression tests ( |
2 participants
Summary\n- add a central policy gate for write_file and edit_file\n- show a bounded diff through ask_user metadata before an edit runs\n- revalidate the approved file fingerprint immediately before writing\n- add independent interactive and unattended edit policy modes\n\n## Verification\n- cargo test --lib --no-fail-fast\n\nThe ALTAI-side event renderer and permission-mode mapping will follow once this crate API is merged and pinned.