From 7f0d154df95ad558e44d38b07a969986ac0b93df Mon Sep 17 00:00:00 2001 From: Utsab Date: Wed, 1 Jul 2026 17:48:36 +0545 Subject: [PATCH 1/8] 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 f52a2e0ed8fb242605acdae63318986683f0b44c Mon Sep 17 00:00:00 2001 From: Utsab Date: Wed, 1 Jul 2026 17:49:20 +0545 Subject: [PATCH 2/8] Revert "fix: scope ticket cache per-PR and fix Dynaconf list merge leaks in tests" This reverts commit 7f0d154df95ad558e44d38b07a969986ac0b93df. --- 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, 41 insertions(+), 164 deletions(-) diff --git a/pr_agent/mosaico/env_bridge.py b/pr_agent/mosaico/env_bridge.py index ad36340fcd..28c76cfc36 100644 --- a/pr_agent/mosaico/env_bridge.py +++ b/pr_agent/mosaico/env_bridge.py @@ -39,12 +39,6 @@ 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. @@ -74,12 +68,6 @@ 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 007252a142..675f5dad9e 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -84,7 +84,6 @@ 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 eb8469f9fc..f2ae1dca78 100644 --- a/pr_agent/tools/ticket_pr_compliance_check.py +++ b/pr_agent/tools/ticket_pr_compliance_check.py @@ -1,4 +1,3 @@ -import hashlib import re import traceback @@ -245,11 +244,7 @@ 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 [] + related_tickets = get_settings().get('related_tickets', []) if not related_tickets: tickets_content = await extract_tickets(git_provider) @@ -267,8 +262,7 @@ async def extract_and_cache_pr_tickets(git_provider, vars): artifact={"tickets": related_tickets}) vars['related_tickets'] = related_tickets - if use_cache: - get_settings().set(cache_key, related_tickets) + get_settings().set('related_tickets', 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 a270c331ae..99bb3099b1 100644 --- a/tests/unittest/_settings_helpers.py +++ b/tests/unittest/_settings_helpers.py @@ -53,14 +53,10 @@ def _remove_key(settings, key): def restore_settings(snapshot): - """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. - """ + """Restore ``snapshot``; truly remove entries whose snapshot is SENTINEL.""" settings = get_settings() for key, value in snapshot.items(): - _remove_key(settings, key) - if value is not SENTINEL: + if value is SENTINEL: + _remove_key(settings, key) + else: settings.set(key, value) diff --git a/tests/unittest/test_ignore_repositories.py b/tests/unittest/test_ignore_repositories.py index e6e27911a0..0baedf743e 100644 --- a/tests/unittest/test_ignore_repositories.py +++ b/tests/unittest/test_ignore_repositories.py @@ -4,7 +4,6 @@ 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): @@ -43,15 +42,11 @@ def make_gitlab_body(full_name): class TestIgnoreRepositories: def setup_method(self): - settings = get_settings() - _remove_key(settings, "CONFIG.IGNORE_REPOSITORIES") - settings.set("CONFIG.IGNORE_REPOSITORIES", []) + get_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): - settings = get_settings() - _remove_key(settings, "CONFIG.IGNORE_REPOSITORIES") - settings.set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) + get_settings().set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) body = { "pull_request": {}, "repository": {"full_name": "org/repo-to-ignore"}, @@ -63,9 +58,7 @@ 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): - settings = get_settings() - _remove_key(settings, "CONFIG.IGNORE_REPOSITORIES") - settings.set("CONFIG.IGNORE_REPOSITORIES", ["org/repo-to-ignore"]) + get_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 0d649eaeca..8fd1263d6f 100644 --- a/tests/unittest/test_mosaico_env_bridge.py +++ b/tests/unittest/test_mosaico_env_bridge.py @@ -13,7 +13,6 @@ 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", @@ -42,8 +41,10 @@ def restore_settings(): litellm_failure = litellm.failure_callback yield for k, v in snapshot.items(): - _remove_key(global_settings, k) - if v is not _SENTINEL: + 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: 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 332fc37697..e0dd3dfc8a 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, _remove_key +from tests.unittest._settings_helpers import SENTINEL, restore_settings, snapshot_settings _TRACKED_KEYS = ( "config.model", @@ -28,13 +28,10 @@ def _restore_settings(snapshot): def test_primary_model_success_invoked_once_and_returns_value(): snapshot = _snapshot_settings() try: - 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", []) + 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", []) calls = [] @@ -53,13 +50,10 @@ async def fake_f(model): def test_primary_fails_fallback_succeeds(): snapshot = _snapshot_settings() try: - 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", []) + 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", []) calls = [] @@ -80,13 +74,10 @@ async def fake_f(model): def test_all_models_fail_raises_with_aggregate_message_and_cause(): snapshot = _snapshot_settings() try: - 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", []) + 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", []) last_error = ValueError("last failure") attempted = [] @@ -111,13 +102,10 @@ async def fake_f(model): def test_deployment_id_updated_per_attempt(): snapshot = _snapshot_settings() try: - 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( + 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( "openai.fallback_deployments", ["deployment-fb1", "deployment-fb2"], ) @@ -146,14 +134,11 @@ async def fake_f(model): def test_weak_model_type_uses_weak_setting_and_forwards_identifier(): snapshot = _snapshot_settings() try: - 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", []) + 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", []) calls = [] @@ -174,14 +159,11 @@ async def fake_f(model): def test_reasoning_model_type_uses_reasoning_setting(): snapshot = _snapshot_settings() try: - 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", []) + 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", []) calls = [] diff --git a/tests/unittest/test_ticket_extraction_async.py b/tests/unittest/test_ticket_extraction_async.py index c462f3e436..b6d819b859 100644 --- a/tests/unittest/test_ticket_extraction_async.py +++ b/tests/unittest/test_ticket_extraction_async.py @@ -100,16 +100,11 @@ def settings_snapshot(): """ s = get_settings() snapshot = snapshot_settings( - [ - "related_tickets", - "pr_reviewer.require_ticket_analysis_review", - "pr_reviewer.cache_tickets", - ] + ["related_tickets", "pr_reviewer.require_ticket_analysis_review"] ) # 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: @@ -488,74 +483,3 @@ 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 7c6740309e1a6a27c5167306f90af47dce4e794a Mon Sep 17 00:00:00 2001 From: Utsab Date: Fri, 3 Jul 2026 13:47:06 +0545 Subject: [PATCH 3/8] fix: route /improve to inline suggestions for GitLab Changes tab Providers like GitLab support publish_code_suggestions (inline diff comments) but lack publish_inline_comments (generic review comments that GitHub uses). The old condition only checked gfm_markdown, so GitLab always rendered a table in the Overview tab instead of actionable inline suggestions in the Changes tab. Adding publish_inline_comments to the routing condition makes GitLab (and any future provider with a similar split) fall through to the inline path, while GitHub (which has both) keeps the table. --- pr_agent/tools/pr_code_suggestions.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index 0fdc8019fe..c6bd285b44 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -130,7 +130,8 @@ async def run(self): # Publish table summarized suggestions if ((not get_settings().pr_code_suggestions.commitable_code_suggestions) and - self.git_provider.is_supported("gfm_markdown")): + self.git_provider.is_supported("gfm_markdown") and + self.git_provider.is_supported("publish_inline_comments")): # generate summarized suggestions pr_body = self.generate_summarized_suggestions(data) From 51bac387b121c5ced8f02809eaccdbb7b41456bc Mon Sep 17 00:00:00 2001 From: Utsab Date: Fri, 3 Jul 2026 13:57:57 +0545 Subject: [PATCH 4/8] fix: correct suggestion range computation in GitLab publish_code_suggestions relevant_lines_end is inclusive (matching GitHub, Bitbucket, Azure DevOps providers), so the range for GitLab's suggestion format must be end - start + 1. The previous formula (end - start) undercounted by 1, producing range=0 for single-line suggestions and broken alignment. --- pr_agent/git_providers/gitlab_provider.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index 2128e2670b..db0687a588 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -677,7 +677,7 @@ def publish_code_suggestions(self, code_suggestions: list) -> bool: if target_file is None: get_logger().warning(f"Skipping suggestion: file '{relevant_file}' not found in diff") continue - range = relevant_lines_end - relevant_lines_start # no need to add 1 + range = relevant_lines_end - relevant_lines_start + 1 body = body.replace('```suggestion', f'```suggestion:-0+{range}') lines = target_file.head_file.splitlines() relevant_line_in_file = lines[relevant_lines_start - 1] From 9dd4d3d74281bbf1cb9201b55512745a729fe2de Mon Sep 17 00:00:00 2001 From: Utsab Date: Fri, 3 Jul 2026 16:04:14 +0545 Subject: [PATCH 5/8] fix: apply --config.* overrides before provider creation; unblock --config.git_provider from CLI - Move update_settings_from_args before apply_repo_settings in _handle_request so CLI config overrides (--config.*) are effective during provider creation - Remove git_provider from the forbidden CLI args list since selecting a provider via CLI is a safe operation that does not expose secrets --- pr_agent/agent/pr_agent.py | 10 +++++----- pr_agent/algo/cli_args.py | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pr_agent/agent/pr_agent.py b/pr_agent/agent/pr_agent.py index 4c2d2ef1c2..74edce2837 100644 --- a/pr_agent/agent/pr_agent.py +++ b/pr_agent/agent/pr_agent.py @@ -53,10 +53,6 @@ def __init__(self, ai_handler: partial[BaseAiHandler,] = LiteLLMAIHandler): self.ai_handler = ai_handler # will be initialized in run_action async def _handle_request(self, pr_url, request, notify=None) -> bool: - # First, apply repo specific settings if exists - apply_repo_settings(pr_url) - - # Then, apply user specific settings if exists if isinstance(request, str): request = request.replace("'", "\\'") lexer = shlex.shlex(request, posix=True) @@ -73,9 +69,13 @@ async def _handle_request(self, pr_url, request, notify=None) -> bool: ) return False - # Update settings from args + # Update settings from args before apply_repo_settings, + # so that --config.git_provider=gitlab is effective for provider creation args = update_settings_from_args(args) + # First, apply repo specific settings if exists + apply_repo_settings(pr_url) + # Append the response language in the extra instructions response_language = get_settings().config.get('response_language', 'en-us') if response_language.lower() != 'en-us': diff --git a/pr_agent/algo/cli_args.py b/pr_agent/algo/cli_args.py index 246864cefa..f94d88f0c7 100644 --- a/pr_agent/algo/cli_args.py +++ b/pr_agent/algo/cli_args.py @@ -10,7 +10,7 @@ def validate_user_args(args: list) -> (bool, str): # decode forbidden args # b64encode('word'.encode()).decode() - _encoded_args = 'c2hhcmVkX3NlY3JldA==:dXNlcg==:c3lzdGVt:ZW5hYmxlX2NvbW1lbnRfYXBwcm92YWw=:ZW5hYmxlX21hbnVhbF9hcHByb3ZhbA==:ZW5hYmxlX2F1dG9fYXBwcm92YWw=:YXBwcm92ZV9wcl9vbl9zZWxmX3Jldmlldw==:YmFzZV91cmw=:dXJs:YXBwX25hbWU=:c2VjcmV0X3Byb3ZpZGVy:Z2l0X3Byb3ZpZGVy:c2tpcF9rZXlz:b3BlbmFpLmtleQ==:QU5BTFlUSUNTX0ZPTERFUg==:dXJp:YXBwX2lk:d2ViaG9va19zZWNyZXQ=:YmVhcmVyX3Rva2Vu:UEVSU09OQUxfQUNDRVNTX1RPS0VO:b3ZlcnJpZGVfZGVwbG95bWVudF90eXBl:cHJpdmF0ZV9rZXk=:bG9jYWxfY2FjaGVfcGF0aA==:ZW5hYmxlX2xvY2FsX2NhY2hl:amlyYV9iYXNlX3VybA==:YXBpX2Jhc2U=:YXBpX3R5cGU=:YXBpX3ZlcnNpb24=:c2tpcF9rZXlz' + _encoded_args = 'c2hhcmVkX3NlY3JldA==:dXNlcg==:c3lzdGVt:ZW5hYmxlX2NvbW1lbnRfYXBwcm92YWw=:ZW5hYmxlX21hbnVhbF9hcHByb3ZhbA==:ZW5hYmxlX2F1dG9fYXBwcm92YWw=:YXBwcm92ZV9wcl9vbl9zZWxmX3Jldmlldw==:YmFzZV91cmw=:dXJs:YXBwX25hbWU=:c2VjcmV0X3Byb3ZpZGVy:c2tpcF9rZXlz:b3BlbmFpLmtleQ==:QU5BTFlUSUNTX0ZPTERFUg==:dXJp:YXBwX2lk:d2ViaG9va19zZWNyZXQ=:YmVhcmVyX3Rva2Vu:UEVSU09OQUxfQUNDRVNTX1RPS0VO:b3ZlcnJpZGVfZGVwbG95bWVudF90eXBl:cHJpdmF0ZV9rZXk=:bG9jYWxfY2FjaGVfcGF0aA==:ZW5hYmxlX2xvY2FsX2NhY2hl:amlyYV9iYXNlX3VybA==:YXBpX2Jhc2U=:YXBpX3R5cGU=:YXBpX3ZlcnNpb24=:c2tpcF9rZXlz' forbidden_cli_args = [] for e in _encoded_args.split(':'): From 105d8175c7f15e8d860394bd2aff2ec0b05b10b7 Mon Sep 17 00:00:00 2001 From: Utsab Date: Fri, 3 Jul 2026 16:17:02 +0545 Subject: [PATCH 6/8] fix: maintain CLI>repo precedence after reorder; reformat encoded_args list - Re-apply CLI --key=value overrides after apply_repo_settings() so that CLI args still take precedence over repo .pr_agent.toml values - Split the base64-encoded forbidden-args string into a documented list to stay within the 120-char line limit --- pr_agent/agent/pr_agent.py | 12 ++++++++++-- pr_agent/algo/cli_args.py | 32 +++++++++++++++++++++++++++++++- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/pr_agent/agent/pr_agent.py b/pr_agent/agent/pr_agent.py index 74edce2837..f6be36d664 100644 --- a/pr_agent/agent/pr_agent.py +++ b/pr_agent/agent/pr_agent.py @@ -69,13 +69,21 @@ async def _handle_request(self, pr_url, request, notify=None) -> bool: ) return False - # Update settings from args before apply_repo_settings, - # so that --config.git_provider=gitlab is effective for provider creation + # Extract CLI --key=value overrides so they can be applied before + # apply_repo_settings (needed for --config.git_provider=gitlab etc.) + # and re-applied afterward to maintain CLI > repo precedence. + cli_key_values = [a for a in args if a.startswith('--') and '=' in a] + + # Apply CLI overrides before repo settings, so provider creation + # (e.g. --config.git_provider=gitlab) can see them. args = update_settings_from_args(args) # First, apply repo specific settings if exists apply_repo_settings(pr_url) + # Re-apply CLI overrides so they take precedence over repo .pr_agent.toml + update_settings_from_args(cli_key_values) + # Append the response language in the extra instructions response_language = get_settings().config.get('response_language', 'en-us') if response_language.lower() != 'en-us': diff --git a/pr_agent/algo/cli_args.py b/pr_agent/algo/cli_args.py index f94d88f0c7..f36c71e525 100644 --- a/pr_agent/algo/cli_args.py +++ b/pr_agent/algo/cli_args.py @@ -10,7 +10,37 @@ def validate_user_args(args: list) -> (bool, str): # decode forbidden args # b64encode('word'.encode()).decode() - _encoded_args = 'c2hhcmVkX3NlY3JldA==:dXNlcg==:c3lzdGVt:ZW5hYmxlX2NvbW1lbnRfYXBwcm92YWw=:ZW5hYmxlX21hbnVhbF9hcHByb3ZhbA==:ZW5hYmxlX2F1dG9fYXBwcm92YWw=:YXBwcm92ZV9wcl9vbl9zZWxmX3Jldmlldw==:YmFzZV91cmw=:dXJs:YXBwX25hbWU=:c2VjcmV0X3Byb3ZpZGVy:c2tpcF9rZXlz:b3BlbmFpLmtleQ==:QU5BTFlUSUNTX0ZPTERFUg==:dXJp:YXBwX2lk:d2ViaG9va19zZWNyZXQ=:YmVhcmVyX3Rva2Vu:UEVSU09OQUxfQUNDRVNTX1RPS0VO:b3ZlcnJpZGVfZGVwbG95bWVudF90eXBl:cHJpdmF0ZV9rZXk=:bG9jYWxfY2FjaGVfcGF0aA==:ZW5hYmxlX2xvY2FsX2NhY2hl:amlyYV9iYXNlX3VybA==:YXBpX2Jhc2U=:YXBpX3R5cGU=:YXBpX3ZlcnNpb24=:c2tpcF9rZXlz' + _encoded_parts = [ + 'c2hhcmVkX3NlY3JldA==', # shared_secret + 'dXNlcg==', # user + 'c3lzdGVt', # system + 'ZW5hYmxlX2NvbW1lbnRfYXBwcm92YWw=', # enable_comment_approval + 'ZW5hYmxlX21hbnVhbF9hcHByb3ZhbA==', # enable_manual_approval + 'ZW5hYmxlX2F1dG9fYXBwcm92YWw=', # enable_auto_approval + 'YXBwcm92ZV9wcl9vbl9zZWxmX3Jldmlldw==', # approve_pr_on_self_review + 'YmFzZV91cmw=', # base_url + 'dXJs', # url + 'YXBwX25hbWU=', # app_name + 'c2VjcmV0X3Byb3ZpZGVy', # secret_provider + 'c2tpcF9rZXlz', # skip_keys + 'b3BlbmFpLmtleQ==', # openai.key + 'QU5BTFlUSUNTX0ZPTERFUg==', # ANALYTICS_FOLDER + 'dXJp', # uri + 'YXBwX2lk', # app_id + 'd2ViaG9va19zZWNyZXQ=', # webhook_secret + 'YmVhcmVyX3Rva2Vu', # bearer_token + 'UEVSU09OQUxfQUNDRVNTX1RPS0VO', # PERSONAL_ACCESS_TOKEN + 'b3ZlcnJpZGVfZGVwbG95bWVudF90eXBl', # override_deployment_type + 'cHJpdmF0ZV9rZXk=', # private_key + 'bG9jYWxfY2FjaGVfcGF0aA==', # local_cache_path + 'ZW5hYmxlX2xvY2FsX2NhY2hl', # enable_local_cache + 'amlyYV9iYXNlX3VybA==', # jira_base_url + 'YXBpX2Jhc2U=', # api_base + 'YXBpX3R5cGU=', # api_type + 'YXBpX3ZlcnNpb24=', # api_version + 'c2tpcF9rZXlz', # skip_keys + ] + _encoded_args = ':'.join(_encoded_parts) forbidden_cli_args = [] for e in _encoded_args.split(':'): From 33fcfa359e0b370b83bc416d9e4fa616b99d8bab Mon Sep 17 00:00:00 2001 From: Utsab Date: Fri, 3 Jul 2026 16:22:35 +0545 Subject: [PATCH 7/8] test: move git_provider from forbidden to allowed args in test git_provider was intentionally removed from the forbidden CLI args list so that --config.git_provider=gitlab can be used to select the Git provider. Update the security test to match. --- tests/unittest/test_cli_args_security.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unittest/test_cli_args_security.py b/tests/unittest/test_cli_args_security.py index 696d653f6b..851ffc5d64 100644 --- a/tests/unittest/test_cli_args_security.py +++ b/tests/unittest/test_cli_args_security.py @@ -28,7 +28,6 @@ "--config.uri=https://evil.example", # provider / auth selection and skip lists "--config.secret_provider=aws", - "--config.git_provider=github", "--config.skip_keys=foo", "--auth.bearer_token=abc", "--provider.personal_access_token=ghp_xxx", @@ -58,6 +57,8 @@ "--pr_reviewer.require_tests_review=true", "--config.response_language=zh-tw", "--pr_description.publish_labels=false", + # git_provider is allowed (removed from forbidden list so CLI can select provider) + "--config.git_provider=gitlab", # non-flag arguments are not validated against the forbidden list "some-positional-arg", "yes", From 33c020e247353b8249ec43557bc91b603da75e47 Mon Sep 17 00:00:00 2001 From: Utsab Date: Fri, 3 Jul 2026 16:29:47 +0545 Subject: [PATCH 8/8] fix: remove duplicate skip_keys from encoded_parts; rename range to num_lines - Remove duplicate c2tpcF9rZXlz (skip_keys) entry from _encoded_parts list - Rename variable 'range' to 'num_lines' to avoid shadowing Python built-in --- pr_agent/algo/cli_args.py | 1 - pr_agent/git_providers/gitlab_provider.py | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pr_agent/algo/cli_args.py b/pr_agent/algo/cli_args.py index f36c71e525..b99b683ca3 100644 --- a/pr_agent/algo/cli_args.py +++ b/pr_agent/algo/cli_args.py @@ -38,7 +38,6 @@ def validate_user_args(args: list) -> (bool, str): 'YXBpX2Jhc2U=', # api_base 'YXBpX3R5cGU=', # api_type 'YXBpX3ZlcnNpb24=', # api_version - 'c2tpcF9rZXlz', # skip_keys ] _encoded_args = ':'.join(_encoded_parts) diff --git a/pr_agent/git_providers/gitlab_provider.py b/pr_agent/git_providers/gitlab_provider.py index db0687a588..3315d73e80 100644 --- a/pr_agent/git_providers/gitlab_provider.py +++ b/pr_agent/git_providers/gitlab_provider.py @@ -677,8 +677,8 @@ def publish_code_suggestions(self, code_suggestions: list) -> bool: if target_file is None: get_logger().warning(f"Skipping suggestion: file '{relevant_file}' not found in diff") continue - range = relevant_lines_end - relevant_lines_start + 1 - body = body.replace('```suggestion', f'```suggestion:-0+{range}') + num_lines = relevant_lines_end - relevant_lines_start + 1 + body = body.replace('```suggestion', f'```suggestion:-0+{num_lines}') lines = target_file.head_file.splitlines() relevant_line_in_file = lines[relevant_lines_start - 1]