[taljeon]

Closes #3356.

What changed

• Validate the repaired YAML and minimum non-empty review shape inside each _prepare_prediction model attempt, before that attempt is considered successful.
• Raise an unusable response into the existing fallback-model loop, so a valid fallback can produce the review instead of the primary response reaching the renderer and failure comment.
• Reset prediction and chunk bookkeeping for every model attempt so failed primary state cannot leak into a fallback result.
• Apply the same shape gate to chunk responses. Malformed content now retries at the model level; transport failures still preserve the existing partial-review behavior when another chunk succeeded.
• Keep final failure handling unchanged: if every configured model returns unusable review data, the run publishes the existing single sanitized failure result.

The existing load_yaml repair chain still runs before validation. This change deliberately checks only the current non-empty mapping contract; strict field/type validation remains outside this issue.

Validation

• TDD: the new fallback and chunk controls produced 5 failures before the implementation and all pass afterward.
• Focused reviewer, chunking, fallback, and ticket-budget suites: 108 passed.
• Full unit suite: 7,855 passed, 33 skipped, 1 xfailed.
• Repository-wide ruff check . passed.
• Applicable pre-commit hooks passed on all five touched files; the workflow-only actionlint hook was skipped because no workflow file changed.
• git diff --check passed.

AI assistance

I used OpenAI Codex to help inspect the retry lifecycle, implement the change, and draft tests. I reviewed the full diff and ran the validation above locally.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Retry malformed review responses with fallback models

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Validate repaired review YAML before accepting model or chunk responses.
• Retry malformed reviews with fallback models while isolating each attempt's state.
• Preserve partial chunk reviews for transport failures and existing final failure handling.

Diagram

graph TD
  A["Model attempt"] --> B["Reset attempt state"] --> C["Generate response"] --> D["Repair YAML"] --> E{"Valid review?"}
  E -- "valid" --> F["Publish review"]
  E -- "invalid" --> G["Fallback loop"] -- "retry" --> A
  G -- "exhausted" --> H["Publish failure"]

Loading

High-Level Assessment

Validating at the model-attempt boundary is the best approach because it reuses the existing fallback mechanism and prevents unusable state from reaching rendering. Renderer-only validation would occur too late to retry, while strict schema validation would exceed the current compatibility contract.

Files changed (5) +165 / -21

Bug fix (1) +18 / -6

pr_reviewer.pyValidate review shape within every model attempt +18/-6

Validate review shape within every model attempt

• Resets prediction and chunk state before each model attempt, then requires repaired YAML to contain a non-empty review mapping before accepting it. Malformed single or chunk responses now raise into the fallback loop, while transport-failed chunks can still contribute partial reviews when siblings succeed.

pr_agent/tools/pr_reviewer.py

Tests (4) +147 / -15

test_linked_ticket_token_budget.pyReturn valid review YAML from token-budget test stubs +2/-0

Return valid review YAML from token-budget test stubs

• Updates the reviewer prediction stub to satisfy the newly enforced minimum review shape while preserving generic responses for other tools.

tests/unittest/test_linked_ticket_token_budget.py

test_pr_reviewer_core.pyAlign core prediction tests with review validation +8/-6

Align core prediction tests with review validation

• Introduces a shared valid review prediction and updates preparation tests to use and assert the accepted YAML response.

tests/unittest/test_pr_reviewer_core.py

test_pr_reviewer_fallback_validation.pyCover malformed response fallback and publication behavior +129/-0

Cover malformed response fallback and publication behavior

• Adds tests proving malformed primary responses invoke a fallback, valid fallbacks publish normally, and exhaustion publishes exactly one failure result. The fixture isolates model, deployment, publication, and error-propagation settings.

tests/unittest/test_pr_reviewer_fallback_validation.py

test_review_large_diff_chunking.pyRequire malformed chunks to fail their model attempt +8/-9

Require malformed chunks to fail their model attempt

• Updates chunking expectations so malformed or empty review mappings raise validation errors instead of being silently skipped or retried as a single call. Assertions also verify invalid chunk data is not retained.

tests/unittest/test_review_large_diff_chunking.py

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

• Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 5c327e4 ⚖️ Balanced

Results up to commit 546c732

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

[IsmaelMartinez]

Thanks for having the draft up within twenty minutes of asking, and for keeping the change to the plan you wrote down. One small ask inline (the lines sit just outside the hunk, so it is a code block rather than a suggestion), then this is ready; I have released CI on the branch.

Malformed then valid across two models now publishes one review with no failure comment, both models malformed posts exactly one, and a response the repair chain can fix never reaches the fallback. The transport-error partial path for chunks is unchanged.

The inline suggestion removes a branch this change made unreachable; its log line describes a fallback that no longer exists.

[IsmaelMartinez]

Just below this hunk, lines 845-849: with two or more chunks every prediction now either lands in chunk_errors (transport error) or in chunk_outputs (validated, or it raises), so the warning("No chunk produced a parsable review, falling back ...") branch cannot run and its message describes a single-call fallback that no longer exists. Coverage lists 848-849 as missing. GitHub will not take a suggestion outside the hunk, so the replacement is:

        if not chunk_outputs:
            raise chunk_errors[0]

Applied here: 108 passed on the four files, full suite unchanged.

[taljeon]

Removed the unreachable branch exactly as suggested in noreply 5c327e4f: when a multi-chunk attempt produces no validated output, it now raises the first recorded chunk error instead of retaining a dead single-call fallback path and stale warning.

Validation on the pushed head:

• the four reviewer/chunk/fallback/ticket-budget files: 108 passed
• full unit suite: 7,855 passed, 33 skipped, 1 xfailed
• Ruff and all applicable pre-commit hooks on pr_reviewer.py passed; actionlint was skipped because no workflow file changed
• exact one-file diff, commit metadata, and publication leak checks passed

Release Drafter is green on 5c327e4f; Build-and-test, pre-commit, CodeQL, and Provider smoke need fresh fork-workflow approval for this head. Could you please re-review it when those settle? Thanks for catching the now-unreachable fallback rather than leaving misleading dead code behind.

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 5c327e4

[IsmaelMartinez]

Thanks for turning the code block into a four-line commit within the quarter hour and posting your own run beside it; approving.

The branch now carries exactly the removal asked for, the surviving raise always has an error to hand because a chunked attempt holds at least two predictions and each is recorded before that check, and the fallback tests go red with main's reviewer restored. Qodo has nothing on this head.

CI was gated on the new head; released.