fix(pr_agent/algo/utils.py): returning an empty dict when every yaml fallback fails

[dwin-gharibi]

Closes #2729.

Description

When every YAML fallback fails, load_yaml returns None; nine callers across five tools assume a dict and raise instead of taking their graceful path.

Root cause

load_yaml has no return on its final failure path, so it falls off the end and yields None.
Every caller was written against the documented contract of "a dict", and the guards they contain
('key' not in data, data.get(...)) are exactly the expressions that raise on None.

The fix

• load_yaml returns {} instead of None on total failure, so every existing 'key' not in data
  and data.get(...) guard starts working as written.
• pr_code_suggestions._prepare_pr_code_suggestions gains the explicit guard its siblings already
  have: a non-dict or code_suggestions-less payload is logged with the raw prediction and returns
  {'code_suggestions': []}.

Behaviour change

Before	An unparseable prediction raises TypeError/AttributeError inside the tool
After	An unparseable prediction returns an empty result and logs the specific parse failure

Files changed

pr_agent/algo/utils.py                | 2 ++
 pr_agent/tools/pr_code_suggestions.py | 4 ++++
 2 files changed, 6 insertions(+)

Testing

New regression coverage in tests/unittest/test_load_yaml_unparseable.py — 7 tests, each written to fail
without the fix:

$ PYTHONPATH=. pytest tests/unittest/test_load_yaml_unparseable.py
7 passed

Proven to be a genuine regression test: with every changed pr_agent/ file reverted to its
origin/main version and the new test file left in place, the suite fails. It only passes
with the fix applied.

Full pipeline, reproduced locally exactly as .github/workflows/build-and-test.yaml runs it:

docker build -f docker/Dockerfile --target test .
docker run --rm <image> pytest -v tests/unittest
-> 1968 passed, 1 skipped, 1 xfailed

on python:3.12.13-slim — no failures.

Also checked:

• pytest tests/unittest — no new failures vs main
• ruff — no new findings vs main; isort — clean on every file touched
• No new code comments authored

Risk / compatibility

{} is falsy, so if not data: checks behave identically to None. Callers that explicitly
compare against None were checked — there are none.

Checklist

• Focused on a single fix
• Existing tests pass
• New regression tests added, proven to fail without the fix
• No new dependencies
• Reviewed by a maintainer

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[qodo-code-review]

PR Summary by Qodo

Fix load_yaml to return {} on total parse failure; harden code suggestions parsing

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Return an empty mapping from load_yaml when all YAML fallbacks fail.
• Add a defensive guard in code-suggestions parsing to avoid crashes on malformed predictions.
• Add regression tests ensuring unparseable AI output degrades gracefully across tools.

Diagram

graph TD
  A{{"AI prediction text"}} --> B["load_yaml()"] --> C["Parsed dict or {}"]
  C --> D["PRReviewer"] --> G["Empty result + log"]
  C --> E["PRCodeSuggestions"] --> H["{code_suggestions: []} + log"]
  C --> F["PRGenerateLabels"] --> I["Empty data + safe membership checks"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Raise a dedicated parse exception from load_yaml

• ➕ Makes parse failure explicit and stack-traceable
• ➕ Avoids silently converting invalid input into an empty mapping
• ➖ Requires updating many callers to catch/handle exceptions
• ➖ Higher risk of missing a handler and reintroducing crashes

2. Change load_yaml contract to Optional[dict] and enforce caller checks

• ➕ More semantically accurate than {} vs None
• ➕ Encourages callers to handle parse failure explicitly
• ➖ Broad refactor across multiple tools and call sites
• ➖ Increased boilerplate and easy to regress if a caller forgets the None guard

3. Return a structured Result (data + errors)

• ➕ Preserves parse error details without relying on logs
• ➕ Allows callers to choose behavior based on failure reason
• ➖ Heavier API/typing change for a small bug fix
• ➖ Requires coordinated updates across the codebase

Recommendation: The PR’s approach is the best fit for a targeted regression fix: returning {} preserves the documented “dict” contract and immediately makes existing caller guards ('key' not in data, data.get(...)) work as intended. The extra explicit guard in pr_code_suggestions is appropriate because it subscripts/assumes a specific shape even when the payload is a dict.

Files changed (3) +63 / -0

Bug fix (2) +6 / -0

utils.pyReturn {} when YAML parsing fully fails +2/-0

Return {} when YAML parsing fully fails

• Adds an explicit fallback return of an empty dict when all parsing attempts yield 'None'. This aligns runtime behavior with the documented contract and prevents downstream 'TypeError'/'AttributeError' from membership checks and '.get()' calls.

pr_agent/algo/utils.py

pr_code_suggestions.pyGuard malformed parsed payloads in code suggestions tool +4/-0

Guard malformed parsed payloads in code suggestions tool

• Adds a defensive check ensuring the parsed payload is a dict and contains 'code_suggestions'. On failure, logs the raw prediction and returns an empty 'code_suggestions' list to avoid tool crashes.

pr_agent/tools/pr_code_suggestions.py

Tests (1) +57 / -0

test_load_yaml_unparseable.pyRegression tests for unparseable AI predictions across tools +57/-0

Regression tests for unparseable AI predictions across tools

• Introduces tests asserting 'load_yaml' returns '{}' for unparseable input and that common callers (reviewer, code suggestions, label generation) take graceful paths without raising. Covers both the failure mode and the unchanged behavior for valid YAML.

tests/unittest/test_load_yaml_unparseable.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Validate suggestions collection type ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new guard accepts any dictionary containing code_suggestions, including `code_suggestions:
null` or a scalar, and the method then directly enumerates that value. Such malformed predictions
still raise TypeError instead of returning the promised empty result.

Code

pr_agent/tools/pr_code_suggestions.py[R594-597]

+        if not isinstance(data, dict) or 'code_suggestions' not in data:
+            get_logger().error("Failed to parse code suggestions from the AI prediction",
+                               artifact={'predictions': predictions})
+            return {'code_suggestions': []}

Relevance

●●● Strong

Malformed nested values can still raise TypeError; defensive collection validation is a
straightforward correctness fix.

PR-#2212
PR-#2231

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added guard checks only the outer type and key presence, while the method later enumerates the
unchecked field directly. The enumeration occurs outside the per-suggestion error handling, so
malformed collection values escape that handling and abort the tool.

pr_agent/tools/pr_code_suggestions.py[594-597]
pr_agent/tools/pr_code_suggestions.py[602-603]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_prepare_pr_code_suggestions` only checks that the parsed payload is a dictionary containing `code_suggestions`; it does not check that the field is an iterable collection of suggestions. YAML such as `code_suggestions: null` therefore still crashes during direct enumeration.

### Issue Context
Preserve the existing empty-result behavior for malformed AI predictions. Validate the field before iteration and return `{'code_suggestions': []}` with the existing parse-failure logging when it is not a list (or otherwise safely normalize only the supported shape).

### Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[594-597]
- pr_agent/tools/pr_code_suggestions.py[602-603]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Single-quoted literals added ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The changed Python code introduces single-quoted string literals in the code_suggestions guard and
fallback mapping. These literals violate the repository requirement to use double quotes for Python
strings.

Code

pr_agent/tools/pr_code_suggestions.py[R595-598]

+            get_logger().error("Failed to parse code suggestions from the AI prediction",
+                               artifact={'predictions': predictions})
+            return {'code_suggestions': []}

Relevance

●●● Strong

Recent precedent: team accepted converting single-quoted literals to double quotes for style
compliance.

PR-#2687

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires all non-docstring Python string literals in changed files to use double
quotes. The added guard and return value contain single-quoted literals on the cited lines.

Rule 2694657: Use double quotes for all Python string literals
pr_agent/tools/pr_code_suggestions.py[595-598]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The changed code uses single-quoted Python string literals, contrary to the project-wide double-quote convention.

## Issue Context
Use double quotes for the `code_suggestions` key and mapping value while preserving the guard and fallback behavior.

## Fix Focus Areas
- pr_agent/tools/pr_code_suggestions.py[595-598]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

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

Context sources

✅ Compliance rules (platform): 34 rules

Review mode: 🚀 Fast: The latest push is a localized validation tightening in one runtime path plus focused regression tests, with no security, API, migration, or broad cross-cutting risk.

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-code-review]

Code review by qodo was updated up to the latest commit d8de789

[IsmaelMartinez]

#2618 just landed and wrapped the try_fix_yaml fallbacks in if data is not None:. This still merges clean, but three of its new tests assert load_yaml returns None, so they go red under your {} contract. Worth updating them here.

[IsmaelMartinez]

Thanks for this, and sorry for the wait. #2618 just landed.

#2618, #2622 and #2061 have each added is None assertions since the 23rd, so on the merged tree five tests go red across test_load_yaml.py and test_try_fix_yaml.py. The green tick here is from 21 August, before any of those landed, which is why it does not show.

Six one-line swaps to == {} and this is ready. Rebase and do that, or say the word and I will push it for you.

[qodo-code-review]

Code Review by Qodo

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again manually by commenting /agentic_review on this PR.

[IsmaelMartinez]

Thanks for this, and for coming back with the tests this morning.

One thing it was going to break: #2867 landed today and warns when only part of a run succeeded, and returning a clean empty list meant a failed chunk no longer got counted, so a partial run would have reported as a complete one. I have pushed the small fix for that with a test, rather than send it back to you over one line.

Merging.

[IsmaelMartinez]

Green on the new head. Approving.