From 49b28d1646286a1bae20afb93ee3532dfba35888 Mon Sep 17 00:00:00 2001 From: HenkDz Date: Sat, 16 May 2026 11:55:49 +0100 Subject: [PATCH] fix(acp): avoid duplicate edit approval diffs --- acp_adapter/tools.py | 15 ++++--------- tests/acp/test_mcp_e2e.py | 13 ++++------- tests/acp/test_tools.py | 45 ++++++++++++++++++--------------------- 3 files changed, 29 insertions(+), 44 deletions(-) diff --git a/acp_adapter/tools.py b/acp_adapter/tools.py index 77a62e243bc..e9ea747324b 100644 --- a/acp_adapter/tools.py +++ b/acp_adapter/tools.py @@ -895,7 +895,7 @@ def _build_tool_complete_content( if len(display_result) > 5000: display_result = display_result[:4900] + f"\n... ({len(result)} chars total, truncated)" - if tool_name in {"write_file", "patch", "skill_manage"}: + if tool_name == "skill_manage": try: from agent.display import extract_edit_diff @@ -936,22 +936,15 @@ def build_tool_start( if tool_name == "patch": mode = arguments.get("mode", "replace") - if mode == "replace": - path = arguments.get("path", "") - old = arguments.get("old_string", "") - new = arguments.get("new_string", "") - content = [acp.tool_diff_content(path=path, new_text=new, old_text=old)] - else: - patch_text = arguments.get("patch", "") - content = _build_patch_mode_content(patch_text) + path = arguments.get("path") or "patch input" + content = [_text(f"Preparing {mode} edit for {path}. Approval prompt shows the diff.")] return acp.start_tool_call( tool_call_id, title, kind=kind, content=content, locations=locations, ) if tool_name == "write_file": path = arguments.get("path", "") - file_content = arguments.get("content", "") - content = [acp.tool_diff_content(path=path, new_text=file_content)] + content = [_text(f"Preparing write to {path}. Approval prompt shows the diff." if path else "Preparing file write. Approval prompt shows the diff.")] return acp.start_tool_call( tool_call_id, title, kind=kind, content=content, locations=locations, ) diff --git a/tests/acp/test_mcp_e2e.py b/tests/acp/test_mcp_e2e.py index dab46071980..00bf53b21f3 100644 --- a/tests/acp/test_mcp_e2e.py +++ b/tests/acp/test_mcp_e2e.py @@ -183,7 +183,7 @@ class TestMcpRegistrationE2E: assert "hello" in complete_event.content[0].content.text assert complete_event.raw_output is None - def test_patch_mode_tool_start_emits_diff_blocks_for_v4a_patch(self): + def test_patch_mode_tool_start_defers_diff_to_edit_approval_prompt(self): update = build_tool_start( "tc-1", "patch", @@ -193,14 +193,9 @@ class TestMcpRegistrationE2E: }, ) - assert len(update.content) == 2 - assert update.content[0].type == "diff" - assert update.content[0].path == "src/app.py" - assert update.content[0].old_text == "old line" - assert update.content[0].new_text == "new line" - assert update.content[1].type == "diff" - assert update.content[1].path == "src/new.py" - assert update.content[1].new_text == "hello" + assert len(update.content) == 1 + assert update.content[0].type == "content" + assert "Approval prompt shows the diff" in update.content[0].content.text @pytest.mark.asyncio async def test_prompt_tool_results_paired_by_call_id(self, acp_agent, mock_manager): diff --git a/tests/acp/test_tools.py b/tests/acp/test_tools.py index dc62b296c69..11a427591da 100644 --- a/tests/acp/test_tools.py +++ b/tests/acp/test_tools.py @@ -147,7 +147,7 @@ class TestBuildToolTitle: class TestBuildToolStart: def test_build_tool_start_for_patch(self): - """patch should produce a FileEditToolCallContent (diff).""" + """patch start should not duplicate the edit-approval diff.""" args = { "path": "src/main.py", "old_string": "print('hello')", @@ -156,24 +156,23 @@ class TestBuildToolStart: result = build_tool_start("tc-1", "patch", args) assert isinstance(result, ToolCallStart) assert result.kind == "edit" - # The first content item should be a diff assert len(result.content) >= 1 - diff_item = result.content[0] - assert isinstance(diff_item, FileEditToolCallContent) - assert diff_item.path == "src/main.py" - assert diff_item.new_text == "print('world')" - assert diff_item.old_text == "print('hello')" + item = result.content[0] + assert isinstance(item, ContentToolCallContent) + assert "Approval prompt shows the diff" in item.content.text + assert "src/main.py" in item.content.text def test_build_tool_start_for_write_file(self): - """write_file should produce a FileEditToolCallContent (diff).""" + """write_file start should not duplicate the edit-approval diff.""" args = {"path": "new_file.py", "content": "print('hello')"} result = build_tool_start("tc-w1", "write_file", args) assert isinstance(result, ToolCallStart) assert result.kind == "edit" assert len(result.content) >= 1 - diff_item = result.content[0] - assert isinstance(diff_item, FileEditToolCallContent) - assert diff_item.path == "new_file.py" + item = result.content[0] + assert isinstance(item, ContentToolCallContent) + assert "Approval prompt shows the diff" in item.content.text + assert "new_file.py" in item.content.text def test_build_tool_start_for_terminal(self): """terminal should produce text content with the command.""" @@ -452,8 +451,8 @@ class TestBuildToolComplete: assert len(display_text) < 6000 assert "truncated" in display_text - def test_build_tool_complete_for_patch_uses_diff_blocks(self): - """Completed patch calls should keep structured diff content for Zed.""" + def test_build_tool_complete_for_patch_summarizes_without_repeating_diff(self): + """Completed patch calls should not duplicate the edit-approval diff.""" patch_result = ( '{"success": true, "diff": "--- a/README.md\\n+++ b/README.md\\n@@ -1 +1,2 @@\\n old line\\n+new line\\n", ' '"files_modified": ["README.md"]}' @@ -461,18 +460,17 @@ class TestBuildToolComplete: result = build_tool_complete("tc-p1", "patch", patch_result) assert isinstance(result, ToolCallProgress) assert len(result.content) == 1 - diff_item = result.content[0] - assert isinstance(diff_item, FileEditToolCallContent) - assert diff_item.path == "README.md" - assert diff_item.old_text == "old line" - assert diff_item.new_text == "old line\nnew line" + item = result.content[0] + assert isinstance(item, ContentToolCallContent) + assert "✅ patch completed" in item.content.text + assert "README.md" in item.content.text def test_build_tool_complete_for_patch_falls_back_to_text_when_no_diff(self): result = build_tool_complete("tc-p2", "patch", '{"success": true}') assert isinstance(result, ToolCallProgress) assert isinstance(result.content[0], ContentToolCallContent) - def test_build_tool_complete_for_write_file_uses_snapshot_diff(self, tmp_path): + def test_build_tool_complete_for_write_file_summarizes_without_repeating_diff(self, tmp_path): target = tmp_path / "diff-test.txt" snapshot = type("Snapshot", (), {"paths": [target], "before": {str(target): None}})() target.write_text("hello from hermes\n", encoding="utf-8") @@ -486,11 +484,10 @@ class TestBuildToolComplete: ) assert isinstance(result, ToolCallProgress) assert len(result.content) == 1 - diff_item = result.content[0] - assert isinstance(diff_item, FileEditToolCallContent) - assert diff_item.path.endswith("diff-test.txt") - assert diff_item.old_text is None - assert diff_item.new_text == "hello from hermes" + item = result.content[0] + assert isinstance(item, ContentToolCallContent) + assert "✅ write_file completed" in item.content.text + assert "diff-test.txt" in item.content.text # ---------------------------------------------------------------------------