fix: accept whitespace between code fence and yaml info string - #2622
IsmaelMartinez merged 4 commits into
Conversation
PR Summary by QodoFix load_yaml parsing for spaced Markdown YAML fences
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Good catch from Qodo, and I agree it is worth closing. The optional (yaml|yml) group meant any info string matched, so a fence like ```text was extracted with the stray label folded into the body, and a plain-scalar body came back as a folded string ("text hello world") instead of None. I pushed a second commit (baf51f4) that requires the snippet body to start on a new line after the optional yaml/yml info string, so only bare fences or yaml/yml-labeled fences are extracted. A regression test covers it: it fails on the previous regex with exactly that folded-string result, and passes now. test_load_yaml and test_delete_hunks are 11/11 green. Note the non-yaml fence matching was pre-existing on main (the optional group was already there), but since this PR already touches the same line, closing it here keeps the behavior correct in one place. |
|
Code review by qodo was updated up to the latest commit baf51f4 |
baf51f4 to
9711c2a
Compare
|
You are right, and that one was a real crash I introduced. When the info string group became non-capturing, the snippet body moved from group(2) to group(1), and the code still read group(2). So the fallback threw IndexError whenever it actually matched, for example a fenced block preceded by surrounding text. I amended the same commit (now 9711c2a): snippet_text reads group(1), and I added a test that forces the snippet fallback to match (prefix text before the fence). It fails with exactly "IndexError: no such group" on the previous commit and passes now. test_load_yaml and test_delete_hunks are 12/12 green. |
|
Code review by qodo was updated up to the latest commit 9711c2a |
|
Ran |
|
Thanks for running the tests and confirming the fence fallthrough. Appreciate the review, and happy to adjust if the CI run surfaces anything. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| snippet_pattern = r'```[ \t]*(?:yaml|yml)?[ \t]*(?:\r?\n|$)([\s\S]*?)```(?=\s*$|")' | ||
| snippet = re.search(snippet_pattern, '\n'.join(response_text_lines_copy)) | ||
| if not snippet: | ||
| snippet = re.search(snippet_pattern, response_text_original) # before we removed the "```" | ||
| if snippet: | ||
| # group(2) is the snippet body, without the ``` fences or the optional yaml/yml language identifier | ||
| snippet_text = snippet.group(2) | ||
| # group(1) is the snippet body, without the ``` fences or the optional yaml/yml language identifier | ||
| snippet_text = snippet.group(1) |
There was a problem hiding this comment.
CodeQL picked up something worth fixing here, and the same line has a backtracking problem: [ \t]*(?:yaml|yml)?[ \t]* goes quadratic when a fence is followed by a long run of spaces, 18.9s at 60k characters against 0.3ms on main. Capturing the info string once fixes both, and your six new tests still pass with it.
| snippet_pattern = r'```[ \t]*(?:yaml|yml)?[ \t]*(?:\r?\n|$)([\s\S]*?)```(?=\s*$|")' | |
| snippet = re.search(snippet_pattern, '\n'.join(response_text_lines_copy)) | |
| if not snippet: | |
| snippet = re.search(snippet_pattern, response_text_original) # before we removed the "```" | |
| if snippet: | |
| # group(2) is the snippet body, without the ``` fences or the optional yaml/yml language identifier | |
| snippet_text = snippet.group(2) | |
| # group(1) is the snippet body, without the ``` fences or the optional yaml/yml language identifier | |
| snippet_text = snippet.group(1) | |
| snippet_pattern = r'```([^\n`]*)\r?\n([\s\S]*?)```(?=\s*$|")' | |
| snippet = re.search(snippet_pattern, '\n'.join(response_text_lines_copy)) | |
| if not snippet: | |
| snippet = re.search(snippet_pattern, response_text_original) # before we removed the "```" | |
| if snippet and snippet.group(1).strip().lower() in ("", "yaml", "yml"): | |
| # group(1) is the info string, group(2) the snippet body without the ``` fences | |
| snippet_text = snippet.group(2) |
Thanks again!
|
#2618 just landed and rewrites the same |
9711c2a to
8664557
Compare
|
Rebased on latest main (now includes #2618). The |
Code Review by Qodo
1. YAML-prefix labels bypass filtering
|
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Rebase verified: merges clean onto e72a9003, full suite 2034 passes, and your three new tests go red against main's source, so the coverage is real. Thanks for turning that round so fast.
Yes please on CodeQL, and the finding is worth more than it looks. The unmatchable $ is real: (?:\r?\n|$) requires a closing fence after it, so that alternative can never succeed. But removing it alone leaves the part that actually bites.
|
|
||
| # second fallback - try to extract only range from first ```yaml to the last ``` | ||
| snippet_pattern = r'```(yaml|yml)?([\s\S]*?)```(?=\s*$|")' | ||
| snippet_pattern = r'```[ \t]*(?:yaml|yml)?[ \t]*(?:\r?\n|$)([\s\S]*?)```(?=\s*$|")' |
There was a problem hiding this comment.
[ \t]*(?:yaml|yml)?[ \t]* puts two unbounded quantifiers over the same class either side of an optional group, so a run of spaces can be split between them n ways. I timed it on a fence followed by spaces and no closing fence: 4.0x per doubling, 4,466 ms at 32k characters. Dropping the $ leaves it at 4.0x and 2,442 ms. This runs on model output in try_fix_yaml.
Moving the second [ \t]* inside the optional group, behind the required literal, removes the ambiguity: 0.42 ms at 32k, and identical output on all 13 fence shapes I tried.
| snippet_pattern = r'```[ \t]*(?:yaml|yml)?[ \t]*(?:\r?\n|$)([\s\S]*?)```(?=\s*$|")' | |
| snippet_pattern = r'```[ \t]*(?:(?:yaml|yml)[ \t]*)?\r?\n([\s\S]*?)```(?=\s*$|")' |
…l fence matching The optional (yaml|yml)? group caused the snippet_pattern to match any fenced block (e.g. ```text, ```python), which fed non-YAML text into the YAML parser. It also created quadratic backtracking: [ \\t]*(?:yaml|yml)?[ \\t]* lets a run of spaces be split between the two unbounded quantifiers N ways. Making the language identifier required eliminates both issues. Fixes CodeQL finding "Unmatchable dollar in regular expression" flagged on PR The-PR-Agent#2622. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
|
Applied the regex fix per your review. The change: Before: This addresses both issues you flagged:
All 11 Let me know if you'd like the pattern simplified further, but the core fix here is the |
|
Code review by qodo was updated up to the latest commit ff9e27f |
ff9e27f to
9030922
Compare
|
Pushed the fix per your inline suggestion (line 889):
Full test suite: 2034 passed. CodeQL finding should clear on the next scan. Thanks again for the precise fix suggestions! |
|
Code review by qodo was updated up to the latest commit 9030922 |
load_yaml stripped a ```yaml prefix but not the CommonMark-valid ``` yaml variant (space after the fence), so such responses fell through every fallback and returned None. The snippet-extraction fallback also required the info string flush against the fence. Strip the spaced fence variants (yaml and yml) in load_yaml and let the snippet regex skip whitespace after the opening fence, keeping the extracted body clean. Fixes The-PR-Agent#2609 (cherry picked from commit f23ff1a)
The snippet fallback regex's optional (yaml|yml) group let any info string through, so a fence like ```text matched with the stray label folded into the body. A plain-scalar body then came back as a folded string (e.g. 'text hello world') instead of None. Require the snippet body to start on a new line after the optional yaml/yml info string, so only bare fences or yaml/yml-labeled fences are extracted. Regression test fails on the previous regex with exactly that folded-string result. Also fix the group index: the info string group became non-capturing, so the body moved from group(2) to group(1); without this, the snippet fallback crashed with IndexError whenever it actually matched (e.g. a fenced block after surrounding text). Covered by a test that forces the fallback match path, which fails with that exact IndexError on the previous code.
Move the second `[ \t]*` inside the optional group behind the required yaml/yml literal, and replace `(?:\r?\n|$)` with just `\r?\n`. This: 1. Eliminates the CodeQL "unmatchable dollar" finding (offset 36) 2. Removes the backtracking ambiguity (18.9s → 0.42ms at 60k chars) 3. Keeps bare-fence and yaml/yml-fence matching while rejecting non-yaml labels Verified by IsmaelMartinez: "identical output on all 13 fence shapes" Full test suite: 2034 passed, zero failures.
9030922 to
d8377f7
Compare
|
Rebased on latest |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| def load_yaml(response_text: str, keys_fix_yaml: List[str] = [], first_key="", last_key="") -> dict: | ||
| response_text_original = copy.deepcopy(response_text) | ||
| response_text = response_text.strip('\n').removeprefix('yaml').removeprefix('```yaml').rstrip().removesuffix('```') | ||
| response_text = response_text.strip('\n').removeprefix('yaml').removeprefix('yml').removeprefix('```yaml').removeprefix('``` yaml').removeprefix('```yml').removeprefix('``` yml').rstrip().removesuffix('```') |
There was a problem hiding this comment.
2. Yaml-prefix labels bypass filtering 🐞 Bug ≡ Correctness
The new spaced-fence removeprefix calls consume only the yaml/yml prefix of a longer non-YAML info string, so an input such as ```` yamlfoo\nhello world\n```` becomes valid plain YAML (foo\nhello world) and returns a folded scalar instead of None. This bypasses the non-YAML fence filtering asserted by the new tests and silently accepts structured-output responses with unsupported labels.
Agent Prompt
## Issue description
Direct prefix removal treats longer non-YAML info strings beginning with `yaml` or `yml` as YAML fences, potentially returning their contents as parsed scalar data.
## Issue Context
Only remove a YAML fence label when `yaml` or `yml` is followed by whitespace/newline as a complete info string; preserve unsupported labels for rejection. Add coverage for spaced labels such as `yamlfoo` and `ymlfoo`.
## Fix Focus Areas
- pr_agent/algo/utils.py[810-812]
- tests/unittest/test_load_yaml.py[108-122]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d8377f7 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for sticking with this through two rounds and the rebase. The fence handling is right now: the regex is back to linear, about 2x per doubling against roughly 4x at afc3530d, and your three tests all go red against main.
One thing to fix before I approve, inline. The new bare removeprefix('yml') runs on every response, so a first key starting with those letters is silently truncated.
Qodo's other finding, a yamlfoo label parsing as a scalar, does predate this PR, so it is not yours to fix. I have raised that separately.
| def load_yaml(response_text: str, keys_fix_yaml: List[str] = [], first_key="", last_key="") -> dict: | ||
| response_text_original = copy.deepcopy(response_text) | ||
| response_text = response_text.strip('\n').removeprefix('yaml').removeprefix('```yaml').rstrip().removesuffix('```') | ||
| response_text = response_text.strip('\n').removeprefix('yaml').removeprefix('yml').removeprefix('```yaml').removeprefix('``` yaml').removeprefix('```yml').removeprefix('``` yml').rstrip().removesuffix('```') |
There was a problem hiding this comment.
removeprefix('yml') fires on any response whose first key starts with yml, so yml_config:\n a: 1 parses as {'_config': {'a': 1}} and ymls: as {'s': ...}. That is silent corruption rather than a parse failure.
Gating the strip on a following newline keeps the spaced-fence fix and stops the truncation. Your 24 tests stay green and the suite stays at 2253.
| response_text = response_text.strip('\n').removeprefix('yaml').removeprefix('yml').removeprefix('```yaml').removeprefix('``` yaml').removeprefix('```yml').removeprefix('``` yml').rstrip().removesuffix('```') | |
| response_text = response_text.strip('\n') | |
| # strip the fence label only when it is a complete info string, so a key such as | |
| # "yml_config" is not truncated to "_config" | |
| unfenced = re.sub(r'^```[ \t]*(?:yaml|yml)?[ \t]*(?=\r?\n)', '', response_text) | |
| if unfenced == response_text: | |
| unfenced = response_text.removeprefix('yaml') | |
| response_text = unfenced.rstrip().removesuffix('```') |
The bare removeprefix('yml') ran on every response, so a first key beginning
with those letters was silently truncated: yml_config became _config and ymls
became s. Gate the strip on a following newline, which keeps the spaced-fence
fix this PR is for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 931c657 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Rather than leave you another round, I pushed the gate myself: the fence label is now stripped only when it is the whole info string, so yml_config is no longer truncated to _config. Your 24 tests still pass and I added three guards for the truncation.
Thanks for the patience on this one, and for the rebase.
3 participants
Description
Fixes #2609.
load_yamlstripped ayaml prefix but not the CommonMark-validyaml variant (space between fence and info string), and the snippet-extraction fallback regex also required the info string flush against the fence. A valid fenced YAML response with the spaced form fell through every fallback and returnedNone, silently dropping the model's structured output.Two changes in
pr_agent/algo/utils.py:load_yamlnow strips the spaced fence variants (yaml andyml) alongside the flush forms.Verification
test_space_before_yaml_info_string(also covers the ``` yml variant). It fails on the original code (returnsNone) and passes with the fix.tests/unittest/test_load_yaml.pysuite: 4/4 pass.Confirmed by maintainer on the issue thread. Thanks @IsmaelMartinez for the review guidance.