Add review risk assessment and optional repository review rules#2391
Open
xiaodeng666a wants to merge 17 commits into
Open
Add review risk assessment and optional repository review rules#2391xiaodeng666a wants to merge 17 commits into
xiaodeng666a wants to merge 17 commits into
Conversation
Contributor
Review Summary by QodoAdd review risk assessment, merge recommendations, and repository review rules
WalkthroughsDescription• Add risk assessment and merge recommendation fields to PR reviews • Implement optional repository-specific review rules injection • Add review history tracking and retrieval from JSONL file • Enhance markdown rendering for new review fields Diagramflowchart LR
A["PR Review Process"] --> B["Load Review Rules"]
A --> C["Load Review History"]
B --> D["Inject into Prompt"]
C --> D
D --> E["Generate Review"]
E --> F["Extract Risk Level"]
E --> G["Extract Merge Recommendation"]
E --> H["Extract Priority Files"]
F --> I["Store in Memory"]
G --> I
H --> I
I --> J["Render Markdown"]
File Changes1. pr_agent/algo/utils.py
|
Contributor
Code Review by Qodo
1. Review rules lack tests
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Contributor
|
Persistent review updated to latest commit 104a696 |
Contributor
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again |
Contributor
|
Persistent review updated to latest commit fc8ee1d |
Contributor
|
Persistent review updated to latest commit d8cbb89 |
Contributor
|
Persistent review updated to latest commit e237f87 |
Contributor
|
Persistent review updated to latest commit 13136c6 |
Contributor
|
Persistent review updated to latest commit 03e0a0d |
Contributor
|
Persistent review updated to latest commit 2d099fa |
Comment on lines
+255
to
+262
| elif "Review priority files" in key_nice.lower(): | ||
| if not value: | ||
| markdown_text += f"### {emoji} Priority files: None\n\n" | ||
| else: | ||
| markdown_text += f"### {emoji} Priority files\n\n" | ||
| for priority_file in value: | ||
| markdown_text += f"- {priority_file}\n" | ||
| markdown_text += "\n" |
Contributor
There was a problem hiding this comment.
1. Priority files condition unreachable 📘 Rule violation ≡ Correctness
The new convert_to_markdown_v2() branch intended to render review_priority_files is effectively dead code because it checks for a capitalized/mixed-case substring against key_nice.lower(), so it can never match. As a result, review_priority_files falls through to the generic renderer instead of being formatted as the intended priority-files list/output.
Agent Prompt
## Issue description
The `review_priority_files` rendering branch in `convert_to_markdown_v2()` is unreachable because it performs a case-sensitive substring search using a mixed/capitalized literal (e.g., `"Review priority files"`) against `key_nice.lower()`, which is always lowercase.
## Issue Context
`key_nice` is transformed and then compared via `key_nice.lower()`. With the current mixed-case substring in the condition, the match can never succeed, so the new priority-files rendering path never runs and `review_priority_files` falls through to the generic renderer instead of being formatted as the intended priority-files list/output.
## Fix Focus Areas
- pr_agent/algo/utils.py[175-180]
- pr_agent/algo/utils.py[255-263]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4
to
7
|
|
||
|
|
||
|
|
||
| from collections import OrderedDict |
Contributor
There was a problem hiding this comment.
2. Extra blank lines in imports 📘 Rule violation ⚙ Maintainability
The PR introduces three consecutive blank lines within the import block, which is likely to fail Ruff/PEP8 blank-line rules and create unnecessary diff noise. This can break linting CI and violates the repository formatting conventions.
Agent Prompt
## Issue description
There are too many consecutive blank lines inside the imports section, which can violate Ruff/PEP8 formatting rules.
## Issue Context
The file currently has three blank lines between `import traceback` and the next import group.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[4-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+241
to
+253
| elif "risk level" in key_nice.lower(): | ||
| risk_value = str(value).strip().lower().replace("_", " ") | ||
| risk_display = risk_value.capitalize() if risk_value else "Unknown" | ||
| markdown_text += f"### {emoji} Risk level: {risk_display}\n\n" | ||
|
|
||
| elif "merge recommendation" in key_nice.lower(): | ||
| recommendation = str(value).strip().replace("_", " ") | ||
| recommendation_display = ( | ||
| recommendation.capitalize() if recommendation else "Unknown" | ||
| ) | ||
| markdown_text += ( | ||
| f"### {emoji} Merge recommendation: {recommendation_display}\n\n" | ||
| ) |
Contributor
There was a problem hiding this comment.
3. Headings break gfm table 🐞 Bug ≡ Correctness
When gfm_supported is true, convert_to_markdown_v2() opens an HTML <table> but the new risk_level and merge_recommendation branches emit Markdown headings (### ...) rather than <tr><td>...</td></tr>. This produces malformed mixed HTML/Markdown output and can render the review comment unpredictably.
Agent Prompt
## Issue description
`convert_to_markdown_v2()` writes markdown headings for `risk_level` / `merge_recommendation` even while a `<table>` is open in GFM mode, breaking the table structure.
## Issue Context
The function opens `<table>` when `gfm_supported` is true, and most branches emit `<tr><td>...</td></tr>`. The new branches should follow the same pattern (or close the table before emitting headings).
## Fix Focus Areas
- pr_agent/algo/utils.py[171-173]
- pr_agent/algo/utils.py[241-253]
- pr_agent/algo/utils.py[347-349]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 71011b3 |
Contributor
|
Persistent review updated to latest commit 666a265 |
Comment on lines
234
to
239
| if get_settings().pr_reviewer.persistent_comment and not self.incremental.is_incremental: | ||
| final_update_message = get_settings().pr_reviewer.final_update_message | ||
| self.git_provider.publish_persistent_comment(pr_review, | ||
| initial_header=f"{PRReviewHeader.REGULAR.value} 🔍", | ||
| initial_header=f"{PRReviewHeader.REGULAR.value}", | ||
| update_header=True, | ||
| final_update_message=final_update_message, ) |
Contributor
There was a problem hiding this comment.
1. Persistent header string mismatch 🐞 Bug ≡ Correctness
PRReviewer now uses an initial_header without the "🔍" suffix while convert_to_markdown_v2() still emits a header that includes it. In persistent-comment mode this causes publish_persistent_comment_full() to replace only the prefix and leave a stray "🔍" line in the updated comment body.
Agent Prompt
## Issue description
Persistent comment updates rely on exact string matching/replacement of the review header. The PR changed `initial_header` passed to `publish_persistent_comment(...)` to omit the "🔍" suffix, but the generated markdown header still includes it, causing a dangling emoji line after header update.
## Issue Context
- `convert_to_markdown_v2()` emits a header with "🔍".
- `publish_persistent_comment_full()` updates an existing comment by `startswith(initial_header)` and then doing `pr_comment.replace(initial_header, updated_header)`.
- If `initial_header` is a prefix of the generated header (missing the emoji), the replacement leaves the emoji behind.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[233-240]
- pr_agent/algo/utils.py[159-166]
- pr_agent/git_providers/git_provider.py[301-316]
## Suggested fix
Choose one of:
1) Restore `initial_header` to exactly match the generated header (include " 🔍"), or
2) Remove the emoji from the generated header in `convert_to_markdown_v2()` and keep `initial_header` emoji-free, or
3) Centralize header construction (single helper/constant) so both generation and persistent-update use the exact same string.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 8c93ceb |
Contributor
|
Persistent review updated to latest commit 15d8940 |
Contributor
|
Persistent review updated to latest commit 20a256d |
Contributor
|
Persistent review updated to latest commit b71e7be |
| @@ -1,6 +1,9 @@ | |||
| import copy | |||
| import copy | |||
Contributor
There was a problem hiding this comment.
1. Bom character in import copy 📘 Rule violation ⚙ Maintainability
pr_reviewer.py now begins with a hidden UTF-8 BOM/zero-width character before import copy, which can cause linting/pre-commit failures, inconsistent behavior across linters/formatters, and confusing diffs across editors/tools. This violates repository hygiene expectations for clean, stable text formatting in changed files.
Agent Prompt
## Issue description
`pr_agent/tools/pr_reviewer.py` has a hidden UTF-8 BOM/zero-width character (U+FEFF) at the very start of the file (before `import copy`). This non-printable character can break tooling, create inconsistent linter/formatter behavior, and violate pre-commit hygiene expectations.
## Issue Context
The diff shows the first line as `+import copy` (note the hidden character before `import`). This is typically introduced by an editor/encoding change and should be removed so the file starts directly with the import statement.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[1-1]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit b97e833 |
Comment on lines
+84
to
+89
| require_risk_assessment=false | ||
| require_merge_recommendation=false | ||
| require_priority_files=false | ||
| enable_review_rules=true | ||
| review_rules_paths=[".pr_agent/review_rules.md",".github/review_rules.md","docs/review_rules.md"] | ||
| max_review_rules_tokens=1200 |
Contributor
There was a problem hiding this comment.
1. .pr_agent.toml missing new keys 📘 Rule violation ⚙ Maintainability
New /review configuration knobs were added to pr_agent/settings/configuration.toml, but the mirrored root .pr_agent.toml was not updated to include these options, risking divergence and user confusion about available/default behavior. This violates the mirror-sync requirement when behavior/config surface area changes.
Agent Prompt
## Issue description
New `pr_reviewer` settings were added in `pr_agent/settings/configuration.toml` but are not reflected in the root `.pr_agent.toml` mirror, making mirrored configuration sources inconsistent.
## Issue Context
The repo uses both a packaged default config (`pr_agent/settings/*.toml`) and a root `.pr_agent.toml` as a mirror/example for behavior changes.
## Fix Focus Areas
- pr_agent/settings/configuration.toml[84-89]
- .pr_agent.toml[1-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Persistent review updated to latest commit 983d227 |
Comment on lines
+128
to
+183
| def _get_review_rules(self) -> str: | ||
| if not get_settings().pr_reviewer.get("enable_review_rules", False): | ||
| return "" | ||
|
|
||
| rule_paths = get_settings().pr_reviewer.get("review_rules_paths", []) or [] | ||
| if isinstance(rule_paths, str): | ||
| rule_paths = [rule_paths] | ||
| elif isinstance(rule_paths, (list, tuple)): | ||
| rule_paths = [ | ||
| str(rule_path).strip() | ||
| for rule_path in rule_paths | ||
| if str(rule_path).strip() | ||
| ] | ||
| else: | ||
| get_logger().warning( | ||
| "Invalid review_rules_paths value; expected string or list of strings", | ||
| artifacts={"review_rules_paths": rule_paths}, | ||
| ) | ||
| rule_paths = [] | ||
|
|
||
| ref = self.git_provider.get_pr_base_ref() | ||
| if not ref: | ||
| get_logger().warning("Could not resolve a trusted base ref for review rules") | ||
| return "" | ||
| loaded_rules = [] | ||
| loaded_rule_paths = [] | ||
|
|
||
| for rule_path in rule_paths: | ||
| try: | ||
| rule_content = self.git_provider.get_pr_file_content(rule_path, ref) | ||
| except Exception: | ||
| continue | ||
|
|
||
| if rule_content and rule_content.strip(): | ||
| loaded_rule_paths.append(rule_path) | ||
| loaded_rules.append(f"File: `{rule_path}`\n{rule_content.strip()}") | ||
|
|
||
| if not loaded_rules: | ||
| get_logger().info("No review rules file found for this PR") | ||
| return "" | ||
|
|
||
| review_rules = "\n\n---\n\n".join(loaded_rules) | ||
| max_tokens = get_settings().pr_reviewer.get("max_review_rules_tokens", 0) | ||
| try: | ||
| max_tokens = int(max_tokens) | ||
| except (TypeError, ValueError): | ||
| get_logger().warning( | ||
| "Invalid max_review_rules_tokens value; skipping token clipping", | ||
| artifacts={"max_review_rules_tokens": max_tokens}, | ||
| ) | ||
| max_tokens = 0 | ||
|
|
||
| if max_tokens > 0: | ||
| review_rules = clip_tokens(review_rules, max_tokens) | ||
| get_logger().info("Loaded review rules for this PR", artifacts={"rule_files": loaded_rule_paths}) | ||
| return review_rules |
Contributor
There was a problem hiding this comment.
1. Review rules lack tests 📘 Rule violation ☼ Reliability
This PR adds new /review behavior (repo review rules injection + new review fields rendering) but does not add/update pytest coverage for these new outputs, increasing regression risk and making changes harder to validate.
Agent Prompt
## Issue description
New review features were added (review-rules loading/injection and new `risk_level`/`merge_recommendation`/`review_priority_files` formatting), but there are no accompanying pytest updates covering the new behavior.
## Issue Context
The repository already has unit tests for markdown conversion, but none exercise the newly added keys or the review-rules loading logic; this can allow prompt/output regressions to slip through CI.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[128-183]
- pr_agent/algo/utils.py[241-270]
- pr_agent/settings/pr_reviewer_prompts.toml[71-80]
- pr_agent/settings/pr_reviewer_prompts.toml[122-137]
- tests/unittest/test_convert_to_markdown.py[48-125]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
This PR introduces two related review enhancements for PR-Agent:
risk_levelmerge_recommendationreview_priority_filesMotivation
These changes make review output more actionable and more adaptable to repository-specific expectations, while keeping the default behavior backward-compatible.
Notes