Skip to content

Fix JSON parsing to handle markdown-wrapped LLM responses - #11

Merged
pandarun merged 1 commit into
mainfrom
005-fix-json-parsing
Oct 16, 2025
Merged

pandarun merged 1 commit into
mainfrom
005-fix-json-parsing

Conversation

@pandarun

Copy link
Copy Markdown
Owner

Summary

This PR fixes a critical bug where the classification module fails to parse LLM responses when the JSON is wrapped in markdown code blocks (json... or ...). The Scibox LLM (Qwen2.5-72B-Instruct-AWQ) occasionally returns responses in this format, causing JSON decode errors.

Changes

Core Fix

  • Added strip_markdown_code_blocks() function to preprocess LLM responses before JSON parsing (src/classification/classifier.py:32-62)
  • Integrated stripping at the JSON parsing stage (classifier.py:138)
  • Handles both json and generic code block formats
  • Case-insensitive language specifier handling (JSON, json)
  • Fixed processing_time_ms validation to ensure minimum 1ms

Test Coverage

  • 9 unit tests in tests/unit/classification/test_markdown_stripping.py

    • Tests for ```json code blocks
    • Tests for generic ``` code blocks
    • Whitespace handling
    • Backward compatibility with unwrapped JSON
    • Uppercase language specifiers
    • Edge cases (incomplete markers, extra newlines)
  • 6 integration tests in tests/integration/classification/test_markdown_parsing_integration.py

    • Full classification flow with markdown-wrapped responses
    • Backward compatibility verification
    • Error handling with malformed JSON
    • Multiple format variations

All 15 tests passing ✅

Documentation

  • Complete feature specification (specs/005-fix-json-parsing/spec.md)
  • Implementation plan with constitution compliance (plan.md)
  • Research and design decisions (research.md)
  • Developer quickstart guide (quickstart.md)
  • Task breakdown with dependencies (tasks.md)

Technical Details

Implementation Approach:

  • Simple string methods (startswith/endswith) for performance (<1μs overhead vs 1-2μs for regex)
  • No new dependencies added
  • 100% backward compatible with existing unwrapped JSON responses
  • Follows TDD methodology (tests written first)

Performance:

  • <1ms parsing overhead (measured <1μs per call)
  • No impact on existing classification performance

Testing

# Run unit tests
pytest tests/unit/classification/test_markdown_stripping.py -v

# Run integration tests
pytest tests/integration/classification/test_markdown_parsing_integration.py -v

# All tests passing: 9 unit + 6 integration = 15/15 ✅

Checklist

  • Tests written first (TDD approach)
  • All tests passing
  • Backward compatibility verified
  • Performance validated (<1ms overhead)
  • Error handling tested
  • Documentation complete
  • Constitution compliance verified

Related Issues

Fixes the JSON parsing error:

[DEBUG] RAW LLM RESPONSE: ```json
{
  "category": "...",
  "subcategory": "...",
  "confidence": 0.95
}

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Add markdown code block stripping to classification module to handle
LLM responses that wrap JSON in ```json...``` or ```...``` markers.

Changes:
- Add strip_markdown_code_blocks() function to remove markdown markers
- Handle both ```json and generic ``` code blocks
- Case-insensitive language specifier handling (```JSON, ```json)
- Preserve backward compatibility with unwrapped JSON
- Fix processing_time_ms validation (ensure minimum 1ms)

Tests:
- 9 unit tests for markdown stripping function
- 6 integration tests for full classification flow
- All tests passing (15/15)
- Error handling verified with malformed JSON

Documentation:
- Complete feature specification (spec.md)
- Implementation plan with constitution compliance (plan.md)
- Research and design decisions (research.md)
- Developer quickstart guide (quickstart.md)
- Detailed task breakdown (tasks.md)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@claude

claude Bot commented Oct 16, 2025

Copy link
Copy Markdown

Pull Request Review: Fix JSON Parsing for Markdown-Wrapped LLM Responses

Summary

This PR successfully addresses a critical production bug where the classification module fails to parse LLM responses wrapped in markdown code blocks. The implementation is clean, well-tested, and maintains backward compatibility.

Overall Assessment: ✅ APPROVE with minor suggestions


Code Quality & Best Practices

Strengths ✅

  1. Clean Implementation (classifier.py:32-62)

    • The strip_markdown_code_blocks() function is simple, readable, and self-documenting
    • Excellent docstring with examples demonstrating usage
    • Function signature is clear with proper type hints
  2. Test-Driven Development

    • Comprehensive test coverage: 9 unit tests + 6 integration tests = 15 total
    • Tests were clearly written first (TDD approach evident)
    • Test names are descriptive and follow clear naming conventions
  3. Documentation Excellence

    • Exceptional documentation in specs/005-fix-json-parsing/
    • Includes research notes, implementation plan, and quickstart guide
    • Constitution compliance checks demonstrate architectural thinking

Potential Bugs & Issues

Critical: None identified - The implementation handles edge cases well.

Minor Observations:

  1. Processing time validation at line 180 ensures minimum 1ms - consider adding comment explaining why 0ms is invalid
  2. Edge case with multiple code blocks - current implementation strips only outer markers (acceptable, worth monitoring)

Performance Considerations

Excellent ⚡

  • Research notes document <0.2μs overhead (well under 1ms target)
  • No new dependencies (Python stdlib only)
  • Backward compatible with minimal overhead

Security Concerns

No Security Issues Identified ✅

  • Input sanitization already in place
  • Error handling doesn't expose sensitive data
  • API key management follows best practices

Test Coverage

Excellent Coverage 🧪

  • 9 unit tests covering all edge cases
  • 6 integration tests for full classification flow
  • Backward compatibility verified
  • Error handling tested

Suggestions for additional tests:

  1. Empty code block edge case
  2. Property-based testing with hypothesis (optional)

Architecture & Design

Strengths 🏗️

  1. Modular design - fix isolated to classification module
  2. Constitution compliance verified across all six principles
  3. Research-driven decision making (excellent decision matrix in research.md)
  4. Follows Wrapper/Adapter pattern appropriately

Documentation

Exceptional 📚
The specs/005-fix-json-parsing/ directory is impressively thorough. This level of documentation is rare and commendable. It will be invaluable for future maintainers and onboarding.


Recommendations

Before Merge:

  1. Run full test suite to ensure no regressions
  2. Manual validation with actual Scibox API
  3. Update CHANGELOG if maintained

Post-Merge:

  1. Monitor production logs for markdown patterns
  2. Consider adding telemetry to track frequency
  3. Update troubleshooting knowledge base

Conclusion

This is a high-quality pull request demonstrating:

  • ✅ Clear problem identification
  • ✅ Simple, effective solution
  • ✅ Comprehensive testing
  • ✅ Excellent documentation
  • ✅ Backward compatibility
  • ✅ Performance consciousness
  • ✅ Security awareness

Recommendation: APPROVE and MERGE ✅

The code is production-ready. Minor suggestions above are for enhancement, not blockers.


Review by Claude Code - Constitution compliance verified

@pandarun
pandarun merged commit bd6eba7 into main Oct 16, 2025
1 check passed
@pandarun
pandarun deleted the 005-fix-json-parsing branch October 16, 2025 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant