Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 215 additions & 0 deletions src/api/providers/__tests__/claude-code.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
import { describe, test, expect, vi, beforeEach } from "vitest"
import { ClaudeCodeHandler } from "../claude-code"
import { ApiHandlerOptions } from "../../../shared/api"

// Mock the runClaudeCode function
vi.mock("../../../integrations/claude-code/run", () => ({
runClaudeCode: vi.fn(),
}))

const { runClaudeCode } = await import("../../../integrations/claude-code/run")
const mockRunClaudeCode = vi.mocked(runClaudeCode)

// Mock the EventEmitter for the process
class MockEventEmitter {
private handlers: { [event: string]: ((...args: any[]) => void)[] } = {}

on(event: string, handler: (...args: any[]) => void) {
if (!this.handlers[event]) {
this.handlers[event] = []
}
this.handlers[event].push(handler)
}

emit(event: string, ...args: any[]) {
if (this.handlers[event]) {
this.handlers[event].forEach((handler) => handler(...args))
}
}
}

describe("ClaudeCodeHandler", () => {
let handler: ClaudeCodeHandler
let mockProcess: any

beforeEach(() => {
const options: ApiHandlerOptions = {
claudeCodePath: "claude",
apiModelId: "claude-3-5-sonnet-20241022",
}
handler = new ClaudeCodeHandler(options)

const mainEmitter = new MockEventEmitter()
mockProcess = {
stdout: new MockEventEmitter(),
stderr: new MockEventEmitter(),
on: mainEmitter.on.bind(mainEmitter),
emit: mainEmitter.emit.bind(mainEmitter),
}

mockRunClaudeCode.mockReturnValue(mockProcess)
})

test("should handle thinking content properly", async () => {
const systemPrompt = "You are a helpful assistant"
const messages = [{ role: "user" as const, content: "Hello" }]

// Start the stream
const stream = handler.createMessage(systemPrompt, messages)
const streamGenerator = stream[Symbol.asyncIterator]()

// Simulate thinking content response
const thinkingResponse = {
type: "assistant",
message: {
id: "msg_123",
type: "message",
role: "assistant",
model: "claude-3-5-sonnet-20241022",
content: [
{
type: "thinking",
thinking: "I need to think about this carefully...",
signature: "abc123",
},
],
stop_reason: null,
stop_sequence: null,
usage: {
input_tokens: 10,
output_tokens: 20,
service_tier: "standard" as const,
},
},
session_id: "session_123",
}

// Emit the thinking response and wait for processing
setTimeout(() => {
mockProcess.stdout.emit("data", JSON.stringify(thinkingResponse) + "\n")
}, 10)

// 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.

}, 50)

// Get the result
const result = await streamGenerator.next()

expect(result.done).toBe(false)
expect(result.value).toEqual({
type: "reasoning",
text: "I need to think about this carefully...",
})
})

test("should handle mixed content types", async () => {
const systemPrompt = "You are a helpful assistant"
const messages = [{ role: "user" as const, content: "Hello" }]

const stream = handler.createMessage(systemPrompt, messages)
const streamGenerator = stream[Symbol.asyncIterator]()

// Simulate mixed content response
const mixedResponse = {
type: "assistant",
message: {
id: "msg_123",
type: "message",
role: "assistant",
model: "claude-3-5-sonnet-20241022",
content: [
{
type: "thinking",
thinking: "Let me think about this...",
},
{
type: "text",
text: "Here's my response!",
},
],
stop_reason: null,
stop_sequence: null,
usage: {
input_tokens: 10,
output_tokens: 20,
service_tier: "standard" as const,
},
},
session_id: "session_123",
}

// Emit the mixed response and wait for processing
setTimeout(() => {
mockProcess.stdout.emit("data", JSON.stringify(mixedResponse) + "\n")
}, 10)

// Emit process close after data
setTimeout(() => {
mockProcess.emit("close", 0)
}, 50)

// Get the first result (thinking)
const thinkingResult = await streamGenerator.next()
expect(thinkingResult.done).toBe(false)
expect(thinkingResult.value).toEqual({
type: "reasoning",
text: "Let me think about this...",
})

// Get the second result (text)
const textResult = await streamGenerator.next()
expect(textResult.done).toBe(false)
expect(textResult.value).toEqual({
type: "text",
text: "Here's my response!",
})
})

test("should handle stop_reason with thinking content in error messages", async () => {
const systemPrompt = "You are a helpful assistant"
const messages = [{ role: "user" as const, content: "Hello" }]

const stream = handler.createMessage(systemPrompt, messages)
const streamGenerator = stream[Symbol.asyncIterator]()

// Simulate error response with thinking content
const errorResponse = {
type: "assistant",
message: {
id: "msg_123",
type: "message",
role: "assistant",
model: "claude-3-5-sonnet-20241022",
content: [
{
type: "thinking",
thinking: "This is an error scenario",
},
],
stop_reason: "max_tokens",
stop_sequence: null,
usage: {
input_tokens: 10,
output_tokens: 20,
service_tier: "standard" as const,
},
},
session_id: "session_123",
}

// Emit the error response and wait for processing
setTimeout(() => {
mockProcess.stdout.emit("data", JSON.stringify(errorResponse) + "\n")
}, 10)

// Emit process close after data
setTimeout(() => {
mockProcess.emit("close", 0)
}, 50)

// Should throw error with thinking content
await expect(streamGenerator.next()).rejects.toThrow("This is an error scenario")
})
})
32 changes: 27 additions & 5 deletions src/api/providers/claude-code.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,20 @@ export class ClaudeCodeHandler extends BaseProvider implements ApiHandler {
let processError = null
let errorOutput = ""
let exitCode: number | null = null
let buffer = ""

claudeProcess.stdout.on("data", (data) => {
const output = data.toString()
const lines = output.split("\n").filter((line: string) => line.trim() !== "")
buffer += data.toString()
const lines = buffer.split("\n")

// Keep the last line in buffer as it might be incomplete
buffer = lines.pop() || ""

// Process complete lines
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.

}
}
})

Expand All @@ -44,6 +51,11 @@ export class ClaudeCodeHandler extends BaseProvider implements ApiHandler {

claudeProcess.on("close", (code) => {
exitCode = code
// Process any remaining data in buffer
if (buffer.trim()) {
dataQueue.push(buffer.trim())
buffer = ""
}
})

claudeProcess.on("error", (error) => {
Expand Down Expand Up @@ -101,8 +113,13 @@ export class ClaudeCodeHandler extends BaseProvider implements ApiHandler {
const message = chunk.message

if (message.stop_reason !== null && message.stop_reason !== "tool_use") {
const firstContent = message.content[0]
const errorMessage =
message.content[0]?.text ||
(firstContent?.type === "text"
? firstContent.text
: firstContent?.type === "thinking"
? firstContent.thinking
: undefined) ||
t("common:errors.claudeCode.stoppedWithReason", { reason: message.stop_reason })

if (errorMessage.includes("Invalid model name")) {
Expand All @@ -118,8 +135,13 @@ export class ClaudeCodeHandler extends BaseProvider implements ApiHandler {
type: "text",
text: content.text,
}
} else if (content.type === "thinking") {
yield {
type: "reasoning",
text: content.thinking,
}
} 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?

}
}

Expand Down
14 changes: 10 additions & 4 deletions src/integrations/claude-code/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@ type InitMessage = {
mcp_servers: string[]
}

type ClaudeCodeContent = {
type: "text"
text: string
}
type ClaudeCodeContent =
| {
type: "text"
text: string
}
| {
type: "thinking"
thinking: string
signature?: string
}

type AssistantMessage = {
type: "assistant"
Expand Down