Skip to content

fix: bounds check Substring calls in MarkdownConverter#2754

Open
rorychatt wants to merge 3 commits intomainfrom
fix/markdown-conversion-crash
Open

fix: bounds check Substring calls in MarkdownConverter#2754
rorychatt wants to merge 3 commits intomainfrom
fix/markdown-conversion-crash

Conversation

@rorychatt
Copy link
Copy Markdown
Collaborator

Fixes ArgumentOutOfRangeException in MarkdownConverter that was causing documentation extraction failures for files like IVYCHILD001.md. Also includes a new test suite to verify the fix.

@rorychatt
Copy link
Copy Markdown
Collaborator Author

@claude review this

@ivy-interactive-claude-code
Copy link
Copy Markdown
Contributor

ivy-interactive-claude-code bot commented Mar 23, 2026

Claude finished @rorychatt's task in 1m 16s —— View job


Code Review

  • Gather context and read changed files
  • Review code changes in MarkdownConverter.cs
  • Review new tests in MarkdownConverterTests.cs
  • Provide feedback

Overall: Looks good! ✓ The fix is solid and the approach is consistent.

Summary

The PR adds defensive bounds checking to 4 Substring calls that could throw ArgumentOutOfRangeException when Markdig span positions don't align perfectly with the markdown content length.

What's Good

  1. Consistent fix pattern - All 4 locations use the same robust pattern:

    int start = Math.Max(0, mBlock.Span.Start);
    int length = Math.Min(mBlock.Span.Length, markdownContent.Length - start);
  2. Tests cover edge cases - The emoji test is particularly good since surrogate pairs can cause offset misalignment between character and byte positions.

  3. No over-engineering - The fix is minimal and focused.

Suggestions (Optional)

  1. One more Substring to protect - Line 85 in ConvertAsync has an unprotected call:

    string yamlContent = markdownContent.Substring(yamlBlock.Span.Start, yamlBlock.Span.Length);

    Consider applying the same bounds check for consistency. Fix this →

  2. Test cleanup - Minor: the using System.Text; and using Markdig; imports in the test file appear unused. Not a blocker.

Verdict

Approve - The core fix is correct and addresses the reported issue. The optional suggestion about line 85 can be addressed in a follow-up if desired.


|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant