Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 32 additions & 61 deletions src/reviewbot/agent/tasks/issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from reviewbot.core.issues import IssueModel
from reviewbot.core.issues.issue_model import IssueModelList
from reviewbot.tools.diff import get_diff_from_file
from reviewbot.tools.read_file import read_file_from_store

console = Console()

Expand Down Expand Up @@ -371,6 +372,7 @@ async def quick_scan_file(

try:
diff_content = get_diff_from_file(agent.store, file_path)
file_content = read_file_from_store(agent.store, file_path)
except Exception as e:
console.print(f"[yellow]Could not fetch diff for {file_path}: {e}[/yellow]")
return True # If can't get diff, do deep review to be safe
Expand Down Expand Up @@ -399,11 +401,13 @@ async def quick_scan_file(
HumanMessage(
content=f"""Quickly scan this file and determine if it needs deep review: {file_path}

Here is the diff:

```diff
Here is the file:
{file_content}

Here is the diff:
{diff_content}
```"""
"""
),
]

Expand Down Expand Up @@ -482,32 +486,10 @@ async def review_single_file(
reasoning_context = get_reasoning_context(agent.store)

# Force a reasoning pass to ensure think() is invoked during deep review
try:
diff_content = get_diff_from_file(agent.store, file_path)
think_messages: list[BaseMessage] = [
SystemMessage(
content=(
"You are a senior code reviewer. You must think and review. "
"with 2-5 sentences of reasoning about the provided diff. "
"Do not use any other tools. Once finished, reply with the "
"single word DONE."
)
),
HumanMessage(
content=f"""Diff for {file_path}:

```diff
{diff_content}
```
""",
),
]
if model:
think_settings = ToolCallerSettings(max_tool_calls=40)
ido_agent = create_ido_agent(model=model, tools=tools or [])
await ido_agent.with_tool_caller(think_settings).ainvoke(think_messages)
except Exception as e:
console.print(f"[yellow]Failed to record reasoning for {file_path}: {e}[/yellow]")
diff_content = get_diff_from_file(agent.store, file_path)
file_content = read_file_from_store(agent.store, file_path)
if not file_content:
file_content = ""

messages: list[BaseMessage] = [
SystemMessage(
Expand Down Expand Up @@ -584,15 +566,11 @@ async def review_single_file(
HumanMessage(
content=f"""Review the merge request diff for the file: {file_path}

WORKFLOW:
1. Use `get_diff("{file_path}")` to get the diff
2. Analyze the changes for bugs, security issues, and logic errors
3. If you need context beyond the diff (imports, variable declarations, surrounding code):
- Use `read_file("{file_path}")` to see the complete file
4. Use `think()` to document your reasoning and analysis
5. Return your findings as a list of issues (or empty list if none)
File content:
{file_content}

Find the real bugs - that's what matters most!
Diff:
{diff_content}
"""
),
]
Expand Down Expand Up @@ -661,26 +639,19 @@ async def validate_issues_for_file(
messages: list[BaseMessage] = [
SystemMessage(
content=(
"You are an issue validator. Your job is to remove FALSE POSITIVES while keeping real bugs.\n\n"
"AVAILABLE TOOLS:\n"
"- `read_file(file_path)` - Read the complete file to verify issues\n"
"- `ls_dir(dir_path)` - List directory contents to verify file structure\n\n"
"WHAT TO REMOVE (false positives):\n"
"- 'Variable X undefined' - when X is actually defined elsewhere in the file\n"
"- 'Import Y missing' - when Y exists at the top of the file\n"
"- 'Function Z not declared' - when Z is defined in the complete file\n\n"
"WHAT TO KEEP (real issues):\n"
"- Logic errors - wrong conditions, broken algorithms, incorrect business logic\n"
"- Security vulnerabilities - SQL injection, XSS, auth bypass, etc.\n"
"- Bugs that will crash or produce wrong results\n"
"- Data corruption risks\n"
"- Performance problems\n\n"
"RULES:\n"
"- KEEP issues about logic, bugs, security, and functionality\n"
"- ONLY remove issues that are provably false (use read_file to verify)\n"
"- When in doubt, KEEP the issue - don't filter out real bugs\n"
"- Do NOT create new issues\n"
"- Do NOT modify issue fields"
"You are a strict Issue Validator filter. The project is ALREADY COMPILED AND RUNNING.\n\n"
"### CRITICAL CONTEXT\n"
"1. THE CODEBASE HAS ALREADY COMPILED AND BUILT SUCCESSFULLY.\n"
"2. If a file is deleted in a diff, it means the references were already cleaned up.\n"
"3. PROVISION: Any issue claiming 'compilation error', 'missing reference', or 'broken startup' is FACTUALLY WRONG.\n\n"
"### VALIDATION ARCHITECTURE\n"
"Your ONLY goal is to discard issues that assume the code is currently broken. "
"Since the build passed, the code is structurally sound. You are only looking for LOGIC bugs in NEW code.\n\n"
"### DISCARD IMMEDIATELY (False Positives):\n"
"- **Deletions:** Claims that deleting code/files will break the app (The build passed, so it didn't).\n"
"- **References:** Claims that a symbol is undefined (It is likely defined in a file you can't see).\n"
"- **Build/Runtime:** Any mention of 'compilation errors', 'build failures', or 'initialization failures'.\n"
"- **Assumptions:** Speculation about files outside the provided diff.\n\n"
)
),
HumanMessage(
Expand All @@ -695,17 +666,17 @@ async def validate_issues_for_file(
{json.dumps(issues_payload, indent=2)}

TASK:
1. For issues about "undefined/missing" code, use `read_file("{file_path}")` to check if the code actually exists elsewhere
2. Remove ONLY clear false positives
3. Keep all logic bugs, security issues, and real functionality problems
1. If the diff shows a file being DELETED, and the issue claims this deletion causes a failure elsewhere: DISCARD THE ISSUE.
2. The fact that the file was deleted and the project STILL COMPILED means the initialization logic was moved or is no longer necessary.
3. Validate only the logic within the lines starting with '+' (added).

Return a ValidationResult with:
- valid_issues: confirmed real issues
- removed: false positives with reason for removal"""
),
]

validation_settings = ToolCallerSettings(max_tool_calls=5) # Allow tool calls for validation
validation_settings = ToolCallerSettings(max_tool_calls=10) # Allow tool calls for validation

try:
if model is None:
Expand Down
40 changes: 0 additions & 40 deletions src/reviewbot/agent/workflow/diff_extract.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,46 +3,6 @@
from typing import Any


def _extract_code_from_diff(diff_text: str, line_start: int, line_end: int) -> str:
hunk_header_pattern = re.compile(r"^@@\s+-(\d+)(?:,(\d+))?\s+\+(\d+)(?:,(\d+))?\s+@@")
lines = diff_text.splitlines()

extracted = []
current_new = 0
in_hunk = False

for line in lines:
match = hunk_header_pattern.match(line)
if match:
current_new = int(match.group(3))
in_hunk = True
continue

if not in_hunk:
continue

# We only care about the lines in the NEW file (the result of the change)
if line.startswith("+"):
if line_start <= current_new <= line_end:
extracted.append(line[1:]) # Remove '+'
current_new += 1
elif line.startswith("-"):
# Skip deleted lines for code extraction of the 'new' state
continue
else:
# Context line
if line_start <= current_new <= line_end:
extracted.append(line[1:] if line else "")
current_new += 1

# FIX: Exit early if we've passed the end of our requested range
if current_new > line_end:
if extracted: # Only break if we actually found lines
break

return "\n".join(extracted)


def generate_line_code(file_path: str, old_line: int | None, new_line: int | None) -> str:
"""
Generates a GitLab-compatible line_code.
Expand Down
68 changes: 59 additions & 9 deletions src/reviewbot/agent/workflow/gitlab_notes.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from typing import Any, NamedTuple
from urllib.parse import quote

from ido_agents.agents.tool_runner import ToolCallerSettings
from langchain_core.language_models.chat_models import BaseChatModel
from langgraph.func import task # type: ignore
from rich.console import Console # type: ignore
Expand Down Expand Up @@ -28,7 +29,7 @@ class AcknowledgmentResult(NamedTuple):


@task
def post_review_acknowledgment(
async def post_review_acknowledgment(
*, gitlab: GitProviderConfig, diffs: list[FileDiff], model: BaseChatModel
) -> AcknowledgmentResult | None:
"""
Expand Down Expand Up @@ -147,11 +148,10 @@ def post_review_acknowledgment(
try:
# Get response with no tool calls allowed
from ido_agents.agents.ido_agent import create_ido_agent
from ido_agents.agents.tool_runner import ToolCallerSettings

summary_settings = ToolCallerSettings(max_tool_calls=0)
ido_agent = create_ido_agent(model=model, tools=[])
summary = ido_agent.with_tool_caller(summary_settings).invoke(messages)
summary = await ido_agent.with_tool_caller(summary_settings).ainvoke(messages)

# Post as a discussion (so we can update it later)
acknowledgment_body = f"""<img src="https://img.shields.io/badge/Code_Review-Starting-blue?style=flat-square" />
Expand Down Expand Up @@ -186,7 +186,8 @@ def post_review_acknowledgment(
return None


def update_review_summary(
@task
async def update_review_summary(
api_v4: str,
token: str,
project_id: str,
Expand Down Expand Up @@ -234,6 +235,43 @@ def update_review_summary(
total_files = len(diffs)
files_with_issues = len(issues_by_file)

# Build change overview for context
new_files: list[str] = []
deleted_files: list[str] = []
renamed_files: list[tuple[str, str]] = []
modified_files: list[str] = []

for diff in diffs:
if diff.is_renamed:
renamed_files.append((diff.old_path or "unknown", diff.new_path or "unknown"))
elif diff.is_new_file:
new_files.append(diff.new_path or diff.old_path or "unknown")
elif diff.is_deleted_file:
deleted_files.append(diff.old_path or diff.new_path or "unknown")
else:
modified_files.append(diff.new_path or diff.old_path or "unknown")

change_stats = (
"Files changed: "
f"{total_files} (new: {len(new_files)}, modified: {len(modified_files)}, "
f"renamed: {len(renamed_files)}, deleted: {len(deleted_files)})"
)
change_overview_lines: list[str] = []
change_overview_lines.extend(f"- {path} (new)" for path in new_files)
change_overview_lines.extend(f"- {path} (deleted)" for path in deleted_files)
change_overview_lines.extend(f"- {old} -> {new} (renamed)" for old, new in renamed_files)
change_overview_lines.extend(f"- {path} (modified)" for path in modified_files)

max_change_lines = 12
if len(change_overview_lines) > max_change_lines:
remaining = len(change_overview_lines) - max_change_lines
change_overview_lines = change_overview_lines[:max_change_lines]
change_overview_lines.append(f"- ... and {remaining} more file(s)")

change_overview_text = (
"\n".join(change_overview_lines) if change_overview_lines else "- No files listed."
)

# Prepare issue details for LLM
issues_summary: list[str] = []
for issue in issues:
Expand All @@ -256,7 +294,7 @@ def update_review_summary(
- Provide reasoning about the overall merge request purpose and code quality.
- Highlight key concerns or positive aspects
- Be constructive and professional
- DO NOT use any tools
- Use tools to generate a comprehensive summary
- Use paragraphs with readable flow. Use two paragrahs with 3-5 sentences.
Paragraphs should be wrapped with <p> tags. Use new <p> tag for a newline.
Example
Expand All @@ -267,7 +305,8 @@ def update_review_summary(
<p>
paragraph2
</p>
- Focus on the big picture, not individual issue details"""
- Focus on the big picture, not individual issue details
- Reference the changes overview so the summary stays grounded in what changed, even if there are no issues"""
),
HumanMessage(
content=f"""A code review has been completed with the following results:
Expand All @@ -280,24 +319,35 @@ def update_review_summary(
- Medium severity: {medium_count}
- Low severity: {low_count}

**Changes overview:**
{change_stats}
{change_overview_text}

**Issues found:**
{issues_text}

- Use EXACTLY two paragraphs, each wrapped in <p> tags.
1. Provides overall assessment of the purpose of the merge request purpose and code quality.
2. Highlights the most important concerns (if any)
3. Gives reasoning about the review findings
4. Is constructive and actionable """
4. Is constructive and actionable
5. Mention the kinds of changes and at least one example file from the changes overview """
),
]

if model is None:
raise ValueError("model parameter is required for ido-agents migration")

ido_agent = create_ido_agent(model=model, tools=tools or [])
llm_summary = ido_agent.with_structured_output(ReviewSummary).invoke(messages)

llm_summary = str(llm_summary)
summary_settings = ToolCallerSettings(max_tool_calls=5)
llm_summary = await (
ido_agent.with_structured_output(ReviewSummary)
.with_tool_caller(summary_settings)
.ainvoke(messages)
)

llm_summary = llm_summary.summary if llm_summary else "Review completed successfully."

except Exception as e:
console.print(f"[yellow]Failed to generate LLM summary: {e}[/yellow]")
Expand Down
10 changes: 3 additions & 7 deletions src/reviewbot/core/reviews/review_model.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
from pydantic import RootModel
from pydantic import BaseModel


class ReviewSummary(RootModel[str]):
def __str__(self) -> str:
return self.root

def __len__(self) -> int:
return len(self.root)
class ReviewSummary(BaseModel):
summary: str
Loading