fix: retry only failed chunks in large PR reviews - #3402
Merged
IsmaelMartinez merged 14 commits intoSep 16, 2026
Merged
Conversation
Contributor
PR Summary by QodoRetry only failed chunks in large PR reviews
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Contributor
Code Review by Qodo
1. Reserved output makes retries overflow
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+810
to
+812
| patches_diff_list = getattr(self, "_chunked_patches_diff_list", None) | ||
| remaining_files_list = getattr(self, "_chunked_remaining_files_list", None) | ||
| if patches_diff_list is None: |
Contributor
There was a problem hiding this comment.
1. Smaller fallback models reject chunks 🐞 Bug ☼ Reliability
_prepare_chunked_prediction reuses _chunked_patches_diff_list without checking whether its chunks fit the current fallback model's tokenizer and context limit. When the primary model creates a chunk larger than a fallback accepts, the pending request exceeds that fallback's budget and can exhaust the fallback chain even though regenerating its chunks would allow the review to complete.
Agent Prompt
## Issue description
Cached chunks are generated using the primary model's token handler and context limit, then reused for fallback models without compatibility validation. A smaller fallback can therefore receive an oversized pending chunk and fail unnecessarily.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[810-856]
- pr_agent/algo/pr_processing.py[215-282]
- tests/unittest/test_review_large_diff_chunking.py[191-262]
## Recommended Fix
Associate the cached chunk plan with the model and token-budget assumptions used to create it. Before reusing it for a fallback, verify every pending chunk against the fallback model's token handler and maximum-token limit; regenerate or split incompatible pending chunks while preserving successful results and their merge order. Add a regression test where the fallback has a smaller context limit than the primary model.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
utsab345
force-pushed
the
fix/3397-retry-failed-chunks
branch
from
September 15, 2026 08:05
20bfcad to
d3f2d00
Compare
Contributor
|
Code review by qodo was updated up to the latest commit d3f2d00 |
Contributor
|
Code review by qodo was updated up to the latest commit d78bfd2 |
Contributor
|
Code review by qodo was updated up to the latest commit 0263ccf |
Contributor
|
Code review by qodo was updated up to the latest commit 0d959cf |
Contributor
|
Code review by qodo was updated up to the latest commit 01eb781 |
Contributor
|
Code review by qodo was updated up to the latest commit f761140 |
Contributor
|
Code review by qodo was updated up to the latest commit 95e1e00 |
|
Preparing review... |
|
Failed to review PR |
1 similar comment
|
Failed to review PR |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for turning the requested shape round within the hour of the decline.
Not yet. The retry is right: a valid chunk is never resent to the fallback. But the terminal state moved with it. On main a chunk that fails with no model left still publishes the rest with the failed count in the footer; here the review fails, review_failed_chunk_count is only ever 0, and the #3229 finding-state guard is dead. Keep the raise while a later model can retry, and on the last one publish what succeeded with the failed count; the simplest place is run() after the chain is exhausted, merging the cache. A test pinning the partial result through retry_with_fallback_models goes with it.
Qodo's open point stands: main re-chunks per model, this reuses the primary's plan. With the partial path back that costs a chunk, not the review.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+798
to
+799
| has_incomplete_chunk_plan = hasattr(self, "_chunked_patches_diff_list") | ||
| if chunking_enabled and (self.remaining_files_list or has_incomplete_chunk_plan): |
Contributor
There was a problem hiding this comment.
1. Larger fallbacks skip reviewable files 🐞 Bug ≡ Correctness
_prepare_prediction enters the cached plan whenever _chunked_patches_diff_list exists, so _prepare_chunked_prediction ignores a larger fallback's newly prepared full diff and retries only indices from the primary plan. When the primary plan exhausted max_number_of_calls and left files in _chunked_remaining_files_list, _merge_cached_review_chunks restores that old omission list even though the fallback could have reviewed those files.
Agent Prompt
## Issue description
A larger-context fallback can fit files omitted by the primary chunk plan, but the cached-plan path ignores the freshly prepared fallback diff and permanently preserves the primary omission list.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[786-889]
- pr_agent/algo/pr_processing.py[215-282]
- tests/unittest/test_review_large_diff_chunking.py[341-357]
## Recommended Fix
Retain successful cached chunk results, but when the current fallback can cover files in the cached remaining-files list, prepare additional work containing only those omitted files. Append that work to the pending chunk plan, update the cached remaining-files list from the fallback result, and test that already successful chunks are not requested again while newly coverable files are reviewed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 4ed3c3b |
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Contributor
|
Code review by qodo was updated up to the latest commit fce4e9f |
Contributor
|
Code review by qodo was updated up to the latest commit c3c17c1 |
Contributor
|
Code review by qodo was updated up to the latest commit 639d554 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for bringing the partial path back through run() and pinning it on the real fallback chain; only the failed chunk is resent now, exhaustion publishes what succeeded with the failed count, and cached results keep their chunk when a smaller fallback splits pending work.
One thing, from the last commit. A chunk that parses but fails the schema is now retried and, on the last model, dropped. #3372 set that check to warn-only yesterday and the single-call path still only warns, so a chunk answering relevant_tests: no loses its findings after a wasted fallback call, where main merges them. The inlines restore the warn-only call and main's test.
Comment on lines
+873
to
+882
| try: | ||
| data = self._load_valid_review_yaml(prediction, source=f"review chunk {chunk_index + 1}") | ||
| if not self._validate_review_schema(data): | ||
| raise ValueError(f"review chunk {chunk_index + 1} failed schema validation") | ||
| except Exception as error: | ||
| chunk_errors.append(error) | ||
| get_logger().warning(f"Failed to parse review chunk {chunk_index + 1}; retrying it with fallback", | ||
| artifact={"error": error}) | ||
| continue | ||
| chunk_results[chunk_index] = (prediction, data, model) |
Collaborator
There was a problem hiding this comment.
Schema failures stayed warn-only in #3372 and still are in _prepare_pr_review; raising here drops a chunk whose YAML reads no as a boolean. The parse stays the retry trigger.
Suggested change
| try: | |
| data = self._load_valid_review_yaml(prediction, source=f"review chunk {chunk_index + 1}") | |
| if not self._validate_review_schema(data): | |
| raise ValueError(f"review chunk {chunk_index + 1} failed schema validation") | |
| except Exception as error: | |
| chunk_errors.append(error) | |
| get_logger().warning(f"Failed to parse review chunk {chunk_index + 1}; retrying it with fallback", | |
| artifact={"error": error}) | |
| continue | |
| chunk_results[chunk_index] = (prediction, data, model) | |
| try: | |
| data = self._load_valid_review_yaml(prediction, source=f"review chunk {chunk_index + 1}") | |
| except Exception as error: | |
| chunk_errors.append(error) | |
| get_logger().warning(f"Failed to parse review chunk {chunk_index + 1}; retrying it with fallback", | |
| artifact={"error": error}) | |
| continue | |
| self._validate_review_schema(data) | |
| chunk_results[chunk_index] = (prediction, data, model) |
|
|
||
| @pytest.mark.asyncio | ||
| async def test_invalid_chunk_emits_one_schema_warning_before_rendering(chunking_enabled): | ||
| async def test_invalid_chunk_is_retried_after_a_single_schema_warning(chunking_enabled): |
Collaborator
There was a problem hiding this comment.
Names what the test now pins.
Suggested change
| async def test_invalid_chunk_is_retried_after_a_single_schema_warning(chunking_enabled): | |
| async def test_invalid_chunk_emits_one_schema_warning_before_rendering(chunking_enabled): |
Comment on lines
+551
to
+569
| pytest.raises(ValueError, match="failed schema validation"), | ||
| ): | ||
| await reviewer._prepare_prediction("model") | ||
| reviewer._prepare_pr_review() | ||
|
|
||
| warnings = get_logger.return_value.warning.call_args_list | ||
| schema_warnings = [call for call in warnings if call.args == ("Review output failed schema validation",)] | ||
| assert len(schema_warnings) == 1 | ||
| assert schema_warnings[0].kwargs["artifact"] == {"field": "review.score", "value": 101} | ||
| assert reviewer.prediction_data is None | ||
|
|
||
| reviewer._get_prediction.side_effect = [CHUNK_A] | ||
| await reviewer._prepare_chunked_prediction("model") | ||
| reviewer._prepare_pr_review() | ||
|
|
||
| assert reviewer.review_chunk_count == 2 | ||
| assert reviewer.review_failed_chunk_count == 0 | ||
| assert [call.args[1] for call in reviewer._get_prediction.await_args_list] == [ | ||
| "chunk-a", "chunk-b", "chunk-a", | ||
| ] |
Collaborator
There was a problem hiding this comment.
Main's contract: one warning, no retry, chunk merged.
Suggested change
| pytest.raises(ValueError, match="failed schema validation"), | |
| ): | |
| await reviewer._prepare_prediction("model") | |
| reviewer._prepare_pr_review() | |
| warnings = get_logger.return_value.warning.call_args_list | |
| schema_warnings = [call for call in warnings if call.args == ("Review output failed schema validation",)] | |
| assert len(schema_warnings) == 1 | |
| assert schema_warnings[0].kwargs["artifact"] == {"field": "review.score", "value": 101} | |
| assert reviewer.prediction_data is None | |
| reviewer._get_prediction.side_effect = [CHUNK_A] | |
| await reviewer._prepare_chunked_prediction("model") | |
| reviewer._prepare_pr_review() | |
| assert reviewer.review_chunk_count == 2 | |
| assert reviewer.review_failed_chunk_count == 0 | |
| assert [call.args[1] for call in reviewer._get_prediction.await_args_list] == [ | |
| "chunk-a", "chunk-b", "chunk-a", | |
| ] | |
| ): | |
| await reviewer._prepare_prediction("model") | |
| reviewer._prepare_pr_review() | |
| warnings = get_logger.return_value.warning.call_args_list | |
| schema_warnings = [call for call in warnings if call.args == ("Review output failed schema validation",)] | |
| assert len(schema_warnings) == 1 | |
| assert schema_warnings[0].kwargs["artifact"] == {"field": "review.score", "value": 101} | |
| # schema validation stays warn-only (#3372): the chunk is merged, not retried | |
| assert reviewer._get_prediction.await_count == 2 | |
| assert reviewer.review_chunk_count == 2 | |
| assert reviewer.review_failed_chunk_count == 0 | |
| assert reviewer.prediction_data["review"]["score"] == 40 |
Resolves the pr_reviewer.py conflict with The-PR-Agent#3389 by keeping the cached-chunk path and forwarding output_token_reserve inside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpThzLDt7pgLathoucBkcL
Collaborator
|
#3389 landed a few minutes after the review above and this branch conflicted with it in the |
| """Split oversized pending chunks at file boundaries while preserving result order.""" | ||
| chunks = self._chunked_patches_diff_list | ||
| results = getattr(self, "_chunked_results", {}) | ||
| budget = get_max_tokens(model) - OUTPUT_BUFFER_TOKENS_SOFT_THRESHOLD - self.token_handler.prompt_tokens |
Contributor
There was a problem hiding this comment.
1. Reserved output makes retries overflow 🐞 Bug ☼ Reliability
_resize_pending_review_chunks, _include_newly_reviewable_files, and _get_prediction calculate input capacity by subtracting the fixed 1,500-token OUTPUT_BUFFER_TOKENS_SOFT_THRESHOLD instead of the active AI handler’s resolved output reserve. When configured completion limits, Claude extended-thinking allowances, or OpenRouter reasoning reserves exceed that threshold, cached fallback chunks pass resizing and preflight checks without enough context headroom, causing rejected fallback requests or avoidable partial reviews even though reserve-aware packing would split them further.
Agent Prompt
## Issue description
Cached fallback retries calculate available input capacity using a fixed 1,500-token output allowance, while normal multi-diff preparation uses the AI handler’s model-specific resolved output reserve. This can admit chunks that do not fit alongside the configured completion limit, Claude extended-thinking allowance, or OpenRouter reasoning reserve.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[904-930]
- pr_agent/tools/pr_reviewer.py[938-961]
- pr_agent/tools/pr_reviewer.py[987-1002]
- pr_agent/algo/token_budget.py[17-91]
## Recommended Fix
Create or reuse one shared `AttemptTokenBudget` for the current model via `AttemptTokenBudget.for_attempt`, passing `ai_handler.get_output_token_reserve` when available and preserving the existing minimum reserve. Use that budget’s available-token calculation and model-bound tokenizer consistently for cached-chunk resizing, omitted-file inclusion, and the final pre-send chunk guard so these paths apply the same reserve policy as `get_pr_multi_diffs()`. Add coverage for resolved output reserves greater than 1,500 tokens.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 17e12ac |
Contributor
|
Code review by qodo was updated up to the latest commit dd42732 |
Contributor
Author
|
@IsmaelMartinez done — schema validation is warn-only again (parse failures still retry with fallback), and the test was reverted to |
IsmaelMartinez
approved these changes
Sep 16, 2026
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for applying the three inlines exactly and keeping the parse failure as the only retry trigger.
Approving. A chunk answering relevant_tests: no now merges with one warning and no retry, as on main; a parse failure resends only that chunk; exhaustion still publishes what succeeded with the failed count. Suite green on today's main, coverage holds.
Qodo's reserve point (a fixed 1500 against the handler's reserve in the resize and pre-send checks) is real but narrow: fallback path only, reserve above 1500, and it degrades to a labelled partial review. I will file it as a follow-up on top of #3395 rather than hold this.
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
4 participants
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.
References to other Issues or PRs
Fixes #3397
Brief description of what is fixed or changed
Large PR reviews can be split into multiple model calls. If one chunk returns malformed output or raises a model error, the previous implementation discarded the successful chunks and reran the entire review with the fallback model.
This change caches the prepared chunks and successfully parsed results. Fallback attempts now retry only failed chunks, then merge successful chunk results in their original order. Once all fallback models are exhausted, any successful chunks are published with the failed count in the coverage note; incomplete reviews cannot resolve persistent findings.
Regression tests cover malformed output and model-call failures, including verification that successful chunks are not requested again.
Other comments
A review still fails when no chunk succeeds. Cached chunks retain the primary model's plan, so a smaller fallback can leave an oversized chunk uncovered.
Validation: 138 focused tests passed, including the real fallback-chain partial-publication and all-failed paths. Ruff and pre-commit passed.
AI Generation Disclosure
AI assistance was used for the collaborator-review fixes and regression tests.
Release Notes