diff --git a/packages/types/src/providers/anthropic.ts b/packages/types/src/providers/anthropic.ts index 5fbf62d507..766869c7e6 100644 --- a/packages/types/src/providers/anthropic.ts +++ b/packages/types/src/providers/anthropic.ts @@ -3,7 +3,7 @@ import type { ModelInfo } from "../model.js" // https://docs.anthropic.com/en/docs/about-claude/models export type AnthropicModelId = keyof typeof anthropicModels -export const anthropicDefaultModelId: AnthropicModelId = "claude-sonnet-4-20250514" +export const anthropicDefaultModelId: AnthropicModelId = "claude-sonnet-4-5" export const anthropicModels = { "claude-sonnet-4-5": { @@ -49,7 +49,7 @@ export const anthropicModels = { ], }, "claude-opus-4-1-20250805": { - maxTokens: 8192, + maxTokens: 32_000, // Overridden to 8k if `enableReasoningEffort` is false. contextWindow: 200_000, supportsImages: true, supportsPromptCache: true, diff --git a/packages/types/src/providers/bedrock.ts b/packages/types/src/providers/bedrock.ts index 9935d90b12..170a1ec6ac 100644 --- a/packages/types/src/providers/bedrock.ts +++ b/packages/types/src/providers/bedrock.ts @@ -4,7 +4,7 @@ import type { ModelInfo } from "../model.js" export type BedrockModelId = keyof typeof bedrockModels -export const bedrockDefaultModelId: BedrockModelId = "anthropic.claude-sonnet-4-20250514-v1:0" +export const bedrockDefaultModelId: BedrockModelId = "anthropic.claude-sonnet-4-5-20250929-v1:0" export const bedrockDefaultPromptRouterModelId: BedrockModelId = "anthropic.claude-3-sonnet-20240229-v1:0" diff --git a/packages/types/src/providers/cerebras.ts b/packages/types/src/providers/cerebras.ts index d578db0ddc..a027d6e853 100644 --- a/packages/types/src/providers/cerebras.ts +++ b/packages/types/src/providers/cerebras.ts @@ -7,13 +7,13 @@ export const cerebrasDefaultModelId: CerebrasModelId = "gpt-oss-120b" export const cerebrasModels = { "zai-glm-4.6": { - maxTokens: 16_384, - contextWindow: 128000, + maxTokens: 16384, // consistent with their other models + contextWindow: 131072, supportsImages: false, supportsPromptCache: false, inputPrice: 0, outputPrice: 0, - description: "Highly intelligent general-purpose model with ~2000 tokens/s", + description: "Highly intelligent general purpose model with up to 1,000 tokens/s", }, "qwen-3-coder-480b-free": { maxTokens: 40000, diff --git a/packages/types/src/providers/claude-code.ts b/packages/types/src/providers/claude-code.ts index d0b0f680af..c0982889a7 100644 --- a/packages/types/src/providers/claude-code.ts +++ b/packages/types/src/providers/claude-code.ts @@ -21,7 +21,7 @@ export function convertModelNameForVertex(modelName: string): string { // Claude Code export type ClaudeCodeModelId = keyof typeof claudeCodeModels -export const claudeCodeDefaultModelId: ClaudeCodeModelId = "claude-sonnet-4-20250514" +export const claudeCodeDefaultModelId: ClaudeCodeModelId = "claude-sonnet-4-5" export const CLAUDE_CODE_DEFAULT_MAX_OUTPUT_TOKENS = 16000 /** diff --git a/packages/types/src/providers/openrouter.ts b/packages/types/src/providers/openrouter.ts index 3a77ba14fc..e151570796 100644 --- a/packages/types/src/providers/openrouter.ts +++ b/packages/types/src/providers/openrouter.ts @@ -1,7 +1,7 @@ import type { ModelInfo } from "../model.js" // https://openrouter.ai/models?order=newest&supported_parameters=tools -export const openRouterDefaultModelId = "anthropic/claude-sonnet-4" +export const openRouterDefaultModelId = "anthropic/claude-sonnet-4.5" export const openRouterDefaultModelInfo: ModelInfo = { maxTokens: 8192, diff --git a/packages/types/src/providers/unbound.ts b/packages/types/src/providers/unbound.ts index cc73f420d1..9715b835c9 100644 --- a/packages/types/src/providers/unbound.ts +++ b/packages/types/src/providers/unbound.ts @@ -1,6 +1,6 @@ import type { ModelInfo } from "../model.js" -export const unboundDefaultModelId = "anthropic/claude-3-7-sonnet-20250219" +export const unboundDefaultModelId = "anthropic/claude-sonnet-4-5" export const unboundDefaultModelInfo: ModelInfo = { maxTokens: 8192, diff --git a/packages/types/src/providers/zgsm.ts b/packages/types/src/providers/zgsm.ts index 27ca2a9bbb..68576110e8 100644 --- a/packages/types/src/providers/zgsm.ts +++ b/packages/types/src/providers/zgsm.ts @@ -1,6 +1,6 @@ import { ModelInfo } from "../model.js" -export const zgsmDefaultModelId = "GLM-4.5" +export const zgsmDefaultModelId = "GLM-4.6" export const zgsmModels = { default: { diff --git a/src/api/providers/__tests__/claude-code.spec.ts b/src/api/providers/__tests__/claude-code.spec.ts index 8d154d794f..4137543202 100644 --- a/src/api/providers/__tests__/claude-code.spec.ts +++ b/src/api/providers/__tests__/claude-code.spec.ts @@ -44,7 +44,7 @@ describe("ClaudeCodeHandler", () => { const handlerWithInvalidModel = new ClaudeCodeHandler(options) const model = handlerWithInvalidModel.getModel() - expect(model.id).toBe("claude-sonnet-4-20250514") // default model + expect(model.id).toBe("claude-sonnet-4-5") // default model }) test("should override maxTokens when claudeCodeMaxOutputTokens is provided", () => { @@ -69,7 +69,7 @@ describe("ClaudeCodeHandler", () => { const handlerWithMaxTokens = new ClaudeCodeHandler(options) const model = handlerWithMaxTokens.getModel() - expect(model.id).toBe("claude-sonnet-4-20250514") // default model + expect(model.id).toBe("claude-sonnet-4-5") // default model expect(model.info.maxTokens).toBe(16384) // Should use the configured value }) diff --git a/src/api/providers/__tests__/openrouter.spec.ts b/src/api/providers/__tests__/openrouter.spec.ts index 24ddd066ca..5e4184e81b 100644 --- a/src/api/providers/__tests__/openrouter.spec.ts +++ b/src/api/providers/__tests__/openrouter.spec.ts @@ -66,6 +66,18 @@ vitest.mock("../fetchers/modelCache", () => ({ description: "Claude 3.7 Sonnet", thinking: false, }, + "anthropic/claude-sonnet-4.5": { + maxTokens: 8192, + contextWindow: 200000, + supportsImages: true, + supportsPromptCache: true, + inputPrice: 3, + outputPrice: 15, + cacheWritesPrice: 3.75, + cacheReadsPrice: 0.3, + description: "Claude 4.5 Sonnet", + thinking: false, + }, "anthropic/claude-3.7-sonnet:thinking": { maxTokens: 128000, contextWindow: 200000, @@ -121,7 +133,7 @@ describe("OpenRouterHandler", () => { it("returns default model info when options are not provided", async () => { const handler = new OpenRouterHandler({}) const result = await handler.fetchModel() - expect(result.id).toBe("anthropic/claude-sonnet-4") + expect(result.id).toBe("anthropic/claude-sonnet-4.5") expect(result.info.supportsPromptCache).toBe(true) }) diff --git a/src/api/providers/__tests__/unbound.spec.ts b/src/api/providers/__tests__/unbound.spec.ts index 46a539ee9c..622be66310 100644 --- a/src/api/providers/__tests__/unbound.spec.ts +++ b/src/api/providers/__tests__/unbound.spec.ts @@ -22,6 +22,18 @@ vitest.mock("../fetchers/modelCache", () => ({ description: "Claude 3.5 Sonnet", thinking: false, }, + "anthropic/claude-sonnet-4-5": { + maxTokens: 8192, + contextWindow: 200000, + supportsImages: true, + supportsPromptCache: true, + inputPrice: 3, + outputPrice: 15, + cacheWritesPrice: 3.75, + cacheReadsPrice: 0.3, + description: "Claude 4.5 Sonnet", + thinking: false, + }, "anthropic/claude-3-7-sonnet-20250219": { maxTokens: 8192, contextWindow: 200000, @@ -312,7 +324,7 @@ describe("UnboundHandler", () => { it("should return default model when invalid model provided", async () => { const handlerWithInvalidModel = new UnboundHandler({ ...mockOptions, unboundModelId: "invalid/model" }) const modelInfo = await handlerWithInvalidModel.fetchModel() - expect(modelInfo.id).toBe("anthropic/claude-3-7-sonnet-20250219") + expect(modelInfo.id).toBe("anthropic/claude-sonnet-4-5") expect(modelInfo.info).toBeDefined() }) }) diff --git a/src/api/providers/zgsm.ts b/src/api/providers/zgsm.ts index 4e5766d4fd..7f6c9282eb 100644 --- a/src/api/providers/zgsm.ts +++ b/src/api/providers/zgsm.ts @@ -172,6 +172,7 @@ export class ZgsmAiHandler extends BaseProvider implements SingleCompletionHandl Object.assign(requestOptions, { extra_body: { prompt_mode: "strict", + mode: metadata?.mode, }, }) } diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 0be2608152..b3969b0f2c 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -750,9 +750,8 @@ export class Task extends EventEmitter implements TaskLike { // deallocated. (Although we set Cline = undefined in provider, that // simply removes the reference to this instance, but the instance is // still alive until this promise resolves or rejects.) - // Exception: Allow resume asks even when aborted for soft-interrupt UX - if (this.abort && type !== "resume_task" && type !== "resume_completed_task") { - throw new Error(`[CoStrict#ask] task ${this.taskId}.${this.instanceId} aborted`) + if (this.abort) { + throw new Error(`[RooCode#ask] task ${this.taskId}.${this.instanceId} aborted`) } let askTs: number @@ -1289,7 +1288,7 @@ export class Task extends EventEmitter implements TaskLike { ]) } - public async resumeTaskFromHistory() { + private async resumeTaskFromHistory() { // if (this.enableBridge) { // try { // await BridgeOrchestrator.subscribeToTask(this) @@ -1381,13 +1380,6 @@ export class Task extends EventEmitter implements TaskLike { const { response, text, images } = await this.ask(askType) // Calls `postStateToWebview`. - // Reset abort flags AFTER user responds to resume ask. - // This is critical for the cancel → resume flow: when a task is soft-aborted - // (abandoned = false), we keep the instance alive but set abort = true. - // We only clear these flags after the user confirms they want to resume, - // preventing the old stream from continuing if abort was set. - this.resetAbortAndStreamingState() - let responseText: string | undefined let responseImages: string[] | undefined @@ -1546,86 +1538,6 @@ export class Task extends EventEmitter implements TaskLike { await this.initiateTaskLoop(newUserContent) } - /** - * Resets abort flags and streaming state to allow task resumption. - * Centralizes the state reset logic used after user confirms task resumption. - * - * @private - */ - private resetAbortAndStreamingState(): void { - this.abort = false - this.abandoned = false - this.abortReason = undefined - this.didFinishAbortingStream = false - this.isStreaming = false - - // Reset streaming-local fields to avoid stale state from previous stream - this.currentStreamingContentIndex = 0 - this.currentStreamingDidCheckpoint = false - this.assistantMessageContent = [] - this.didCompleteReadingStream = false - this.userMessageContent = [] - this.userMessageContentReady = false - this.didRejectTool = false - this.didAlreadyUseTool = false - this.presentAssistantMessageLocked = false - this.presentAssistantMessageHasPendingUpdates = false - this.assistantMessageParser.reset() - } - - /** - * Present a resumable ask on an aborted task without rehydrating. - * Used by soft-interrupt (cancelTask) to show Resume/Terminate UI. - * Selects the appropriate ask type based on the last relevant message. - * If the user clicks Resume, resets abort flags and continues the task loop. - */ - public async presentResumableAsk(): Promise { - const lastClineMessage = this.clineMessages - .slice() - .reverse() - .find((m) => !(m.ask === "resume_task" || m.ask === "resume_completed_task")) - - let askType: ClineAsk - if (lastClineMessage?.ask === "completion_result") { - askType = "resume_completed_task" - } else { - askType = "resume_task" - } - - const { response, text, images } = await this.ask(askType) - - // If user clicked Resume (not Terminate), reset abort flags and continue - if (response === "yesButtonClicked" || response === "messageResponse") { - // Reset abort flags to allow the loop to continue - this.resetAbortAndStreamingState() - - // Prepare content for resuming the task loop - let userContent: Anthropic.Messages.ContentBlockParam[] = [] - - if (response === "messageResponse" && text) { - // User provided additional instructions - await this.say("user_feedback", text, images) - userContent.push({ - type: "text", - text: `\n\nNew instructions for task continuation:\n\n${text}\n`, - }) - if (images && images.length > 0) { - userContent.push(...formatResponse.imageBlocks(images)) - } - } else { - // Simple resume with no new instructions - userContent.push({ - type: "text", - text: "[TASK RESUMPTION] Resuming task...", - }) - } - - // Continue the task loop - await this.initiateTaskLoop(userContent) - } - // If user clicked Terminate (noButtonClicked), do nothing - task stays aborted - } - public async abortTask(isAbandoned = false) { // Aborting task @@ -1637,17 +1549,12 @@ export class Task extends EventEmitter implements TaskLike { this.abort = true this.emit(RooCodeEventName.TaskAborted) - // Only dispose if this is a hard abort (abandoned) - // For soft abort (user cancel), keep the instance alive so we can present a resumable ask - if (isAbandoned) { - try { - this.dispose() // Call the centralized dispose method - } catch (error) { - console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) - // Don't rethrow - we want abort to always succeed - } + try { + this.dispose() // Call the centralized dispose method + } catch (error) { + console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) + // Don't rethrow - we want abort to always succeed } - // Save the countdown message in the automatic retry or other content. try { // Save the countdown message in the automatic retry or other content. diff --git a/src/core/task/__tests__/Task.presentResumableAsk.abort-reset.spec.ts b/src/core/task/__tests__/Task.presentResumableAsk.abort-reset.spec.ts deleted file mode 100644 index f37cfeccf8..0000000000 --- a/src/core/task/__tests__/Task.presentResumableAsk.abort-reset.spec.ts +++ /dev/null @@ -1,146 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" -import { ProviderSettings } from "@roo-code/types" - -import { Task } from "../Task" -import { ClineProvider } from "../../webview/ClineProvider" - -// Mocks similar to Task.dispose.test.ts -vi.mock("../../webview/ClineProvider") -vi.mock("../../../integrations/terminal/TerminalRegistry", () => ({ - TerminalRegistry: { - releaseTerminalsForTask: vi.fn(), - }, -})) -vi.mock("../../ignore/RooIgnoreController") -vi.mock("../../protect/RooProtectedController") -vi.mock("../../context-tracking/FileContextTracker") -vi.mock("../../../services/browser/UrlContentFetcher") -vi.mock("../../../services/browser/BrowserSession") -vi.mock("../../../integrations/editor/DiffViewProvider") -vi.mock("../../tools/ToolRepetitionDetector") -vi.mock("../../../api", () => ({ - buildApiHandler: vi.fn(() => ({ - getModel: () => ({ info: {}, id: "test-model" }), - })), -})) -vi.mock("../AutoApprovalHandler") - -// Mock TelemetryService -vi.mock("@roo-code/telemetry", () => ({ - TelemetryService: { - instance: { - captureTaskCreated: vi.fn(), - captureTaskRestarted: vi.fn(), - captureConversationMessage: vi.fn(), - }, - }, -})) - -describe("Task.presentResumableAsk abort reset", () => { - let mockProvider: any - let mockApiConfiguration: ProviderSettings - let task: Task - - beforeEach(() => { - vi.clearAllMocks() - - mockProvider = { - context: { - globalStorageUri: { fsPath: "/test/path" }, - }, - getState: vi.fn().mockResolvedValue({ mode: "code" }), - postStateToWebview: vi.fn().mockResolvedValue(undefined), - postMessageToWebview: vi.fn().mockResolvedValue(undefined), - updateTaskHistory: vi.fn().mockResolvedValue(undefined), - log: vi.fn(), - } - - mockApiConfiguration = { - apiProvider: "anthropic", - apiKey: "test-key", - } as ProviderSettings - - task = new Task({ - provider: mockProvider as ClineProvider, - apiConfiguration: mockApiConfiguration, - startTask: false, - }) - }) - - afterEach(() => { - // Ensure we don't leave event listeners dangling - task.dispose() - }) - - it("resets abort flags and continues the loop on yesButtonClicked", async () => { - // Arrange aborted state - task.abort = true - task.abortReason = "user_cancelled" - task.didFinishAbortingStream = true - task.isStreaming = true - - // minimal message history - task.clineMessages = [{ ts: Date.now() - 1000, type: "say", say: "text", text: "prev" } as any] - - // Spy and stub ask + loop - const askSpy = vi.spyOn(task as any, "ask").mockResolvedValue({ response: "yesButtonClicked" }) - const loopSpy = vi.spyOn(task as any, "initiateTaskLoop").mockResolvedValue(undefined) - - // Act - await task.presentResumableAsk() - - // Assert ask was presented - expect(askSpy).toHaveBeenCalled() - - // Abort flags cleared - expect(task.abort).toBe(false) - expect(task.abandoned).toBe(false) - expect(task.abortReason).toBeUndefined() - expect(task.didFinishAbortingStream).toBe(false) - expect(task.isStreaming).toBe(false) - - // Streaming-local state cleared - expect(task.currentStreamingContentIndex).toBe(0) - expect(task.assistantMessageContent).toEqual([]) - expect(task.userMessageContentReady).toBe(false) - expect(task.didRejectTool).toBe(false) - expect(task.presentAssistantMessageLocked).toBe(false) - - // Loop resumed - expect(loopSpy).toHaveBeenCalledTimes(1) - }) - - it("includes user feedback when resuming with messageResponse", async () => { - task.abort = true - task.clineMessages = [{ ts: Date.now() - 1000, type: "say", say: "text", text: "prev" } as any] - - const askSpy = vi - .spyOn(task as any, "ask") - .mockResolvedValue({ response: "messageResponse", text: "Continue with this", images: undefined }) - const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined as any) - const loopSpy = vi.spyOn(task as any, "initiateTaskLoop").mockResolvedValue(undefined) - - await task.presentResumableAsk() - - expect(askSpy).toHaveBeenCalled() - expect(saySpy).toHaveBeenCalledWith("user_feedback", "Continue with this", undefined) - expect(loopSpy).toHaveBeenCalledTimes(1) - }) - - it("does nothing when user clicks Terminate (noButtonClicked)", async () => { - task.abort = true - task.abortReason = "user_cancelled" - task.clineMessages = [{ ts: Date.now() - 1000, type: "say", say: "text", text: "prev" } as any] - - vi.spyOn(task as any, "ask").mockResolvedValue({ response: "noButtonClicked" }) - const loopSpy = vi.spyOn(task as any, "initiateTaskLoop").mockResolvedValue(undefined) - - await task.presentResumableAsk() - - // Still aborted - expect(task.abort).toBe(true) - expect(task.abortReason).toBe("user_cancelled") - // No loop resume - expect(loopSpy).not.toHaveBeenCalled() - }) -}) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index fba76f479c..b13e303a93 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1730,12 +1730,12 @@ describe("Cline", () => { // Mock the dispose method to track cleanup const disposeSpy = vi.spyOn(task, "dispose").mockImplementation(() => {}) - // Call abortTask (soft cancel - same path as UI Cancel button) + // Call abortTask await task.abortTask() - // Verify the same behavior as Cancel button: soft abort sets abort flag but does not dispose + // Verify the same behavior as Cancel button expect(task.abort).toBe(true) - expect(disposeSpy).not.toHaveBeenCalled() + expect(disposeSpy).toHaveBeenCalled() }) it("should work with TaskLike interface", async () => { @@ -1779,8 +1779,8 @@ describe("Cline", () => { // Spy on console.error to verify error is logged const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - // abortTask should not throw even if dispose fails (hard abort triggers dispose) - await expect(task.abortTask(true)).resolves.not.toThrow() + // abortTask should not throw even if dispose fails + await expect(task.abortTask()).resolves.not.toThrow() // Verify error was logged expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining("Error during task"), mockError) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index f41b7fe779..0de9a7a9b1 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -52,7 +52,6 @@ import { Package } from "../../shared/package" import { findLast } from "../../shared/array" import { supportPrompt, type SupportPromptType } from "../../shared/support-prompt" import { GlobalFileNames } from "../../shared/globalFileNames" -import { safeJsonParse } from "../../shared/safeJsonParse" import type { ExtensionMessage, ExtensionState, MarketplaceInstalledMetadata } from "../../shared/ExtensionMessage" import { Mode, defaultModeSlug, getModeBySlug, ZgsmCodeMode } from "../../shared/modes" import { experimentDefault } from "../../shared/experiments" @@ -155,10 +154,6 @@ export class ClineProvider private pendingOperations: Map = new Map() private static readonly PENDING_OPERATION_TIMEOUT_MS = 30000 // 30 seconds - // Transactional state posting - private uiUpdatePaused: boolean = false - private pendingState: ExtensionState | null = null - public isViewLaunched = false public settingsImportedAt?: number public readonly latestAnnouncementId = "nov-2025-v3.30.0-pr-fixer" // v3.30.0 PR Fixer announcement @@ -931,7 +926,13 @@ export class ClineProvider } public async createTaskWithHistoryItem(historyItem: HistoryItem & { rootTask?: Task; parentTask?: Task }) { - await this.removeClineFromStack() + // Check if we're rehydrating the current task to avoid flicker + const currentTask = this.getCurrentTask() + const isRehydratingCurrentTask = currentTask && currentTask.taskId === historyItem.id + + if (!isRehydratingCurrentTask) { + await this.removeClineFromStack() + } // If the history item has a saved mode, restore it and its associated API configuration. if (historyItem.mode) { @@ -1007,11 +1008,46 @@ export class ClineProvider enableBridge: BridgeOrchestrator.isEnabled(cloudUserInfo, taskSyncEnabled), }) - await this.addClineToStack(task) + if (isRehydratingCurrentTask) { + // Replace the current task in-place to avoid UI flicker + const stackIndex = this.clineStack.length - 1 - this.log( - `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, - ) + // Properly dispose of the old task to ensure garbage collection + const oldTask = this.clineStack[stackIndex] + + // Abort the old task to stop running processes and mark as abandoned + try { + await oldTask.abortTask(true) + } catch (e) { + this.log( + `[createTaskWithHistoryItem] abortTask() failed for old task ${oldTask.taskId}.${oldTask.instanceId}: ${e.message}`, + ) + } + + // Remove event listeners from the old task + const cleanupFunctions = this.taskEventListeners.get(oldTask) + if (cleanupFunctions) { + cleanupFunctions.forEach((cleanup) => cleanup()) + this.taskEventListeners.delete(oldTask) + } + + // Replace the task in the stack + this.clineStack[stackIndex] = task + task.emit(RooCodeEventName.TaskFocused) + + // Perform preparation tasks and set up event listeners + await this.performPreparationTasks(task) + + this.log( + `[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`, + ) + } else { + await this.addClineToStack(task) + + this.log( + `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, + ) + } // Check if there's a pending edit after checkpoint restoration const operationId = `task-${task.taskId}` @@ -1703,26 +1739,8 @@ export class ClineProvider await this.postStateToWebview() } - public beginStateTransaction(): void { - this.uiUpdatePaused = true - } - - public async endStateTransaction(): Promise { - this.uiUpdatePaused = false - if (this.pendingState) { - await this.view?.webview.postMessage({ type: "state", state: this.pendingState }) - this.pendingState = null - } - } - async postStateToWebview() { const state = await this.getStateToPostToWebview() - - if (this.uiUpdatePaused) { - this.pendingState = state - return - } - this.postMessageToWebview({ type: "state", state }) // Check MDM compliance and send user to account tab if not compliant @@ -2793,13 +2811,24 @@ export class ClineProvider console.log(`[cancelTask] cancelling task ${task.taskId}.${task.instanceId}`) - // Mark this as a user-initiated cancellation + const { historyItem, uiMessagesFilePath } = await this.getTaskWithId(task.taskId) + + // Preserve parent and root task information for history item. + const rootTask = task.rootTask + const parentTask = task.parentTask + + // Mark this as a user-initiated cancellation so provider-only rehydration can occur task.abortReason = "user_cancelled" - // Soft abort (isAbandoned = false) to keep the instance alive - await task.abortTask(false) + // Capture the current instance to detect if rehydrate already occurred elsewhere + const originalInstanceId = task.instanceId + + // Begin abort (non-blocking) + task.abortTask() + + // Immediately mark the original instance as abandoned to prevent any residual activity + task.abandoned = true - // Wait for abort to complete await pWaitFor( () => this.getCurrentTask()! === undefined || @@ -2816,52 +2845,29 @@ export class ClineProvider console.error("Failed to abort task") }) task?.api?.cancelChat?.(task.abortReason) - // Deterministic spinner stop: If the last api_req_started has no cost and no cancelReason, - // inject cancelReason to stop the spinner - try { - let lastApiReqStartedIndex = -1 - for (let i = task.clineMessages.length - 1; i >= 0; i--) { - if (task.clineMessages[i].type === "say" && task.clineMessages[i].say === "api_req_started") { - lastApiReqStartedIndex = i - break - } - } - if (lastApiReqStartedIndex !== -1) { - const lastApiReqStarted = task.clineMessages[lastApiReqStartedIndex] - const apiReqInfo = safeJsonParse<{ cost?: number; cancelReason?: string }>( - lastApiReqStarted.text || "{}", - {}, - ) + // Defensive safeguard: if current instance already changed, skip rehydrate + const current = this.getCurrentTask() + if (current && current.instanceId !== originalInstanceId) { + this.log( + `[cancelTask] Skipping rehydrate: current instance ${current.instanceId} != original ${originalInstanceId}`, + ) + return + } - if (apiReqInfo && apiReqInfo.cost === undefined && apiReqInfo.cancelReason === undefined) { - apiReqInfo.cancelReason = "user_cancelled" - lastApiReqStarted.text = JSON.stringify(apiReqInfo) - await task.overwriteClineMessages([...task.clineMessages]) - console.log(`[cancelTask] Injected cancelReason for deterministic spinner stop`) - } + // Final race check before rehydrate to avoid duplicate rehydration + { + const currentAfterCheck = this.getCurrentTask() + if (currentAfterCheck && currentAfterCheck.instanceId !== originalInstanceId) { + this.log( + `[cancelTask] Skipping rehydrate after final check: current instance ${currentAfterCheck.instanceId} != original ${originalInstanceId}`, + ) + return } - } catch (error) { - console.error(`[cancelTask] Failed to inject cancelReason:`, error) } - // Update UI immediately to reflect current state - await this.postStateToWebview() - - // Schedule non-blocking resumption to present "Resume Task" ask - // Use setImmediate to avoid blocking the webview handler - setImmediate(() => { - if (task && !task.abandoned) { - // Present a resume ask without rehydrating - just show the Resume/Terminate UI - task.presentResumableAsk().catch((error) => { - console.error( - `[cancelTask] Failed to present resume ask: ${ - error instanceof Error ? error.message : String(error) - }`, - ) - }) - } - }) + // Clears task again, so we need to abortTask manually above. + await this.createTaskWithHistoryItem({ ...historyItem, rootTask, parentTask }) } // Clear the current task without treating it as a subtask. diff --git a/src/core/webview/__tests__/ClineProvider.cancelTask.present-ask.spec.ts b/src/core/webview/__tests__/ClineProvider.cancelTask.present-ask.spec.ts deleted file mode 100644 index f89dcc4c5a..0000000000 --- a/src/core/webview/__tests__/ClineProvider.cancelTask.present-ask.spec.ts +++ /dev/null @@ -1,60 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" -import { ClineProvider } from "../ClineProvider" - -describe("ClineProvider.cancelTask - schedules presentResumableAsk", () => { - let provider: ClineProvider - let mockTask: any - - beforeEach(() => { - vi.useFakeTimers() - // Create a bare instance without running constructor - provider = Object.create(ClineProvider.prototype) as ClineProvider - - mockTask = { - taskId: "task-1", - instanceId: "inst-1", - abortReason: undefined, - abandoned: false, - abortTask: vi.fn().mockResolvedValue(undefined), - isStreaming: false, - didFinishAbortingStream: true, - isWaitingForFirstChunk: false, - // Last api_req_started without cost/cancelReason so provider injects cancelReason - clineMessages: [ - { ts: Date.now() - 2000, type: "say", say: "text", text: "hello" }, - { ts: Date.now() - 1000, type: "say", say: "api_req_started", text: JSON.stringify({}) }, - ], - overwriteClineMessages: vi.fn().mockResolvedValue(undefined), - presentResumableAsk: vi.fn().mockResolvedValue(undefined), - } - - // Patch required instance methods used by cancelTask - ;(provider as any).getCurrentTask = vi.fn().mockReturnValue(mockTask) - ;(provider as any).postStateToWebview = vi.fn().mockResolvedValue(undefined) - }) - - afterEach(() => { - vi.useRealTimers() - }) - - it("injects cancelReason and schedules presentResumableAsk on soft cancel", async () => { - // Act - await (provider as any).cancelTask() - - // Assert that abort was initiated - expect(mockTask.abortTask).toHaveBeenCalledWith(false) - - // cancelReason injected for spinner stop - const last = mockTask.clineMessages.at(-1) - expect(last.say).toBe("api_req_started") - const parsed = JSON.parse(last.text || "{}") - expect(parsed.cancelReason).toBe("user_cancelled") - - // presentResumableAsk is scheduled via setImmediate - expect(mockTask.presentResumableAsk).not.toHaveBeenCalled() - vi.runAllTimers() - // run microtasks as well to flush any pending promises in the scheduled callback - await Promise.resolve() - expect(mockTask.presentResumableAsk).toHaveBeenCalledTimes(1) - }) -}) diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts new file mode 100644 index 0000000000..36c23512e7 --- /dev/null +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -0,0 +1,321 @@ +import { beforeEach, describe, expect, it, vi } from "vitest" +import * as vscode from "vscode" + +import { ClineProvider } from "../ClineProvider" +import { Task } from "../../task/Task" +import { ContextProxy } from "../../config/ContextProxy" +import type { ProviderSettings, HistoryItem } from "@roo-code/types" + +// Mock dependencies +vi.mock("vscode", () => { + const mockDisposable = { dispose: vi.fn() } + return { + workspace: { + getConfiguration: vi.fn(() => ({ + get: vi.fn().mockReturnValue([]), + update: vi.fn().mockResolvedValue(undefined), + })), + workspaceFolders: [], + onDidChangeConfiguration: vi.fn(() => mockDisposable), + }, + env: { + uriScheme: "vscode", + language: "en", + }, + EventEmitter: vi.fn().mockImplementation(() => ({ + event: vi.fn(), + fire: vi.fn(), + })), + Disposable: { + from: vi.fn(), + }, + window: { + showErrorMessage: vi.fn(), + createTextEditorDecorationType: vi.fn().mockReturnValue({ + dispose: vi.fn(), + }), + onDidChangeActiveTextEditor: vi.fn(() => mockDisposable), + }, + Uri: { + file: vi.fn().mockReturnValue({ toString: () => "file://test" }), + }, + } +}) + +vi.mock("../../task/Task") +vi.mock("../../config/ContextProxy") +vi.mock("../../../services/mcp/McpServerManager", () => ({ + McpServerManager: { + getInstance: vi.fn().mockResolvedValue({ + registerClient: vi.fn(), + }), + unregisterProvider: vi.fn(), + }, +})) +vi.mock("../../../services/marketplace") +vi.mock("../../../integrations/workspace/WorkspaceTracker") +vi.mock("../../config/ProviderSettingsManager") +vi.mock("../../config/CustomModesManager") +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn().mockReturnValue("/test/workspace"), +})) + +// Mock TelemetryService +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + setProvider: vi.fn(), + captureTaskCreated: vi.fn(), + }, + }, +})) + +// Mock CloudService +vi.mock("@roo-code/cloud", () => ({ + CloudService: { + hasInstance: vi.fn().mockReturnValue(false), + instance: { + isAuthenticated: vi.fn().mockReturnValue(false), + }, + }, + BridgeOrchestrator: { + isEnabled: vi.fn().mockReturnValue(false), + }, + getRooCodeApiUrl: vi.fn().mockReturnValue("https://api.roo-code.com"), +})) + +vi.mock("../../../shared/embeddingModels", () => ({ + EMBEDDING_MODEL_PROFILES: [], +})) + +describe("ClineProvider flicker-free cancel", () => { + let provider: ClineProvider + let mockContext: any + let mockOutputChannel: any + let mockTask1: any + let mockTask2: any + + const mockApiConfig: ProviderSettings = { + apiProvider: "anthropic", + apiKey: "test-key", + } as ProviderSettings + + beforeEach(() => { + vi.clearAllMocks() + + // Setup mock extension context + mockContext = { + globalState: { + get: vi.fn().mockReturnValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + globalStorageUri: { fsPath: "/test/storage" }, + secrets: { + get: vi.fn().mockResolvedValue(undefined), + store: vi.fn().mockResolvedValue(undefined), + delete: vi.fn().mockResolvedValue(undefined), + }, + workspaceState: { + get: vi.fn().mockReturnValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + extensionUri: { fsPath: "/test/extension" }, + } + + // Setup mock output channel + mockOutputChannel = { + appendLine: vi.fn(), + dispose: vi.fn(), + } + + // Setup mock context proxy + const mockContextProxy = { + getValues: vi.fn().mockReturnValue({}), + getValue: vi.fn().mockReturnValue(undefined), + setValue: vi.fn().mockResolvedValue(undefined), + getProviderSettings: vi.fn().mockReturnValue(mockApiConfig), + extensionUri: mockContext.extensionUri, + globalStorageUri: mockContext.globalStorageUri, + } + + // Create provider instance + provider = new ClineProvider(mockContext, mockOutputChannel, "sidebar", mockContextProxy as any) + + // Mock provider methods + provider.getState = vi.fn().mockResolvedValue({ + apiConfiguration: mockApiConfig, + mode: "code", + }) + + provider.postStateToWebview = vi.fn().mockResolvedValue(undefined) + // Mock private method using any cast + ;(provider as any).updateGlobalState = vi.fn().mockResolvedValue(undefined) + provider.activateProviderProfile = vi.fn().mockResolvedValue(undefined) + provider.performPreparationTasks = vi.fn().mockResolvedValue(undefined) + provider.getTaskWithId = vi.fn().mockImplementation((id) => + Promise.resolve({ + historyItem: { + id, + number: 1, + ts: Date.now(), + task: "test task", + tokensIn: 100, + tokensOut: 200, + totalCost: 0.001, + workspace: "/test/workspace", + }, + }), + ) + + // Setup mock tasks + mockTask1 = { + taskId: "task-1", + instanceId: "instance-1", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + abandoned: false, + dispose: vi.fn(), + on: vi.fn(), + off: vi.fn(), + } + + mockTask2 = { + taskId: "task-1", // Same ID for rehydration scenario + instanceId: "instance-2", // Different instance + emit: vi.fn(), + on: vi.fn(), + off: vi.fn(), + } + + // Mock Task constructor + vi.mocked(Task).mockImplementation(() => mockTask2 as any) + }) + + it("should not remove current task from stack when rehydrating same taskId", async () => { + // Setup: Add a task to the stack first + ;(provider as any).clineStack = [mockTask1] + + // Mock event listeners for cleanup + ;(provider as any).taskEventListeners = new WeakMap() + const mockCleanupFunctions = [vi.fn(), vi.fn()] + ;(provider as any).taskEventListeners.set(mockTask1, mockCleanupFunctions) + + // Spy on removeClineFromStack to verify it's NOT called + const removeClineFromStackSpy = vi.spyOn(provider, "removeClineFromStack") + + // Create history item with same taskId as current task + const historyItem: HistoryItem = { + id: "task-1", // Same as mockTask1.taskId + number: 1, + task: "test task", + ts: Date.now(), + tokensIn: 100, + tokensOut: 200, + totalCost: 0.001, + workspace: "/test/workspace", + } + + // Act: Create task with history item (should rehydrate in-place) + await provider.createTaskWithHistoryItem(historyItem) + + // Assert: removeClineFromStack should NOT be called + expect(removeClineFromStackSpy).not.toHaveBeenCalled() + + // Verify the task was replaced in-place + expect((provider as any).clineStack).toHaveLength(1) + expect((provider as any).clineStack[0]).toBe(mockTask2) + + // Verify old event listeners were cleaned up + expect(mockCleanupFunctions[0]).toHaveBeenCalled() + expect(mockCleanupFunctions[1]).toHaveBeenCalled() + + // Verify new task received focus event + expect(mockTask2.emit).toHaveBeenCalledWith("taskFocused") + }) + + it("should remove task from stack when creating different task", async () => { + // Setup: Add a task to the stack first + ;(provider as any).clineStack = [mockTask1] + + // Spy on removeClineFromStack to verify it IS called + const removeClineFromStackSpy = vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined) + + // Create history item with different taskId + const historyItem: HistoryItem = { + id: "task-2", // Different from mockTask1.taskId + number: 2, + task: "different task", + ts: Date.now(), + tokensIn: 150, + tokensOut: 250, + totalCost: 0.002, + workspace: "/test/workspace", + } + + // Act: Create task with different history item + await provider.createTaskWithHistoryItem(historyItem) + + // Assert: removeClineFromStack should be called + expect(removeClineFromStackSpy).toHaveBeenCalled() + }) + + it("should handle empty stack gracefully during rehydration attempt", async () => { + // Setup: Empty stack + ;(provider as any).clineStack = [] + + // Spy on removeClineFromStack + const removeClineFromStackSpy = vi.spyOn(provider, "removeClineFromStack").mockResolvedValue(undefined) + + // Create history item + const historyItem: HistoryItem = { + id: "task-1", + number: 1, + task: "test task", + ts: Date.now(), + tokensIn: 100, + tokensOut: 200, + totalCost: 0.001, + workspace: "/test/workspace", + } + + // Act: Should not error and should call removeClineFromStack + await provider.createTaskWithHistoryItem(historyItem) + + // Assert: removeClineFromStack should be called (no current task to rehydrate) + expect(removeClineFromStackSpy).toHaveBeenCalled() + }) + + it("should maintain task stack integrity during flicker-free replacement", async () => { + // Setup: Stack with multiple tasks + const mockParentTask = { + taskId: "parent-task", + instanceId: "parent-instance", + emit: vi.fn(), + } + + ;(provider as any).clineStack = [mockParentTask, mockTask1] + ;(provider as any).taskEventListeners = new WeakMap() + ;(provider as any).taskEventListeners.set(mockTask1, [vi.fn()]) + + // Act: Rehydrate the current (top) task + const historyItem: HistoryItem = { + id: "task-1", + number: 1, + task: "test task", + ts: Date.now(), + tokensIn: 100, + tokensOut: 200, + totalCost: 0.001, + workspace: "/test/workspace", + } + + await provider.createTaskWithHistoryItem(historyItem) + + // Assert: Stack should maintain parent task and replace current task + expect((provider as any).clineStack).toHaveLength(2) + expect((provider as any).clineStack[0]).toBe(mockParentTask) + expect((provider as any).clineStack[1]).toBe(mockTask2) + }) +}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index 1fffc2849d..ba8444ae95 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1124,26 +1124,18 @@ export const webviewMessageHandler = async ( const result = checkoutRestorePayloadSchema.safeParse(message.payload) if (result.success) { - // Begin transaction to buffer state updates - provider.beginStateTransaction() + await provider.cancelTask() try { - await provider.cancelTask() - - try { - await pWaitFor(() => provider.getCurrentTask()?.isInitialized === true, { timeout: 3_000 }) - } catch (error) { - vscode.window.showErrorMessage(t("common:errors.checkpoint_timeout")) - } + await pWaitFor(() => provider.getCurrentTask()?.isInitialized === true, { timeout: 3_000 }) + } catch (error) { + vscode.window.showErrorMessage(t("common:errors.checkpoint_timeout")) + } - try { - await provider.getCurrentTask()?.checkpointRestore(result.data) - } catch (error) { - vscode.window.showErrorMessage(t("common:errors.checkpoint_failed")) - } - } finally { - // End transaction and post consolidated state - await provider.endStateTransaction() + try { + await provider.getCurrentTask()?.checkpointRestore(result.data) + } catch (error) { + vscode.window.showErrorMessage(t("common:errors.checkpoint_failed")) } } diff --git a/src/services/checkpoints/ShadowCheckpointService.ts b/src/services/checkpoints/ShadowCheckpointService.ts index 1c4c8b2943..69f7003c37 100644 --- a/src/services/checkpoints/ShadowCheckpointService.ts +++ b/src/services/checkpoints/ShadowCheckpointService.ts @@ -4,7 +4,7 @@ import * as path from "path" import crypto from "crypto" import EventEmitter from "events" -import simpleGit, { SimpleGit } from "simple-git" +import simpleGit, { SimpleGit, SimpleGitOptions } from "simple-git" import pWaitFor from "p-wait-for" import * as vscode from "vscode" @@ -15,6 +15,65 @@ import { t } from "../../i18n" import { CheckpointDiff, CheckpointResult, CheckpointEventMap } from "./types" import { getExcludePatterns } from "./excludes" +/** + * Creates a SimpleGit instance with sanitized environment variables to prevent + * interference from inherited git environment variables like GIT_DIR and GIT_WORK_TREE. + * This ensures checkpoint operations always target the intended shadow repository. + * + * @param baseDir - The directory where git operations should be executed + * @returns A SimpleGit instance with sanitized environment + */ +function createSanitizedGit(baseDir: string): SimpleGit { + // Create a clean environment by explicitly unsetting git-related environment variables + // that could interfere with checkpoint operations + const sanitizedEnv: Record = {} + const removedVars: string[] = [] + + // Copy all environment variables except git-specific ones + for (const [key, value] of Object.entries(process.env)) { + // Skip git environment variables that would override repository location + if ( + key === "GIT_DIR" || + key === "GIT_WORK_TREE" || + key === "GIT_INDEX_FILE" || + key === "GIT_OBJECT_DIRECTORY" || + key === "GIT_ALTERNATE_OBJECT_DIRECTORIES" || + key === "GIT_CEILING_DIRECTORIES" + ) { + removedVars.push(`${key}=${value}`) + continue + } + + // Only include defined values + if (value !== undefined) { + sanitizedEnv[key] = value + } + } + + // Log which git env vars were removed (helps with debugging Dev Container issues) + if (removedVars.length > 0) { + console.log( + `[createSanitizedGit] Removed git environment variables for checkpoint isolation: ${removedVars.join(", ")}`, + ) + } + + const options: Partial = { + baseDir, + config: [], + } + + // Create git instance and set the sanitized environment + const git = simpleGit(options) + + // Use the .env() method to set the complete sanitized environment + // This replaces the inherited environment with our sanitized version + git.env(sanitizedEnv) + + console.log(`[createSanitizedGit] Created git instance for baseDir: ${baseDir}`) + + return git +} + export abstract class ShadowCheckpointService extends EventEmitter { public readonly taskId: string public readonly checkpointsDir: string @@ -85,7 +144,7 @@ export abstract class ShadowCheckpointService extends EventEmitter { } await fs.mkdir(this.checkpointsDir, { recursive: true }) - const git = simpleGit(this.checkpointsDir) + const git = createSanitizedGit(this.checkpointsDir) const gitVersion = await git.version() this.log(`[${this.constructor.name}#create] git = ${gitVersion}`) @@ -391,7 +450,7 @@ export abstract class ShadowCheckpointService extends EventEmitter { }) { const workspaceRepoDir = this.workspaceRepoDir({ globalStorageDir, workspaceDir }) const branchName = `roo-${taskId}` - const git = simpleGit(workspaceRepoDir) + const git = createSanitizedGit(workspaceRepoDir) const success = await this.deleteBranch(git, branchName) if (success) { diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index def558132b..2e0977f165 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -844,6 +844,95 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( // File should be back to original state expect(await fs.readFile(testFile, "utf-8")).toBe("Hello, world!") }) + + it("isolates checkpoint operations from GIT_DIR environment variable", async () => { + // This test verifies the fix for the issue where GIT_DIR environment variable + // causes checkpoint commits to go to the wrong repository. + // In the real-world Dev Container scenario, GIT_DIR is set BEFORE Roo starts, + // so we need to set it BEFORE creating the checkpoint service. + + // Create a separate git directory to simulate GIT_DIR pointing elsewhere + const externalGitDir = path.join(tmpDir, `external-git-${Date.now()}`) + await fs.mkdir(externalGitDir, { recursive: true }) + const externalGit = simpleGit(externalGitDir) + await externalGit.init() + await externalGit.addConfig("user.name", "External User") + await externalGit.addConfig("user.email", "external@example.com") + + // Create and commit a file in the external repo + const externalFile = path.join(externalGitDir, "external.txt") + await fs.writeFile(externalFile, "External content") + await externalGit.add(".") + await externalGit.commit("External commit") + + // Store the original commit count in the external repo + const externalLogBefore = await externalGit.log() + const externalCommitCountBefore = externalLogBefore.total + + // Initialize the workspace repo BEFORE setting GIT_DIR + // (In Dev Containers, the workspace repo already exists before GIT_DIR is set) + const testShadowDir = path.join(tmpDir, `shadow-git-dir-test-${Date.now()}`) + const testWorkspaceDir = path.join(tmpDir, `workspace-git-dir-test-${Date.now()}`) + const testRepo = await initWorkspaceRepo({ workspaceDir: testWorkspaceDir }) + + // Set GIT_DIR to point to the external repository BEFORE creating the service + // This simulates the Dev Container environment where GIT_DIR is already set + const originalGitDir = process.env.GIT_DIR + const externalDotGit = path.join(externalGitDir, ".git") + process.env.GIT_DIR = externalDotGit + + try { + // Create a new checkpoint service with GIT_DIR already set + // This is the key difference - we're creating the service + // while GIT_DIR is set, just like in a real Dev Container + const testService = await klass.create({ + taskId: `test-git-dir-${Date.now()}`, + shadowDir: testShadowDir, + workspaceDir: testWorkspaceDir, + log: () => {}, + }) + await testService.initShadowGit() + + // Make a change in the workspace and save a checkpoint + const testWorkspaceFile = path.join(testWorkspaceDir, "test.txt") + await fs.writeFile(testWorkspaceFile, "Modified with GIT_DIR set") + const commit = await testService.saveCheckpoint("Checkpoint with GIT_DIR set") + expect(commit?.commit).toBeTruthy() + + // Verify the checkpoint was saved in the shadow repo, not the external repo + // Temporarily clear GIT_DIR to check the external repo + delete process.env.GIT_DIR + const externalGitCheck = simpleGit(externalGitDir) + const externalLogAfter = await externalGitCheck.log() + const externalCommitCountAfter = externalLogAfter.total + // Restore GIT_DIR + process.env.GIT_DIR = externalDotGit + + // External repo should have the same number of commits (no new commits) + expect(externalCommitCountAfter).toBe(externalCommitCountBefore) + + // Verify the checkpoint is accessible in the shadow repo + const diff = await testService.getDiff({ to: commit!.commit }) + expect(diff).toHaveLength(1) + expect(diff[0].paths.relative).toBe("test.txt") + expect(diff[0].content.after).toBe("Modified with GIT_DIR set") + + // Verify we can restore the checkpoint + await fs.writeFile(testWorkspaceFile, "Another modification") + await testService.restoreCheckpoint(commit!.commit) + expect(await fs.readFile(testWorkspaceFile, "utf-8")).toBe("Modified with GIT_DIR set") + } finally { + // Restore original GIT_DIR + if (originalGitDir !== undefined) { + process.env.GIT_DIR = originalGitDir + } else { + delete process.env.GIT_DIR + } + + // Clean up external git directory + await fs.rm(externalGitDir, { recursive: true, force: true }) + } + }) }) }, ) diff --git a/src/services/code-index/__tests__/config-manager.spec.ts b/src/services/code-index/__tests__/config-manager.spec.ts index e02bfdfb04..16513158ef 100644 --- a/src/services/code-index/__tests__/config-manager.spec.ts +++ b/src/services/code-index/__tests__/config-manager.spec.ts @@ -1807,6 +1807,124 @@ describe("CodeIndexConfigManager", () => { // Should return undefined since custom dimension is invalid expect(configManager.currentModelDimension).toBe(undefined) }) + + describe("OpenRouter provider dimension handling", () => { + it("should correctly handle OpenRouter mistral model dimensions across restarts", async () => { + // Mock getModelDimension to return correct dimensions for OpenRouter models + mockedGetModelDimension.mockImplementation((provider, modelId) => { + if (provider === "openrouter") { + if (modelId === "mistralai/codestral-embed-2505") return 1536 + if (modelId === "mistralai/mistral-embed-2312") return 1024 + if (modelId === "openai/text-embedding-3-large") return 3072 + } + return undefined + }) + + // Initial configuration with OpenRouter and Mistral model + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexEmbedderProvider: "openrouter", + codebaseIndexEmbedderModelId: "mistralai/codestral-embed-2505", + codebaseIndexQdrantUrl: "http://localhost:6333", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codebaseIndexOpenRouterApiKey") return "test-openrouter-key" + if (key === "codeIndexQdrantApiKey") return "test-qdrant-key" + return undefined + }) + + configManager = new CodeIndexConfigManager(mockContextProxy) + await configManager.loadConfiguration() + + // Should correctly return the built-in dimension for the Mistral model + expect(configManager.currentModelDimension).toBe(1536) + expect(mockedGetModelDimension).toHaveBeenCalledWith("openrouter", "mistralai/codestral-embed-2505") + + // Simulate restart by creating a new config manager with same configuration + const restartConfigManager = new CodeIndexConfigManager(mockContextProxy) + await restartConfigManager.loadConfiguration() + + // After "restart", dimension should still be correct + expect(restartConfigManager.currentModelDimension).toBe(1536) + expect(restartConfigManager.isFeatureConfigured).toBe(true) + }) + + it("should not require restart for OpenRouter when same model dimensions are used", async () => { + // Mock both models to have same dimension + mockedGetModelDimension.mockImplementation((provider, modelId) => { + if (provider === "openrouter") { + if (modelId === "mistralai/codestral-embed-2505") return 1536 + if (modelId === "openai/text-embedding-3-small") return 1536 + } + return undefined + }) + + // Initial state with OpenRouter and Mistral model + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexEmbedderProvider: "openrouter", + codebaseIndexEmbedderModelId: "mistralai/codestral-embed-2505", + codebaseIndexQdrantUrl: "http://localhost:6333", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codebaseIndexOpenRouterApiKey") return "test-key" + if (key === "codeIndexQdrantApiKey") return "test-key" + return undefined + }) + + await configManager.loadConfiguration() + + // Change to another model with same dimension + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexEmbedderProvider: "openrouter", + codebaseIndexEmbedderModelId: "openai/text-embedding-3-small", // Same 1536 dimension + codebaseIndexQdrantUrl: "http://localhost:6333", + }) + + const result = await configManager.loadConfiguration() + // Should NOT require restart since dimensions are the same + expect(result.requiresRestart).toBe(false) + }) + + it("should require restart for OpenRouter when model dimensions change", async () => { + // Mock models with different dimensions + mockedGetModelDimension.mockImplementation((provider, modelId) => { + if (provider === "openrouter") { + if (modelId === "mistralai/codestral-embed-2505") return 1536 + if (modelId === "mistralai/mistral-embed-2312") return 1024 + } + return undefined + }) + + // Initial state with 1536-dimension model + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexEmbedderProvider: "openrouter", + codebaseIndexEmbedderModelId: "mistralai/codestral-embed-2505", + codebaseIndexQdrantUrl: "http://localhost:6333", + }) + mockContextProxy.getSecret.mockImplementation((key: string) => { + if (key === "codebaseIndexOpenRouterApiKey") return "test-key" + if (key === "codeIndexQdrantApiKey") return "test-key" + return undefined + }) + + await configManager.loadConfiguration() + + // Change to model with different dimension + mockContextProxy.getGlobalState.mockReturnValue({ + codebaseIndexEnabled: true, + codebaseIndexEmbedderProvider: "openrouter", + codebaseIndexEmbedderModelId: "mistralai/mistral-embed-2312", // Different 1024 dimension + codebaseIndexQdrantUrl: "http://localhost:6333", + }) + + const result = await configManager.loadConfiguration() + // Should require restart since dimensions changed + expect(result.requiresRestart).toBe(true) + }) + }) }) }) }) diff --git a/src/shared/embeddingModels.ts b/src/shared/embeddingModels.ts index 8c2f8fd44c..14015c80db 100644 --- a/src/shared/embeddingModels.ts +++ b/src/shared/embeddingModels.ts @@ -86,7 +86,7 @@ export const EMBEDDING_MODEL_PROFILES: EmbeddingModelProfiles = { "google/gemini-embedding-001": { dimension: 3072, scoreThreshold: 0.4 }, // Mistral models via OpenRouter "mistralai/mistral-embed-2312": { dimension: 1024, scoreThreshold: 0.4 }, - "mistralai/codestral-embed-2505": { dimension: 3072, scoreThreshold: 0.4 }, + "mistralai/codestral-embed-2505": { dimension: 1536, scoreThreshold: 0.4 }, // Qwen models via OpenRouter "qwen/qwen3-embedding-8b": { dimension: 4096, scoreThreshold: 0.4 }, }, diff --git a/webview-ui/src/components/chat/ApiConfigSelector.tsx b/webview-ui/src/components/chat/ApiConfigSelector.tsx index 786ad01dff..4396019a2d 100644 --- a/webview-ui/src/components/chat/ApiConfigSelector.tsx +++ b/webview-ui/src/components/chat/ApiConfigSelector.tsx @@ -193,27 +193,31 @@ export const ApiConfigSelector = ({ )} - {/* Config list */} -
- {filteredConfigs.length === 0 && searchValue ? ( -
- {t("common:ui.no_results")} -
- ) : ( -
- {/* Pinned configs */} - {pinnedConfigs.map((config) => renderConfigItem(config, true))} - - {/* Separator between pinned and unpinned */} - {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( -
- )} - - {/* Unpinned configs */} - {unpinnedConfigs.map((config) => renderConfigItem(config, false))} -
- )} -
+ {/* Config list - single scroll container */} + {filteredConfigs.length === 0 && searchValue ? ( +
{t("common:ui.no_results")}
+ ) : ( +
+ {/* Pinned configs - sticky header */} + {pinnedConfigs.length > 0 && ( +
0 && "border-b border-vscode-dropdown-foreground/10", + )} + aria-label="Pinned configurations"> + {pinnedConfigs.map((config) => renderConfigItem(config, true))} +
+ )} + + {/* Unpinned configs */} + {unpinnedConfigs.length > 0 && ( +
+ {unpinnedConfigs.map((config) => renderConfigItem(config, false))} +
+ )} +
+ )} {/* Bottom bar with buttons on left and title on right */}
diff --git a/webview-ui/src/components/chat/BatchFilePermission.tsx b/webview-ui/src/components/chat/BatchFilePermission.tsx index 3b7c7daf92..9846653bea 100644 --- a/webview-ui/src/components/chat/BatchFilePermission.tsx +++ b/webview-ui/src/components/chat/BatchFilePermission.tsx @@ -2,9 +2,10 @@ import { memo } from "react" import { ToolUseBlock, ToolUseBlockHeader } from "../common/ToolUseBlock" import { vscode } from "@src/utils/vscode" -import { removeLeadingNonAlphanumeric } from "@src/utils/removeLeadingNonAlphanumeric" import { getJumpLine } from "@/utils/path-mentions" import { FilePermissionItem } from "@roo-code/types" +import { formatPathTooltip } from "@src/utils/formatPathTooltip" +import { PathTooltip } from "../ui/PathTooltip" export interface BatchFilePermissionProps { files: FilePermissionItem[] @@ -35,10 +36,18 @@ export const BatchFilePermission = memo(({ files = [], onPermissionResponse, ts }) }}> {file.path?.startsWith(".") && .} - - {removeLeadingNonAlphanumeric(file.path ?? "") + "\u200E"} - {file.lineSnippet && ` ${file.lineSnippet}`} - + + + {formatPathTooltip( + file.path, + file.lineSnippet ? ` ${file.lineSnippet}` : undefined, + )} + +
diff --git a/webview-ui/src/components/chat/ChatRow.tsx b/webview-ui/src/components/chat/ChatRow.tsx index e503247bb8..6836ec265f 100644 --- a/webview-ui/src/components/chat/ChatRow.tsx +++ b/webview-ui/src/components/chat/ChatRow.tsx @@ -15,7 +15,7 @@ import { useCopyToClipboard } from "@src/utils/clipboard" import { useExtensionState } from "@src/context/ExtensionStateContext" import { findMatchingResourceOrTemplate } from "@src/utils/mcp" import { vscode } from "@src/utils/vscode" -import { removeLeadingNonAlphanumeric } from "@src/utils/removeLeadingNonAlphanumeric" +import { formatPathTooltip } from "@src/utils/formatPathTooltip" import { getLanguageFromPath } from "@src/utils/getLanguageFromPath" import { Button } from "@src/components/ui" @@ -70,6 +70,7 @@ import { cn } from "@/lib/utils" import { getJumpLine } from "@/utils/path-mentions" import { useZgsmUserInfo } from "@/hooks/useZgsmUserInfo" import { format } from "date-fns" +import { PathTooltip } from "../ui/PathTooltip" interface ChatRowProps { message: ClineMessage @@ -633,10 +634,11 @@ export const ChatRowContent = ({ }) }}> {tool.path?.startsWith(".") && .} - - {removeLeadingNonAlphanumeric(tool.path ?? "") + "\u200E"} - {tool.reason} - + + + {formatPathTooltip(tool.path, tool.reason)} + +
{ e.stopPropagation() // handleEditClick() @@ -1309,6 +1312,7 @@ export const ChatRowContent = ({
{ e.stopPropagation() vscode.postMessage({ type: "deleteMessage", value: message.ts }) diff --git a/webview-ui/src/components/chat/ChatView.tsx b/webview-ui/src/components/chat/ChatView.tsx index a509c4136f..475eba3e74 100644 --- a/webview-ui/src/components/chat/ChatView.tsx +++ b/webview-ui/src/components/chat/ChatView.tsx @@ -61,7 +61,6 @@ import ChatSearch from "./ChatSearch" // import { useCloudUpsell } from "@src/hooks/useCloudUpsell" // import { Cloud } from "lucide-react" // import CloudAgents from "../cloud/CloudAgents" -import { safeJsonParse } from "../../../../src/shared/safeJsonParse" export interface ChatViewProps { isHidden: boolean @@ -548,21 +547,10 @@ const ChatViewComponent: React.ForwardRefRenderFunction { // Search value should be maintained expect(searchInput.value).toBe("Config") }) + + test("pinned configs remain fixed at top while unpinned configs scroll", () => { + // Create a list with many configs to test scrolling + const manyConfigs = Array.from({ length: 15 }, (_, i) => ({ + id: `config${i + 1}`, + name: `Config ${i + 1}`, + modelId: `model-${i + 1}`, + })) + + const props = { + ...defaultProps, + listApiConfigMeta: manyConfigs, + pinnedApiConfigs: { + config1: true, + config2: true, + config3: true, + }, + } + + render() + + const trigger = screen.getByTestId("dropdown-trigger") + fireEvent.click(trigger) + + const popoverContent = screen.getByTestId("popover-content") + + // Should have a single scroll container with max-h-[300px] and overflow-y-auto + const scrollContainer = popoverContent.querySelector(".max-h-\\[300px\\].overflow-y-auto") + expect(scrollContainer).toBeInTheDocument() + + // Check for pinned configs sticky header + const pinnedStickyHeader = scrollContainer?.querySelector(".sticky.top-0.z-10.bg-vscode-dropdown-background") + expect(pinnedStickyHeader).toBeInTheDocument() + expect(pinnedStickyHeader).toHaveAttribute("aria-label", "Pinned configurations") + + // Check for Config 1, 2, 3 being visible in the sticky header (pinned) + expect(screen.getAllByText("Config 1").length).toBeGreaterThan(0) + expect(screen.getAllByText("Config 2").length).toBeGreaterThan(0) + expect(screen.getAllByText("Config 3").length).toBeGreaterThan(0) + + // Verify pinned container contains the pinned configs + if (pinnedStickyHeader) { + const elements = pinnedStickyHeader.querySelectorAll(".flex-shrink-0") + const pinnedConfigTexts = Array.from(elements) + .map((el) => (el as Element).textContent) + .filter((text) => text?.startsWith("Config")) + + expect(pinnedConfigTexts).toContain("Config 1") + expect(pinnedConfigTexts).toContain("Config 2") + expect(pinnedConfigTexts).toContain("Config 3") + } + + // Check for unpinned configs section + const unpinnedSection = scrollContainer?.querySelector('[aria-label="All configurations"]') + expect(unpinnedSection).toBeInTheDocument() + + // Verify separator exists as border on pinned section when unpinned configs exist + expect(pinnedStickyHeader).toHaveClass("border-b") + }) + + test("displays all configs in scrollable container when no configs are pinned", () => { + const manyConfigs = Array.from({ length: 10 }, (_, i) => ({ + id: `config${i + 1}`, + name: `Config ${i + 1}`, + modelId: `model-${i + 1}`, + })) + + const props = { + ...defaultProps, + listApiConfigMeta: manyConfigs, + pinnedApiConfigs: {}, // No pinned configs + } + + render() + + const trigger = screen.getByTestId("dropdown-trigger") + fireEvent.click(trigger) + + const popoverContent = screen.getByTestId("popover-content") + + // Should have a single scroll container with max-h-[300px] and overflow-y-auto + const scrollContainer = popoverContent.querySelector(".max-h-\\[300px\\].overflow-y-auto") + expect(scrollContainer).toBeInTheDocument() + + // No pinned section should exist when no configs are pinned + const pinnedSection = scrollContainer?.querySelector(".sticky.top-0") + expect(pinnedSection).not.toBeInTheDocument() + + // Should have unpinned configs section with all configs + const unpinnedSection = scrollContainer?.querySelector('[aria-label="All configurations"]') + expect(unpinnedSection).toBeInTheDocument() + + // All configs should be in the unpinned section + const allConfigRows = unpinnedSection?.querySelectorAll(".group") + expect(allConfigRows?.length).toBe(10) + + // No separator should exist when no pinned configs (no sticky header exists) + const stickyHeader = scrollContainer?.querySelector(".sticky.top-0") + expect(stickyHeader).not.toBeInTheDocument() + }) }) diff --git a/webview-ui/src/components/common/CodeAccordian.tsx b/webview-ui/src/components/common/CodeAccordian.tsx index cf952e6fa2..4e1ea0c4ea 100644 --- a/webview-ui/src/components/common/CodeAccordian.tsx +++ b/webview-ui/src/components/common/CodeAccordian.tsx @@ -2,10 +2,11 @@ import { memo, useMemo } from "react" import { VSCodeProgressRing } from "@vscode/webview-ui-toolkit/react" import { type ToolProgressStatus } from "@roo-code/types" import { getLanguageFromPath } from "@src/utils/getLanguageFromPath" -import { removeLeadingNonAlphanumeric } from "@src/utils/removeLeadingNonAlphanumeric" +import { formatPathTooltip } from "@src/utils/formatPathTooltip" import { ToolUseBlock, ToolUseBlockHeader } from "./ToolUseBlock" import CodeBlock from "./CodeBlock" +import { PathTooltip } from "../ui/PathTooltip" interface CodeAccordianProps { path?: string @@ -51,7 +52,9 @@ const CodeAccordian = ({ {header ? (
- {header} + + {header} +
) : isFeedback ? (
@@ -63,9 +66,11 @@ const CodeAccordian = ({ ) : ( <> {path?.startsWith(".") && .} - - {removeLeadingNonAlphanumeric(path ?? "") + "\u200E"} - + + + {formatPathTooltip(path)} + + )}
diff --git a/webview-ui/src/components/ui/PathTooltip.tsx b/webview-ui/src/components/ui/PathTooltip.tsx new file mode 100644 index 0000000000..dc6e176d8e --- /dev/null +++ b/webview-ui/src/components/ui/PathTooltip.tsx @@ -0,0 +1,54 @@ +import { ReactNode } from "react" +import { StandardTooltip } from "./standard-tooltip" + +interface PathTooltipProps { + /** The file path or content to display in the tooltip */ + content: string + /** The element(s) that trigger the tooltip */ + children: ReactNode + /** The preferred side of the trigger to render the tooltip */ + side?: "top" | "right" | "bottom" | "left" + /** The preferred alignment against the trigger */ + align?: "start" | "center" | "end" + /** Distance in pixels from the trigger */ + sideOffset?: number + /** Whether the trigger should be rendered as a child */ + asChild?: boolean +} + +/** + * PathTooltip component specifically designed for displaying file paths with appropriate + * wrapping and responsive width behavior. Use this for truncated file paths that need + * tooltips to show the full path. + * + * Features: + * - Responsive max-width that adapts to panel size (max 300px on wide panels, 100vw on narrow) + * - Uses text-wrap instead of text-balance to minimize whitespace + * - Leverages existing break-words CSS for natural line breaking at path separators + * + * @example + * + * ...path.tsx + * + */ +export function PathTooltip({ + content, + children, + side = "top", + align = "start", + sideOffset = 4, + asChild = true, +}: PathTooltipProps) { + return ( + + {children} + + ) +} diff --git a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts index e79b7fad7f..f14a426273 100644 --- a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts +++ b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts @@ -370,7 +370,7 @@ describe("useSelectedModel", () => { const { result } = renderHook(() => useSelectedModel(), { wrapper }) expect(result.current.provider).toBe("zgsm") - expect(result.current.id).toBe("GLM-4.5") + expect(result.current.id).toBe("GLM-4.6") }) }) @@ -441,7 +441,7 @@ describe("useSelectedModel", () => { const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) expect(result.current.provider).toBe("claude-code") - expect(result.current.id).toBe("claude-sonnet-4-20250514") // Default model + expect(result.current.id).toBe("claude-sonnet-4-5") // Default model expect(result.current.info).toBeDefined() expect(result.current.info?.supportsImages).toBe(false) }) diff --git a/webview-ui/src/utils/formatPathTooltip.ts b/webview-ui/src/utils/formatPathTooltip.ts new file mode 100644 index 0000000000..cfe0b54a7f --- /dev/null +++ b/webview-ui/src/utils/formatPathTooltip.ts @@ -0,0 +1,28 @@ +import { removeLeadingNonAlphanumeric } from "./removeLeadingNonAlphanumeric" + +/** + * Formats a file path for display in tooltips with consistent formatting. + * + * @param path - The file path to format + * @param additionalContent - Optional additional content to append (e.g., line snippets, reasons) + * @returns Formatted string ready for tooltip display + * + * @example + * formatPathTooltip("/src/components/MyComponent.tsx") + * // Returns: "src/components/MyComponent.tsx" + U+200E + * + * @example + * formatPathTooltip("/src/utils/helper.ts", ":42-45") + * // Returns: "src/utils/helper.ts:42-45" + U+200E + */ +export function formatPathTooltip(path?: string, additionalContent?: string): string { + if (!path) return "" + + const formattedPath = removeLeadingNonAlphanumeric(path) + "\u200E" + + if (additionalContent) { + return formattedPath + additionalContent + } + + return formattedPath +}