Skip to content
Open
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
33 changes: 33 additions & 0 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1831,6 +1831,39 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
return formatResponse.toolError(formatResponse.missingToolParameterError(paramName))
}

/**
* Finalize a partial "tool" ask message without blocking for user input.
* Call this in error paths where a partial tool message was opened during streaming
* but execution failed before the normal approval flow could close it, so the webview
* spinner does not get stuck in a loading state.
*
* The matching partial message may no longer be the final entry if another asynchronous
* message was inserted between the partial ask and the error handler, so search backward
* instead of relying on clineMessages.at(-1).
*/
async finalizePartialToolAsk(text?: string): Promise<void> {
const partialToolAsk = this.clineMessages
.slice()
.reverse()
.find(
(message) =>
message.partial === true &&
message.type === "ask" &&
message.ask === "tool" &&
(text === undefined || message.text === text),
)

if (!partialToolAsk) {
return
}

partialToolAsk.partial = false
await this.saveClineMessages()
await this.updateClineMessage(partialToolAsk).catch((error) => {
console.error("[Task#finalizePartialToolAsk] updateClineMessage failed:", error)
})
}

// Lifecycle
// Start / Resume / Abort / Dispose

Expand Down
72 changes: 72 additions & 0 deletions src/core/task/__tests__/Task.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2857,6 +2857,78 @@ describe("Cline", () => {
saveSpy.mockRestore()
})

it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => {
const updateSpy = vi
.spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage")
.mockResolvedValue(undefined)
const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true)

const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "test task",
startTask: false,
})

const partialToolAsk = {
ts: Date.now() - 2,
type: "ask" as const,
ask: "tool" as const,
text: "partial tool message",
partial: true,
}

task.clineMessages.push(partialToolAsk)
task.clineMessages.push({
ts: Date.now() - 1,
type: "say",
say: "error",
text: "intervening async message",
})

await task.finalizePartialToolAsk("partial tool message")
await flushMicrotasks()

expect(partialToolAsk.partial).toBe(false)
expect(saveSpy).toHaveBeenCalled()
expect(updateSpy).toHaveBeenCalledWith(partialToolAsk)

updateSpy.mockRestore()
saveSpy.mockRestore()
})

it("finalizePartialToolAsk ignores non-matching partial tool asks when text is provided", async () => {
const updateSpy = vi
.spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage")
.mockResolvedValue(undefined)
const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true)

const task = new Task({
provider: mockProvider,
apiConfiguration: mockApiConfig,
task: "test task",
startTask: false,
})

task.clineMessages.push({
ts: Date.now() - 1,
type: "ask",
ask: "tool",
text: "other partial tool message",
partial: true,
})

await task.finalizePartialToolAsk("target partial tool message")
await flushMicrotasks()

expect(task.clineMessages[0].partial).toBe(true)
expect(saveSpy).not.toHaveBeenCalled()
expect(updateSpy).not.toHaveBeenCalled()

updateSpy.mockRestore()
saveSpy.mockRestore()
})

it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => {
// Pins the .catch arm on the fire-and-forget updateClineMessage call
// in ask() when a new partial ask arrives while the previous partial
Expand Down
3 changes: 3 additions & 0 deletions src/core/task/__tests__/Task.throttle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,12 @@ describe("Task token usage throttling", () => {
let mockProvider: any
let mockApiConfiguration: ProviderSettings
let task: Task
let consoleLogSpy: ReturnType<typeof vi.spyOn>

beforeEach(() => {
// Reset all mocks
vi.clearAllMocks()
consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {})
vi.useFakeTimers()

// Mock provider
Expand Down Expand Up @@ -101,6 +103,7 @@ describe("Task token usage throttling", () => {
if (task && !task.abort) {
task.dispose()
}
consoleLogSpy.mockRestore()
})

test("should emit TaskTokenUsageUpdated immediately on first change", async () => {
Expand Down
153 changes: 128 additions & 25 deletions src/core/tools/WriteToFileTool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from "path"
import delay from "delay"
import fs from "fs/promises"

import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
import { type ClineSayTool, DEFAULT_WRITE_DELAY_MS, RooCodeEventName } from "@roo-code/types"

import { Task } from "../task/Task"
import { formatResponse } from "../prompts/responses"
Expand All @@ -26,10 +26,87 @@ interface WriteToFileParams {
export class WriteToFileTool extends BaseTool<"write_to_file"> {
readonly name = "write_to_file" as const

/**
* Tracks filesystem failures from diff-view streaming by task id. Tool instances are
* singletons, so this state must be keyed per task to avoid one task's failing partial
* stream suppressing another task's streaming deltas.
*/
private partialStreamFailuresByTaskId = new Set<string>()

/**
* Tracks partial path stabilization by task id. The tool is a singleton, so using the
* BaseTool singleton path state lets concurrent tasks incorrectly stabilize each other.
*/
private lastSeenPartialPathByTaskId = new Map<string, string | undefined>()

/**
* Tracks abort cleanup listeners for per-task partial state so normal execute()
* finalization can unregister them and abandoned streams are torn down on abort.
*/
private partialStateAbortCleanupByTaskId = new Map<string, { task: Task; cleanup: () => void }>()

private getPartialStreamFailureKey(task: Task): string {
return `${task.taskId}.${task.instanceId}`
}

private registerTaskPartialStateCleanup(task: Task): void {
const key = this.getPartialStreamFailureKey(task)
if (this.partialStateAbortCleanupByTaskId.has(key)) {
return
}

const cleanup = () => this.resetTaskPartialState(task)
this.partialStateAbortCleanupByTaskId.set(key, { task, cleanup })
task.once(RooCodeEventName.TaskAborted, cleanup)
}

private hasPathStabilizedForTask(task: Task, partialPath: string | undefined): boolean {
this.registerTaskPartialStateCleanup(task)
const key = this.getPartialStreamFailureKey(task)
const lastSeenPath = this.lastSeenPartialPathByTaskId.get(key)
const pathHasStabilized = lastSeenPath !== undefined && lastSeenPath === partialPath
this.lastSeenPartialPathByTaskId.set(key, partialPath)
return pathHasStabilized && !!partialPath
}

private resetTaskPartialState(task: Task): void {
const key = this.getPartialStreamFailureKey(task)
const abortCleanup = this.partialStateAbortCleanupByTaskId.get(key)
if (abortCleanup) {
task.off(RooCodeEventName.TaskAborted, abortCleanup.cleanup)
this.partialStateAbortCleanupByTaskId.delete(key)
}
this.lastSeenPartialPathByTaskId.delete(key)
this.partialStreamFailuresByTaskId.delete(key)
}

private async resetDiffViewAfterWrite(task: Task): Promise<void> {
await task.diffViewProvider.reset().catch((resetError) => {
console.error("Error resetting write_to_file diff view:", resetError)
})
}

private async finalizePartialToolAskAfterFailure(task: Task, text?: string): Promise<void> {
await task.finalizePartialToolAsk(text).catch((finalizeError) => {
console.error("Error finalizing write_to_file partial tool ask:", finalizeError)
})
}

override resetPartialState(): void {
super.resetPartialState()
for (const { task, cleanup } of this.partialStateAbortCleanupByTaskId.values()) {
task.off(RooCodeEventName.TaskAborted, cleanup)
}
this.partialStreamFailuresByTaskId.clear()
this.lastSeenPartialPathByTaskId.clear()
this.partialStateAbortCleanupByTaskId.clear()
}

async execute(params: WriteToFileParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
const { pushToolResult, handleError, askApproval } = callbacks
const relPath = params.path
let newContent = params.content
const partialStreamFailureKey = this.getPartialStreamFailureKey(task)

if (!relPath) {
task.consecutiveMistakeCount++
Expand Down Expand Up @@ -67,12 +144,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
task.diffViewProvider.editType = fileExists ? "modify" : "create"
}

// Create parent directories early for new files to prevent ENOENT errors
// in subsequent operations (e.g., diffViewProvider.open, fs.readFile)
if (!fileExists) {
await createDirectoriesForFile(absolutePath)
}

if (newContent.startsWith("```")) {
newContent = newContent.split("\n").slice(1).join("\n")
}
Expand All @@ -97,6 +168,13 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
}

try {
// Create parent directories for new files inside the try block so filesystem
// errors (EROFS, EACCES, etc.) route through handleError with proper cleanup
// and consecutive-mistake counting, rather than escaping unhandled.
if (!fileExists) {
await createDirectoriesForFile(absolutePath)
}

task.consecutiveMistakeCount = 0

const provider = task.providerRef.deref()
Expand Down Expand Up @@ -179,26 +257,40 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {

pushToolResult(message)

await task.diffViewProvider.reset()
this.resetPartialState()
await this.resetDiffViewAfterWrite(task)

task.processQueuedMessages()

return
} catch (error) {
// Finalize any open partial tool message so the UI spinner doesn't get stuck.
// The partial ask fired during streaming (handlePartial) or early in execute sets
// partial: true on the webview message; without this, the spinner persists even
// after the error bubble appears.
await this.finalizePartialToolAskAfterFailure(task)
await handleError("writing file", error as Error)
await task.diffViewProvider.reset()
this.resetPartialState()
await this.resetDiffViewAfterWrite(task)
return
} finally {
this.resetTaskPartialState(task)
}
}

override async handlePartial(task: Task, block: ToolUse<"write_to_file">): Promise<void> {
const relPath: string | undefined = block.params.path
const newContent: string | undefined = block.params.content

const partialStreamFailureKey = this.getPartialStreamFailureKey(task)

// A prior streaming delta for this task already hit a fatal filesystem error.
// Skip further streaming work so we don't create a new partial tool message on every
// subsequent delta. execute() will report the error once when the block completes.
if (this.partialStreamFailuresByTaskId.has(partialStreamFailureKey)) {
return
}

// Wait for path to stabilize before showing UI (prevents truncated paths)
if (!this.hasPathStabilized(relPath) || newContent === undefined) {
if (!this.hasPathStabilizedForTask(task, relPath) || newContent === undefined) {
return
}

Expand All @@ -224,12 +316,6 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
task.diffViewProvider.editType = fileExists ? "modify" : "create"
}

// Create parent directories early for new files to prevent ENOENT errors
// in subsequent operations (e.g., diffViewProvider.open)
if (!fileExists) {
await createDirectoriesForFile(absolutePath)
}

const isWriteProtected = task.rooProtectedController?.isWriteProtected(relPath!) || false
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)

Expand All @@ -245,14 +331,31 @@ export class WriteToFileTool extends BaseTool<"write_to_file"> {
await task.ask("tool", partialMessage, block.partial).catch(() => {})

if (newContent) {
if (!task.diffViewProvider.isEditing) {
await task.diffViewProvider.open(relPath!)
}
try {
if (!task.diffViewProvider.isEditing) {
await task.diffViewProvider.open(relPath!)
}

await task.diffViewProvider.update(
everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent,
false,
)
await task.diffViewProvider.update(
everyLineHasLineNumbers(newContent) ? stripLineNumbers(newContent) : newContent,
false,
)
} catch (error) {
// Opening or updating the diff view can throw on filesystem errors
// (EACCES/EROFS on read-only paths). Finalize the partial tool message
// so the UI spinner doesn't get stuck and reset the diff view. Do NOT
// rethrow: the same filesystem operation is retried in execute() once the
// block completes, and that authoritative non-partial path reports the
// error to the user. Surfacing it here too would show the same error twice.
// Swallowing it here is safe because the agent loop advances naturally when
// the non-partial block arrives (it does not depend on this throw).
console.error(`Error streaming write_to_file diff view:`, error)
// Mark the stream as failed so later deltas don't re-attempt and spawn a new
// partial tool message each time.
this.partialStreamFailuresByTaskId.add(partialStreamFailureKey)
await this.finalizePartialToolAskAfterFailure(task, partialMessage)
await this.resetDiffViewAfterWrite(task)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
}
Expand Down
Loading
Loading