[Feature]: Add mandatory plan step before task execution#37
Merged
louisdevzz merged 1 commit intomainfrom Mar 5, 2026
Merged
Conversation
Implement mandatory plan-before-execute requirement for all agent tasks: Agent changes (src/agent/agent.rs): - Add generate_and_confirm_plan() method that creates execution plan - Plan is triggered on first tool iteration when write operations detected - Plan lists: files to modify, commands to execute, expected outcomes - User must explicitly approve (yes/no) before execution proceeds - Read-only operations (file_read, etc.) are exempt from plan confirmation - Plan is added to conversation history for audit purposes Documentation (AGENTS.md): - Update section 5.2 'Plan enforcement' with detailed behavior - Document implementation location and exemption rules Features: - Automatic plan generation based on actual tool calls - Clear bullet-point plan format - User confirmation required before any state changes - Graceful rejection handling - Test-friendly implementation (all 3139 tests pass) Fixes #21
PR Intake Checks - Warnings (non-blocking)The following are recommendations:
|
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Documentation AGENTS.md |
Clarifies plan enforcement requirement: plan generation/confirmation must occur before executing state-modifying tools. Documents mandatory plan-step behavior for first tool iteration (listing actions, requiring "yes" approval, aborting on rejection, read-only ops exempt). |
Agent Planning Logic src/agent/agent.rs |
Adds private generate_and_confirm_plan() async method that inspects tool calls for write operations, generates formatted plan, prompts user approval, and appends approved plan to history. Integrates mandatory planning into turn flow at iteration == 0 before tool execution. |
Sequence Diagram
sequenceDiagram
participant User
participant Agent as Agent System
participant Tools as Tool Execution
User->>Agent: Submit task
Agent->>Agent: Evaluate next tool calls
alt Has Write Operations
Agent->>Agent: Generate plan from tool metadata
Agent->>User: Present plan for approval
User->>User: Review planned actions
User-->>Agent: Approve ("yes")
Agent->>Agent: Append plan to history
Agent->>Tools: Execute approved tools
Tools-->>Agent: Tool results
Agent->>User: Return execution results
else Read-Only Operations
Agent->>Tools: Execute tools directly
Tools-->>Agent: Tool results
Agent->>User: Return execution results
else User Rejects Plan
User-->>Agent: Reject plan
Agent->>User: Abort with no changes notice
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title clearly and concisely describes the main feature being implemented—adding a mandatory plan step before task execution. |
| Description check | ✅ Passed | The PR description comprehensively covers all required template sections including summary, metadata, validation evidence, security/privacy/compatibility assessments, verification details, rollback plan, and risks. |
| Linked Issues check | ✅ Passed | The implementation fully addresses issue #21 objectives: mandatory plan generation with user confirmation, read-only exemptions, plan logging in conversation history, and AGENTS.md documentation updates. |
| Out of Scope Changes check | ✅ Passed | All changes are scoped to the stated objectives. Only AGENTS.md and src/agent/agent.rs are modified to implement the mandatory plan step; no extraneous refactoring or unrelated changes are present. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/mandatory-plan-step
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
louisdevzz
added
the
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/agent/agent.rs (3)
442-446: Unusedmodelparameter.The
modelparameter is accepted but never used withingenerate_and_confirm_plan. Either remove it or document the intended future use.♻️ Proposed fix
async fn generate_and_confirm_plan( &mut self, - model: &str, calls: &[ParsedToolCall], ) -> Result<bool> {And update the call site at line 602:
let plan_confirmed = self - .generate_and_confirm_plan(&effective_model, &calls) + .generate_and_confirm_plan(&calls) .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent.rs` around lines 442 - 446, The generate_and_confirm_plan function currently accepts an unused model parameter; remove the unused parameter from the function signature (async fn generate_and_confirm_plan(&mut self, calls: &[ParsedToolCall]) -> Result<bool>) and update every call site that passed the model (e.g., the invocation near the previous call site) to stop supplying that argument, or alternatively if the model is intended to be used later, document the parameter and use it in the function body; prefer removing the parameter now and update callers to match the new signature (search for generate_and_confirm_plan and adjust callers accordingly).
787-871: Consider adding test coverage for the plan confirmation path.The existing tests exercise non-write-op scenarios where plan confirmation is skipped. The new
generate_and_confirm_planlogic isn't directly tested. Consider:
- A unit test for
generate_and_confirm_planwith a mock stdin, or- Refactoring confirmation input behind a trait for testability
This could be addressed in a follow-up PR given the complexity of mocking stdin.
Would you like me to open an issue to track adding test coverage for the plan confirmation workflow?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent.rs` around lines 787 - 871, Tests currently don't cover the plan confirmation path; add a unit test that exercises generate_and_confirm_plan by either (A) refactoring the confirmation input into an injectable trait (e.g., a ConfirmProvider) and adding a mock implementation passed via Agent::builder, or (B) writing a test that mocks stdin to simulate user confirmation, then calling Agent::turn to trigger generate_and_confirm_plan and asserting the plan-confirmation behavior (e.g., the agent proceeds and expected history entries appear). Modify Agent::builder to accept the injectable confirmation provider (or wiring for mocked stdin) and add a test that validates the confirmed-plan flow so generate_and_confirm_plan is exercised.
506-510: Blocking stdin read in async context may limit non-CLI usage.
std::io::stdin().read_line()blocks the tokio runtime thread. While this works for CLI mode, it could cause issues when:
- The agent runs via non-interactive channels (Telegram, Discord, Slack)
- Running in automated tests or CI pipelines
Consider abstracting the confirmation mechanism behind a trait or using
tokio::io::stdin()withAsyncBufReadExt::read_line()for async-compatible I/O. Alternatively, document that this method is CLI-only.💡 Alternative: async stdin
use tokio::io::{AsyncBufReadExt, BufReader}; // In generate_and_confirm_plan: let mut reader = BufReader::new(tokio::io::stdin()); let mut input = String::new(); reader.read_line(&mut input).await .map_err(|e| anyhow::anyhow!("Failed to read user input: {e}"))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/agent.rs` around lines 506 - 510, The blocking std::io::stdin().read_line() call inside generate_and_confirm_plan blocks the tokio runtime and prevents non-CLI usage; replace it with an async-compatible confirmation abstraction: either (1) introduce a trait (e.g., ConfirmPrompt) with an async method confirm(&mut self) -> Result<String> and inject an implementation used in generate_and_confirm_plan so non-CLI callers can provide alternate behavior, or (2) switch to tokio::io::stdin() + tokio::io::BufReader and AsyncBufReadExt::read_line(). Update generate_and_confirm_plan to depend on the async trait or await the async read_line and propagate errors as anyhow::Error, and ensure tests/CLI wiring construct the CLI implementation while non-interactive callers can stub/mock the trait.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/agent.rs`:
- Around line 448-458: The current write-op detection in has_write_ops (the
closure on calls.iter().any that matches call.name.as_str()) is missing several
sandbox- and GitHub-related state-modifying tool names; update that matches list
to also include sandbox_create, sandbox_kill, sandbox_save_snapshot,
sandbox_restore_snapshot and the GitHub mutating tools (github_create_issue,
github_create_pr, github_create_issue_with_hashtags, github_edit_issue,
github_close_issue, github_comment_issue, github_comment_pr,
github_reply_comment, github_review_pr, github_review_pr_with_checklist) so
these calls trigger the plan-confirmation gate, or alternatively refactor the
logic in the has_write_ops computation to invert the check to an explicit
allowlist of read-only tool names (e.g., file_read, sandbox_read_file,
sandbox_list_files, etc.) to ensure a safer default.
---
Nitpick comments:
In `@src/agent/agent.rs`:
- Around line 442-446: The generate_and_confirm_plan function currently accepts
an unused model parameter; remove the unused parameter from the function
signature (async fn generate_and_confirm_plan(&mut self, calls:
&[ParsedToolCall]) -> Result<bool>) and update every call site that passed the
model (e.g., the invocation near the previous call site) to stop supplying that
argument, or alternatively if the model is intended to be used later, document
the parameter and use it in the function body; prefer removing the parameter now
and update callers to match the new signature (search for
generate_and_confirm_plan and adjust callers accordingly).
- Around line 787-871: Tests currently don't cover the plan confirmation path;
add a unit test that exercises generate_and_confirm_plan by either (A)
refactoring the confirmation input into an injectable trait (e.g., a
ConfirmProvider) and adding a mock implementation passed via Agent::builder, or
(B) writing a test that mocks stdin to simulate user confirmation, then calling
Agent::turn to trigger generate_and_confirm_plan and asserting the
plan-confirmation behavior (e.g., the agent proceeds and expected history
entries appear). Modify Agent::builder to accept the injectable confirmation
provider (or wiring for mocked stdin) and add a test that validates the
confirmed-plan flow so generate_and_confirm_plan is exercised.
- Around line 506-510: The blocking std::io::stdin().read_line() call inside
generate_and_confirm_plan blocks the tokio runtime and prevents non-CLI usage;
replace it with an async-compatible confirmation abstraction: either (1)
introduce a trait (e.g., ConfirmPrompt) with an async method confirm(&mut self)
-> Result<String> and inject an implementation used in generate_and_confirm_plan
so non-CLI callers can provide alternate behavior, or (2) switch to
tokio::io::stdin() + tokio::io::BufReader and AsyncBufReadExt::read_line().
Update generate_and_confirm_plan to depend on the async trait or await the async
read_line and propagate errors as anyhow::Error, and ensure tests/CLI wiring
construct the CLI implementation while non-interactive callers can stub/mock the
trait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a70bfe3-277c-4ec0-8047-8269981a9578
📒 Files selected for processing (2)
AGENTS.mdsrc/agent/agent.rs
louisdevzz
added a commit
that referenced
this pull request
Mar 5, 2026
Added detailed FIX_PLAN_BUGS.md documenting: - Current implementation and flow - 5 specific bugs with acceptance criteria - Step-by-step implementation guide - Code structure and key methods - Testing strategy This document will help any agent understand and fix the bugs in the mandatory plan-before-execute feature. Related: #38, #37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mainLabel Snapshot (required)
risk: mediumsize: Magent,coreagent: orchestrationtrusted contributorChange Metadata
featureagentLinked Issue
Validation Evidence
Security Impact
NoNoNoNoPrivacy and Data Hygiene
passCompatibility / Migration
YesNoNoi18n Follow-Through
NoRollback Plan
git revertor revert PRRisks and Mitigations