fix(pr_agent/algo/git_patch_processing.py): skipping lines that are not unified hunk headers

[dwin-gharibi]

Closes #2756.

Description

extract_hunk_headers(match) is called whenever a line starts with @@, without checking that RE_HUNK_HEADER actually matched.

Root cause

A @@-prefixed line that is not a standard unified hunk header — a combined/merge diff
@@@ … @@@, for instance — makes RE_HUNK_HEADER.match return None, which is then passed
straight to extract_hunk_headers.

Scoping, stated honestly: I could not demonstrate a provider that surfaces such a patch, so
this is a latent robustness gap rather than a reachable crash. The plain-diff provider accepts
arbitrary user-supplied diffs and is the most plausible route.

The fix

Both sites check RE_HUNK_HEADER.match(line) first. An unrecognised @@ line is logged and skipped, and in extract_hunk_lines_from_patch its body is skipped too, so the valid hunks in the same patch still parse.

Behaviour change

Before	One unrecognised @@ line raises, or silently discards the whole patch
After	The unrecognised hunk is skipped with a warning; the valid hunks still parse

Files changed

pr_agent/algo/git_patch_processing.py     | 12 ++++++++++--
 tests/unittest/test_diff_pipeline_core.py |  8 +++-----
 2 files changed, 13 insertions(+), 7 deletions(-)

Testing

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

$ PYTHONPATH=. pytest tests/unittest/test_hunk_header_parse_guard.py
5 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
-> 1966 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

Well-formed patches take an identical path — the match result was already computed at both sites.

tests/unittest/test_diff_pipeline_core.py::test_malformed_patch_returns_empty_tuple asserted the
old ("", "") return, and its own comment described it as a consequence of the crash being
swallowed. It is updated to assert the new contract — no hunk content is produced — and renamed
accordingly.

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

Guard unified hunk parsing: skip non-matching '@@' lines and keep valid hunks

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Skip and warn on '@@' lines that don’t match the unified hunk header regex.
• Prevent malformed/combined diff headers from discarding otherwise valid hunks.
• Add regression tests ensuring parsing degrades gracefully without crashing.

Diagram

graph TD
  P["Patch lines"] --> D["decouple_and_convert..."] --> C1{"Unified hunk header?"}
  C1 -->|"yes"| O["Parsed hunks/selection"]
  C1 -->|"no"| L["Warn + skip"] --> O
  P --> E["extract_hunk_lines..."] --> C2{"Unified hunk header?"}
  C2 -->|"yes"| O
  C2 -->|"no"| L

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Support combined/merge hunks explicitly

• ➕ Could preserve content from @@@ ... @@@ (combined diff) rather than skipping it
• ➕ Improves compatibility if any providers ever emit combined diff patches
• ➖ Requires defining semantics for combined hunks (multiple parents) in downstream logic
• ➖ Higher complexity and risk than a defensive guard, especially if the pipeline assumes unified diffs

2. Use a dedicated diff parsing library (e.g., unidiff)

• ➕ More robust parsing across diff variants and edge cases
• ➕ Centralizes parsing behavior instead of maintaining custom regex logic
• ➖ May add dependency/behavior changes and integration work
• ➖ Potential performance and formatting differences compared to current string-based output

Recommendation: The current approach (guard + warn + skip) is the right minimal fix because the pipeline is designed around unified diff semantics. It eliminates a latent crash and avoids re-defining support for combined diff formats. If combined diffs become a real input source, consider adding explicit support or switching to a dedicated diff parser at that time.

Files changed (3) +63 / -7

Bug fix (1) +10 / -2

git_patch_processing.pySkip non-unified '@@' lines before extracting hunk headers +10/-2

Skip non-unified '@@' lines before extracting hunk headers

• Adds RE_HUNK_HEADER match guards in both hunk-rendering and line-selection paths before calling extract_hunk_headers. When a line starts with '@@' but doesn’t match the unified hunk header regex, the code logs a warning and skips the invalid header (and skips that hunk’s body in the selection helper) so subsequent valid hunks still parse.

pr_agent/algo/git_patch_processing.py

Tests (2) +53 / -5

test_diff_pipeline_core.pyUpdate malformed patch test to match new skip/ignore contract +3/-5

Update malformed patch test to match new skip/ignore contract

• Renames and adjusts the malformed patch test to assert that invalid '@@' headers produce no hunk content in the returned full patch output. Validates that malformed headers and their body lines are omitted while the selected output remains empty.

tests/unittest/test_diff_pipeline_core.py

test_hunk_header_parse_guard.pyAdd regression coverage for '@@' lines that are not unified hunk headers +50/-0

Add regression coverage for '@@' lines that are not unified hunk headers

• Introduces a focused test suite covering combined/merge diff headers (e.g., '@@@ ... @@@') and ensuring both parsing entrypoints degrade gracefully. Verifies that invalid headers are skipped without raising and that valid hunks still render/select correctly even after an invalid header appears earlier in the patch.

tests/unittest/test_hunk_header_parse_guard.py

[qodo-code-review]

Code Review by Qodo

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

1. Negative line numbers leak ✓ Resolved 🐞 Bug ≡ Correctness

Description

When decouple_and_convert_to_hunks_with_lines_numbers() skips a non-matching @@ header, it still
accumulates subsequent lines; on the next valid hunk header, those lines can be flushed using
start2=-1, producing negative line numbers and emitting invalid diff content as a hunk.

Code

pr_agent/algo/git_patch_processing.py[R355-358]

+            if not RE_HUNK_HEADER.match(line):
+                get_logger().warning("Skipping a line that starts with '@@' but is not a unified "
+                                     "hunk header", artifact={"line": line})
+                continue

Relevance

●●● Strong

Recent accepted parser robustness findings favor fixing state leakage that emits invalid numbered
hunks.

PR-#2677
PR-#2679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds a guard that continues on invalid @@ headers, but the function still appends
subsequent non-header lines into new_content_lines/old_content_lines. On the next valid header,
the code flushes any accumulated lines and renders numbered __new hunk__ lines using start2 + i,
where start2 is initialized to -1 until the first valid header is parsed. Downstream,
remove_line_numbers() only strips lines whose first character is a digit, so negative-number lines
are not stripped.

pr_agent/algo/git_patch_processing.py[343-396]
pr_agent/algo/git_patch_processing.py[361-373]
pr_agent/tools/pr_code_suggestions.py[748-764]

Agent prompt

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

### Issue description
`decouple_and_convert_to_hunks_with_lines_numbers()` now skips invalid `@@` headers, but it does **not** skip the *body* lines that follow them. Those lines remain in `new_content_lines`/`old_content_lines` and will be flushed when the next valid hunk header is seen.

If the invalid header appears before any valid hunk, `start2` is still initialized to `-1`, so flushing renders negative line numbers (e.g., `-1 ...`). This also breaks downstream logic that strips line numbers by checking `line[0].isdigit()`, because `-` is not a digit.

### Issue Context
A patch like `COMBINED + "\n" + NORMAL` (added in tests) triggers this: the combined header line is skipped, but its body lines are still accumulated and then flushed when `NORMAL` is parsed.

### Fix Focus Areas
- pr_agent/algo/git_patch_processing.py[343-396]
- pr_agent/tools/pr_code_suggestions.py[748-764]

### What to change
1. In `decouple_and_convert_to_hunks_with_lines_numbers()`, avoid calling `RE_HUNK_HEADER.match()` twice: compute `match` once.
2. When encountering a line that starts with `@@` but does not match `RE_HUNK_HEADER`, ensure the subsequent body lines are not buffered into the next valid hunk. Options:
  - Introduce a `skip_hunk`/`skipping_invalid_hunk` flag (similar to `extract_hunk_lines_from_patch`) that stays `True` until the next valid header is found, and while `True` ignore `+/-/context` lines.
  - Also clear `new_content_lines`/`old_content_lines` and reset `match/start1/start2` appropriately so nothing flushes with `start2=-1`.
3. Add/adjust a unit test asserting that after `COMBINED + "\n" + NORMAL`, the output contains the normal hunk but does **not** contain negative line numbers or combined-diff body lines.

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

---

2. Docstrings not imperative mood ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New docstrings are written as descriptive statements (e.g., "A … must …") rather than imperative
phrasing as required. This reduces consistency/readability and violates the docstring style
compliance rule.

Code

tests/unittest/test_hunk_header_parse_guard.py[1]

+"""A line starting with '@@' that is not a unified hunk header must not crash parsing."""

Relevance

●● Moderate

Imperative-docstring feedback was recently accepted, but an equally recent module-docstring
precedent was rejected.

PR-#2703
PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2694688 requires newly added docstrings/comments to use imperative phrasing. In the
new test module, the module docstring and multiple test docstrings are written as descriptive
statements ("A ... must ...") rather than imperative commands.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_hunk_header_parse_guard.py[1-1]
tests/unittest/test_hunk_header_parse_guard.py[16-18]

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 newly added docstrings use descriptive phrasing (e.g., "A ... must ...") instead of imperative mood (e.g., "Skip ...", "Do not ...").

## Issue Context
Compliance requires first-line docstrings/comments to be in imperative mood for consistency.

## Fix Focus Areas
- tests/unittest/test_hunk_header_parse_guard.py[1-50]

ⓘ 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: ⚖️ Balanced: This is a focused runtime parsing fix with behavior changes at two related paths; it carries real correctness risk, but not enough independent logic for extended review.

Tip of the day

💡 Did you know, you can tweak Display preferences with a live preview to see your comment before it ships

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 14dc0ca ⚖️ Balanced

Results up to commit 84e1f52 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

1. Negative line numbers leak ✓ Resolved 🐞 Bug ≡ Correctness

Description

When decouple_and_convert_to_hunks_with_lines_numbers() skips a non-matching @@ header, it still
accumulates subsequent lines; on the next valid hunk header, those lines can be flushed using
start2=-1, producing negative line numbers and emitting invalid diff content as a hunk.

Code

pr_agent/algo/git_patch_processing.py[R355-358]

+            if not RE_HUNK_HEADER.match(line):
+                get_logger().warning("Skipping a line that starts with '@@' but is not a unified "
+                                     "hunk header", artifact={"line": line})
+                continue

Relevance

●●● Strong

Recent accepted parser robustness findings favor fixing state leakage that emits invalid numbered
hunks.

PR-#2677
PR-#2679

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds a guard that continues on invalid @@ headers, but the function still appends
subsequent non-header lines into new_content_lines/old_content_lines. On the next valid header,
the code flushes any accumulated lines and renders numbered __new hunk__ lines using start2 + i,
where start2 is initialized to -1 until the first valid header is parsed. Downstream,
remove_line_numbers() only strips lines whose first character is a digit, so negative-number lines
are not stripped.

pr_agent/algo/git_patch_processing.py[343-396]
pr_agent/algo/git_patch_processing.py[361-373]
pr_agent/tools/pr_code_suggestions.py[748-764]

Agent prompt

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

### Issue description
`decouple_and_convert_to_hunks_with_lines_numbers()` now skips invalid `@@` headers, but it does **not** skip the *body* lines that follow them. Those lines remain in `new_content_lines`/`old_content_lines` and will be flushed when the next valid hunk header is seen.

If the invalid header appears before any valid hunk, `start2` is still initialized to `-1`, so flushing renders negative line numbers (e.g., `-1 ...`). This also breaks downstream logic that strips line numbers by checking `line[0].isdigit()`, because `-` is not a digit.

### Issue Context
A patch like `COMBINED + "\n" + NORMAL` (added in tests) triggers this: the combined header line is skipped, but its body lines are still accumulated and then flushed when `NORMAL` is parsed.

### Fix Focus Areas
- pr_agent/algo/git_patch_processing.py[343-396]
- pr_agent/tools/pr_code_suggestions.py[748-764]

### What to change
1. In `decouple_and_convert_to_hunks_with_lines_numbers()`, avoid calling `RE_HUNK_HEADER.match()` twice: compute `match` once.
2. When encountering a line that starts with `@@` but does not match `RE_HUNK_HEADER`, ensure the subsequent body lines are not buffered into the next valid hunk. Options:
  - Introduce a `skip_hunk`/`skipping_invalid_hunk` flag (similar to `extract_hunk_lines_from_patch`) that stays `True` until the next valid header is found, and while `True` ignore `+/-/context` lines.
  - Also clear `new_content_lines`/`old_content_lines` and reset `match/start1/start2` appropriately so nothing flushes with `start2=-1`.
3. Add/adjust a unit test asserting that after `COMBINED + "\n" + NORMAL`, the output contains the normal hunk but does **not** contain negative line numbers or combined-diff body lines.

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

---

2. Docstrings not imperative mood ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

New docstrings are written as descriptive statements (e.g., "A … must …") rather than imperative
phrasing as required. This reduces consistency/readability and violates the docstring style
compliance rule.

Code

tests/unittest/test_hunk_header_parse_guard.py[1]

+"""A line starting with '@@' that is not a unified hunk header must not crash parsing."""

Relevance

●● Moderate

Imperative-docstring feedback was recently accepted, but an equally recent module-docstring
precedent was rejected.

PR-#2703
PR-#2661

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2694688 requires newly added docstrings/comments to use imperative phrasing. In the
new test module, the module docstring and multiple test docstrings are written as descriptive
statements ("A ... must ...") rather than imperative commands.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_hunk_header_parse_guard.py[1-1]
tests/unittest/test_hunk_header_parse_guard.py[16-18]

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 newly added docstrings use descriptive phrasing (e.g., "A ... must ...") instead of imperative mood (e.g., "Skip ...", "Do not ...").

## Issue Context
Compliance requires first-line docstrings/comments to be in imperative mood for consistency.

## Fix Focus Areas
- tests/unittest/test_hunk_header_parse_guard.py[1-50]

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

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 14dc0ca