fix: address PR 27 review feedback

Author: doctarock

src/core/task/__tests__/apiConversationHistory.spec.ts

@@ -0,0 +1,107 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"
+
+import { prepareApiConversationMessage } from "../apiConversationHistory.js"
+
+describe("prepareApiConversationMessage", () => {
+	beforeEach(() => {
+		vi.useFakeTimers()
+		vi.setSystemTime(new Date("2026-01-02T03:04:05.000Z"))
+	})
+
+	afterEach(() => {
+		vi.useRealTimers()
+	})
+
+	it("prepends Anthropic thinking blocks with thought signatures", () => {
+		const result = prepareApiConversationMessage({
+			message: { role: "assistant", content: "answer" },
+			reasoning: "private reasoning",
+			api: {
+				getResponseId: () => "response-1",
+				getThoughtSignature: () => "signature-1",
+			} as any,
+			apiConfiguration: { apiProvider: "anthropic", apiModelId: "claude-3-5-sonnet" } as any,
+			apiConversationHistory: [],
+		}) as any
+
+		expect(result).toMatchObject({
+			role: "assistant",
+			id: "response-1",
+			ts: Date.now(),
+		})
+		expect(result.content).toEqual([
+			{ type: "thinking", thinking: "private reasoning", signature: "signature-1" },
+			{ type: "text", text: "answer" },
+		])
+	})
+
+	it("prepends non-Anthropic reasoning blocks without dead summary fallback", () => {
+		const result = prepareApiConversationMessage({
+			message: { role: "assistant", content: "answer" },
+			reasoning: "visible reasoning",
+			api: {} as any,
+			apiConfiguration: { apiProvider: "openrouter", openRouterModelId: "openai/gpt-4" } as any,
+			apiConversationHistory: [],
+		}) as any
+
+		expect(result.content).toEqual([
+			{ type: "reasoning", text: "visible reasoning" },
+			{ type: "text", text: "answer" },
+		])
+		expect(result.content[0]).not.toHaveProperty("summary")
+	})
+
+	it("preserves encrypted reasoning content", () => {
+		const result = prepareApiConversationMessage({
+			message: { role: "assistant", content: [{ type: "text", text: "answer" }] },
+			api: {
+				getEncryptedContent: () => ({ encrypted_content: "encrypted", id: "reasoning-1" }),
+			} as any,
+			apiConfiguration: { apiProvider: "openrouter", openRouterModelId: "openai/gpt-4" } as any,
+			apiConversationHistory: [],
+		}) as any
+
+		expect(result.content).toEqual([
+			{ type: "reasoning", summary: [], encrypted_content: "encrypted", id: "reasoning-1" },
+			{ type: "text", text: "answer" },
+		])
+	})
+
+	it("appends thought signatures for non-Anthropic protocols", () => {
+		const result = prepareApiConversationMessage({
+			message: { role: "assistant", content: "answer" },
+			api: {
+				getThoughtSignature: () => "signature-1",
+				getReasoningDetails: () => [{ type: "reasoning", text: "detail" }],
+			} as any,
+			apiConfiguration: { apiProvider: "openrouter", openRouterModelId: "openai/gpt-4" } as any,
+			apiConversationHistory: [],
+		}) as any
+
+		expect(result.reasoning_details).toEqual([{ type: "reasoning", text: "detail" }])
+		expect(result.content).toEqual([
+			{ type: "text", text: "answer" },
+			{ type: "thoughtSignature", thoughtSignature: "signature-1" },
+		])
+	})
+
+	it("validates user tool_result blocks against the last effective assistant message", () => {
+		const result = prepareApiConversationMessage({
+			message: {
+				role: "user",
+				content: [{ type: "tool_result", tool_use_id: "wrong-id", content: "done" }],
+			},
+			api: {} as any,
+			apiConfiguration: { apiProvider: "openrouter", openRouterModelId: "openai/gpt-4" } as any,
+			apiConversationHistory: [
+				{
+					role: "assistant",
+					content: [{ type: "tool_use", id: "tool-1", name: "read_file", input: {} }],
+				} as any,
+			],
+		}) as any
+
+		expect(result.content).toEqual([{ type: "tool_result", tool_use_id: "tool-1", content: "done" }])
+		expect(result.ts).toBe(Date.now())
+	})
+})

src/core/task/apiConversationHistory.ts

@@ -11,7 +11,6 @@ type ApiHistoryHandler = ApiHandler & {
 	getResponseId?: () => string | undefined
 	getEncryptedContent?: () => { encrypted_content: string; id?: string } | undefined
 	getThoughtSignature?: () => string | undefined
-	getSummary?: () => any[] | undefined
 	getReasoningDetails?: () => any[] | undefined
 }
 
@@ -46,7 +45,6 @@ function prepareAssistantMessage(
 	const responseId = handler.getResponseId?.()
 	const reasoningData = handler.getEncryptedContent?.()
 	const thoughtSignature = handler.getThoughtSignature?.()
-	const reasoningSummary = handler.getSummary?.()
 	const reasoningDetails = handler.getReasoningDetails?.()
 
 	const modelId = getModelId(apiConfiguration)
@@ -79,7 +77,6 @@ function prepareAssistantMessage(
 		const reasoningBlock = {
 			type: "reasoning",
 			text: reasoning,
-			summary: reasoningSummary ?? ([] as any[]),
 		}
 
 		prependContentBlock(messageWithTs, reasoningBlock)

src/core/webview/PendingEditOperationStore.ts

@@ -9,6 +9,7 @@ export interface PendingEditOperation {
 }
 
 export type PendingEditOperationInput = Omit<PendingEditOperation, "timeoutId" | "createdAt">
+export type PendingEditOperationView = Omit<PendingEditOperation, "timeoutId" | "createdAt">
 
 export class PendingEditOperationStore {
 	private readonly operations = new Map<string, PendingEditOperation>()
@@ -23,7 +24,7 @@ export class PendingEditOperationStore {
 
 		const timeoutId = setTimeout(() => {
 			this.clear(operationId)
-			this.log(`[setPendingEditOperation] Automatically cleared stale pending operation: ${operationId}`)
+			this.log(`[PendingEditOperationStore.set] Automatically cleared stale pending operation: ${operationId}`)
 		}, this.timeoutMs)
 
 		this.operations.set(operationId, {
@@ -32,11 +33,22 @@ export class PendingEditOperationStore {
 			createdAt: Date.now(),
 		})
 
-		this.log(`[setPendingEditOperation] Set pending operation: ${operationId}`)
+		this.log(`[PendingEditOperationStore.set] Set pending operation: ${operationId}`)
 	}
 
-	get(operationId: string): PendingEditOperation | undefined {
-		return this.operations.get(operationId)
+	get(operationId: string): PendingEditOperationView | undefined {
+		const operation = this.operations.get(operationId)
+		if (!operation) {
+			return undefined
+		}
+
+		return {
+			messageTs: operation.messageTs,
+			editedContent: operation.editedContent,
+			images: operation.images,
+			messageIndex: operation.messageIndex,
+			apiConversationHistoryIndex: operation.apiConversationHistoryIndex,
+		}
 	}
 
 	clear(operationId: string): boolean {
@@ -47,7 +59,7 @@ export class PendingEditOperationStore {
 
 		clearTimeout(operation.timeoutId)
 		this.operations.delete(operationId)
-		this.log(`[clearPendingEditOperation] Cleared pending operation: ${operationId}`)
+		this.log(`[PendingEditOperationStore.clear] Cleared pending operation: ${operationId}`)
 		return true
 	}
 
@@ -57,6 +69,6 @@ export class PendingEditOperationStore {
 		}
 
 		this.operations.clear()
-		this.log("[clearAllPendingEditOperations] Cleared all pending operations")
+		this.log("[PendingEditOperationStore.clearAll] Cleared all pending operations")
 	}
 }

src/core/webview/__tests__/PendingEditOperationStore.spec.ts

@@ -0,0 +1,85 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"
+
+import { PendingEditOperationStore, type PendingEditOperationInput } from "../PendingEditOperationStore.js"
+
+describe("PendingEditOperationStore", () => {
+	const editData: PendingEditOperationInput = {
+		messageTs: 123,
+		editedContent: "edited",
+		images: ["image.png"],
+		messageIndex: 1,
+		apiConversationHistoryIndex: 2,
+	}
+
+	let log: ReturnType<typeof vi.fn>
+	let store: PendingEditOperationStore
+
+	beforeEach(() => {
+		vi.useFakeTimers()
+		log = vi.fn()
+		store = new PendingEditOperationStore(1_000, log)
+	})
+
+	afterEach(() => {
+		vi.useRealTimers()
+	})
+
+	it("clears a prior operation with the same id", () => {
+		store.set("op", editData)
+		vi.advanceTimersByTime(500)
+
+		store.set("op", { ...editData, editedContent: "replacement" })
+		vi.advanceTimersByTime(500)
+
+		expect(store.get("op")).toMatchObject({ editedContent: "replacement" })
+		expect(log).toHaveBeenCalledWith("[PendingEditOperationStore.clear] Cleared pending operation: op")
+
+		vi.advanceTimersByTime(500)
+
+		expect(store.get("op")).toBeUndefined()
+	})
+
+	it("returns false on clear miss and true on hit", () => {
+		expect(store.clear("missing")).toBe(false)
+
+		store.set("op", editData)
+
+		expect(store.clear("op")).toBe(true)
+		expect(store.get("op")).toBeUndefined()
+	})
+
+	it("hides timer metadata from get results", () => {
+		store.set("op", editData)
+
+		const operation = store.get("op")
+
+		expect(operation).toEqual(editData)
+		expect(operation).not.toHaveProperty("timeoutId")
+		expect(operation).not.toHaveProperty("createdAt")
+	})
+
+	it("auto-clears after timeoutMs and logs", () => {
+		store.set("op", editData)
+
+		vi.advanceTimersByTime(1_000)
+
+		expect(store.get("op")).toBeUndefined()
+		expect(log).toHaveBeenCalledWith(
+			"[PendingEditOperationStore.set] Automatically cleared stale pending operation: op",
+		)
+	})
+
+	it("clearAll clears timers without later auto-clear logs", () => {
+		store.set("op-1", editData)
+		store.set("op-2", { ...editData, editedContent: "second" })
+
+		store.clearAll()
+		log.mockClear()
+
+		vi.advanceTimersByTime(1_000)
+
+		expect(store.get("op-1")).toBeUndefined()
+		expect(store.get("op-2")).toBeUndefined()
+		expect(log).not.toHaveBeenCalled()
+	})
+})