Skip to content

feat(lmstudio): Add Harmony format parser for GPT-OSS models#6

Merged
nachoal merged 1 commit into
mainfrom
feature/gpt-oss-harmony-support
Aug 5, 2025
Merged

feat(lmstudio): Add Harmony format parser for GPT-OSS models#6
nachoal merged 1 commit into
mainfrom
feature/gpt-oss-harmony-support

Conversation

@nachoal

@nachoal nachoal commented Aug 5, 2025

Copy link
Copy Markdown
Owner
  • Implement Harmony channel parser to extract analysis, final, and commentary
  • Parse tool calls from commentary channel to standard JSON format
  • Handle multiline markdown content in final channel responses
  • Remove GPT-OSS tool restrictions as Harmony parser enables full support
  • Add comprehensive tests for Harmony format parsing
  • Document the issue and solution in GPT-OSS-HARMONY-ISSUE.md

This allows GPT-OSS models to work properly with tool calling through LM Studio by parsing their native Harmony output format.

🤖 Generated with Claude Code

- Implement Harmony channel parser to extract analysis, final, and commentary
- Parse tool calls from commentary channel to standard JSON format
- Handle multiline markdown content in final channel responses
- Remove GPT-OSS tool restrictions as Harmony parser enables full support
- Add comprehensive tests for Harmony format parsing
- Document the issue and solution in GPT-OSS-HARMONY-ISSUE.md

This allows GPT-OSS models to work properly with tool calling through
LM Studio by parsing their native Harmony output format.

🤖 Generated with Claude Code

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

claude Bot commented Aug 5, 2025

Copy link
Copy Markdown

Claude finished @nachoal's task —— View job


PR Review: Harmony Format Parser for GPT-OSS Models

Review Tasks:

  • Read repository guidelines (CLAUDE.md)
  • Analyze main.go changes
  • Review new documentation (GPT-OSS-HARMONY-ISSUE.md)
  • Examine LM Studio client modifications
  • Analyze Harmony parser implementation
  • Review test coverage and quality
  • Check for potential bugs and security issues
  • Evaluate performance considerations
  • Provide overall feedback and recommendations

Overall Assessment: Excellent Implementation

This PR successfully addresses a specific and complex problem with GPT-OSS models outputting Harmony format that breaks standard parsers. The implementation is well-architected, thoroughly tested, and follows Go best practices.

Code Quality & Best Practices

Strengths:

  • Clean Architecture: Parser is properly isolated in its own module (harmony.go)
  • Comprehensive Testing: 246 lines of tests covering edge cases and real-world scenarios
  • Robust Error Handling: Graceful fallbacks when parsing fails
  • Go Idioms: Proper use of regex, interfaces, and error handling patterns
  • Documentation: Excellent problem description and implementation rationale

Minor Improvements:

  1. Regex Performance (harmony.go:39): Consider compiling regexes as package-level variables to avoid recompilation:

    var commentaryRe = regexp.MustCompile(`(?s)<\|channel\|>commentary\s+to=functions\.(\w+).*?<\|message\|>(.*?)(?:<\|call\|>|<\|end\|>|$)`)
  2. JSON Validation (harmony.go:60-72): The double parse/marshal for JSON validation is robust but could be optimized:

    // Current approach is safe but consider caching valid JSON

Security Considerations

Well Handled:

  • Input Sanitization: All user input is properly escaped in regex patterns
  • JSON Safety: Safe JSON parsing with proper error handling
  • No Code Injection: No dynamic code execution or unsafe reflection

No Security Issues Found

Performance Considerations

Good Practices:

  • Lazy Evaluation: Harmony parsing only triggers for GPT-OSS models
  • Efficient Regex: Uses compiled patterns with appropriate flags
  • Memory Management: No memory leaks in streaming operations

Potential Optimizations:

  1. Regex Caching: Compile patterns once at package level
  2. String Builder: For large content manipulation, consider strings.Builder

Test Coverage Analysis

Excellent Coverage (95%+ estimated):

  • Happy Path: Simple responses, tool calls, multiline content
  • Edge Cases: Empty responses, malformed input, multiple channels
  • Real-World: Actual GPT-OSS output examples
  • Integration: Tests both parsing and conversion functions

Test Quality Highlights:

  • Multiline markdown handling (harmony_test.go:176-209)
  • JSON argument validation with normalization
  • Real-world example validation (harmony_test.go:211-246)

Technical Implementation Review

Harmony Parser (harmony.go):

  • Regex Patterns: Well-designed for multiline content with (?s) flag
  • Channel Extraction: Proper handling of analysis, final, and commentary channels
  • Tool Call Conversion: Correct mapping to OpenAI format
  • Error Recovery: Graceful handling of malformed input

Client Integration (client.go:156-204):

  • Detection Logic: Smart GPT-OSS model detection
  • Response Transformation: Clean integration with existing response flow
  • Debug Support: Comprehensive logging for troubleshooting
  • Backward Compatibility: No impact on non-GPT-OSS models

Main.go Changes:

  • Clean Removal: Properly removed GPT-OSS restrictions
  • Comments: Clear documentation of why restrictions were removed

Potential Bugs & Issues

Minor Edge Cases:

  1. Empty Tool Args (harmony.go:58): Consider handling "" args explicitly
  2. Malformed JSON: Current handling is good, but could log warnings
  3. Large Responses: No size limits on channel content (probably fine for this use case)

No Critical Issues Found

Documentation Quality

The GPT-OSS-HARMONY-ISSUE.md is outstanding:

  • Clear problem statement with examples
  • Technical background and root cause analysis
  • Implementation approach with code samples
  • Testing instructions and expected outcomes

Integration & Architecture

Excellent Design Decisions:

  • Provider-Specific: Parser only affects LM Studio provider
  • Non-Breaking: Zero impact on existing functionality
  • Extensible: Easy to add more channel types or modify parsing logic
  • Testable: Clean separation allows comprehensive testing

Recommendations

Before Merge:

  1. Consider pre-compiling regex patterns for better performance
  2. Add size limits for very large responses (optional)

Future Enhancements:

  1. Metrics: Track Harmony parsing success/failure rates for monitoring
  2. Configuration: Allow users to disable Harmony parsing if needed
  3. Streaming Support: Consider Harmony parsing for streaming responses

Summary

This is a high-quality implementation that solves a real problem elegantly. The code is well-tested, properly documented, and follows Go best practices. The parser correctly handles the complex Harmony format while maintaining backward compatibility.

Recommendation: Approve and merge

The implementation demonstrates excellent engineering practices and will significantly improve GPT-OSS model compatibility with simple-agent-go.


@nachoal nachoal merged commit 928d516 into main Aug 5, 2025
2 checks passed
@nachoal nachoal deleted the feature/gpt-oss-harmony-support branch August 5, 2025 23:03
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