Skip to content

fix: preserve nested code fences in review snippets - #3418

Open
kevin-lozada-santos wants to merge 8 commits into
The-PR-Agent:mainfrom
kevin-lozada-santos:fix/3415-review-code-fences
Open

kevin-lozada-santos wants to merge 8 commits into
The-PR-Agent:mainfrom
kevin-lozada-santos:fix/3415-review-code-fences

Conversation

@kevin-lozada-santos

@kevin-lozada-santos kevin-lozada-santos commented Sep 16, 2026

Copy link
Copy Markdown

Review follow-up at 54d3f63: clarified two test names and corrected test documentation. Executable AST is unchanged after excluding docstrings and those names; all 33 focused tests, Ruff and applicable pre-commit hooks pass. The full-suite and six successful CI results below are from the earlier 7f24014 revision; fresh CI for this follow-up is pending.

Fixes #3415. Restores @evandrocoan's fix from #2312 against current main, preserving all five original commits and their authorship. Adds four regression cases for nested fences in patch-derived snippets, with and without dedenting.

Extracted lines containing a closing triple-backtick fence currently close their own wrapper: GitHub then renders the review footer inside a second code block. The restored helper selects the shortest safe backtick or tilde fence, preserving the snippet and keeping the surrounding review intact. This does not change the separately identified issue_content interpolation.

Validation (Windows Python 3.12.13 and Linux Python 3.12.3):

  • Original extraction tests against base c425ad7: 4 failed, 12 passed, reproducing the bug.
  • Patched extraction and fence tests, including the added patch cases: 33 passed.
  • GitHub Markdown API, same public README fixture before/after: before = two code blocks and footer inside code; after = one intact snippet and footer outside code.
  • Ruff and all applicable pre-commit hooks pass.
  • Full Linux suite on the same commit, Python 3.12.3 with frozen uv.lock: 8,593 passed, 33 skipped, 1 xfailed in 128.23s.
  • Full Windows suite: 8,549 passed, 39 failed, 33 skipped, 1 xfailed, 5 errors. The optional failing-test baseline comparison was stopped before completion after several minutes without new output; a complete baseline match and Windows-wide compatibility are not claimed.
  • Upstream CI verified September 16, 2026 on commit 7f24014: build-and-test, pre-commit, github-provider-smoke, Analyze (python), CodeQL, and update_release_draft all completed successfully. Ready for maintainer review. No provider/LLM end-to-end test is claimed.

Terminal excerpts:

baseline: 4 failed, 12 passed in 2.89s
candidate: 33 passed in 0.89s

I used Brain Scanner to organize a focused map of the review-formatting path and its test evidence. The diagnosis and repair are verified by the tests and renderer comparison above. Try it now: https://brainscanner.dev/

evandrocoan and others added 6 commits September 16, 2026 06:02
When a file's extracted lines contain their own fenced code blocks
(e.g. a README with a ```bash section), wrapping them in the standard
triple-backtick fence caused the Markdown parser to misinterpret the
inner ``` as a closing fence. This broke rendering inside GitHub
<details> collapsible blocks, producing a stray ``` before </details>.

Introduce _get_fence(content) in pr_agent/algo/utils.py. The helper
scans the content for the longest consecutive run of backticks and
returns a fence that is one backtick longer (minimum three). This
guarantees the outer fence can never be prematurely closed by any
backtick sequence inside the content.

Update extract_relevant_lines_str to call _get_fence instead of
hard-coding ``` . The guard "if relevant_lines_str" that now wraps
the fence assignment also prevents building a fence string around an
empty result, which was a latent bug.

Add tests/unittest/test_extract_relevant_lines_str.py covering:

- Empty/missing file list returns an empty string.
- Single-line and multi-line extraction with correct fences.
- Language identifier propagated into the opening fence.
- Optional dedent of common leading whitespace.
- Fallback path that mines lines from the patch when head_file is None.
- Filename with leading/trailing spaces is matched correctly.
- Content containing ``` triggers a 4-backtick outer fence, and the
  inner block is preserved verbatim.
- End-to-end check via convert_to_markdown_v2 with a real README
  containing a ```bash block: the <details> body uses ````markdown
  and the inner ```bash is kept intact.
Updated the content construction in the
_test_make_file method for better readability
by using parentheses for multiline strings.
Also improved the assertion message in the
test case to enhance clarity when the
expected output does not match.
The `_get_fence` utility previously only considered backtick sequences when
computing a safe outer fence for Markdown code blocks. When content (such
as a README) contained triple-backtick fenced blocks, the function would
produce a four-backtick fence. For content with even longer backtick runs,
the fence could grow arbitrarily long, risking truncation by LLM providers
or Markdown renderers with line-length limits.

The updated algorithm evaluates both backtick and tilde fence candidates
independently, then selects whichever yields the shorter safe fence. Since
most real-world Markdown content uses backtick fences and rarely contains
tilde sequences, a triple-backtick block in the content now triggers a
three-tilde fence rather than a four-backtick fence. This keeps the outer
fence at the minimum safe length in the common case.

The tie-breaking rule preserves the previous default: when both candidates
produce the same length, backticks are returned, so plain content without
either character still produces the familiar ``` fence.

All affected unit tests are updated to assert tilde fences where the old
logic would have produced four-backtick fences, and a new dedicated test
suite for `_get_fence` covers empty input, single-character runs, long
runs of each character, mixed content, tie-breaking, and the invariant
that the returned fence never appears verbatim inside the content.

Reduces the risk of provider-side truncation caused by unexpectedly long
fence lines when wrapping Markdown content that itself contains fenced
code blocks.
… double quotes

Updated string literals in the test file to use double quotes for consistency
and improved readability. This change affects various sections of the code,
including the definition of Python code blocks and the construction of
synthetic files for testing. The adjustments ensure uniformity in string
formatting across the test cases.
…unction

The change updates string literals in the `_get_fence` function from single
quotes to double quotes. This is a purely cosmetic, style-consistency
change with no behavioral impact.

Python projects often enforce a single quoting style across the codebase
to maintain readability and reduce cognitive overhead when reading code.
Tools like `black`, the widely adopted Python formatter, default to double
quotes. Aligning with this convention ensures the codebase remains
consistent and passes automated style checks without requiring exceptions
or suppressions.

Standardizing quote style reduces noise in future diffs, making it easier
to spot meaningful changes rather than stylistic ones.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Preserve nested code fences in review snippets

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Selects the shortest collision-free backtick or tilde fence for extracted review snippets.
• Preserves nested Markdown fences across head-file and patch-derived extraction paths.
• Adds regression coverage for dedenting, details rendering, and pathological fence runs.
Diagram

graph TD
    A["Review issue"] --> B["Snippet extractor"] --> C{"Head available"}
    C -->|Yes| D["Head lines"] --> F["Optional dedent"] --> G["Safe fence"] --> H["GFM details"]
    C -->|No| E["Patch lines"] --> F
Loading
High-Level Assessment

The dynamic shortest-safe-fence strategy is appropriate because it preserves snippet content verbatim, retains language-aware fenced rendering, and handles arbitrary runs of both backticks and tildes. Fixed-length fences, escaping nested markers, or switching to HTML preformatted blocks would be less robust or alter existing rendering behavior.

Files changed (3) +483 / -1

Bug fix (1) +25 / -1
utils.pyChoose collision-free fences for extracted review snippets +25/-1

Choose collision-free fences for extracted review snippets

• Adds '_get_fence' to select the shortest safe backtick or tilde fence based on runs present in the snippet. Extracted content is fenced only when non-empty, preventing nested fences from terminating review code blocks prematurely.

pr_agent/algo/utils.py

Tests (2) +458 / -0
test_extract_relevant_lines_str.pyCover snippet extraction and nested-fence rendering +385/-0

Cover snippet extraction and nested-fence rendering

• Adds regression coverage for head-file and patch-derived extraction, optional dedenting, language identifiers, missing content, and filename normalization. Integration-style cases verify nested Markdown fences remain intact inside GFM details blocks without falling back to '<pre>'.

tests/unittest/test_extract_relevant_lines_str.py

test_get_fence.pyValidate safe fence selection across delimiter runs +73/-0

Validate safe fence selection across delimiter runs

• Tests minimum fence lengths, backtick-versus-tilde selection, equal-length tie behavior, collision avoidance, and pathological delimiter runs.

tests/unittest/test_get_fence.py

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Issue fences can still break reviews 📎 Requirement gap ≡ Correctness
Description
test_fenced_block_in_issue_content_uses_longer_fence supplies an issue_content value containing
only inline backticks, while convert_to_markdown_v2 still interpolates that value directly into
the details summary. When generated issue text contains a backtick or tilde code fence, this test
never exercises the required case and the interpolation can consume surrounding details markup.
Code

tests/unittest/test_extract_relevant_lines_str.py[R142-143]

+    def test_fenced_block_in_issue_content_uses_longer_fence(self):
+        # issue_content as the LLM produces it — no triple backticks (confirmed by real log)
Evidence
Compliance rule 3175560 requires independent handling and automated coverage for fences inside
issue_content. The new test states that its issue_content has no triple-backtick fence, while
the production template still inserts issue_content directly without applying the new fence
helper.

Handle Markdown fences safely in issue content
tests/unittest/test_extract_relevant_lines_str.py[142-147]
pr_agent/algo/utils.py[534-547]



Remediation recommended

2. A backtick test comment is narrative 📘 Rule violation ⚙ Maintainability ⭐ New
Description
test_no_triple_backticks_in_content documents its assertion with Ensures... and `which
would...`, using descriptive clauses rather than an imperative instruction. A later change to this
rendering regression encounters a statement of current behavior instead of a directive identifying
the invariant the test must preserve.
Code

tests/unittest/test_extract_relevant_lines_str.py[R98-99]

+        # Ensures the generated string never contains triple backticks inside the code block,
+        # which would break markdown rendering inside <details> tags on GitHub.
Evidence
Compliance rule 2694688 requires behavioral comments to use imperative phrasing, while the added
comment begins with the descriptive third-person verb Ensures and continues with a narrative
consequence.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_extract_relevant_lines_str.py[98-99]

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 comment in `test_no_triple_backticks_in_content` uses descriptive third-person phrasing instead of an imperative instruction.

## Fix Focus Areas
- tests/unittest/test_extract_relevant_lines_str.py[98-99]

## Recommended Fix
Rewrite the comment as a direct imperative that tells maintainers to keep triple-backtick sequences out of the generated block content so GitHub details rendering remains intact.

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


3. Two fence comments are not imperative ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
test_content_with_inner_fenced_block_uses_safe_fence uses narrative comments beginning `When the
extracted lines... and The new implementation chooses...`. When fence selection changes, later
editors receive descriptions of the current implementation rather than direct guidance about the
behavior the test must preserve.
Code

tests/unittest/test_extract_relevant_lines_str.py[R113-114]

+        # When the extracted lines contain ```, the outer fence must not collide with that run.
+        # The new implementation chooses a tilde fence (~~~) because it is shorter than ````.
Evidence
Compliance rule 2694688 requires newly added behavioral comments to use imperative phrasing, while
the cited comments narrate the condition and current implementation in descriptive prose.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_extract_relevant_lines_str.py[113-114]

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

## Issue description
Two comments describing the nested-fence test use narrative phrasing instead of imperative instructions.
## Fix Focus Areas
- tests/unittest/test_extract_relevant_lines_str.py[113-114]
## Recommended Fix
Rewrite the comments as direct instructions, such as `Avoid colliding with an embedded triple-backtick run` and `Choose the shorter tilde fence instead of four backticks`, without changing the test behavior.

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


4. Fence tests use narrative docstrings ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
TestGetFence gives its new test methods descriptive fragments such as No runs ... and
third-person outcomes such as tilde wins instead of imperative docstrings. This pattern spans the
added helper tests, leaving later test documentation inconsistent with the required
instruction-oriented convention.
Code

tests/unittest/test_get_fence.py[R5-6]

+    def test_empty_content_returns_backtick_minimum(self):
+        """No runs of either character → minimum 3-backtick fence."""
Evidence
Compliance rule 2694688 requires newly added behavioral docstrings and comments to use imperative
phrasing. The added test docstrings instead begin with noun phrases such as No runs, `Content
with, and A run`, and other added class documentation begins with third-person verbs such as
Verifies and Tests.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_get_fence.py[5-22]
tests/unittest/test_extract_relevant_lines_str.py[130-140]
tests/unittest/test_extract_relevant_lines_str.py[208-215]

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 new fence-test method docstrings narrate conditions and outcomes rather than using imperative phrasing as required by the documentation convention.
## Fix Focus Areas
- tests/unittest/test_get_fence.py[5-70]
- tests/unittest/test_extract_relevant_lines_str.py[130-367]
## Recommended Fix
Rewrite each behavioral docstring and comment with an imperative opening, such as `Verify that empty content returns the minimum backtick fence.` Preserve the assertions and test behavior unchanged.

ⓘ 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.

Grey Divider

Context sources
✅ Compliance rules (platform): 31 rules
Review mode: ⚖️ Balanced: This is a localized runtime Markdown-fence behavior fix with meaningful rendering edge cases and integration-path impact, so it warrants a careful single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 6c4549f ⚖️ Balanced

Results up to commit 54d3f63


🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (1) 📜 Skill insights (0)


Action required
1. Issue fences can still break reviews 📎 Requirement gap ≡ Correctness
Description
test_fenced_block_in_issue_content_uses_longer_fence supplies an issue_content value containing
only inline backticks, while convert_to_markdown_v2 still interpolates that value directly into
the details summary. When generated issue text contains a backtick or tilde code fence, this test
never exercises the required case and the interpolation can consume surrounding details markup.
Code

tests/unittest/test_extract_relevant_lines_str.py[R142-143]

+    def test_fenced_block_in_issue_content_uses_longer_fence(self):
+        # issue_content as the LLM produces it — no triple backticks (confirmed by real log)
Evidence
Compliance rule 3175560 requires independent handling and automated coverage for fences inside
issue_content. The new test states that its issue_content has no triple-backtick fence, while
the production template still inserts issue_content directly without applying the new fence
helper.

Handle Markdown fences safely in issue content
tests/unittest/test_extract_relevant_lines_str.py[142-147]
pr_agent/algo/utils.py[534-547]



Remediation recommended
2. Two fence comments are not imperative 📘 Rule violation ⚙ Maintainability ⭐ New
Description
test_content_with_inner_fenced_block_uses_safe_fence uses narrative comments beginning `When the
extracted lines... and The new implementation chooses...`. When fence selection changes, later
editors receive descriptions of the current implementation rather than direct guidance about the
behavior the test must preserve.
Code

tests/unittest/test_extract_relevant_lines_str.py[R113-114]

+        # When the extracted lines contain ```, the outer fence must not collide with that run.
+        # The new implementation chooses a tilde fence (~~~) because it is shorter than ````.
Evidence
Compliance rule 2694688 requires newly added behavioral comments to use imperative phrasing, while
the cited comments narrate the condition and current implementation in descriptive prose.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_extract_relevant_lines_str.py[113-114]

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

## Issue description
Two comments describing the nested-fence test use narrative phrasing instead of imperative instructions.

## Fix Focus Areas
- tests/unittest/test_extract_relevant_lines_str.py[113-114]

## Recommended Fix
Rewrite the comments as direct instructions, such as `Avoid colliding with an embedded triple-backtick run` and `Choose the shorter tilde fence instead of four backticks`, without changing the test behavior.

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


3. Fence tests use narrative docstrings ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
TestGetFence gives its new test methods descriptive fragments such as No runs ... and
third-person outcomes such as tilde wins instead of imperative docstrings. This pattern spans the
added helper tests, leaving later test documentation inconsistent with the required
instruction-oriented convention.
Code

tests/unittest/test_get_fence.py[R5-6]

+    def test_empty_content_returns_backtick_minimum(self):
+        """No runs of either character → minimum 3-backtick fence."""
Evidence
Compliance rule 2694688 requires newly added behavioral docstrings and comments to use imperative
phrasing. The added test docstrings instead begin with noun phrases such as No runs, `Content
with, and A run`, and other added class documentation begins with third-person verbs such as
Verifies and Tests.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_get_fence.py[5-22]
tests/unittest/test_extract_relevant_lines_str.py[130-140]
tests/unittest/test_extract_relevant_lines_str.py[208-215]

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 new fence-test method docstrings narrate conditions and outcomes rather than using imperative phrasing as required by the documentation convention.
## Fix Focus Areas
- tests/unittest/test_get_fence.py[5-70]
- tests/unittest/test_extract_relevant_lines_str.py[130-367]
## Recommended Fix
Rewrite each behavioral docstring and comment with an imperative opening, such as `Verify that empty content returns the minimum backtick fence.` Preserve the assertions and test behavior unchanged.

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


Context sources
✅ Compliance rules (platform): 31 rules
Review mode: ⚖️ Balanced: This is a focused runtime formatting fix with contained logic, but it changes review-output rendering and fence selection, warranting a careful single-pass review.
Results up to commit 7f24014


🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (1) 📜 Skill insights (0)


Action required
1. Issue fences can still break reviews 📎 Requirement gap ≡ Correctness
Description
test_fenced_block_in_issue_content_uses_longer_fence supplies an issue_content value containing
only inline backticks, while convert_to_markdown_v2 still interpolates that value directly into
the details summary. When generated issue text contains a backtick or tilde code fence, this test
never exercises the required case and the interpolation can consume surrounding details markup.
Code

tests/unittest/test_extract_relevant_lines_str.py[R142-143]

+    def test_fenced_block_in_issue_content_uses_longer_fence(self):
+        # issue_content as the LLM produces it — no triple backticks (confirmed by real log)
Evidence
Compliance rule 3175560 requires independent handling and automated coverage for fences inside
issue_content. The new test states that its issue_content has no triple-backtick fence, while
the production template still inserts issue_content directly without applying the new fence
helper.

Handle Markdown fences safely in issue content
tests/unittest/test_extract_relevant_lines_str.py[142-147]
pr_agent/algo/utils.py[534-547]

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

## Issue description
Issue content is still interpolated directly into the details template, and the newly added test explicitly uses issue content without a fenced code block. Consequently, backtick or tilde fences in generated issue text remain unhandled and untested.

## Fix Focus Areas
- pr_agent/algo/utils.py[534-547]
- tests/unittest/test_extract_relevant_lines_str.py[142-205]

## Recommended Fix
Apply dedicated content-safe fence handling or escaping to `issue_content` before inserting it into the details template. Update the regression test so `issue_content` itself contains backtick and tilde fenced blocks, then assert that the complete details structure remains valid and `</details>` remains outside those blocks.

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



Remediation recommended
2. Fence tests use narrative docstrings 📘 Rule violation ⚙ Maintainability
Description
TestGetFence gives its new test methods descriptive fragments such as No runs ... and
third-person outcomes such as tilde wins instead of imperative docstrings. This pattern spans the
added helper tests, leaving later test documentation inconsistent with the required
instruction-oriented convention.
Code

tests/unittest/test_get_fence.py[R5-6]

+    def test_empty_content_returns_backtick_minimum(self):
+        """No runs of either character → minimum 3-backtick fence."""
Evidence
Compliance rule 2694688 requires newly added behavioral docstrings and comments to use imperative
phrasing. The added test docstrings instead begin with noun phrases such as No runs, `Content
with, and A run`, and other added class documentation begins with third-person verbs such as
Verifies and Tests.

Rule 2694688: Docstrings and comments must use imperative phrasing
tests/unittest/test_get_fence.py[5-22]
tests/unittest/test_extract_relevant_lines_str.py[130-140]
tests/unittest/test_extract_relevant_lines_str.py[208-215]

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 new fence-test method docstrings narrate conditions and outcomes rather than using imperative phrasing as required by the documentation convention.

## Fix Focus Areas
- tests/unittest/test_get_fence.py[5-70]
- tests/unittest/test_extract_relevant_lines_str.py[130-367]

## Recommended Fix
Rewrite each behavioral docstring and comment with an imperative opening, such as `Verify that empty content returns the minimum backtick fence.` Preserve the assertions and test behavior unchanged.

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


Context sources
✅ Compliance rules (platform): 31 rules
Review mode: ⚖️ Balanced: This is a localized runtime Markdown-formatting fix with meaningful rendering edge cases and broad test changes, warranting a careful single-pass review but not multiple independent passes.
ⓘ  1 issues published inline · 2 in summary

Grey Divider

Qodo Logo

Comment thread tests/unittest/test_extract_relevant_lines_str.py Outdated
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 54d3f63

Signed-off-by: kevin-lozada-santos <kevinolozadasantos@gmail.com>
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 6c4549f

@kevin-lozada-santos

Copy link
Copy Markdown
Author

Updated the two nested-fence comments to imperative phrasing in 6c4549f.

All 33 focused tests, Ruff, and applicable local pre-commit hooks pass. An exact Python AST comparison against the parent confirms that executable code, assertions, test names, and docstrings are unchanged.

The separate issue_content item remains deferred as recorded in the review thread. The new build/test, provider-smoke, pre-commit, and CodeQL workflow runs need maintainer approval; no fresh full-suite CI success is claimed.

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.

extract_relevant_lines_str breaks the review block when the extracted lines contain a code fence

2 participants