Add iterative PR review agent tools (#786)
This commit is contained in:
Glenn Jocher
+283
-23
@@ -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.
|
||||
)
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
+242
-18
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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"]
|
||||
|
||||
Reference in New Issue
Block a user