diff --git a/src/reviewbot/agent/tasks/issues.py b/src/reviewbot/agent/tasks/issues.py index 26e6b2c..1638b55 100644 --- a/src/reviewbot/agent/tasks/issues.py +++ b/src/reviewbot/agent/tasks/issues.py @@ -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() @@ -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 @@ -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} -```""" +""" ), ] @@ -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( @@ -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} """ ), ] @@ -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( @@ -695,9 +666,9 @@ 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 @@ -705,7 +676,7 @@ async def validate_issues_for_file( ), ] - 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: diff --git a/src/reviewbot/agent/workflow/diff_extract.py b/src/reviewbot/agent/workflow/diff_extract.py index 73a76d7..18c9b40 100644 --- a/src/reviewbot/agent/workflow/diff_extract.py +++ b/src/reviewbot/agent/workflow/diff_extract.py @@ -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. diff --git a/src/reviewbot/agent/workflow/gitlab_notes.py b/src/reviewbot/agent/workflow/gitlab_notes.py index 262f6d2..2bf7bcc 100644 --- a/src/reviewbot/agent/workflow/gitlab_notes.py +++ b/src/reviewbot/agent/workflow/gitlab_notes.py @@ -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 @@ -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: """ @@ -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""" @@ -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, @@ -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: @@ -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

tags. Use new

tag for a newline. Example @@ -267,7 +305,8 @@ def update_review_summary(

paragraph2

-- 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: @@ -280,6 +319,10 @@ def update_review_summary( - Medium severity: {medium_count} - Low severity: {low_count} +**Changes overview:** +{change_stats} +{change_overview_text} + **Issues found:** {issues_text} @@ -287,7 +330,8 @@ def update_review_summary( 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 """ ), ] @@ -295,9 +339,15 @@ def update_review_summary( 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]") diff --git a/src/reviewbot/core/reviews/review_model.py b/src/reviewbot/core/reviews/review_model.py index 5cbc44a..015c238 100644 --- a/src/reviewbot/core/reviews/review_model.py +++ b/src/reviewbot/core/reviews/review_model.py @@ -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 diff --git a/src/reviewbot/tools/read_file.py b/src/reviewbot/tools/read_file.py index 46b4174..26a8e87 100644 --- a/src/reviewbot/tools/read_file.py +++ b/src/reviewbot/tools/read_file.py @@ -1,6 +1,7 @@ from pathlib import Path from langchain.tools import ToolRuntime, tool # type: ignore +from langgraph.func import BaseStore # type: ignore from rich.console import Console from reviewbot.agent.workflow.state import CodebaseState @@ -8,43 +9,19 @@ console = Console() -@tool -def read_file( +def read_file_from_store( + store: BaseStore, file_path: str, - runtime: ToolRuntime, # type: ignore line_start: int | None = None, line_end: int | None = None, ) -> str: - """Read the contents of a file from the repository. - - Use this tool to get the full context of a file when the diff alone is not sufficient - to understand the code. This helps avoid false positives by seeing the complete picture. - - Args: - file_path: Relative path to the file in the repository (e.g., "src/main.go") - line_start: Optional line number to start reading from (1-indexed) - line_end: Optional line number to stop reading at (inclusive) - - Returns: - The file contents, optionally limited to the specified line range - - Examples: - - read_file("src/main.go") - Read entire file - - read_file("src/main.go", line_start=10, line_end=50) - Read lines 10-50 - - Note: - Returns an error message if the file is newly added (doesn't exist in current checkout) - """ - if runtime.store is None: - console.print("[red]read_file: Store not found in runtime[/red]") - raise ValueError("Store not found in runtime") - + """Read file contents using the codebase state stored in the runtime store.""" line_range = f" (lines {line_start}-{line_end})" if line_start or line_end else "" console.print(f"[cyan]read_file: '{file_path}'{line_range}[/cyan]") # Get codebase state from store NS = ("codebase",) - raw = runtime.store.get(NS, "state") + raw = store.get(NS, "state") if not raw: console.print("[red]read_file: Codebase state not found in store[/red]") raise ValueError("Codebase state not found in store") @@ -174,3 +151,42 @@ def read_file( console.print(f"[dim] → Message: {error_msg}[/dim]") raise ValueError(error_msg) from e + + +@tool +def read_file( + file_path: str, + runtime: ToolRuntime, # type: ignore + line_start: int | None = None, + line_end: int | None = None, +) -> str: + """Read the contents of a file from the repository. + + Use this tool to get the full context of a file when the diff alone is not sufficient + to understand the code. This helps avoid false positives by seeing the complete picture. + + Args: + file_path: Relative path to the file in the repository (e.g., "src/main.go") + line_start: Optional line number to start reading from (1-indexed) + line_end: Optional line number to stop reading at (inclusive) + + Returns: + The file contents, optionally limited to the specified line range + + Examples: + - read_file("src/main.go") - Read entire file + - read_file("src/main.go", line_start=10, line_end=50) - Read lines 10-50 + + Note: + Returns an error message if the file is newly added (doesn't exist in current checkout) + """ + if runtime.store is None: + console.print("[red]read_file: Store not found in runtime[/red]") + raise ValueError("Store not found in runtime") + + return read_file_from_store( + runtime.store, + file_path, + line_start=line_start, + line_end=line_end, + )