fix(config): prevent .pr_agent.toml state leak between repositories#2346
Open
tharatynowicz wants to merge 2 commits intoThe-PR-Agent:mainfrom
Open
fix(config): prevent .pr_agent.toml state leak between repositories#2346tharatynowicz wants to merge 2 commits intoThe-PR-Agent:mainfrom
tharatynowicz wants to merge 2 commits intoThe-PR-Agent:mainfrom
Conversation
apply_repo_settings() mutates the module-level global_settings singleton without resetting it first. When a subsequent PR comes from a repo with no .pr_agent.toml (get_repo_settings() returns b""), the loader early- exits on `if repo_settings:` and sections set by a previous repo's .pr_agent.toml persist for the lifetime of the process. Snapshot global_settings on first call to reset_to_startup_defaults() (lazy, so apply_secrets_manager_config() and any other post-import startup work is captured). Restore at the top of apply_repo_settings() before loading per-repo config. Fixes The-PR-Agent#2345
Two tests in tests/unittest/test_apply_repo_settings.py: - test_repo_settings_from_toml_are_applied: sanity — .pr_agent.toml extra_instructions are loaded when present. - test_repo_without_toml_does_not_inherit_previous_repo_settings: 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. Both pass with the fix in the preceding commit. The regression test fails on main without the fix.
Contributor
Review Summary by QodoPrevent .pr_agent.toml state leak between repositories
WalkthroughsDescription• Prevent .pr_agent.toml configuration state leaking between repositories • Add reset_to_startup_defaults() function to restore global settings • Call reset at start of apply_repo_settings() for clean per-repo loads • Add regression tests verifying settings isolation between repos Diagramflowchart LR
A["apply_repo_settings called"] --> B["reset_to_startup_defaults"]
B --> C["Restore global_settings snapshot"]
C --> D["Load repo-specific config"]
D --> E["Clean state for next repo"]
File Changes1. pr_agent/config_loader.py
|
Contributor
Code Review by Qodo
1. Auto flag wiped
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
14
to
19
| 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) |
Contributor
There was a problem hiding this comment.
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
Comment on lines
+102
to
+122
| 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) |
Contributor
There was a problem hiding this comment.
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
Author
|
Thanks for the review. Going through the three findings: 1. "Auto flag wiped" ‚ confirmed bug, introduced by this PR. The flow in 2. "Reset misses context settings" ‚ architecturally valid. In deployments that install a per-request 3. "Non-atomic global reset" ‚ theoretical in the current webhook deployments. In single-worker async servers the event loop serializes; in multi-worker ( Two directions to fix #1 and #2a) Minimal iteration on this PR: b) Larger refactor: push per-request settings isolation through Happy to do (a) on this PR if that's acceptable, or open a separate PR for (b) if you'd prefer to go that route instead. Which direction do you prefer? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2345.
Summary
apply_repo_settings()mutates the module-levelglobal_settingssingleton without resetting it first. When a PR comes from a repo without.pr_agent.toml,get_repo_settings()returnsb"", the loader early-exits onif repo_settings:, and whatever was injected by a previous repo's.pr_agent.tomllingers inglobal_settingsfor the rest of the process. Details, root-cause analysis and a self-contained reproducer are in #2345.Approach
reset_to_startup_defaults()inconfig_loader.py— lazy snapshot on first call (so any post-import startup, e.g.apply_secrets_manager_config(), is captured), full restore on subsequent calls.apply_repo_settings()so every per-repo load starts from a clean state..pr_agent.toml(other than their settings no longer carrying leftovers from a previous repo's load).Alternative I considered was a larger per-request isolation refactor (clone
global_settingsper request viacontext["settings"]set by a FastAPI middleware). Happy to go that route instead if preferred — this PR takes the minimal path.Tests
tests/unittest/test_apply_repo_settings.pywith two tests: a sanity test that.pr_agent.tomlinstructions are applied, and a regression test that a repo without.pr_agent.tomldoes not inherit the previous repo'sextra_instructions.mainwithout this fix and passes with it.main+ this change: 296 passed, 0 failed (the 2 new tests included).Diff: +147 / −1 across 3 files (fix: +36/−1 in 2 files; test: +111/−0 in 1 new file).