Fix plan approval gate and add "Create a plan:" prefix for first message
- Fix build warnings: add #[allow(dead_code)] to unused deserialization fields - Fix plan approval gate bug: block file changes when no plan exists (not just when plan exists but is unapproved) - Add "Create a plan: " prefix to first user message in plan mode - Add prepare_plan_mode_input() helper function for testability - Reset is_first_plan_message flag when entering plan mode via /plan command - Add tests for approval gate (no plan + no changes, no plan + changes) - Add tests for prepare_plan_mode_input (happy, negative, boundary cases)
This commit is contained in:
@@ -52,6 +52,18 @@ pub fn build_prompt(in_multiline: bool, in_plan_mode: bool, agent_name: Option<&
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Prepare user input for plan mode, prepending "Create a plan: " if this is the first message.
|
||||||
|
/// Returns the (possibly modified) input and whether the flag should be reset.
|
||||||
|
pub fn prepare_plan_mode_input(input: &str, is_first_plan_message: bool, in_plan_mode: bool) -> (String, bool) {
|
||||||
|
if in_plan_mode && is_first_plan_message {
|
||||||
|
// Prepend "Create a plan: " and signal to reset the flag
|
||||||
|
(format!("Create a plan: {}", input), true)
|
||||||
|
} else {
|
||||||
|
// No modification needed
|
||||||
|
(input.to_string(), false)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Check if the input is an approval command (for plan mode).
|
/// Check if the input is an approval command (for plan mode).
|
||||||
///
|
///
|
||||||
/// Recognizes: "a", "approve", "approved", and common misspellings.
|
/// Recognizes: "a", "approve", "approved", and common misspellings.
|
||||||
@@ -191,6 +203,9 @@ pub async fn run_interactive<W: UiWriter>(
|
|||||||
// Track plan mode state (start in plan mode for non-agent mode)
|
// Track plan mode state (start in plan mode for non-agent mode)
|
||||||
let mut in_plan_mode = !from_agent_mode;
|
let mut in_plan_mode = !from_agent_mode;
|
||||||
|
|
||||||
|
// Track if this is the first message in plan mode (to prepend "Create a plan: ")
|
||||||
|
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);
|
||||||
|
|
||||||
@@ -275,8 +290,13 @@ pub async fn run_interactive<W: UiWriter>(
|
|||||||
// Reprint input with formatting
|
// Reprint input with formatting
|
||||||
reprint_formatted_input(&input, &prompt);
|
reprint_formatted_input(&input, &prompt);
|
||||||
|
|
||||||
|
// Prepend "Create a plan: " for first message in plan mode
|
||||||
|
let (final_input, should_reset) = prepare_plan_mode_input(&input, is_first_plan_message, in_plan_mode);
|
||||||
|
if should_reset {
|
||||||
|
is_first_plan_message = false;
|
||||||
|
}
|
||||||
execute_user_input(
|
execute_user_input(
|
||||||
&mut agent, &input, show_prompt, show_code, &output, from_agent_mode
|
&mut agent, &final_input, show_prompt, show_code, &output, from_agent_mode
|
||||||
).await;
|
).await;
|
||||||
} else {
|
} else {
|
||||||
// Single line input
|
// Single line input
|
||||||
@@ -328,6 +348,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);
|
||||||
|
is_first_plan_message = true;
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -336,8 +357,13 @@ pub async fn run_interactive<W: UiWriter>(
|
|||||||
// Reprint input with formatting
|
// Reprint input with formatting
|
||||||
reprint_formatted_input(&input, &prompt);
|
reprint_formatted_input(&input, &prompt);
|
||||||
|
|
||||||
|
// Prepend "Create a plan: " for first message in plan mode
|
||||||
|
let (final_input, should_reset) = prepare_plan_mode_input(&input, is_first_plan_message, in_plan_mode);
|
||||||
|
if should_reset {
|
||||||
|
is_first_plan_message = false;
|
||||||
|
}
|
||||||
execute_user_input(
|
execute_user_input(
|
||||||
&mut agent, &input, show_prompt, show_code, &output, from_agent_mode
|
&mut agent, &final_input, show_prompt, show_code, &output, from_agent_mode
|
||||||
).await;
|
).await;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -529,5 +555,65 @@ mod tests {
|
|||||||
assert!(is_approval_input("yes!"));
|
assert!(is_approval_input("yes!"));
|
||||||
assert!(is_approval_input("ok,"));
|
assert!(is_approval_input("ok,"));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Tests for prepare_plan_mode_input
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_happy_path_first_message() {
|
||||||
|
// Happy path: First message in plan mode gets "Create a plan: " prefix
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input("fix the bug", true, true);
|
||||||
|
assert_eq!(result, "Create a plan: fix the bug");
|
||||||
|
assert!(should_reset);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_negative_second_message() {
|
||||||
|
// Negative: Second message (is_first_plan_message = false) should NOT get prefix
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input("fix the bug", false, true);
|
||||||
|
assert_eq!(result, "fix the bug");
|
||||||
|
assert!(!should_reset);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_negative_not_in_plan_mode() {
|
||||||
|
// Negative: Not in plan mode should NOT get prefix even if is_first_plan_message is true
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input("fix the bug", true, false);
|
||||||
|
assert_eq!(result, "fix the bug");
|
||||||
|
assert!(!should_reset);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_negative_neither_condition() {
|
||||||
|
// Negative: Neither in plan mode nor first message
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input("fix the bug", false, false);
|
||||||
|
assert_eq!(result, "fix the bug");
|
||||||
|
assert!(!should_reset);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_boundary_empty_input() {
|
||||||
|
// Boundary: Empty input would get prefix, but in practice empty input
|
||||||
|
// is filtered out by the caller before reaching this function.
|
||||||
|
// This test documents the function's behavior in isolation.
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input("", true, true);
|
||||||
|
assert_eq!(result, "Create a plan: ");
|
||||||
|
assert!(should_reset);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_boundary_whitespace_input() {
|
||||||
|
// Boundary: Whitespace-only input gets prefix preserved
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input(" ", true, true);
|
||||||
|
assert_eq!(result, "Create a plan: ");
|
||||||
|
assert!(should_reset);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_prepare_plan_mode_input_boundary_multiline_input() {
|
||||||
|
// Boundary: Multiline input gets prefix on first line only
|
||||||
|
let (result, should_reset) = prepare_plan_mode_input("line1\nline2\nline3", true, true);
|
||||||
|
assert_eq!(result, "Create a plan: line1\nline2\nline3");
|
||||||
|
assert!(should_reset);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -716,6 +716,7 @@ impl UiWriter for ConsoleUiWriter {
|
|||||||
#[derive(serde::Deserialize)]
|
#[derive(serde::Deserialize)]
|
||||||
struct PlanCompact {
|
struct PlanCompact {
|
||||||
plan_id: String,
|
plan_id: String,
|
||||||
|
#[allow(dead_code)]
|
||||||
revision: u32,
|
revision: u32,
|
||||||
approved_revision: Option<u32>,
|
approved_revision: Option<u32>,
|
||||||
items: Vec<PlanItemCompact>,
|
items: Vec<PlanItemCompact>,
|
||||||
@@ -731,6 +732,7 @@ impl UiWriter for ConsoleUiWriter {
|
|||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
evidence: Vec<String>,
|
evidence: Vec<String>,
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
|
#[allow(dead_code)]
|
||||||
notes: Option<String>,
|
notes: Option<String>,
|
||||||
}
|
}
|
||||||
#[derive(serde::Deserialize)]
|
#[derive(serde::Deserialize)]
|
||||||
|
|||||||
@@ -1010,8 +1010,34 @@ pub fn check_plan_approval_gate(session_id: &str, working_dir: Option<&str>) ->
|
|||||||
// 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) => return ApprovalGateResult::Allowed, // No plan, allow
|
Ok(None) => {
|
||||||
Err(_) => return ApprovalGateResult::Allowed, // Can't read plan, allow
|
// 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
|
||||||
|
}
|
||||||
|
|
||||||
|
// There are file changes but no plan - block and require plan creation
|
||||||
|
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![],
|
||||||
|
};
|
||||||
|
}
|
||||||
|
Err(_) => return ApprovalGateResult::Allowed, // Can't read plan, allow (error case)
|
||||||
};
|
};
|
||||||
|
|
||||||
if plan.is_approved() {
|
if plan.is_approved() {
|
||||||
@@ -1424,12 +1450,40 @@ items: []
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_approval_gate_no_plan() {
|
fn test_approval_gate_no_plan_no_changes() {
|
||||||
// With a non-existent session, there's no plan, so it should allow
|
// With a non-existent session and no uncommitted changes, it should allow.
|
||||||
let result = check_plan_approval_gate("nonexistent-session-xyz", Some("."));
|
// We use a temp dir that's a fresh git repo with no changes.
|
||||||
|
let temp_dir = tempfile::tempdir().unwrap();
|
||||||
|
std::process::Command::new("git")
|
||||||
|
.args(["init"])
|
||||||
|
.current_dir(temp_dir.path())
|
||||||
|
.output()
|
||||||
|
.unwrap();
|
||||||
|
let result = check_plan_approval_gate("nonexistent-session-xyz", Some(temp_dir.path().to_str().unwrap()));
|
||||||
assert!(matches!(result, ApprovalGateResult::Allowed));
|
assert!(matches!(result, ApprovalGateResult::Allowed));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_approval_gate_no_plan_with_changes() {
|
||||||
|
// With a non-existent session but uncommitted changes, it should block.
|
||||||
|
let temp_dir = tempfile::tempdir().unwrap();
|
||||||
|
std::process::Command::new("git")
|
||||||
|
.args(["init"])
|
||||||
|
.current_dir(temp_dir.path())
|
||||||
|
.output()
|
||||||
|
.unwrap();
|
||||||
|
// 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()));
|
||||||
|
assert!(matches!(result, ApprovalGateResult::Blocked { .. }));
|
||||||
|
|
||||||
|
// Verify the blocking message mentions creating a plan
|
||||||
|
if let ApprovalGateResult::Blocked { message, .. } = result {
|
||||||
|
assert!(message.contains("plan_write"));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[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
|
||||||
|
|||||||
Reference in New Issue
Block a user