feat: add inline diff previews for write actions
Show inline diffs in the CLI transcript when write_file, patch, or skill_manage modifies files. Captures a filesystem snapshot before the tool runs, computes a unified diff after, and renders it with ANSI coloring in the activity feed. Adds tool_start_callback and tool_complete_callback hooks to AIAgent for pre/post tool execution notifications. Also fixes _extract_parallel_scope_path to normalize relative paths to absolute, preventing the parallel overlap detection from missing conflicts when the same file is referenced with different path styles. Gated by display.inline_diffs config option (default: true). Based on PR #3774 by @kshitijk4poor.
This commit is contained in:
kshitijk4poor
@@ -1,7 +1,17 @@
|
||||
"""Tests for agent/display.py — build_tool_preview()."""
|
||||
"""Tests for agent/display.py — build_tool_preview() and inline diff previews."""
|
||||
|
||||
import os
|
||||
import pytest
|
||||
from agent.display import build_tool_preview
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
from agent.display import (
|
||||
build_tool_preview,
|
||||
capture_local_edit_snapshot,
|
||||
extract_edit_diff,
|
||||
_render_inline_unified_diff,
|
||||
_summarize_rendered_diff_sections,
|
||||
render_edit_diff_with_delta,
|
||||
)
|
||||
|
||||
|
||||
class TestBuildToolPreview:
|
||||
@@ -83,3 +93,110 @@ class TestBuildToolPreview:
|
||||
assert build_tool_preview("terminal", 0) is None
|
||||
assert build_tool_preview("terminal", "") is None
|
||||
assert build_tool_preview("terminal", []) is None
|
||||
|
||||
|
||||
class TestEditDiffPreview:
|
||||
def test_extract_edit_diff_for_patch(self):
|
||||
diff = extract_edit_diff("patch", '{"success": true, "diff": "--- a/x\\n+++ b/x\\n"}')
|
||||
assert diff is not None
|
||||
assert "+++ b/x" in diff
|
||||
|
||||
def test_render_inline_unified_diff_colors_added_and_removed_lines(self):
|
||||
rendered = _render_inline_unified_diff(
|
||||
"--- a/cli.py\n"
|
||||
"+++ b/cli.py\n"
|
||||
"@@ -1,2 +1,2 @@\n"
|
||||
"-old line\n"
|
||||
"+new line\n"
|
||||
" context\n"
|
||||
)
|
||||
|
||||
assert "a/cli.py" in rendered[0]
|
||||
assert "b/cli.py" in rendered[0]
|
||||
assert any("old line" in line for line in rendered)
|
||||
assert any("new line" in line for line in rendered)
|
||||
assert any("48;2;" in line for line in rendered)
|
||||
|
||||
def test_extract_edit_diff_ignores_non_edit_tools(self):
|
||||
assert extract_edit_diff("web_search", '{"diff": "--- a\\n+++ b\\n"}') is None
|
||||
|
||||
def test_extract_edit_diff_uses_local_snapshot_for_write_file(self, tmp_path):
|
||||
target = tmp_path / "note.txt"
|
||||
target.write_text("old\n", encoding="utf-8")
|
||||
|
||||
snapshot = capture_local_edit_snapshot("write_file", {"path": str(target)})
|
||||
|
||||
target.write_text("new\n", encoding="utf-8")
|
||||
|
||||
diff = extract_edit_diff(
|
||||
"write_file",
|
||||
'{"bytes_written": 4}',
|
||||
function_args={"path": str(target)},
|
||||
snapshot=snapshot,
|
||||
)
|
||||
|
||||
assert diff is not None
|
||||
assert "--- a/" in diff
|
||||
assert "+++ b/" in diff
|
||||
assert "-old" in diff
|
||||
assert "+new" in diff
|
||||
|
||||
def test_render_edit_diff_with_delta_invokes_printer(self):
|
||||
printer = MagicMock()
|
||||
|
||||
rendered = render_edit_diff_with_delta(
|
||||
"patch",
|
||||
'{"diff": "--- a/x\\n+++ b/x\\n@@ -1 +1 @@\\n-old\\n+new\\n"}',
|
||||
print_fn=printer,
|
||||
)
|
||||
|
||||
assert rendered is True
|
||||
assert printer.call_count >= 2
|
||||
calls = [call.args[0] for call in printer.call_args_list]
|
||||
assert any("a/x" in line and "b/x" in line for line in calls)
|
||||
assert any("old" in line for line in calls)
|
||||
assert any("new" in line for line in calls)
|
||||
|
||||
def test_render_edit_diff_with_delta_skips_without_diff(self):
|
||||
rendered = render_edit_diff_with_delta(
|
||||
"patch",
|
||||
'{"success": true}',
|
||||
)
|
||||
|
||||
assert rendered is False
|
||||
|
||||
def test_render_edit_diff_with_delta_handles_renderer_errors(self, monkeypatch):
|
||||
printer = MagicMock()
|
||||
|
||||
monkeypatch.setattr("agent.display._summarize_rendered_diff_sections", MagicMock(side_effect=RuntimeError("boom")))
|
||||
|
||||
rendered = render_edit_diff_with_delta(
|
||||
"patch",
|
||||
'{"diff": "--- a/x\\n+++ b/x\\n"}',
|
||||
print_fn=printer,
|
||||
)
|
||||
|
||||
assert rendered is False
|
||||
assert printer.call_count == 0
|
||||
|
||||
def test_summarize_rendered_diff_sections_truncates_large_diff(self):
|
||||
diff = "--- a/x.py\n+++ b/x.py\n" + "".join(f"+line{i}\n" for i in range(120))
|
||||
|
||||
rendered = _summarize_rendered_diff_sections(diff, max_lines=20)
|
||||
|
||||
assert len(rendered) == 21
|
||||
assert "omitted" in rendered[-1]
|
||||
|
||||
def test_summarize_rendered_diff_sections_limits_file_count(self):
|
||||
diff = "".join(
|
||||
f"--- a/file{i}.py\n+++ b/file{i}.py\n+line{i}\n"
|
||||
for i in range(8)
|
||||
)
|
||||
|
||||
rendered = _summarize_rendered_diff_sections(diff, max_files=3, max_lines=50)
|
||||
|
||||
assert any("a/file0.py" in line for line in rendered)
|
||||
assert any("a/file1.py" in line for line in rendered)
|
||||
assert any("a/file2.py" in line for line in rendered)
|
||||
assert not any("a/file7.py" in line for line in rendered)
|
||||
assert "additional file" in rendered[-1]
|
||||
|
||||
@@ -1239,6 +1239,42 @@ class TestConcurrentToolExecution:
|
||||
)
|
||||
assert result == "result"
|
||||
|
||||
def test_sequential_tool_callbacks_fire_in_order(self, agent):
|
||||
tool_call = _mock_tool_call(name="web_search", arguments='{"query":"hello"}', call_id="c1")
|
||||
mock_msg = _mock_assistant_msg(content="", tool_calls=[tool_call])
|
||||
messages = []
|
||||
starts = []
|
||||
completes = []
|
||||
agent.tool_start_callback = lambda tool_call_id, function_name, function_args: starts.append((tool_call_id, function_name, function_args))
|
||||
agent.tool_complete_callback = lambda tool_call_id, function_name, function_args, function_result: completes.append((tool_call_id, function_name, function_args, function_result))
|
||||
|
||||
with patch("run_agent.handle_function_call", return_value='{"success": true}'):
|
||||
agent._execute_tool_calls_sequential(mock_msg, messages, "task-1")
|
||||
|
||||
assert starts == [("c1", "web_search", {"query": "hello"})]
|
||||
assert completes == [("c1", "web_search", {"query": "hello"}, '{"success": true}')]
|
||||
|
||||
def test_concurrent_tool_callbacks_fire_for_each_tool(self, agent):
|
||||
tc1 = _mock_tool_call(name="web_search", arguments='{"query":"one"}', call_id="c1")
|
||||
tc2 = _mock_tool_call(name="web_search", arguments='{"query":"two"}', call_id="c2")
|
||||
mock_msg = _mock_assistant_msg(content="", tool_calls=[tc1, tc2])
|
||||
messages = []
|
||||
starts = []
|
||||
completes = []
|
||||
agent.tool_start_callback = lambda tool_call_id, function_name, function_args: starts.append((tool_call_id, function_name, function_args))
|
||||
agent.tool_complete_callback = lambda tool_call_id, function_name, function_args, function_result: completes.append((tool_call_id, function_name, function_args, function_result))
|
||||
|
||||
with patch("run_agent.handle_function_call", side_effect=['{"id":1}', '{"id":2}']):
|
||||
agent._execute_tool_calls_concurrent(mock_msg, messages, "task-1")
|
||||
|
||||
assert starts == [
|
||||
("c1", "web_search", {"query": "one"}),
|
||||
("c2", "web_search", {"query": "two"}),
|
||||
]
|
||||
assert len(completes) == 2
|
||||
assert {entry[0] for entry in completes} == {"c1", "c2"}
|
||||
assert {entry[3] for entry in completes} == {'{"id":1}', '{"id":2}'}
|
||||
|
||||
def test_invoke_tool_handles_agent_level_tools(self, agent):
|
||||
"""_invoke_tool should handle todo tool directly."""
|
||||
with patch("tools.todo_tool.todo_tool", return_value='{"ok":true}') as mock_todo:
|
||||
@@ -1280,6 +1316,38 @@ class TestPathsOverlap:
|
||||
assert not _paths_overlap(Path("src/a.py"), Path(""))
|
||||
|
||||
|
||||
class TestParallelScopePathNormalization:
|
||||
def test_extract_parallel_scope_path_normalizes_relative_to_cwd(self, tmp_path, monkeypatch):
|
||||
from run_agent import _extract_parallel_scope_path
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
scoped = _extract_parallel_scope_path("write_file", {"path": "./notes.txt"})
|
||||
|
||||
assert scoped == tmp_path / "notes.txt"
|
||||
|
||||
def test_extract_parallel_scope_path_treats_relative_and_absolute_same_file_as_same_scope(self, tmp_path, monkeypatch):
|
||||
from run_agent import _extract_parallel_scope_path, _paths_overlap
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
abs_path = tmp_path / "notes.txt"
|
||||
|
||||
rel_scoped = _extract_parallel_scope_path("write_file", {"path": "notes.txt"})
|
||||
abs_scoped = _extract_parallel_scope_path("write_file", {"path": str(abs_path)})
|
||||
|
||||
assert rel_scoped == abs_scoped
|
||||
assert _paths_overlap(rel_scoped, abs_scoped)
|
||||
|
||||
def test_should_parallelize_tool_batch_rejects_same_file_with_mixed_path_spellings(self, tmp_path, monkeypatch):
|
||||
from run_agent import _should_parallelize_tool_batch
|
||||
|
||||
monkeypatch.chdir(tmp_path)
|
||||
tc1 = _mock_tool_call(name="write_file", arguments='{"path":"notes.txt","content":"one"}', call_id="c1")
|
||||
tc2 = _mock_tool_call(name="write_file", arguments=f'{{"path":"{tmp_path / "notes.txt"}","content":"two"}}', call_id="c2")
|
||||
|
||||
assert not _should_parallelize_tool_batch([tc1, tc2])
|
||||
|
||||
|
||||
class TestHandleMaxIterations:
|
||||
def test_returns_summary(self, agent):
|
||||
resp = _mock_response(content="Here is a summary of what I did.")
|
||||
|
||||
Reference in New Issue
Block a user