diff --git a/pr_agent/config_loader.py b/pr_agent/config_loader.py index ac7343f288..ac0f85c2f8 100644 --- a/pr_agent/config_loader.py +++ b/pr_agent/config_loader.py @@ -1,3 +1,4 @@ +import copy from os.path import abspath, dirname, join from pathlib import Path from typing import Optional @@ -91,6 +92,37 @@ def _find_pyproject() -> Optional[Path]: get_settings().load_file(pyproject_path, env=f'tool.{PR_AGENT_TOML_KEY}') +# --- State-leak fix --------------------------------------------------------- +# Snapshot of global_settings captured on first call to reset_to_startup_defaults(). +# Lazy (not taken at import time) so that startup tasks like +# apply_secrets_manager_config() that run after module import are included. +_STARTUP_SETTINGS_SNAPSHOT: Optional[dict] = None + + +def reset_to_startup_defaults(): + """Restore ``global_settings`` to the state captured at the first call. + + Must be invoked at the top of ``apply_repo_settings()`` so that sections + set by a previously-reviewed repo's ``.pr_agent.toml`` do not leak into + subsequent PRs processed by the same Python process. + """ + global _STARTUP_SETTINGS_SNAPSHOT + if _STARTUP_SETTINGS_SNAPSHOT is None: + _STARTUP_SETTINGS_SNAPSHOT = copy.deepcopy(global_settings.as_dict()) + return # nothing to restore on the very first call + + current_sections = set(global_settings.as_dict().keys()) + snapshot_sections = set(_STARTUP_SETTINGS_SNAPSHOT.keys()) + # Remove sections that appeared after startup (i.e. injected by apply_repo_settings). + for section in current_sections - snapshot_sections: + global_settings.unset(section) + # Restore pre-existing sections to their startup values. + for section, contents in _STARTUP_SETTINGS_SNAPSHOT.items(): + global_settings.unset(section) + global_settings.set(section, copy.deepcopy(contents), merge=False) +# --------------------------------------------------------------------------- + + def apply_secrets_manager_config(): """ Retrieve configuration from AWS Secrets Manager and override existing settings diff --git a/pr_agent/git_providers/utils.py b/pr_agent/git_providers/utils.py index 1e64b9578d..539b6f89e8 100644 --- a/pr_agent/git_providers/utils.py +++ b/pr_agent/git_providers/utils.py @@ -6,13 +6,16 @@ from dynaconf import Dynaconf from starlette_context import context -from pr_agent.config_loader import get_settings +from pr_agent.config_loader import get_settings, reset_to_startup_defaults from pr_agent.git_providers import get_git_provider_with_context from pr_agent.log import get_logger def apply_repo_settings(pr_url): os.environ["AUTO_CAST_FOR_DYNACONF"] = "false" + # Reset first so that .pr_agent.toml from a previously-reviewed repo + # cannot leak into this call via the module-level global_settings singleton. + reset_to_startup_defaults() git_provider = get_git_provider_with_context(pr_url) if get_settings().config.use_repo_settings_file: repo_settings_file = None diff --git a/tests/unittest/test_apply_repo_settings.py b/tests/unittest/test_apply_repo_settings.py new file mode 100644 index 0000000000..4083f1e0f2 --- /dev/null +++ b/tests/unittest/test_apply_repo_settings.py @@ -0,0 +1,111 @@ +import copy + +import pytest + +from pr_agent.config_loader import get_settings, global_settings +from pr_agent.git_providers import utils as git_utils + + +REPO_A_TOML = b""" +[pr_reviewer] +extra_instructions = "MARKER-FROM-REPO-A" + +[pr_code_suggestions] +extra_instructions = "MARKER-FROM-REPO-A" +""" + + +class FakeGitProvider: + """Minimal stand-in used only to feed ``get_repo_settings()`` into + ``apply_repo_settings()``.""" + + def __init__(self, repo_settings: bytes): + self._repo_settings = repo_settings + + def get_repo_settings(self): + return self._repo_settings + + # Touched only by handle_configurations_errors() on invalid TOML. + def is_supported(self, feature): + return False + + def publish_comment(self, body): + pass + + def publish_persistent_comment(self, *args, **kwargs): + pass + + +@pytest.fixture +def fresh_global_settings(): + """Snapshot and restore ``global_settings`` around each test so the + module-level Dynaconf singleton doesn't carry state across tests.""" + snapshot = copy.deepcopy(global_settings.as_dict()) + yield + for section in set(global_settings.as_dict().keys()) - set(snapshot.keys()): + global_settings.unset(section) + for section, contents in snapshot.items(): + global_settings.unset(section) + global_settings.set(section, copy.deepcopy(contents), merge=False) + + +class TestApplyRepoSettings: + def _extra_instructions(self, section: str) -> str: + return get_settings().get(f"{section}.extra_instructions", "") or "" + + def test_repo_settings_from_toml_are_applied( + self, fresh_global_settings, monkeypatch + ): + """Sanity: ``apply_repo_settings()`` loads ``extra_instructions`` from a + repo's ``.pr_agent.toml``.""" + monkeypatch.setattr( + "pr_agent.git_providers.utils.get_git_provider_with_context", + lambda url: FakeGitProvider(REPO_A_TOML), + ) + git_utils.apply_repo_settings( + "https://git.example/projects/A/repos/a/pull-requests/1" + ) + + assert "MARKER-FROM-REPO-A" in self._extra_instructions("pr_reviewer") + assert "MARKER-FROM-REPO-A" in self._extra_instructions("pr_code_suggestions") + + def test_repo_without_toml_does_not_inherit_previous_repo_settings( + self, fresh_global_settings, monkeypatch + ): + """Regression: after ``apply_repo_settings()`` loads a repo with a + ``.pr_agent.toml``, a subsequent call for a repo WITHOUT + ``.pr_agent.toml`` must not still carry the previous repo's + ``extra_instructions``. + + Before the fix this failed — the per-section merge in + ``apply_repo_settings()`` never cleared state from a prior repo, and + when the next repo returned an empty settings blob, the whole load + block was skipped and the prior state persisted. + """ + # 1) first repo loads custom instructions + monkeypatch.setattr( + "pr_agent.git_providers.utils.get_git_provider_with_context", + lambda url: FakeGitProvider(REPO_A_TOML), + ) + git_utils.apply_repo_settings( + "https://git.example/projects/A/repos/a/pull-requests/1" + ) + assert "MARKER-FROM-REPO-A" in self._extra_instructions("pr_reviewer"), ( + "precondition: repo A's marker should be present after its load" + ) + + # 2) second repo has no .pr_agent.toml (get_repo_settings() → b"") + monkeypatch.setattr( + "pr_agent.git_providers.utils.get_git_provider_with_context", + lambda url: FakeGitProvider(b""), + ) + git_utils.apply_repo_settings( + "https://git.example/projects/B/repos/b/pull-requests/1" + ) + + assert "MARKER-FROM-REPO-A" not in self._extra_instructions("pr_reviewer"), ( + "repo A's [pr_reviewer].extra_instructions leaked into repo B" + ) + assert "MARKER-FROM-REPO-A" not in self._extra_instructions( + "pr_code_suggestions" + ), "repo A's [pr_code_suggestions].extra_instructions leaked into repo B"