diff --git a/.pr_agent.toml b/.pr_agent.toml index e0e1800dcd..7c2f4f6f94 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -6,6 +6,7 @@ [pr_reviewer] enable_review_labels_effort = true enable_auto_approval = true +cache_tickets = true [github_app] pr_commands = [ diff --git a/pr_agent/mosaico/env_bridge.py b/pr_agent/mosaico/env_bridge.py index 28c76cfc36..ad36340fcd 100644 --- a/pr_agent/mosaico/env_bridge.py +++ b/pr_agent/mosaico/env_bridge.py @@ -39,6 +39,12 @@ def apply_mosaico_env() -> None: if model_name: model = model_name if "/" in model_name else f"openai/{model_name}" settings.set("CONFIG.MODEL", model) + # Dynaconf merge_enabled=True appends list values instead of replacing; + # pop the leaf first so the subsequent .set() produces a fresh list. + # Use case-insensitive matching because Dynaconf can normalize key casing. + for _k in list(settings.CONFIG.keys()): + if _k.lower() == "fallback_models": + settings.CONFIG.pop(_k, None) settings.set("CONFIG.FALLBACK_MODELS", []) # MOSAICO models are not in pr-agent's built-in MAX_TOKENS table; declare a budget # so reviews don't fail with "not defined in MAX_TOKENS". Overridable via env. @@ -68,6 +74,12 @@ def _register_langfuse_callback(settings) -> None: current = [c for c in (settings.get(key, []) or []) if c != LEGACY_LANGFUSE_CALLBACK] if LANGFUSE_CALLBACK_NAME not in current: current.append(LANGFUSE_CALLBACK_NAME) + # Dynaconf merge_enabled=True appends list values; pop the leaf first. + # Use case-insensitive matching because Dynaconf can normalize key casing. + leaf = key.split(".", 1)[-1] + for _k in list(settings.LITELLM.keys()): + if _k.lower() == leaf.lower(): + settings.LITELLM.pop(_k, None) settings.set(key, current) settings.set("LITELLM.ENABLE_CALLBACKS", True) get_logger().info("MOSAICO: registered Langfuse litellm callback.") diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index 675f5dad9e..007252a142 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -84,6 +84,7 @@ require_security_review=true require_estimate_contribution_time_cost=false require_todo_scan=false require_ticket_analysis_review=true +cache_tickets=true # general options publish_output_no_suggestions=true # Set to "false" if you only need the reviewer's remarks (not labels, not "security audit", etc.) and want to avoid noisy "No major issues detected" comments. persistent_comment=true diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index c4917f3597..576a44c489 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -1,5 +1,6 @@ import copy import datetime +import hashlib import traceback from collections import OrderedDict from functools import partial @@ -96,7 +97,7 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, "custom_labels": "", "enable_custom_labels": get_settings().config.enable_custom_labels, "is_ai_metadata": get_settings().get("config.enable_ai_metadata", False), - "related_tickets": get_settings().get('related_tickets', []), + "related_tickets": self._load_related_tickets(), 'duplicate_prompt_examples': get_settings().config.get('duplicate_prompt_examples', False), "date": datetime.datetime.now().strftime('%Y-%m-%d'), } @@ -108,6 +109,22 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, get_settings().pr_review_prompt.user ) + def _load_related_tickets(self): + if not get_settings().get('pr_reviewer.cache_tickets', True): + return [] + pr_url = getattr(self.git_provider, 'pr_url', None) + if pr_url: + cache_key = f'related_tickets_{hashlib.md5(pr_url.encode()).hexdigest()}' + from pr_agent.tools.ticket_pr_compliance_check import _tickets_cache, _TICKETS_CACHE_TTL + import time + cached = _tickets_cache.get(cache_key) + if cached is not None: + ts, tickets = cached + if time.time() - ts < _TICKETS_CACHE_TTL: + return tickets + del _tickets_cache[cache_key] + return get_settings().get('related_tickets', []) + def parse_incremental(self, args: List[str]): is_incremental = False if args and len(args) >= 1: diff --git a/pr_agent/tools/ticket_pr_compliance_check.py b/pr_agent/tools/ticket_pr_compliance_check.py index f2ae1dca78..13d8324107 100644 --- a/pr_agent/tools/ticket_pr_compliance_check.py +++ b/pr_agent/tools/ticket_pr_compliance_check.py @@ -1,4 +1,6 @@ +import hashlib import re +import time import traceback from pr_agent.config_loader import get_settings @@ -6,6 +8,11 @@ from pr_agent.git_providers import AzureDevopsProvider from pr_agent.log import get_logger +# In-memory ticket cache with TTL to avoid unbounded Dynaconf key accumulation. +# Keys: md5(pr_url), values: (timestamp, ticket_list) +_tickets_cache: dict = {} +_TICKETS_CACHE_TTL = 3600 # 1 hour + # Compile the regex pattern once, outside the function GITHUB_TICKET_PATTERN = re.compile( r'(https://github[^/]+/[^/]+/[^/]+/issues/\d+)|(\b(\w+)/(\w+)#(\d+)\b)|(#\d+)' @@ -244,7 +251,29 @@ async def extract_and_cache_pr_tickets(git_provider, vars): if not get_settings().get('pr_reviewer.require_ticket_analysis_review', False): return - related_tickets = get_settings().get('related_tickets', []) + pr_url = getattr(git_provider, 'pr_url', None) + if not pr_url: + get_logger().info("No PR URL available — cannot cache tickets per-PR") + related_tickets = [] + use_cache = False + else: + use_cache = get_settings().get('pr_reviewer.cache_tickets', True) + cache_key = ( + f'related_tickets_{hashlib.md5(pr_url.encode()).hexdigest()}' + ) + if use_cache: + cached = _tickets_cache.get(cache_key) + if cached is not None: + ts, tickets = cached + if time.time() - ts < _TICKETS_CACHE_TTL: + related_tickets = tickets + else: + del _tickets_cache[cache_key] + related_tickets = [] + else: + related_tickets = [] + else: + related_tickets = [] if not related_tickets: tickets_content = await extract_tickets(git_provider) @@ -261,8 +290,9 @@ async def extract_and_cache_pr_tickets(git_provider, vars): get_logger().info("Extracted tickets and sub-issues from PR description", artifact={"tickets": related_tickets}) - vars['related_tickets'] = related_tickets - get_settings().set('related_tickets', related_tickets) + vars['related_tickets'] = related_tickets + if use_cache: + _tickets_cache[cache_key] = (time.time(), related_tickets) else: get_logger().info("Using cached tickets", artifact={"tickets": related_tickets}) vars['related_tickets'] = related_tickets diff --git a/tests/unittest/_settings_helpers.py b/tests/unittest/_settings_helpers.py index 99bb3099b1..a270c331ae 100644 --- a/tests/unittest/_settings_helpers.py +++ b/tests/unittest/_settings_helpers.py @@ -53,10 +53,14 @@ def _remove_key(settings, key): def restore_settings(snapshot): - """Restore ``snapshot``; truly remove entries whose snapshot is SENTINEL.""" + """Restore ``snapshot``; truly remove entries whose snapshot is SENTINEL. + + Because ``merge_enabled=True`` causes ``settings.set()`` to *append* list + values rather than replace them, the key is always removed first via + ``_remove_key`` before setting the restored value. + """ settings = get_settings() for key, value in snapshot.items(): - if value is SENTINEL: - _remove_key(settings, key) - else: + _remove_key(settings, key) + if value is not SENTINEL: settings.set(key, value) diff --git a/tests/unittest/test_ignore_repositories.py b/tests/unittest/test_ignore_repositories.py index 0baedf743e..e6e27911a0 100644 --- a/tests/unittest/test_ignore_repositories.py +++ b/tests/unittest/test_ignore_repositories.py @@ -4,6 +4,7 @@ from pr_agent.servers.bitbucket_app import should_process_pr_logic as bitbucket_should_process_pr_logic from pr_agent.servers.github_app import should_process_pr_logic as github_should_process_pr_logic from pr_agent.servers.gitlab_webhook import should_process_pr_logic as gitlab_should_process_pr_logic +from tests.unittest._settings_helpers import _remove_key def make_bitbucket_payload(full_name): @@ -42,11 +43,15 @@ def make_gitlab_body(full_name): class TestIgnoreRepositories: def setup_method(self): - get_settings().set("CONFIG.IGNORE_REPOSITORIES", []) + settings = get_settings() + _remove_key(settings, "CONFIG.IGNORE_REPOSITORIES") + settings.set("CONFIG.IGNORE_REPOSITORIES", []) @pytest.mark.parametrize("provider_name, provider_func, body_func", PROVIDERS) def test_should_ignore_matching_repository(self, provider_name, provider_func, body_func): - get_settings().set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) + settings = get_settings() + _remove_key(settings, "CONFIG.IGNORE_REPOSITORIES") + settings.set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) body = { "pull_request": {}, "repository": {"full_name": "org/repo-to-ignore"}, @@ -58,7 +63,9 @@ def test_should_ignore_matching_repository(self, provider_name, provider_func, b @pytest.mark.parametrize("provider_name, provider_func, body_func", PROVIDERS) def test_should_not_ignore_non_matching_repository(self, provider_name, provider_func, body_func): - get_settings().set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) + settings = get_settings() + _remove_key(settings, "CONFIG.IGNORE_REPOSITORIES") + settings.set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) body = { "pull_request": {}, "repository": {"full_name": "org/other-repo"}, diff --git a/tests/unittest/test_mosaico_env_bridge.py b/tests/unittest/test_mosaico_env_bridge.py index 8fd1263d6f..0d649eaeca 100644 --- a/tests/unittest/test_mosaico_env_bridge.py +++ b/tests/unittest/test_mosaico_env_bridge.py @@ -13,6 +13,7 @@ langfuse_env_present) from pr_agent.mosaico.observability import (mosaico_log_context, parse_observability_metadata) +from tests.unittest._settings_helpers import _remove_key _SNAPSHOT_KEYS = [ "OPENAI.API_BASE", @@ -41,10 +42,8 @@ def restore_settings(): litellm_failure = litellm.failure_callback yield for k, v in snapshot.items(): - if v is _SENTINEL: - # Key was absent before; best-effort reset to empty so the global is not polluted. - global_settings.set(k, [] if k.endswith("CALLBACK") or k == "CONFIG.FALLBACK_MODELS" else None) - else: + _remove_key(global_settings, k) + if v is not _SENTINEL: global_settings.set(k, v) litellm.success_callback = litellm_success litellm.failure_callback = litellm_failure diff --git a/tests/unittest/test_retry_with_fallback_models.py b/tests/unittest/test_retry_with_fallback_models.py index e0dd3dfc8a..332fc37697 100644 --- a/tests/unittest/test_retry_with_fallback_models.py +++ b/tests/unittest/test_retry_with_fallback_models.py @@ -5,7 +5,7 @@ from pr_agent.algo.pr_processing import retry_with_fallback_models from pr_agent.algo.utils import ModelType from pr_agent.config_loader import get_settings -from tests.unittest._settings_helpers import SENTINEL, restore_settings, snapshot_settings +from tests.unittest._settings_helpers import SENTINEL, restore_settings, snapshot_settings, _remove_key _TRACKED_KEYS = ( "config.model", @@ -28,10 +28,13 @@ def _restore_settings(snapshot): def test_primary_model_success_invoked_once_and_returns_value(): snapshot = _snapshot_settings() try: - get_settings().set("config.model", "primary-model") - get_settings().set("config.fallback_models", ["fallback-1", "fallback-2"]) - get_settings().set("openai.deployment_id", None) - get_settings().set("openai.fallback_deployments", []) + settings = get_settings() + _remove_key(settings, "config.fallback_models") + settings.set("config.model", "primary-model") + settings.set("config.fallback_models", ["fallback-1", "fallback-2"]) + _remove_key(settings, "openai.fallback_deployments") + settings.set("openai.deployment_id", None) + settings.set("openai.fallback_deployments", []) calls = [] @@ -50,10 +53,13 @@ async def fake_f(model): def test_primary_fails_fallback_succeeds(): snapshot = _snapshot_settings() try: - get_settings().set("config.model", "primary-model") - get_settings().set("config.fallback_models", ["fallback-1", "fallback-2"]) - get_settings().set("openai.deployment_id", None) - get_settings().set("openai.fallback_deployments", []) + settings = get_settings() + _remove_key(settings, "config.fallback_models") + settings.set("config.model", "primary-model") + settings.set("config.fallback_models", ["fallback-1", "fallback-2"]) + _remove_key(settings, "openai.fallback_deployments") + settings.set("openai.deployment_id", None) + settings.set("openai.fallback_deployments", []) calls = [] @@ -74,10 +80,13 @@ async def fake_f(model): def test_all_models_fail_raises_with_aggregate_message_and_cause(): snapshot = _snapshot_settings() try: - get_settings().set("config.model", "primary-model") - get_settings().set("config.fallback_models", ["fallback-1"]) - get_settings().set("openai.deployment_id", None) - get_settings().set("openai.fallback_deployments", []) + settings = get_settings() + _remove_key(settings, "config.fallback_models") + settings.set("config.model", "primary-model") + settings.set("config.fallback_models", ["fallback-1"]) + _remove_key(settings, "openai.fallback_deployments") + settings.set("openai.deployment_id", None) + settings.set("openai.fallback_deployments", []) last_error = ValueError("last failure") attempted = [] @@ -102,10 +111,13 @@ async def fake_f(model): def test_deployment_id_updated_per_attempt(): snapshot = _snapshot_settings() try: - get_settings().set("config.model", "primary-model") - get_settings().set("config.fallback_models", ["fallback-1", "fallback-2"]) - get_settings().set("openai.deployment_id", "deployment-primary") - get_settings().set( + settings = get_settings() + _remove_key(settings, "config.fallback_models") + _remove_key(settings, "openai.fallback_deployments") + settings.set("config.model", "primary-model") + settings.set("config.fallback_models", ["fallback-1", "fallback-2"]) + settings.set("openai.deployment_id", "deployment-primary") + settings.set( "openai.fallback_deployments", ["deployment-fb1", "deployment-fb2"], ) @@ -134,11 +146,14 @@ async def fake_f(model): def test_weak_model_type_uses_weak_setting_and_forwards_identifier(): snapshot = _snapshot_settings() try: - get_settings().set("config.model", "regular-model") - get_settings().set("config.model_weak", "weak-model-id") - get_settings().set("config.fallback_models", []) - get_settings().set("openai.deployment_id", None) - get_settings().set("openai.fallback_deployments", []) + settings = get_settings() + _remove_key(settings, "config.fallback_models") + _remove_key(settings, "openai.fallback_deployments") + settings.set("config.model", "regular-model") + settings.set("config.model_weak", "weak-model-id") + settings.set("config.fallback_models", []) + settings.set("openai.deployment_id", None) + settings.set("openai.fallback_deployments", []) calls = [] @@ -159,11 +174,14 @@ async def fake_f(model): def test_reasoning_model_type_uses_reasoning_setting(): snapshot = _snapshot_settings() try: - get_settings().set("config.model", "regular-model") - get_settings().set("config.model_reasoning", "reasoning-model-id") - get_settings().set("config.fallback_models", []) - get_settings().set("openai.deployment_id", None) - get_settings().set("openai.fallback_deployments", []) + settings = get_settings() + _remove_key(settings, "config.fallback_models") + _remove_key(settings, "openai.fallback_deployments") + settings.set("config.model", "regular-model") + settings.set("config.model_reasoning", "reasoning-model-id") + settings.set("config.fallback_models", []) + settings.set("openai.deployment_id", None) + settings.set("openai.fallback_deployments", []) calls = [] diff --git a/tests/unittest/test_ticket_extraction_async.py b/tests/unittest/test_ticket_extraction_async.py index b6d819b859..dfe6d53d48 100644 --- a/tests/unittest/test_ticket_extraction_async.py +++ b/tests/unittest/test_ticket_extraction_async.py @@ -6,6 +6,7 @@ network access is performed. """ import asyncio +import hashlib import pytest @@ -100,11 +101,16 @@ def settings_snapshot(): """ s = get_settings() snapshot = snapshot_settings( - ["related_tickets", "pr_reviewer.require_ticket_analysis_review"] + [ + "related_tickets", + "pr_reviewer.require_ticket_analysis_review", + "pr_reviewer.cache_tickets", + ] ) # Reset to known defaults for each test s.set("related_tickets", []) s.set("pr_reviewer.require_ticket_analysis_review", False) + s.set("pr_reviewer.cache_tickets", True) try: yield s finally: @@ -424,9 +430,18 @@ def __getattr__(self, name): def test_uses_existing_related_tickets_cache_without_extract( self, settings_snapshot, monkeypatch ): + import time settings_snapshot.set("pr_reviewer.require_ticket_analysis_review", True) cached = [{"ticket_id": 42, "title": "cached"}] - settings_snapshot.set("related_tickets", cached) + settings_snapshot.set("pr_reviewer.cache_tickets", True) + + class _Provider: + pr_url = "https://github.com/org/repo/pull/42" + + provider = _Provider() + + cache_key = f'related_tickets_{hashlib.md5(provider.pr_url.encode()).hexdigest()}' + tpc._tickets_cache[cache_key] = (time.time(), cached) async def _boom(_): raise AssertionError("extract_tickets should not be called when cache is set") @@ -434,15 +449,19 @@ async def _boom(_): monkeypatch.setattr(tpc, "extract_tickets", _boom) vars_ = {} - # Provider value irrelevant — should never be used - asyncio.run(extract_and_cache_pr_tickets(object(), vars_)) + asyncio.run(extract_and_cache_pr_tickets(provider, vars_)) assert vars_["related_tickets"] == cached def test_stores_sub_issues_before_main_issue_in_related_tickets( self, settings_snapshot, monkeypatch ): settings_snapshot.set("pr_reviewer.require_ticket_analysis_review", True) - settings_snapshot.set("related_tickets", []) + settings_snapshot.set("pr_reviewer.cache_tickets", True) + + class _Provider: + pr_url = "https://github.com/org/repo/pull/99" + + provider = _Provider() sub_a = {"ticket_url": "u/sub_a", "title": "sub_a", "body": "s1"} sub_b = {"ticket_url": "u/sub_b", "title": "sub_b", "body": "s2"} @@ -461,15 +480,17 @@ async def _fake_extract(_): monkeypatch.setattr(tpc, "extract_tickets", _fake_extract) vars_ = {} - asyncio.run(extract_and_cache_pr_tickets(object(), vars_)) + asyncio.run(extract_and_cache_pr_tickets(provider, vars_)) # Per current production order: sub-issues are appended first, then main. stored = vars_["related_tickets"] assert stored == [sub_a, sub_b, main_ticket] - # Settings cache is also populated - assert get_settings().get("related_tickets") == stored + # In-memory cache is also populated under the PR-scoped key + cache_key = f'related_tickets_{hashlib.md5(provider.pr_url.encode()).hexdigest()}' + _, cached = tpc._tickets_cache[cache_key] + assert cached == stored - def test_no_tickets_extracted_leaves_vars_untouched( + def test_no_tickets_extracted_sets_empty_list_in_vars( self, settings_snapshot, monkeypatch ): settings_snapshot.set("pr_reviewer.require_ticket_analysis_review", True) @@ -482,4 +503,75 @@ async def _empty(_): vars_ = {} asyncio.run(extract_and_cache_pr_tickets(object(), vars_)) - assert "related_tickets" not in vars_ + assert vars_["related_tickets"] == [] + + def test_per_pr_cache_isolation( + self, settings_snapshot, monkeypatch + ): + settings_snapshot.set("pr_reviewer.require_ticket_analysis_review", True) + + tickets_pr_a = [{"ticket_id": 11111, "title": "PR A ticket"}] + tickets_pr_b = [{"ticket_id": 22222, "title": "PR B ticket"}] + + call_count = {"n": 0} + + async def _side_effect(git_provider): + call_count["n"] += 1 + if "11111" in (getattr(git_provider, "pr_url", "") or ""): + return tickets_pr_a + return tickets_pr_b + + monkeypatch.setattr(tpc, "extract_tickets", _side_effect) + + class _Provider: + pass + + provider_a = _Provider() + provider_a.pr_url = "https://dev.azure.com/org/project/_git/repo/pullrequest/11111" + provider_b = _Provider() + provider_b.pr_url = "https://dev.azure.com/org/project/_git/repo/pullrequest/22222" + + # First PR — should fetch + vars_a = {} + asyncio.run(extract_and_cache_pr_tickets(provider_a, vars_a)) + assert vars_a["related_tickets"] == tickets_pr_a + assert call_count["n"] == 1 + + # Second PR — different URL, should fetch fresh + vars_b = {} + asyncio.run(extract_and_cache_pr_tickets(provider_b, vars_b)) + assert vars_b["related_tickets"] == tickets_pr_b + assert call_count["n"] == 2 + + # Repeat first PR — should use cache, not fetch + vars_a2 = {} + asyncio.run(extract_and_cache_pr_tickets(provider_a, vars_a2)) + assert vars_a2["related_tickets"] == tickets_pr_a + assert call_count["n"] == 2 # no additional fetch + + def test_cache_tickets_disabled_always_fetches( + self, settings_snapshot, monkeypatch + ): + settings_snapshot.set("pr_reviewer.require_ticket_analysis_review", True) + settings_snapshot.set("pr_reviewer.cache_tickets", False) + + call_count = {"n": 0} + + async def _side_effect(_): + call_count["n"] += 1 + return [{"ticket_id": call_count["n"], "title": f"fetch {call_count['n']}"}] + + monkeypatch.setattr(tpc, "extract_tickets", _side_effect) + + provider = object() + + vars1 = {} + asyncio.run(extract_and_cache_pr_tickets(provider, vars1)) + assert vars1["related_tickets"] == [{"ticket_id": 1, "title": "fetch 1"}] + assert call_count["n"] == 1 + + # Second call should also fetch (no cache) + vars2 = {} + asyncio.run(extract_and_cache_pr_tickets(provider, vars2)) + assert vars2["related_tickets"] == [{"ticket_id": 2, "title": "fetch 2"}] + assert call_count["n"] == 2