feat: /diff-preview — approve/reject file changes with diff#15
Merged
kienbui1995 merged 1 commit intomainfrom Apr 11, 2026
Merged
feat: /diff-preview — approve/reject file changes with diff#15kienbui1995 merged 1 commit intomainfrom
kienbui1995 merged 1 commit intomainfrom
Conversation
When enabled via /diff-preview, write_file/edit_file/batch_edit/apply_patch show a diff preview and require Y/N approval before writing. - Write tools routed to sequential execution for interactive prompting - Diff computed from current file vs proposed content - Uses existing TUI permission prompt (Y/N/A) - Toggle on/off with /diff-preview command
📝 WalkthroughWalkthroughA diff-preview toggle feature is added across multiple crates. Users invoke Changes
Sequence DiagramsequenceDiagram
participant User
participant TUI as TUI Commands
participant App
participant CLI as CLI Event Loop
participant Runtime
participant ToolRegistry
participant Prompter
User->>TUI: /diff-preview
TUI->>App: Toggle review_writes flag
TUI->>App: Set pending_command = ReviewToggle
App-->>User: Status message (diff preview ON/OFF)
Note over CLI: Event loop detects pending command
CLI->>Runtime: set_review_writes(enabled)
Runtime->>ToolRegistry: Store review_writes state (atomic)
ToolRegistry-->>Runtime: State updated
Note over User,ToolRegistry: Later: Write operation triggered
User->>Runtime: Execute write_file/edit_file
Runtime->>ToolRegistry: Load review_writes (atomic)
alt review_writes enabled
Runtime->>Runtime: Classify tool as sequential
Runtime->>Runtime: Generate diff_preview_summary()
Runtime->>Prompter: decide(PermissionRequest with diff)
Prompter-->>User: Show diff & prompt
User-->>Prompter: Approve/Deny
Prompter-->>Runtime: Permission result
else review_writes disabled
Runtime->>Runtime: Use policy-based authorization
end
Runtime-->>User: Execute or deny tool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-cli/src/main.rs`:
- Around line 1031-1035: The ReviewToggle branch currently ignores the updated
app.review_writes when runtime.try_lock() fails, causing UI/state desync; modify
the logic so PendingCommand::ReviewToggle records the desired state (e.g., set a
new queued_review_writes or pending_review_toggle flag storing
app.review_writes) instead of dropping it, and then, in the existing
deferred-lock/retry section that later obtains the runtime lock, apply the
queued value by calling rt.set_review_writes(queued_value) and clear the queued
flag; ensure you still call rt.set_review_writes immediately when try_lock()
succeeds (the existing rt.set_review_writes call) so behavior is unchanged on
success.
In `@mc/crates/mc-core/src/parallel_tools.rs`:
- Around line 287-311: compute_diff_preview currently reads files from disk
(std::fs::read_to_string) using model-supplied paths which can leak local file
contents; instead generate previews only from validated/parsing outputs after
tool-layer sandbox checks. Change the flow so compute_diff_preview (and callers)
are invoked after path/patch validation/parsing (the code path that validates
tool calls before dispatch) and update compute_diff_preview to accept validated
inputs (e.g., a sanitized path and content or a Diff/Patch structure) rather
than reading the filesystem; also ensure handlers for write_file, edit_file,
batch_edit and apply_patch use the provided/parsed content and patches to
compute diffs (via simple_diff or existing patch diffing) so previews reflect
what will actually be applied and never read raw files. Ensure function
signatures and call sites (compute_diff_preview, write_file handling,
batch_edit/apply_patch preview code) are updated accordingly so no direct
std::fs::read_to_string occurs during preview generation.
In `@mc/crates/mc-core/src/runtime.rs`:
- Around line 636-655: The current branch skips policy.authorize for reviewed
write tools; instead always call policy.authorize(name, input, prompter) first
(using the same prompter handling you already have), capture its
PermissionOutcome, and only if that outcome indicates the action is permitted or
promptable AND tool_registry.review_writes/matches!(...) (i.e., is_reviewed) run
crate::parallel_tools::diff_preview_summary and then call
PermissionPrompter::decide() to confirm/deny; if decide() returns Deny override
the outcome to Deny, otherwise keep the original authorize Allow/Prompt result.
Ensure you reference policy.authorize, PermissionPrompter::decide,
diff_preview_summary, and the is_reviewed check when applying this change.
In `@mc/crates/mc-tui/src/commands.rs`:
- Around line 57-61: Handler for "/diff-preview" toggles review_writes
(PendingCommand::ReviewToggle) but the command was not added to discovery so
help/tab-complete misses it; update App::SLASH_COMMANDS to include an entry for
"/diff-preview" with a short description matching the behavior (e.g., "Diff
preview: toggle write tools to prompt with diff") and ensure any discovery/path
lists used by help or completion include the same string so the command appears
in /help and tab completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04bec8e0-eae0-4395-85b9-ea9daf2930eb
📒 Files selected for processing (6)
mc/crates/mc-cli/src/main.rsmc/crates/mc-core/src/parallel_tools.rsmc/crates/mc-core/src/runtime.rsmc/crates/mc-tools/src/registry.rsmc/crates/mc-tui/src/app.rsmc/crates/mc-tui/src/commands.rs
Comment on lines
+1031
to
+1035
| PendingCommand::ReviewToggle => { | ||
| if let Ok(rt) = runtime.try_lock() { | ||
| rt.set_review_writes(app.review_writes); | ||
| } | ||
| } |
There was a problem hiding this comment.
Persist /diff-preview toggles when the runtime lock is busy.
app.review_writes is already flipped before this branch runs, but a failed runtime.try_lock() drops the update entirely. That leaves the UI showing “Diff preview: ON/OFF” while ConversationRuntime keeps the previous value. Queue the requested state and apply it in the deferred lock section instead of silently skipping it.
🛠️ One way to keep the toggle in sync
+ let mut pending_review_sync: Option<bool> = None;
...
PendingCommand::ReviewToggle => {
- if let Ok(rt) = runtime.try_lock() {
- rt.set_review_writes(app.review_writes);
- }
+ pending_review_sync = Some(app.review_writes);
}
...
if let Ok(mut rt) = runtime.try_lock() {
+ if let Some(enabled) = pending_review_sync.take() {
+ rt.set_review_writes(enabled);
+ }
if pending_plan_sync {
rt.plan_mode = app.plan_mode;
pending_plan_sync = false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-cli/src/main.rs` around lines 1031 - 1035, The ReviewToggle
branch currently ignores the updated app.review_writes when runtime.try_lock()
fails, causing UI/state desync; modify the logic so PendingCommand::ReviewToggle
records the desired state (e.g., set a new queued_review_writes or
pending_review_toggle flag storing app.review_writes) instead of dropping it,
and then, in the existing deferred-lock/retry section that later obtains the
runtime lock, apply the queued value by calling
rt.set_review_writes(queued_value) and clear the queued flag; ensure you still
call rt.set_review_writes immediately when try_lock() succeeds (the existing
rt.set_review_writes call) so behavior is unchanged on success.
Comment on lines
+287
to
+311
| fn compute_diff_preview(tool_name: &str, input_json: &str) -> String { | ||
| let v: serde_json::Value = serde_json::from_str(input_json).unwrap_or_default(); | ||
| let path = v["path"].as_str().unwrap_or("?"); | ||
| match tool_name { | ||
| "write_file" => { | ||
| let old = std::fs::read_to_string(path).unwrap_or_default(); | ||
| let new = v["content"].as_str().unwrap_or(""); | ||
| let action = if old.is_empty() { "CREATE" } else { "MODIFY" }; | ||
| let diff = simple_diff(&old, new); | ||
| format!("📝 [{action}] {path}\n{diff}") | ||
| } | ||
| "edit_file" => { | ||
| let old_str = v["old_string"].as_str().unwrap_or(""); | ||
| let new_str = v["new_string"].as_str().unwrap_or(""); | ||
| let mut diff = format!("📝 [EDIT] {path}\n"); | ||
| for line in old_str.lines() { | ||
| diff.push_str(&format!("- {line}\n")); | ||
| } | ||
| for line in new_str.lines() { | ||
| diff.push_str(&format!("+ {line}\n")); | ||
| } | ||
| diff | ||
| } | ||
| _ => format!("📝 {tool_name}: {path}"), | ||
| } |
There was a problem hiding this comment.
Generate reviewed-write previews from validated inputs, not raw filesystem reads.
write_file previewing does std::fs::read_to_string(path) on the model-supplied path before the tool layer validates sandbox/allowed roots. Since mc/crates/mc-core/src/runtime.rs Line 640 calls this helper before dispatch, a reviewed write can leak arbitrary local file contents into the approval prompt even if the actual tool call would be denied later. The same fallback also leaves batch_edit and apply_patch without a real diff at all. Move preview generation behind the validated tool path/parsing logic so every reviewed write shows a genuine preview without bypassing sandbox checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/parallel_tools.rs` around lines 287 - 311,
compute_diff_preview currently reads files from disk (std::fs::read_to_string)
using model-supplied paths which can leak local file contents; instead generate
previews only from validated/parsing outputs after tool-layer sandbox checks.
Change the flow so compute_diff_preview (and callers) are invoked after
path/patch validation/parsing (the code path that validates tool calls before
dispatch) and update compute_diff_preview to accept validated inputs (e.g., a
sanitized path and content or a Diff/Patch structure) rather than reading the
filesystem; also ensure handlers for write_file, edit_file, batch_edit and
apply_patch use the provided/parsed content and patches to compute diffs (via
simple_diff or existing patch diffing) so previews reflect what will actually be
applied and never read raw files. Ensure function signatures and call sites
(compute_diff_preview, write_file handling, batch_edit/apply_patch preview code)
are updated accordingly so no direct std::fs::read_to_string occurs during
preview generation.
Comment on lines
+636
to
+655
| let outcome = { | ||
| let is_reviewed = self.tool_registry.review_writes.load(std::sync::atomic::Ordering::Relaxed) | ||
| && matches!(name, "write_file" | "edit_file" | "batch_edit" | "apply_patch"); | ||
| if is_reviewed { | ||
| let diff_summary = crate::parallel_tools::diff_preview_summary(name, input); | ||
| match prompter { | ||
| Some(ref mut p) => p.decide(&mc_tools::PermissionRequest { | ||
| tool_name: name.to_string(), | ||
| input_summary: diff_summary, | ||
| }), | ||
| None => PermissionOutcome::Deny { | ||
| reason: "diff preview requires interactive mode".into(), | ||
| }, | ||
| } | ||
| } else { | ||
| match prompter { | ||
| Some(ref mut p) => policy.authorize(name, input, Some(&mut **p)), | ||
| None => policy.authorize(name, input, None), | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep diff preview as an extra gate, not a permission bypass.
This branch calls PermissionPrompter::decide() directly and never consults policy.authorize(). In ReadOnly mode, that means a user can still approve write_file/edit_file/batch_edit/apply_patch and execute them, which breaks the configured permission policy. The diff-preview prompt should run only after the policy says the tool is otherwise allowed or promptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/runtime.rs` around lines 636 - 655, The current branch
skips policy.authorize for reviewed write tools; instead always call
policy.authorize(name, input, prompter) first (using the same prompter handling
you already have), capture its PermissionOutcome, and only if that outcome
indicates the action is permitted or promptable AND
tool_registry.review_writes/matches!(...) (i.e., is_reviewed) run
crate::parallel_tools::diff_preview_summary and then call
PermissionPrompter::decide() to confirm/deny; if decide() returns Deny override
the outcome to Deny, otherwise keep the original authorize Allow/Prompt result.
Ensure you reference policy.authorize, PermissionPrompter::decide,
diff_preview_summary, and the is_reviewed check when applying this change.
Comment on lines
+57
to
+61
| "/diff-preview" => { | ||
| app.review_writes = !app.review_writes; | ||
| app.push(&format!("Diff preview: {}", if app.review_writes { "ON — write tools will prompt with diff" } else { "OFF" })); | ||
| app.pending_command = Some(PendingCommand::ReviewToggle); | ||
| } |
There was a problem hiding this comment.
Add /diff-preview to the discovery paths.
The handler exists, but /help still omits it and App::SLASH_COMMANDS in mc/crates/mc-tui/src/app.rs does not include it, so the new feature will not show up in help or tab completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-tui/src/commands.rs` around lines 57 - 61, Handler for
"/diff-preview" toggles review_writes (PendingCommand::ReviewToggle) but the
command was not added to discovery so help/tab-complete misses it; update
App::SLASH_COMMANDS to include an entry for "/diff-preview" with a short
description matching the behavior (e.g., "Diff preview: toggle write tools to
prompt with diff") and ensure any discovery/path lists used by help or
completion include the same string so the command appears in /help and tab
completion.
This was referenced Apr 11, 2026
Merged
This was referenced Apr 12, 2026
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.
What
New
/diff-previewtoggle command. When enabled, all write tools (write_file, edit_file, batch_edit, apply_patch) show a diff preview and require user approval before writing.How it works
/diff-previewtoggles the mode on/offChanges
mc-tools/src/registry.rs: Addreview_writesAtomicBool flagmc-core/src/runtime.rs: Route write tools to sequential when review_writes on, force permission prompt with diffmc-core/src/parallel_tools.rs: Adddiff_preview_summary()helpermc-tui/src/app.rs: Addreview_writesstate +ReviewTogglecommandmc-tui/src/commands.rs: Add/diff-previewcommandmc-cli/src/main.rs: Handle ReviewToggle152 tests pass.
Summary by CodeRabbit
/diff-previewslash command to toggle interactive diff review for write operations. When enabled, previews changes with up to 30 modified lines and requires approval before file operations execute.