From 6e17916c6b8628d1a3898a37f507583283778a85 Mon Sep 17 00:00:00 2001 From: Glenn Jocher Date: Sat, 4 Jul 2026 14:41:41 +0200 Subject: [PATCH] Add iterative PR review agent tools (#786) --- actions/review_pr.py | 306 +++++++++++++++++++++++++++++--- actions/utils/__init__.py | 2 + actions/utils/openai_utils.py | 260 +++++++++++++++++++++++++-- tests/test_first_interaction.py | 88 ++++++++- tests/test_github_report.py | 13 +- tests/test_openai_utils.py | 264 +++++++++++++++++++++++++++ 6 files changed, 882 insertions(+), 51 deletions(-) diff --git a/actions/review_pr.py b/actions/review_pr.py index d4fb005..93b1830 100644 --- a/actions/review_pr.py +++ b/actions/review_pr.py @@ -4,30 +4,265 @@ from __future__ import annotations import json import re +from fnmatch import fnmatch from pathlib import Path from .utils import ( ACTIONS_CREDIT, + COMMON_EXCLUDED_DIRS, DIFF_FILE_PATTERN, GITHUB_API_URL, MAX_PROMPT_CHARS, Action, format_skipped_files_dropdown, - get_response, + get_agent_response, get_review_model, remove_html_comments, sanitize_ai_text, should_skip_file, ) +from .utils.openai_utils import _is_anthropic_model REVIEW_MARKER = "## 🔍 PR Review" ERROR_MARKER = "⚠️ Review generation encountered an error" EMOJI_MAP = {"CRITICAL": "❗", "HIGH": "⚠️", "MEDIUM": "💡", "LOW": "📝", "SUGGESTION": "💭"} MAX_CONTEXT_FILE_CHARS = 5000 MAX_REVIEW_COMMENTS = 8 +MAX_TOOL_OUTPUT_CHARS = 20000 +MAX_TOOL_FILE_LINES = 240 +MAX_AGENT_TURNS = 16 +MAX_REVIEW_COST = 2.00 # USD ceiling per review across all agent turns SEVERITY_RANK = {"CRITICAL": 0, "HIGH": 1, "MEDIUM": 2, "LOW": 3, "SUGGESTION": 4, None: 5} +def _clip_tool_output(text: str, limit: int = MAX_TOOL_OUTPUT_CHARS) -> str: + """Limit model-facing tool output size.""" + return text if len(text) <= limit else f"{text[:limit].rstrip()}\n... (truncated)" + + +def _resolve_repo_path(path: str) -> Path: + """Resolve a repo-relative path without allowing access outside the checkout.""" + root = Path.cwd().resolve() + target = (root / (path or ".")).resolve() + try: + target.relative_to(root) + except ValueError as e: + raise ValueError(f"path must stay inside repository: {path}") from e + return target + + +def read_file(path: str, start_line=None, end_line=None) -> str: + """Read a bounded range from a repository file for agent review context.""" + try: + target = _resolve_repo_path(path) + except ValueError as e: + return str(e) + rel = target.relative_to(Path.cwd().resolve()).as_posix() + if not target.is_file(): + return f"{path} is not a file." + if should_skip_file(rel) or target.stat().st_size > 500_000: + return f"{path} is skipped because it is generated, vendored, or too large." + + lines = target.read_text(encoding="utf-8", errors="ignore").splitlines() + start = max(1, int(start_line or 1)) + end = min(len(lines), int(end_line or len(lines)), start + MAX_TOOL_FILE_LINES - 1) + if end < start: + return f"{path} has no lines in requested range {start}-{end}." + numbered = "\n".join(f"{i:>5}: {lines[i - 1]}" for i in range(start, end + 1)) + return _clip_tool_output(f"{rel}:{start}-{end}\n{numbered}") + + +def _iter_repo_files(path_glob=None): + """Yield repository files, including hidden files, pruning vendored dirs and paths outside the checkout.""" + root = Path.cwd().resolve() + stack = [root] + while stack: + try: + children = list(stack.pop().iterdir()) + except OSError: + continue + for path in children: + if path.is_dir(): + if path.name not in COMMON_EXCLUDED_DIRS and not path.is_symlink(): + stack.append(path) + continue + rel = path.relative_to(root).as_posix() + if (path_glob and not fnmatch(rel, path_glob)) or should_skip_file(rel): + continue + try: + target = path.resolve() + target.relative_to(root) + except (OSError, ValueError): + continue + if target.is_file(): + yield target, rel + + +def list_files(path_glob=None) -> str: + """List repository files matching an optional glob.""" + files = [rel for _, rel in _iter_repo_files(path_glob)] + if not files: + return "No matching files found." + return _clip_tool_output("\n".join(sorted(files)[:300])) + + +def search_repo(query: str, path_glob=None) -> str: + """Search repository text for agent review context.""" + if not query: + return "query is required." + matches = [] + for target, rel in _iter_repo_files(path_glob): + if target.stat().st_size > 500_000: + continue + for line_no, line in enumerate(target.read_text(encoding="utf-8", errors="ignore").splitlines(), 1): + if query in line: + matches.append(f"{rel}:{line_no}:{line}") + if len(matches) >= 200: + return _clip_tool_output("\n".join(matches)) + return _clip_tool_output("\n".join(matches)) if matches else "No matches found." + + +def _split_augmented_diff_by_file(augmented_diff: str) -> dict[str, list[str]]: + """Split an augmented diff into per-file chunks.""" + chunks, current_file = {}, None + for line in augmented_diff.splitlines(): + if line.startswith("diff --git"): + match = DIFF_FILE_PATTERN.search(line) + current_file = match.group(1).rstrip('"') if match else None + if current_file: + chunks[current_file] = [] + if current_file: + chunks[current_file].append(line) + return chunks + + +def build_review_agent_tools(diff_files: dict | None = None, augmented_diff: str = "") -> tuple[list[dict], dict]: + """Build read-only tools for the PR review agent.""" + diff_files = diff_files or {} + diff_chunks = _split_augmented_diff_by_file(augmented_diff) + + def list_changed_files(path_glob=None) -> str: + """List changed files in this PR with added/removed line counts.""" + rows = [] + for path in sorted(diff_files): + if path_glob and not fnmatch(path, path_glob): + continue + sides = diff_files[path] + rows.append(f"{path} (+{len(sides['RIGHT'])}/-{len(sides['LEFT'])})") + return _clip_tool_output("\n".join(rows)) if rows else "No changed files found." + + def read_diff(path: str, start_line=None, end_line=None) -> str: + """Read the line-numbered augmented diff for one changed file.""" + if path not in diff_chunks: + return f"{path} is not in the changed-file diff." + lines = diff_chunks[path] + start = max(1, int(start_line or 1)) + end = min(len(lines), int(end_line or len(lines))) + if end < start: + return f"{path} has no diff lines in requested range {start}-{end}." + return _clip_tool_output(f"{path} diff:{start}-{end}\n" + "\n".join(lines[start - 1 : end])) + + tools = [ + {"type": "web_search"}, + { + "type": "function", + "name": "list_changed_files", + "description": ( + "List every changed file in the PR with added/removed line counts. Use this when the PR has many " + "files or the initial diff is truncated so every changed file can be considered." + ), + "parameters": { + "type": "object", + "properties": { + "path_glob": {"type": ["string", "null"], "description": "Optional changed-file glob, or null."}, + }, + "required": ["path_glob"], + "additionalProperties": False, + }, + "strict": True, + }, + { + "type": "function", + "name": "read_diff", + "description": ( + "Read the line-numbered augmented diff for one changed file. Use this to inspect changed hunks that " + "were not included in the initial prompt or to recover exact R/L line numbers for inline comments." + ), + "parameters": { + "type": "object", + "properties": { + "path": {"type": "string", "description": "Changed file path from list_changed_files."}, + "start_line": {"type": ["integer", "null"], "description": "1-based diff output line, or null."}, + "end_line": {"type": ["integer", "null"], "description": "1-based diff output line, or null."}, + }, + "required": ["path", "start_line", "end_line"], + "additionalProperties": False, + }, + "strict": True, + }, + { + "type": "function", + "name": "read_file", + "description": ( + "Read a bounded line range from a repository file in the checked-out PR head, including unchanged " + "files such as pyproject.toml, tests, configs, and shared helpers. Use this to verify changed code or " + "nearby definitions before making a review finding." + ), + "parameters": { + "type": "object", + "properties": { + "path": {"type": "string", "description": "Repository-relative file path."}, + "start_line": {"type": ["integer", "null"], "description": "1-based first line, or null."}, + "end_line": {"type": ["integer", "null"], "description": "1-based last line, or null."}, + }, + "required": ["path", "start_line", "end_line"], + "additionalProperties": False, + }, + "strict": True, + }, + { + "type": "function", + "name": "search_repo", + "description": ( + "Search the checked-out repository. Use focused literal strings to find related " + "definitions, dependencies, tests, config, or prior patterns before deciding whether a diff hunk is " + "wrong." + ), + "parameters": { + "type": "object", + "properties": { + "query": {"type": "string", "description": "Literal string to search for."}, + "path_glob": {"type": ["string", "null"], "description": "Optional glob, or null."}, + }, + "required": ["query", "path_glob"], + "additionalProperties": False, + }, + "strict": True, + }, + { + "type": "function", + "name": "list_files", + "description": "List repository files matching an optional glob when you need to locate related files.", + "parameters": { + "type": "object", + "properties": { + "path_glob": {"type": ["string", "null"], "description": "Optional glob, or null."}, + }, + "required": ["path_glob"], + "additionalProperties": False, + }, + "strict": True, + }, + ] + return tools, { + "list_changed_files": list_changed_files, + "read_diff": read_diff, + "read_file": read_file, + "search_repo": search_repo, + "list_files": list_files, + } + + def get_repo_guidelines(model: str = "") -> str: """Read guidelines from the repository root if they exist (one agent file + CONTRIBUTING.md).""" guidelines = [] @@ -121,11 +356,12 @@ def generate_pr_review( # Read model-appropriate guidelines from repo root for project-specific review context review_model = get_review_model() + is_agent_review_model = not _is_anthropic_model(review_model) guidelines_section = get_repo_guidelines(review_model) # Fetch full file contents for better context if within token budget full_files_section = "" - if event and len(file_list) <= 10: # Reasonable file count limit + if event and not is_agent_review_model and len(file_list) <= 10: # Reasonable file count limit file_contents, total_chars = [], len(augmented_diff) + len(guidelines_section) for file_path in file_list: try: @@ -155,6 +391,40 @@ def generate_pr_review( # Calculate remaining budget for diff and check if truncation needed diff_budget = max(1000, MAX_PROMPT_CHARS - len(guidelines_section) - len(full_files_section)) diff_truncated = len(augmented_diff) > diff_budget + is_large_pr = diff_truncated or len(file_list) > 30 + if is_agent_review_model: # must match the get_agent_response fallback gate + visibility_section = ( + "CONTEXT AND VERIFICATION:\n" + "- Start from the diff, then use tools to inspect full files or related code when context affects a finding\n" + "- Verify suspected issues before commenting: read the enclosing function, and check definitions, callers, and existing patterns\n" + "- Assume the author is knowledgeable about new package versions, dependencies, and codebase architecture unless tool evidence proves otherwise\n" + "- Do NOT flag version updates; check imports or references to code outside the diff with tools instead of flagging them\n" + "- If still unsure after checking, assume the author knows what they're doing\n" + "- If PROJECT GUIDELINES (CLAUDE.md/AGENTS.md) are provided, respect project-specific conventions and standards\n\n" + "AVAILABLE TOOLS:\n" + "- list_changed_files: list every changed file in the PR with added/removed counts; use this for large " + "PRs or when the initial diff is truncated\n" + "- read_diff: inspect the line-numbered diff for a changed file; use this to review files not visible in " + "the initial prompt and to recover exact R/L line numbers for comments\n" + "- read_file: inspect bounded line ranges from repository files, including unchanged files; the checkout is " + "the PR head, so file line numbers match RIGHT-side diff line numbers\n" + "- search_repo: find related definitions, dependencies, tests, config, or existing patterns across the checkout\n" + "- list_files: locate repository files by glob\n" + "- web_search: verify current public docs, package docs, release notes, issues, or vendor behavior when a " + "review finding depends on external behavior\n" + "- Tool turns and cost are budgeted: batch independent tool calls in the same turn, e.g. read several " + "diffs or files at once\n" + "- Do not quote large tool output. Use tools only to verify concise, actionable review findings.\n\n" + ) + else: + visibility_section = ( + "LIMITED VISIBILITY - IMPORTANT:\n" + "- You can only see the diff and partial file contents, not the full codebase\n" + "- Assume the author is knowledgeable about: new package versions, imports to functions defined elsewhere, dependencies, and codebase architecture\n" + "- Do NOT flag: version updates, new imports that appear unused in the diff, or references to code outside the diff\n" + "- If unsure whether something is an error, assume the author knows what they're doing\n" + "- If PROJECT GUIDELINES (CLAUDE.md/AGENTS.md) are provided, respect project-specific conventions and standards\n\n" + ) content = ( "You are an expert code reviewer for Ultralytics. Review code changes and provide inline comments ONLY for genuine issues.\n\n" @@ -169,12 +439,7 @@ def generate_pr_review( "- Minor naming preferences\n" "- 'Consider using X' without clear benefit\n" "- Issues in unchanged context lines\n\n" - "LIMITED VISIBILITY - IMPORTANT:\n" - "- You can only see the diff and partial file contents, not the full codebase\n" - "- Assume the author is knowledgeable about: new package versions, imports to functions defined elsewhere, dependencies, and codebase architecture\n" - "- Do NOT flag: version updates, new imports that appear unused in the diff, or references to code outside the diff\n" - "- If unsure whether something is an error, assume the author knows what they're doing\n" - "- If PROJECT GUIDELINES (CLAUDE.md/AGENTS.md) are provided, respect project-specific conventions and standards\n\n" + f"{visibility_section}" "QUALITY OVER QUANTITY:\n" "- Zero comments is valid for clean PRs - don't invent issues\n" "- Each comment must be actionable with clear reasoning\n" @@ -195,11 +460,12 @@ def generate_pr_review( " context <- no prefix = unchanged context, don't comment on these\n" "- Extract the integer after R or L prefix (e.g., 'R 123' -> line 123, 'L 45' -> line 45)\n" "- Suggestions ONLY work on RIGHT (added) lines, never LEFT (removed) lines\n" - "- ONLY use line numbers you see explicitly prefixed with R or L in the diff\n\n" + "- ONLY use line numbers you see explicitly prefixed with R or L in the initial diff or read_diff output\n\n" "Return JSON: " '{"comments": [{"file": "exact/path", "line": N, "side": "RIGHT", "severity": "HIGH", "message": "..."}], "summary": "..."}\n\n' "JSON rules: exact paths (no ./), severity: CRITICAL|HIGH|MEDIUM|LOW|SUGGESTION\n" f"Files changed: {len(file_list)} ({', '.join(file_list[:30])}{'...' if len(file_list) > 30 else ''}), Lines: {lines_changed}\n" + f"{'Large or truncated PR: use list_changed_files and read_diff to inspect changed files not shown in the initial prompt. ' if is_large_pr else ''}\n" ) messages = [ @@ -250,23 +516,17 @@ def generate_pr_review( "additionalProperties": False, } - response = get_response( + tools, tool_handlers = build_review_agent_tools(diff_files, augmented_diff) + response = get_agent_response( messages, text_format={"format": {"type": "json_schema", "name": "pr_review", "strict": True, "schema": schema}}, model=review_model, - tools=[ - { - "type": "web_search", - "filters": { - "allowed_domains": [ - "ultralytics.com", - "github.com", - "stackoverflow.com", - ] - }, - } - ], - retries=0, + tools=tools, + tool_handlers=tool_handlers, + max_turns=MAX_AGENT_TURNS, + max_cost=MAX_REVIEW_COST, + request_timeout=(30, 120), + retries=1, # one transient failure on any of the sequential turns would otherwise abort the whole review # Do not pass background=True; queued background reviews can consume the full 900s poll timeout. ) diff --git a/actions/utils/__init__.py b/actions/utils/__init__.py index abacc27..340f65b 100644 --- a/actions/utils/__init__.py +++ b/actions/utils/__init__.py @@ -20,6 +20,7 @@ from .github_utils import GITHUB_API_URL, GITHUB_GRAPHQL_URL, Action, ultralytic from .openai_utils import ( MAX_PROMPT_CHARS, filter_labels, + get_agent_response, get_pr_open_response, get_pr_summary_guidelines, get_pr_summary_prompt, @@ -49,6 +50,7 @@ __all__ = ( "filter_labels", "format_skipped_files_dropdown", "format_skipped_files_note", + "get_agent_response", "get_pr_open_response", "get_pr_summary_guidelines", "get_pr_summary_prompt", diff --git a/actions/utils/openai_utils.py b/actions/utils/openai_utils.py index 9e4b3d4..5772bda 100644 --- a/actions/utils/openai_utils.py +++ b/actions/utils/openai_utils.py @@ -6,6 +6,7 @@ import json import os import re import time +from collections.abc import Callable import requests @@ -185,6 +186,245 @@ def _poll_openai_response(response_json: dict, headers: dict, timeout: int = 900 return response_json +def _openai_response_text(response_json: dict) -> str: + """Extract assistant text from an OpenAI Responses API response.""" + content = "" + for item in response_json.get("output", []): + if item.get("type") == "message": + for c in item.get("content", []): + if c.get("type") == "output_text": + content += c.get("text") or "" + return content.strip() + + +def _count_response_tool_calls(output_items: list[dict]) -> int: + """Count Responses API tool-call output items, including hosted tools.""" + return sum(1 for item in output_items if (item.get("type") or "").endswith("_call")) + + +def _add_openai_usage(total_usage: dict | None, response_json: dict) -> dict | None: + """Add one Responses API usage block into a cumulative usage block.""" + usage = response_json.get("usage") + if not usage: + return total_usage + + total_usage = total_usage or { + "input_tokens": 0, + "output_tokens": 0, + "input_tokens_details": {"cached_tokens": 0}, + "output_tokens_details": {"reasoning_tokens": 0}, + } + total_usage["input_tokens"] += usage.get("input_tokens", 0) + total_usage["output_tokens"] += usage.get("output_tokens", 0) + total_usage["input_tokens_details"]["cached_tokens"] += (usage.get("input_tokens_details") or {}).get( + "cached_tokens", 0 + ) + total_usage["output_tokens_details"]["reasoning_tokens"] += (usage.get("output_tokens_details") or {}).get( + "reasoning_tokens", 0 + ) + return total_usage + + +def _openai_usage_cost(usage: dict, model: str) -> float: + """Compute billed USD cost for a Responses API usage block (cached input billed at 10% of the input rate).""" + costs = MODEL_COSTS.get(model, (0.0, 0.0)) + input_tokens = usage.get("input_tokens", 0) + cached_tokens = (usage.get("input_tokens_details") or {}).get("cached_tokens", 0) + return ((input_tokens - cached_tokens * 0.9) * costs[0] + usage.get("output_tokens", 0) * costs[1]) / 1e6 + + +def _print_openai_usage(response_json: dict, model: str, elapsed: float, metadata: str = "") -> None: + """Print Responses API token/cost telemetry.""" + if usage := response_json.get("usage"): + input_tokens = usage.get("input_tokens", 0) + output_tokens = usage.get("output_tokens", 0) + cached_tokens = (usage.get("input_tokens_details") or {}).get("cached_tokens", 0) + thinking_tokens = (usage.get("output_tokens_details") or {}).get("reasoning_tokens", 0) + cost = _openai_usage_cost(usage, model) + cached_str = f" ({cached_tokens} cached)" if cached_tokens else "" + token_str = f"{input_tokens}{cached_str}→{output_tokens - thinking_tokens}" + if thinking_tokens: + token_str += f" (+{thinking_tokens} thinking)" + metadata = f", {metadata}" if metadata else "" + print(f"{model} ({token_str} = {input_tokens + output_tokens} tokens, ${cost:.5f}, {elapsed:.1f}s{metadata})") + + +def _post_openai_response( + data: dict, headers: dict, retries: int, request_timeout: tuple[int, int] +) -> tuple[dict, float]: + """Post to the Responses API with the same transient retry policy as get_response().""" + url = "https://api.openai.com/v1/responses" + for attempt in range(retries + 1): + try: + r = requests.post(url, json=data, headers=headers, timeout=request_timeout) + elapsed = r.elapsed.total_seconds() + success = r.status_code == 200 + print(f"{'✓' if success else '✗'} POST {url} → {r.status_code} ({elapsed:.1f}s)") + + if attempt < retries and r.status_code >= 500: + print(f"Retrying {r.status_code} in {2**attempt}s (attempt {attempt + 1}/{retries + 1})...") + time.sleep(2**attempt) + continue + + if r.status_code >= 400: + error_body = r.text + print(f"API Error {r.status_code}: {error_body}") + r.reason = f"{r.reason}\n{error_body}" + + r.raise_for_status() + return r.json(), elapsed + except (requests.exceptions.ConnectionError, requests.exceptions.Timeout, json.JSONDecodeError): + if attempt < retries: + print(f"Retrying API request in {2**attempt}s (attempt {attempt + 1}/{retries + 1})...") + time.sleep(2**attempt) + continue + raise + + raise RuntimeError("OpenAI response failed without returning an HTTP response") + + +def _finalize_response_content(response_json: dict, text_format: dict | None) -> str | dict: + """Extract assistant text, strip code fences, and decode structured JSON output when requested.""" + content = remove_outer_codeblocks(_openai_response_text(response_json)) + if text_format and text_format.get("format", {}).get("type") in ["json_object", "json_schema"]: + return json.loads(content) + return content + + +def _parse_tool_arguments(call: dict) -> dict: + """Parse a Responses API function call argument payload.""" + arguments = call.get("arguments") or "{}" + parsed = json.loads(arguments) + if not isinstance(parsed, dict): + raise TypeError("function call arguments must decode to an object") + return parsed + + +def _handle_function_call(call: dict, tool_handlers: dict[str, Callable]) -> dict: + """Execute one model-requested function call and return a Responses API output item.""" + name = call.get("name") + call_id = call.get("call_id") + try: + if name not in tool_handlers: + raise KeyError(f"Unknown tool: {name}") + output = tool_handlers[name](**_parse_tool_arguments(call)) + if not isinstance(output, str): + output = json.dumps(output) + except Exception as e: + output = f"{name or 'tool'} failed: {type(e).__name__}: {e}" + return {"type": "function_call_output", "call_id": call_id, "output": output} + + +def get_agent_response( + messages: list[dict[str, str]], + tools: list[dict], + tool_handlers: dict[str, Callable], + text_format: dict | None = None, + model: str | None = None, + temperature: float = 1.0, + reasoning_effort: str | None = None, + max_turns: int = 6, + max_cost: float = 0.0, + retries: int = 2, + request_timeout: tuple[int, int] = (30, 120), +) -> str | dict: + """Run an iterative OpenAI Responses API agent with application-managed function tools. + + max_cost is a USD ceiling across all turns (0 disables); once reached, remaining tool turns are skipped and the + agent synthesizes a final answer. Models missing from MODEL_COSTS report zero cost, so only max_turns bounds them. + """ + model = model or _get_default_model() + if _is_anthropic_model(model): + print("Anthropic review model selected; falling back to single-shot response without local agent tools") + return get_response( + messages, + temperature=temperature, + reasoning_effort=reasoning_effort, + text_format=text_format, + model=model, + tools=[t for t in tools if t.get("type") != "function"], + retries=retries, + ) + + assert OPENAI_API_KEY, "OpenAI API key is required." + headers = {"Authorization": f"Bearer {OPENAI_API_KEY}", "Content-Type": "application/json"} + + conversation = [m.copy() for m in messages] + if conversation and conversation[0].get("role") == "system": + conversation[0]["content"] += "\n\n" + SYSTEM_PROMPT_ADDITION + + base_data = { + "model": model, + "store": True, + "temperature": temperature, + "tools": tools, + "tool_choice": "auto", + "parallel_tool_calls": True, # batched tool calls share one turn, so the history is re-billed fewer times + } + if "gpt-5" in model: + base_data["reasoning"] = {"effort": reasoning_effort or "low"} + if text_format: + base_data["text"] = text_format + + tool_calls = 0 + total_elapsed = 0.0 + total_usage = None + previous_response_id = None + next_input = conversation + turn = -1 + for turn in range(max_turns): + data = {**base_data, "input": next_input} + if previous_response_id: + data["previous_response_id"] = previous_response_id + response_json, elapsed = _post_openai_response(data, headers, retries, request_timeout) + total_elapsed += elapsed + total_usage = _add_openai_usage(total_usage, response_json) + previous_response_id = response_json.get("id") + output_items = response_json.get("output", []) + turn_tool_calls = _count_response_tool_calls(output_items) + tool_calls += turn_tool_calls + _print_openai_usage(response_json, model, elapsed, f"turn {turn + 1}/{max_turns}, tools {turn_tool_calls}") + function_calls = [item for item in output_items if item.get("type") == "function_call"] + + if not function_calls: + _print_openai_usage( + {"usage": total_usage}, model, total_elapsed, f"agent total, turns {turn + 1}, tools {tool_calls}" + ) + return _finalize_response_content(response_json, text_format) + + print(f"Agent tool turn {turn + 1}: {', '.join(c.get('name', 'unknown') for c in function_calls)}") + if not previous_response_id: + raise RuntimeError("OpenAI response did not include an id for server-managed continuation") + if max_cost and _openai_usage_cost(total_usage, model) >= max_cost: + print(f"Agent cost budget ${max_cost:.2f} reached; skipping remaining tool turns") + next_input = [ # pending calls still need outputs for the chained synthesis request to be valid + {"type": "function_call_output", "call_id": call.get("call_id"), "output": "Tool budget exhausted."} + for call in function_calls + ] + break + next_input = [_handle_function_call(call, tool_handlers) for call in function_calls] + + final_instruction = { + "role": "user", + "content": ( + "You have used all available tool-calling steps. Do not call tools. Synthesize the gathered tool results " + "and return the best final answer now in the required response format. If the gathered context is " + "incomplete, say so in the final answer instead of dumping raw tool output." + ), + } + data = {**base_data, "input": [*next_input, final_instruction], "tool_choice": "none"} + if previous_response_id: + data["previous_response_id"] = previous_response_id + response_json, elapsed = _post_openai_response(data, headers, max(retries, 2), request_timeout) + total_elapsed += elapsed + total_usage = _add_openai_usage(total_usage, response_json) + _print_openai_usage(response_json, model, elapsed, f"turn final/{max_turns}, tools 0") + _print_openai_usage( + {"usage": total_usage}, model, total_elapsed, f"agent total, turns {turn + 2}, tools {tool_calls}" + ) + return _finalize_response_content(response_json, text_format) + + def get_response( messages: list[dict[str, str]], check_links: bool = True, @@ -295,24 +535,8 @@ def get_response( f"{model} ({input_tokens}→{output_tokens} = {input_tokens + output_tokens} tokens, ${cost:.5f}, {elapsed:.1f}s)" ) else: - content = "" - for item in response_json.get("output", []): - if item.get("type") == "message": - for c in item.get("content", []): - if c.get("type") == "output_text": - content += c.get("text") or "" - content = content.strip() - - if usage := response_json.get("usage"): - input_tokens = usage.get("input_tokens", 0) - output_tokens = usage.get("output_tokens", 0) - thinking_tokens = (usage.get("output_tokens_details") or {}).get("reasoning_tokens", 0) - costs = MODEL_COSTS.get(model, (0.0, 0.0)) - cost = (input_tokens * costs[0] + output_tokens * costs[1]) / 1e6 - token_str = f"{input_tokens}→{output_tokens - thinking_tokens}" - if thinking_tokens: - token_str += f" (+{thinking_tokens} thinking)" - print(f"{model} ({token_str} = {input_tokens + output_tokens} tokens, ${cost:.5f}, {elapsed:.1f}s)") + content = _openai_response_text(response_json) + _print_openai_usage(response_json, model, elapsed) if text_format and text_format.get("format", {}).get("type") in ["json_object", "json_schema"]: content = remove_outer_codeblocks(content) diff --git a/tests/test_first_interaction.py b/tests/test_first_interaction.py index 4a2b2a2..c8ef7f1 100644 --- a/tests/test_first_interaction.py +++ b/tests/test_first_interaction.py @@ -99,10 +99,12 @@ def test_get_first_interaction_response(mock_get_response): mock_get_response.assert_called_once() -@patch("actions.review_pr.get_response") -def test_generate_pr_review_uses_synchronous_response(mock_get_response): +@patch("actions.review_pr.get_agent_response") +def test_generate_pr_review_uses_synchronous_response(mock_get_agent_response, tmp_path, monkeypatch): """Test PR reviews avoid background polling for code diffs.""" - mock_get_response.return_value = {"comments": [], "summary": "LGTM"} + monkeypatch.chdir(tmp_path) + (tmp_path / "test.py").write_text("old = False\n", encoding="utf-8") + mock_get_agent_response.return_value = {"comments": [], "summary": "LGTM"} diff = """diff --git a/test.py b/test.py --- a/test.py +++ b/test.py @@ -111,10 +113,84 @@ def test_generate_pr_review_uses_synchronous_response(mock_get_response): +old = False """ - review = review_pr.generate_pr_review("ultralytics/actions", diff, "Test PR", "") + review = review_pr.generate_pr_review("ultralytics/actions", diff, "Test PR", "", event=MagicMock()) assert review["summary"] == "LGTM" - mock_get_response.assert_called_once() - kwargs = mock_get_response.call_args.kwargs + mock_get_agent_response.assert_called_once() + kwargs = mock_get_agent_response.call_args.kwargs assert "background" not in kwargs assert kwargs["tools"][0]["type"] == "web_search" + assert "filters" not in kwargs["tools"][0] + assert {tool.get("name") for tool in kwargs["tools"] if tool.get("type") == "function"} == { + "list_changed_files", + "list_files", + "read_diff", + "read_file", + "search_repo", + } + assert kwargs["max_turns"] == review_pr.MAX_AGENT_TURNS + assert kwargs["max_cost"] == review_pr.MAX_REVIEW_COST + assert kwargs["request_timeout"] == (30, 120) + assert "FULL FILE CONTENTS" not in mock_get_agent_response.call_args.args[0][1]["content"] + + +def test_review_agent_tools_can_read_repo_but_not_outside(tmp_path, monkeypatch): + """Test local review tools can inspect unchanged repo files without escaping the checkout.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "changed.py").write_text("target = True\n", encoding="utf-8") + (tmp_path / "secret.py").write_text("secret = True\n", encoding="utf-8") + (tmp_path / "patterns.py").write_text("literal = '(a+)+$'\n", encoding="utf-8") + workflow = tmp_path / ".github" / "workflows" / "format.yml" + workflow.parent.mkdir(parents=True) + workflow.write_text("workflow = True\n", encoding="utf-8") + outside = tmp_path.parent / "outside_secret.py" + outside.write_text("outside_secret = True\n", encoding="utf-8") + (tmp_path / "linked_secret.py").symlink_to(outside) + (tmp_path / "linked_internal.py").symlink_to(tmp_path / "secret.py") + + _, handlers = review_pr.build_review_agent_tools() + + assert "target = True" in handlers["read_file"](path="changed.py", start_line=None, end_line=None) + assert "secret = True" in handlers["read_file"](path="secret.py", start_line=None, end_line=None) + assert "path must stay inside repository" in handlers["read_file"]( + path="linked_secret.py", start_line=None, end_line=None + ) + listed_files = handlers["list_files"](path_glob=None).splitlines() + assert "secret.py" in listed_files + assert ".github/workflows/format.yml" in handlers["list_files"](path_glob=".github/**").splitlines() + assert "secret.py:1:secret = True" in handlers["search_repo"](query="secret = True", path_glob=None) + assert "No matches found." == handlers["search_repo"](query="secret.*True", path_glob=None) + assert "patterns.py:1:literal = '(a+)+$'" in handlers["search_repo"](query="(a+)+$", path_glob=None) + assert ".github/workflows/format.yml:1:workflow = True" in handlers["search_repo"]( + query="workflow = True", path_glob=".github/**" + ) + + +def test_review_agent_tools_can_list_and_read_changed_file_diffs(): + """Test PR diff tools let the agent inspect every changed file on demand.""" + diff = "\n".join( + [ + "diff --git a/early.py b/early.py", + "--- a/early.py", + "+++ b/early.py", + "@@ -1 +1 @@", + "-old = True", + "+old = False", + "diff --git a/nested/late.py b/nested/late.py", + "--- a/nested/late.py", + "+++ b/nested/late.py", + "@@ -10 +10 @@", + "-value = 1", + "+value = 2", + ] + ) + diff_files, augmented_diff = review_pr.parse_diff_files(diff) + _, handlers = review_pr.build_review_agent_tools(diff_files, augmented_diff) + + changed_files = handlers["list_changed_files"](path_glob=None) + assert "early.py (+1/-1)" in changed_files + assert "nested/late.py (+1/-1)" in changed_files + assert handlers["list_changed_files"](path_glob="nested/**") == "nested/late.py (+1/-1)" + late_diff = handlers["read_diff"](path="nested/late.py", start_line=None, end_line=None) + assert "R 10 +value = 2" in late_diff + assert "L 10 -value = 1" in late_diff diff --git a/tests/test_github_report.py b/tests/test_github_report.py index 68bb22d..7deeb90 100644 --- a/tests/test_github_report.py +++ b/tests/test_github_report.py @@ -137,6 +137,11 @@ def test_paginate_collects_until_short_page(monkeypatch): def test_collect_failed_actions_latest_run_per_workflow(monkeypatch): """Only the latest default-branch run for each workflow should determine whether it is reported.""" + now = datetime.now(timezone.utc).replace(microsecond=0) + recent_success = (now - timedelta(hours=3)).isoformat().replace("+00:00", "Z") + older_failure = (now - timedelta(days=1)).isoformat().replace("+00:00", "Z") + recent_failure = (now - timedelta(hours=2)).isoformat().replace("+00:00", "Z") + recent_failure_updated = (now - timedelta(hours=1, minutes=50)).isoformat().replace("+00:00", "Z") def fake_paginate(path, params=None, key=None, max_pages=100, token=None, allow_skip=False): if path == "/orgs/ultralytics/repos": @@ -161,22 +166,22 @@ def test_collect_failed_actions_latest_run_per_workflow(monkeypatch): "workflow_id": 1, "name": "Nightly", "conclusion": "success", - "run_started_at": "2026-06-30T03:00:00Z", + "run_started_at": recent_success, }, { "workflow_id": 1, "name": "Nightly", "conclusion": "failure", - "run_started_at": "2026-06-29T03:00:00Z", + "run_started_at": older_failure, }, { "workflow_id": 2, "name": "Links", "event": "push", "conclusion": "timed_out", - "run_started_at": "2026-06-30T04:00:00Z", + "run_started_at": recent_failure, "run_number": 42, - "updated_at": "2026-06-30T04:10:00Z", + "updated_at": recent_failure_updated, "head_sha": "abcdef123456", "display_title": "Links", "html_url": "https://github.com/ultralytics/private-repo/actions/runs/42", diff --git a/tests/test_openai_utils.py b/tests/test_openai_utils.py index ff0a89d..81cfe17 100644 --- a/tests/test_openai_utils.py +++ b/tests/test_openai_utils.py @@ -2,10 +2,14 @@ from unittest.mock import MagicMock, patch +import requests + from actions.utils.openai_utils import ( OPENAI_MODEL_DEFAULT, PR_REVIEW_MODEL_DEFAULT, + _count_response_tool_calls, _is_anthropic_model, + get_agent_response, get_response, get_review_model, remove_outer_codeblocks, @@ -27,6 +31,17 @@ def test_is_anthropic_model(): assert _is_anthropic_model("gpt-5-mini-2025-08-07") is False +def test_count_response_tool_calls(): + """Test Responses API tool-call item counting.""" + output_items = [ + {"type": "function_call"}, + {"type": "web_search_call"}, + {"type": "message"}, + {"type": "function_call_output"}, + ] + assert _count_response_tool_calls(output_items) == 2 + + def test_remove_outer_codeblocks(): """Test removing outer code block markers from strings.""" # Test with python code block @@ -116,6 +131,173 @@ def test_get_response_with_link_check(mock_check_links, mock_post): mock_check_links.assert_called_once() +@patch("requests.post") +def test_get_agent_response_calls_function_tools(mock_post): + """Test iterative Responses API agent calls local tools and returns structured output.""" + first_response = MagicMock() + first_response.status_code = 200 + first_response.elapsed.total_seconds.return_value = 1.0 + first_response.json.return_value = { + "id": "resp_first", + "output": [ + { + "type": "function_call", + "call_id": "call_123", + "name": "lookup_value", + "arguments": '{"value": "abc"}', + } + ], + "usage": {"input_tokens": 10, "input_tokens_details": {"cached_tokens": 4}, "output_tokens": 5}, + } + second_response = MagicMock() + second_response.status_code = 200 + second_response.elapsed.total_seconds.return_value = 1.0 + second_response.json.return_value = { + "id": "resp_second", + "output": [ + { + "type": "message", + "content": [{"type": "output_text", "text": '{"comments": [], "summary": "done"}'}], + } + ], + "usage": {"input_tokens": 20, "input_tokens_details": {"cached_tokens": 8}, "output_tokens": 7}, + } + mock_post.side_effect = [first_response, second_response] + + schema = { + "type": "object", + "properties": {"comments": {"type": "array"}, "summary": {"type": "string"}}, + "required": ["comments", "summary"], + "additionalProperties": False, + } + tools = [ + { + "type": "function", + "name": "lookup_value", + "description": "Lookup a value.", + "parameters": { + "type": "object", + "properties": {"value": {"type": "string"}}, + "required": ["value"], + "additionalProperties": False, + }, + "strict": True, + } + ] + + with patch("actions.utils.openai_utils.OPENAI_API_KEY", "test-key"): + with patch("builtins.print") as mock_print: + result = get_agent_response( + [{"role": "user", "content": "review"}], + tools=tools, + tool_handlers={"lookup_value": lambda value: {"found": value}}, + text_format={"format": {"type": "json_schema", "name": "review", "strict": True, "schema": schema}}, + retries=0, + ) + + assert result == {"comments": [], "summary": "done"} + assert mock_post.call_count == 2 + first_payload = mock_post.call_args_list[0].kwargs["json"] + assert first_payload["store"] is True + assert "include" not in first_payload + assert "previous_response_id" not in first_payload + assert first_payload["input"] == [{"role": "user", "content": "review"}] + assert mock_post.call_args_list[1].kwargs["json"]["previous_response_id"] == "resp_first" + second_input = mock_post.call_args_list[1].kwargs["json"]["input"] + assert second_input == [ + { + "type": "function_call_output", + "call_id": "call_123", + "output": '{"found": "abc"}', + } + ] + printed = "\n".join(str(c.args[0]) for c in mock_print.call_args_list if c.args) + assert "turn 1/6, tools 1" in printed + assert "turn 2/6, tools 0" in printed + assert "30 (12 cached)→12 = 42 tokens, $0.00046" in printed + assert "agent total, turns 2, tools 1" in printed + + +@patch("requests.post") +def test_get_agent_response_summarizes_after_max_turns(mock_post): + """Test max-turn exhaustion makes a final no-tool synthesis call.""" + tool_response = MagicMock() + tool_response.status_code = 200 + tool_response.elapsed.total_seconds.return_value = 1.0 + tool_response.json.return_value = { + "id": "resp_tool", + "output": [ + { + "type": "function_call", + "call_id": "call_123", + "name": "lookup_value", + "arguments": '{"value": "abc"}', + } + ], + "usage": {"input_tokens": 10, "output_tokens": 5}, + } + final_response = MagicMock() + final_response.status_code = 200 + final_response.elapsed.total_seconds.return_value = 1.0 + final_response.json.return_value = { + "id": "resp_final", + "output": [ + { + "type": "message", + "content": [{"type": "output_text", "text": '{"comments": [], "summary": "synthesized"}'}], + } + ], + "usage": {"input_tokens": 20, "output_tokens": 7}, + } + mock_post.side_effect = [tool_response, requests.exceptions.Timeout(), final_response] + + schema = { + "type": "object", + "properties": {"comments": {"type": "array"}, "summary": {"type": "string"}}, + "required": ["comments", "summary"], + "additionalProperties": False, + } + tools = [ + { + "type": "function", + "name": "lookup_value", + "description": "Lookup a value.", + "parameters": { + "type": "object", + "properties": {"value": {"type": "string"}}, + "required": ["value"], + "additionalProperties": False, + }, + "strict": True, + } + ] + + with patch("actions.utils.openai_utils.OPENAI_API_KEY", "test-key"): + with patch("actions.utils.openai_utils.time.sleep") as mock_sleep: + result = get_agent_response( + [{"role": "user", "content": "review"}], + tools=tools, + tool_handlers={"lookup_value": lambda value: f"raw tool output for {value}"}, + text_format={"format": {"type": "json_schema", "name": "review", "strict": True, "schema": schema}}, + max_turns=1, + retries=0, + ) + + assert result == {"comments": [], "summary": "synthesized"} + assert mock_post.call_count == 3 + mock_sleep.assert_called_once_with(1) + final_payload = mock_post.call_args_list[2].kwargs["json"] + assert final_payload["tools"] == tools + assert final_payload["tool_choice"] == "none" + assert final_payload["previous_response_id"] == "resp_tool" + assert final_payload["input"][0] == { + "type": "function_call_output", + "call_id": "call_123", + "output": "raw tool output for abc", + } + assert "Synthesize the gathered tool results" in final_payload["input"][-1]["content"] + + @patch("requests.post") def test_get_response_anthropic(mock_post): """Test Anthropic Messages API completion function with mocked response.""" @@ -140,3 +322,85 @@ def test_get_response_anthropic(mock_post): # Verify Anthropic endpoint was called call_args = mock_post.call_args assert call_args[0][0] == "https://api.anthropic.com/v1/messages" + + +@patch("requests.post") +def test_get_agent_response_stops_at_cost_budget(mock_post): + """Test the cost budget skips remaining tool turns and forces final synthesis with stub tool outputs.""" + tool_response = MagicMock() + tool_response.status_code = 200 + tool_response.elapsed.total_seconds.return_value = 1.0 + tool_response.json.return_value = { + "id": "resp_tool", + "output": [ + { + "type": "function_call", + "call_id": "call_123", + "name": "lookup_value", + "arguments": '{"value": "abc"}', + } + ], + "usage": {"input_tokens": 1_000_000, "output_tokens": 0}, # $5.00 for gpt-5.5, over any small budget + } + final_response = MagicMock() + final_response.status_code = 200 + final_response.elapsed.total_seconds.return_value = 1.0 + final_response.json.return_value = { + "id": "resp_final", + "output": [ + { + "type": "message", + "content": [{"type": "output_text", "text": '{"comments": [], "summary": "budget"}'}], + } + ], + "usage": {"input_tokens": 20, "output_tokens": 7}, + } + mock_post.side_effect = [tool_response, final_response] + + schema = { + "type": "object", + "properties": {"comments": {"type": "array"}, "summary": {"type": "string"}}, + "required": ["comments", "summary"], + "additionalProperties": False, + } + tools = [ + { + "type": "function", + "name": "lookup_value", + "description": "Lookup a value.", + "parameters": { + "type": "object", + "properties": {"value": {"type": "string"}}, + "required": ["value"], + "additionalProperties": False, + }, + "strict": True, + } + ] + + def forbidden_handler(value): + raise AssertionError("tool handlers must not run once the cost budget is reached") + + with patch("actions.utils.openai_utils.OPENAI_API_KEY", "test-key"): + result = get_agent_response( + [{"role": "user", "content": "review"}], + tools=tools, + tool_handlers={"lookup_value": forbidden_handler}, + text_format={"format": {"type": "json_schema", "name": "review", "strict": True, "schema": schema}}, + model="gpt-5.5", + max_turns=8, + max_cost=1.00, + retries=0, + ) + + assert result == {"comments": [], "summary": "budget"} + assert mock_post.call_count == 2 # budget reached on turn 1, remaining turns skipped + final_payload = mock_post.call_args_list[1].kwargs["json"] + assert final_payload["tool_choice"] == "none" + assert final_payload["previous_response_id"] == "resp_tool" + assert final_payload["input"][0] == { + "type": "function_call_output", + "call_id": "call_123", + "output": "Tool budget exhausted.", + } + assert "Synthesize the gathered tool results" in final_payload["input"][-1]["content"]