Skip to content

fix(pr_agent/tools/pr_code_suggestions.py): reporting no suggestions when the key is missing - #2903

Closed
dwin-gharibi wants to merge 2 commits into
The-PR-Agent:mainfrom
dwin-gharibi:fix/improve-missing-suggestions-key
Closed

fix(pr_agent/tools/pr_code_suggestions.py): reporting no suggestions when the key is missing#2903
dwin-gharibi wants to merge 2 commits into
The-PR-Agent:mainfrom
dwin-gharibi:fix/improve-missing-suggestions-key

Conversation

@dwin-gharibi

Copy link
Copy Markdown
Contributor

Closes #2902.

Description

_prepare_pr_code_suggestions iterates data['code_suggestions'] without checking that the key exists or holds a list.

Root cause

Answering in prose when there is nothing to suggest is the behaviour of a helpful assistant, and it is
exactly the case the tool should report cleanly. Instead it is the case that loses the run.

The fix

Validate the parsed payload once, before the loop: anything that is not a dict carrying a code_suggestions list is logged with the raw prediction and reported as an empty suggestion list.

Behaviour change

Before A prose answer fails the whole /improve run
After A prose answer reports no suggestions; a well-formed response is unchanged

Files changed

pr_agent/tools/pr_code_suggestions.py | 5 +++++
 1 file changed, 5 insertions(+)

Testing

Written test-first: the test was committed red, then the fix turned it green.

New regression coverage in tests/unittest/test_improve_missing_suggestions_key.py7 tests:

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

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

Full unit suite on this branch:

$ PYTHONPATH=. pytest tests/unittest
3 failed, 2767 passed, 1 skipped, 1 xfailed, 89 warnings in 27.88s

The 3 failures are tests/unittest/test_extra_config_url.py, which fail identically on unmodified main in this
sandbox because they need outbound network; they pass in CI.

Also checked:

  • ruff — no new findings vs main; isort — clean on every file touched
  • No new code comments authored

Risk / compatibility

The existing normalisation of a top-level list is preserved and still tested. Only inputs that used to raise now return an empty list.

Copilot AI lite review requested due to automatic review settings August 31, 2026 07:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions Bot added the bug label Aug 31, 2026
@qodo-code-review

qodo-code-review Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

Grey Divider

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.

Grey Divider

Qodo Logo

@IsmaelMartinez

Copy link
Copy Markdown
Collaborator

Closing as a duplicate of your own #2730, which adds the same guard to the same function, fixes load_yaml at source as well, and went green this morning once you pushed the empty-dict tests. The two conflict textually so only one can land, and #2730 is the stronger of the pair. Worth moving the new test cases across, minus the code_suggestions: | / none one, which already returns an empty list on main.

One thing to carry over there before it goes in. #2867 merged at 08:10 today, about forty minutes after you opened this, so you could not have seen it. It counts a chunk as failed only when _get_prediction raises, so returning a clean empty list leaves failed_chunk_count at 0 and the coverage footer never renders: a malformed chunk then reads as a fully successful run with fewer suggestions rather than a partial one.

Thanks for both of these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/improve fails when the model answers in prose or omits the suggestions key

3 participants