Skip to content

Commit bd2769f

Browse files
myk1ytmyk1yt
committed
[Fix] Append queued feedback once and bind mention cwd before the await
What: `persistQueuedFeedbackAndAcknowledge` now appends its `user_feedback` row once, before the save-retry loop, and records the append in `queuedFeedbackRowWritten` so a retry (or a reclaimed message persisted again) re-saves the existing row instead of appending a duplicate. The record is cleared on consume and on the queue-prune sweep. `resolveIncomingImages` reads its cwd before its own awaits, so mention resolution uses the workspace captured with the task rather than whichever task is current after the state read. Why: the retry loop re-entered the append on every attempt, so a save that failed and then succeeded left two identical `user_feedback` rows in history and the webview; a message released by `releaseUnconsumedQueuedMessage` and persisted on a later turn appended a second row for text already retained in `clineMessages`. Separately, `cwd: getCurrentCwd()` was evaluated after `await provider.getState()`, so a task switch during that await resolved image mentions against the new task's workspace (security seat LOW-3). Impact: queued feedback history stays single-row per message, and queued messages resolve `@` image mentions against the workspace that receives them. No public API change; budget stays at 500/500 committed non-test lines. Regression tests fail on the pre-fix shapes.
1 parent 52d9e79 commit bd2769f

6 files changed

Lines changed: 108 additions & 45 deletions

File tree

‎src/core/task/Task.ts‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
520520
// `ask()` calls in flight. A drain must not submit while one is pending: the
521521
// submission would answer that ask with text written for another turn.
522522
private pendingAskCount = 0
523+
// Queued feedback whose `user_feedback` row is already in `clineMessages`, so
524+
// a retry or reclaim re-saves it instead of appending a duplicate row.
525+
private queuedFeedbackRowWritten = new Set<string>()
523526

524527
// Streaming
525528
isWaitingForFirstChunk = false
@@ -1115,7 +1118,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11151118
text?: string,
11161119
images?: string[],
11171120
): Promise<boolean> {
1118-
await this.say("user_feedback", text ?? "", images)
1121+
// Append the row once, before the retry loop: a retry only re-attempts the
1122+
// durable save, and a reclaimed message already has its row.
1123+
if (!this.queuedFeedbackRowWritten.has(messageId)) {
1124+
await this.say("user_feedback", text ?? "", images)
1125+
this.queuedFeedbackRowWritten.add(messageId)
1126+
}
11191127
for (let attempt = 0; attempt <= QUEUED_FEEDBACK_SAVE_RETRY_DELAYS_MS.length; attempt++) {
11201128
if (this.abort) {
11211129
this.releaseUnconsumedQueuedMessage(messageId)
@@ -1265,15 +1273,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12651273
private consumeQueuedMessage(messageId: string): void {
12661274
this.messageQueueService.removeMessage(messageId)
12671275
this.drainRetainedQueuedMessageIds.delete(messageId)
1276+
this.queuedFeedbackRowWritten.delete(messageId)
12681277
}
12691278

12701279
/**
1271-
* Undo a claim whose consume did not happen.
1272-
*
1273-
* A drain submitted this message and no ask has delivered it to the model
1274-
* yet, so the claim is released for a later consumer and the retention record
1275-
* keeps it out of every `yesButtonClicked`-mapped ask. Releasing without that
1276-
* record would re-arm it as fresh input for the next command ask to approve.
1280+
* Undo a claim whose consume did not happen: release it for a later consumer
1281+
* and re-arm the retention record, so it cannot look like fresh input for the
1282+
* next command ask to approve.
12771283
*/
12781284
private releaseUnconsumedQueuedMessage(messageId: string): void {
12791285
this.drainRetainedQueuedMessageIds.add(messageId)
@@ -1411,18 +1417,19 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
14111417
}
14121418

14131419
/**
1414-
* Queued messages this ask must not claim. `queuedResponseForAsk` answers a
1420+
* Queued messages this ask must not claim: `queuedResponseForAsk` answers a
14151421
* command, tool, or use_mcp_server ask with `yesButtonClicked`, so claiming a
1416-
* drain-retained message there would turn between-turn feedback into an
1417-
* approval with `checkAutoApproval` skipped.
1422+
* drain-retained message would turn between-turn feedback into an approval
1423+
* with `checkAutoApproval` skipped.
14181424
*/
14191425
private queuedClaimExclusionsForAsk(type: ClineAsk, text?: string): ReadonlySet<string> | undefined {
14201426
// Drop IDs the queue no longer holds (deleted from the webview or the API)
1421-
// so the record cannot outlive its message.
1427+
// so neither record can outlive its message.
14221428
const queuedIds = new Set(this.messageQueueService.messages.map((message) => message.id))
14231429
for (const id of this.drainRetainedQueuedMessageIds) {
14241430
if (!queuedIds.has(id)) {
14251431
this.drainRetainedQueuedMessageIds.delete(id)
1432+
this.queuedFeedbackRowWritten.delete(id)
14261433
}
14271434
}
14281435

@@ -6557,9 +6564,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
65576564
* `queuedResponseForAsk` mapping answers `yesButtonClicked` excludes those
65586565
* IDs, so the retained text cannot silently approve a later execution ask.
65596566
*
6560-
* A drain also never submits while an `ask()` is pending: the submission
6561-
* would answer that ask with text written for another turn. Drains are
6562-
* serialized per task, so two of them cannot interleave into the single slot.
6567+
* A drain never submits while an `ask()` is pending, and drains are
6568+
* serialized per task, so two of them cannot interleave into the slot.
65636569
*
65646570
* @returns true only when this call submitted a message. A retained queue
65656571
* head reports false: it was already submitted, and the single ask-response

‎src/core/task/__tests__/ask-auto-deny.spec.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ function buildTask(provider: ProviderStub, taskCwd: string) {
3939
task["lastMessageTs"] = undefined
4040
task["pendingAskCount"] = 0
4141
task["drainRetainedQueuedMessageIds"] = new Set<string>()
42+
task["queuedFeedbackRowWritten"] = new Set<string>()
4243
task["addToClineMessages"] = vi.fn(async () => {})
4344
task["saveClineMessages"] = vi.fn(async () => true)
4445
task["updateClineMessage"] = vi.fn(async () => {})

‎src/core/task/__tests__/ask-queued-message-drain.spec.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ describe("Task.ask queued message drain", () => {
2424
;(task as any).lastMessageTs = undefined
2525
task["pendingAskCount"] = 0
2626
task["drainRetainedQueuedMessageIds"] = new Set<string>()
27+
task["queuedFeedbackRowWritten"] = new Set<string>()
2728
return import("../../message-queue/MessageQueueService").then(({ MessageQueueService }) => {
2829
;(task as any).messageQueueService = new MessageQueueService()
2930
;(task as any).addToClineMessages = vi.fn(async () => {})
@@ -465,4 +466,34 @@ describe("Task.ask queued message drain", () => {
465466
vi.useRealTimers()
466467
}
467468
})
469+
470+
it("does not append a second user_feedback row when a reclaimed message is persisted again", async () => {
471+
const task = await createTask()
472+
task.messageQueueService.addMessage("Persist me twice")
473+
const result = await task.ask("completion_result", "Done", false)
474+
const access = getQueueTaskTestAccess(task)
475+
const say = vi.fn().mockResolvedValue(undefined)
476+
access.say = say
477+
478+
// First attempt appends the row, then fails every save and releases the
479+
// claim, so the message is reclaimed as fresh input for a later turn.
480+
access.saveClineMessages = vi.fn().mockResolvedValue(false)
481+
vi.useFakeTimers()
482+
try {
483+
const first = task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images)
484+
await vi.runAllTimersAsync()
485+
await expect(first).resolves.toBe(false)
486+
} finally {
487+
vi.useRealTimers()
488+
}
489+
expect(say).toHaveBeenCalledTimes(1)
490+
491+
// The retry succeeds, but the row it would append is already in history.
492+
access.saveClineMessages = vi.fn().mockResolvedValue(true)
493+
await expect(
494+
task.persistQueuedFeedbackAndAcknowledge(result.queuedMessageId!, result.text, result.images),
495+
).resolves.toBe(true)
496+
expect(say).toHaveBeenCalledTimes(1)
497+
expect(task.messageQueueService.isEmpty()).toBe(true)
498+
})
468499
})

‎src/core/tools/ExecuteCommandTool.ts‎

Lines changed: 16 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,9 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
331331
// settle delay while the result is still pending. Every site publishes
332332
// through `publishToolResult`, which settles the signal with the
333333
// publication outcome, so the drain waits on a structural signal and
334-
// skips after a failed publication.
334+
// skips after a failed publication. Every throw path must settle it
335+
// too: the drain awaits it, so an unsettled promise would hang that
336+
// chain and hold task references forever. Settling is idempotent.
335337
let settleToolResultPublished: ((published: boolean) => void) | undefined
336338
const toolResultPublished = new Promise<boolean>((resolve) => {
337339
settleToolResultPublished = resolve
@@ -392,10 +394,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
392394
publishToolResult(result)
393395
shouldDrainQueuedMessages = commandSubmitted
394396
} catch (fallbackError) {
395-
// The retry never published: settle the signal (and
396-
// rethrow) so the background-completion drain chain
397-
// awaiting toolResultPublished cannot hang forever and
398-
// hold task references. Settling is idempotent.
397+
// The retry never published: settle before rethrowing.
399398
settleToolResultPublished?.(false)
400399
throw fallbackError
401400
}
@@ -407,8 +406,7 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
407406
"Command was submitted in the VS Code terminal, but shell integration did not report its output or completion status. Do not run the command again automatically.",
408407
)
409408
} catch (warningError) {
410-
// Same hang hazard as the fallback arm: a throw before
411-
// publishToolResult leaves the signal unsettled.
409+
// A throw before publishToolResult leaves the signal unsettled.
412410
settleToolResultPublished?.(false)
413411
throw warningError
414412
}
@@ -419,14 +417,8 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
419417
} else {
420418
// Ordinary execution error (e.g. the terminal failed to start) —
421419
// not a shell-integration failure, so it must not emit the
422-
// shell-integration warning; surface it through the tool's
423-
// error path instead.
424-
// Settle the publication signal before the error bubbles: the
425-
// background-completion drain chain captured toolResultPublished
426-
// and awaits it, so an unresolved promise would hang that chain
427-
// (and hold task references) forever. The outcome is false, so
428-
// the chain never drains after a failed publication. Settling
429-
// is idempotent.
420+
// shell-integration warning; surface it through the tool's error
421+
// path instead. Settle first: false skips the drain.
430422
settleToolResultPublished?.(false)
431423
throw error
432424
}
@@ -553,10 +545,9 @@ export async function executeCommandInTerminal(
553545
let lastCommandOutputEmitAt = 0
554546
let pendingCommandOutputEmitTimer: NodeJS.Timeout | undefined
555547
let commandOutputSayChain: Promise<void> = Promise.resolve()
556-
// Outcome of the most recent `command_output` publication. The final forced
557-
// publication is the last one queued before the background drain, so after it
558-
// settles this reports whether that output reached the webview. A later
559-
// success clears an earlier partial failure: only the final output matters.
548+
// Outcome of the most recent `command_output` publication: the final forced
549+
// one is the last queued before the background drain, so it reports whether
550+
// that output reached the webview. A later success clears an earlier failure.
560551
let commandOutputPublishFailed = false
561552

562553
const queueCommandOutputMessage = (text: string, partial: boolean, force = false): Promise<void> => {
@@ -670,17 +661,12 @@ export async function executeCommandInTerminal(
670661
return
671662
}
672663
const published = await toolResultPublished
673-
// A task cancelled while the command finished in the
674-
// background must not drain: the guards inside
675-
// processQueuedMessages would no-op anyway, and skipping here
676-
// avoids pointless work plus error-log noise on the dying task.
677-
if (task.abort || task.abandoned) {
678-
return
679-
}
680-
// A failed publication must not drain either: handleError
681-
// reports the command error first, and the queued message
682-
// waits for a later turn.
683-
if (!published) {
664+
// A task cancelled while the command finished in the background
665+
// must not drain: the guards inside processQueuedMessages would
666+
// no-op anyway, and skipping avoids error-log noise on the dying
667+
// task. A failed publication must not drain either: handleError
668+
// reports the command error first.
669+
if (task.abort || task.abandoned || !published) {
684670
return
685671
}
686672
// The final `command_output` say is part of what the drain depends

‎src/core/webview/__tests__/webviewMessageHandler.spec.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,6 +1009,41 @@ describe("webviewMessageHandler - image mentions", () => {
10091009
expect(taskA.handleWebviewAskResponse).toHaveBeenCalledTimes(type === "askResponse" ? 1 : 0)
10101010
expect(taskB.handleWebviewAskResponse).not.toHaveBeenCalled()
10111011
})
1012+
1013+
// Mentions must resolve against the workspace of the task the message is
1014+
// delivered to. The cwd is read before the handler's own awaits, so a task
1015+
// switch during the state read cannot move resolution to another workspace.
1016+
it("resolves mentions against the workspace captured before the state await", async () => {
1017+
const taskA = {
1018+
cwd: "/workspace-a",
1019+
rooIgnoreController: undefined,
1020+
messageQueueService: { addMessage: vi.fn() },
1021+
handleWebviewAskResponse: vi.fn(),
1022+
}
1023+
const taskB = {
1024+
cwd: "/workspace-b",
1025+
rooIgnoreController: undefined,
1026+
messageQueueService: { addMessage: vi.fn() },
1027+
handleWebviewAskResponse: vi.fn(),
1028+
}
1029+
1030+
let currentTask = taskA
1031+
vi.mocked(mockClineProvider.getCurrentTask).mockImplementation(() => currentTask as never)
1032+
// The switch lands while the handler is suspended on the state read, before
1033+
// mention resolution starts.
1034+
mockClineProvider.getState = vi.fn().mockImplementation(async () => {
1035+
currentTask = taskB
1036+
return { maxImageFileSize: 5, maxTotalImageSize: 20 }
1037+
})
1038+
1039+
await webviewMessageHandler(mockClineProvider, {
1040+
type: "queueMessage",
1041+
text: "see @/img.png",
1042+
images: [],
1043+
})
1044+
1045+
expect(vi.mocked(resolveImageMentions)).toHaveBeenCalledWith(expect.objectContaining({ cwd: "/workspace-a" }))
1046+
})
10121047
})
10131048

10141049
describe("webviewMessageHandler - requestOllamaModels", () => {

‎src/core/webview/webviewMessageHandler.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,16 +223,20 @@ export const webviewMessageHandler = async (
223223
/**
224224
* Resolves image file mentions in incoming messages.
225225
* Matches read_file behavior: respects size limits and model capabilities.
226+
*
227+
* The cwd is read before the awaits below: it must be the workspace of the
228+
* task this message belongs to, which callers capture before their own await.
226229
*/
227230
const resolveIncomingImages = async (payload: { text?: string; images?: string[] }) => {
228231
const text = payload.text ?? ""
229232
const images = payload.images
230233
const currentTask = provider.getCurrentTask()
234+
const cwd = getCurrentCwd()
231235
const state = await provider.getState()
232236
const resolved = await resolveImageMentions({
233237
text,
234238
images,
235-
cwd: getCurrentCwd(),
239+
cwd,
236240
rooIgnoreController: currentTask?.rooIgnoreController,
237241
maxImageFileSize: state.maxImageFileSize,
238242
maxTotalImageSize: state.maxTotalImageSize,

0 commit comments

Comments
 (0)