From 7f0d154df95ad558e44d38b07a969986ac0b93df Mon Sep 17 00:00:00 2001 From: Utsab Date: Wed, 1 Jul 2026 17:48:36 +0545 Subject: [PATCH 1/3] fix: scope ticket cache per-PR and fix Dynaconf list merge leaks in tests - Cache key is now 'related_tickets_' instead of global 'related_tickets', preventing stale tickets leaking between unrelated PRs - Added cache_tickets=true config flag under [pr_reviewer] to allow disabling - Fixed Dynaconf merge_enabled=True list append bug in both production code (env_bridge.py uses case-insensitive pop before set) and test helpers (restore_settings now calls _remove_key before every set) - Added azure-identity dependency for Azure DevOps provider availability - 1043 tests pass, 0 failures --- pr_agent/mosaico/env_bridge.py | 12 +++ pr_agent/settings/configuration.toml | 1 + pr_agent/tools/ticket_pr_compliance_check.py | 10 ++- tests/unittest/_settings_helpers.py | 12 ++- tests/unittest/test_ignore_repositories.py | 13 +++- tests/unittest/test_mosaico_env_bridge.py | 7 +- .../test_retry_with_fallback_models.py | 72 ++++++++++------- .../unittest/test_ticket_extraction_async.py | 78 ++++++++++++++++++- 8 files changed, 164 insertions(+), 41 deletions(-) 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/ticket_pr_compliance_check.py b/pr_agent/tools/ticket_pr_compliance_check.py index f2ae1dca78..eb8469f9fc 100644 --- a/pr_agent/tools/ticket_pr_compliance_check.py +++ b/pr_agent/tools/ticket_pr_compliance_check.py @@ -1,3 +1,4 @@ +import hashlib import re import traceback @@ -244,7 +245,11 @@ 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', []) + use_cache = get_settings().get('pr_reviewer.cache_tickets', True) + pr_url = getattr(git_provider, 'pr_url', None) + cache_key = f'related_tickets_{hashlib.md5(pr_url.encode()).hexdigest()}' if pr_url else 'related_tickets' + + related_tickets = get_settings().get(cache_key, []) if use_cache else [] if not related_tickets: tickets_content = await extract_tickets(git_provider) @@ -262,7 +267,8 @@ async def extract_and_cache_pr_tickets(git_provider, vars): artifact={"tickets": related_tickets}) vars['related_tickets'] = related_tickets - get_settings().set('related_tickets', related_tickets) + if use_cache: + get_settings().set(cache_key, 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..c462f3e436 100644 --- a/tests/unittest/test_ticket_extraction_async.py +++ b/tests/unittest/test_ticket_extraction_async.py @@ -100,11 +100,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: @@ -483,3 +488,74 @@ async def _empty(_): vars_ = {} asyncio.run(extract_and_cache_pr_tickets(object(), vars_)) assert "related_tickets" not in vars_ + + 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 From fc8ea8c75c32820c2589f6cfff699a14faa816f5 Mon Sep 17 00:00:00 2001 From: Utsab Date: Thu, 2 Jul 2026 11:33:56 +0545 Subject: [PATCH 2/3] fix: address bot review comments on PR #2489 - Skip caching entirely when PR URL is unavailable (avoid global key fallback) - Add cache_tickets to .pr_agent.toml to mirror configuration.toml - Wrap overlong cache_key assignment to stay within 120-char line limit - Update PRReviewer to read PR-scoped cache key for accurate token budgeting --- .pr_agent.toml | 1 + pr_agent/tools/pr_reviewer.py | 12 +++++++- pr_agent/tools/ticket_pr_compliance_check.py | 14 +++++++--- .../unittest/test_ticket_extraction_async.py | 28 ++++++++++++++----- 4 files changed, 43 insertions(+), 12 deletions(-) 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/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index c4917f3597..513f923d52 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,15 @@ 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): + pr_url = getattr(self.git_provider, 'pr_url', None) + if pr_url: + cache_key = f'related_tickets_{hashlib.md5(pr_url.encode()).hexdigest()}' + tickets = get_settings().get(cache_key, None) + if tickets is not None: + return tickets + 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 eb8469f9fc..12de710729 100644 --- a/pr_agent/tools/ticket_pr_compliance_check.py +++ b/pr_agent/tools/ticket_pr_compliance_check.py @@ -245,11 +245,17 @@ async def extract_and_cache_pr_tickets(git_provider, vars): if not get_settings().get('pr_reviewer.require_ticket_analysis_review', False): return - use_cache = get_settings().get('pr_reviewer.cache_tickets', True) pr_url = getattr(git_provider, 'pr_url', None) - cache_key = f'related_tickets_{hashlib.md5(pr_url.encode()).hexdigest()}' if pr_url else 'related_tickets' - - related_tickets = get_settings().get(cache_key, []) if use_cache else [] + 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()}' + ) + related_tickets = get_settings().get(cache_key, []) if use_cache else [] if not related_tickets: tickets_content = await extract_tickets(git_provider) diff --git a/tests/unittest/test_ticket_extraction_async.py b/tests/unittest/test_ticket_extraction_async.py index c462f3e436..ec0a1d1df4 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 @@ -431,7 +432,15 @@ def test_uses_existing_related_tickets_cache_without_extract( ): 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()}' + settings_snapshot.set(cache_key, cached) async def _boom(_): raise AssertionError("extract_tickets should not be called when cache is set") @@ -439,15 +448,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"} @@ -466,13 +479,14 @@ 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 + # Settings cache is also populated under the PR-scoped key + cache_key = f'related_tickets_{hashlib.md5(provider.pr_url.encode()).hexdigest()}' + assert get_settings().get(cache_key) == stored def test_no_tickets_extracted_leaves_vars_untouched( self, settings_snapshot, monkeypatch From b36aa26852470db2c9be4b71cb565fdc4d243b2c Mon Sep 17 00:00:00 2001 From: Utsab Date: Thu, 2 Jul 2026 12:16:00 +0545 Subject: [PATCH 3/3] fix: address bot review comments for ticket cache scoping (#2383) - Add cache_tickets check to _load_related_tickets (prevent cache bypass) - Cache empty extraction results too (avoid repeated API calls) - Replace Dynaconf cache with in-memory dict + 1h TTL (bounded cache) - Update tests to use new in-memory cache --- pr_agent/tools/pr_reviewer.py | 13 +++++++--- pr_agent/tools/ticket_pr_compliance_check.py | 26 ++++++++++++++++--- .../unittest/test_ticket_extraction_async.py | 12 +++++---- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index 513f923d52..576a44c489 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -110,12 +110,19 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, ) 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()}' - tickets = get_settings().get(cache_key, None) - if tickets is not None: - return tickets + 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]): diff --git a/pr_agent/tools/ticket_pr_compliance_check.py b/pr_agent/tools/ticket_pr_compliance_check.py index 12de710729..13d8324107 100644 --- a/pr_agent/tools/ticket_pr_compliance_check.py +++ b/pr_agent/tools/ticket_pr_compliance_check.py @@ -1,5 +1,6 @@ import hashlib import re +import time import traceback from pr_agent.config_loader import get_settings @@ -7,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+)' @@ -255,7 +261,19 @@ async def extract_and_cache_pr_tickets(git_provider, vars): cache_key = ( f'related_tickets_{hashlib.md5(pr_url.encode()).hexdigest()}' ) - related_tickets = get_settings().get(cache_key, []) if use_cache else [] + 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) @@ -272,9 +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 - if use_cache: - get_settings().set(cache_key, 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/test_ticket_extraction_async.py b/tests/unittest/test_ticket_extraction_async.py index ec0a1d1df4..dfe6d53d48 100644 --- a/tests/unittest/test_ticket_extraction_async.py +++ b/tests/unittest/test_ticket_extraction_async.py @@ -430,6 +430,7 @@ 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("pr_reviewer.cache_tickets", True) @@ -440,7 +441,7 @@ class _Provider: provider = _Provider() cache_key = f'related_tickets_{hashlib.md5(provider.pr_url.encode()).hexdigest()}' - settings_snapshot.set(cache_key, cached) + tpc._tickets_cache[cache_key] = (time.time(), cached) async def _boom(_): raise AssertionError("extract_tickets should not be called when cache is set") @@ -484,11 +485,12 @@ async def _fake_extract(_): # 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 under the PR-scoped key + # In-memory cache is also populated under the PR-scoped key cache_key = f'related_tickets_{hashlib.md5(provider.pr_url.encode()).hexdigest()}' - assert get_settings().get(cache_key) == stored + _, 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) @@ -501,7 +503,7 @@ 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