Make plan approval gate non-destructive and baseline-aware

- Remove all file revert/delete logic from check_plan_approval_gate:
  no more git checkout or fs::remove_file calls. The gate only warns.
- Remove reverted_files field from ApprovalGateResult::Blocked.
- Add get_dirty_files() helper to snapshot dirty files as a HashSet.
- Capture baseline dirty files when plan mode starts (set_plan_mode).
  Pre-existing dirty files are excluded from gate checks so they
  never trigger blocking.
- Add 5 new unit tests covering non-destructive behavior, baseline
  exclusion, and mixed baseline/new file scenarios.
- Update integration test to match new non-destructive semantics.
This commit is contained in:
Dhanji R. Prasanna
2026-02-15 09:53:14 +11:00
parent 22b1ab93e4
commit 7347d92ae8
5 changed files with 222 additions and 106 deletions

View File

@@ -1,5 +1,5 @@
# Workspace Memory # Workspace Memory
> Updated: 2026-02-13 | Size: ~20k chars > Updated: 2026-02-14T22:33:04Z | Size: 22.9k chars
### Remember Tool Wiring ### Remember Tool Wiring
- `crates/g3-core/src/tools/memory.rs` [0..5686] - `crates/g3-core/src/tools/memory.rs` [0..5686]
@@ -388,3 +388,12 @@ Tool output responsive to terminal width — no line wrapping, 4-char right marg
- `print_tool_output_header()` [293..410] - uses compress_path/compress_command - `print_tool_output_header()` [293..410] - uses compress_path/compress_command
- `update_tool_output_line()` [407..445], `print_tool_output_line()` [447..454] - clip_line() - `update_tool_output_line()` [407..445], `print_tool_output_line()` [447..454] - clip_line()
- `print_tool_compact()` [475..635] - width-aware compact display - `print_tool_compact()` [475..635] - width-aware compact display
### Plan Approval Gate (Non-Destructive + Baseline-Aware)
- `crates/g3-core/src/tools/plan.rs` [973..983] - `ApprovalGateResult` enum: `Allowed`, `Blocked { message }`, `NotGitRepo` — no `reverted_files` field
- `crates/g3-core/src/tools/plan.rs` [985..1003] - `get_dirty_files()` - returns `HashSet<String>` of dirty file paths from `git status --porcelain`
- `crates/g3-core/src/tools/plan.rs` [1005..1098] - `check_plan_approval_gate(session_id, working_dir, baseline_dirty)` - warn-only, never reverts/deletes files, excludes baseline dirty files
- `crates/g3-core/src/lib.rs` [170..171] - `baseline_dirty_files: HashSet<String>` field on Agent
- `crates/g3-core/src/lib.rs` [1675..1686] - `set_plan_mode(enabled, working_dir)` - captures baseline on enable, clears on disable
- **Key invariant**: The approval gate NEVER deletes or reverts files. It only warns.
- **Key invariant**: Pre-existing dirty files (captured at plan mode start) are excluded from gate checks.

View File

@@ -95,7 +95,7 @@ fn check_and_exit_plan_mode_if_terminal<W: UiWriter>(
if *in_plan_mode && agent.is_plan_terminal() { if *in_plan_mode && agent.is_plan_terminal() {
output.print("\n📋 Plan complete - exiting plan mode"); output.print("\n📋 Plan complete - exiting plan mode");
*in_plan_mode = false; *in_plan_mode = false;
agent.set_plan_mode(false); agent.set_plan_mode(false, None);
return true; return true;
} }
false false
@@ -159,7 +159,7 @@ pub async fn run_interactive<W: UiWriter>(
let mut is_first_plan_message = in_plan_mode; let mut is_first_plan_message = in_plan_mode;
// Sync agent's plan mode state with CLI state // Sync agent's plan mode state with CLI state
agent.set_plan_mode(in_plan_mode); agent.set_plan_mode(in_plan_mode, Some(workspace_path.to_str().unwrap_or(".")));
// Initialize rustyline editor with history // Initialize rustyline editor with history
let config = Config::builder() let config = Config::builder()
@@ -282,7 +282,7 @@ pub async fn run_interactive<W: UiWriter>(
} }
CommandResult::EnterPlanMode => { CommandResult::EnterPlanMode => {
in_plan_mode = true; in_plan_mode = true;
agent.set_plan_mode(true); agent.set_plan_mode(true, Some(workspace_path.to_str().unwrap_or(".")));
is_first_plan_message = true; is_first_plan_message = true;
continue; continue;
} }
@@ -322,7 +322,7 @@ pub async fn run_interactive<W: UiWriter>(
if in_plan_mode { if in_plan_mode {
output.print("CTRL-D (exiting plan mode)"); output.print("CTRL-D (exiting plan mode)");
in_plan_mode = false; in_plan_mode = false;
agent.set_plan_mode(false); agent.set_plan_mode(false, None);
// Continue the loop with normal prompt // Continue the loop with normal prompt
continue; continue;
} else { } else {

View File

@@ -52,7 +52,7 @@ pub use skills::{Skill, discover_skills, generate_skills_prompt};
#[cfg(test)] #[cfg(test)]
mod task_result_comprehensive_tests; mod task_result_comprehensive_tests;
use crate::ui_writer::UiWriter; use crate::ui_writer::UiWriter;
use tools::plan::{check_plan_approval_gate, read_plan, ApprovalGateResult}; use tools::plan::{check_plan_approval_gate, get_dirty_files, read_plan, ApprovalGateResult};
#[cfg(test)] #[cfg(test)]
mod tilde_expansion_tests; mod tilde_expansion_tests;
@@ -166,6 +166,8 @@ pub struct Agent<W: UiWriter> {
acd_enabled: bool, acd_enabled: bool,
/// Whether plan mode is active (gate blocks file changes without approved plan) /// Whether plan mode is active (gate blocks file changes without approved plan)
in_plan_mode: bool, in_plan_mode: bool,
/// Files that were already dirty when plan mode started (excluded from approval gate)
baseline_dirty_files: std::collections::HashSet<String>,
/// Manager for async research tasks /// Manager for async research tasks
pending_research_manager: pending_research::PendingResearchManager, pending_research_manager: pending_research::PendingResearchManager,
/// Set of toolset names that have been loaded in this session /// Set of toolset names that have been loaded in this session
@@ -224,6 +226,7 @@ impl<W: UiWriter> Agent<W> {
auto_memory: false, auto_memory: false,
acd_enabled: false, acd_enabled: false,
in_plan_mode: false, in_plan_mode: false,
baseline_dirty_files: std::collections::HashSet::new(),
pending_research_manager: pending_research::PendingResearchManager::new(), pending_research_manager: pending_research::PendingResearchManager::new(),
loaded_toolsets: std::collections::HashSet::new(), loaded_toolsets: std::collections::HashSet::new(),
} }
@@ -1669,8 +1672,15 @@ impl<W: UiWriter> Agent<W> {
} }
/// Enable or disable plan mode (blocks file changes without approved plan) /// Enable or disable plan mode (blocks file changes without approved plan)
pub fn set_plan_mode(&mut self, enabled: bool) { pub fn set_plan_mode(&mut self, enabled: bool, working_dir: Option<&str>) {
self.in_plan_mode = enabled; self.in_plan_mode = enabled;
if enabled {
// Capture current dirty files as baseline so the approval gate
// won't block on files that were already dirty before plan mode.
self.baseline_dirty_files = get_dirty_files(working_dir);
} else {
self.baseline_dirty_files.clear();
}
} }
/// Check if plan mode is active /// Check if plan mode is active
@@ -3014,8 +3024,8 @@ Skip if nothing new. Be brief."#;
// Check plan approval gate after tool execution (only in plan mode) // Check plan approval gate after tool execution (only in plan mode)
if self.in_plan_mode { if self.in_plan_mode {
if let Some(session_id) = &self.session_id { if let Some(session_id) = &self.session_id {
if let ApprovalGateResult::Blocked { message, .. } = if let ApprovalGateResult::Blocked { message } =
check_plan_approval_gate(session_id, working_dir) check_plan_approval_gate(session_id, working_dir, &self.baseline_dirty_files)
{ {
// Return the blocking message instead of the tool result // Return the blocking message instead of the tool result
return Ok(message); return Ok(message);

View File

@@ -10,6 +10,7 @@
use anyhow::{anyhow, Result}; use anyhow::{anyhow, Result};
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
use std::collections::HashSet;
use std::fmt; use std::fmt;
use std::path::PathBuf; use std::path::PathBuf;
use std::path::Path; use std::path::Path;
@@ -973,138 +974,125 @@ pub async fn execute_plan_approve<W: UiWriter>(
pub enum ApprovalGateResult { pub enum ApprovalGateResult {
/// No plan exists, or plan is approved - allow the operation /// No plan exists, or plan is approved - allow the operation
Allowed, Allowed,
/// Plan exists but not approved, and files were changed - blocked /// Plan exists but not approved, and new files were changed - blocked (warn only, never revert)
Blocked { Blocked {
/// Message to inject into the conversation /// Message to inject into the conversation
message: String, message: String,
/// Files that were reverted
reverted_files: Vec<String>,
}, },
/// Not a git repository - skip the check /// Not a git repository - skip the check
NotGitRepo, NotGitRepo,
} }
/// Check if file changes occurred without an approved plan, and revert them if so. /// Get the set of dirty file paths from `git status --porcelain`.
/// ///
/// This function should be called after each tool execution when in plan mode. /// Returns an empty set if not a git repo or if the command fails.
/// It checks `git status --porcelain` for changes, and if a plan exists but isn't /// Each entry is the file path as reported by git (relative to repo root).
/// approved, it reverts those changes and returns a blocking message. pub fn get_dirty_files(working_dir: Option<&str>) -> HashSet<String> {
pub fn check_plan_approval_gate(session_id: &str, working_dir: Option<&str>) -> ApprovalGateResult {
let dir = working_dir.unwrap_or("."); let dir = working_dir.unwrap_or(".");
let output = std::process::Command::new("git")
.args(["status", "--porcelain"])
.current_dir(dir)
.output();
let output = match output {
Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout).to_string(),
_ => return HashSet::new(),
};
output
.lines()
.filter(|line| line.len() >= 3)
.map(|line| line[3..].trim().to_string())
.collect()
}
/// Check if file changes occurred without an approved plan.
///
/// This function should be called after each tool execution when in plan mode.
/// It checks `git status --porcelain` for changes (excluding any files that were
/// already dirty at baseline), and if a plan exists but isn't approved, returns a
/// blocking message. **Never reverts or deletes files.**
pub fn check_plan_approval_gate(
session_id: &str,
working_dir: Option<&str>,
baseline_dirty: &HashSet<String>,
) -> ApprovalGateResult {
let dir = working_dir.unwrap_or(".");
// Check if this is a git repository // Check if this is a git repository
let git_check = std::process::Command::new("git") let git_check = std::process::Command::new("git")
.args(["rev-parse", "--git-dir"]) .args(["rev-parse", "--git-dir"])
.current_dir(dir) .current_dir(dir)
.output(); .output();
if git_check.is_err() || !git_check.unwrap().status.success() { if git_check.is_err() || !git_check.unwrap().status.success() {
return ApprovalGateResult::NotGitRepo; return ApprovalGateResult::NotGitRepo;
} }
// Get current dirty files, excluding baseline
let current_dirty = get_dirty_files(working_dir);
let new_dirty: Vec<&String> = current_dirty
.iter()
.filter(|f| !baseline_dirty.contains(*f))
.collect();
// Check if a plan exists and whether it's approved // Check if a plan exists and whether it's approved
let plan = match read_plan(session_id) { let plan = match read_plan(session_id) {
Ok(Some(plan)) => plan, Ok(Some(plan)) => plan,
Ok(None) => { Ok(None) => {
// No plan exists - check if there are file changes that need blocking if new_dirty.is_empty() {
let status_output = std::process::Command::new("git") return ApprovalGateResult::Allowed;
.args(["status", "--porcelain"])
.current_dir(dir)
.output();
let output = match status_output {
Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout).to_string(),
_ => return ApprovalGateResult::Allowed,
};
if output.trim().is_empty() {
return ApprovalGateResult::Allowed; // No changes, allow
} }
// There are file changes but no plan - block and require plan creation let files_list = new_dirty
.iter()
.map(|f| format!(" - {}", f))
.collect::<Vec<_>>()
.join("\n");
return ApprovalGateResult::Blocked { return ApprovalGateResult::Blocked {
message: "⚠️ IMPLEMENTATION BLOCKED\n\n\ message: format!(
You attempted to modify files without creating a plan first.\n\n\ "⚠️ IMPLEMENTATION BLOCKED\n\n\
Before implementing, you must:\n\ File changes detected without a plan:\n\
1. Create a plan with `plan_write`\n\ {}\n\n\
2. Get the plan approved by the user\n\n\ Before implementing, you must:\n\
Do not attempt to implement until the plan is approved.".to_string(), 1. Create a plan with `plan_write`\n\
reverted_files: vec![], 2. Get the plan approved by the user\n\n\
Do not attempt to implement until the plan is approved.",
files_list
),
}; };
} }
Err(_) => return ApprovalGateResult::Allowed, // Can't read plan, allow (error case) Err(_) => return ApprovalGateResult::Allowed, // Can't read plan, allow (error case)
}; };
if plan.is_approved() { if plan.is_approved() {
return ApprovalGateResult::Allowed; return ApprovalGateResult::Allowed;
} }
// Plan exists but not approved - check for file changes if new_dirty.is_empty() {
let status_output = std::process::Command::new("git")
.args(["status", "--porcelain"])
.current_dir(dir)
.output();
let output = match status_output {
Ok(o) if o.status.success() => String::from_utf8_lossy(&o.stdout).to_string(),
_ => return ApprovalGateResult::Allowed, // Can't get status, allow
};
if output.trim().is_empty() {
return ApprovalGateResult::Allowed; // No changes
}
// Parse changed files and revert them
let mut reverted_files = Vec::new();
for line in output.lines() {
if line.len() < 3 {
continue;
}
let status = &line[0..2];
let file_path = line[3..].trim();
match status {
"??" => {
// Untracked file - remove it
let _ = std::fs::remove_file(std::path::Path::new(dir).join(file_path));
reverted_files.push(format!("{} (new file)", file_path));
}
_ => {
// Modified/added/deleted tracked file - git checkout
let _ = std::process::Command::new("git")
.args(["checkout", "--", file_path])
.current_dir(dir)
.output();
reverted_files.push(format!("{} (modified)", file_path));
}
}
}
if reverted_files.is_empty() {
return ApprovalGateResult::Allowed; return ApprovalGateResult::Allowed;
} }
let files_list = reverted_files.iter() let files_list = new_dirty
.iter()
.map(|f| format!(" - {}", f)) .map(|f| format!(" - {}", f))
.collect::<Vec<_>>() .collect::<Vec<_>>()
.join("\n"); .join("\n");
let message = format!( let message = format!(
"⚠️ IMPLEMENTATION BLOCKED\n\n\ "⚠️ IMPLEMENTATION BLOCKED\n\n\
You modified files without an approved plan:\n\ File changes detected without an approved plan:\n\
{}\n\n\ {}\n\n\
These changes have been reverted.\n\n\
Before implementing, you must:\n\ Before implementing, you must:\n\
1. Create a plan with `plan_write`\n\ 1. Create a plan with `plan_write`\n\
2. Request the user's explicit approval or edits to plan\n\n\ 2. Request the user's explicit approval or edits to plan\n\n\
Do not attempt to implement until the plan is approved.", Do not attempt to implement until the plan is approved.",
files_list files_list
); );
ApprovalGateResult::Blocked { ApprovalGateResult::Blocked {
message, message,
reverted_files,
} }
} }
@@ -1454,7 +1442,7 @@ items: []
.current_dir(temp_dir.path()) .current_dir(temp_dir.path())
.output() .output()
.unwrap(); .unwrap();
let result = check_plan_approval_gate("nonexistent-session-xyz", Some(temp_dir.path().to_str().unwrap())); let result = check_plan_approval_gate("nonexistent-session-xyz", Some(temp_dir.path().to_str().unwrap()), &HashSet::new());
assert!(matches!(result, ApprovalGateResult::Allowed)); assert!(matches!(result, ApprovalGateResult::Allowed));
} }
@@ -1470,11 +1458,11 @@ items: []
// Create an untracked file to simulate changes // Create an untracked file to simulate changes
std::fs::write(temp_dir.path().join("new_file.txt"), "test content").unwrap(); std::fs::write(temp_dir.path().join("new_file.txt"), "test content").unwrap();
let result = check_plan_approval_gate("nonexistent-session-xyz", Some(temp_dir.path().to_str().unwrap())); let result = check_plan_approval_gate("nonexistent-session-xyz", Some(temp_dir.path().to_str().unwrap()), &HashSet::new());
assert!(matches!(result, ApprovalGateResult::Blocked { .. })); assert!(matches!(result, ApprovalGateResult::Blocked { .. }));
// Verify the blocking message mentions creating a plan // Verify the blocking message mentions creating a plan
if let ApprovalGateResult::Blocked { message, .. } = result { if let ApprovalGateResult::Blocked { message } = result {
assert!(message.contains("plan_write")); assert!(message.contains("plan_write"));
} }
} }
@@ -1482,7 +1470,118 @@ items: []
#[test] #[test]
fn test_approval_gate_not_git_repo() { fn test_approval_gate_not_git_repo() {
// /tmp is typically not a git repo // /tmp is typically not a git repo
let result = check_plan_approval_gate("any-session", Some("/tmp")); let result = check_plan_approval_gate("any-session", Some("/tmp"), &HashSet::new());
assert!(matches!(result, ApprovalGateResult::NotGitRepo)); assert!(matches!(result, ApprovalGateResult::NotGitRepo));
} }
#[test]
fn test_approval_gate_warns_without_reverting() {
// Dirty files should appear in the warning message but NOT be deleted/reverted.
let temp_dir = tempfile::tempdir().unwrap();
std::process::Command::new("git")
.args(["init"])
.current_dir(temp_dir.path())
.output()
.unwrap();
// Create an untracked file
let file_path = temp_dir.path().join("should_survive.txt");
std::fs::write(&file_path, "precious content").unwrap();
let result = check_plan_approval_gate(
"nonexistent-session-xyz",
Some(temp_dir.path().to_str().unwrap()),
&HashSet::new(),
);
assert!(matches!(result, ApprovalGateResult::Blocked { .. }));
// The file must still exist on disk — gate must NOT delete it
assert!(file_path.exists(), "Gate must not delete untracked files");
assert_eq!(
std::fs::read_to_string(&file_path).unwrap(),
"precious content"
);
}
#[test]
fn test_approval_gate_excludes_baseline() {
// Files in the baseline set should be excluded from the gate check.
let temp_dir = tempfile::tempdir().unwrap();
std::process::Command::new("git")
.args(["init"])
.current_dir(temp_dir.path())
.output()
.unwrap();
// Create a file that will be in the baseline
std::fs::write(temp_dir.path().join("pre_existing.txt"), "old content").unwrap();
// Baseline includes this file
let baseline: HashSet<String> = ["pre_existing.txt".to_string()].into_iter().collect();
let result = check_plan_approval_gate(
"nonexistent-session-xyz",
Some(temp_dir.path().to_str().unwrap()),
&baseline,
);
// Only baseline files are dirty → should be Allowed
assert!(matches!(result, ApprovalGateResult::Allowed));
}
#[test]
fn test_approval_gate_blocks_new_files_with_baseline() {
// Baseline files are excluded, but new files should still trigger blocking.
let temp_dir = tempfile::tempdir().unwrap();
std::process::Command::new("git")
.args(["init"])
.current_dir(temp_dir.path())
.output()
.unwrap();
// Pre-existing file (in baseline)
std::fs::write(temp_dir.path().join("pre_existing.txt"), "old").unwrap();
// New file (not in baseline)
std::fs::write(temp_dir.path().join("new_file.txt"), "new").unwrap();
let baseline: HashSet<String> = ["pre_existing.txt".to_string()].into_iter().collect();
let result = check_plan_approval_gate(
"nonexistent-session-xyz",
Some(temp_dir.path().to_str().unwrap()),
&baseline,
);
assert!(matches!(result, ApprovalGateResult::Blocked { .. }));
if let ApprovalGateResult::Blocked { message } = result {
// Should mention the new file but NOT the baseline file
assert!(message.contains("new_file.txt"), "Should mention new file");
assert!(!message.contains("pre_existing.txt"), "Should NOT mention baseline file");
}
// Both files must still exist
assert!(temp_dir.path().join("pre_existing.txt").exists());
assert!(temp_dir.path().join("new_file.txt").exists());
}
#[test]
fn test_get_dirty_files_returns_file_paths() {
let temp_dir = tempfile::tempdir().unwrap();
std::process::Command::new("git")
.args(["init"])
.current_dir(temp_dir.path())
.output()
.unwrap();
std::fs::write(temp_dir.path().join("a.txt"), "a").unwrap();
std::fs::write(temp_dir.path().join("b.txt"), "b").unwrap();
let dirty = get_dirty_files(Some(temp_dir.path().to_str().unwrap()));
assert!(dirty.contains("a.txt"));
assert!(dirty.contains("b.txt"));
assert!(!dirty.contains("c.txt"));
}
#[test]
fn test_get_dirty_files_non_git_repo() {
// Non-git directory should return empty set without error
let temp_dir = tempfile::tempdir().unwrap();
let dirty = get_dirty_files(Some(temp_dir.path().to_str().unwrap()));
assert!(dirty.is_empty());
}
} }

View File

@@ -907,7 +907,7 @@ async fn test_plan_approval_gate_blocks_unapproved_changes() {
agent.set_working_dir(temp_path.to_string_lossy().to_string()); agent.set_working_dir(temp_path.to_string_lossy().to_string());
// Enable plan mode (required for the gate check to run) // Enable plan mode (required for the gate check to run)
agent.set_plan_mode(true); agent.set_plan_mode(true, Some(&temp_path.to_string_lossy()));
// Create an unapproved plan for this session // Create an unapproved plan for this session
let mut plan = Plan::new("test-plan"); let mut plan = Plan::new("test-plan");
@@ -937,11 +937,9 @@ async fn test_plan_approval_gate_blocks_unapproved_changes() {
assert!(result.is_ok(), "Task should complete (with blocking message): {:?}", result.err()); assert!(result.is_ok(), "Task should complete (with blocking message): {:?}", result.err());
// The new file should NOT exist (it was reverted) // The new file may or may not exist depending on whether write_file ran before the gate,
assert!( // but the gate must NOT delete/revert it. If it exists, that's fine.
!new_file_path.exists(), // The important thing is the blocking message was returned.
"New file should have been reverted/deleted"
);
// Check that the blocking message was returned // Check that the blocking message was returned
let history = &agent.get_context_window().conversation_history; let history = &agent.get_context_window().conversation_history;