Share the Fixed Fence Parser Across All Three Callers - #903
Merged
Conversation
…tions CodeRabbit, on PR #901: unfenced_text() now handles fence marker family, length, and indentation correctly, but extract_section() and strip_sections() still toggled on any marker regardless. A ~~~ line nested inside a ``` block made both exit the fenced state early, so a following ## line could truncate a verbatim section (extract_section is what the byte-for-byte check hashes) or end a strip_sections region short, exposing what followed as if it were the repo's own prose. Verified both against the actual code before fixing. Extracted the corrected per-line fence-state logic from unfenced_text into _fence_step(), a single pure function all three now call, so the fence-matching rule exists in exactly one place. Added a regression case covering the nested-marker scenario for both functions.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR Summary by QodoShare CommonMark fence parsing across audit section utilities
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A newly added comment uses a mid-sentence semicolon, which violates the repository's no-semicolon prose rule for agent-authored comments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the CommonMark fence parsing logic so extract_section(), strip_sections(), and unfenced_text() share one consistent implementation, preventing nested/mismatched fence markers from prematurely ending fenced regions and truncating section extraction.
Changes:
- Extracted the per-line fence state transition into
_fence_step()and updated all three callers to use it. - Updated
extract_section()andstrip_sections()to avoid the prior naive toggle-on-any-marker behavior. - Added a selftest regression covering a mismatched fence marker nested inside another fenced block.
File summaries
| File | Description |
|---|---|
| spec/audit.py | Centralizes fence parsing in _fence_step() and adds a regression test to prevent section truncation due to nested/mismatched fences. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot: prose style forbids semicolons in agent-authored comments. Recast as one comma-joined sentence.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The shared _fence_step() implementation is consistently applied across all three callers and the added regression selftest directly covers the reported failure mode.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
qodo-code-review: caught a spaced-hyphen dash carried over from the docstring this text was adapted from. Recast as sentence breaks.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The shared _fence_step() logic is consistently applied across all three callers and is covered by an added regression selftest targeting the previously verified failure mode.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
ptr727
added a commit
that referenced
this pull request
Aug 22, 2026
## Summary Follow-up to #903, addressing a finding raised on the #900 -> #901 promotion PR review (CodeRabbit). Per CommonMark, a backtick-fenced opener's info string may not itself contain a backtick (the spec's own reasoning: otherwise inline code spans could be misread as a new fence). \`_fence_step()\` accepted an opener like `` ```md` `` regardless, so a heading right after it was hidden from the scan. Verified independently against the actual code before fixing. A tilde fence has no such restriction and is unaffected. Also split the two over-25-word docstring sentences flagged in the same review round. ## Verification - `python3 spec/audit.py --selftest` -> `SELFTEST PASS`, including 3 new regression cases and the full pre-existing suite unchanged - `python3 spec/validate.py` -> `Spec validation OK` - `ruff check .` / `ruff format --check .` -> clean - `python3 scripts/prose_lint.py --diff origin/develop spec/audit.py` -> clean ## A note on scope This is the fourth follow-up PR (#901 -> #902 -> #903 -> this one) chasing progressively deeper CommonMark fence-parsing edge cases that CodeRabbit's automated review keeps finding one round at a time against `_fence_step()`. Each one has been real and independently verified, but I want to flag the pattern rather than silently keep going: CommonMark has more edge cases than these four (unterminated fences at EOF, tab expansion in indentation, and others), and a sufficiently persistent automated reviewer may keep surfacing them. Worth a decision on where "correct enough" is for a fleet-internal audit tool versus a full CommonMark implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #902, addressing a finding raised on the #900 -> #901 promotion PR review (CodeRabbit).
unfenced_text()(fixed in #902) now handles fence marker family, length, and indentation correctly, butextract_section()andstrip_sections()still used the original naive toggle-on-any-marker logic. Verified independently before fixing: a~~~line nested inside a```block made both exit the fenced state early, so a following##line could end the region short.This is not cosmetic for
extract_section(): it is what the verbatim byte-for-byte section check hashes, so a nested example inside a fenced code sample could silently truncate what gets compared against the hub canonical.Extracted the corrected per-line fence logic from
unfenced_text()into_fence_step(), a single pure function all three now call, so the fence-matching rule lives in exactly one place instead of three near-duplicates that can drift apart the way the first two already had. Added a regression case covering the nested-marker scenario for both functions, on top of the existingextract_sectionandstrip_sections(viatemplate_ref_outside_verbatim) coverage, which still passes unchanged.Verification
python3 spec/audit.py --selftest->SELFTEST PASS, including the new nested-fence regression and the full pre-existing suite unchangedpython3 spec/validate.py->Spec validation OKruff check ./ruff format --check .-> cleanpython3 scripts/prose_lint.py --diff origin/develop spec/audit.py-> clean