From c2012c6027b9cbdfcb53fcbeff2b109855c79658 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Mon, 16 Feb 2026 15:23:45 +0900 Subject: [PATCH] fix: address 8-domain Oracle review findings (C1, C2, M1-M4) - C1: thinking-prepend unique part IDs per message (global PK collision) - C2: recover-thinking-disabled-violation try/catch guard on SDK call - M1: remove non-schema truncated/originalSize fields from SDK interfaces - M2: messageHasContentFromSDK treats thinking-only messages as non-empty - M3: syncAllTasksToTodos persists finalTodos + no-id rename dedup guard - M4: AbortSignal.timeout(30s) on HTTP fetch calls in opencode-http-api All 2739 tests pass, typecheck clean. --- .../empty-content-recovery-sdk.ts | 12 ++- .../message-builder.ts | 12 ++- .../pruning-tool-output-truncation.ts | 6 +- .../tool-result-storage-sdk.ts | 8 +- .../recover-thinking-disabled-violation.ts | 51 +++++++----- .../storage/thinking-prepend.ts | 4 +- src/shared/opencode-http-api.test.ts | 10 ++- src/shared/opencode-http-api.ts | 2 + src/tools/task/todo-sync.test.ts | 78 ++++++++++++++++++- src/tools/task/todo-sync.ts | 11 ++- 10 files changed, 148 insertions(+), 46 deletions(-) diff --git a/src/hooks/anthropic-context-window-limit-recovery/empty-content-recovery-sdk.ts b/src/hooks/anthropic-context-window-limit-recovery/empty-content-recovery-sdk.ts index ccafb145..05cf5b44 100644 --- a/src/hooks/anthropic-context-window-limit-recovery/empty-content-recovery-sdk.ts +++ b/src/hooks/anthropic-context-window-limit-recovery/empty-content-recovery-sdk.ts @@ -20,10 +20,15 @@ function messageHasContentFromSDK(message: SDKMessage): boolean { const parts = message.parts if (!parts || parts.length === 0) return false + let hasIgnoredParts = false + for (const part of parts) { const type = part.type if (!type) continue - if (IGNORE_TYPES.has(type)) continue + if (IGNORE_TYPES.has(type)) { + hasIgnoredParts = true + continue + } if (type === "text") { if (part.text?.trim()) return true @@ -31,9 +36,12 @@ function messageHasContentFromSDK(message: SDKMessage): boolean { } if (TOOL_TYPES.has(type)) return true + + return true } - return false + // Messages with only thinking/meta parts are NOT empty — they have content + return hasIgnoredParts } function getSdkMessages(response: unknown): SDKMessage[] { diff --git a/src/hooks/anthropic-context-window-limit-recovery/message-builder.ts b/src/hooks/anthropic-context-window-limit-recovery/message-builder.ts index 1482c154..aedfbf5c 100644 --- a/src/hooks/anthropic-context-window-limit-recovery/message-builder.ts +++ b/src/hooks/anthropic-context-window-limit-recovery/message-builder.ts @@ -31,10 +31,15 @@ function messageHasContentFromSDK(message: SDKMessage): boolean { const parts = message.parts if (!parts || parts.length === 0) return false + let hasIgnoredParts = false + for (const part of parts) { const type = part.type if (!type) continue - if (IGNORE_TYPES.has(type)) continue + if (IGNORE_TYPES.has(type)) { + hasIgnoredParts = true + continue + } if (type === "text") { if (part.text?.trim()) return true @@ -42,9 +47,12 @@ function messageHasContentFromSDK(message: SDKMessage): boolean { } if (TOOL_TYPES.has(type)) return true + + return true } - return false + // Messages with only thinking/meta parts are NOT empty — they have content + return hasIgnoredParts } async function findEmptyMessageIdsFromSDK( diff --git a/src/hooks/anthropic-context-window-limit-recovery/pruning-tool-output-truncation.ts b/src/hooks/anthropic-context-window-limit-recovery/pruning-tool-output-truncation.ts index 3db4ec8b..69c9ff7f 100644 --- a/src/hooks/anthropic-context-window-limit-recovery/pruning-tool-output-truncation.ts +++ b/src/hooks/anthropic-context-window-limit-recovery/pruning-tool-output-truncation.ts @@ -24,9 +24,7 @@ interface SDKToolPart { type: string callID?: string tool?: string - state?: { output?: string } - truncated?: boolean - originalSize?: number + state?: { output?: string; time?: { compacted?: number } } } interface SDKMessage { @@ -120,7 +118,7 @@ async function truncateToolOutputsByCallIdFromSDK( for (const part of msg.parts) { if (part.type !== "tool" || !part.callID) continue if (!callIds.has(part.callID)) continue - if (!part.state?.output || part.truncated) continue + if (!part.state?.output || part.state?.time?.compacted) continue const result = await truncateToolResultAsync(client, sessionID, messageID, part.id, part) if (result.success) { diff --git a/src/hooks/anthropic-context-window-limit-recovery/tool-result-storage-sdk.ts b/src/hooks/anthropic-context-window-limit-recovery/tool-result-storage-sdk.ts index 2db298d3..c0b71075 100644 --- a/src/hooks/anthropic-context-window-limit-recovery/tool-result-storage-sdk.ts +++ b/src/hooks/anthropic-context-window-limit-recovery/tool-result-storage-sdk.ts @@ -19,8 +19,6 @@ interface SDKToolPart { error?: string time?: { start?: number; end?: number; compacted?: number } } - truncated?: boolean - originalSize?: number } interface SDKMessage { @@ -42,7 +40,7 @@ export async function findToolResultsBySizeFromSDK( if (!messageID || !msg.parts) continue for (const part of msg.parts) { - if (part.type === "tool" && part.state?.output && !part.truncated && part.tool) { + if (part.type === "tool" && part.state?.output && !part.state?.time?.compacted && part.tool) { results.push({ partPath: "", partId: part.id, @@ -74,8 +72,6 @@ export async function truncateToolResultAsync( const updatedPart: Record = { ...part, - truncated: true, - originalSize, state: { ...part.state, output: TRUNCATION_MESSAGE, @@ -108,7 +104,7 @@ export async function countTruncatedResultsFromSDK( for (const msg of messages) { if (!msg.parts) continue for (const part of msg.parts) { - if (part.truncated === true) count++ + if (part.state?.time?.compacted) count++ } } diff --git a/src/hooks/session-recovery/recover-thinking-disabled-violation.ts b/src/hooks/session-recovery/recover-thinking-disabled-violation.ts index 44e7a3f5..cdb6556d 100644 --- a/src/hooks/session-recovery/recover-thinking-disabled-violation.ts +++ b/src/hooks/session-recovery/recover-thinking-disabled-violation.ts @@ -4,6 +4,7 @@ import { findMessagesWithThinkingBlocks, stripThinkingParts } from "./storage" import { isSqliteBackend } from "../../shared/opencode-storage-detection" import { stripThinkingPartsAsync } from "./storage/thinking-strip" import { THINKING_TYPES } from "./constants" +import { log } from "../../shared/logger" type Client = ReturnType @@ -35,31 +36,39 @@ async function recoverThinkingDisabledViolationFromSDK( client: Client, sessionID: string ): Promise { - const response = await client.session.messages({ path: { id: sessionID } }) - const messages = (response.data ?? []) as MessageData[] + try { + const response = await client.session.messages({ path: { id: sessionID } }) + const messages = (response.data ?? []) as MessageData[] - const messageIDsWithThinking: string[] = [] - for (const msg of messages) { - if (msg.info?.role !== "assistant") continue - if (!msg.info?.id) continue - if (!msg.parts) continue + const messageIDsWithThinking: string[] = [] + for (const msg of messages) { + if (msg.info?.role !== "assistant") continue + if (!msg.info?.id) continue + if (!msg.parts) continue - const hasThinking = msg.parts.some((part) => THINKING_TYPES.has(part.type)) - if (hasThinking) { - messageIDsWithThinking.push(msg.info.id) + const hasThinking = msg.parts.some((part) => THINKING_TYPES.has(part.type)) + if (hasThinking) { + messageIDsWithThinking.push(msg.info.id) + } } - } - if (messageIDsWithThinking.length === 0) { + if (messageIDsWithThinking.length === 0) { + return false + } + + let anySuccess = false + for (const messageID of messageIDsWithThinking) { + if (await stripThinkingPartsAsync(client, sessionID, messageID)) { + anySuccess = true + } + } + + return anySuccess + } catch (error) { + log("[session-recovery] recoverThinkingDisabledViolationFromSDK failed", { + sessionID, + error: String(error), + }) return false } - - let anySuccess = false - for (const messageID of messageIDsWithThinking) { - if (await stripThinkingPartsAsync(client, sessionID, messageID)) { - anySuccess = true - } - } - - return anySuccess } diff --git a/src/hooks/session-recovery/storage/thinking-prepend.ts b/src/hooks/session-recovery/storage/thinking-prepend.ts index c63a57fb..476eadb4 100644 --- a/src/hooks/session-recovery/storage/thinking-prepend.ts +++ b/src/hooks/session-recovery/storage/thinking-prepend.ts @@ -49,7 +49,7 @@ export function prependThinkingPart(sessionID: string, messageID: string): boole const previousThinking = findLastThinkingContent(sessionID, messageID) - const partId = "prt_0000000000_thinking" + const partId = `prt_0000000000_${messageID}_thinking` const part = { id: partId, sessionID, @@ -104,7 +104,7 @@ export async function prependThinkingPartAsync( ): Promise { const previousThinking = await findLastThinkingContentFromSDK(client, sessionID, messageID) - const partId = "prt_0000000000_thinking" + const partId = `prt_0000000000_${messageID}_thinking` const part: Record = { id: partId, sessionID, diff --git a/src/shared/opencode-http-api.test.ts b/src/shared/opencode-http-api.test.ts index fc5538b4..80b86bae 100644 --- a/src/shared/opencode-http-api.test.ts +++ b/src/shared/opencode-http-api.test.ts @@ -87,14 +87,15 @@ describe("patchPart", () => { expect(result).toBe(true) expect(mockFetch).toHaveBeenCalledWith( "https://api.example.com/session/ses123/message/msg456/part/part789", - { + expect.objectContaining({ method: "PATCH", headers: { "Content-Type": "application/json", "Authorization": "Basic b3BlbmNvZGU6dGVzdHBhc3N3b3Jk", }, body: JSON.stringify(body), - } + signal: expect.any(AbortSignal), + }) ) }) @@ -145,12 +146,13 @@ describe("deletePart", () => { expect(result).toBe(true) expect(mockFetch).toHaveBeenCalledWith( "https://api.example.com/session/ses123/message/msg456/part/part789", - { + expect.objectContaining({ method: "DELETE", headers: { "Authorization": "Basic b3BlbmNvZGU6dGVzdHBhc3N3b3Jk", }, - } + signal: expect.any(AbortSignal), + }) ) }) diff --git a/src/shared/opencode-http-api.ts b/src/shared/opencode-http-api.ts index 69942afc..618224a7 100644 --- a/src/shared/opencode-http-api.ts +++ b/src/shared/opencode-http-api.ts @@ -81,6 +81,7 @@ export async function patchPart( "Authorization": auth, }, body: JSON.stringify(body), + signal: AbortSignal.timeout(30_000), }) if (!response.ok) { @@ -122,6 +123,7 @@ export async function deletePart( headers: { "Authorization": auth, }, + signal: AbortSignal.timeout(30_000), }) if (!response.ok) { diff --git a/src/tools/task/todo-sync.test.ts b/src/tools/task/todo-sync.test.ts index 8c4468d5..e35d1978 100644 --- a/src/tools/task/todo-sync.test.ts +++ b/src/tools/task/todo-sync.test.ts @@ -418,12 +418,16 @@ describe("syncAllTasksToTodos", () => { }, ]; mockCtx.client.session.todo.mockResolvedValue(currentTodos); + let writtenTodos: TodoInfo[] = []; + const writer = async (input: { sessionID: string; todos: TodoInfo[] }) => { + writtenTodos = input.todos; + }; // when - await syncAllTasksToTodos(mockCtx, tasks, "session-1"); + await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer); // then - expect(mockCtx.client.session.todo).toHaveBeenCalled(); + expect(writtenTodos.some((t: TodoInfo) => t.id === "T-1")).toBe(false); }); it("preserves existing todos not in task list", async () => { @@ -451,12 +455,17 @@ describe("syncAllTasksToTodos", () => { }, ]; mockCtx.client.session.todo.mockResolvedValue(currentTodos); + let writtenTodos: TodoInfo[] = []; + const writer = async (input: { sessionID: string; todos: TodoInfo[] }) => { + writtenTodos = input.todos; + }; // when - await syncAllTasksToTodos(mockCtx, tasks, "session-1"); + await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer); // then - expect(mockCtx.client.session.todo).toHaveBeenCalled(); + expect(writtenTodos.some((t: TodoInfo) => t.id === "T-existing")).toBe(true); + expect(writtenTodos.some((t: TodoInfo) => t.content === "Task 1")).toBe(true); }); it("handles empty task list", async () => { @@ -471,6 +480,67 @@ describe("syncAllTasksToTodos", () => { expect(mockCtx.client.session.todo).toHaveBeenCalled(); }); + it("calls writer with final todos", async () => { + // given + const tasks: Task[] = [ + { + id: "T-1", + subject: "Task 1", + description: "Description 1", + status: "pending", + blocks: [], + blockedBy: [], + }, + ]; + mockCtx.client.session.todo.mockResolvedValue([]); + let writerCalled = false; + const writer = async (input: { sessionID: string; todos: TodoInfo[] }) => { + writerCalled = true; + expect(input.sessionID).toBe("session-1"); + expect(input.todos.length).toBe(1); + expect(input.todos[0].content).toBe("Task 1"); + }; + + // when + await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer); + + // then + expect(writerCalled).toBe(true); + }); + + it("deduplicates no-id todos when task replaces existing content", async () => { + // given + const tasks: Task[] = [ + { + id: "T-1", + subject: "Task 1 (updated)", + description: "Description 1", + status: "in_progress", + blocks: [], + blockedBy: [], + }, + ]; + const currentTodos: TodoInfo[] = [ + { + content: "Task 1 (updated)", + status: "pending", + }, + ]; + mockCtx.client.session.todo.mockResolvedValue(currentTodos); + let writtenTodos: TodoInfo[] = []; + const writer = async (input: { sessionID: string; todos: TodoInfo[] }) => { + writtenTodos = input.todos; + }; + + // when + await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer); + + // then — no duplicates + const matching = writtenTodos.filter((t: TodoInfo) => t.content === "Task 1 (updated)"); + expect(matching.length).toBe(1); + expect(matching[0].status).toBe("in_progress"); + }); + it("preserves todos without id field", async () => { // given const tasks: Task[] = [ diff --git a/src/tools/task/todo-sync.ts b/src/tools/task/todo-sync.ts index 68717fe4..c11849f8 100644 --- a/src/tools/task/todo-sync.ts +++ b/src/tools/task/todo-sync.ts @@ -139,6 +139,7 @@ export async function syncAllTasksToTodos( ctx: PluginInput, tasks: Task[], sessionID?: string, + writer?: TodoWriter, ): Promise { try { let currentTodos: TodoInfo[] = []; @@ -156,8 +157,10 @@ export async function syncAllTasksToTodos( const newTodos: TodoInfo[] = []; const tasksToRemove = new Set(); + const allTaskSubjects = new Set(); for (const task of tasks) { + allTaskSubjects.add(task.subject); const todo = syncTaskToTodo(task); if (todo === null) { tasksToRemove.add(task.id); @@ -176,13 +179,19 @@ export async function syncAllTasksToTodos( const isInNewTodos = newTodos.some((newTodo) => todosMatch(existing, newTodo)); const isRemovedById = existing.id ? tasksToRemove.has(existing.id) : false; const isRemovedByContent = !existing.id && removedTaskSubjects.has(existing.content); - if (!isInNewTodos && !isRemovedById && !isRemovedByContent) { + const isReplacedByTask = !existing.id && allTaskSubjects.has(existing.content); + if (!isInNewTodos && !isRemovedById && !isRemovedByContent && !isReplacedByTask) { finalTodos.push(existing); } } finalTodos.push(...newTodos); + const resolvedWriter = writer ?? (await resolveTodoWriter()); + if (resolvedWriter && sessionID) { + await resolvedWriter({ sessionID, todos: finalTodos }); + } + log("[todo-sync] Synced todos", { count: finalTodos.length, sessionID,