Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Jun 23, 2025

Description

Fixes the Claude Code provider showing raw JSON output instead of properly parsed content. This PR addresses two key issues that were causing poor user experience with the Claude Code integration.

Changes Made

  • JSON Buffering Fix: Implemented proper line-based buffering in src/api/providers/claude-code.ts to handle incomplete data chunks that were causing JSON parsing failures
  • Reasoning Block Display: Changed thinking content to yield as "reasoning" type instead of "text" type so it renders as collapsible reasoning blocks in the UI
  • Type System Enhancement: Extended ClaudeCodeContent type in src/integrations/claude-code/types.ts to support both text and thinking content
  • Comprehensive Testing: Added full test suite in src/api/providers/__tests__/claude-code.spec.ts covering thinking content, mixed content, and error scenarios

Testing

  • All existing tests pass
  • Added comprehensive test coverage for Claude Code provider:
    • Thinking content properly yields as "reasoning" type
    • Mixed content types handled correctly
    • Error scenarios with thinking content
  • Manual testing completed:
    • JSON parsing no longer fails on incomplete chunks
    • Thinking content displays as collapsible reasoning blocks
    • No more raw JSON output in chat

Technical Details

JSON Buffering Implementation:

// Before: Direct JSON parsing caused failures on incomplete chunks
JSON.parse(data.toString())

// After: Line-based buffering handles incomplete data properly
lines.forEach(line => {
  if (line.trim()) {
    const parsed = JSON.parse(line)
    // Process parsed data...
  }
})

Reasoning Block Fix:

// Before: Thinking content displayed as raw text
if (content.type === "thinking") {
  yield { type: "text", text: content.thinking }
}

// After: Thinking content displays as collapsible reasoning blocks
if (content.type === "thinking") {
  yield { type: "reasoning", text: content.thinking }
}

Verification of Fix

  • UI Integration: Confirmed ChatRow.tsx has proper case "reasoning": handling that renders ReasoningBlock components
  • Type Safety: All TypeScript types properly support the new content structure
  • Backward Compatibility: No breaking changes to existing functionality

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Comments added for complex logic
  • All tests pass with comprehensive coverage
  • No breaking changes
  • ESLint warnings resolved
  • TypeScript checks pass

Important

Fix JSON parsing and reasoning block display in Claude Code provider by implementing line-based buffering and updating content types.

  • JSON Parsing:
    • Implement line-based buffering in claude-code.ts to handle incomplete JSON data.
    • Add isLikelyValidJSON() to validate JSON before processing.
  • Reasoning Block Display:
    • Change thinking content to yield as "reasoning" type in claude-code.ts.
    • Update ClaudeCodeContent type in types.ts to support thinking content.
  • Testing:
    • Add tests in claude-code.spec.ts for thinking content, mixed content, and error scenarios.
  • Misc:
    • Add getContentText() in claude-code.ts to extract text from content objects.
    • Improve error logging in attemptParseChunk() in claude-code.ts.

This description was created by Ellipsis for dd9e146. You can customize this summary. It will automatically update as commits are pushed.

…splay

- Implement proper JSON buffering to handle incomplete data chunks
- Change thinking content to yield as 'reasoning' type for proper UI display
- Add comprehensive tests for Claude Code provider functionality
- Fix raw JSON output appearing instead of parsed content
Copilot AI review requested due to automatic review settings June 23, 2025 17:35
@hannesrudolph hannesrudolph requested review from cte, jr and mrubens as code owners June 23, 2025 17:35
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 23, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request fixes the JSON buffering issue and corrects the display of thinking content by yielding it as a "reasoning" type, improving the integration with the Claude Code provider. The key changes include:

  • Implementing line-based buffering in src/api/providers/claude-code.ts to handle incomplete JSON chunks.
  • Modifying the thinking content handling to yield "reasoning" instead of "text" in src/api/providers/claude-code.ts.
  • Extending the ClaudeCodeContent type in src/integrations/claude-code/types.ts to support the new thinking content.
  • Adding comprehensive tests in src/api/providers/tests/claude-code.spec.ts to cover mixed, thinking, and error scenarios.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/integrations/claude-code/types.ts Extended ClaudeCodeContent union type to include thinking content.
src/api/providers/claude-code.ts Added buffering to process incoming data and updated reasoning block display.
src/api/providers/tests/claude-code.spec.ts Added tests to verify thinking content and mixed content handling.

@dosubot dosubot bot added the bug Something isn't working label Jun 23, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 23, 2025
}
} else {
console.warn("Unsupported content type:", content.type)
console.warn("Unsupported content type:", (content as any).type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type assertion necessary? Since you've already extended the ClaudeCodeContent type to include thinking content, TypeScript should handle this properly without the cast to any.

If there are other content types not covered in the type definition, would it be better to add them to the ClaudeCodeContent union type for better type safety?


// Emit process close after data
setTimeout(() => {
mockProcess.emit("close", 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests use hardcoded setTimeout delays which could be flaky in CI environments. Have you considered using a more deterministic approach? For example:

// Instead of setTimeout, use immediate execution
mockProcess.stdout.emit("data", JSON.stringify(thinkingResponse) + "\n")
mockProcess.emit("close", 0)

// Or use a test utility for controlled async behavior
await nextTick() // or similar

This would make the tests more reliable and faster to execute.

for (const line of lines) {
dataQueue.push(line)
if (line.trim() !== "") {
dataQueue.push(line.trim())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed an inconsistency in how empty lines are handled. Here you trim before checking if the line is empty, but in line 56 you check buffer.trim().

Would it be cleaner to handle this consistently? Perhaps:

// Process complete lines
for (const line of lines) {
  const trimmedLine = line.trim()
  if (trimmedLine !== "") {
    dataQueue.push(trimmedLine)
  }
}

And similarly for the buffer check in the close handler.

- Remove unnecessary type assertion for better type safety
- Add helper function for content text extraction
- Improve buffer trimming consistency
- Enhance error logging with data preview for debugging
- Replace setTimeout with setImmediate in tests for better reliability

Addresses review feedback to improve code quality and maintainability.
- Remove unnecessary type assertion for better type safety
- Add isLikelyValidJSON helper to validate JSON structure before processing
- Implement getContentText helper for cleaner error message extraction
- Standardize buffer trimming logic across the codebase
- Enhance error logging with data preview for better debugging
- Replace setTimeout with setImmediate for more reliable test timing
- Add test case for incomplete JSON handling on process close

These changes address all review feedback and improve the robustness
of JSON parsing in the Claude Code provider.
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 23, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Jun 23, 2025
@mrubens mrubens merged commit cabf191 into main Jun 23, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 23, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 23, 2025
@mrubens mrubens deleted the fix/claude-code-json-parsing-and-reasoning-blocks branch June 23, 2025 18:14
cte pushed a commit that referenced this pull request Jun 24, 2025
Alorse pushed a commit to Alorse/Roo-Code that referenced this pull request Jun 27, 2025
Alorse pushed a commit to Alorse/Roo-Code that referenced this pull request Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants