Skip to content

fix(suggestions): align fallback chunks after conversion failure - #2867

Merged
IsmaelMartinez merged 2 commits into
The-PR-Agent:mainfrom
YaoSong808:fix/2863-fallback-chunk-alignment
Aug 31, 2026
Merged

fix(suggestions): align fallback chunks after conversion failure#2867
IsmaelMartinez merged 2 commits into
The-PR-Agent:mainfrom
YaoSong808:fix/2863-fallback-chunk-alignment

Conversation

@YaoSong808

@YaoSong808 YaoSong808 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • rebuild unnumbered chunks from the final numbered fallback chunks so their membership and boundaries stay aligned
  • make push_inline_code_suggestions return None explicitly on every path to satisfy CodeQL
  • add regression coverage where fallback chunking changes from one chunk to two

Testing

  • PYTHONPATH=. ./.venv/bin/pytest tests/unittest -q (2764 passed, 1 skipped, 1 xfailed)
  • ./.venv/bin/pre-commit run --files pr_agent/tools/pr_code_suggestions.py tests/unittest/test_pr_code_suggestions_core.py tests/unittest/test_pr_code_suggestions_rendering.py

Follow-up to #2863.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Align fallback suggestion chunks after conversion failure

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Rebuild unnumbered fallback chunks from final numbered chunks to preserve boundaries and
 membership.
• Reject residual numbered/unnumbered chunk mismatches before pairing predictions.
• Explicitly return None from suggestion publishing and cover fallback realignment regressions.
Diagram

graph TD
  A["Initial chunk views"] --> B{"Conversion succeeds?"}
  B -->|Yes| C["Converted numbered"] --> E{"Counts align?"}
  B -->|No| D["Fallback numbered"] --> F["Derive unnumbered"] --> E
  E -->|Aligned| G["Prediction calls"]
  E -->|Mismatch| F
  F -->|Still mismatched| H["Alignment error"]
Loading
High-Level Assessment

Using the final numbered chunk list as the canonical fallback representation is the safest approach because both prediction inputs must share identical boundaries. Independently re-chunking both views or relying on zip() would preserve the original truncation risk, so explicit regeneration and validation are preferable.

Files changed (3) +52 / -5

Bug fix (1) +14 / -4
pr_code_suggestions.pyRealign fallback chunk views before suggestion prediction +14/-4

Realign fallback chunk views before suggestion prediction

• Rebuilds unnumbered chunks from the final numbered fallback chunks and validates equal list lengths before pairing prediction inputs. It also declares and explicitly preserves the 'None' return contract for inline suggestion publishing.

pr_agent/tools/pr_code_suggestions.py

Tests (2) +38 / -1
test_pr_code_suggestions_core.pyCover fallback chunk-boundary realignment +36/-0

Cover fallback chunk-boundary realignment

• Adds an asynchronous regression test where conversion fails and fallback chunking changes one stale chunk into two. The test verifies each numbered chunk is paired with its matching unnumbered form and both predictions are retained.

tests/unittest/test_pr_code_suggestions_core.py

test_pr_code_suggestions_rendering.pyVerify empty suggestion publishing returns None +2/-1

Verify empty suggestion publishing returns None

• Extends the no-suggestions rendering test to assert the publishing method returns 'None' while still posting the expected comment.

tests/unittest/test_pr_code_suggestions_rendering.py

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider

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.

Grey Divider

Tip of the day
💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified correct, and this is the default path: decouple_hunks = false.

On main the fallback re-chunks with line numbers but keeps the stale unnumbered list, so zip misaligns and truncates: 4 numbered chunks became 3 prediction calls, each paired with another chunk's text.

Reachable in a default config, no mock needed: a model outside MAX_TOKENS makes the conversion throw and return []. Your head passes all 12 scenarios I ran; main fails 8.

Thanks for deriving both views from one canonical list rather than re-chunking twice. One ask inline.

Comment thread pr_agent/tools/pr_code_suggestions.py Outdated
@YaoSong808
YaoSong808 force-pushed the fix/2863-fallback-chunk-alignment branch from 868a6fe to d651bb9 Compare August 31, 2026 03:22
@YaoSong808

Copy link
Copy Markdown
Contributor Author

/agentic_review

@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 IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Applied exactly as asked, and the new coverage still goes red against main. Taking it from here, thanks again for the follow-up.

@IsmaelMartinez
IsmaelMartinez merged commit a6768dd into The-PR-Agent:main Aug 31, 2026
5 checks passed
IsmaelMartinez added a commit to dwin-gharibi/pr-agent that referenced this pull request Aug 31, 2026
Returning an empty list stopped the chunk from raising, so The-PR-Agent#2867's coverage footer
counted it as successful and a partial run reported as a complete one. Record the parse
failure and add it to failed_chunk_count.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants