From 2d0db83356c2fc0b81b7b6d5c311fe24e33f888d Mon Sep 17 00:00:00 2001 From: Peter Dave Hello <3691490+PeterDaveHello@users.noreply.github.com> Date: Fri, 25 Sep 2026 16:25:44 +0800 Subject: [PATCH 01/12] chore(models): drop retired o1-preview temperature override (#3677) --- docs/docs/usage-guide/configuration_reference.md | 2 +- pr_agent/settings/configuration.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/docs/usage-guide/configuration_reference.md b/docs/docs/usage-guide/configuration_reference.md index cc57545acb..0e5cdd6779 100644 --- a/docs/docs/usage-guide/configuration_reference.md +++ b/docs/docs/usage-guide/configuration_reference.md @@ -117,7 +117,7 @@ to-do list. | `add_user_to_requests` | false | send the current command and PR URL in the OpenAI-compatible "user" request field, for provider-side attribution of requests (e.g. OpenRouter "external_user") | | `reasoning_effort` | "medium" | "none", "minimal", "low", "medium", "high", "xhigh", "max" | | `additional_reasoning_effort_models` | [] | Optional: additional model IDs that accept config.reasoning_effort. Reasoning support is otherwise decided by litellm's bundled model metadata (and the maintained Grok registry), so add an ID here only when litellm does not know the model (custom OpenAI-compatible endpoints). Model IDs match exactly or through any provider prefix (e.g. "deepseek-v4-flash-0731" matches "openai/deepseek-v4-flash-0731"). LiteLLM whitelists reasoning_effort through allowed_openai_params for OpenAI-compatible models it does not recognize, so the parameter reaches the endpoint. The default "medium" may be rejected by providers that accept a different subset (e.g. "none"/"low"/"high"/"max"); adding a custom model id now surfaces a provider-side error instead of the previous silent drop. | -| `no_temperature_models` | ["deepseek/deepseek-reasoner", "o1-mini", "o1-mini-2024-09-12", "o1", "o1-2024-12-17", "o3-mini", "o3-mini-2025-01-31", "o1-preview", "o3", "o3-2025-04-16", "o4-mini", "o4-mini-2025-04-16", "gpt-5.1-codex", "gpt-5.1-codex-mini", "gpt-5.2-codex", "gpt-5.3-codex", "gpt-5-mini"] | Optional: model IDs that must never receive the temperature parameter, on top of what litellm's parameter metadata reports. Temperature support is otherwise decided by litellm.get_supported_openai_params() (mirroring reasoning_effort), so add an ID here when litellm reports temperature as supported but the provider rejects it, or when an OpenAI-compatible endpoint accepts but you still want it dropped. Adaptive-thinking Claude models (Opus 4.7/4.8 and Opus/Sonnet/Fable 5) never receive temperature. Model IDs match exactly or through any provider prefix. The defaults below preserve the former static registry entries that litellm's metadata still marks temperature-capable; the probe diff for this change is posted on the issue. | +| `no_temperature_models` | ["deepseek/deepseek-reasoner", "o1-mini", "o1-mini-2024-09-12", "o1", "o1-2024-12-17", "o3-mini", "o3-mini-2025-01-31", "o3", "o3-2025-04-16", "o4-mini", "o4-mini-2025-04-16", "gpt-5.1-codex", "gpt-5.1-codex-mini", "gpt-5.2-codex", "gpt-5.3-codex", "gpt-5-mini"] | Optional: model IDs that must never receive the temperature parameter, on top of what litellm's parameter metadata reports. Temperature support is otherwise decided by litellm.get_supported_openai_params() (mirroring reasoning_effort), so add an ID here when litellm reports temperature as supported but the provider rejects it, or when an OpenAI-compatible endpoint accepts but you still want it dropped. Adaptive-thinking Claude models (Opus 4.7/4.8 and Opus/Sonnet/Fable 5) never receive temperature. Model IDs match exactly or through any provider prefix. The defaults below preserve the former static registry entries that litellm's metadata still marks temperature-capable; the probe diff for this change is posted on the issue. | **extended thinking for Claude reasoning models** | Key | Default | Description | diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index a8a2b6ebc8..0f06d48100 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -117,7 +117,7 @@ additional_reasoning_effort_models = [] # Model IDs match exactly or through any provider prefix. The defaults below preserve # the former static registry entries that litellm's metadata still marks # temperature-capable; the probe diff for this change is posted on the issue. -no_temperature_models = ["deepseek/deepseek-reasoner", "o1-mini", "o1-mini-2024-09-12", "o1", "o1-2024-12-17", "o3-mini", "o3-mini-2025-01-31", "o1-preview", "o3", "o3-2025-04-16", "o4-mini", "o4-mini-2025-04-16", "gpt-5.1-codex", "gpt-5.1-codex-mini", "gpt-5.2-codex", "gpt-5.3-codex", "gpt-5-mini"] +no_temperature_models = ["deepseek/deepseek-reasoner", "o1-mini", "o1-mini-2024-09-12", "o1", "o1-2024-12-17", "o3-mini", "o3-mini-2025-01-31", "o3", "o3-2025-04-16", "o4-mini", "o4-mini-2025-04-16", "gpt-5.1-codex", "gpt-5.1-codex-mini", "gpt-5.2-codex", "gpt-5.3-codex", "gpt-5-mini"] # extended thinking for Claude reasoning models enable_claude_extended_thinking = false # Set to true to enable extended thinking feature extended_thinking_budget_tokens = 2048 From 5fd82422f1a59b9121527bae0cbfb4ce7fa1d9c3 Mon Sep 17 00:00:00 2001 From: Peter Dave Hello <3691490+PeterDaveHello@users.noreply.github.com> Date: Fri, 25 Sep 2026 16:26:00 +0800 Subject: [PATCH 02/12] test: pin LiteLLM future Claude fallbacks (#3660) --- .../test_litellm_future_claude_fallbacks.py | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 tests/unittest/test_litellm_future_claude_fallbacks.py diff --git a/tests/unittest/test_litellm_future_claude_fallbacks.py b/tests/unittest/test_litellm_future_claude_fallbacks.py new file mode 100644 index 0000000000..156f276cd7 --- /dev/null +++ b/tests/unittest/test_litellm_future_claude_fallbacks.py @@ -0,0 +1,36 @@ +import litellm +from litellm.llms.bedrock.chat.converse_transformation import AmazonConverseConfig + +_FUTURE_CLAUDE_MODEL = "claude-opus-9-1" +_FUTURE_BEDROCK_MODEL_ID = "eu.anthropic.claude-opus-9-1" +_FUTURE_BEDROCK_MODEL = f"bedrock/{_FUTURE_BEDROCK_MODEL_ID}" + + +def test_litellm_routes_unmapped_future_claude_to_anthropic(): + # Keep the synthetic id absent from the bundled map to exercise fallback routing. + assert _FUTURE_CLAUDE_MODEL not in litellm.model_cost + + resolved_model, provider, _, _ = litellm.get_llm_provider(model=_FUTURE_CLAUDE_MODEL) + + assert resolved_model == _FUTURE_CLAUDE_MODEL + assert provider == "anthropic" + + +def test_litellm_preserves_adaptive_thinking_for_unmapped_future_bedrock_claude(): + # Keep both lookup forms absent so capability resolution must use fallback rules. + assert _FUTURE_BEDROCK_MODEL_ID not in litellm.model_cost + assert _FUTURE_BEDROCK_MODEL not in litellm.model_cost + + info = litellm.get_model_info( + _FUTURE_BEDROCK_MODEL_ID, + custom_llm_provider="bedrock", + ) + mapped = AmazonConverseConfig().map_openai_params( + {"thinking": {"type": "adaptive"}}, + {}, + _FUTURE_BEDROCK_MODEL, + False, + ) + + assert info["supports_adaptive_thinking"] is True + assert mapped["thinking"] == {"type": "adaptive"} From 77d22228d1456aee97332863b99de2c901aae1ac Mon Sep 17 00:00:00 2001 From: Alex Tumanov Date: Fri, 25 Sep 2026 03:30:01 -0500 Subject: [PATCH 03/12] fix(artifacts): preserve CI context after command settings (#3674) Co-authored-by: Alex Tumanov <6143578+oleksii-tumanov@users.noreply.github.com> --- pr_agent/agent/pr_agent.py | 3 + pr_agent/algo/artifacts.py | 42 ++-- pr_agent/cli.py | 7 +- tests/unittest/test_artifacts.py | 9 +- tests/unittest/test_cli_artifact_context.py | 94 +++++++++ tests/unittest/test_cli_plain_diff_mode.py | 25 +++ .../test_github_action_runner_core.py | 185 ++++++++++++++++++ tests/unittest/test_pr_agent_routing.py | 86 ++++++++ 8 files changed, 433 insertions(+), 18 deletions(-) diff --git a/pr_agent/agent/pr_agent.py b/pr_agent/agent/pr_agent.py index 14837b4280..9bf4e70d14 100644 --- a/pr_agent/agent/pr_agent.py +++ b/pr_agent/agent/pr_agent.py @@ -10,6 +10,7 @@ from pr_agent.algo.ai_handlers.base_ai_handler import BaseAiHandler from pr_agent.algo.ai_handlers.litellm_ai_handler import LiteLLMAIHandler +from pr_agent.algo.artifacts import reapply_artifact_context from pr_agent.algo.cli_args import CliArgs from pr_agent.algo.comment_identity import add_comment_identity, comment_matches_identity from pr_agent.algo.utils import update_settings_from_args @@ -372,6 +373,8 @@ async def _run_command( span.set_attribute("error.message", f"Unknown command: {action}") return False + reapply_artifact_context() + # Only after validation: an unknown action is arbitrary user input and # must not become a span name, span attribute, or metric label. span.update_name(f"pr_agent {action}") diff --git a/pr_agent/algo/artifacts.py b/pr_agent/algo/artifacts.py index 1c12eb0073..eddd7f00dc 100644 --- a/pr_agent/algo/artifacts.py +++ b/pr_agent/algo/artifacts.py @@ -1,4 +1,5 @@ import os +from contextvars import ContextVar from pathlib import Path from typing import Optional @@ -12,6 +13,28 @@ "It was produced by a prior CI step." ) +_artifact_context: ContextVar[Optional[tuple[str, frozenset[str]]]] = ContextVar( + "pr_agent_artifact_context", default=None +) + + +def _append_artifact_context(settings, text, targets): + separator = "\n======\n\n" + for key in settings: + setting = settings.get(key) + if isinstance(setting, dynaconf.DataDict) and key.lower() in targets and hasattr(setting, "extra_instructions"): + extra_instructions = str(setting.extra_instructions or "") + if text not in extra_instructions: + setting.extra_instructions = extra_instructions + separator + text if extra_instructions else text + + +def reapply_artifact_context() -> None: + """Compose already-read context after final command settings, without file I/O.""" + payload = _artifact_context.get() + if payload is not None: + text, targets = payload + _append_artifact_context(get_settings(), text, targets) + def resolve_artifact_path(path: str) -> Optional[Path]: if not path: @@ -119,6 +142,10 @@ def inject_artifact_context() -> None: ARTIFACT_PATH in the environment turns the feature on by itself. Called once before a command runs, by the GitHub Action runner and by the CLI. """ + # Each ingress prepares a new payload. Failed, empty, or disabled ingress must + # not leave an earlier task's payload available for dispatcher reapplication. + _artifact_context.set(None) + artifact_path_env = ( os.environ.get("ARTIFACT_PATH") or os.environ.get("PR_AGENT_ARTIFACT_PATH") or "" ).strip() @@ -147,18 +174,9 @@ def inject_artifact_context() -> None: ) if isinstance(target_tools, str): target_tools = [t.strip() for t in target_tools.split(",") if t.strip()] - target_tools = {str(t).lower() for t in target_tools} - separator = "\n======\n\n" - for key in get_settings(): - setting = get_settings().get(key) - if isinstance(setting, dynaconf.DataDict): - if key.lower() in target_tools and hasattr(setting, 'extra_instructions'): - extra_instructions = str(setting.extra_instructions or "") - if artifact_text not in extra_instructions: - setting.extra_instructions = ( - extra_instructions + separator + artifact_text - if extra_instructions else artifact_text - ) + target_tools = frozenset(str(t).lower() for t in target_tools) + _artifact_context.set((artifact_text, target_tools)) + _append_artifact_context(get_settings(), artifact_text, target_tools) get_logger().info(f"Injected artifact context into tools: {target_tools}") except (OSError, ValueError, TypeError) as e: get_logger().warning(f"Failed to process artifacts: {e}", exc_info=True) diff --git a/pr_agent/cli.py b/pr_agent/cli.py index c13086a1c5..c85a898095 100644 --- a/pr_agent/cli.py +++ b/pr_agent/cli.py @@ -215,11 +215,10 @@ def run(inargs=None, args=None): env_branch = (os.environ.get("PR_AGENT_CONFIG_BRANCH") or "").strip() get_settings().set("CONFIG.CONFIG_BRANCH", cli_branch or env_branch or None) get_settings().set("CONFIG.EXTRA_CONFIG_URL", getattr(args, "extra_config_url", None)) - # A CI artifact (see [artifacts]) reaches the prompts from the environment or the settings files, - # the same way it does under the GitHub Action, so any pipeline that runs the CLI can supply one. - inject_artifact_context() - async def inner(): + # A CI artifact (see [artifacts]) reaches prompts from the environment or settings files, + # the same way it does under the GitHub Action. Each asyncio.run gets a fresh task context. + inject_artifact_context() if args.issue_url: result = await asyncio.create_task(PRAgent().handle_request(args.issue_url, [command] + args.rest)) else: diff --git a/tests/unittest/test_artifacts.py b/tests/unittest/test_artifacts.py index 5713a8f8c9..42633ed64f 100644 --- a/tests/unittest/test_artifacts.py +++ b/tests/unittest/test_artifacts.py @@ -5,6 +5,7 @@ from pr_agent.algo.artifacts import ( DEFAULT_ARTIFACT_INSTRUCTIONS, + _artifact_context, _read_and_truncate, format_artifact_content, inject_artifact_context, @@ -275,8 +276,12 @@ def settings(self): s.set("artifacts.target_tools", ["pr_reviewer", "pr_description", "pr_code_suggestions"]) for tool in ("pr_reviewer", "pr_description", "pr_code_suggestions"): s.set(f"{tool}.extra_instructions", "") - yield s - restore_settings(snapshot) + token = _artifact_context.set(None) + try: + yield s + finally: + _artifact_context.reset(token) + restore_settings(snapshot) @pytest.fixture def report(self, tmp_path): diff --git a/tests/unittest/test_cli_artifact_context.py b/tests/unittest/test_cli_artifact_context.py index abf2f12070..79df6b111a 100644 --- a/tests/unittest/test_cli_artifact_context.py +++ b/tests/unittest/test_cli_artifact_context.py @@ -1,7 +1,15 @@ +import copy from types import SimpleNamespace from unittest.mock import MagicMock, patch +import pytest +from starlette_context import request_cycle_context + from pr_agent import cli +from pr_agent.agent import pr_agent as agent_module +from pr_agent.algo import artifacts +from pr_agent.config_loader import get_settings +from pr_agent.git_providers import utils as git_utils def test_run_injects_the_artifact_context_before_handling_the_request(): @@ -20,3 +28,89 @@ async def fake_handle_request(*_args, **_kwargs): cli.run(inargs=["--pr_url=https://github.com/a/b/pull/1", "review"]) assert order == ["inject", "handle_request"] + + +def test_cli_disabled_ingress_does_not_reuse_a_calling_context_payload(monkeypatch): + observed = [] + + class FakeAgent: + async def handle_request(self, *_args, **_kwargs): + artifacts.reapply_artifact_context() + observed.append(str(get_settings().pr_reviewer.extra_instructions)) + return True + + with request_cycle_context({"settings": copy.deepcopy(get_settings())}): + settings = get_settings() + settings.set("ARTIFACTS.ENABLE", False) + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", "") + monkeypatch.delenv("ARTIFACT_PATH", raising=False) + monkeypatch.delenv("PR_AGENT_ARTIFACT_PATH", raising=False) + payload = ("STALE_ARTIFACT", frozenset({"pr_reviewer"})) + token = artifacts._artifact_context.set(payload) + try: + monkeypatch.setattr(cli, "PRAgent", FakeAgent) + monkeypatch.setattr(cli, "litellm_callbacks_registered", lambda: False) + + cli.run(inargs=["--pr_url=https://example.com/org/repo/pull/1", "review"]) + + assert observed == [""] + assert artifacts._artifact_context.get() == payload + finally: + artifacts._artifact_context.reset(token) + + +@pytest.mark.parametrize("command_override", [False, True]) +def test_cli_artifact_survives_effective_instructions(monkeypatch, tmp_path, command_override): + report = tmp_path / "report.txt" + report.write_text("CI_FAILURE_MARKER") + replacement = tmp_path / "replacement.txt" + replacement.write_text("UNSELECTED_ARTIFACT") + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + monkeypatch.setenv("ARTIFACT_PATH", str(report)) + + class Provider: + def get_repo_settings(self): + return ''' +[artifacts] +artifact_path = "replacement.txt" +target_tools = ["pr_description"] +[pr_reviewer] +extra_instructions = "Repository instructions" +''' + + observed = [] + + class RecordingReviewer: + def __init__(self, *_args, **_kwargs): + observed.append((get_settings().pr_reviewer.extra_instructions, + get_settings().pr_description.extra_instructions)) + + async def run(self): + pass + + monkeypatch.setattr(git_utils, "get_git_provider_with_context", lambda _url: Provider()) + monkeypatch.setitem(agent_module.command2class, "review", RecordingReviewer) + monkeypatch.setattr(agent_module, "flush_telemetry", lambda: None) + monkeypatch.setattr(cli, "litellm_callbacks_registered", lambda: False) + read = MagicMock(wraps=artifacts._read_and_truncate) + monkeypatch.setattr(artifacts, "_read_and_truncate", read) + + with request_cycle_context({"settings": copy.deepcopy(get_settings())}): + settings = get_settings() + settings.set("ARTIFACTS.TARGET_TOOLS", ["pr_reviewer"]) + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", "") + settings.set("PR_DESCRIPTION.EXTRA_INSTRUCTIONS", "") + settings.set("CONFIG.RESPONSE_LANGUAGE", "es") + argv = ["--pr_url=https://example.invalid/org/repo/pull/1", "review"] + if command_override: + argv.append('--pr_reviewer.extra_instructions="Command instructions"') + cli.run(argv) + + assert len(observed) == 1 + reviewer, description = observed[0] + assert reviewer.startswith("Command instructions" if command_override else "Repository instructions") + assert reviewer.count("CI_FAILURE_MARKER") == 1 + assert "locale code: 'es'" in reviewer + assert "UNSELECTED_ARTIFACT" not in reviewer + assert "CI_FAILURE_MARKER" not in description + read.assert_called_once_with(report.resolve(), 50000) diff --git a/tests/unittest/test_cli_plain_diff_mode.py b/tests/unittest/test_cli_plain_diff_mode.py index e9d84c41e3..f1165a52e9 100644 --- a/tests/unittest/test_cli_plain_diff_mode.py +++ b/tests/unittest/test_cli_plain_diff_mode.py @@ -445,3 +445,28 @@ async def handle_request(self, target, request, notify=None): run(inargs=["--stdin", "--json-output", str(output), "review"]) assert captured["json_output_path"] == str(output) + + +def test_plain_diff_fake_handler_sees_eager_artifact_without_new_keyword(monkeypatch, tmp_path): + """Keep the three-argument fake-handler contract while CLI owns the scope.""" + artifact = tmp_path / "artifact.txt" + artifact.write_text("PLAIN_DIFF_ARTIFACT", encoding="utf-8") + captured = {} + + class FakeAgent: + async def handle_request(self, target, request, notify=None): + captured["target"] = target + captured["request"] = request + captured["instructions"] = str(get_settings().pr_reviewer.extra_instructions) + return True + + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + monkeypatch.setenv("ARTIFACT_PATH", str(artifact)) + monkeypatch.setattr("pr_agent.cli.PRAgent", FakeAgent) + monkeypatch.setattr("sys.stdin", io.StringIO(_DIFF)) + + run(inargs=["--stdin", "review"]) + + assert captured["target"] == "local_diff" + assert captured["request"] == ["review"] + assert captured["instructions"].count("PLAIN_DIFF_ARTIFACT") == 1 diff --git a/tests/unittest/test_github_action_runner_core.py b/tests/unittest/test_github_action_runner_core.py index 9d7529c96e..46bef8622b 100644 --- a/tests/unittest/test_github_action_runner_core.py +++ b/tests/unittest/test_github_action_runner_core.py @@ -6,7 +6,9 @@ import pr_agent.agent.pr_agent as pr_agent_module import pr_agent.servers.github_action_runner as github_action_runner +from pr_agent.algo import artifacts from pr_agent.config_loader import get_settings +from pr_agent.git_providers import utils as git_utils from pr_agent.git_providers.github_provider import IncompletePullRequestFilesError @@ -209,6 +211,21 @@ def restore_github_settings(): getattr(settings, section).extra_instructions = extra_instructions +@pytest.fixture +def restore_artifact_action_settings(): + """Keep B1 ingress settings local to the Action tests that exercise them.""" + settings = get_settings() + originals = { + key: copy.deepcopy(settings.get(key, None)) + for key in ("ARTIFACTS", "PR_REVIEWER", "PR_DESCRIPTION", "PR_CODE_SUGGESTIONS") + } + original_use_repo_settings = settings.get("CONFIG.USE_REPO_SETTINGS_FILE", None) + yield settings + for key, value in originals.items(): + settings.set(key, value, merge=False) + settings.set("CONFIG.USE_REPO_SETTINGS_FILE", original_use_repo_settings) + + def _write_synchronize_event(tmp_path, before_sha="abc", after_sha="def", merge_commit_sha=None, sender_type="User"): payload = { "action": "synchronize", @@ -897,3 +914,171 @@ async def test_issue_comment_body_reaches_the_agent_with_its_case_preserved( assert handled, "comment was not handled" assert handled[0][1] == body + + +@pytest.mark.asyncio +async def test_action_configured_commands_reapply_one_artifact_after_real_repo_merges( + monkeypatch, tmp_path, restore_github_settings, restore_artifact_action_settings, +): + """Verify repeated repository merges share the Action's single artifact read.""" + settings = restore_artifact_action_settings + artifact = tmp_path / "report.txt" + artifact.write_text("ACTION_CONFIGURED_ARTIFACT", encoding="utf-8") + observed = [] + + class Provider: + def get_repo_settings(self): + return '[pr_reviewer]\nextra_instructions = "Repository instruction"\n' + + def get_files(self): + return [] + + class RecordingReviewer: + def __init__(self, _pr_url, ai_handler=None, args=None): + observed.append(str(get_settings().pr_reviewer.extra_instructions)) + + async def run(self): + return None + + settings.set("ARTIFACTS", { + "enable": True, + "artifact_path": str(artifact), + "target_tools": ["pr_reviewer"], + }, merge=False) + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", "Host instruction") + settings.set("CONFIG.USE_REPO_SETTINGS_FILE", True) + settings.set("GITHUB_ACTION_CONFIG", { + "handle_push_trigger": True, + "push_commands": ["/review", "/review"], + "push_trigger_ignore_merge_commits": False, + "push_trigger_ignore_bot_commits": False, + }, merge=False) + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_synchronize_event(tmp_path))) + monkeypatch.setenv("GITHUB_TOKEN", "token") + monkeypatch.setattr(git_utils, "get_git_provider_with_context", lambda _url: Provider()) + monkeypatch.setitem(pr_agent_module.command2class, "review", RecordingReviewer) + monkeypatch.setattr(pr_agent_module, "flush_telemetry", lambda: None) + read = Mock(wraps=artifacts._read_and_truncate) + monkeypatch.setattr(artifacts, "_read_and_truncate", read) + + await github_action_runner.run_action() + + assert len(observed) == 2 + assert all(text.startswith("Repository instruction") for text in observed) + assert all(text.count("ACTION_CONFIGURED_ARTIFACT") == 1 for text in observed) + read.assert_called_once_with(artifact.resolve(), 50000) + + +@pytest.mark.asyncio +async def test_direct_action_and_workflow_run_keep_artifact_before_ci_conclusion( + monkeypatch, tmp_path, restore_github_settings, restore_artifact_action_settings, +): + settings = restore_artifact_action_settings + artifact = tmp_path / "report.txt" + artifact.write_text("ACTION_DIRECT_ARTIFACT", encoding="utf-8") + observations = [] + + class RecordingTool: + section = "pr_reviewer" + + def __init__(self, _pr_url): + observations.append((self.section, str(getattr(settings, self.section).extra_instructions))) + + async def run(self): + return None + + class Description(RecordingTool): + section = "pr_description" + + class Reviewer(RecordingTool): + section = "pr_reviewer" + + class Suggestions(RecordingTool): + section = "pr_code_suggestions" + + settings.set("ARTIFACTS", { + "enable": True, + "artifact_path": str(artifact), + "target_tools": ["pr_reviewer", "pr_description", "pr_code_suggestions"], + }, merge=False) + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + monkeypatch.setattr(github_action_runner, "apply_repo_settings", lambda _url: None) + monkeypatch.setattr(github_action_runner, "PRDescription", Description) + monkeypatch.setattr(github_action_runner, "PRReviewer", Reviewer) + monkeypatch.setattr(github_action_runner, "PRCodeSuggestions", Suggestions) + + def action_settings(key, default=None): + values = { + "GITHUB_ACTION_CONFIG.PR_ACTIONS": ["opened"], + "GITHUB_ACTION.AUTO_DESCRIBE": True, + "GITHUB_ACTION.AUTO_REVIEW": True, + "GITHUB_ACTION.AUTO_IMPROVE": True, + "GITHUB_ACTION_CONFIG.ENABLE_OUTPUT": True, + } + return values.get(key, default) + + monkeypatch.setattr(github_action_runner, "get_setting_or_env", action_settings) + monkeypatch.setenv("GITHUB_EVENT_NAME", "pull_request") + event = tmp_path / "opened.json" + event.write_text(json.dumps({"action": "opened", "pull_request": { + "url": "https://api.github.com/repos/org/repo/pulls/1", + "html_url": "https://github.com/org/repo/pull/1", + }})) + monkeypatch.setenv("GITHUB_EVENT_PATH", str(event)) + monkeypatch.setenv("GITHUB_TOKEN", "token") + + await github_action_runner.run_action() + + assert {section for section, _text in observations} == { + "pr_description", "pr_reviewer", "pr_code_suggestions", + } + assert all(text.count("ACTION_DIRECT_ARTIFACT") == 1 for _section, text in observations) + + observations.clear() + monkeypatch.setenv("GITHUB_EVENT_NAME", "workflow_run") + monkeypatch.setenv( + "GITHUB_EVENT_PATH", str(_write_workflow_run_event(tmp_path, conclusion="failure")) + ) + + await github_action_runner.run_action() + + reviewer_text = next(text for section, text in observations if section == "pr_reviewer") + assert reviewer_text.count("ACTION_DIRECT_ARTIFACT") == 1 + assert reviewer_text.index("ACTION_DIRECT_ARTIFACT") < reviewer_text.index("concluded: failure") + + +@pytest.mark.asyncio +async def test_non_pr_action_routes_do_not_read_artifacts( + monkeypatch, tmp_path, restore_github_settings, restore_artifact_action_settings, +): + artifact = tmp_path / "report.txt" + artifact.write_text("NON_PR_ARTIFACT", encoding="utf-8") + settings = restore_artifact_action_settings + settings.set("ARTIFACTS", { + "enable": True, + "artifact_path": str(artifact), + "target_tools": ["pr_reviewer"], + }, merge=False) + + def fail_if_read(*_args, **_kwargs): + pytest.fail("a non-PR Action route must not read an artifact") + + handled = [] + _patch_issue_comment_deps(monkeypatch, handled) + monkeypatch.setattr(artifacts, "_read_and_truncate", fail_if_read) + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + monkeypatch.setenv("GITHUB_EVENT_NAME", "issue_comment") + monkeypatch.setenv("GITHUB_EVENT_PATH", str(_write_plain_issue_comment_event(tmp_path))) + monkeypatch.setenv("GITHUB_TOKEN", "token") + + await github_action_runner.run_action() + + assert handled == [("https://api.github.com/repos/org/repo/issues/1", "/ask what is this issue about?")] + + monkeypatch.setenv("GITHUB_EVENT_NAME", "workflow_run") + monkeypatch.setenv( + "GITHUB_EVENT_PATH", str(_write_workflow_run_event(tmp_path, originating_event="push")) + ) + await github_action_runner.run_action() diff --git a/tests/unittest/test_pr_agent_routing.py b/tests/unittest/test_pr_agent_routing.py index 1b6df8a969..8d058e8ab7 100644 --- a/tests/unittest/test_pr_agent_routing.py +++ b/tests/unittest/test_pr_agent_routing.py @@ -4,6 +4,7 @@ import pytest import pr_agent.agent.pr_agent as pr_agent_module +from pr_agent.algo import artifacts from pr_agent.config_loader import get_settings @@ -465,6 +466,91 @@ async def run(self): assert calls == [("https://example/pr/1", False, True, [], "fake-ai")] +@pytest.mark.asyncio +async def test_auto_review_reapplies_prepared_artifact_without_notifying(monkeypatch, tmp_path): + artifact = tmp_path / "artifact.txt" + artifact.write_text("AUTO_REVIEW_ARTIFACT", encoding="utf-8") + settings = get_settings() + original_artifacts = settings.get("ARTIFACTS") + original_instructions = settings.pr_reviewer.extra_instructions + observed = [] + notify = Mock() + + class FakeReviewer: + def __init__(self, _pr_url, is_answer=False, is_auto=False, args=None, ai_handler=None): + observed.append((is_answer, is_auto, args, str(settings.pr_reviewer.extra_instructions))) + + async def run(self): + return None + + try: + settings.set("ARTIFACTS", { + "enable": True, + "artifact_path": str(artifact), + "target_tools": ["pr_reviewer"], + }, merge=False) + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", "Base instructions") + monkeypatch.setenv("GITHUB_WORKSPACE", str(tmp_path)) + _patch_request_dependencies( + monkeypatch, + update_settings_fn=lambda args: ["--kept"], + ) + + def replace_instructions(_pr_url): + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", "Repository instructions") + + monkeypatch.setattr(pr_agent_module, "apply_repo_settings", replace_instructions) + monkeypatch.setattr(pr_agent_module, "PRReviewer", FakeReviewer) + + artifacts.inject_artifact_context() + handled = await pr_agent_module.PRAgent(ai_handler="fake-ai")._handle_request( + "https://example/pr/1", "/auto_review", notify + ) + + assert handled is True + assert [(is_answer, is_auto, args) for is_answer, is_auto, args, _text in observed] == [ + (False, True, ["--kept"]) + ] + assert observed[0][3].startswith("Repository instructions") + assert observed[0][3].count("AUTO_REVIEW_ARTIFACT") == 1 + notify.assert_not_called() + finally: + settings.set("ARTIFACTS", original_artifacts, merge=False) + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", original_instructions) + + +@pytest.mark.asyncio +async def test_unscoped_dispatcher_does_not_load_artifact_or_change_instructions(monkeypatch): + settings = get_settings() + original_instructions = settings.pr_reviewer.extra_instructions + observed = [] + + class FakeTool: + def __init__(self, _pr_url, ai_handler=None, args=None): + observed.append(str(settings.pr_reviewer.extra_instructions)) + + async def run(self): + return None + + def fail_if_loaded(): + pytest.fail("dispatcher without an ingress must not load an artifact") + + token = artifacts._artifact_context.set(None) + try: + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", "Unscoped instructions") + _patch_request_dependencies(monkeypatch) + monkeypatch.setattr(artifacts, "load_artifact", fail_if_loaded) + monkeypatch.setitem(pr_agent_module.command2class, "review", FakeTool) + + handled = await pr_agent_module.PRAgent()._handle_request("https://example/pr/1", "/review") + + assert handled is True + assert observed == ["Unscoped instructions"] + finally: + settings.set("PR_REVIEWER.EXTRA_INSTRUCTIONS", original_instructions) + artifacts._artifact_context.reset(token) + + @pytest.mark.asyncio async def test_handle_request_returns_false_for_unknown_command(monkeypatch): _patch_request_dependencies(monkeypatch) From d39c47c25ec2632a6d221bed4a439a8b57a836af Mon Sep 17 00:00:00 2001 From: Alex Tumanov Date: Fri, 25 Sep 2026 03:30:15 -0500 Subject: [PATCH 04/12] fix(models): avoid replaying rejected model requests (#3683) Co-authored-by: Alex Tumanov <6143578+oleksii-tumanov@users.noreply.github.com> --- .../algo/ai_handlers/langchain_ai_handler.py | 4 +- .../algo/ai_handlers/litellm_ai_handler.py | 3 +- .../algo/ai_handlers/openai_ai_handler.py | 4 +- .../test_litellm_chat_completion_core.py | 51 ++++++++++- tests/unittest/test_litellm_retry_config.py | 22 +++++ .../test_openai_compat_handler_retry.py | 89 +++++++++++++++++++ 6 files changed, 169 insertions(+), 4 deletions(-) create mode 100644 tests/unittest/test_openai_compat_handler_retry.py diff --git a/pr_agent/algo/ai_handlers/langchain_ai_handler.py b/pr_agent/algo/ai_handlers/langchain_ai_handler.py index c272f141f4..6ecde593e6 100644 --- a/pr_agent/algo/ai_handlers/langchain_ai_handler.py +++ b/pr_agent/algo/ai_handlers/langchain_ai_handler.py @@ -64,7 +64,9 @@ async def _create_chat_async(self, deployment_id=None): raise ValueError(error_msg) from e @retry( - retry=retry_if_exception_type(openai.APIError) & retry_if_not_exception_type(openai.RateLimitError), + retry=retry_if_exception_type(openai.APIError) & retry_if_not_exception_type( + (openai.RateLimitError, openai.BadRequestError, openai.UnprocessableEntityError) + ), stop=stop_after_attempt(OPENAI_RETRIES), ) async def chat_completion( diff --git a/pr_agent/algo/ai_handlers/litellm_ai_handler.py b/pr_agent/algo/ai_handlers/litellm_ai_handler.py index 4fcb62632f..527f23cc26 100644 --- a/pr_agent/algo/ai_handlers/litellm_ai_handler.py +++ b/pr_agent/algo/ai_handlers/litellm_ai_handler.py @@ -258,8 +258,9 @@ def _should_retry_same_model(exc: BaseException) -> bool: With config.retry_same_model_on_timeout set to false, a timed-out call is handed to the fallback-models loop instead of being replayed on the model that just missed the deadline. + Request validation errors also surface immediately rather than replaying the same request. """ - if isinstance(exc, openai.RateLimitError): + if isinstance(exc, (openai.RateLimitError, openai.BadRequestError, openai.UnprocessableEntityError)): return False if isinstance(exc, openai.APITimeoutError): return _as_bool(get_settings().config.get("retry_same_model_on_timeout", True), default=True) diff --git a/pr_agent/algo/ai_handlers/openai_ai_handler.py b/pr_agent/algo/ai_handlers/openai_ai_handler.py index e50d0ee60d..85791ea29d 100644 --- a/pr_agent/algo/ai_handlers/openai_ai_handler.py +++ b/pr_agent/algo/ai_handlers/openai_ai_handler.py @@ -40,7 +40,9 @@ def deployment_id(self): return get_settings().get("OPENAI.DEPLOYMENT_ID", None) @retry( - retry=retry_if_exception_type(openai.APIError) & retry_if_not_exception_type(openai.RateLimitError), + retry=retry_if_exception_type(openai.APIError) & retry_if_not_exception_type( + (openai.RateLimitError, openai.BadRequestError, openai.UnprocessableEntityError) + ), stop=stop_after_attempt(OPENAI_RETRIES), ) async def chat_completion(self, model: str, system: str, user: str, temperature: float = 0.2, img_path: str = None): diff --git a/tests/unittest/test_litellm_chat_completion_core.py b/tests/unittest/test_litellm_chat_completion_core.py index c2de525399..0608257899 100644 --- a/tests/unittest/test_litellm_chat_completion_core.py +++ b/tests/unittest/test_litellm_chat_completion_core.py @@ -3,6 +3,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import httpx +import litellm import openai import pytest @@ -605,7 +606,7 @@ async def test_chat_completion_keeps_image_for_custom_reasoning_models(monkeypat # Wiring tests for the retry knobs: the helpers (_should_retry_same_model, # _configured_client_retries) are unit-tested in test_litellm_retry_config.py, but those # tests keep passing when the @retry predicate or the kwargs pass-through in -# chat_completion is reverted. The four tests below drive chat_completion itself, so a +# chat_completion is reverted. The tests below drive chat_completion itself, so a # regression in the wiring — not just the helpers — fails a test. @@ -613,6 +614,54 @@ def _timeout_error(): return openai.APITimeoutError(request=httpx.Request("POST", "http://model.invalid")) +@pytest.mark.asyncio +@pytest.mark.parametrize( + "error, expected_attempts", + [ + pytest.param( + openai.BadRequestError( + "invalid request", + response=httpx.Response(400, request=httpx.Request("POST", "http://model.invalid")), + body=None, + ), + 1, + id="bad-request", + ), + pytest.param( + openai.UnprocessableEntityError( + "invalid request", + response=httpx.Response(422, request=httpx.Request("POST", "http://model.invalid")), + body=None, + ), + 1, + id="unprocessable-entity", + ), + pytest.param( + litellm.ContextWindowExceededError("context too long", model="gpt-4o", llm_provider="openai"), + 1, + id="litellm-context-window", + ), + pytest.param( + openai.APIError("temporary failure", request=httpx.Request("POST", "http://model.invalid"), body=None), + litellm_handler.MODEL_RETRIES, + id="generic-api-error", + ), + ], +) +async def test_chat_completion_retry_classification(monkeypatch, error, expected_attempts): + monkeypatch.setattr(litellm_handler, "get_settings", FakeSettings) + + with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: + mock_call.side_effect = error + handler = litellm_handler.LiteLLMAIHandler() + + with pytest.raises(type(error)) as raised: + await handler.chat_completion(model="gpt-4o", system="sys", user="usr") + + assert raised.value is error + assert mock_call.call_count == expected_attempts + + @pytest.mark.asyncio async def test_chat_completion_passes_configured_retries_to_completion_call(monkeypatch): monkeypatch.setattr(litellm_handler, "get_settings", lambda: FakeSettings(config_values={"num_retries": 0})) diff --git a/tests/unittest/test_litellm_retry_config.py b/tests/unittest/test_litellm_retry_config.py index ea12308df9..070391af1b 100644 --- a/tests/unittest/test_litellm_retry_config.py +++ b/tests/unittest/test_litellm_retry_config.py @@ -8,6 +8,7 @@ """ import httpx +import litellm import openai import pytest @@ -61,6 +62,27 @@ def test_rate_limit_never_retries_same_model(self): def test_other_api_errors_still_retry(self): assert _should_retry_same_model(_api_error()) is True + @pytest.mark.parametrize("error_type,status", [ + (openai.BadRequestError, 400), + (openai.UnprocessableEntityError, 422), + ]) + def test_request_validation_errors_do_not_retry(self, error_type, status): + error = error_type( + "invalid request", + response=httpx.Response(status, request=httpx.Request("POST", "http://model.invalid")), + body=None, + ) + assert _should_retry_same_model(error) is False + + @pytest.mark.parametrize("error_type", [ + litellm.BadRequestError, + litellm.ContextWindowExceededError, + litellm.ContentPolicyViolationError, + ]) + def test_litellm_bad_request_subtypes_do_not_retry(self, error_type): + error = error_type("rejected request", model="gpt-4o", llm_provider="openai") + assert _should_retry_same_model(error) is False + def test_non_api_errors_never_retry(self): assert _should_retry_same_model(ValueError("not an API error")) is False diff --git a/tests/unittest/test_openai_compat_handler_retry.py b/tests/unittest/test_openai_compat_handler_retry.py new file mode 100644 index 0000000000..c4b2d30880 --- /dev/null +++ b/tests/unittest/test_openai_compat_handler_retry.py @@ -0,0 +1,89 @@ +import importlib +import sys +from types import ModuleType, SimpleNamespace +from unittest.mock import AsyncMock + +import httpx +import openai +import pytest +from tenacity import RetryError + +import pr_agent.algo.ai_handlers.openai_ai_handler as openai_handler + + +@pytest.fixture(params=["openai", "langchain"]) +def handler_and_completion(request, monkeypatch): + completion = AsyncMock() + settings = SimpleNamespace(openai=SimpleNamespace(key="test-key"), get=lambda key, default=None: default) + if request.param == "openai": + monkeypatch.setenv("OPENAI_API_KEY", "test-key") + monkeypatch.setattr(openai_handler, "get_settings", lambda: settings) + client = SimpleNamespace(chat=SimpleNamespace(completions=SimpleNamespace(create=completion))) + monkeypatch.setattr(openai_handler, "AsyncOpenAI", lambda: client) + return openai_handler.OpenAIHandler(), completion, openai_handler.OPENAI_RETRIES + + class FakeRunnable: + async def ainvoke(self, **kwargs): + return await completion(**kwargs) + + messages = ModuleType("langchain_core.messages") + messages.HumanMessage = lambda content: SimpleNamespace(content=content) + messages.SystemMessage = lambda content: SimpleNamespace(content=content) + runnables = ModuleType("langchain_core.runnables") + runnables.Runnable = FakeRunnable + clients = ModuleType("langchain_openai") + clients.ChatOpenAI = type("ChatOpenAI", (), {}) + clients.AzureChatOpenAI = type("AzureChatOpenAI", (), {}) + for module in (messages, runnables, clients): + monkeypatch.setitem(sys.modules, module.__name__, module) + + module_name = "pr_agent.algo.ai_handlers.langchain_ai_handler" + # Track both import caches, including missing entries, before loading the fake-backed module. + parent = importlib.import_module("pr_agent.algo.ai_handlers") + monkeypatch.setattr(parent, "langchain_ai_handler", None, raising=False) + monkeypatch.setitem(sys.modules, module_name, None) + monkeypatch.delitem(sys.modules, module_name) + langchain_handler = importlib.import_module(module_name) + monkeypatch.setattr(langchain_handler, "get_settings", lambda: settings) + monkeypatch.setattr( + langchain_handler.LangChainOpenAIHandler, "_create_chat_async", AsyncMock(return_value=FakeRunnable()) + ) + return langchain_handler.LangChainOpenAIHandler(), completion, langchain_handler.OPENAI_RETRIES + + +def _error(error_type): + request = httpx.Request("POST", "http://model.invalid") + if error_type is openai.APITimeoutError: + return error_type(request=request) + if error_type is openai.APIError: + return error_type("temporary failure", request=request, body=None) + status = {openai.BadRequestError: 400, openai.UnprocessableEntityError: 422, openai.RateLimitError: 429}[error_type] + return error_type("rejected request", response=httpx.Response(status, request=request), body=None) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("error_type", [openai.BadRequestError, openai.UnprocessableEntityError, openai.RateLimitError]) +async def test_rejected_requests_surface_without_same_handler_replay(handler_and_completion, error_type): + handler, completion, _ = handler_and_completion + error = _error(error_type) + completion.side_effect = error + + with pytest.raises(error_type) as raised: + await handler.chat_completion(model="gpt-test", system="sys", user="usr") + + assert raised.value is error + assert completion.await_count == 1 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("error_type", [openai.APITimeoutError, openai.APIError]) +async def test_retryable_errors_keep_attempt_count_and_retryerror(handler_and_completion, error_type): + handler, completion, attempts = handler_and_completion + error = _error(error_type) + completion.side_effect = error + + with pytest.raises(RetryError) as raised: + await handler.chat_completion(model="gpt-test", system="sys", user="usr") + + assert raised.value.last_attempt.exception() is error + assert completion.await_count == attempts From 2edda5bb58d74ae1fc6ef1c3776e1dde5e70c746 Mon Sep 17 00:00:00 2001 From: Utsab Dahal <134686066+utsab345@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:15:40 +0545 Subject: [PATCH 05/12] fix: redact GCS secret retrieval failures from logs (#3680) --- .../google_cloud_storage_secret_provider.py | 3 ++- ...e_cloud_storage_secret_provider_logging.py | 24 +++++++++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/pr_agent/secret_providers/google_cloud_storage_secret_provider.py b/pr_agent/secret_providers/google_cloud_storage_secret_provider.py index cef9ad8bc8..e94dfc4c2e 100644 --- a/pr_agent/secret_providers/google_cloud_storage_secret_provider.py +++ b/pr_agent/secret_providers/google_cloud_storage_secret_provider.py @@ -22,7 +22,8 @@ def get_secret(self, secret_name: str) -> str: blob = self.bucket.blob(secret_name) return blob.download_as_string() except Exception as e: - get_logger().warning(f"Failed to get secret {secret_name} from Google Cloud Storage: {e}") + # Omit the secret name because the GitLab webhook passes its token here. + get_logger().warning(f"Failed to get secret from Google Cloud Storage: {type(e).__name__}") return "" def store_secret(self, secret_name: str, secret_value: str): diff --git a/tests/unittest/test_google_cloud_storage_secret_provider_logging.py b/tests/unittest/test_google_cloud_storage_secret_provider_logging.py index 416136e159..8e4db48071 100644 --- a/tests/unittest/test_google_cloud_storage_secret_provider_logging.py +++ b/tests/unittest/test_google_cloud_storage_secret_provider_logging.py @@ -36,14 +36,14 @@ def provider_logs(): get_logger().remove(sink) -def assert_safe_log(messages, expected_message): +def assert_safe_log(messages, expected_message, level="ERROR"): assert len(messages) == 1 message = messages[0] assert ERROR_SECRET not in str(message) assert STORE_SECRET_NAME not in str(message) assert "Traceback (most recent call last)" not in str(message) assert message.record["message"] == expected_message - assert message.record["level"].name == "ERROR" + assert message.record["level"].name == level assert message.record["exception"] is None assert not message.record["extra"] @@ -85,3 +85,23 @@ def test_store_does_not_log_secret_name_or_raw_sdk_error(provider_logs): provider_logs, "Failed to store secret in Google Cloud Storage: RuntimeError", ) + + +def test_get_secret_does_not_log_secret_name_or_raw_sdk_error(provider_logs): + error = RuntimeError(ERROR_SECRET) + blob = MagicMock() + blob.download_as_string.side_effect = error + bucket = MagicMock() + bucket.blob.return_value = blob + + provider = object.__new__(gcs_provider.GoogleCloudStorageSecretProvider) + provider.bucket = bucket + + assert provider.get_secret(STORE_SECRET_NAME) == "" + bucket.blob.assert_called_once_with(STORE_SECRET_NAME) + blob.download_as_string.assert_called_once_with() + assert_safe_log( + provider_logs, + "Failed to get secret from Google Cloud Storage: RuntimeError", + level="WARNING", + ) From 28588393125239b49dfe0e7e8c3a3052ed0460d8 Mon Sep 17 00:00:00 2001 From: Utsab Dahal <134686066+utsab345@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:42:25 +0545 Subject: [PATCH 06/12] feat(ai_handlers): surface prompt cache visibility (#3618) --- .../usage-guide/configuration_reference.md | 2 +- .../algo/ai_handlers/litellm_ai_handler.py | 112 ++++++++++- pr_agent/algo/run_details.py | 55 ++++++ pr_agent/algo/run_output.py | 5 + pr_agent/settings/configuration.toml | 9 +- ..._litellm_cache_control_injection_points.py | 185 ++++++++++++++++++ tests/unittest/test_run_details.py | 90 +++++++++ tests/unittest/test_show_run_details.py | 34 ++++ 8 files changed, 483 insertions(+), 9 deletions(-) diff --git a/docs/docs/usage-guide/configuration_reference.md b/docs/docs/usage-guide/configuration_reference.md index 0e5cdd6779..acbc69c65c 100644 --- a/docs/docs/usage-guide/configuration_reference.md +++ b/docs/docs/usage-guide/configuration_reference.md @@ -429,7 +429,7 @@ _This section only documents commented-out examples; see the [TOML source](https | `force_streaming_custom_llm_provider` | "" | Force streaming when the request matches this provider AND its api_base contains one of the substrings below. Some OpenAI-compatible endpoints return a response that LiteLLM cannot normalize in non-streaming mode. Both must be set for the workaround to apply. | | `force_streaming_api_base_substrings` | [] | | | `callback_timeout_seconds` | 30 | max seconds to wait for pending litellm callbacks to flush before exiting | -| `cache_control_injection_points` | [] | Optional: enable Anthropic prompt caching via LiteLLM, e.g. [{location = "message", role = "system"}] (https://docs.litellm.ai/docs/tutorials/prompt_caching) | +| `cache_control_injection_points` | [] | Optional: enable Anthropic prompt caching via LiteLLM, e.g. [{location = "message", role = "system"}] (https://docs.litellm.ai/docs/tutorials/prompt_caching). PR-Agent forwards these points only for models whose name contains "claude"; LiteLLM adds the cache_control blocks. LiteLLM's own default injection (`litellm.enable_anthropic_prompt_caching`, env `LITELLM_ENABLE_ANTHROPIC_PROMPT_CACHING`, off by default) applies only when no points are configured here, so the two never double-inject. A warning is logged once per process when the points cannot take effect (non-Anthropic model, no prompt-cache support, or a prefix below the model's minimum). | ## `[openrouter]` diff --git a/pr_agent/algo/ai_handlers/litellm_ai_handler.py b/pr_agent/algo/ai_handlers/litellm_ai_handler.py index 527f23cc26..7747984a38 100644 --- a/pr_agent/algo/ai_handlers/litellm_ai_handler.py +++ b/pr_agent/algo/ai_handlers/litellm_ai_handler.py @@ -141,6 +141,17 @@ _IMAGE_HEAD_TIMEOUT_SECONDS = 5 OPENAI_DEFAULT_API_BASE = "https://api.openai.com/v1" +# Token-count allowances used when estimating the cached prompt prefix for the +# cache_control_injection_points pre-call warning. Mirrors pr_help_message.py. +_CACHE_MESSAGE_FRAMING_ALLOWANCE = 16 +_CACHE_REPLY_FRAMING_ALLOWANCE = 16 +# Providers that serve Anthropic Claude models and honor cache_control injection. +_ANTHROPIC_CACHE_REQUEST_PROVIDERS = ("anthropic", "bedrock", "bedrock_mantle", "vertex_ai") +# One-time warnings telling the operator when an enabled prompt-cache config cannot take +# effect, keyed by (model, reason) so the same warning is logged once per process. See +# _warn_prompt_cache_conditions. +_ANTHROPIC_CACHE_WARNING_LOG: set[tuple[str, str]] = set() + PROVIDER_SETTING_PATHS = { "anthropic": {"api_key": "ANTHROPIC.KEY"}, "codestral": {"api_key": "CODESTRAL.KEY"}, @@ -267,6 +278,17 @@ def _should_retry_same_model(exc: BaseException) -> bool: return isinstance(exc, openai.APIError) +def _log_anthropic_cache_warning(model: str, reason: str) -> None: + """Log one warning per process for a prompt-cache config that cannot take effect.""" + key = (model, reason) + if key in _ANTHROPIC_CACHE_WARNING_LOG: + return + _ANTHROPIC_CACHE_WARNING_LOG.add(key) + get_logger().warning( + f"cache_control_injection_points may not take effect for {model}: {reason}" + ) + + class LiteLLMAIHandler(BaseAiHandler): """Handle chat completions across supported providers through LiteLLM. @@ -2407,6 +2429,80 @@ def _resolve_cache_control_injection_points(): raise ValueError("LITELLM.CACHE_CONTROL_INJECTION_POINTS must be a JSON/TOML array") return cache_control_injection_points + @staticmethod + def _warn_prompt_cache_conditions( + model: str, system: str, user: str, injection_points, request_provider: str | None = None + ) -> None: + """Warn once per process when an enabled prompt-cache config cannot take effect. + + LiteLLM skips Anthropic prompt caching silently when the model does not support it or + the cached prefix stays below the model's ``prompt_cache_min_tokens``. Both conditions + are knowable before the call, so surface them instead of leaving the operator blind. + Best effort: a metadata gap or estimate failure skips the check, never fails the call. + """ + if not isinstance(model, str) or not model: + return + is_claude_named = "claude" in model.lower() + is_anthropic_provider = request_provider in _ANTHROPIC_CACHE_REQUEST_PROVIDERS + if not is_claude_named and not is_anthropic_provider: + # cache_control_injection_points is an Anthropic-only kwarg; a config pointing at + # another provider (or a model identifier that cannot resolve as Anthropic) will + # never attach, so warn instead of silently dropping it in a debug line. + _log_anthropic_cache_warning( + model, "the request does not route to an Anthropic Claude model" + ) + return + try: + supports = litellm.utils.supports_prompt_caching(model) + except Exception: + return + if supports is False and is_claude_named: + # Conclusive only for a model identifier we recognize; a provider-aliased model + # (e.g. anthropic/my-deployment) may simply be absent from litellm's cost map. + _log_anthropic_cache_warning(model, "the model does not support prompt caching") + return + try: + min_tokens = litellm.get_model_info(model).get("prompt_cache_min_tokens") + except Exception: + return + if not isinstance(min_tokens, int) or isinstance(min_tokens, bool) or min_tokens <= 0: + return + cached_tokens = LiteLLMAIHandler._estimate_cached_prefix_tokens(system, user, injection_points) + if 0 < cached_tokens < min_tokens: + _log_anthropic_cache_warning( + model, + f"the cached prefix is below the model's {min_tokens} token minimum", + ) + + @staticmethod + def _estimate_cached_prefix_tokens(system: str, user: str, injection_points) -> int: + """Estimate the tokens in the prompt segment the injection points will cache. + + A cache_control breakpoint caches everything from the start of the prompt up to the + targeted message, so the estimate counts the targeted messages plus every message + before them (system, then user in this handler's call shape). Returns 0 when none of + the points targets a supported role or the estimate cannot be produced, which skips + the below-minimum check entirely. + """ + targets_user = any( + isinstance(point, dict) and point.get("role") == "user" for point in injection_points + ) + if not any(isinstance(point, dict) and point.get("role") in ("system", "user") + for point in injection_points): + return 0 + try: + from pr_agent.algo.token_handler import TokenEncoder + + encoder = TokenEncoder.get_token_encoder("anthropic/claude") + system_tokens = len(encoder.encode(system or "", disallowed_special=())) + user_tokens = len(encoder.encode(user or "", disallowed_special=())) + except Exception: + system_tokens = len(system or "") // 4 + user_tokens = len(user or "") // 4 + cached_tokens = system_tokens + (user_tokens if targets_user else 0) + cached_framing = _CACHE_MESSAGE_FRAMING_ALLOWANCE * (2 if targets_user else 1) + _CACHE_REPLY_FRAMING_ALLOWANCE + return cached_tokens + cached_framing + async def chat_completion(self, model: str, system: str, user: str, temperature: float = 0.2, img_path: str = None): configured_deployment_id = self.deployment_id return await self._chat_completion_with_retry( @@ -2763,16 +2859,18 @@ async def _chat_completion_with_retry( # Anthropic prompt caching via LiteLLM's cache_control_injection_points. The value # is validated before the try/except (see above) so a malformed config surfaces as # a ValueError instead of being retried. The kwarg is Anthropic-specific (Claude via - # the Anthropic API, Bedrock or Vertex), so gate on the model to avoid passing an - # unsupported param to other providers when litellm.drop_params is off. setdefault - # guards against overwriting a value already merged into kwargs. + # the Anthropic API, Bedrock or Vertex), so gate the forwarding on the model to + # avoid passing an unsupported param to other providers when litellm.drop_params is + # off. The pre-call warning runs for every configured model instead, so an operator + # who misconfigures an aliased or non-Anthropic model gets a signal rather than a + # silently skipped debug line. setdefault guards against overwriting a value already + # merged into kwargs. if cache_control_injection_points: if isinstance(model, str) and "claude" in model.lower(): kwargs.setdefault("cache_control_injection_points", cache_control_injection_points) - else: - get_logger().debug( - f"cache_control_injection_points configured but not applied: {model} is not an " - "Anthropic (Claude) model") + self._warn_prompt_cache_conditions( + model, system, user, cache_control_injection_points, request_provider=request_provider + ) # Classic `bedrock/` calls use model_id for Bedrock Runtime inference profiles. # Bedrock Mantle uses Projects, so `bedrock_mantle/` intentionally omits it. diff --git a/pr_agent/algo/run_details.py b/pr_agent/algo/run_details.py index 7324fe53ac..d0f53fd301 100644 --- a/pr_agent/algo/run_details.py +++ b/pr_agent/algo/run_details.py @@ -45,6 +45,12 @@ class RunDetails: # Counts failed fallback attempts as well, so it reflects what the run really cost, # while `model_used` names only the model behind the final answer. total_tokens: int = 0 + # Provider-reported prompt-cache activity summed over every AI call of the run. + # Named after litellm's normalized usage fields (Anthropic cache_read_input_tokens / + # cache_creation_input_tokens; DeepSeek prompt_cache_hit_tokens maps onto read). + # Both stay 0 when the provider does not report cache usage. + cache_read_tokens: int = 0 + cache_creation_tokens: int = 0 # Successful LLM invocations, counted even when their token usage is unavailable. num_ai_calls: int = 0 # Accumulate costs only when cost output is enabled and LiteLLM can synchronously @@ -74,6 +80,10 @@ def has_token_usage(self) -> bool: or self.completion_tokens > 0 ) + @property + def has_cache_usage(self) -> bool: + return self.cache_read_tokens > 0 or self.cache_creation_tokens > 0 + @property def cost_status(self) -> str: """Return whether every, some, or none of the successful calls were priced.""" @@ -129,6 +139,45 @@ def _read_token_field(usage, name: str) -> int: return value if isinstance(value, int) else 0 +def _read_cache_token_field(usage, public_name: str, private_name: str) -> int: + """Read a cache-token field from a litellm usage object or dict. + + litellm's ``Usage`` keeps these under private attributes (``_cache_read_input_tokens``, + ``_cache_creation_input_tokens``), while a raw dict carries the public key. + ``prompt_tokens_details.cached_tokens`` is litellm's normalized alias for cache reads, and + DeepSeek reports cache hits as ``prompt_cache_hit_tokens``. The details sub-object may be + None in the raw payload, so the lookup must never assume it is a mapping. + """ + read_aliases = ("prompt_tokens_details", "prompt_cache_hit_tokens") + if isinstance(usage, dict): + value = usage.get(public_name) + if value is None and public_name == "cache_read_input_tokens": + for alias in read_aliases: + details = usage.get(alias) + if isinstance(details, dict): + value = details.get("cached_tokens") + elif isinstance(details, int) and not isinstance(details, bool): + value = details + if value is not None: + break + else: + value = getattr(usage, public_name, None) + if value is None: + value = getattr(usage, private_name, None) + if value is None and public_name == "cache_read_input_tokens": + for alias in read_aliases: + details = getattr(usage, alias, None) + if isinstance(details, dict): + value = details.get("cached_tokens") + elif isinstance(details, int) and not isinstance(details, bool): + value = details + elif details is not None: + value = getattr(details, "cached_tokens", None) + if value is not None: + break + return value if isinstance(value, int) and not isinstance(value, bool) else 0 + + def add_token_usage(usage) -> None: """Accumulate token counts from a litellm usage object or dict.""" details = get_run_details() @@ -142,6 +191,12 @@ def add_token_usage(usage) -> None: details.prompt_tokens += prompt_tokens details.completion_tokens += completion_tokens details.total_tokens += total_tokens + details.cache_read_tokens += _read_cache_token_field( + usage, "cache_read_input_tokens", "_cache_read_input_tokens" + ) + details.cache_creation_tokens += _read_cache_token_field( + usage, "cache_creation_input_tokens", "_cache_creation_input_tokens" + ) def _as_decimal_cost(cost_usd) -> Optional[Decimal]: diff --git a/pr_agent/algo/run_output.py b/pr_agent/algo/run_output.py index c7feab1401..7aa66e9d76 100644 --- a/pr_agent/algo/run_output.py +++ b/pr_agent/algo/run_output.py @@ -203,6 +203,11 @@ def show_run_details(gfm_supported: bool) -> str: (details.total_tokens, "total")] reported = [f"{value:,} {label}" for value, label in counts if value] lines.append(f"- Tokens: {' / '.join(reported)}") + if details.has_cache_usage: + # Prompt-cache activity proves whether the cache config is earning anything. + cache_counts = [(details.cache_read_tokens, "read"), (details.cache_creation_tokens, "written")] + cache_reported = [f"{value:,} {label}" for value, label in cache_counts if value] + lines.append(f"- Prompt cache: {' / '.join(cache_reported)}") lines.append(f"- Time cost: {details.duration_seconds:.1f}s") if details.num_ai_calls: lines.append(f"- AI calls: {details.num_ai_calls}") diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 0f06d48100..3912e3f3aa 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -456,7 +456,14 @@ callback_timeout_seconds = 30 # max seconds to wait for pending litellm callback # Application inference profile ARN for classic `bedrock/` calls through the `bedrock-runtime` APIs. # Not used by `bedrock_mantle/`, which uses Projects for cost allocation. # model_id = "" -cache_control_injection_points = [] # Optional: enable Anthropic prompt caching via LiteLLM, e.g. [{location = "message", role = "system"}] (https://docs.litellm.ai/docs/tutorials/prompt_caching) +# Optional: enable Anthropic prompt caching via LiteLLM, e.g. [{location = "message", role = "system"}] +# (https://docs.litellm.ai/docs/tutorials/prompt_caching). PR-Agent forwards these points only for models +# whose name contains "claude"; LiteLLM adds the cache_control blocks. LiteLLM's own default injection +# (litellm.enable_anthropic_prompt_caching, env LITELLM_ENABLE_ANTHROPIC_PROMPT_CACHING, off by default) +# applies only when no points are configured here, so the two never double-inject. A warning is logged +# once per process when the points cannot take effect (non-Anthropic model, no prompt-cache support, or +# a prefix below the model's minimum). +cache_control_injection_points = [] [openrouter] # OpenRouter provider routing, reasoning control and output cap. These apply only diff --git a/tests/unittest/test_litellm_cache_control_injection_points.py b/tests/unittest/test_litellm_cache_control_injection_points.py index 78ddc9ae76..6c8d8d3c1e 100644 --- a/tests/unittest/test_litellm_cache_control_injection_points.py +++ b/tests/unittest/test_litellm_cache_control_injection_points.py @@ -141,3 +141,188 @@ async def test_not_injected_for_non_anthropic_model(monkeypatch): await handler.chat_completion(model="gpt-4o", system="sys", user="usr") assert "cache_control_injection_points" not in mock_call.call_args.kwargs + + +def _warn_settings(points=None): + return lambda: FakeSettings( + settings_values={ + "LITELLM.CACHE_CONTROL_INJECTION_POINTS": points or [{"location": "message", "role": "system"}] + } + ) + + +@pytest.mark.asyncio +async def test_warns_once_when_model_does_not_support_prompt_caching(monkeypatch): + # The call must still go through (caching is best effort), but the operator needs one + # audible warning per (model, reason) that their config cannot take effect. + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + monkeypatch.setattr(litellm_handler, "get_settings", _warn_settings()) + monkeypatch.setattr(litellm_handler.litellm.utils, "supports_prompt_caching", lambda model: False) + + with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: + mock_call.return_value = _mock_response() + handler = litellm_handler.LiteLLMAIHandler() + await handler.chat_completion(model="claude-sonnet-5", system="sys", user="usr") + await handler.chat_completion(model="claude-sonnet-5", system="sys", user="usr") + + assert mock_call.call_count == 2 + warning_texts = [call.args[0] for call in mock_logger.warning.call_args_list] + assert len([text for text in warning_texts if "does not support prompt caching" in text]) == 1 + assert "claude-sonnet-5" in warning_texts[0] + + +@pytest.mark.asyncio +async def test_warns_once_for_non_anthropic_model_even_though_forwarding_is_gated(monkeypatch): + # A non-Claude model never gets the kwarg forwarded, but it must no longer be silently + # dropped in a debug line: the operator gets one warning naming the model and the reason. + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + monkeypatch.setattr(litellm_handler, "get_settings", _warn_settings()) + + with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: + mock_call.return_value = _mock_response() + handler = litellm_handler.LiteLLMAIHandler() + await handler.chat_completion(model="gpt-4o", system="sys", user="usr") + await handler.chat_completion(model="gpt-4o", system="sys", user="usr") + + assert mock_call.call_count == 2 + assert "cache_control_injection_points" not in mock_call.call_args.kwargs + warning_texts = [call.args[0] for call in mock_logger.warning.call_args_list] + assert len([text for text in warning_texts if "does not route to an Anthropic Claude model" in text]) == 1 + + +def test_anthropic_routed_alias_without_metadata_is_silent(monkeypatch): + # A provider-aliased Claude deployment (e.g. anthropic/my-deployment) may be absent from + # litellm's cost map: best-effort metadata lookups must not emit a false warning. + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + monkeypatch.setattr( + litellm_handler.litellm.utils, "supports_prompt_caching", MagicMock(side_effect=RuntimeError("no model")) + ) + + handler = litellm_handler.LiteLLMAIHandler() + handler._warn_prompt_cache_conditions( + "my-claude-gateway", "sys", "usr", [{"location": "message", "role": "system"}], request_provider="anthropic" + ) + + assert mock_logger.warning.call_count == 0 + + +def test_openrouter_claude_route_skips_route_warning(monkeypatch): + # LiteLLM writes cache_control into the OpenRouter payload for Claude models too, + # so this route must not get the non-Anthropic warning. + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + + handler = litellm_handler.LiteLLMAIHandler() + handler._warn_prompt_cache_conditions( + "openrouter/anthropic/claude-3.5-sonnet", + "sys", + "usr", + [{"location": "message", "role": "system"}], + request_provider="openrouter", + ) + + warning_texts = [call.args[0] for call in mock_logger.warning.call_args_list] + assert not [text for text in warning_texts if "does not route to an Anthropic Claude model" in text] + + +@pytest.mark.asyncio +async def test_warns_when_cached_prefix_below_model_minimum(monkeypatch): + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + monkeypatch.setattr(litellm_handler, "get_settings", _warn_settings()) + monkeypatch.setattr(litellm_handler.litellm.utils, "supports_prompt_caching", lambda model: True) + monkeypatch.setattr(litellm_handler.litellm, "get_model_info", lambda model: {"prompt_cache_min_tokens": 4096}) + monkeypatch.setattr( + litellm_handler.LiteLLMAIHandler, + "_estimate_cached_prefix_tokens", + staticmethod(lambda system, user, points: 120), + ) + + with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: + mock_call.return_value = _mock_response() + handler = litellm_handler.LiteLLMAIHandler() + await handler.chat_completion(model="claude-sonnet-5", system="sys", user="usr") + + assert mock_call.call_count == 1 + warning_texts = [call.args[0] for call in mock_logger.warning.call_args_list] + assert len([text for text in warning_texts if "below the model's 4096 token minimum" in text]) == 1 + + +@pytest.mark.asyncio +async def test_no_warning_when_support_and_prefix_match(monkeypatch): + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + monkeypatch.setattr(litellm_handler, "get_settings", _warn_settings()) + monkeypatch.setattr(litellm_handler.litellm.utils, "supports_prompt_caching", lambda model: True) + monkeypatch.setattr(litellm_handler.litellm, "get_model_info", lambda model: {"prompt_cache_min_tokens": 1024}) + monkeypatch.setattr( + litellm_handler.LiteLLMAIHandler, + "_estimate_cached_prefix_tokens", + staticmethod(lambda system, user, points: 2000), + ) + + with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: + mock_call.return_value = _mock_response() + handler = litellm_handler.LiteLLMAIHandler() + await handler.chat_completion(model="claude-sonnet-5", system="sys", user="usr") + + assert mock_logger.warning.call_count == 0 + + +@pytest.mark.asyncio +async def test_no_warning_without_cache_metadata_but_call_proceeds(monkeypatch): + # A metadata lookup failure must be silent, not an error: the warning is best effort. + mock_logger = MagicMock() + monkeypatch.setattr(litellm_handler, "get_logger", lambda: mock_logger) + monkeypatch.setattr(litellm_handler, "_ANTHROPIC_CACHE_WARNING_LOG", set()) + monkeypatch.setattr(litellm_handler, "get_settings", _warn_settings()) + monkeypatch.setattr( + litellm_handler.litellm.utils, "supports_prompt_caching", MagicMock(side_effect=RuntimeError("no model")) + ) + + with patch("pr_agent.algo.ai_handlers.litellm_ai_handler.acompletion", new_callable=AsyncMock) as mock_call: + mock_call.return_value = _mock_response() + handler = litellm_handler.LiteLLMAIHandler() + await handler.chat_completion(model="claude-sonnet-5", system="sys", user="usr") + + assert mock_call.call_count == 1 + assert mock_logger.warning.call_count == 0 + + +def test_estimate_counts_prompt_prefix_up_to_targeted_message(monkeypatch): + class _FakeEncoder: + @staticmethod + def encode(text, **kwargs): + return [1] * ((len(text) if text else 0) // 2 + 1) + + monkeypatch.setattr( + "pr_agent.algo.token_handler.TokenEncoder.get_token_encoder", + staticmethod(lambda model: _FakeEncoder()), + ) + + handler = litellm_handler.LiteLLMAIHandler() + system_only = handler._estimate_cached_prefix_tokens( + "pineapple", "banana", [{"location": "message", "role": "system"}] + ) + user_only = handler._estimate_cached_prefix_tokens("pineapple", "banana", [{"location": "message", "role": "user"}]) + + # Caching the user message also caches the system message that precedes it, so its + # estimate must be strictly larger than the system-only one. + assert system_only == len("pineapple") // 2 + 1 + 32 + assert user_only == len("pineapple") // 2 + 1 + len("banana") // 2 + 1 + 48 + assert user_only > system_only + + +def test_estimate_is_zero_without_targeted_role(monkeypatch): + handler = litellm_handler.LiteLLMAIHandler() + assert handler._estimate_cached_prefix_tokens("pineapple", "banana", [{"location": "message", "role": "none"}]) == 0 + assert handler._estimate_cached_prefix_tokens("pineapple", "banana", []) == 0 diff --git a/tests/unittest/test_run_details.py b/tests/unittest/test_run_details.py index 6022948367..7e5343fbcd 100644 --- a/tests/unittest/test_run_details.py +++ b/tests/unittest/test_run_details.py @@ -37,6 +37,7 @@ def test_init_returns_fresh_instance_with_zeroed_counters(): assert details.model_costs_usd == {} assert details.cost_status == "unavailable" assert details.has_token_usage is False + assert details.has_cache_usage is False assert details.duration_seconds >= 0 @@ -108,6 +109,95 @@ def test_add_token_usage_ignores_none_and_partial_objects(): assert details.has_token_usage is False +def test_add_token_usage_accumulates_cache_tokens_from_dict(): + init_run_details() + + add_token_usage({ + "prompt_tokens": 100, + "completion_tokens": 10, + "cache_read_input_tokens": 40, + "cache_creation_input_tokens": 60, + }) + + details = get_run_details() + assert details.cache_read_tokens == 40 + assert details.cache_creation_tokens == 60 + assert details.has_cache_usage is True + + +def test_add_token_usage_reads_cache_tokens_from_usage_private_attrs(): + class _CachedUsage: + def __init__(self): + self.prompt_tokens = 100 + self.completion_tokens = 10 + self.total_tokens = 110 + self._cache_read_input_tokens = 40 + self._cache_creation_input_tokens = 60 + + init_run_details() + add_token_usage(_CachedUsage()) + + details = get_run_details() + assert details.cache_read_tokens == 40 + assert details.cache_creation_tokens == 60 + assert details.has_cache_usage is True + + +def test_add_token_usage_reads_cache_read_via_prompt_tokens_details(): + class _CachedUsage: + prompt_tokens = 100 + completion_tokens = 10 + total_tokens = 110 + prompt_tokens_details = type("Details", (), {"cached_tokens": 40})() + + init_run_details() + add_token_usage(_CachedUsage()) + + assert get_run_details().cache_read_tokens == 40 + assert get_run_details().cache_creation_tokens == 0 + + +def test_add_token_usage_ignores_bool_like_cache_values(): + init_run_details() + + add_token_usage({"cache_read_input_tokens": True, "cache_creation_input_tokens": 0}) + + details = get_run_details() + assert details.has_cache_usage is False + + +def test_add_token_usage_tolerates_null_prompt_tokens_details(): + # Raw OpenAI payloads can carry prompt_tokens_details: null; the cache lookup must not + # chain attribute access off it after a completion already succeeded. + init_run_details() + + add_token_usage({ + "prompt_tokens": 10, + "completion_tokens": 2, + "prompt_tokens_details": None, + }) + + assert get_run_details().cache_read_tokens == 0 + + +def test_add_token_usage_reads_deepseek_cache_hits_as_cache_reads(): + init_run_details() + + add_token_usage({"prompt_tokens": 7, "prompt_cache_hit_tokens": 3}) + + assert get_run_details().cache_read_tokens == 3 + + class _DeepSeekUsage: + prompt_tokens = 7 + completion_tokens = 1 + prompt_cache_hit_tokens = 5 + + init_run_details() + add_token_usage(_DeepSeekUsage()) + + assert get_run_details().cache_read_tokens == 5 + + def test_record_ai_call_counts_calls_even_without_usage(): init_run_details() diff --git a/tests/unittest/test_show_run_details.py b/tests/unittest/test_show_run_details.py index 2f901eecb7..46282356b8 100644 --- a/tests/unittest/test_show_run_details.py +++ b/tests/unittest/test_show_run_details.py @@ -99,6 +99,40 @@ def test_omits_ai_calls_line_when_no_calls_were_recorded(): assert "AI calls:" not in output +def test_renders_prompt_cache_line_when_cache_usage_reported(): + init_run_details() + record_model_used("anthropic/claude-sonnet-5", is_fallback=False) + record_ai_call( + {"prompt_tokens": 1234, "cache_read_input_tokens": 900, "cache_creation_input_tokens": 200} + ) + + output = show_run_details(gfm_supported=True) + + assert "Prompt cache: 900 read / 200 written" in output + + +def test_omits_cache_line_when_only_read_or_creation_reported(): + init_run_details() + record_model_used("anthropic/claude-sonnet-5", is_fallback=False) + record_ai_call({"prompt_tokens": 10, "cache_read_input_tokens": 7}) + + output = show_run_details(gfm_supported=True) + + assert "Prompt cache: 7 read" in output + assert "written" not in output + + +def test_omits_prompt_cache_line_when_no_cache_usage(): + init_run_details() + record_model_used("anthropic/claude-sonnet-5", is_fallback=False) + record_ai_call({"prompt_tokens": 10, "completion_tokens": 2}) + + output = show_run_details(gfm_supported=True) + + assert "Tokens: 10 in / 2 out / 12 total" in output + assert "Prompt cache:" not in output + + def test_returns_empty_string_when_no_model_was_recorded(): init_run_details() From e6282c9e66a0b70632ce942b5b5e74fb037787dc Mon Sep 17 00:00:00 2001 From: Utsab Dahal <134686066+utsab345@users.noreply.github.com> Date: Fri, 25 Sep 2026 14:42:38 +0545 Subject: [PATCH 07/12] fix(codecommit): keep publishing remaining suggestions after one failure (#3681) --- pr_agent/git_providers/codecommit_provider.py | 3 +- tests/unittest/test_codecommit_provider.py | 44 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/pr_agent/git_providers/codecommit_provider.py b/pr_agent/git_providers/codecommit_provider.py index cb80555076..0d402e0807 100644 --- a/pr_agent/git_providers/codecommit_provider.py +++ b/pr_agent/git_providers/codecommit_provider.py @@ -339,7 +339,8 @@ def publish_code_suggestions(self, code_suggestions: list) -> bool: ) published_count += 1 except Exception as e: - raise ValueError(f"CodeCommit Cannot publish code suggestions for PR: {self.pr_num}") from e + get_logger().warning( + f"Could not publish code suggestion #{counter} for PR {self.pr_num}: {e}") counter += 1 diff --git a/tests/unittest/test_codecommit_provider.py b/tests/unittest/test_codecommit_provider.py index bc46e20a3f..f3eb1c5709 100644 --- a/tests/unittest/test_codecommit_provider.py +++ b/tests/unittest/test_codecommit_provider.py @@ -1109,6 +1109,50 @@ def test_publish_code_suggestion_uses_target_that_contains_file(self): annotation_line=4, ) + def test_publish_code_suggestions_continues_after_one_failure(self): + provider = object.__new__(CodeCommitProvider) + provider.repo_name = "source-repository" + provider.pr_num = 321 + provider.codecommit_client = MagicMock() + provider.codecommit_client.publish_comment.side_effect = [ + None, RuntimeError("network down"), None + ] + provider._get_target_contexts_for_file = MagicMock(return_value=[{ + "repository_name": "source-repository", + "destination_commit": "destination-commit-1", + "source_commit": "source-commit-1", + }]) + + result = provider.publish_code_suggestions([ + {"body": "Use a constant", "relevant_file": "one.py", "relevant_lines_start": 1}, + {"body": "Use a helper", "relevant_file": "two.py", "relevant_lines_start": 2}, + {"body": "Use a factory", "relevant_file": "three.py", "relevant_lines_start": 3}, + ]) + + # A partial failure must not abort the later suggestions or report + # failure, or the caller would republish the already-posted ones. + assert result is True + assert provider.codecommit_client.publish_comment.call_count == 3 + + def test_publish_code_suggestions_reports_total_failure(self): + provider = object.__new__(CodeCommitProvider) + provider.repo_name = "source-repository" + provider.pr_num = 321 + provider.codecommit_client = MagicMock() + provider.codecommit_client.publish_comment.side_effect = RuntimeError("network down") + provider._get_target_contexts_for_file = MagicMock(return_value=[{ + "repository_name": "source-repository", + "destination_commit": "destination-commit-1", + "source_commit": "source-commit-1", + }]) + + result = provider.publish_code_suggestions([ + {"body": "Use a constant", "relevant_file": "one.py", "relevant_lines_start": 1}, + ]) + + assert result is False + assert provider.codecommit_client.publish_comment.called + def test_get_title(self): # Test that the get_title() function returns the PR title with patch.object(CodeCommitProvider, "__init__", lambda x, y: None): From 179110c4fc69afbe501e9928fe94727aa7cdda0b Mon Sep 17 00:00:00 2001 From: Utsab Dahal <134686066+utsab345@users.noreply.github.com> Date: Fri, 25 Sep 2026 16:11:12 +0545 Subject: [PATCH 08/12] feat(gitlab): support inline key issues (#3650) --- docs/docs/tools/review.md | 2 +- .../usage-guide/configuration_reference.md | 2 +- pr_agent/git_providers/gitlab_provider.py | 50 +++++ pr_agent/settings/configuration.toml | 2 +- tests/unittest/test_inline_comment_dedup.py | 200 +++++++++++++++++- tests/unittest/test_pr_reviewer_core.py | 13 ++ 6 files changed, 265 insertions(+), 4 deletions(-) diff --git a/docs/docs/tools/review.md b/docs/docs/tools/review.md index 8cde64797e..15898ae7a9 100644 --- a/docs/docs/tools/review.md +++ b/docs/docs/tools/review.md @@ -116,7 +116,7 @@ for the authoritative default values. inline_key_issues - If set to true, each key issue is published as an inline comment where the provider supports verified inline-comment publication. A finding leaves the review summary when a matching comment exists or the provider accepts the new comment. Findings that cannot be anchored or published stay in the summary. + If set to true, each key issue is published as an inline comment where the provider supports verified inline-comment publication (GitHub, Bitbucket Cloud, Azure DevOps, GitLab). A finding leaves the review summary when a matching comment exists or the provider accepts the new comment. Findings that cannot be anchored or published stay in the summary. diff --git a/docs/docs/usage-guide/configuration_reference.md b/docs/docs/usage-guide/configuration_reference.md index acbc69c65c..89d13b4525 100644 --- a/docs/docs/usage-guide/configuration_reference.md +++ b/docs/docs/usage-guide/configuration_reference.md @@ -159,7 +159,7 @@ to-do list. | `persistent_comment` | true | | | `review_heading` | "PR Reviewer Guide" | Visible base heading for full and incremental review comments. Identity is tracked separately. | | `persistent_finding_state` | true | Persist review finding state across complete review runs. | -| `inline_key_issues` | false | | +| `inline_key_issues` | false | Publish each review finding as an inline comment where the provider can verify inline-comment publication (GitHub, Bitbucket Cloud, Azure DevOps, GitLab). | | `extra_instructions` | "" | | | `num_max_findings` | 3 | | | `final_update_message` | true | | diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index ac58494832..76c0abd2d1 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -263,6 +263,7 @@ def __init__(self, merge_request_url: Optional[str] = None, incremental: Optiona self.diff_files = None self.git_files = None self.temp_comments = [] + self._published_inline_comment_bodies: list[str] = [] self._submodule_cache: dict[tuple[str, str, str], list[dict]] = {} self.pr_url = merge_request_url self._set_merge_request(merge_request_url) @@ -1373,6 +1374,47 @@ def create_inline_comment(self, body: str, relevant_file: str, relevant_line_in_ absolute_position: int = None): raise NotImplementedError("GitLab provider does not support creating inline comments yet") + def get_recent_inline_comment_bodies(self) -> list[str]: + """Return inline comment bodies published during this provider run.""" + return list(getattr(self, "_published_inline_comment_bodies", [])) + + def get_persistent_comment_bodies(self) -> list[str]: + """Return existing GitLab MR note bodies for inline deduplication.""" + bodies = list(getattr(self, "_published_inline_comment_bodies", [])) + seen = set(bodies) + if self.mr is None: + return bodies + for discussion in self.mr.discussions.list(get_all=True): + attrs = getattr(discussion, "attributes", None) or {} + for note in attrs.get("notes", []) or []: + if isinstance(note, dict): + body = note.get("body", "") or "" + if body and body not in seen: + bodies.append(body) + seen.add(body) + for note in self.mr.notes.list(get_all=True): + body = getattr(note, "body", "") or "" + if body and body not in seen: + bodies.append(body) + seen.add(body) + try: + for draft in self.mr.draft_notes.list(get_all=True): + body = getattr(draft, "note", "") or "" + if body and body not in seen: + bodies.append(body) + seen.add(body) + except (GitlabError, RequestException) as e: + get_logger().warning(f"Could not list pending draft notes for MR {self.id_mr}: {e}") + return bodies + + def _remember_published_inline_comment_body(self, body: str) -> None: + recent = getattr(self, "_published_inline_comment_bodies", None) + if recent is None: + recent = [] + self._published_inline_comment_bodies = recent + if body and body not in recent: + recent.append(body) + def send_inline_comment(self, body: str, edit_type: str, found: bool, relevant_file: str, relevant_line_in_file: str, source_line_no: int, target_file: str, target_line_no: int, @@ -1441,6 +1483,7 @@ def _create_suggestion_note(self, as_draft: bool, body: str, pos_obj: dict, diff self.mr.draft_notes.create({'note': body, 'position': pos_obj}) else: self.mr.discussions.create({'body': body, 'position': pos_obj}) + self._remember_published_inline_comment_body(body) if store is not None: store.add(body_fp) store.add(code_fp) @@ -1497,6 +1540,7 @@ def _create_suggestion_note(self, as_draft: bool, body: str, pos_obj: dict, diff self.mr.draft_notes.create({'note': body_fallback, 'position': fallback_position}) else: self.mr.notes.create({'body': body_fallback, 'position': fallback_position}) + self._remember_published_inline_comment_body(body_fallback) get_logger().debug(f"Created fallback comment in MR {self.id_mr} with position {pos_obj}") if store is not None: store.add(body_fp) @@ -1625,6 +1669,12 @@ def publish_code_suggestions(self, code_suggestions: list) -> bool: pending = [] if pending: self.mr.draft_notes.bulk_publish() + # Drafts only count as published once bulk_publish succeeds, so a failed + # batch does not mark visible-to-reviewers findings that are still pending. + for draft in pending: + body = getattr(draft, "note", "") or "" + if body: + self._remember_published_inline_comment_body(body) except (GitlabError, RequestException) as e: # Draft notes are only visible to the posting user until published, so a failure here # leaves the suggestions invisible to everyone else. They aren't lost: GitLab keeps diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 3912e3f3aa..60c77eacb3 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -173,7 +173,7 @@ persistent_comment=true review_heading = "PR Reviewer Guide" persistent_finding_state=true # Persist review finding state across complete review runs. -inline_key_issues=false +inline_key_issues=false # Publish each review finding as an inline comment where the provider can verify inline-comment publication (GitHub, Bitbucket Cloud, Azure DevOps, GitLab). extra_instructions = "" num_max_findings = 3 final_update_message = true diff --git a/tests/unittest/test_inline_comment_dedup.py b/tests/unittest/test_inline_comment_dedup.py index 26552965f1..a0f097299a 100644 --- a/tests/unittest/test_inline_comment_dedup.py +++ b/tests/unittest/test_inline_comment_dedup.py @@ -197,7 +197,7 @@ def _azure_provider(existing_threads=None): def test_inline_publication_verification_supports_providers_with_comment_capability(): assert d.can_verify_inline_comment_publication(_azure_provider()) is True assert d.can_verify_inline_comment_publication(_gh_provider([])) is True - assert d.can_verify_inline_comment_publication(_gl_provider([])) is False + assert d.can_verify_inline_comment_publication(_gl_provider([])) is True class FooProvider: pass @@ -380,6 +380,204 @@ def test_gitlab_flag_off_posts_unmarked(): gs.stop() +def test_gitlab_recent_inline_bodies_start_empty(): + p = _gl_provider([]) + + assert p.get_recent_inline_comment_bodies() == [] + + +def test_gitlab_recent_inline_bodies_record_successful_posts(): + p = _gl_provider([]) + gs = patch("pr_agent.git_providers.gitlab_provider.get_settings") + m = gs.start() + m.return_value.get.side_effect = _flag_side_effect(False) + try: + # a repeated in-memory post is recalled only once + _send(p, "inline finding") + _send(p, "inline finding") + finally: + gs.stop() + + assert p.get_recent_inline_comment_bodies() == ["inline finding"] + + +def test_gitlab_recent_inline_bodies_include_marked_posts(): + p = _gl_provider([]) + gs = _flag_on_gitlab() + try: + _send(p, "**Suggestion:** fix it [possible issue, importance: 7]") + finally: + gs.stop() + + bodies = p.get_recent_inline_comment_bodies() + assert len(bodies) == 1 + assert "**Suggestion:** fix it [possible issue, importance: 7]" in bodies[0] + assert ""), + ], discussion_id='open-thread') + human_resolved = _thread([ + _thread_note(resolved=True, resolved_by={'id': 99, 'name': 'Alice'}), + ], discussion_id='human-resolved') + bot_resolved = _thread([ + _thread_note(resolved=True, resolved_by={'id': _BOT_USER_ID, 'name': 'GitLab Bot'}), + {'author': {'id': 99, 'name': 'Alice'}, 'system': False, 'body': 'We will not do this.'}, + ], discussion_id='bot-resolved') + general = _thread([_thread_note(body=_HUMAN_BODY)], discussion_id='general') + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [open_thread, general, human_resolved, bot_resolved] + + discussions = json.loads(gitlab_provider.get_code_suggestion_thread_context()) + + assert discussions == [ + { + "thread_id": "bot-resolved", + "status": "resolved", + "file": "src/app.py", + "start_line": 12, + "end_line": 12, + "suggestion": _AGENT_BODY, + "replies": [{"author": "Alice", "message": "We will not do this."}], + }, + { + "thread_id": "human-resolved", + "status": "resolved", + "file": "src/app.py", + "start_line": 12, + "end_line": 12, + "suggestion": _AGENT_BODY, + "replies": [], + }, + { + "thread_id": "open-thread", + "status": "open", + "file": "src/app.py", + "start_line": 12, + "end_line": 12, + "suggestion": _AGENT_BODY, + "replies": [], + }, + ] + + def test_get_code_suggestion_thread_context_truncates_messages_and_replies(self, gitlab_provider): + gitlab_provider._own_user_id = _BOT_USER_ID + long_message = _AGENT_BODY + "\n" + "x" * 900 + long_reply = "y" * 900 + reply = {'author': {'id': 99, 'name': 'Alice'}, 'system': False, 'body': long_reply} + thread = _thread([_thread_note(body=long_message), reply], discussion_id='d1') + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [thread] + + discussions = json.loads(gitlab_provider.get_code_suggestion_thread_context()) + + assert len(discussions[0]["suggestion"]) == 750 + assert discussions[0]["replies"] == [{"author": "Alice", "message": long_reply[:750]}] + + def test_get_code_suggestion_thread_context_caps_thread_count(self, gitlab_provider): + gitlab_provider._own_user_id = _BOT_USER_ID + threads = [_thread([_thread_note()], discussion_id=f'd{i}') for i in range(60)] + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = threads + + discussions = json.loads(gitlab_provider.get_code_suggestion_thread_context()) + + assert len(discussions) == 50 + + def test_get_code_suggestion_thread_context_enforces_context_char_budget(self, gitlab_provider): + gitlab_provider._own_user_id = _BOT_USER_ID + big_message = "**Suggestion:** " + "x" * 740 + threads = [_thread([_thread_note(body=big_message)], discussion_id=f'd{i}') for i in range(60)] + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = threads + + result = gitlab_provider.get_code_suggestion_thread_context() + + assert len(result) <= _MAX_DISCUSSION_CONTEXT_CHARS + assert len(json.loads(result)) < 60 + + def test_get_code_suggestion_thread_context_empty_without_agent_threads(self, gitlab_provider): + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [_thread([_thread_note(body=_HUMAN_BODY)])] + + assert gitlab_provider.get_code_suggestion_thread_context() == "" + + def test_get_code_suggestion_thread_context_soft_fails(self, gitlab_provider): + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.side_effect = GitlabError("gitlab api error") + + assert gitlab_provider.get_code_suggestion_thread_context() == "" + def test_resolve_comment_thread_ignores_unrelated_discussions(self, gitlab_provider): # An open discussion that does not own our note must be left untouched. other = MagicMock() From e02da50e8ccca1519f2c4e32f0d83e241b04deba Mon Sep 17 00:00:00 2001 From: Utsab Dahal Date: Fri, 25 Sep 2026 22:18:09 +0545 Subject: [PATCH 12/12] refactor(git_providers): share code-suggestion thread context --- docs/docs/installation/azure.md | 4 + .../usage-guide/configuration_reference.md | 1 + .../git_providers/azuredevops_provider.py | 55 ++-- pr_agent/git_providers/git_provider.py | 108 ++++++++ pr_agent/git_providers/gitlab_provider.py | 56 ++-- pr_agent/settings/configuration.toml | 2 + tests/unittest/test_azure_devops_provider.py | 166 +++++++++++- .../test_code_suggestion_thread_context.py | 239 ++++++++++++++++++ tests/unittest/test_config_reference.py | 2 +- tests/unittest/test_gitlab_provider.py | 60 ++++- 10 files changed, 623 insertions(+), 70 deletions(-) create mode 100644 tests/unittest/test_code_suggestion_thread_context.py diff --git a/docs/docs/installation/azure.md b/docs/docs/installation/azure.md index 61a61d93d9..d77e118c54 100644 --- a/docs/docs/installation/azure.md +++ b/docs/docs/installation/azure.md @@ -122,6 +122,10 @@ configure a stable identity (GUID/ID, unique name, or descriptor). A list can be agent_identity = "" ``` +The same identity gates the prior-suggestion thread context that `/improve` reads: a thread is only included when its +opening comment is attributable to that identity, so leave `agent_identity` configured if you rely on the context. +`pr_code_suggestions.max_discussion_context_chars` caps the injected block, and `0` disables it. + For webhook security, create a sporadic username/password pair and configure the webhook username and password on both the server and Azure DevOps webhook. These will be sent as basic Auth data by the webhook with each request: ```toml diff --git a/docs/docs/usage-guide/configuration_reference.md b/docs/docs/usage-guide/configuration_reference.md index 89d13b4525..41cd1fcb93 100644 --- a/docs/docs/usage-guide/configuration_reference.md +++ b/docs/docs/usage-guide/configuration_reference.md @@ -266,6 +266,7 @@ to-do list. | `max_suggestions_per_file` | 0 | Maximum suggestions retained per file after all chunks are merged; 0 disables the cap. Skip unresolvable line locations before applying a positive cap to summarized output; leave inline selection unchanged. | | `max_number_of_calls` | 3 | | | `parallel_calls` | true | | +| `max_discussion_context_chars` | 24000 | Character budget for the prior code-suggestion thread context injected into the /improve prompt; 0 disables it. | | `decouple_hunks` | false | | **self-review checkbox** diff --git a/pr_agent/git_providers/azuredevops_provider.py b/pr_agent/git_providers/azuredevops_provider.py index 63c4eff870..1e920c4658 100644 --- a/pr_agent/git_providers/azuredevops_provider.py +++ b/pr_agent/git_providers/azuredevops_provider.py @@ -2,7 +2,6 @@ import datetime as _dt import difflib -import json import re from collections import Counter from types import SimpleNamespace @@ -36,7 +35,12 @@ ) from ..config_loader import get_settings, get_verbosity_level from ..log import get_logger -from .git_provider import GitProvider, IncrementalPR +from .git_provider import ( + _MAX_DISCUSSION_MESSAGE_CHARS, + _MAX_DISCUSSION_REPLIES, + GitProvider, + IncrementalPR, +) AZURE_DEVOPS_AVAILABLE = True ADO_APP_CLIENT_DEFAULT_ID = "499b84ac-1321-427f-aa17-267ca6975798/.default" @@ -49,10 +53,6 @@ ) _SUGGESTIONS_HEADER_PREFIX = "## PR Code Suggestions" _FALLBACK_SUGGESTIONS_HEADER = "## Unanchored Code Suggestions" -_MAX_DISCUSSION_CONTEXT_CHARS = 24000 -_MAX_DISCUSSION_REPLIES = 10 -_MAX_DISCUSSION_THREADS = 50 -_MAX_DISCUSSION_MESSAGE_CHARS = 750 def _is_not_found_error(error: Exception) -> bool: @@ -1384,53 +1384,56 @@ def _is_agent_comment(cls, content: str) -> bool: return True return comment_matches_any_identity(content.lstrip(), cls._AGENT_COMMENT_IDENTIFIERS) - def get_code_suggestion_thread_context(self) -> str: - discussions = [] + def _iter_code_suggestion_threads(self): + """Yield this PR's Azure DevOps code-suggestion threads, newest first. + + Threads are dropped unless the opener carries a code suggestion and its author is + verifiable as this PR-Agent identity, so a human thread cannot be read back as the + agent's own suggestion. Replies skip progress notes; the shared limits in + `GitProvider.get_code_suggestion_thread_context()` bound what is yielded here. + """ for thread in reversed(self._get_threads()): comments = self._value(thread, "comments") or [] if not comments: continue - root_body = self._value(comments[0], "content") + opener = comments[0] + root_body = self._value(opener, "content") if not isinstance(root_body, str): continue if not _is_code_suggestion_body(root_body): continue + thread_id = self._value(thread, "id") + try: + if not self.is_comment_authored_by_pr_agent(opener): + continue + except RuntimeError as e: + get_logger().debug(f"Skipping Azure DevOps thread {thread_id}: {e}") + continue replies = [] - for comment in comments[1:][-_MAX_DISCUSSION_REPLIES:]: + for comment in comments[1:]: message = self._value(comment, "content") if not isinstance(message, str) or AZURE_AGENT_PROGRESS_MARKER in message: continue - message = message.replace(AZURE_AGENT_RESPONSE_MARKER, "").strip() - if not message: + if not message.strip(): continue author = self._value(comment, "author") author_name = (self._value(author, "display_name", "displayName") or self._value(author, "unique_name", "uniqueName") or "Unknown") - replies.append({ - "author": author_name, - "message": message[:_MAX_DISCUSSION_MESSAGE_CHARS], - }) + replies.append({"author": author_name, "message": message}) context = self._value(thread, "thread_context", "threadContext") path = self._value(context, "file_path", "filePath") start_position = self._value(context, "right_file_start", "rightFileStart") end_position = self._value(context, "right_file_end", "rightFileEnd") or start_position - discussion = { - "thread_id": self._value(thread, "id"), + yield { + "thread_id": thread_id, "status": self._value(thread, "status"), "file": path, "start_line": self._value(start_position, "line"), "end_line": self._value(end_position, "line"), - "suggestion": root_body.split("|\[pr-agent-[^\]\n]+\]:\s*https://github\.com/The-PR-Agent/pr-agent)\s*$", + re.IGNORECASE, +) + # The reaction PR-Agent has always added when it picks a comment command up. Used as the # fallback for `reaction_on_start` so that a deployment whose configuration.toml predates @@ -898,6 +912,100 @@ def get_comment_url(self, comment) -> str: def get_review_thread_comments(self, comment_id: int) -> list[dict]: pass + #### code suggestion thread context #### + def _iter_code_suggestion_threads(self): + """Yield this PR's code-suggestion threads, newest first. + + Each item is a mapping shaped like the entry written into the context block: + `thread_id`, `status`, `file`, `start_line`, `end_line`, `suggestion` and `replies` + (a list of `{"author": ..., "message": ...}`). Providers override this to read + their own thread API; they own the filtering, so they must skip threads that are + not code suggestions and skip threads they cannot attribute to this PR-Agent + identity (`is_comment_authored_by_pr_agent()` raises when authorship cannot be + verified, which means the thread must be left out). Reply text must arrive + already cleaned of system notes, because the shared limits below keep the last + replies of what a provider yields. The default provider has no thread API. + """ + return iter(()) + + def _code_suggestion_context_budget(self) -> int: + """Return the character budget for the code-suggestion context block. + + `pr_code_suggestions.max_discussion_context_chars` caps the rendered block; 0 + (or any non-positive value) disables the block, and a value that is not a number + falls back to the default rather than dropping the context. + """ + raw_limit = get_settings().pr_code_suggestions.get( + "max_discussion_context_chars", _DEFAULT_DISCUSSION_CONTEXT_CHARS) + try: + return int(raw_limit) + except (TypeError, ValueError): + get_logger().warning( + f"max_discussion_context_chars is not a number ({raw_limit!r}); " + f"using {_DEFAULT_DISCUSSION_CONTEXT_CHARS}") + return _DEFAULT_DISCUSSION_CONTEXT_CHARS + + @staticmethod + def _code_suggestion_context_message(text: str) -> str: + """Return comment text without its trailing hidden marker, bounded in length. + + PR-Agent appends its identity marker as the last line of a comment, so only + trailing marker lines are dropped: a suggestion that quotes `` + in its own text keeps the quoted text. + """ + lines = text.splitlines() + while lines and _TRAILING_AGENT_MARKER_LINE_RE.match(lines[-1]): + lines.pop() + return "\n".join(lines).strip()[:_MAX_DISCUSSION_MESSAGE_CHARS] + + @classmethod + def _code_suggestion_context_thread(cls, thread: dict) -> dict: + """Return one bounded thread entry: trailing markers removed, replies capped.""" + replies = [] + for reply in list(thread.get("replies") or [])[-_MAX_DISCUSSION_REPLIES:]: + message = reply.get("message") + if not isinstance(message, str) or not message.strip(): + continue + replies.append({ + "author": reply.get("author") or "Unknown", + "message": cls._code_suggestion_context_message(message), + }) + suggestion = thread.get("suggestion") + return { + "thread_id": thread.get("thread_id"), + "status": thread.get("status"), + "file": thread.get("file"), + "start_line": thread.get("start_line"), + "end_line": thread.get("end_line"), + "suggestion": cls._code_suggestion_context_message(suggestion) + if isinstance(suggestion, str) else "", + "replies": replies, + } + + def get_code_suggestion_thread_context(self) -> str: + """Return a bounded JSON block of prior code-suggestion threads on this PR. + + Shared by every provider: the limits, the marker cleanup and the budget all live + here, so a provider only supplies `_iter_code_suggestion_threads()`. The budget + is measured on the exact indented JSON that is returned, and threads are appended + newest first until one no longer fits. Empty when the PR has no such threads or + they cannot be listed. + """ + budget = self._code_suggestion_context_budget() + if budget <= 0: + return "" + discussions, context = [], "" + for thread in self._iter_code_suggestion_threads(): + candidate = discussions + [self._code_suggestion_context_thread(thread)] + rendered = json.dumps(candidate, ensure_ascii=False, indent=2) + if len(rendered) > budget: + break + discussions = candidate + context = rendered + if len(discussions) >= _MAX_DISCUSSION_THREADS: + break + return context + #### labels operations #### @abstractmethod def publish_labels(self, labels): diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 6c856da8c0..a78b960558 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -1,5 +1,4 @@ import difflib -import json import posixpath import re import urllib.parse @@ -49,13 +48,6 @@ redact_credentials, ) -# Bounds for the code-suggestion thread context block, matching the Azure DevOps provider: -# a bounded JSON list of prior suggestion threads is injected into the /improve prompt. -_MAX_DISCUSSION_CONTEXT_CHARS = 24000 -_MAX_DISCUSSION_REPLIES = 10 -_MAX_DISCUSSION_THREADS = 50 -_MAX_DISCUSSION_MESSAGE_CHARS = 750 - class DiffNotFoundError(Exception): """Raised when the diff for a merge request cannot be found.""" @@ -1150,17 +1142,20 @@ def supports_review_finding_state(self) -> bool: def supports_code_suggestion_state(self) -> bool: return True - def get_code_suggestion_thread_context(self) -> str: - """Return a bounded JSON block of prior code-suggestion threads on this MR. + def _iter_code_suggestion_threads(self): + """Yield this MR's code-suggestion discussions, newest first. - Empty when the MR has no such threads or they cannot be listed. + Discussions are dropped unless the opener note is an inline agent comment whose + author is this authenticated user, so a human thread that quotes a suggestion + cannot be read back as the agent's own. System notes are skipped; the shared + limits in `GitProvider.get_code_suggestion_thread_context()` bound what is + yielded here. """ try: discussions = self.mr.discussions.list(get_all=True) except (GitlabError, RequestException) as e: get_logger().warning(f"Failed to list discussions of merge request {self.id_mr}: {e}") - return "" - threads, context = [], "" + return for discussion in reversed(discussions): notes = discussion.attributes.get('notes') or [] opener = notes[0] if notes and isinstance(notes[0], dict) else {} @@ -1169,29 +1164,32 @@ def get_code_suggestion_thread_context(self) -> str: line = position.get('new_line') or position.get('old_line') if not isinstance(body, str) or not is_agent_inline_comment(body) or not isinstance(line, int): continue + try: + if not self.is_comment_authored_by_pr_agent(opener): + continue + except RuntimeError as e: + get_logger().debug(f"Skipping merge request {self.id_mr} discussion " + f"{discussion.id}: {e}") + continue replies = [] - for note in notes[1:][-_MAX_DISCUSSION_REPLIES:]: - message = note.get('body') if isinstance(note, dict) and not note.get('system') else None - if isinstance(message, str) and message.strip(): - author = note.get('author') or {} - replies.append({"author": author.get('name') or author.get('username') or "Unknown", - "message": message.strip()[:_MAX_DISCUSSION_MESSAGE_CHARS]}) - threads.append({ + for note in notes[1:]: + if not isinstance(note, dict) or note.get('system'): + continue + message = note.get('body') + if not isinstance(message, str) or not message.strip(): + continue + author = note.get('author') or {} + replies.append({"author": author.get('name') or author.get('username') or "Unknown", + "message": message}) + yield { "thread_id": discussion.id, "status": "resolved" if opener.get('resolved') is True else "open", "file": position.get('new_path') if position.get('new_line') else position.get('old_path'), "start_line": line, "end_line": line, - "suggestion": body.split("\"\n```\n\n" + "\n" + "" + )], + )] + + discussion = json.loads(provider.get_code_suggestion_thread_context())[0] + + assert discussion["suggestion"] == ( + "**Suggestion:** drop the marker\n```suggestion\n" + "body = \"\"\n```" + ) + + def test_strips_the_agent_response_marker_from_replies(self, agent_identity): + provider = _provider_with_diff("/src/app.py") + provider.azure_devops_client.get_threads.return_value = [SimpleNamespace( + id=26, + status="active", + thread_context=SimpleNamespace( + file_path="/src/app.py", + right_file_start=SimpleNamespace(line=9), + right_file_end=SimpleNamespace(line=9), + ), + comments=[ + _agent_comment("**Suggestion:** use value\n```suggestion\nvalue\n```"), + SimpleNamespace( + content="Could this be nullable?\n\n", + author=SimpleNamespace(unique_name="developer@example.com"), + ), + ], + )] + + discussion = json.loads(provider.get_code_suggestion_thread_context())[0] + + assert discussion["replies"] == [{"author": "developer@example.com", "message": "Could this be nullable?"}] + + def test_keeps_the_last_replies_after_filtering_progress_notes(self, agent_identity): + """Progress notes are dropped before the reply cap, so the cap is filled with real replies.""" + provider = _provider_with_diff("/src/app.py") + provider.azure_devops_client.get_threads.return_value = [SimpleNamespace( + id=27, + status="active", + thread_context=SimpleNamespace( + file_path="/src/app.py", + right_file_start=SimpleNamespace(line=11), + right_file_end=SimpleNamespace(line=11), + ), + comments=[ + _agent_comment("**Suggestion:** use value\n```suggestion\nvalue\n```"), + *[ + SimpleNamespace( + content=f"On it! {index}\n", + author=SimpleNamespace(unique_name=_AGENT_IDENTITY), + ) + for index in range(20) + ], + SimpleNamespace( + content="Last word.", + author=SimpleNamespace(unique_name="developer@example.com"), + ), + ], + )] + + discussion = json.loads(provider.get_code_suggestion_thread_context())[0] + + assert discussion["replies"] == [{"author": "developer@example.com", "message": "Last word."}] + def test_adapts_azure_thread_comments_for_conversation_history(self): provider = _provider_with_diff("/src/app.py") provider.azure_devops_client.get_pull_request_thread.return_value = SimpleNamespace(comments=[ @@ -1976,7 +2117,7 @@ def test_excludes_temporary_progress_reply_from_conversation_history(self): assert [comment.id for comment in comments] == [1] - def test_excludes_temporary_progress_reply_from_suggestion_discussions(self): + def test_excludes_temporary_progress_reply_from_suggestion_discussions(self, agent_identity): provider = _provider_with_diff("/src/app.py") provider.azure_devops_client.create_comment.return_value = SimpleNamespace() provider.reply_to_thread(21, "On it! ⏳", True) @@ -1991,7 +2132,7 @@ def test_excludes_temporary_progress_reply_from_suggestion_discussions(self): right_file_end=SimpleNamespace(line=4), ), comments=[ - SimpleNamespace(content="**Suggestion:** fix\n```suggestion\nvalue\n```"), + _agent_comment("**Suggestion:** fix\n```suggestion\nvalue\n```"), SimpleNamespace( content=progress_content, author=SimpleNamespace(display_name="PR-Agent"), @@ -2409,10 +2550,13 @@ def test_rendered_thread_still_feeds_the_next_run(self): suggestion["relevant_lines_start"] = suggestion["relevant_lines_end"] = 2 with patch("pr_agent.git_providers.azuredevops_provider.get_settings") as settings: settings.return_value.get.side_effect = lambda key, default=None: ( - True if key == "config.persistent_inline_comments" else default + _AGENT_IDENTITY if key == "azure_devops_server.agent_identity" else ( + True if key == "config.persistent_inline_comments" else default + ) ) settings.return_value.azure_devops.get.return_value = "active" provider.publish_code_suggestions([suggestion]) + published_opener = _created_threads(provider)[0].comments[0] stored = SimpleNamespace( id=17, status="active", @@ -2422,7 +2566,7 @@ def test_rendered_thread_still_feeds_the_next_run(self): right_file_end=SimpleNamespace(line=2), ), comments=[ - _created_threads(provider)[0].comments[0], + _agent_comment(published_opener.content), SimpleNamespace(content="Not doing this.", author=SimpleNamespace(display_name="Alex")), ], ) diff --git a/tests/unittest/test_code_suggestion_thread_context.py b/tests/unittest/test_code_suggestion_thread_context.py new file mode 100644 index 0000000000..d05c92278a --- /dev/null +++ b/tests/unittest/test_code_suggestion_thread_context.py @@ -0,0 +1,239 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Tests for the shared code-suggestion thread context block. + +`GitProvider.get_code_suggestion_thread_context()` is the single place that bounds and +serializes the prior suggestion threads injected into the /improve prompt. Providers supply +`_iter_code_suggestion_threads()`; the shared code owns the limits, the marker cleanup and +the budget, so the block is measured on the exact string a provider returns. +""" + +import json +from contextlib import contextmanager +from unittest.mock import patch + +from pr_agent.git_providers.git_provider import ( + _DEFAULT_DISCUSSION_CONTEXT_CHARS, + _MAX_DISCUSSION_MESSAGE_CHARS, + _MAX_DISCUSSION_REPLIES, + _MAX_DISCUSSION_THREADS, + GitProvider, +) + +SUGGESTION = "**Suggestion:** guard the value\n```suggestion\nsafe()\n```" + + +class _StubProvider(GitProvider): + """A provider whose only thread source is the shared iterator hook.""" + + def __init__(self, threads=(), budget=_DEFAULT_DISCUSSION_CONTEXT_CHARS): + self._threads = list(threads) + self._budget = budget + self.iterated = False + + def _iter_code_suggestion_threads(self): + self.iterated = True + return iter(self._threads) + + def is_supported(self, capability: str) -> bool: + return False + + def get_files(self) -> list: + return [] + + def get_diff_files(self) -> list: + return [] + + def publish_description(self, pr_title: str, pr_body: str): + pass + + def publish_code_suggestions(self, code_suggestions: list) -> bool: + return False + + def get_languages(self): + return {} + + def get_pr_branch(self): + return "" + + def get_user_id(self): + return "" + + def get_pr_description_full(self) -> str: + return "" + + def get_repo_settings(self): + return b"" + + def publish_comment(self, pr_comment: str, is_temporary: bool = False): + pass + + def publish_inline_comment( + self, body: str, relevant_file: str, relevant_line_in_file: str, original_suggestion=None + ): + pass + + def publish_inline_comments(self, comments: list[dict]): + pass + + def remove_initial_comment(self): + pass + + def remove_comment(self, comment): + pass + + def get_issue_comments(self): + return [] + + def publish_labels(self, labels): + pass + + def get_pr_labels(self, update=False): + return [] + + def remove_reaction(self, issue_comment_id: int, reaction_id: int) -> bool: + return False + + def get_commit_messages(self) -> str: + return "" + + +@contextmanager +def _configured_budget(value): + with patch("pr_agent.git_providers.git_provider.get_settings") as settings: + settings.return_value.pr_code_suggestions.get.side_effect = lambda key, default=None: ( + value if key == "max_discussion_context_chars" else default + ) + yield + + +def _thread(thread_id="d1", suggestion=SUGGESTION, replies=()): + return { + "thread_id": thread_id, + "status": "open", + "file": "src/app.py", + "start_line": 4, + "end_line": 4, + "suggestion": suggestion, + "replies": list(replies), + } + + +def _reply(index, message=None): + return {"author": f"Author {index}", "message": message or f"Reply {index}"} + + +def _rendered(threads, indent=2): + return json.dumps( + [GitProvider._code_suggestion_context_thread(thread) for thread in threads], + ensure_ascii=False, + indent=indent, + ) + + +def test_base_provider_reports_no_threads(): + provider = _StubProvider() + + assert provider.get_code_suggestion_thread_context() == "" + + +def test_budget_is_measured_on_the_returned_indented_json(): + """The block is sized as returned; a compact-only check would let a thread overrun it.""" + threads = [_thread(f"d{index}", suggestion="**Suggestion:** use value") for index in range(10)] + budget = len(_rendered(threads[:5])) + # The compact form of six threads is what Azure DevOps used to measure, and it fits the budget. + assert len(json.dumps( + [GitProvider._code_suggestion_context_thread(thread) for thread in threads[:6]], ensure_ascii=False + )) <= budget + + provider = _StubProvider(threads) + with _configured_budget(budget): + result = provider.get_code_suggestion_thread_context() + + assert len(result) <= budget + assert len(json.loads(result)) == 5 + + +def test_zero_budget_disables_the_block_without_reading_threads(): + provider = _StubProvider([_thread()]) + + with _configured_budget(0): + assert provider.get_code_suggestion_thread_context() == "" + + assert provider.iterated is False + + +def test_non_numeric_budget_falls_back_to_the_default(): + provider = _StubProvider([_thread()]) + + with _configured_budget("not-a-number"): + result = provider.get_code_suggestion_thread_context() + + assert len(result) <= _DEFAULT_DISCUSSION_CONTEXT_CHARS + assert json.loads(result)[0]["thread_id"] == "d1" + + +def test_thread_count_is_capped(): + provider = _StubProvider([_thread(f"d{index}") for index in range(80)]) + + assert len(json.loads(provider.get_code_suggestion_thread_context())) == _MAX_DISCUSSION_THREADS + + +def test_replies_are_capped_to_the_last_ones(): + provider = _StubProvider([_thread(replies=[_reply(index) for index in range(25)])]) + + replies = json.loads(provider.get_code_suggestion_thread_context())[0]["replies"] + + assert len(replies) == _MAX_DISCUSSION_REPLIES + assert replies[0]["author"] == "Author 15" + assert replies[-1]["author"] == "Author 24" + + +def test_blank_replies_are_dropped(): + provider = _StubProvider([_thread(replies=[_reply(0), {"author": "A", "message": " "}, _reply(1)])]) + + replies = json.loads(provider.get_code_suggestion_thread_context())[0]["replies"] + + assert replies == [_reply(0), _reply(1)] + + +def test_long_messages_are_truncated(): + provider = _StubProvider([_thread(suggestion="x" * 2000, replies=[_reply(0, "y" * 2000)])]) + + discussion = json.loads(provider.get_code_suggestion_thread_context())[0] + + assert len(discussion["suggestion"]) == _MAX_DISCUSSION_MESSAGE_CHARS + assert len(discussion["replies"][0]["message"]) == _MAX_DISCUSSION_MESSAGE_CHARS + + +def test_only_trailing_markers_are_stripped(): + """Quoted marker syntax inside a suggestion is kept; the appended marker lines are not.""" + body = ( + "**Suggestion:** drop the marker\n```suggestion\n" + 'body = ""\n```\n\n' + "\n" + "[pr-agent-dedup-code: 112233445566]: https://github.com/The-PR-Agent/pr-agent" + ) + provider = _StubProvider([_thread(suggestion=body)]) + + discussion = json.loads(provider.get_code_suggestion_thread_context())[0] + + assert discussion["suggestion"] == ( + "**Suggestion:** drop the marker\n```suggestion\n" + 'body = ""\n```' + ) + + +def test_missing_fields_do_not_break_the_block(): + provider = _StubProvider([{"thread_id": "d1", "suggestion": SUGGESTION}]) + + discussion = json.loads(provider.get_code_suggestion_thread_context())[0] + + assert discussion == { + "thread_id": "d1", + "status": None, + "file": None, + "start_line": None, + "end_line": None, + "suggestion": SUGGESTION, + "replies": [], + } diff --git a/tests/unittest/test_config_reference.py b/tests/unittest/test_config_reference.py index 1868014420..7dd5c4ecd1 100644 --- a/tests/unittest/test_config_reference.py +++ b/tests/unittest/test_config_reference.py @@ -23,7 +23,7 @@ def _documented_keys(text: str) -> Counter: def test_config_reference_covers_every_active_key(): keys = _toml_keys(CONFIG_TOML.read_text(encoding="utf-8")) - assert sum(keys.values()) == 270 + assert sum(keys.values()) == 271 assert { "model", diff --git a/tests/unittest/test_gitlab_provider.py b/tests/unittest/test_gitlab_provider.py index f1a82997de..e53ad9145c 100644 --- a/tests/unittest/test_gitlab_provider.py +++ b/tests/unittest/test_gitlab_provider.py @@ -9,9 +9,8 @@ from requests.exceptions import RequestException from pr_agent.algo.comment_identity import PRCodeSuggestionsIdentity, PRReviewHeader, PRReviewIdentity -from pr_agent.git_providers.git_provider import IncrementalPR +from pr_agent.git_providers.git_provider import _DEFAULT_DISCUSSION_CONTEXT_CHARS, IncrementalPR from pr_agent.git_providers.gitlab_provider import ( - _MAX_DISCUSSION_CONTEXT_CHARS, GitLabProvider, _GitLabIncrementalCommit, _GitLabIncrementalNote, @@ -1087,7 +1086,7 @@ def test_get_code_suggestion_thread_context_enforces_context_char_budget(self, g result = gitlab_provider.get_code_suggestion_thread_context() - assert len(result) <= _MAX_DISCUSSION_CONTEXT_CHARS + assert len(result) <= _DEFAULT_DISCUSSION_CONTEXT_CHARS assert len(json.loads(result)) < 60 def test_get_code_suggestion_thread_context_empty_without_agent_threads(self, gitlab_provider): @@ -1096,6 +1095,61 @@ def test_get_code_suggestion_thread_context_empty_without_agent_threads(self, gi assert gitlab_provider.get_code_suggestion_thread_context() == "" + def test_get_code_suggestion_thread_context_skips_human_suggestion_threads(self, gitlab_provider): + """A human who opens a thread with a ```suggestion fence is not the agent's own suggestion.""" + gitlab_provider._own_user_id = _BOT_USER_ID + human_suggestion = _thread([_thread_note(author_id=99, body=_AGENT_BODY)], discussion_id='human') + bot_suggestion = _thread([_thread_note()], discussion_id='bot') + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [human_suggestion, bot_suggestion] + + discussions = json.loads(gitlab_provider.get_code_suggestion_thread_context()) + + assert [discussion["thread_id"] for discussion in discussions] == ['bot'] + + def test_get_code_suggestion_thread_context_skips_threads_when_author_is_unknown(self, gitlab_provider): + gitlab_provider._own_user_id = None + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [_thread([_thread_note()])] + + assert gitlab_provider.get_code_suggestion_thread_context() == "" + + def test_get_code_suggestion_thread_context_keeps_quoted_marker_text(self, gitlab_provider): + gitlab_provider._own_user_id = _BOT_USER_ID + body = ( + _AGENT_BODY + + "\n\n\n" + + '[pr-agent-dedup-code: 112233445566]: https://github.com/The-PR-Agent/pr-agent' + ) + thread = _thread([_thread_note(body=body)], discussion_id='d1') + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [thread] + + discussion = json.loads(gitlab_provider.get_code_suggestion_thread_context())[0] + + assert discussion["suggestion"] == _AGENT_BODY + + def test_get_code_suggestion_thread_context_keeps_last_replies_after_filtering(self, gitlab_provider): + """System notes are dropped before the reply cap, so the cap holds real replies.""" + gitlab_provider._own_user_id = _BOT_USER_ID + notes = [_thread_note()] + notes += [ + {'author': {'id': _BOT_USER_ID, 'name': 'PR-Agent'}, 'system': True, 'body': f'changed line {i}'} + for i in range(20) + ] + notes += [{'author': {'id': 99, 'name': 'Alice'}, 'system': False, 'body': 'Last word.'}] + thread = _thread(notes, discussion_id='d1') + + gitlab_provider.mr = MagicMock() + gitlab_provider.mr.discussions.list.return_value = [thread] + + discussion = json.loads(gitlab_provider.get_code_suggestion_thread_context())[0] + + assert discussion["replies"] == [{"author": "Alice", "message": "Last word."}] + def test_get_code_suggestion_thread_context_soft_fails(self, gitlab_provider): gitlab_provider.mr = MagicMock() gitlab_provider.mr.discussions.list.side_effect = GitlabError("gitlab api error")