Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ export function buildApiHandler(configuration: ProviderSettings): ApiHandler {
case "io-intelligence":
return new IOIntelligenceHandler(options)
case "roo":
// Never throw exceptions from provider constructors
// The provider-proxy server will handle authentication and return appropriate error codes
return new RooHandler(options)
case "featherless":
return new FeatherlessHandler(options)
Expand Down
30 changes: 20 additions & 10 deletions src/api/providers/__tests__/roo.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,21 +131,25 @@ describe("RooHandler", () => {
expect(handler.getModel().id).toBe(mockOptions.apiModelId)
})

it("should throw error if CloudService is not available", () => {
it("should not throw error if CloudService is not available", () => {
mockHasInstanceFn.mockReturnValue(false)
expect(() => {
new RooHandler(mockOptions)
}).toThrow("Authentication required for Roo Code Cloud")
expect(t).toHaveBeenCalledWith("common:errors.roo.authenticationRequired")
}).not.toThrow()
// Constructor should succeed even without CloudService
const handler = new RooHandler(mockOptions)
expect(handler).toBeInstanceOf(RooHandler)
})

it("should throw error if session token is not available", () => {
it("should not throw error if session token is not available", () => {
mockHasInstanceFn.mockReturnValue(true)
mockGetSessionTokenFn.mockReturnValue(null)
expect(() => {
new RooHandler(mockOptions)
}).toThrow("Authentication required for Roo Code Cloud")
expect(t).toHaveBeenCalledWith("common:errors.roo.authenticationRequired")
}).not.toThrow()
// Constructor should succeed even without session token
const handler = new RooHandler(mockOptions)
expect(handler).toBeInstanceOf(RooHandler)
})

it("should initialize with default model if no model specified", () => {
Expand Down Expand Up @@ -400,7 +404,7 @@ describe("RooHandler", () => {
expect(mockGetSessionTokenFn).toHaveBeenCalled()
})

it("should handle undefined auth service", () => {
it("should handle undefined auth service gracefully", () => {
mockHasInstanceFn.mockReturnValue(true)
// Mock CloudService with undefined authService
const originalGetter = Object.getOwnPropertyDescriptor(CloudService, "instance")?.get
Expand All @@ -413,7 +417,10 @@ describe("RooHandler", () => {

expect(() => {
new RooHandler(mockOptions)
}).toThrow("Authentication required for Roo Code Cloud")
}).not.toThrow()
// Constructor should succeed even with undefined auth service
const handler = new RooHandler(mockOptions)
expect(handler).toBeInstanceOf(RooHandler)
} finally {
// Always restore original getter, even if test fails
if (originalGetter) {
Expand All @@ -425,12 +432,15 @@ describe("RooHandler", () => {
}
})

it("should handle empty session token", () => {
it("should handle empty session token gracefully", () => {
mockGetSessionTokenFn.mockReturnValue("")

expect(() => {
new RooHandler(mockOptions)
}).toThrow("Authentication required for Roo Code Cloud")
}).not.toThrow()
// Constructor should succeed even with empty session token
const handler = new RooHandler(mockOptions)
expect(handler).toBeInstanceOf(RooHandler)
})
})
})
18 changes: 8 additions & 10 deletions src/api/providers/roo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,27 @@ import { CloudService } from "@roo-code/cloud"

import type { ApiHandlerOptions } from "../../shared/api"
import { ApiStream } from "../transform/stream"
import { t } from "../../i18n"

import type { ApiHandlerCreateMessageMetadata } from "../index"
import { BaseOpenAiCompatibleProvider } from "./base-openai-compatible-provider"

export class RooHandler extends BaseOpenAiCompatibleProvider<RooModelId> {
constructor(options: ApiHandlerOptions) {
// Check if CloudService is available and get the session token.
if (!CloudService.hasInstance()) {
throw new Error(t("common:errors.roo.authenticationRequired"))
}

const sessionToken = CloudService.instance.authService?.getSessionToken()
// Get the session token if available, but don't throw if not.
// The server will handle authentication errors and return appropriate status codes.
let sessionToken = ""

if (!sessionToken) {
throw new Error(t("common:errors.roo.authenticationRequired"))
if (CloudService.hasInstance()) {
sessionToken = CloudService.instance.authService?.getSessionToken() || ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding debug logging when falling back to an empty token (without exposing the actual token value). This could help with debugging authentication issues.

}

// Always construct the handler, even without a valid token.
// The provider-proxy server will return 401 if authentication fails.
super({
...options,
providerName: "Roo Code Cloud",
baseURL: process.env.ROO_CODE_PROVIDER_URL ?? "https://api.roocode.com/proxy/v1",
apiKey: sessionToken,
apiKey: sessionToken || "unauthenticated", // Use a placeholder if no token
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the string "unauthenticated" as a placeholder API key could be misleading. Consider using an empty string or a more descriptive placeholder like "NO_AUTH_TOKEN" to make the intent clearer:

defaultProviderModelId: rooDefaultModelId,
providerModels: rooModels,
defaultTemperature: 0.7,
Expand Down
19 changes: 18 additions & 1 deletion src/core/webview/webviewMessageHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,24 @@ export const webviewMessageHandler = async (
// Initializing new instance of Cline will make sure that any
// agentically running promises in old instance don't affect our new
// task. This essentially creates a fresh slate for the new task.
await provider.createTask(message.text, message.images)
try {
await provider.createTask(message.text, message.images)
// Task created successfully - notify the UI to reset
await provider.postMessageToWebview({
type: "invoke",
invoke: "newChat",
})
} catch (error) {
// For all errors, reset the UI and show error
await provider.postMessageToWebview({
Copy link
Contributor

Choose a reason for hiding this comment

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

The error handling here catches all errors generically but still sends a "newChat" message on error, which might not be the intended behavior. Also, the error message shown to the user doesn't distinguish between authentication errors and other types of errors. Consider:

  1. Should we really send "newChat" on error?
  2. Could we provide more specific error messages based on the error type?

type: "invoke",
invoke: "newChat",
})
// Show error to user
vscode.window.showErrorMessage(
`Failed to create task: ${error instanceof Error ? error.message : String(error)}`,
)
}
break
case "customInstructions":
await provider.updateCustomInstructions(message.text)
Expand Down
Loading