diff --git a/analysis/memory.md b/analysis/memory.md index 147ac72..6b15dc5 100644 --- a/analysis/memory.md +++ b/analysis/memory.md @@ -1,5 +1,5 @@ # Workspace Memory -> Updated: 2026-02-13 | Size: ~20k chars +> Updated: 2026-02-14T22:33:04Z | Size: 22.9k chars ### Remember Tool Wiring - `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 - `update_tool_output_line()` [407..445], `print_tool_output_line()` [447..454] - clip_line() - `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` 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` 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. \ No newline at end of file diff --git a/crates/g3-cli/src/interactive.rs b/crates/g3-cli/src/interactive.rs index ad3e261..446990a 100644 --- a/crates/g3-cli/src/interactive.rs +++ b/crates/g3-cli/src/interactive.rs @@ -95,7 +95,7 @@ fn check_and_exit_plan_mode_if_terminal( if *in_plan_mode && agent.is_plan_terminal() { output.print("\nšŸ“‹ Plan complete - exiting plan mode"); *in_plan_mode = false; - agent.set_plan_mode(false); + agent.set_plan_mode(false, None); return true; } false @@ -159,7 +159,7 @@ pub async fn run_interactive( let mut is_first_plan_message = in_plan_mode; // 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 let config = Config::builder() @@ -282,7 +282,7 @@ pub async fn run_interactive( } CommandResult::EnterPlanMode => { 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; continue; } @@ -322,7 +322,7 @@ pub async fn run_interactive( if in_plan_mode { output.print("CTRL-D (exiting plan mode)"); in_plan_mode = false; - agent.set_plan_mode(false); + agent.set_plan_mode(false, None); // Continue the loop with normal prompt continue; } else { diff --git a/crates/g3-core/src/lib.rs b/crates/g3-core/src/lib.rs index f6fa290..f3f28ae 100644 --- a/crates/g3-core/src/lib.rs +++ b/crates/g3-core/src/lib.rs @@ -52,7 +52,7 @@ pub use skills::{Skill, discover_skills, generate_skills_prompt}; #[cfg(test)] mod task_result_comprehensive_tests; 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)] mod tilde_expansion_tests; @@ -166,6 +166,8 @@ pub struct Agent { acd_enabled: bool, /// Whether plan mode is active (gate blocks file changes without approved plan) in_plan_mode: bool, + /// Files that were already dirty when plan mode started (excluded from approval gate) + baseline_dirty_files: std::collections::HashSet, /// Manager for async research tasks pending_research_manager: pending_research::PendingResearchManager, /// Set of toolset names that have been loaded in this session @@ -224,6 +226,7 @@ impl Agent { auto_memory: false, acd_enabled: false, in_plan_mode: false, + baseline_dirty_files: std::collections::HashSet::new(), pending_research_manager: pending_research::PendingResearchManager::new(), loaded_toolsets: std::collections::HashSet::new(), } @@ -1669,8 +1672,15 @@ impl Agent { } /// 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; + 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 @@ -3014,8 +3024,8 @@ Skip if nothing new. Be brief."#; // Check plan approval gate after tool execution (only in plan mode) if self.in_plan_mode { if let Some(session_id) = &self.session_id { - if let ApprovalGateResult::Blocked { message, .. } = - check_plan_approval_gate(session_id, working_dir) + if let ApprovalGateResult::Blocked { message } = + check_plan_approval_gate(session_id, working_dir, &self.baseline_dirty_files) { // Return the blocking message instead of the tool result return Ok(message); diff --git a/crates/g3-core/src/tools/plan.rs b/crates/g3-core/src/tools/plan.rs index badd028..10875fd 100644 --- a/crates/g3-core/src/tools/plan.rs +++ b/crates/g3-core/src/tools/plan.rs @@ -10,6 +10,7 @@ use anyhow::{anyhow, Result}; use serde::{Deserialize, Serialize}; +use std::collections::HashSet; use std::fmt; use std::path::PathBuf; use std::path::Path; @@ -973,138 +974,125 @@ pub async fn execute_plan_approve( pub enum ApprovalGateResult { /// No plan exists, or plan is approved - allow the operation 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 { /// Message to inject into the conversation message: String, - /// Files that were reverted - reverted_files: Vec, }, /// Not a git repository - skip the check NotGitRepo, } -/// Check if file changes occurred without an approved plan, and revert them if so. -/// -/// This function should be called after each tool execution when in plan mode. -/// It checks `git status --porcelain` for changes, and if a plan exists but isn't -/// approved, it reverts those changes and returns a blocking message. -pub fn check_plan_approval_gate(session_id: &str, working_dir: Option<&str>) -> ApprovalGateResult { +/// Get the set of dirty file paths from `git status --porcelain`. +/// +/// Returns an empty set if not a git repo or if the command fails. +/// Each entry is the file path as reported by git (relative to repo root). +pub fn get_dirty_files(working_dir: Option<&str>) -> HashSet { 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, +) -> ApprovalGateResult { + let dir = working_dir.unwrap_or("."); + // Check if this is a git repository let git_check = std::process::Command::new("git") .args(["rev-parse", "--git-dir"]) .current_dir(dir) .output(); - + if git_check.is_err() || !git_check.unwrap().status.success() { 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 let plan = match read_plan(session_id) { Ok(Some(plan)) => plan, Ok(None) => { - // No plan exists - check if there are file changes that need blocking - 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, - }; - - if output.trim().is_empty() { - return ApprovalGateResult::Allowed; // No changes, allow + if new_dirty.is_empty() { + return ApprovalGateResult::Allowed; } - - // There are file changes but no plan - block and require plan creation + + let files_list = new_dirty + .iter() + .map(|f| format!(" - {}", f)) + .collect::>() + .join("\n"); + return ApprovalGateResult::Blocked { - message: "āš ļø IMPLEMENTATION BLOCKED\n\n\ - You attempted to modify files without creating a plan first.\n\n\ - Before implementing, you must:\n\ - 1. Create a plan with `plan_write`\n\ - 2. Get the plan approved by the user\n\n\ - Do not attempt to implement until the plan is approved.".to_string(), - reverted_files: vec![], + message: format!( + "āš ļø IMPLEMENTATION BLOCKED\n\n\ + File changes detected without a plan:\n\ + {}\n\n\ + Before implementing, you must:\n\ + 1. Create a plan with `plan_write`\n\ + 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() { return ApprovalGateResult::Allowed; } - - // Plan exists but not approved - check for file changes - 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() { + + if new_dirty.is_empty() { return ApprovalGateResult::Allowed; } - - let files_list = reverted_files.iter() + + let files_list = new_dirty + .iter() .map(|f| format!(" - {}", f)) .collect::>() .join("\n"); - + let message = format!( "āš ļø IMPLEMENTATION BLOCKED\n\n\ - You modified files without an approved plan:\n\ + File changes detected without an approved plan:\n\ {}\n\n\ - These changes have been reverted.\n\n\ Before implementing, you must:\n\ 1. Create a plan with `plan_write`\n\ 2. Request the user's explicit approval or edits to plan\n\n\ Do not attempt to implement until the plan is approved.", files_list ); - + ApprovalGateResult::Blocked { message, - reverted_files, } } @@ -1454,7 +1442,7 @@ items: [] .current_dir(temp_dir.path()) .output() .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)); } @@ -1470,11 +1458,11 @@ items: [] // Create an untracked file to simulate changes 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 { .. })); // 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")); } } @@ -1482,7 +1470,118 @@ items: [] #[test] fn test_approval_gate_not_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)); } + + #[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 = ["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 = ["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()); + } } diff --git a/crates/g3-core/tests/mock_provider_integration_test.rs b/crates/g3-core/tests/mock_provider_integration_test.rs index 2474674..210b152 100644 --- a/crates/g3-core/tests/mock_provider_integration_test.rs +++ b/crates/g3-core/tests/mock_provider_integration_test.rs @@ -907,7 +907,7 @@ async fn test_plan_approval_gate_blocks_unapproved_changes() { agent.set_working_dir(temp_path.to_string_lossy().to_string()); // 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 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()); - // The new file should NOT exist (it was reverted) - assert!( - !new_file_path.exists(), - "New file should have been reverted/deleted" - ); + // The new file may or may not exist depending on whether write_file ran before the gate, + // but the gate must NOT delete/revert it. If it exists, that's fine. + // The important thing is the blocking message was returned. // Check that the blocking message was returned let history = &agent.get_context_window().conversation_history;