diff --git a/agent/chat.py b/agent/chat.py new file mode 100644 index 00000000..efdba398 --- /dev/null +++ b/agent/chat.py @@ -0,0 +1,159 @@ +"""Chat graph. + +A read-only "chat with this PR" agent for the review UI. Unlike the main agent +and reviewer, it has **no sandbox**: it answers questions about a single pull +request using the diff, the published review findings, and read-only access to +the repository over the GitHub API. + +PR context (diff, findings, overview) is seeded as virtual files under ``/pr/`` +into the ``files`` state channel by the dashboard chat proxy +(``agent/dashboard/review_chat_api.py``); the built-in ``read_file``/``grep`` +tools operate over those. Repo coordinates and the reviewer thread id arrive in +``configurable``; a repo-scoped GitHub App token is resolved here so the +GitHub-backed tools never receive a user credential. +""" +# ruff: noqa: E402 + +from __future__ import annotations + +import logging +import warnings + +from langgraph.graph.state import RunnableConfig +from langgraph.pregel import Pregel + +warnings.filterwarnings("ignore", module="langchain_core._api.deprecation") +warnings.filterwarnings("ignore", message=".*Pydantic V1.*", category=UserWarning) + +from deepagents import create_deep_agent +from langchain.agents.middleware import ModelCallLimitMiddleware + +from .dashboard.options import SUPPORTED_MODEL_IDS, model_supports_effort +from .dashboard.team_settings import get_team_default_model +from .middleware import ( + ExcludeToolsMiddleware, + SanitizeThinkingBlocksMiddleware, + SanitizeToolInputsMiddleware, + ToolErrorMiddleware, +) +from .server import ( + DEFAULT_LLM_MAX_TOKENS, + DEFAULT_RECURSION_LIMIT, + graph_loaded_for_execution, +) +from .tools import ( + fetch_url, + list_review_findings, + read_repo_file, + search_repo_code, + web_search, +) +from .utils.github_app import get_github_app_installation_token +from .utils.model import DEFAULT_LLM_REASONING, make_model, provider_model_kwargs +from .utils.tracing import AGENT_TRACING_PROJECT, traced_graph_factory + +logger = logging.getLogger(__name__) + +CHAT_MODEL_CALL_LIMIT = 100 + +# Read-only: the chat agent never mutates files or runs shell commands. These are +# injected by deepagents' FilesystemMiddleware and stripped before the model sees +# them (there is no sandbox, so ``execute`` would error anyway). +_EXCLUDED_TOOLS = frozenset({"execute", "write_file", "edit_file"}) + +CHAT_PROMPT = """You are a code-review chat assistant. You help the author and reviewers \ +understand one GitHub pull request: `{repo_owner}/{repo_name}` #{pr_number}. + +You have NO sandbox and cannot run code, execute tests, commit, or open PRs. You \ +reason from the PR's diff, the published review findings, and read-only access to \ +the repository. + +Context already loaded as virtual files (use `read_file`, `ls`, `grep`): +- `/pr/overview.md` — title, description, author, branches, head commit, change stats. +- `/pr/diff.patch` — the unified diff under review. +- `/pr/findings.md` — the reviewer's published findings, rendered for reading. + +Tools: +- `read_repo_file(path, ref)` — read any repo file/dir at a commit (defaults to the \ +PR head). Use it to inspect callers, definitions, and neighboring code beyond the diff. +- `search_repo_code(query)` — find a symbol or phrase across the repository. +- `list_review_findings(status_filter)` — the live findings (open/resolved/dismissed) \ +with severity, confidence, and resolution notes. +- `web_search`, `fetch_url` — for external docs or standards. + +Guidance: +- Be concrete and cite specific files and line numbers from the diff. +- Ground claims about the review in the actual findings; don't invent issues. +- When you propose a change, describe it precisely — you cannot apply it yourself. +- Keep answers focused and skimmable. Match the depth of the question. +""" + + +async def _resolve_chat_model(configurable: dict) -> tuple[str, str]: + model_id = configurable.get("chat_model_id") + effort = configurable.get("chat_effort") + if ( + isinstance(model_id, str) + and model_id in SUPPORTED_MODEL_IDS + and isinstance(effort, str) + and model_supports_effort(model_id, effort) + ): + return model_id, effort + # Team review-chat default, which itself inherits the Agent default if unset. + return await get_team_default_model("chat") + + +async def get_chat_agent(config: RunnableConfig) -> Pregel: + """Get a read-only PR chat agent. No sandbox; PR context comes via config.""" + thread_id = config["configurable"].get("thread_id") + config["recursion_limit"] = DEFAULT_RECURSION_LIMIT + + if thread_id is None or not graph_loaded_for_execution(config): + return create_deep_agent(system_prompt="", tools=[]).with_config(config) + + configurable = config["configurable"] + repo_owner = str(configurable.get("chat_repo_owner") or "") + repo_name = str(configurable.get("chat_repo_name") or "") + pr_number = configurable.get("chat_pr_number") + + # Resolve a repo-scoped, read-only App token in-graph so a user credential is + # never passed through the run config. Tools read it from configurable. + token = await get_github_app_installation_token(repositories=[repo_name] if repo_name else None) + if isinstance(token, str) and token: + configurable["chat_github_token"] = token + + model_id, effort = await _resolve_chat_model(configurable) + model_kwargs = provider_model_kwargs( + model_id, + effort, + max_tokens=DEFAULT_LLM_MAX_TOKENS, + openai_reasoning_default=DEFAULT_LLM_REASONING, + ) + + system_prompt = CHAT_PROMPT.format( + repo_owner=repo_owner or "", + repo_name=repo_name or "", + pr_number=pr_number if isinstance(pr_number, int) else "?", + ) + + return create_deep_agent( + model=make_model(model_id, **model_kwargs), + system_prompt=system_prompt, + tools=[ + read_repo_file, + search_repo_code, + list_review_findings, + web_search, + fetch_url, + ], + middleware=[ + SanitizeToolInputsMiddleware(), + ModelCallLimitMiddleware(run_limit=CHAT_MODEL_CALL_LIMIT, exit_behavior="end"), + ToolErrorMiddleware(), + ExcludeToolsMiddleware(excluded=_EXCLUDED_TOOLS), + SanitizeThinkingBlocksMiddleware(), + ], + ).with_config(config) + + +traced_chat_agent = traced_graph_factory(get_chat_agent, AGENT_TRACING_PROJECT) diff --git a/agent/dashboard/review_chat_api.py b/agent/dashboard/review_chat_api.py new file mode 100644 index 00000000..3c74175e --- /dev/null +++ b/agent/dashboard/review_chat_api.py @@ -0,0 +1,507 @@ +"""Backend for the review page's "chat with this PR" feature. + +A dedicated, sandbox-less ``chat`` graph (``agent/chat.py``) answers questions +about one PR. This module mints a per-user chat thread, seeds the PR diff, +review findings, and an overview as virtual files on the first run, and proxies +the LangGraph stream/commands/state/history protocol the frontend SDK speaks — +the chat counterpart of ``thread_api``'s agent proxy, pinned to assistant +``chat`` and scoped to the review's PR. +""" + +from __future__ import annotations + +import json +import logging +from collections.abc import AsyncIterator +from datetime import UTC, datetime +from typing import Any + +import httpx +from deepagents.backends.utils import create_file_data +from fastapi import HTTPException + +from ..reviewer_diff import fetch_pr_diff +from ..reviewer_findings import REVIEWER_THREAD_KIND +from ..utils.github_app import get_github_app_installation_token +from ..utils.thread_ops import langgraph_client, langgraph_url +from .options import SUPPORTED_MODEL_IDS, model_supports_effort +from .review_api import classify_finding, get_review, reviewer_thread_id +from .thread_api import ( + _DASHBOARD_STREAM_MODES, + _langgraph_proxy_headers, + _require_json_content_type, + _stream_thread_events, +) + +logger = logging.getLogger(__name__) + +_CHAT_ASSISTANT_ID = "chat" +_CHAT_SOURCE = "review_chat" +_PROXY_REQUEST_TIMEOUT = httpx.Timeout(30.0, connect=5.0) +_MAX_DIFF_CHARS = 400_000 + + +def _now_ms() -> int: + return int(datetime.now(UTC).timestamp() * 1000) + + +_TITLE_MAX_CHARS = 60 + + +async def _reviewer_thread_exists(owner: str, repo: str, pr_number: int) -> bool: + try: + thread = await langgraph_client().threads.get(reviewer_thread_id(owner, repo, pr_number)) + except Exception: # noqa: BLE001 + return False + metadata = thread.get("metadata") if isinstance(thread, dict) else None + return isinstance(metadata, dict) and metadata.get("kind") == REVIEWER_THREAD_KIND + + +async def get_review_chat(owner: str, repo: str, pr_number: int, login: str) -> dict[str, Any]: + """Chat availability for this PR. Threads are minted client-side per chat.""" + return { + "available": await _reviewer_thread_exists(owner, repo, pr_number), + "assistant_id": _CHAT_ASSISTANT_ID, + } + + +def _chat_thread_search_metadata( + owner: str, repo: str, pr_number: int, login: str +) -> dict[str, Any]: + return { + "kind": _CHAT_SOURCE, + "github_login": login, + "repo_owner": owner, + "repo_name": repo, + "pr_number": pr_number, + } + + +async def list_review_chat_threads( + owner: str, repo: str, pr_number: int, login: str, *, limit: int = 50 +) -> list[dict[str, Any]]: + """This user's chat conversations for the PR, newest first.""" + client = langgraph_client() + try: + threads = await client.threads.search( + metadata=_chat_thread_search_metadata(owner, repo, pr_number, login), + limit=limit, + sort_by="updated_at", + sort_order="desc", + ) + except Exception: # noqa: BLE001 + logger.debug( + "chat thread search failed for %s/%s#%s", owner, repo, pr_number, exc_info=True + ) + return [] + out: list[dict[str, Any]] = [] + for thread in threads or []: + if not isinstance(thread, dict): + continue + metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} + thread_id = thread.get("thread_id") or thread.get("id") + if not isinstance(thread_id, str): + continue + out.append( + { + "thread_id": thread_id, + "title": metadata.get("title") or "New chat", + "updated_at": thread.get("updated_at") + if isinstance(thread.get("updated_at"), str) + else None, + } + ) + return out + + +async def delete_review_chat_thread( + owner: str, repo: str, pr_number: int, login: str, thread_id: str +) -> None: + """Delete one of the user's chat threads (scoped + ownership-checked).""" + metadata = await assert_chat_thread_access(thread_id, owner, repo, pr_number, login) + if metadata is None: + return # already gone; treat as success + await langgraph_client().threads.delete(thread_id) + + +def _first_user_text(params: dict[str, Any]) -> str: + run_input = params.get("input") + messages = run_input.get("messages") if isinstance(run_input, dict) else None + if not isinstance(messages, list): + return "" + for message in messages: + if not isinstance(message, dict) or message.get("type") != "human": + continue + content = message.get("content") + if isinstance(content, str): + return content.strip() + if isinstance(content, list): + for block in content: + if isinstance(block, dict) and block.get("type") == "text": + text = block.get("text") + if isinstance(text, str) and text.strip(): + return text.strip() + return "" + + +def _derive_title(params: dict[str, Any]) -> str: + text = _first_user_text(params) + if not text: + return "New chat" + flattened = " ".join(text.split()) + return flattened[:_TITLE_MAX_CHARS] if flattened else "New chat" + + +def _render_overview(review: dict[str, Any]) -> str: + pr = review.get("pr") if isinstance(review.get("pr"), dict) else {} + lines = [ + f"# {review.get('title') or 'Pull request'} (#{review.get('number')})", + "", + f"- Repository: {review.get('full_name', '')}", + f"- Author: {review.get('author', '')}", + f"- Head: {review.get('head_ref', '')} @ {review.get('head_sha', '')[:12]}", + f"- Base: {review.get('base_ref', '')}", + f"- State: {pr.get('state', '')}", + f"- Changes: +{pr.get('additions', 0)} -{pr.get('deletions', 0)} " + f"across {pr.get('changed_files', 0)} file(s), {pr.get('commits', 0)} commit(s)", + "", + "## Description", + "", + str(pr.get("body") or "_No description provided._"), + ] + return "\n".join(lines) + + +def _render_findings(findings: list[dict[str, Any]]) -> str: + if not findings: + return "# Review findings\n\n_No findings were published for this PR._" + out = ["# Review findings", ""] + for finding in findings: + group = classify_finding(finding) + location = finding.get("file") or "" + start = finding.get("start_line") + if location and start: + location = f"{location}:{start}" + out.append( + f"## [{group}] {finding.get('title') or 'Untitled'} " + f"({finding.get('severity', 'low')}/{finding.get('confidence', 'medium')}, " + f"{finding.get('status', 'open')})" + ) + if location: + out.append(f"`{location}`") + out.append("") + out.append(str(finding.get("description") or "")) + suggestion = finding.get("suggestion") + if suggestion: + out.append("") + out.append(f"Suggested change:\n```\n{suggestion}\n```") + note = finding.get("resolution_note") + if note: + out.append("") + out.append(f"Resolution: {note}") + out.append("") + return "\n".join(out) + + +def _review_head_sha(review: dict[str, Any]) -> str: + pr = review.get("pr") if isinstance(review.get("pr"), dict) else {} + return str(pr.get("head_sha") or review.get("head_sha") or "") + + +async def _build_pr_context( + owner: str, + repo: str, + pr_number: int, + token: str, + *, + review: dict[str, Any] | None = None, +) -> tuple[dict[str, Any], str]: + """Fetch diff + findings + overview as seedable files; return ``(files, head_sha)``. + + Accepts an already-fetched ``review`` to avoid re-fetching it when the caller + has just read it to decide whether a reseed is needed. + """ + if review is None: + review = await get_review(owner, repo, pr_number) + findings = review.get("findings") if isinstance(review.get("findings"), list) else [] + head_sha = _review_head_sha(review) + diff = await fetch_pr_diff(owner=owner, repo=repo, pr_number=pr_number, token=token) or "" + if len(diff) > _MAX_DIFF_CHARS: + diff = diff[:_MAX_DIFF_CHARS] + "\n\n[diff truncated]\n" + files = { + "/pr/overview.md": create_file_data(_render_overview(review)), + "/pr/diff.patch": create_file_data(diff or "[diff unavailable]"), + "/pr/findings.md": create_file_data(_render_findings(findings)), + } + return files, head_sha + + +async def _get_chat_thread_metadata(thread_id: str) -> dict[str, Any] | None: + client = langgraph_client() + try: + thread = await client.threads.get(thread_id) + except Exception: # noqa: BLE001 + return None + return thread.get("metadata") if isinstance(thread, dict) else None + + +async def assert_chat_thread_access( + thread_id: str, owner: str, repo: str, pr_number: int, login: str +) -> dict[str, Any] | None: + """Authorize a client-supplied chat thread id before proxying to LangGraph. + + Chat threads are private per viewer and their ids come from the client, so + every proxy route must confirm the caller owns the thread it names. Returns + the thread metadata when it exists and belongs to ``login`` for this PR, or + ``None`` when the thread doesn't exist yet (it's created lazily on the first + run, so there is nothing to leak). Raises 404 when a thread exists but is + owned by someone else or scoped to a different repo/PR — this also rejects + reviewer (or any non-chat) threads, whose ``kind`` is not ``_CHAT_SOURCE``. + """ + metadata = await _get_chat_thread_metadata(thread_id) + if metadata is None: + return None + owns = ( + metadata.get("kind") == _CHAT_SOURCE + and metadata.get("github_login") == login + and metadata.get("repo_owner") == owner + and metadata.get("repo_name") == repo + and metadata.get("pr_number") == pr_number + ) + if not owns: + raise HTTPException(404, "chat not found") + return metadata + + +async def _create_chat_thread( + thread_id: str, owner: str, repo: str, pr_number: int, login: str, *, title: str +) -> None: + now_ms = _now_ms() + metadata = { + "kind": _CHAT_SOURCE, + "source": _CHAT_SOURCE, + "github_login": login, + "repo_owner": owner, + "repo_name": repo, + "pr_number": pr_number, + "title": title, + "created_at_ms": now_ms, + "updated_at_ms": now_ms, + } + await langgraph_client().threads.create( + thread_id=thread_id, metadata=metadata, if_exists="do_nothing" + ) + + +def _normalize_chat_model(configurable: dict[str, Any]) -> tuple[str | None, str | None]: + model_id = configurable.get("chat_model_id") + effort = configurable.get("chat_effort") + if ( + isinstance(model_id, str) + and model_id in SUPPORTED_MODEL_IDS + and isinstance(effort, str) + and model_supports_effort(model_id, effort) + ): + return model_id, effort + return None, None + + +async def _enrich_chat_command( + command: dict[str, Any], + *, + owner: str, + repo: str, + pr_number: int, + login: str, + thread_id: str, +) -> dict[str, Any]: + if command.get("method") != "run.start": + return command + + params = command.get("params") + if not isinstance(params, dict): + params = {} + command["params"] = params + + metadata = await _get_chat_thread_metadata(thread_id) + created = metadata is None + if created: + await _create_chat_thread( + thread_id, owner, repo, pr_number, login, title=_derive_title(params) + ) + metadata = {} + + client_config = params.get("config") + if not isinstance(client_config, dict): + client_config = {} + client_configurable = client_config.get("configurable") + if not isinstance(client_configurable, dict): + client_configurable = {} + + configurable: dict[str, Any] = { + "thread_id": thread_id, + "source": _CHAT_SOURCE, + "github_login": login, + "chat_repo_owner": owner, + "chat_repo_name": repo, + "chat_pr_number": pr_number, + "reviewer_thread_id": reviewer_thread_id(owner, repo, pr_number), + } + model_id, effort = _normalize_chat_model(client_configurable) + if model_id and effort: + configurable["chat_model_id"] = model_id + configurable["chat_effort"] = effort + + # Seed PR context on the thread's first run, and reseed whenever the PR head + # has moved since the last seed — otherwise the chat keeps answering from a + # stale diff/findings while the review page already shows the current head. + stored_head = metadata.get("chat_head_sha") if isinstance(metadata, dict) else None + stored_head = stored_head if isinstance(stored_head, str) else "" + + review: dict[str, Any] | None = None + needs_seed = created + if not created: + try: + review = await get_review(owner, repo, pr_number) + except HTTPException: + review = None # transient/missing review: keep the existing context + if review is not None: + current_head = _review_head_sha(review) + needs_seed = bool(current_head) and current_head != stored_head + + if needs_seed: + token = await get_github_app_installation_token(repositories=[repo]) + if not token: + raise HTTPException(503, "GitHub App token unavailable") + try: + pr_files, head_sha = await _build_pr_context( + owner, repo, pr_number, token, review=review + ) + except HTTPException: + raise + except Exception as exc: # noqa: BLE001 + logger.warning("Failed to seed PR chat context for %s/%s#%s", owner, repo, pr_number) + raise HTTPException(502, "could not load PR context") from exc + if head_sha: + configurable["chat_head_sha"] = head_sha + await langgraph_client().threads.update( + thread_id=thread_id, metadata={"chat_head_sha": head_sha} + ) + elif stored_head: + configurable["chat_head_sha"] = stored_head + run_input = params.get("input") + if not isinstance(run_input, dict): + run_input = {} + existing_files = run_input.get("files") + run_input["files"] = { + **(existing_files if isinstance(existing_files, dict) else {}), + **pr_files, + } + params["input"] = run_input + elif stored_head: + configurable["chat_head_sha"] = stored_head + + params["assistant_id"] = _CHAT_ASSISTANT_ID + params.setdefault("stream_mode", list(_DASHBOARD_STREAM_MODES)) + params.setdefault("stream_resumable", True) + params["config"] = {**client_config, "configurable": configurable} + command["params"] = params + return command + + +async def proxy_review_chat_commands( + owner: str, + repo: str, + pr_number: int, + login: str, + thread_id: str, + body: bytes, + *, + content_type: str = "application/json", +) -> tuple[int, bytes, str | None]: + # Reject threads the caller doesn't own; a missing thread is created lazily + # below on the first `run.start` (with the caller as owner). + await assert_chat_thread_access(thread_id, owner, repo, pr_number, login) + _require_json_content_type(content_type) + try: + parsed = json.loads(body) + except json.JSONDecodeError as exc: + raise HTTPException(400, "command body must be a JSON object") from exc + if not isinstance(parsed, dict): + raise HTTPException(400, "command body must be a JSON object") + + enriched = await _enrich_chat_command( + parsed, owner=owner, repo=repo, pr_number=pr_number, login=login, thread_id=thread_id + ) + url = f"{langgraph_url().rstrip('/')}/threads/{thread_id}/commands" + headers = _langgraph_proxy_headers(content_type=content_type) + async with httpx.AsyncClient(timeout=_PROXY_REQUEST_TIMEOUT) as client: + response = await client.post(url, content=json.dumps(enriched).encode(), headers=headers) + return response.status_code, response.content, response.headers.get("content-type") + + +async def proxy_review_chat_stream_events( + owner: str, + repo: str, + pr_number: int, + login: str, + thread_id: str, + body: bytes, + *, + content_type: str = "application/json", +) -> AsyncIterator[bytes]: + await assert_chat_thread_access(thread_id, owner, repo, pr_number, login) + _require_json_content_type(content_type) + return _stream_thread_events(thread_id, body, content_type) + + +async def _proxy_passthrough( + method: str, thread_id: str, suffix: str, body: bytes | None, content_type: str +) -> tuple[int, bytes, str | None]: + url = f"{langgraph_url().rstrip('/')}/threads/{thread_id}/{suffix}" + headers = _langgraph_proxy_headers(content_type=content_type) + async with httpx.AsyncClient(timeout=_PROXY_REQUEST_TIMEOUT) as client: + if method == "GET": + response = await client.get(url, headers=headers) + else: + response = await client.post(url, content=body or b"{}", headers=headers) + return response.status_code, response.content, response.headers.get("content-type") + + +async def proxy_review_chat_state( + owner: str, repo: str, pr_number: int, login: str, thread_id: str +) -> tuple[int, bytes, str | None]: + await assert_chat_thread_access(thread_id, owner, repo, pr_number, login) + status_code, content, media_type = await _proxy_passthrough( + "GET", thread_id, "state", None, "application/json" + ) + # The chat thread is created lazily on the first run, so an initial getState + # hits a missing thread. Return an empty idle state so the SDK hydrates a + # fresh thread instead of surfacing the 404 as a hard error. + if status_code == 404: + empty = json.dumps({"values": {}, "next": []}).encode() + return 200, empty, "application/json" + return status_code, content, media_type + + +async def proxy_review_chat_history( + owner: str, + repo: str, + pr_number: int, + login: str, + thread_id: str, + body: bytes, + *, + content_type: str = "application/json", +) -> tuple[int, bytes, str | None]: + await assert_chat_thread_access(thread_id, owner, repo, pr_number, login) + _require_json_content_type(content_type) + status_code, content, media_type = await _proxy_passthrough( + "POST", thread_id, "history", body, content_type + ) + # The thread is created lazily on the first run; before then, hydration + # history reads hit a missing thread. Return an empty list so the SDK + # treats it as a fresh thread instead of erroring. + if status_code == 404: + return 200, b"[]", "application/json" + return status_code, content, media_type diff --git a/agent/dashboard/routes.py b/agent/dashboard/routes.py index 83c45b10..c7f97b90 100644 --- a/agent/dashboard/routes.py +++ b/agent/dashboard/routes.py @@ -69,6 +69,15 @@ from .review_api import ( list_reviews, trigger_re_review, ) +from .review_chat_api import ( + delete_review_chat_thread, + get_review_chat, + list_review_chat_threads, + proxy_review_chat_commands, + proxy_review_chat_history, + proxy_review_chat_state, + proxy_review_chat_stream_events, +) from .review_style_jobs import ( cancel_review_style_analysis, start_bootstrap_analysis, @@ -830,6 +839,136 @@ async def api_re_review( return await trigger_re_review(owner, repo, pr_number, session["sub"]) +# --- PR chat (sandbox-less ``chat`` graph) ----------------------------------- +# The frontend points a LangGraph StreamProvider at the base +# ``/reviews/{owner}/{repo}/{pr_number}/chat``; the SDK then issues the +# ``/threads/{id}/{commands,stream/events,state,history}`` calls proxied below. + + +@router.get("/reviews/{owner}/{repo}/{pr_number}/chat") +async def api_get_review_chat( + owner: str, + repo: str, + pr_number: int, + session: dict[str, Any] = _SESSION_DEP, +) -> dict[str, Any]: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + return await get_review_chat(owner, repo, pr_number, session["sub"]) + + +@router.get("/reviews/{owner}/{repo}/{pr_number}/chat/threads") +async def api_list_review_chat_threads( + owner: str, + repo: str, + pr_number: int, + session: dict[str, Any] = _SESSION_DEP, +) -> dict[str, Any]: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + threads = await list_review_chat_threads(owner, repo, pr_number, session["sub"]) + return {"threads": threads} + + +@router.delete("/reviews/{owner}/{repo}/{pr_number}/chat/threads/{thread_id}") +async def api_delete_review_chat_thread( + owner: str, + repo: str, + pr_number: int, + thread_id: str, + session: dict[str, Any] = _SESSION_DEP, +) -> Response: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + await delete_review_chat_thread(owner, repo, pr_number, session["sub"], thread_id) + return Response(status_code=204) + + +@router.post("/reviews/{owner}/{repo}/{pr_number}/chat/threads/{thread_id}/commands") +async def api_review_chat_commands( + owner: str, + repo: str, + pr_number: int, + thread_id: str, + request: Request, + session: dict[str, Any] = _SESSION_DEP, +) -> Response: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + body = await request.body() + status_code, content, media_type = await proxy_review_chat_commands( + owner, + repo, + pr_number, + session["sub"], + thread_id, + body, + content_type=request.headers.get("content-type", "application/json"), + ) + return Response(content=content, status_code=status_code, media_type=media_type) + + +@router.post("/reviews/{owner}/{repo}/{pr_number}/chat/threads/{thread_id}/stream/events") +async def api_review_chat_stream_events( + owner: str, + repo: str, + pr_number: int, + thread_id: str, + request: Request, + session: dict[str, Any] = _SESSION_DEP, +) -> StreamingResponse: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + body = await request.body() + stream = await proxy_review_chat_stream_events( + owner, + repo, + pr_number, + session["sub"], + thread_id, + body, + content_type=request.headers.get("content-type", "application/json"), + ) + return StreamingResponse( + stream, + media_type="text/event-stream", + headers={"Cache-Control": "no-cache", "Connection": "keep-alive"}, + ) + + +@router.get("/reviews/{owner}/{repo}/{pr_number}/chat/threads/{thread_id}/state") +async def api_review_chat_state( + owner: str, + repo: str, + pr_number: int, + thread_id: str, + session: dict[str, Any] = _SESSION_DEP, +) -> Response: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + status_code, content, media_type = await proxy_review_chat_state( + owner, repo, pr_number, session["sub"], thread_id + ) + return Response(content=content, status_code=status_code, media_type=media_type) + + +@router.post("/reviews/{owner}/{repo}/{pr_number}/chat/threads/{thread_id}/history") +async def api_review_chat_history( + owner: str, + repo: str, + pr_number: int, + thread_id: str, + request: Request, + session: dict[str, Any] = _SESSION_DEP, +) -> Response: + await require_repo_access_for_user(session["sub"], f"{owner}/{repo}") + body = await request.body() + status_code, content, media_type = await proxy_review_chat_history( + owner, + repo, + pr_number, + session["sub"], + thread_id, + body, + content_type=request.headers.get("content-type", "application/json"), + ) + return Response(content=content, status_code=status_code, media_type=media_type) + + @router.post("/review-styles") async def api_create_review_style( body: ReviewStyleCreate, diff --git a/agent/dashboard/team_settings.py b/agent/dashboard/team_settings.py index 267c3123..ed546609 100644 --- a/agent/dashboard/team_settings.py +++ b/agent/dashboard/team_settings.py @@ -52,6 +52,8 @@ class TeamSettingsUpdate(BaseModel): default_reviewer_reasoning_effort: str | None = None default_reviewer_subagent_model: str | None = None default_reviewer_subagent_reasoning_effort: str | None = None + default_chat_model: str | None = None + default_chat_reasoning_effort: str | None = None @field_validator("org_guidelines", mode="before") @classmethod @@ -87,6 +89,9 @@ class TeamSettingsUpdate(BaseModel): self.default_reviewer_subagent_reasoning_effort, "reviewer subagent", ) + _validate_model_effort_pair( + self.default_chat_model, self.default_chat_reasoning_effort, "review chat" + ) return self @@ -139,6 +144,9 @@ def _default_settings() -> dict[str, Any]: "default_reviewer_reasoning_effort": fallback_effort, "default_reviewer_subagent_model": fallback_model, "default_reviewer_subagent_reasoning_effort": fallback_effort, + # No hardcoded chat default: unset means "inherit the Agent default". + "default_chat_model": None, + "default_chat_reasoning_effort": None, "updated_at": None, } @@ -184,6 +192,8 @@ async def upsert_team_settings(update: TeamSettingsUpdate) -> dict[str, Any]: "default_reviewer_reasoning_effort": update.default_reviewer_reasoning_effort, "default_reviewer_subagent_model": update.default_reviewer_subagent_model, "default_reviewer_subagent_reasoning_effort": update.default_reviewer_subagent_reasoning_effort, + "default_chat_model": update.default_chat_model, + "default_chat_reasoning_effort": update.default_chat_reasoning_effort, "updated_at": datetime.now(UTC).isoformat(), } await _client().store.put_item(TEAM_SETTINGS_NAMESPACE, TEAM_SETTINGS_KEY, value) @@ -196,7 +206,7 @@ async def get_team_default_repo() -> dict[str, str] | None: async def get_team_default_model( - role: Literal["agent", "reviewer"], + role: Literal["agent", "reviewer", "chat"], ) -> tuple[str, str]: """Return the team-wide default ``(model_id, reasoning_effort)`` for ``role``. @@ -205,9 +215,25 @@ async def get_team_default_model( (so a stale Anthropic/OpenAI selection stays on its provider rather than jumping cross-provider); otherwise the hardcoded global default from :func:`agent.dashboard.options.default_model_pair`. + + ``"chat"`` (the review-page PR chat) has no hardcoded default: when its + admin setting is unset/invalid it inherits the team **agent** default. """ settings = await get_team_settings() - if role == "agent": + if role == "chat": + model = settings.get("default_chat_model") + effort = settings.get("default_chat_reasoning_effort") + if ( + isinstance(model, str) + and isinstance(effort, str) + and model in SUPPORTED_MODEL_IDS + and model_supports_effort(model, effort) + ): + return _resolve_default_pair(model, effort) + # Inherit the Agent default when no chat-specific model is configured. + model = settings.get("default_agent_model") + effort = settings.get("default_agent_reasoning_effort") + elif role == "agent": model = settings.get("default_agent_model") effort = settings.get("default_agent_reasoning_effort") else: diff --git a/agent/tools/__init__.py b/agent/tools/__init__.py index aea428d3..79f9e21a 100644 --- a/agent/tools/__init__.py +++ b/agent/tools/__init__.py @@ -9,11 +9,14 @@ from .linear_get_issue_comments import linear_get_issue_comments from .linear_list_teams import linear_list_teams from .linear_update_issue import linear_update_issue from .list_findings import list_findings +from .list_review_findings import list_review_findings from .open_pull_request import open_pull_request from .publish_review import publish_review +from .read_repo_file import read_repo_file from .reply_to_finding_thread import reply_to_finding_thread from .request_pr_review import request_pr_review from .resolve_finding_thread import resolve_finding_thread +from .search_repo_code import search_repo_code from .slack_read_thread_messages import slack_read_thread_messages from .slack_thread_reply import slack_thread_reply from .update_finding import update_finding @@ -31,11 +34,14 @@ __all__ = [ "linear_list_teams", "linear_update_issue", "list_findings", + "list_review_findings", "open_pull_request", "publish_review", + "read_repo_file", "request_pr_review", "reply_to_finding_thread", "resolve_finding_thread", + "search_repo_code", "slack_read_thread_messages", "slack_thread_reply", "update_finding", diff --git a/agent/tools/list_review_findings.py b/agent/tools/list_review_findings.py new file mode 100644 index 00000000..16321046 --- /dev/null +++ b/agent/tools/list_review_findings.py @@ -0,0 +1,71 @@ +"""Tool: ``list_review_findings``. Read the published review's findings. + +The PR chat agent runs on its own thread; the findings live on the canonical +reviewer thread for the PR. The reviewer thread id is seeded into the run config +by the dashboard chat proxy. +""" + +from __future__ import annotations + +import asyncio +from typing import Any + +from langgraph.config import get_config + +from ..reviewer_findings import list_findings as list_findings_async + +_COMPACT_FIELDS = ( + "id", + "severity", + "confidence", + "category", + "title", + "description", + "suggestion", + "file", + "start_line", + "end_line", + "side", + "status", + "resolution_note", +) + + +def _compact(finding: dict[str, Any]) -> dict[str, Any]: + return {key: finding.get(key) for key in _COMPACT_FIELDS if finding.get(key) is not None} + + +def list_review_findings(status_filter: str | None = None) -> dict[str, Any]: + """List the findings the reviewer published for this PR. + + Use this to ground answers about the review — what was flagged, the + severity/confidence, and any resolution notes. Prefer quoting these over + re-deriving issues from the diff. + + Args: + status_filter: One of ``open``, ``resolved``, ``dismissed``. ``None`` + (default) returns findings of every status. + + Returns: + ``{findings, count}``; ``{findings: [], count: 0, error}`` on failure. + """ + if status_filter is not None and status_filter not in {"open", "resolved", "dismissed"}: + return {"findings": [], "count": 0, "error": f"Invalid status_filter: {status_filter}"} + + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + reviewer_thread_id = ( + configurable.get("reviewer_thread_id") if isinstance(configurable, dict) else None + ) + if not isinstance(reviewer_thread_id, str) or not reviewer_thread_id: + return {"findings": [], "count": 0, "error": "reviewer thread unavailable"} + + try: + findings = asyncio.run(list_findings_async(reviewer_thread_id)) + except Exception as exc: # noqa: BLE001 + return {"findings": [], "count": 0, "error": f"could not load findings: {exc!s}"} + + if status_filter is not None: + findings = [f for f in findings if f.get("status") == status_filter] + compact = [_compact(f) for f in findings] + return {"findings": compact, "count": len(compact)} diff --git a/agent/tools/read_repo_file.py b/agent/tools/read_repo_file.py new file mode 100644 index 00000000..daac7792 --- /dev/null +++ b/agent/tools/read_repo_file.py @@ -0,0 +1,104 @@ +"""Tool: ``read_repo_file``. Read repo files/dirs over the GitHub API (no sandbox). + +The PR chat agent has no sandbox, so it reads source at a specific ref through +the GitHub contents API. Repo coordinates and a read-only token come from the +run config (seeded by the dashboard chat proxy). +""" + +from __future__ import annotations + +import base64 +from typing import Any + +import requests +from langgraph.config import get_config + +from ..utils.github_checks import github_headers + +_GITHUB_API = "https://api.github.com" +_MAX_FILE_BYTES = 256 * 1024 + + +def _chat_repo_context() -> tuple[str, str, str | None, str | None]: + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + if not isinstance(configurable, dict): + configurable = {} + owner = configurable.get("chat_repo_owner") + repo = configurable.get("chat_repo_name") + token = configurable.get("chat_github_token") + head_sha = configurable.get("chat_head_sha") + return ( + owner if isinstance(owner, str) else "", + repo if isinstance(repo, str) else "", + token if isinstance(token, str) and token else None, + head_sha if isinstance(head_sha, str) and head_sha else None, + ) + + +def read_repo_file(path: str, ref: str | None = None) -> dict[str, Any]: + """Read a file (or list a directory) from the PR's repository at a git ref. + + Use this to inspect code beyond the diff — callers, definitions, neighboring + modules, config — at the exact commit under review. The diff itself is + already available as the virtual file ``/pr/diff.patch``. + + Args: + path: Repo-relative path, e.g. ``src/app/main.py`` or ``src/app`` for a + directory listing. Leading slashes are ignored. + ref: Git ref (branch, tag, or SHA). Defaults to the PR head commit. + + Returns: + For a file: ``{success, path, ref, content, truncated}``. + For a directory: ``{success, path, ref, entries}`` where each entry is + ``{name, type, path}``. + On failure: ``{success: False, error}``. + """ + owner, repo, token, head_sha = _chat_repo_context() + if not owner or not repo: + return {"success": False, "error": "repository context unavailable"} + + clean_path = path.strip().lstrip("/") + resolved_ref = (ref or head_sha or "").strip() + params = {"ref": resolved_ref} if resolved_ref else None + url = f"{_GITHUB_API}/repos/{owner}/{repo}/contents/{clean_path}" + headers = github_headers(token or "") + try: + response = requests.get(url, headers=headers, params=params, timeout=30) + except requests.exceptions.RequestException as exc: + return {"success": False, "error": f"GitHub request failed: {exc!s}"} + + if response.status_code == 404: + return {"success": False, "error": f"not found: {clean_path} @ {resolved_ref or 'default'}"} + if response.status_code >= 400: + return {"success": False, "error": f"GitHub returned {response.status_code}"} + + payload = response.json() + if isinstance(payload, list): + entries = [ + { + "name": item.get("name"), + "type": item.get("type"), + "path": item.get("path"), + } + for item in payload + if isinstance(item, dict) + ] + return {"success": True, "path": clean_path, "ref": resolved_ref, "entries": entries} + + if not isinstance(payload, dict) or payload.get("type") != "file": + return {"success": False, "error": f"unsupported content type for {clean_path}"} + + encoded = payload.get("content") + if not isinstance(encoded, str): + return {"success": False, "error": "file content unavailable (too large for contents API)"} + raw = base64.b64decode(encoded) + truncated = len(raw) > _MAX_FILE_BYTES + text = raw[:_MAX_FILE_BYTES].decode("utf-8", errors="replace") + return { + "success": True, + "path": clean_path, + "ref": resolved_ref, + "content": text, + "truncated": truncated, + } diff --git a/agent/tools/search_repo_code.py b/agent/tools/search_repo_code.py new file mode 100644 index 00000000..2fd8c25c --- /dev/null +++ b/agent/tools/search_repo_code.py @@ -0,0 +1,83 @@ +"""Tool: ``search_repo_code``. Search the PR's repository via the GitHub code-search API.""" + +from __future__ import annotations + +from typing import Any + +import requests +from langgraph.config import get_config + +from ..utils.github_checks import github_headers + +_GITHUB_API = "https://api.github.com" + + +def _chat_repo_context() -> tuple[str, str, str | None]: + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + if not isinstance(configurable, dict): + configurable = {} + owner = configurable.get("chat_repo_owner") + repo = configurable.get("chat_repo_name") + token = configurable.get("chat_github_token") + return ( + owner if isinstance(owner, str) else "", + repo if isinstance(repo, str) else "", + token if isinstance(token, str) and token else None, + ) + + +def search_repo_code(query: str, max_results: int = 20) -> dict[str, Any]: + """Search code in the PR's repository for a keyword, symbol, or phrase. + + Backed by GitHub code search, which indexes the repository's default branch + (not arbitrary refs). Use it to locate where a symbol is defined or used, + then ``read_repo_file`` for the surrounding context. For matches within the + changed lines, search the virtual file ``/pr/diff.patch`` instead. + + Args: + query: Search terms. Repo scoping is added automatically. + max_results: Max matches to return (capped at 50). + + Returns: + ``{success, total_count, results}`` where each result is + ``{path, fragments}``; ``{success: False, error}`` on failure. + """ + owner, repo, token = _chat_repo_context() + if not owner or not repo: + return {"success": False, "error": "repository context unavailable"} + + capped = max(1, min(max_results, 50)) + headers = github_headers(token or "") + headers["Accept"] = "application/vnd.github.text-match+json" + params = {"q": f"{query} repo:{owner}/{repo}", "per_page": capped} + try: + response = requests.get( + f"{_GITHUB_API}/search/code", headers=headers, params=params, timeout=30 + ) + except requests.exceptions.RequestException as exc: + return {"success": False, "error": f"GitHub request failed: {exc!s}"} + + if response.status_code == 422: + return {"success": False, "error": "query rejected by GitHub code search"} + if response.status_code >= 400: + return {"success": False, "error": f"GitHub returned {response.status_code}"} + + payload = response.json() + items = payload.get("items") if isinstance(payload, dict) else None + results: list[dict[str, Any]] = [] + for item in items if isinstance(items, list) else []: + if not isinstance(item, dict): + continue + fragments = [ + match.get("fragment") + for match in item.get("text_matches", []) + if isinstance(match, dict) and isinstance(match.get("fragment"), str) + ] + results.append({"path": item.get("path", ""), "fragments": fragments}) + total = payload.get("total_count") if isinstance(payload, dict) else None + return { + "success": True, + "total_count": total if isinstance(total, int) else len(results), + "results": results, + } diff --git a/langgraph.json b/langgraph.json index aad47474..07811c48 100644 --- a/langgraph.json +++ b/langgraph.json @@ -6,6 +6,7 @@ "agent": "agent.server:traced_agent", "reviewer": "agent.reviewer:traced_reviewer_agent", "analyzer": "agent.analyzer:traced_analyzer", + "chat": "agent.chat:traced_chat_agent", "scheduler": "agent.scheduler:get_scheduler", "ci_monitor": "agent.ci_monitor:get_ci_monitor" }, diff --git a/tests/test_review_chat.py b/tests/test_review_chat.py new file mode 100644 index 00000000..bedfca54 --- /dev/null +++ b/tests/test_review_chat.py @@ -0,0 +1,532 @@ +from __future__ import annotations + +import asyncio +import importlib +from types import SimpleNamespace +from typing import Any + +import pytest + +from agent.dashboard import review_chat_api + +# `agent.tools.__init__` rebinds these names to the tool *functions*, shadowing +# the submodules. Import the real modules so we can monkeypatch their globals. +list_review_findings = importlib.import_module("agent.tools.list_review_findings") +read_repo_file = importlib.import_module("agent.tools.read_repo_file") +search_repo_code = importlib.import_module("agent.tools.search_repo_code") + + +# --- chat thread list / delete / title --------------------------------------- + + +def test_derive_title_from_first_user_message() -> None: + params = { + "input": {"messages": [{"type": "human", "content": " Why did we drop\nstructs? "}]} + } + assert review_chat_api._derive_title(params) == "Why did we drop structs?" + + +def test_derive_title_defaults_when_no_message() -> None: + assert review_chat_api._derive_title({"input": {"messages": []}}) == "New chat" + + +def test_derive_title_truncates() -> None: + params = {"input": {"messages": [{"type": "human", "content": "x" * 200}]}} + assert len(review_chat_api._derive_title(params)) == review_chat_api._TITLE_MAX_CHARS + + +@pytest.mark.asyncio +async def test_list_review_chat_threads_scopes_and_maps(monkeypatch) -> None: + captured: dict[str, Any] = {} + + async def search(**kwargs: Any) -> list[dict[str, Any]]: + captured["metadata"] = kwargs.get("metadata") + return [ + { + "thread_id": "c1", + "updated_at": "2026-06-15T00:00:00Z", + "metadata": {"title": "Why structs?"}, + }, + {"thread_id": "c2", "metadata": {}}, # untitled -> default label + ] + + client = SimpleNamespace(threads=SimpleNamespace(search=search)) + monkeypatch.setattr(review_chat_api, "langgraph_client", lambda: client) + + threads = await review_chat_api.list_review_chat_threads("acme", "repo", 7, "octocat") + assert captured["metadata"] == { + "kind": "review_chat", + "github_login": "octocat", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 7, + } + assert threads[0] == { + "thread_id": "c1", + "title": "Why structs?", + "updated_at": "2026-06-15T00:00:00Z", + } + assert threads[1]["title"] == "New chat" + + +@pytest.mark.asyncio +async def test_delete_review_chat_thread_checks_ownership(monkeypatch) -> None: + deleted: list[str] = [] + + async def get(thread_id: str) -> dict[str, Any]: + return { + "thread_id": thread_id, + "metadata": { + "kind": "review_chat", + "github_login": "octocat", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 7, + }, + } + + async def delete(thread_id: str) -> None: + deleted.append(thread_id) + + client = SimpleNamespace(threads=SimpleNamespace(get=get, delete=delete)) + monkeypatch.setattr(review_chat_api, "langgraph_client", lambda: client) + + await review_chat_api.delete_review_chat_thread("acme", "repo", 7, "octocat", "c1") + assert deleted == ["c1"] + + +@pytest.mark.asyncio +async def test_delete_review_chat_thread_rejects_other_user(monkeypatch) -> None: + async def get(thread_id: str) -> dict[str, Any]: + return { + "thread_id": thread_id, + "metadata": {"kind": "review_chat", "github_login": "hubot"}, + } + + async def delete(thread_id: str) -> None: + raise AssertionError("should not delete another user's chat") + + client = SimpleNamespace(threads=SimpleNamespace(get=get, delete=delete)) + monkeypatch.setattr(review_chat_api, "langgraph_client", lambda: client) + + with pytest.raises(Exception): # noqa: B017,PT011 - HTTPException(404) + await review_chat_api.delete_review_chat_thread("acme", "repo", 7, "octocat", "c1") + + +def _patch_thread_metadata(monkeypatch, metadata: dict[str, Any] | None) -> None: + async def get(thread_id: str) -> dict[str, Any]: + if metadata is None: + raise RuntimeError("not found") + return {"thread_id": thread_id, "metadata": metadata} + + client = SimpleNamespace(threads=SimpleNamespace(get=get)) + monkeypatch.setattr(review_chat_api, "langgraph_client", lambda: client) + + +@pytest.mark.asyncio +async def test_assert_chat_thread_access_allows_owner(monkeypatch) -> None: + _patch_thread_metadata( + monkeypatch, + { + "kind": "review_chat", + "github_login": "octocat", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 7, + }, + ) + meta = await review_chat_api.assert_chat_thread_access("ct-1", "acme", "repo", 7, "octocat") + assert meta is not None + + +@pytest.mark.asyncio +async def test_assert_chat_thread_access_missing_thread_returns_none(monkeypatch) -> None: + _patch_thread_metadata(monkeypatch, None) + assert ( + await review_chat_api.assert_chat_thread_access("ct-1", "acme", "repo", 7, "octocat") + is None + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "metadata", + [ + { # another user's chat thread + "kind": "review_chat", + "github_login": "hubot", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 7, + }, + { # a reviewer (non-chat) thread with the same deterministic id space + "kind": "reviewer", + "github_login": "octocat", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 7, + }, + { # right user, wrong PR scope + "kind": "review_chat", + "github_login": "octocat", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 8, + }, + ], +) +async def test_assert_chat_thread_access_rejects_unauthorized(monkeypatch, metadata) -> None: + _patch_thread_metadata(monkeypatch, metadata) + with pytest.raises(Exception): # noqa: B017,PT011 - HTTPException(404) + await review_chat_api.assert_chat_thread_access("ct-1", "acme", "repo", 7, "octocat") + + +# --- tools ------------------------------------------------------------------- + + +def test_list_review_findings_compacts_and_filters(monkeypatch) -> None: + monkeypatch.setattr( + list_review_findings, + "get_config", + lambda: {"configurable": {"reviewer_thread_id": "rt-1"}}, + ) + + async def fake_list(thread_id: str) -> list[dict[str, Any]]: + assert thread_id == "rt-1" + return [ + { + "id": "f1", + "title": "Open one", + "status": "open", + "severity": "high", + "github_review_comment_id": 999, + }, + {"id": "f2", "title": "Closed one", "status": "resolved", "severity": "low"}, + ] + + monkeypatch.setattr(list_review_findings, "list_findings_async", fake_list) + + result = list_review_findings.list_review_findings(status_filter="open") + assert result["count"] == 1 + finding = result["findings"][0] + assert finding["id"] == "f1" + # compact view drops GitHub plumbing fields + assert "github_review_comment_id" not in finding + + +def test_list_review_findings_requires_reviewer_thread(monkeypatch) -> None: + monkeypatch.setattr(list_review_findings, "get_config", lambda: {"configurable": {}}) + result = list_review_findings.list_review_findings() + assert result["count"] == 0 + assert "reviewer thread" in result["error"] + + +def test_read_repo_file_decodes_file(monkeypatch) -> None: + import base64 + + monkeypatch.setattr( + read_repo_file, + "get_config", + lambda: { + "configurable": { + "chat_repo_owner": "acme", + "chat_repo_name": "repo", + "chat_github_token": "tok", + "chat_head_sha": "deadbeef", + } + }, + ) + + captured: dict[str, Any] = {} + + def fake_get(url, headers=None, params=None, timeout=None): + captured["url"] = url + captured["params"] = params + return SimpleNamespace( + status_code=200, + json=lambda: {"type": "file", "content": base64.b64encode(b"hello\nworld").decode()}, + ) + + monkeypatch.setattr(read_repo_file.requests, "get", fake_get) + + result = read_repo_file.read_repo_file("src/app.py") + assert result["success"] is True + assert result["content"] == "hello\nworld" + assert result["ref"] == "deadbeef" # defaults to head sha + assert captured["params"] == {"ref": "deadbeef"} + + +def test_read_repo_file_lists_directory(monkeypatch) -> None: + monkeypatch.setattr( + read_repo_file, + "get_config", + lambda: { + "configurable": { + "chat_repo_owner": "acme", + "chat_repo_name": "repo", + "chat_github_token": "tok", + } + }, + ) + + def fake_get(url, headers=None, params=None, timeout=None): + return SimpleNamespace( + status_code=200, + json=lambda: [ + {"name": "a.py", "type": "file", "path": "src/a.py"}, + {"name": "sub", "type": "dir", "path": "src/sub"}, + ], + ) + + monkeypatch.setattr(read_repo_file.requests, "get", fake_get) + result = read_repo_file.read_repo_file("src") + assert result["success"] is True + assert {e["name"] for e in result["entries"]} == {"a.py", "sub"} + + +def test_read_repo_file_missing_context(monkeypatch) -> None: + monkeypatch.setattr(read_repo_file, "get_config", lambda: {"configurable": {}}) + result = read_repo_file.read_repo_file("src/app.py") + assert result["success"] is False + + +def test_search_repo_code_scopes_to_repo(monkeypatch) -> None: + monkeypatch.setattr( + search_repo_code, + "get_config", + lambda: { + "configurable": { + "chat_repo_owner": "acme", + "chat_repo_name": "repo", + "chat_github_token": "tok", + } + }, + ) + captured: dict[str, Any] = {} + + def fake_get(url, headers=None, params=None, timeout=None): + captured["params"] = params + return SimpleNamespace( + status_code=200, + json=lambda: { + "total_count": 1, + "items": [{"path": "src/a.py", "text_matches": [{"fragment": "def foo()"}]}], + }, + ) + + monkeypatch.setattr(search_repo_code.requests, "get", fake_get) + result = search_repo_code.search_repo_code("foo") + assert result["success"] is True + assert "repo:acme/repo" in captured["params"]["q"] + assert result["results"][0]["path"] == "src/a.py" + + +# --- proxy enrichment -------------------------------------------------------- + + +def _fake_review() -> dict[str, Any]: + return { + "title": "Fix things", + "number": 7, + "full_name": "acme/repo", + "author": "octocat", + "head_ref": "feature", + "base_ref": "main", + "head_sha": "abc123def456", + "findings": [ + { + "id": "f1", + "title": "Bug", + "severity": "high", + "confidence": "high", + "status": "open", + "file": "src/a.py", + "start_line": 5, + "description": "boom", + "group": "bug", + }, + ], + "pr": { + "state": "open", + "body": "desc", + "additions": 1, + "deletions": 2, + "changed_files": 1, + "commits": 1, + "head_sha": "abc123def456", + }, + } + + +def _client_for_enrich(existing_metadata: dict[str, Any] | None) -> tuple[Any, dict[str, Any]]: + captured: dict[str, Any] = {"created": False, "updated": []} + + async def get(thread_id: str) -> dict[str, Any]: + if existing_metadata is None: + raise RuntimeError("not found") + return {"thread_id": thread_id, "metadata": existing_metadata} + + async def create(**kwargs: Any) -> None: + captured["created"] = True + + async def update(**kwargs: Any) -> None: + captured["updated"].append(kwargs.get("metadata")) + + client = SimpleNamespace(threads=SimpleNamespace(get=get, create=create, update=update)) + return client, captured + + +def _patch_enrich_deps(monkeypatch, *, metadata: dict[str, Any] | None) -> dict[str, Any]: + client, captured = _client_for_enrich(metadata) + monkeypatch.setattr(review_chat_api, "langgraph_client", lambda: client) + + async def fake_get_review(owner, repo, pr_number): + return _fake_review() + + async def fake_diff(*, owner, repo, pr_number, token): + return "diff --git a/x b/x\n+added\n" + + async def fake_token(repositories=None): + return "app-token" + + monkeypatch.setattr(review_chat_api, "get_review", fake_get_review) + monkeypatch.setattr(review_chat_api, "fetch_pr_diff", fake_diff) + monkeypatch.setattr(review_chat_api, "get_github_app_installation_token", fake_token) + return captured + + +@pytest.mark.asyncio +async def test_enrich_chat_command_seeds_context_on_create(monkeypatch) -> None: + _patch_enrich_deps(monkeypatch, metadata=None) + command = {"method": "run.start", "params": {"input": {"messages": []}}} + + enriched = await review_chat_api._enrich_chat_command( + command, owner="acme", repo="repo", pr_number=7, login="octocat", thread_id="ct-1" + ) + + params = enriched["params"] + assert params["assistant_id"] == "chat" + configurable = params["config"]["configurable"] + assert configurable["chat_repo_owner"] == "acme" + assert configurable["chat_repo_name"] == "repo" + assert configurable["chat_pr_number"] == 7 + assert configurable["chat_head_sha"] == "abc123def456" + assert configurable["reviewer_thread_id"] == review_chat_api.reviewer_thread_id( + "acme", "repo", 7 + ) + files = params["input"]["files"] + assert set(files) == {"/pr/overview.md", "/pr/diff.patch", "/pr/findings.md"} + + +@pytest.mark.asyncio +async def test_enrich_chat_command_reuses_context_when_head_unchanged(monkeypatch) -> None: + # Stored head matches the current review head -> no reseed. + captured = _patch_enrich_deps( + monkeypatch, metadata={"kind": "review_chat", "chat_head_sha": "abc123def456"} + ) + command = {"method": "run.start", "params": {"input": {"messages": []}}} + + enriched = await review_chat_api._enrich_chat_command( + command, owner="acme", repo="repo", pr_number=7, login="octocat", thread_id="ct-1" + ) + + params = enriched["params"] + # No re-seeding of files when the head hasn't moved. + assert "files" not in params["input"] + assert params["config"]["configurable"]["chat_head_sha"] == "abc123def456" + assert captured["created"] is False + assert captured["updated"] == [] + + +@pytest.mark.asyncio +async def test_enrich_chat_command_reseeds_on_head_change(monkeypatch) -> None: + # Stored head is stale relative to the current review head -> reseed. + captured = _patch_enrich_deps( + monkeypatch, metadata={"kind": "review_chat", "chat_head_sha": "old-stale-sha"} + ) + command = {"method": "run.start", "params": {"input": {"messages": []}}} + + enriched = await review_chat_api._enrich_chat_command( + command, owner="acme", repo="repo", pr_number=7, login="octocat", thread_id="ct-1" + ) + + params = enriched["params"] + files = params["input"]["files"] + assert set(files) == {"/pr/overview.md", "/pr/diff.patch", "/pr/findings.md"} + assert params["config"]["configurable"]["chat_head_sha"] == "abc123def456" + assert {"chat_head_sha": "abc123def456"} in captured["updated"] + + +@pytest.mark.asyncio +async def test_enrich_chat_command_ignores_non_run_start(monkeypatch) -> None: + _patch_enrich_deps(monkeypatch, metadata=None) + command = {"method": "something.else", "params": {}} + enriched = await review_chat_api._enrich_chat_command( + command, owner="acme", repo="repo", pr_number=7, login="octocat", thread_id="ct-1" + ) + assert enriched == command + assert "assistant_id" not in enriched["params"] + + +@pytest.mark.asyncio +async def test_proxy_state_normalizes_missing_thread(monkeypatch) -> None: + async def fake_passthrough(method, thread_id, suffix, body, content_type): + return 404, b"not found", "text/plain" + + async def no_thread(thread_id: str) -> None: + return None + + monkeypatch.setattr(review_chat_api, "_proxy_passthrough", fake_passthrough) + monkeypatch.setattr(review_chat_api, "_get_chat_thread_metadata", no_thread) + status, content, media_type = await review_chat_api.proxy_review_chat_state( + "acme", "repo", 7, "octocat", "ct-1" + ) + assert status == 200 + assert b'"next": []' in content + + +@pytest.mark.asyncio +async def test_proxy_history_normalizes_missing_thread(monkeypatch) -> None: + async def fake_passthrough(method, thread_id, suffix, body, content_type): + return 404, b"not found", "text/plain" + + async def no_thread(thread_id: str) -> None: + return None + + monkeypatch.setattr(review_chat_api, "_proxy_passthrough", fake_passthrough) + monkeypatch.setattr(review_chat_api, "_get_chat_thread_metadata", no_thread) + status, content, _ = await review_chat_api.proxy_review_chat_history( + "acme", "repo", 7, "octocat", "ct-1", b"{}" + ) + assert status == 200 + assert content == b"[]" + + +@pytest.mark.asyncio +async def test_proxy_state_rejects_foreign_thread(monkeypatch) -> None: + async def other_owner(thread_id: str) -> dict[str, Any]: + return { + "kind": "review_chat", + "github_login": "hubot", + "repo_owner": "acme", + "repo_name": "repo", + "pr_number": 7, + } + + async def fake_passthrough(*args, **kwargs): + raise AssertionError("must not proxy a thread the caller doesn't own") + + monkeypatch.setattr(review_chat_api, "_get_chat_thread_metadata", other_owner) + monkeypatch.setattr(review_chat_api, "_proxy_passthrough", fake_passthrough) + with pytest.raises(Exception): # noqa: B017,PT011 - HTTPException(404) + await review_chat_api.proxy_review_chat_state("acme", "repo", 7, "octocat", "ct-1") + + +# --- graph factory guard ----------------------------------------------------- + + +def test_get_chat_agent_returns_trivial_when_not_for_execution() -> None: + from agent.chat import get_chat_agent + + graph = asyncio.run(get_chat_agent({"configurable": {"thread_id": None}})) + assert graph is not None diff --git a/tests/test_team_settings_org_guidelines.py b/tests/test_team_settings_org_guidelines.py index 594ca623..dd5c35f4 100644 --- a/tests/test_team_settings_org_guidelines.py +++ b/tests/test_team_settings_org_guidelines.py @@ -9,8 +9,12 @@ from agent.dashboard.team_settings import ( ORG_GUIDELINES_MAX_CHARS, TeamSettingsUpdate, get_org_review_guidelines, + get_team_default_model, ) +_AGENT_PAIR = ("anthropic:claude-opus-4-8", "high") +_CHAT_PAIR = ("google_genai:gemini-3.5-flash", "low") + def test_org_guidelines_blank_normalizes_to_none() -> None: assert TeamSettingsUpdate(org_guidelines=" ").org_guidelines is None @@ -45,3 +49,69 @@ async def test_get_org_review_guidelines_returns_none_when_unset() -> None: return_value={"org_guidelines": None}, ): assert await get_org_review_guidelines() is None + + +def _settings(**overrides: object) -> dict[str, object]: + base = { + "default_agent_model": _AGENT_PAIR[0], + "default_agent_reasoning_effort": _AGENT_PAIR[1], + "default_chat_model": None, + "default_chat_reasoning_effort": None, + } + base.update(overrides) + return base + + +@pytest.mark.asyncio +async def test_chat_default_inherits_agent_when_unset() -> None: + with patch( + "agent.dashboard.team_settings.get_team_settings", + new_callable=AsyncMock, + return_value=_settings(), + ): + assert await get_team_default_model("chat") == _AGENT_PAIR + + +@pytest.mark.asyncio +async def test_chat_default_uses_chat_model_when_set() -> None: + with patch( + "agent.dashboard.team_settings.get_team_settings", + new_callable=AsyncMock, + return_value=_settings( + default_chat_model=_CHAT_PAIR[0], + default_chat_reasoning_effort=_CHAT_PAIR[1], + ), + ): + assert await get_team_default_model("chat") == _CHAT_PAIR + + +@pytest.mark.asyncio +async def test_chat_default_inherits_agent_when_chat_model_invalid() -> None: + with patch( + "agent.dashboard.team_settings.get_team_settings", + new_callable=AsyncMock, + return_value=_settings( + default_chat_model="bogus:model", + default_chat_reasoning_effort="high", + ), + ): + assert await get_team_default_model("chat") == _AGENT_PAIR + + +def test_team_settings_update_accepts_chat_pair() -> None: + update = TeamSettingsUpdate( + default_chat_model=_CHAT_PAIR[0], + default_chat_reasoning_effort=_CHAT_PAIR[1], + ) + assert update.default_chat_model == _CHAT_PAIR[0] + assert update.default_chat_reasoning_effort == _CHAT_PAIR[1] + + +def test_team_settings_update_rejects_chat_effort_without_model() -> None: + with pytest.raises(ValidationError): + TeamSettingsUpdate(default_chat_reasoning_effort="high") + + +def test_team_settings_update_rejects_unsupported_chat_effort() -> None: + with pytest.raises(ValidationError): + TeamSettingsUpdate(default_chat_model=_CHAT_PAIR[0], default_chat_reasoning_effort="max") diff --git a/ui/src/components/agents/ReviewChat.tsx b/ui/src/components/agents/ReviewChat.tsx new file mode 100644 index 00000000..2b31c82c --- /dev/null +++ b/ui/src/components/agents/ReviewChat.tsx @@ -0,0 +1,641 @@ +import { useCallback, useEffect, useMemo, useRef, useState } from "react" +import { StreamProvider, useStreamContext } from "@langchain/react" +import { overrideFetchImplementation } from "@langchain/langgraph-sdk" +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query" +import { + ArrowClockwiseIcon, + ArrowUpIcon, + CheckIcon, + PlusIcon, + SparkleIcon, + TrashIcon, + XIcon, +} from "@phosphor-icons/react" +import { Menu } from "@base-ui/react/menu" +import type { BaseMessage } from "@langchain/core/messages" + +import type { ReviewChatThread } from "@/lib/api" +import { Markdown } from "@/components/agents/ported" +import { IconButton } from "@/components/ui/button" +import { Textarea } from "@/components/ui/textarea" +import { Skeleton } from "@/components/ui/skeleton" +import { api, reviewChatApiBase } from "@/lib/api" +import { cn } from "@/lib/utils" + +const dashboardFetch: typeof fetch = (input, init) => + fetch(input, { ...init, credentials: "include" }) + +// The SDK's internal Client issues some reads (getState, history) outside the +// transport's fetch; without this they drop the session cookie cross-origin. +overrideFetchImplementation(dashboardFetch) + +const SUGGESTED_PROMPTS = [ + "Summarize the changes in this PR", + "Walk me through the review findings", + "What are the riskiest parts of this change?", +] + +// Kept in sync with the backend's `_derive_title` so the optimistic tab label +// matches the title the server persists for the thread. +const DEFAULT_TITLE = "New chat" +const TITLE_MAX_CHARS = 60 + +function deriveTitle(text: string): string { + const flattened = text.trim().split(/\s+/).join(" ") + return flattened ? flattened.slice(0, TITLE_MAX_CHARS) : DEFAULT_TITLE +} + +function messageType(message: BaseMessage): string { + const candidate = message as unknown as { + getType?: () => string + type?: string + role?: string + } + return candidate.getType?.() ?? candidate.type ?? candidate.role ?? "ai" +} + +function messageText(content: BaseMessage["content"]): string { + if (typeof content === "string") return content + if (!Array.isArray(content)) return "" + return content + .map((block) => { + if (typeof block === "string") return block + if (typeof block === "object" && "text" in block) { + const text = (block as { text?: unknown }).text + return typeof text === "string" ? text : "" + } + return "" + }) + .filter(Boolean) + .join("\n") +} + +// --- Conversation store ------------------------------------------------------ +// +// The client owns the conversation list. Each conversation is a client-minted +// thread id; the thread is created server-side lazily on its first message. +// The server thread list is only used to recover conversations on a fresh +// browser and to reconcile titles — it is never the sole source for the tabs, +// so an empty/lagging server response can no longer make open chats vanish. + +interface Conversation { + id: string + title: string + createdAt: number +} + +interface ChatState { + conversations: Array + activeId: string +} + +function storageKey(owner: string, repo: string, number: number): string { + return `osw:review-chat:${owner}/${repo}/${number}` +} + +function newDraft(): Conversation { + return { id: crypto.randomUUID(), title: DEFAULT_TITLE, createdAt: Date.now() } +} + +function isConversation(value: unknown): value is Conversation { + if (!value || typeof value !== "object") return false + const candidate = value as Record + return ( + typeof candidate.id === "string" && + typeof candidate.title === "string" && + typeof candidate.createdAt === "number" + ) +} + +function loadState(key: string): ChatState { + if (typeof window !== "undefined") { + try { + const raw = window.localStorage.getItem(key) + if (raw) { + const parsed: unknown = JSON.parse(raw) + const source = (parsed ?? {}) as Record + const conversations = Array.isArray(source.conversations) + ? source.conversations.filter(isConversation) + : [] + const first = conversations[0] + if (first) { + const activeId = + typeof source.activeId === "string" && + conversations.some((c) => c.id === source.activeId) + ? source.activeId + : first.id + return { conversations, activeId } + } + } + } catch { + /* fall through to a fresh draft */ + } + } + const draft = newDraft() + return { conversations: [draft], activeId: draft.id } +} + +function saveState(key: string, state: ChatState): void { + if (typeof window === "undefined") return + try { + window.localStorage.setItem(key, JSON.stringify(state)) + } catch { + /* ignore quota / availability errors */ + } +} + +function useConversations(key: string) { + const [state, setState] = useState(() => loadState(key)) + + const update = useCallback( + (fn: (prev: ChatState) => ChatState) => { + setState((prev) => { + const next = fn(prev) + if (next === prev) return prev + saveState(key, next) + return next + }) + }, + [key], + ) + + const select = useCallback( + (conversation: Conversation) => { + update((prev) => ({ + conversations: prev.conversations.some((c) => c.id === conversation.id) + ? prev.conversations + : [...prev.conversations, conversation], + activeId: conversation.id, + })) + }, + [update], + ) + + const newChat = useCallback(() => { + update((prev) => { + const active = prev.conversations.find((c) => c.id === prev.activeId) + // Reuse a pristine, never-sent draft instead of stacking empty tabs. + if (active && active.title === DEFAULT_TITLE) return prev + const draft = newDraft() + return { conversations: [...prev.conversations, draft], activeId: draft.id } + }) + }, [update]) + + // Names a conversation from its first message; later messages don't rename it. + const nameConversation = useCallback( + (id: string, title: string) => { + update((prev) => { + const current = prev.conversations.find((c) => c.id === id) + if (!current || current.title !== DEFAULT_TITLE) return prev + return { + ...prev, + conversations: prev.conversations.map((c) => + c.id === id ? { ...c, title } : c, + ), + } + }) + }, + [update], + ) + + const close = useCallback( + (id: string) => { + update((prev) => { + const index = prev.conversations.findIndex((c) => c.id === id) + if (index === -1) return prev + const remaining = prev.conversations.filter((c) => c.id !== id) + const fallback = remaining[Math.max(0, index - 1)] + if (!fallback) { + const draft = newDraft() + return { conversations: [draft], activeId: draft.id } + } + return { + conversations: remaining, + activeId: prev.activeId === id ? fallback.id : prev.activeId, + } + }) + }, + [update], + ) + + return { ...state, select, newChat, nameConversation, close } +} + +// --- View -------------------------------------------------------------------- + +function EmptyState({ onPick }: { onPick: (prompt: string) => void }) { + return ( +
+

+ I've reviewed this PR. Ask me about the diff, the findings, or the + surrounding code — I have read-only access to the repository. +

+
+ + Suggested prompts + + {SUGGESTED_PROMPTS.map((prompt) => ( + + ))} +
+
+ ) +} + +function LoadingState() { + return ( +
+
+ +
+
+ + +
+
+ ) +} + +function ChatBody({ + onUserSend, + expectsHistory, +}: { + onUserSend: (text: string) => void + expectsHistory: boolean +}) { + const stream = useStreamContext() + const [value, setValue] = useState("") + const endRef = useRef(null) + const messages = stream.messages + const busy = stream.isLoading + // True during the one-time getState hydration when switching to / loading an + // existing thread, before its messages have arrived. + const hydrating = stream.isThreadLoading + + useEffect(() => { + endRef.current?.scrollIntoView({ behavior: "smooth" }) + }, [messages]) + + const send = useCallback( + (text: string) => { + const trimmed = text.trim() + if (!trimmed || busy) return + onUserSend(trimmed) + void stream.submit({ messages: [{ type: "human", content: trimmed }] }) + }, + [busy, stream, onUserSend], + ) + + const visible = messages.filter((message) => { + const type = messageType(message) + if (type !== "human" && type !== "ai") return false + return messageText(message.content).trim().length > 0 + }) + + const submitComposer = () => { + send(value) + setValue("") + } + + // Show the loading placeholder (not the empty/intro state) while an existing + // conversation hydrates, so a chat with messages never flashes its greeting. + const showEmpty = visible.length === 0 && !busy + const showLoading = showEmpty && hydrating && expectsHistory + + return ( +
+ {showLoading ? ( + + ) : showEmpty ? ( + + ) : ( +
+ {visible.map((message, index) => { + const isUser = messageType(message) === "human" + return ( +
+
+ {isUser ? ( + + {messageText(message.content)} + + ) : ( + + )} +
+
+ ) + })} + {busy && ( +
+
+ Thinking… +
+
+ )} +
+
+ )} + +
+
+