fix: prevent markdown fence balancer from corrupting sequential code blocks#26785
Merged
fix: prevent markdown fence balancer from corrupting sequential code blocks#26785
Conversation
…blocks The balanceCodeRegions() function was incorrectly detecting 'true nesting' when a document contained sequential code blocks without intermediate language-tagged openers. For example, a ```cpp block followed by a bare ``` block would be merged into a single ```` block, corrupting the markdown by treating the closing/opening fences as content. Root cause: the isTrueNesting condition fired when closerCount > openerCount + 1, but when openerCount was 0 (no intermediate language-tagged fences), this triggered for any 2+ bare closers. Fix: require openerCount > 0 for nesting detection. Without intermediate language-tagged openers, multiple bare closers are sequential code blocks (greedy matching per CommonMark), not nested content. Also unskips 6 tests that were masked by this bug, adds real-world regression tests including the exact pattern from the production bug, and adds idempotency and invariant tests. Test count: 66 passing + 6 skipped -> 82 passing, 0 skipped.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the markdown fence “nesting” detection in balanceCodeRegions() so sequential fenced code blocks (e.g., cpp … followed by …) are no longer incorrectly merged/corrupted by fence-length escalation.
Changes:
- Tightens “true nesting” detection by adding a guard requiring at least one intermediate language-tagged opener.
- Updates/expands the Vitest suite: corrects prior expectations that encoded broken behavior, unskips previously skipped cases, and adds multiple real-world regression scenarios plus invariants/idempotency tests.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/markdown_code_region_balancer.cjs | Adjusts the “true nesting” condition used to decide when to lengthen fences and treat intermediate fences as content. |
| actions/setup/js/markdown_code_region_balancer.test.cjs | Updates expectations around greedy matching/sequential blocks and adds extensive regression + invariant coverage. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/markdown_code_region_balancer.test.cjs:1041
- This “true nesting” test only asserts
isBalanced(result) === trueand doesn’t check any nesting/escaping outcome (e.g., outer fence length increase / preservation of inner fence lines as content). As written, it can pass even if the nesting-escape path never runs. Consider asserting a concrete expected output or at least checking for lengthened outer fences on an input that requires escaping.
it("should handle markdown block containing a language-tagged inner block", () => {
// This IS true nesting: ```markdown contains ```python inside it
// The balancer should handle this by making the result balanced
const input = `\`\`\`markdown
Here's an example:
actions/setup/js/markdown_code_region_balancer.test.cjs:1055
- This test also only checks
isBalanced(result)and doesn’t assert that the balancer performed the intended nesting handling (such as escaping by lengthening the outer fence). SinceisBalanced()currently ignores the info string when determining closers, this assertion may not catch regressions in nesting logic. Consider asserting a specific expected output or checking for a longer outer fence.
it("should handle nested documentation example with multiple inner blocks", () => {
// Markdown block containing multiple language-tagged inner blocks
const input = `\`\`\`markdown
## Usage
- Files reviewed: 2/2 changed files
- Comments generated: 3
… matching The isTrueNesting branch was unreachable: directClosers are filtered to have hasOpenerBetween === false, so openerCount over i+1..lastDirectCloser is always 0. Remove the dead code path, fenceLengthAdjustments map, and the fence-length rewriting pass. The balancer now uses straightforward CommonMark greedy matching throughout. -95 lines of dead/unreachable code removed.
Closing fences must be bare (no info string/language tag) per CommonMark. isBalanced() and countCodeRegions() were not checking this, giving false confidence in test assertions. Update both functions and fix tests for ambiguous nesting cases to assert the correct invariant (no worse than input) instead of asserting balanced output.
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.
Problem
The
balanceCodeRegions()function inmarkdown_code_region_balancer.cjswas incorrectly detecting "true nesting" in markdown that contained sequential code blocks with no actual nesting. This caused safe-output bodies (e.g.,create-issue,create-pull-request) to have their fenced code blocks corrupted.Example: A PR body with a
```cppblock followed by a bare```block would get transformed into a single````-fenced block, swallowing the closing/opening fences in between.Observed in: fsprojects/PX4-Autopilot#39
Root cause
The
isTrueNestingcondition was:When
openerCountis 0 (no intermediate language-tagged fences between opener and closers), this fires for any 2+ bare closers — treating perfectly valid sequential code blocks as nested content and wrapping them in longer fences.Fix
Added a guard requiring at least one intermediate language-tagged opener for nesting detection:
Without intermediate openers, multiple bare closers are sequential code blocks (greedy matching per CommonMark), not nested content.
Testing improvements
it.skip/ TODO comments — all pass nowbalance(balance(x)) === balance(x)Test count: 66 passing + 6 skipped → 82 passing, 0 skipped