Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions pr_agent/config_loader.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import copy
from os.path import abspath, dirname, join
from pathlib import Path
from typing import Optional
Expand Down Expand Up @@ -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)
Comment on lines +102 to +122
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Reset misses context settings 🐞 Bug ≡ Correctness

reset_to_startup_defaults() mutates only global_settings, but get_settings() prefers
context["settings"] when present, so apply_repo_settings() can run against a different settings
object than the one being reset. In deployments that install a per-request settings clone (e.g.,
Bitbucket app), the leak-prevention reset may not affect the effective settings for that request.
Agent Prompt
### Issue description
`reset_to_startup_defaults()` resets `global_settings` only, but `apply_repo_settings()` mutates `get_settings()`, which may be `context["settings"]` (a per-request clone). This can make the reset ineffective for the settings object actually used during request handling.

### Issue Context
`get_settings()` always tries `context["settings"]` first; Bitbucket app installs it as `copy.deepcopy(global_settings)`.

### Fix Focus Areas
- pr_agent/config_loader.py[48-62]
- pr_agent/config_loader.py[102-122]
- pr_agent/git_providers/utils.py[14-20]
- pr_agent/servers/bitbucket_app.py[266-273]

### Suggested fix direction
Change the reset logic to reset the *effective* settings object:
- Make `reset_to_startup_defaults()` operate on `get_settings()` (or accept a Dynaconf instance to reset), and
- Apply the startup snapshot contents into that object (context clone or global), so the reset actually affects what `apply_repo_settings()` will read/modify.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

# ---------------------------------------------------------------------------


def apply_secrets_manager_config():
"""
Retrieve configuration from AWS Secrets Manager and override existing settings
Expand Down
5 changes: 4 additions & 1 deletion pr_agent/git_providers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines 14 to 19
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Auto flag wiped 🐞 Bug ≡ Correctness

apply_repo_settings() now resets settings to a startup snapshot on every call, which clears
config.is_auto_command that webhook servers set before invoking PRAgent.handle_request().
Auto-command flows can then behave like manual flows (e.g., publishing temporary/progress comments)
because the flag is read later by tools but is removed by the second apply_repo_settings() call.
Agent Prompt
### Issue description
`reset_to_startup_defaults()` is invoked unconditionally at the top of `apply_repo_settings()`. In auto-command flows, servers set `config.is_auto_command=True` and then call `PRAgent.handle_request()`, but `PRAgent._handle_request()` calls `apply_repo_settings()` again first. The second call resets settings back to the startup snapshot and clears `config.is_auto_command`, changing downstream behavior.

### Issue Context
This is a regression introduced by adding the reset call; prior behavior relied on `is_auto_command` surviving into tool execution.

### Fix Focus Areas
- pr_agent/git_providers/utils.py[14-20]
- pr_agent/agent/pr_agent.py[54-57]
- pr_agent/servers/github_app.py[395-416]
- pr_agent/servers/gitlab_webhook.py[39-59]

### Suggested fix direction
Implement a mechanism so that the second `apply_repo_settings()` call in the same logical request does **not** clear runtime flags:
- Either avoid calling `apply_repo_settings()` twice (e.g., guard in `PRAgent._handle_request()` when settings were already applied), or
- Preserve and restore key runtime flags (at least `config.is_auto_command`, and any similar request-scoped flags) across `reset_to_startup_defaults()`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

if get_settings().config.use_repo_settings_file:
repo_settings_file = None
Expand Down
111 changes: 111 additions & 0 deletions tests/unittest/test_apply_repo_settings.py
Original file line number Diff line number Diff line change
@@ -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"