Repository navigation
fix: validate review model responses - #3372
Conversation
PR Summary by QodoValidate AI review responses against typed output contracts
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Partial review issues disappear
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit ee731e3 |
|
Code review by qodo was updated up to the latest commit 673a5bd |
673a5bd to
54a43f2
Compare
|
Code review by qodo was updated up to the latest commit 54a43f2 |
54a43f2 to
78d175d
Compare
|
Code review by qodo was updated up to the latest commit 78d175d |
78d175d to
17004c2
Compare
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
17004c2 to
a6d6ffb
Compare
|
Code review by qodo was updated up to the latest commit a6d6ffb |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for landing this within the hour of the issue and for working every Qodo thread into the branch. The helper holds: one warning per response, nothing raised, data untouched, markdown byte-identical, chunked path covered.
Two things before merge. It inherits #3371's bare No example, which parses as a boolean and would warn on every review, so that fix lands first. And #3359 rewrites the same chunked-loop lines with the earlier number, so it lands first too and this rebases onto its _load_valid_review_yaml, the single parse site where the warn-only call belongs. When rebasing, keep the original chunk warning so the chunk index stays in the log.
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
| {%- endif %} | ||
| relevant_tests: | | ||
| No | ||
| relevant_tests: No |
There was a problem hiding this comment.
1. Default test reviews log false warnings 🐞 Bug ≡ Correctness
The primary example now emits relevant_tests: No as an unquoted YAML scalar, which load_yaml() parses with yaml.safe_load() as the boolean False rather than the required `Literal["Yes", "No"]` string. Because test review is enabled by default, a model following this displayed example reaches _validate_review_schema() and produces the malformed-output warning that valid prompt-conforming responses are meant to avoid.
Agent Prompt
## Issue description
The primary `/review` prompt example renders `relevant_tests: No` unquoted. PyYAML resolves this YAML 1.1 boolean spelling to `False`, but `Review.relevant_tests` accepts only the strings `"Yes"` and `"No"`, causing a false schema-validation warning for output following the example.
## Fix Focus Areas
- pr_agent/settings/pr_reviewer_prompts.toml[172-172]
## Recommended Fix
Quote the example value as `relevant_tests: "No"`, or use the existing block-scalar form, so parsing preserves a string that satisfies the output model.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d79d7b0 |
d79d7b0 to
b4c1d4e
Compare
|
Code review by qodo was updated up to the latest commit b4c1d4e |
b4c1d4e to
a6d6ffb
Compare
|
Code review by qodo was updated up to the latest commit a6d6ffb |
|
Code review by qodo was updated up to the latest commit db8e89c |
|
Code review by qodo was updated up to the latest commit a3ecad4 |
|
Code review by qodo was updated up to the latest commit a6a6194 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 919b678 |
| def _get_previous_review_comment(self): | ||
| """ | ||
| Get the previous review comment if it exists. | ||
| """ |
There was a problem hiding this comment.
1. Two review comment helpers are unused 📘 Rule violation ⚙ Maintainability
The newly added _get_previous_review_comment and _remove_previous_review_comment methods have no callers anywhere in the repository. Because no review path references either helper, their provider calls and exception handling form a second, inactive comment-management surface that later changes could mistakenly maintain.
Agent Prompt
## Issue description
The newly added previous-review comment helpers are not called anywhere and therefore constitute dead code.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[1238-1260]
## Recommended Fix
Remove `_get_previous_review_comment` and `_remove_previous_review_comment`. If they were intended to replace existing inline behavior, wire them into that behavior and add tests proving both helpers are used instead of retaining inactive methods.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 1542444 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for the rebase attempt and the partial-run state fix.
This head must not merge as it stands: the pr_reviewer.py conflict was resolved by keeping the pre-#3359 file, so the merged tree reverts #3359's fallback retry, #3361's is_supported guard, #3291's PreparedPRDiff reuse and #3371's relevant_tests block scalar, and build-and-test is red for exactly that reason. 1542444 then re-adds parts of main by hand, which a clean rebase makes unnecessary.
Recovery: git reset --hard a6a6194f, squash, then git rebase --onto upstream/main 6aa533db. Two hunks conflict. In the chunk loop keep main's data = self._load_valid_review_yaml(prediction, source=f"review chunk {chunk_index + 1}") and add self._validate_review_schema(data) under it. In test_a_malformed_chunk_fails_the_model_attempt keep main's assert reviewer.prediction_data is None. That shape passed the full suite here, and I will approve on it.
1542444 to
3967890
Compare
|
Code review by qodo was updated up to the latest commit 3967890 |
|
Code review by qodo was updated up to the latest commit 908f7d2 |
|
Code review by qodo was updated up to the latest commit ee99712 |
|
@IsmaelMartinez Done. All requested fixes are applied, the branch is rebased cleanly, and the focused tests are passing. |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for following the reset and rebase to the letter, and for folding Qodo's two renderer threads into the same push. Approving.
The merged tree touches only your five files with nothing from main reverted, the chunk loop carries main's parse plus the schema check, and the markdown is byte-identical with or without the new guard.
The two open Qodo threads on utils.py are answered by the last two commits; I will resolve them on merge.
Summary
Fixes #3355.
Validate parsed
/reviewresponses with thePRReviewoutput model before rendering and while processing review chunks. Invalid responses emit one structured warning containing the first field path and value, while the existing rendering flow and markdown remain unchanged.Unknown review fields are rejected by the output model so misspelled keys are reported. Added tests cover invalid and valid responses and verify unchanged markdown output.
Validation
PYTHONPATH=. uv run pytest tests/unittest/test_output_models.py tests/unittest/test_pr_reviewer_core.py tests/unittest/test_review_large_diff_chunking.py tests/unittest/test_pr_reviewer_prompt_contract.py -q --color=nouv run pre-commit run --files pr_agent/algo/output_models.py pr_agent/tools/pr_reviewer.py tests/unittest/test_pr_reviewer_core.py