feat: Add @codebot explain-diff command for plain-language PR explanations
All checks were successful
Enterprise AI Code Review / ai-review (pull_request) Successful in 39s
All checks were successful
Enterprise AI Code Review / ai-review (pull_request) Successful in 39s
Implements code diff explainer that translates technical changes into plain language for non-technical stakeholders (PMs, designers, new team members). Features: - Plain-language explanations without jargon - File-by-file breakdown with 'what' and 'why' context - Architecture impact analysis - Breaking change detection - Perfect for onboarding and cross-functional reviews Implementation: - Added explain_diff.md prompt template with plain-language guidelines - Implemented _handle_explain_diff_command() in PRAgent - Added _format_diff_explanation() for readable markdown - Updated PRAgent.can_handle() to route explain-diff commands - Added 'explain-diff' to config.yml commands list Workflow Safety (prevents duplicate runs): - Added '@codebot explain-diff' to ai-comment-reply.yml conditions - Excluded from ai-chat.yml to prevent duplication - Only triggers on PR comments (not issues) - Manual command only (no automatic triggering) Testing: - 9 comprehensive tests in TestDiffExplanation class - Tests command detection, formatting, plain-language output - Verifies prompt formatting and empty section handling Documentation: - Updated README.md with explain-diff command and examples - Added detailed implementation guide in CLAUDE.md - Included plain-language rules and use cases Related: Milestone 2 high-priority feature - code diff explainer
This commit is contained in:
@@ -1052,5 +1052,245 @@ class TestChangelogGeneration:
|
||||
assert "changelog" in config["interaction"]["commands"]
|
||||
|
||||
|
||||
class TestDiffExplanation:
|
||||
"""Test diff explanation functionality."""
|
||||
|
||||
def test_explain_diff_prompt_exists(self):
|
||||
"""Verify explain_diff.md prompt file exists."""
|
||||
prompt_path = os.path.join(
|
||||
os.path.dirname(__file__),
|
||||
"..",
|
||||
"tools",
|
||||
"ai-review",
|
||||
"prompts",
|
||||
"explain_diff.md",
|
||||
)
|
||||
assert os.path.exists(prompt_path), "explain_diff.md prompt file not found"
|
||||
|
||||
def test_explain_diff_prompt_formatting(self):
|
||||
"""Test that explain_diff.md can be formatted with placeholders."""
|
||||
prompt_path = os.path.join(
|
||||
os.path.dirname(__file__),
|
||||
"..",
|
||||
"tools",
|
||||
"ai-review",
|
||||
"prompts",
|
||||
"explain_diff.md",
|
||||
)
|
||||
with open(prompt_path) as f:
|
||||
prompt = f.read()
|
||||
|
||||
# Check for key elements
|
||||
assert "plain language" in prompt.lower() or "plain-language" in prompt.lower()
|
||||
assert "overview" in prompt.lower()
|
||||
assert "key changes" in prompt.lower() or "key_changes" in prompt.lower()
|
||||
assert "architecture" in prompt.lower()
|
||||
assert "JSON" in prompt
|
||||
|
||||
# Should be able to format with pr_title and pr_description
|
||||
try:
|
||||
formatted = prompt.format(
|
||||
pr_title="Test PR Title", pr_description="Test PR Description"
|
||||
)
|
||||
assert "Test PR Title" in formatted
|
||||
assert "Test PR Description" in formatted
|
||||
except KeyError as e:
|
||||
pytest.fail(f"Prompt has unescaped placeholders: {e}")
|
||||
|
||||
def test_pr_agent_can_handle_explain_diff_command(self):
|
||||
"""Test that PRAgent can handle @codebot explain-diff in PR comments."""
|
||||
from agents.pr_agent import PRAgent
|
||||
|
||||
config = {
|
||||
"agents": {
|
||||
"pr": {
|
||||
"enabled": True,
|
||||
}
|
||||
},
|
||||
"interaction": {
|
||||
"mention_prefix": "@codebot",
|
||||
},
|
||||
}
|
||||
|
||||
agent = PRAgent(config=config)
|
||||
|
||||
# Test explain-diff command in PR comment
|
||||
event_data = {
|
||||
"action": "created",
|
||||
"issue": {
|
||||
"number": 123,
|
||||
"pull_request": {}, # Indicates this is a PR
|
||||
},
|
||||
"comment": {
|
||||
"body": "@codebot explain-diff please",
|
||||
},
|
||||
}
|
||||
|
||||
assert agent.can_handle("issue_comment", event_data) is True
|
||||
|
||||
def test_pr_agent_can_handle_explain_diff_case_insensitive(self):
|
||||
"""Test that explain-diff command is case-insensitive."""
|
||||
from agents.pr_agent import PRAgent
|
||||
|
||||
config = {
|
||||
"agents": {
|
||||
"pr": {
|
||||
"enabled": True,
|
||||
}
|
||||
},
|
||||
"interaction": {
|
||||
"mention_prefix": "@codebot",
|
||||
},
|
||||
}
|
||||
|
||||
agent = PRAgent(config=config)
|
||||
|
||||
# Test various casings
|
||||
for body in [
|
||||
"@codebot EXPLAIN-DIFF",
|
||||
"@codebot Explain-Diff",
|
||||
"@codebot ExPlAiN-dIfF",
|
||||
]:
|
||||
event_data = {
|
||||
"action": "created",
|
||||
"issue": {
|
||||
"number": 123,
|
||||
"pull_request": {},
|
||||
},
|
||||
"comment": {"body": body},
|
||||
}
|
||||
assert agent.can_handle("issue_comment", event_data) is True
|
||||
|
||||
def test_pr_agent_ignores_explain_diff_on_non_pr(self):
|
||||
"""Test that explain-diff command is ignored on regular issues."""
|
||||
from agents.pr_agent import PRAgent
|
||||
|
||||
config = {
|
||||
"agents": {
|
||||
"pr": {
|
||||
"enabled": True,
|
||||
}
|
||||
},
|
||||
"interaction": {
|
||||
"mention_prefix": "@codebot",
|
||||
},
|
||||
}
|
||||
|
||||
agent = PRAgent(config=config)
|
||||
|
||||
# Regular issue (no pull_request field)
|
||||
event_data = {
|
||||
"action": "created",
|
||||
"issue": {
|
||||
"number": 123,
|
||||
# No pull_request field
|
||||
},
|
||||
"comment": {
|
||||
"body": "@codebot explain-diff",
|
||||
},
|
||||
}
|
||||
|
||||
assert agent.can_handle("issue_comment", event_data) is False
|
||||
|
||||
def test_format_diff_explanation_structure(self):
|
||||
"""Test _format_diff_explanation generates correct structure."""
|
||||
from agents.pr_agent import PRAgent
|
||||
|
||||
agent = PRAgent(config={})
|
||||
|
||||
explanation_data = {
|
||||
"overview": "This PR adds user authentication using JWT tokens",
|
||||
"key_changes": [
|
||||
{
|
||||
"file": "auth/jwt.py",
|
||||
"status": "new",
|
||||
"explanation": "Creates JSON Web Tokens for authenticated users",
|
||||
"why_it_matters": "Enables secure stateless authentication",
|
||||
},
|
||||
{
|
||||
"file": "api/users.py",
|
||||
"status": "modified",
|
||||
"explanation": "Added login endpoint",
|
||||
"why_it_matters": "Users can now authenticate",
|
||||
},
|
||||
],
|
||||
"architecture_impact": {
|
||||
"description": "Introduces authentication layer across all API endpoints",
|
||||
"new_dependencies": ["PyJWT", "bcrypt"],
|
||||
"affected_components": ["API", "Database"],
|
||||
},
|
||||
"breaking_changes": ["All API endpoints now require authentication"],
|
||||
"technical_details": {
|
||||
"files_changed": 5,
|
||||
"insertions": 200,
|
||||
"deletions": 10,
|
||||
"main_components": ["auth/", "api/"],
|
||||
},
|
||||
}
|
||||
|
||||
result = agent._format_diff_explanation(explanation_data, 123)
|
||||
|
||||
# Verify structure
|
||||
assert "## 📖 Code Changes Explained (PR #123)" in result
|
||||
assert "### 🎯 Overview" in result
|
||||
assert "user authentication using JWT tokens" in result
|
||||
assert "### 🔍 What Changed" in result
|
||||
assert "➕ `auth/jwt.py` (new)" in result
|
||||
assert "📝 `api/users.py` (modified)" in result
|
||||
assert "**What changed:**" in result
|
||||
assert "**Why it matters:**" in result
|
||||
assert "### 🏗️ Architecture Impact" in result
|
||||
assert "PyJWT" in result
|
||||
assert "### ⚠️ Breaking Changes" in result
|
||||
assert "All API endpoints now require authentication" in result
|
||||
assert "### 📊 Technical Summary" in result
|
||||
assert "Files changed:** 5" in result
|
||||
assert "+200 / -10" in result
|
||||
|
||||
def test_format_diff_explanation_empty_sections(self):
|
||||
"""Test that empty sections are not included in output."""
|
||||
from agents.pr_agent import PRAgent
|
||||
|
||||
agent = PRAgent(config={})
|
||||
|
||||
explanation_data = {
|
||||
"overview": "Small bugfix",
|
||||
"key_changes": [
|
||||
{
|
||||
"file": "app.py",
|
||||
"status": "modified",
|
||||
"explanation": "Fixed typo",
|
||||
"why_it_matters": "",
|
||||
}
|
||||
],
|
||||
"architecture_impact": {},
|
||||
"breaking_changes": [],
|
||||
"technical_details": {},
|
||||
}
|
||||
|
||||
result = agent._format_diff_explanation(explanation_data, 123)
|
||||
|
||||
# Only overview and key changes should be present
|
||||
assert "### 🎯 Overview" in result
|
||||
assert "### 🔍 What Changed" in result
|
||||
assert "### 🏗️ Architecture Impact" not in result
|
||||
assert "### ⚠️ Breaking Changes" not in result
|
||||
|
||||
def test_config_has_explain_diff_command(self):
|
||||
"""Verify config.yml has explain-diff command."""
|
||||
config_path = os.path.join(
|
||||
os.path.dirname(__file__), "..", "tools", "ai-review", "config.yml"
|
||||
)
|
||||
|
||||
import yaml
|
||||
|
||||
with open(config_path) as f:
|
||||
config = yaml.safe_load(f)
|
||||
|
||||
assert "interaction" in config
|
||||
assert "commands" in config["interaction"]
|
||||
assert "explain-diff" in config["interaction"]["commands"]
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
pytest.main([__file__, "-v"])
|
||||
|
||||
Reference in New Issue
Block a user