feat(improve): load repo best_practices.md in OSS (#2377)#2382
Open
agonzalesipcoop-cmyk wants to merge 2 commits into
Open
feat(improve): load repo best_practices.md in OSS (#2377)#2382agonzalesipcoop-cmyk wants to merge 2 commits into
agonzalesipcoop-cmyk wants to merge 2 commits into
Conversation
Closes The-PR-Agent#2377. OSS pr-agent silently ignored best_practices.md even though the public docs at https://qodo-merge-docs.qodo.ai/tools/improve/#best-practices advertise it as a supported feature. The `relevant_best_practices` template variable in `pr_code_suggestions.py` was declared but never populated, and the OSS prompt did not consume it. This commit fetches `best_practices.md` from the repo's default branch on every `improve` run and feeds it into the code-suggestions prompt as a labeled block. Behavior: - New `[best_practices]` keys: `enable_repo_best_practices_md` (default `true`, matching the docs) and `repo_best_practices_md_path` (default `"best_practices.md"`). - New `GitProvider.get_pr_agent_repo_custom_file(file_path)` base method returning `b""`, with implementations on GitHub, Azure DevOps, GitLab, Bitbucket, and Local providers (mirrors each provider's existing `get_repo_settings` fetch logic, parameterized by file path). - `_load_repo_best_practices_md` helper in `pr_code_suggestions`: - Caches via `starlette_context.context["best_practices_md"]` to avoid repeat fetches in webhook flows. - Decodes bytes/str tolerantly. - Truncates to `[best_practices].max_lines_allowed` (default 800). - Emits an INFO log on fetch (bytes + line count) and a WARNING on truncation. - Code-suggestions prompts (decoupled and not-decoupled variants) get a parallel `{%- if relevant_best_practices %}` block above the existing `extra_instructions` slot. - Reviewer is intentionally untouched; this PR is scoped to `improve`. Tests: tests/unittest/test_pr_code_suggestions_best_practices.py (6 cases) covers default-on path, opt-out, missing file, truncation + WARNING, caching, and bytes/str input. Docs: improve.md "Best practices" section now lists Azure DevOps as supported and documents the OSS toggle.
Contributor
Review Summary by QodoLoad repository best_practices.md in OSS improve tool with provider support
WalkthroughsDescription• Load best_practices.md from repo default branch in OSS improve tool • Add get_pr_agent_repo_custom_file() method to all Git providers • Implement caching and truncation for best practices content • Integrate best practices into code-suggestions prompts with labeled block • Add comprehensive unit tests and documentation updates Diagramflowchart LR
A["Git Providers"] -->|"get_pr_agent_repo_custom_file()"| B["Fetch best_practices.md"]
B -->|"bytes/str decode"| C["_load_repo_best_practices_md()"]
C -->|"truncate + cache"| D["Context Storage"]
D -->|"relevant_best_practices"| E["Code Suggestions Prompts"]
E -->|"labeled block"| F["AI Model Input"]
File Changes1. pr_agent/git_providers/git_provider.py
|
Contributor
Code Review by Qodo
Context used 1. load_repo_best_practices_md line too long
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…view - Extract loader into pr_agent/algo/best_practices.py and reuse it from /improve and /review so OSS behavior is symmetric (closes Qodo The-PR-Agent#1). - Guard int(max_lines_allowed) with TypeError/ValueError fallback to 800 plus a targeted warning (Qodo The-PR-Agent#2). - Bitbucket get_pr_agent_repo_custom_file: only accept HTTP 200, log non-404 failures, add 10s timeout, and use the repo default branch via get_repo_default_branch (Qodo The-PR-Agent#3, The-PR-Agent#5, The-PR-Agent#6). - LocalGitProvider get_pr_agent_repo_custom_file: resolve and verify the candidate path stays inside the repo root before reading (Qodo The-PR-Agent#7). - Soften prompt wording in code-suggestions templates and add a parallel block in pr_reviewer_prompts.toml so the model treats the section as guidance, ignoring obvious non-guideline content. - Drop unused _FakeContext test helper, retarget patches to the new module, add coverage for the invalid max_lines fallback (Qodo The-PR-Agent#4). - Update docs/improve.md and docs/review.md to reflect that both tools now consume best_practices.md in OSS. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Persistent review updated to latest commit 4fb8cad |
| cached = None | ||
| if cached is not None: | ||
| return cached | ||
| file_path = settings.get("best_practices.repo_best_practices_md_path", "best_practices.md") or "best_practices.md" |
Contributor
There was a problem hiding this comment.
1. load_repo_best_practices_md line too long 📘 Rule violation ⚙ Maintainability
The new file_path = ... assignment exceeds the repository’s 120-character Python line-length limit, which will fail Ruff (E501) and violates the repo style requirements.
Agent Prompt
## Issue description
`pr_agent/algo/best_practices.py` contains a new line exceeding the configured 120-character limit, likely triggering Ruff `E501`.
## Issue Context
Ruff is configured with `line-length = 120` in `pyproject.toml`.
## Fix Focus Areas
- pr_agent/algo/best_practices.py[24-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+187
to
+188
| chunks = [c if isinstance(c, (bytes, bytearray)) else str(c).encode("utf-8") for c in contents] | ||
| return b"".join(chunks) |
Contributor
There was a problem hiding this comment.
2. Azure chunks comprehension too long 📘 Rule violation ⚙ Maintainability
The added list comprehension for chunks is on a single line that exceeds the repository’s 120-character limit, risking Ruff (E501) failures and reducing readability.
Agent Prompt
## Issue description
A newly-added list comprehension exceeds Ruff's configured `line-length = 120`.
## Issue Context
The repository uses Ruff with `line-length = 120`.
## Fix Focus Areas
- pr_agent/git_providers/azuredevops_provider.py[187-188]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+158
| import unittest | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from pr_agent.algo.best_practices import load_repo_best_practices_md | ||
|
|
||
|
|
||
| def _provider(returns): | ||
| p = MagicMock(spec=["get_pr_agent_repo_custom_file"]) | ||
| p.get_pr_agent_repo_custom_file.return_value = returns | ||
| return p | ||
|
|
||
|
|
||
| class _FakeContextProxy: | ||
| """Module-level proxy that works as both subscriptable and attribute target.""" | ||
|
|
||
| def __init__(self): | ||
| self._store = {} | ||
|
|
||
| def get(self, key, default=None): | ||
| return self._store.get(key, default) | ||
|
|
||
| def __getitem__(self, key): | ||
| return self._store[key] | ||
|
|
||
| def __setitem__(self, key, value): | ||
| self._store[key] = value | ||
|
|
||
| def reset(self): | ||
| self._store.clear() | ||
|
|
||
|
|
||
| class TestLoadRepoBestPracticesMd(unittest.TestCase): | ||
| def setUp(self): | ||
| self.fake_ctx = _FakeContextProxy() | ||
| self.ctx_patch = patch( | ||
| "pr_agent.algo.best_practices.context", self.fake_ctx | ||
| ) | ||
| self.ctx_patch.start() | ||
|
|
||
| def tearDown(self): | ||
| self.ctx_patch.stop() | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_enabled_by_default_with_content(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"# Best practices\n- rule 1\n- rule 2\n") | ||
| out = load_repo_best_practices_md(prov) | ||
| self.assertIn("rule 1", out) | ||
| self.assertIn("rule 2", out) | ||
| prov.get_pr_agent_repo_custom_file.assert_called_once_with("best_practices.md") | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_opt_out_skips_fetch(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": False, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"should not be read") | ||
| out = load_repo_best_practices_md(prov) | ||
| self.assertEqual(out, "") | ||
| prov.get_pr_agent_repo_custom_file.assert_not_called() | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_file_absent_returns_empty(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"") | ||
| out = load_repo_best_practices_md(prov) | ||
| self.assertEqual(out, "") | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_logger") | ||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_truncation_emits_warning(self, mock_get_settings, mock_get_logger): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 5, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| logger = MagicMock() | ||
| mock_get_logger.return_value = logger | ||
| body = "\n".join(f"line {i}" for i in range(20)) | ||
| prov = _provider(body.encode("utf-8")) | ||
| out = load_repo_best_practices_md(prov) | ||
| self.assertEqual(len(out.splitlines()), 5) | ||
| # WARNING message about truncation must include the from/to counts. | ||
| warning_msgs = [c.args[0] for c in logger.warning.call_args_list] | ||
| self.assertTrue(any("Truncating" in m and "20" in m and "5" in m for m in warning_msgs), | ||
| f"warning not emitted: {warning_msgs}") | ||
| # INFO log emitted on fetch. | ||
| info_msgs = [c.args[0] for c in logger.info.call_args_list] | ||
| self.assertTrue(any("Loaded" in m for m in info_msgs)) | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_caches_across_calls(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider(b"hello\n") | ||
| first = load_repo_best_practices_md(prov) | ||
| second = load_repo_best_practices_md(prov) | ||
| self.assertEqual(first, second) | ||
| prov.get_pr_agent_repo_custom_file.assert_called_once() | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_str_return_tolerated(self, mock_get_settings): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": 800, | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| prov = _provider("text content\n") | ||
| out = load_repo_best_practices_md(prov) | ||
| self.assertIn("text content", out) | ||
|
|
||
| @patch("pr_agent.algo.best_practices.get_logger") | ||
| @patch("pr_agent.algo.best_practices.get_settings") | ||
| def test_invalid_max_lines_falls_back(self, mock_get_settings, mock_get_logger): | ||
| s = MagicMock() | ||
| s.get.side_effect = lambda key, default=None: { | ||
| "best_practices.enable_repo_best_practices_md": True, | ||
| "best_practices.repo_best_practices_md_path": "best_practices.md", | ||
| "best_practices.max_lines_allowed": "not-a-number", | ||
| }.get(key, default) | ||
| mock_get_settings.return_value = s | ||
| logger = MagicMock() | ||
| mock_get_logger.return_value = logger | ||
| prov = _provider(b"line\n") | ||
| out = load_repo_best_practices_md(prov) | ||
| self.assertEqual(out, "line") | ||
| warning_msgs = [c.args[0] for c in logger.warning.call_args_list] | ||
| self.assertTrue( | ||
| any("Invalid best_practices.max_lines_allowed" in m for m in warning_msgs), | ||
| f"fallback warning not emitted: {warning_msgs}", | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
Contributor
There was a problem hiding this comment.
3. testloadrepobestpracticesmd uses unittest 📘 Rule violation ⚙ Maintainability
The new test module is written using unittest.TestCase and unittest.main(), while the repository’s test suite is configured for pytest conventions. This can lead to inconsistent test patterns and maintenance overhead.
Agent Prompt
## Issue description
The new unit tests are implemented with `unittest` (`unittest.TestCase`, `unittest.main()`), but the compliance requirement and repository conventions expect pytest-style tests.
## Issue Context
The repository is configured for pytest test discovery/execution, and other unit tests in `tests/unittest/` follow pytest patterns.
## Fix Focus Areas
- tests/unittest/test_pr_code_suggestions_best_practices.py[1-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
4 tasks
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.
Summary
Closes #2377.
OSS `pr-agent` silently ignored `best_practices.md` even though the public docs at https://qodo-merge-docs.qodo.ai/tools/improve/#best-practices advertise it as a supported feature. The `relevant_best_practices` template variable in `pr_code_suggestions.py` was declared but never populated, and the OSS prompt did not consume it.
This PR fetches `best_practices.md` from the repo's default branch on every `improve` run and feeds it into the code-suggestions prompt as a labeled block.
Behavior
Behavior change on upgrade
`enable_repo_best_practices_md` defaults to `true`, so users with an existing `best_practices.md` at repo root will see it picked up automatically on next `improve` run. This is the explicit fix the issue asks for. Opt out via:
```toml
[best_practices]
enable_repo_best_practices_md = false
```
Test plan
🤖 Generated with Claude Code