integrate claude code #4846
integrate claude code #4846
Conversation
dosubot
bot
added
size:XL
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
hannesrudolph
left a comment
hannesrudolph
left a comment
There was a problem hiding this comment.
Thanks for this contribution! The Claude Code CLI integration is a valuable addition that allows users to run Claude models locally without an API key.
Overall Assessment
The implementation follows the existing provider patterns well and includes comprehensive test coverage. The code quality is good, but there are several issues that need attention before merging.
Critical Issues
- Potential infinite loop in the main message processing loop
- Missing error handling for CLI binary execution failures
- Unused parameter in the API interface
Important Considerations
- Process cleanup: No mechanism to terminate Claude processes on cancellation
- Incomplete i18n: Only Japanese translations provided
- Unimplemented TODO: Session support mentioned but not implemented
Positive Aspects
✅ Well-structured code following existing patterns
✅ Comprehensive unit test coverage
✅ Good documentation
✅ Proper TypeScript typing
✅ Streaming response handling
Recommendations
- Fix the critical issues, especially the potential infinite loop
- Add error handling for CLI execution
- Complete i18n translations for all supported languages
- Consider implementing process cleanup for better resource management
- Either implement session support or remove the TODO comment
Once these issues are addressed, this will be a great addition to Roo Code!
| async *createMessage( | ||
| systemPrompt: string, | ||
| messages: Anthropic.Messages.MessageParam[], | ||
| metadata?: ApiHandlerCreateMessageMetadata, |
There was a problem hiding this comment.
Type Safety: The metadata parameter is accepted but never used. Is this intentional for API compatibility?
If it's not needed for Claude Code, consider either:
- Using it if there's relevant metadata to process
- Adding a comment explaining why it's unused
- Removing it if the interface allows
// @ts-expect-error - metadata not used for Claude Code CLI integration
metadata?: ApiHandlerCreateMessageMetadata,There was a problem hiding this comment.
Seems like it got readded on c5a846f934ca8fbc8bededcab6cc91f184289027
- Added claudeCodePath and claudeCodePathDescription translations - Covers all supported languages: ca, de, en, es, fr, hi, id, it, ja, ko, nl, pl, pt-BR, ru, tr, vi, zh-CN, zh-TW - Addresses PR review feedback
|
✅ Added missing translations for all locales I've implemented the Claude Code translations ( This addresses the i18n coverage issue from the review. |
dosubot
bot
added
size:XXL
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
|
@hannesrudolph |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
daniel-lxs
left a comment
daniel-lxs
left a comment
There was a problem hiding this comment.
Thank you @sahksas!
I've left a few comments on the PR with some suggestions for improvement. Overall, this is a great addition, and I appreciate the detailed implementation!
src/integrations/claude-code/run.ts
Outdated
| validateMessageContent(content) | ||
|
|
||
| promptText += `${role}: ${content}\n\n` | ||
| } |
There was a problem hiding this comment.
The current implementation converts messages to plain text format, but this seems inconsistent with the original approach.
The comment on line 117 states "Claude CLI doesn't accept JSON messages", but looking at the original implementation pattern, it appears the CLI should accept JSON-formatted messages. The current text conversion approach:
- Loses message structure and metadata
- Makes it harder to handle complex message types
- Differs from how other providers handle messages
Could you verify if the Claude Code CLI actually requires text format? If it does accept JSON, consider reverting to:
const args = [
"-p",
JSON.stringify(messages),
"--system-prompt",
systemPrompt,
// ... other args
]This would also simplify the validation logic since JSON.stringify handles escaping automatically.
There was a problem hiding this comment.
@daniel-ks You're absolutely right! I've reverted to using JSON format as suggested: ed046eb
- Removed the plain text conversion logic
- Now using the existing
safeSerializeMessagesfunction - Updated CLI arguments to properly pass JSON messages
Thanks for catching this inconsistency and providing the clear solution!
src/integrations/claude-code/run.ts
Outdated
| }) { | ||
| const claudePath = path || "claude" | ||
| const workspacePath = getCwd() | ||
| const sessionId = SessionManager.getSessionId(workspacePath) |
There was a problem hiding this comment.
Nice work on implementing the SessionManager, it looks solid overall. One thing though: the session ID you're generating isn't actually passed to the Claude CLI, so each call ends up being treated as a new conversation. That could impact:
- Performance (no context reuse)
- User experience (no conversation continuity)
It looks like session management wasn't added to Cline’s implementation yet. From their notes:
I didn't implement sessions because it seems overly complex.
We have to store session IDs somewhere within the tasks (or messages), and handle tasks where the user changes the provider.
On the other hand, the current implementation works well (at least with the simple tasks I've been giving it).
We can rethink this if users raise issues.
That said, you actually can get a task-specific ID from the metadata parameter in the createMessage method, and pass that to the Claude CLI. Something like this:
// In src/api/providers/claude-code.ts
async *createMessage(
systemPrompt: string,
messages: Anthropic.Messages.MessageParam[],
metadata?: ApiHandlerCreateMessageMetadata,
): ApiStream {
// The taskId is available here:
const taskId = metadata?.taskId
// You can then pass it to runClaudeCode
}The metadata type includes:
taskId: string- the unique task ID, which could double as a session IDmode?: string- current modesignal?: AbortSignal- cancellation support
So you've already got the foundation in place. You could either hook that task ID into the CLI call (if sessions are supported), or skip the SessionManager for now and revisit later if needed.
Let me know what you think!
There was a problem hiding this comment.
Thank you for your thoughtful review.
You're absolutely right that including session management implementation in this PR would make the scope too large. Even the basic state management and persistence would require careful design considerations, and when we factor in UX improvements like session state visualization and performance optimization, it becomes a project-wide change with significant impact.
I've created a separate issue (#4934 ) to address comprehensive session management, and we'll tackle that there.
For this PR, I'd like to limit the scope to implementing the foundational part of your suggestion (extracting taskId from metadata and passing it to Claude CLI).
There was a problem hiding this comment.
Thank you for your valuable review comments.
I apologize for my insufficient understanding and for creating an inappropriate issue.
I have implemented the improvements you suggested: c853ea0
- Functionality to use taskId as session identifier and pass it to Claude CLI's
-roption - Practical session management with safe error handling
This implementation should achieve efficient session management at the Claude CLI level.
Thank you for your technically accurate suggestions.
| role: "assistant" | ||
| model: string | ||
| content: ClaudeCodeContent[] | ||
| stop_reason: null |
There was a problem hiding this comment.
The stop_reason field is typed as null, but based on the error handling in claude-code.ts, it can also be a string value (e.g., "error").
This type definition could cause runtime errors. Consider updating to:
stop_reason: string | nullThis would properly handle cases where Claude returns error reasons or other stop conditions.
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Changes Requested]
in Roo Code Roadmap
hannesrudolph
added
PR - Changes Requested
and removed
PR - Needs Preliminary Review
labels
|
Excited for this integration! :--) |
|
IMO this PR shouldn't solely focus on a Claude Code integration, but also generalize it to support competing pieces of software like Codex |
github-project-automation
bot
moved this from PR [Changes Requested]
to Done
in Roo Code Roadmap
|
Thank you @sahksas for your contribution. It's genuinely appreciated, and I'm looking forward to more from you in the future. |
5 participants
Related GitHub Issue
Closes: #4842
Description
Test Procedure
Type of Change
srcor test files.Pre-Submission Checklist
npm run lint).console.log) has been removed.npm test).mainbranch.npm run changesetif this PR includes user-facing changes or dependency updates.Screenshots / Videos
Documentation Updates
Additional Notes
All tests have passed, but I have only been able to check the operation in my environment. If possible, please check it in another environment.
Get in Touch
Arigato~ 🙏🏻 cline/cline#4111
Important
Integrates Claude Code CLI into Roo Code, adding local execution support, configuration options, documentation, and tests.
ClaudeCodeHandlerto handle Claude Code CLI interactions insrc/api/providers/claude-code.ts.claudeCodePathconfiguration option inprovider-settings.tsfor specifying the CLI path.ApiOptions.tsxandClaudeCode.tsxto include Claude Code settings in the UI.claude-code-integration.mddetailing setup and usage of Claude Code CLI.ClaudeCodeHandlerinclaude-code.spec.tsand CLI execution inrun.spec.ts.This description was created by
for 51fe529. You can customize this summary. It will automatically update as commits are pushed.