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.
This commit is contained in:
parent
106cd5c8b1
commit
c2012c6027
@ -20,10 +20,15 @@ function messageHasContentFromSDK(message: SDKMessage): boolean {
|
|||||||
const parts = message.parts
|
const parts = message.parts
|
||||||
if (!parts || parts.length === 0) return false
|
if (!parts || parts.length === 0) return false
|
||||||
|
|
||||||
|
let hasIgnoredParts = false
|
||||||
|
|
||||||
for (const part of parts) {
|
for (const part of parts) {
|
||||||
const type = part.type
|
const type = part.type
|
||||||
if (!type) continue
|
if (!type) continue
|
||||||
if (IGNORE_TYPES.has(type)) continue
|
if (IGNORE_TYPES.has(type)) {
|
||||||
|
hasIgnoredParts = true
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
if (type === "text") {
|
if (type === "text") {
|
||||||
if (part.text?.trim()) return true
|
if (part.text?.trim()) return true
|
||||||
@ -31,9 +36,12 @@ function messageHasContentFromSDK(message: SDKMessage): boolean {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (TOOL_TYPES.has(type)) return true
|
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[] {
|
function getSdkMessages(response: unknown): SDKMessage[] {
|
||||||
|
|||||||
@ -31,10 +31,15 @@ function messageHasContentFromSDK(message: SDKMessage): boolean {
|
|||||||
const parts = message.parts
|
const parts = message.parts
|
||||||
if (!parts || parts.length === 0) return false
|
if (!parts || parts.length === 0) return false
|
||||||
|
|
||||||
|
let hasIgnoredParts = false
|
||||||
|
|
||||||
for (const part of parts) {
|
for (const part of parts) {
|
||||||
const type = part.type
|
const type = part.type
|
||||||
if (!type) continue
|
if (!type) continue
|
||||||
if (IGNORE_TYPES.has(type)) continue
|
if (IGNORE_TYPES.has(type)) {
|
||||||
|
hasIgnoredParts = true
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
if (type === "text") {
|
if (type === "text") {
|
||||||
if (part.text?.trim()) return true
|
if (part.text?.trim()) return true
|
||||||
@ -42,9 +47,12 @@ function messageHasContentFromSDK(message: SDKMessage): boolean {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (TOOL_TYPES.has(type)) return true
|
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(
|
async function findEmptyMessageIdsFromSDK(
|
||||||
|
|||||||
@ -24,9 +24,7 @@ interface SDKToolPart {
|
|||||||
type: string
|
type: string
|
||||||
callID?: string
|
callID?: string
|
||||||
tool?: string
|
tool?: string
|
||||||
state?: { output?: string }
|
state?: { output?: string; time?: { compacted?: number } }
|
||||||
truncated?: boolean
|
|
||||||
originalSize?: number
|
|
||||||
}
|
}
|
||||||
|
|
||||||
interface SDKMessage {
|
interface SDKMessage {
|
||||||
@ -120,7 +118,7 @@ async function truncateToolOutputsByCallIdFromSDK(
|
|||||||
for (const part of msg.parts) {
|
for (const part of msg.parts) {
|
||||||
if (part.type !== "tool" || !part.callID) continue
|
if (part.type !== "tool" || !part.callID) continue
|
||||||
if (!callIds.has(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)
|
const result = await truncateToolResultAsync(client, sessionID, messageID, part.id, part)
|
||||||
if (result.success) {
|
if (result.success) {
|
||||||
|
|||||||
@ -19,8 +19,6 @@ interface SDKToolPart {
|
|||||||
error?: string
|
error?: string
|
||||||
time?: { start?: number; end?: number; compacted?: number }
|
time?: { start?: number; end?: number; compacted?: number }
|
||||||
}
|
}
|
||||||
truncated?: boolean
|
|
||||||
originalSize?: number
|
|
||||||
}
|
}
|
||||||
|
|
||||||
interface SDKMessage {
|
interface SDKMessage {
|
||||||
@ -42,7 +40,7 @@ export async function findToolResultsBySizeFromSDK(
|
|||||||
if (!messageID || !msg.parts) continue
|
if (!messageID || !msg.parts) continue
|
||||||
|
|
||||||
for (const part of msg.parts) {
|
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({
|
results.push({
|
||||||
partPath: "",
|
partPath: "",
|
||||||
partId: part.id,
|
partId: part.id,
|
||||||
@ -74,8 +72,6 @@ export async function truncateToolResultAsync(
|
|||||||
|
|
||||||
const updatedPart: Record<string, unknown> = {
|
const updatedPart: Record<string, unknown> = {
|
||||||
...part,
|
...part,
|
||||||
truncated: true,
|
|
||||||
originalSize,
|
|
||||||
state: {
|
state: {
|
||||||
...part.state,
|
...part.state,
|
||||||
output: TRUNCATION_MESSAGE,
|
output: TRUNCATION_MESSAGE,
|
||||||
@ -108,7 +104,7 @@ export async function countTruncatedResultsFromSDK(
|
|||||||
for (const msg of messages) {
|
for (const msg of messages) {
|
||||||
if (!msg.parts) continue
|
if (!msg.parts) continue
|
||||||
for (const part of msg.parts) {
|
for (const part of msg.parts) {
|
||||||
if (part.truncated === true) count++
|
if (part.state?.time?.compacted) count++
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -4,6 +4,7 @@ import { findMessagesWithThinkingBlocks, stripThinkingParts } from "./storage"
|
|||||||
import { isSqliteBackend } from "../../shared/opencode-storage-detection"
|
import { isSqliteBackend } from "../../shared/opencode-storage-detection"
|
||||||
import { stripThinkingPartsAsync } from "./storage/thinking-strip"
|
import { stripThinkingPartsAsync } from "./storage/thinking-strip"
|
||||||
import { THINKING_TYPES } from "./constants"
|
import { THINKING_TYPES } from "./constants"
|
||||||
|
import { log } from "../../shared/logger"
|
||||||
|
|
||||||
type Client = ReturnType<typeof createOpencodeClient>
|
type Client = ReturnType<typeof createOpencodeClient>
|
||||||
|
|
||||||
@ -35,31 +36,39 @@ async function recoverThinkingDisabledViolationFromSDK(
|
|||||||
client: Client,
|
client: Client,
|
||||||
sessionID: string
|
sessionID: string
|
||||||
): Promise<boolean> {
|
): Promise<boolean> {
|
||||||
const response = await client.session.messages({ path: { id: sessionID } })
|
try {
|
||||||
const messages = (response.data ?? []) as MessageData[]
|
const response = await client.session.messages({ path: { id: sessionID } })
|
||||||
|
const messages = (response.data ?? []) as MessageData[]
|
||||||
|
|
||||||
const messageIDsWithThinking: string[] = []
|
const messageIDsWithThinking: string[] = []
|
||||||
for (const msg of messages) {
|
for (const msg of messages) {
|
||||||
if (msg.info?.role !== "assistant") continue
|
if (msg.info?.role !== "assistant") continue
|
||||||
if (!msg.info?.id) continue
|
if (!msg.info?.id) continue
|
||||||
if (!msg.parts) continue
|
if (!msg.parts) continue
|
||||||
|
|
||||||
const hasThinking = msg.parts.some((part) => THINKING_TYPES.has(part.type))
|
const hasThinking = msg.parts.some((part) => THINKING_TYPES.has(part.type))
|
||||||
if (hasThinking) {
|
if (hasThinking) {
|
||||||
messageIDsWithThinking.push(msg.info.id)
|
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
|
return false
|
||||||
}
|
}
|
||||||
|
|
||||||
let anySuccess = false
|
|
||||||
for (const messageID of messageIDsWithThinking) {
|
|
||||||
if (await stripThinkingPartsAsync(client, sessionID, messageID)) {
|
|
||||||
anySuccess = true
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return anySuccess
|
|
||||||
}
|
}
|
||||||
|
|||||||
@ -49,7 +49,7 @@ export function prependThinkingPart(sessionID: string, messageID: string): boole
|
|||||||
|
|
||||||
const previousThinking = findLastThinkingContent(sessionID, messageID)
|
const previousThinking = findLastThinkingContent(sessionID, messageID)
|
||||||
|
|
||||||
const partId = "prt_0000000000_thinking"
|
const partId = `prt_0000000000_${messageID}_thinking`
|
||||||
const part = {
|
const part = {
|
||||||
id: partId,
|
id: partId,
|
||||||
sessionID,
|
sessionID,
|
||||||
@ -104,7 +104,7 @@ export async function prependThinkingPartAsync(
|
|||||||
): Promise<boolean> {
|
): Promise<boolean> {
|
||||||
const previousThinking = await findLastThinkingContentFromSDK(client, sessionID, messageID)
|
const previousThinking = await findLastThinkingContentFromSDK(client, sessionID, messageID)
|
||||||
|
|
||||||
const partId = "prt_0000000000_thinking"
|
const partId = `prt_0000000000_${messageID}_thinking`
|
||||||
const part: Record<string, unknown> = {
|
const part: Record<string, unknown> = {
|
||||||
id: partId,
|
id: partId,
|
||||||
sessionID,
|
sessionID,
|
||||||
|
|||||||
@ -87,14 +87,15 @@ describe("patchPart", () => {
|
|||||||
expect(result).toBe(true)
|
expect(result).toBe(true)
|
||||||
expect(mockFetch).toHaveBeenCalledWith(
|
expect(mockFetch).toHaveBeenCalledWith(
|
||||||
"https://api.example.com/session/ses123/message/msg456/part/part789",
|
"https://api.example.com/session/ses123/message/msg456/part/part789",
|
||||||
{
|
expect.objectContaining({
|
||||||
method: "PATCH",
|
method: "PATCH",
|
||||||
headers: {
|
headers: {
|
||||||
"Content-Type": "application/json",
|
"Content-Type": "application/json",
|
||||||
"Authorization": "Basic b3BlbmNvZGU6dGVzdHBhc3N3b3Jk",
|
"Authorization": "Basic b3BlbmNvZGU6dGVzdHBhc3N3b3Jk",
|
||||||
},
|
},
|
||||||
body: JSON.stringify(body),
|
body: JSON.stringify(body),
|
||||||
}
|
signal: expect.any(AbortSignal),
|
||||||
|
})
|
||||||
)
|
)
|
||||||
})
|
})
|
||||||
|
|
||||||
@ -145,12 +146,13 @@ describe("deletePart", () => {
|
|||||||
expect(result).toBe(true)
|
expect(result).toBe(true)
|
||||||
expect(mockFetch).toHaveBeenCalledWith(
|
expect(mockFetch).toHaveBeenCalledWith(
|
||||||
"https://api.example.com/session/ses123/message/msg456/part/part789",
|
"https://api.example.com/session/ses123/message/msg456/part/part789",
|
||||||
{
|
expect.objectContaining({
|
||||||
method: "DELETE",
|
method: "DELETE",
|
||||||
headers: {
|
headers: {
|
||||||
"Authorization": "Basic b3BlbmNvZGU6dGVzdHBhc3N3b3Jk",
|
"Authorization": "Basic b3BlbmNvZGU6dGVzdHBhc3N3b3Jk",
|
||||||
},
|
},
|
||||||
}
|
signal: expect.any(AbortSignal),
|
||||||
|
})
|
||||||
)
|
)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
@ -81,6 +81,7 @@ export async function patchPart(
|
|||||||
"Authorization": auth,
|
"Authorization": auth,
|
||||||
},
|
},
|
||||||
body: JSON.stringify(body),
|
body: JSON.stringify(body),
|
||||||
|
signal: AbortSignal.timeout(30_000),
|
||||||
})
|
})
|
||||||
|
|
||||||
if (!response.ok) {
|
if (!response.ok) {
|
||||||
@ -122,6 +123,7 @@ export async function deletePart(
|
|||||||
headers: {
|
headers: {
|
||||||
"Authorization": auth,
|
"Authorization": auth,
|
||||||
},
|
},
|
||||||
|
signal: AbortSignal.timeout(30_000),
|
||||||
})
|
})
|
||||||
|
|
||||||
if (!response.ok) {
|
if (!response.ok) {
|
||||||
|
|||||||
@ -418,12 +418,16 @@ describe("syncAllTasksToTodos", () => {
|
|||||||
},
|
},
|
||||||
];
|
];
|
||||||
mockCtx.client.session.todo.mockResolvedValue(currentTodos);
|
mockCtx.client.session.todo.mockResolvedValue(currentTodos);
|
||||||
|
let writtenTodos: TodoInfo[] = [];
|
||||||
|
const writer = async (input: { sessionID: string; todos: TodoInfo[] }) => {
|
||||||
|
writtenTodos = input.todos;
|
||||||
|
};
|
||||||
|
|
||||||
// when
|
// when
|
||||||
await syncAllTasksToTodos(mockCtx, tasks, "session-1");
|
await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer);
|
||||||
|
|
||||||
// then
|
// 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 () => {
|
it("preserves existing todos not in task list", async () => {
|
||||||
@ -451,12 +455,17 @@ describe("syncAllTasksToTodos", () => {
|
|||||||
},
|
},
|
||||||
];
|
];
|
||||||
mockCtx.client.session.todo.mockResolvedValue(currentTodos);
|
mockCtx.client.session.todo.mockResolvedValue(currentTodos);
|
||||||
|
let writtenTodos: TodoInfo[] = [];
|
||||||
|
const writer = async (input: { sessionID: string; todos: TodoInfo[] }) => {
|
||||||
|
writtenTodos = input.todos;
|
||||||
|
};
|
||||||
|
|
||||||
// when
|
// when
|
||||||
await syncAllTasksToTodos(mockCtx, tasks, "session-1");
|
await syncAllTasksToTodos(mockCtx, tasks, "session-1", writer);
|
||||||
|
|
||||||
// then
|
// 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 () => {
|
it("handles empty task list", async () => {
|
||||||
@ -471,6 +480,67 @@ describe("syncAllTasksToTodos", () => {
|
|||||||
expect(mockCtx.client.session.todo).toHaveBeenCalled();
|
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 () => {
|
it("preserves todos without id field", async () => {
|
||||||
// given
|
// given
|
||||||
const tasks: Task[] = [
|
const tasks: Task[] = [
|
||||||
|
|||||||
@ -139,6 +139,7 @@ export async function syncAllTasksToTodos(
|
|||||||
ctx: PluginInput,
|
ctx: PluginInput,
|
||||||
tasks: Task[],
|
tasks: Task[],
|
||||||
sessionID?: string,
|
sessionID?: string,
|
||||||
|
writer?: TodoWriter,
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
try {
|
try {
|
||||||
let currentTodos: TodoInfo[] = [];
|
let currentTodos: TodoInfo[] = [];
|
||||||
@ -156,8 +157,10 @@ export async function syncAllTasksToTodos(
|
|||||||
|
|
||||||
const newTodos: TodoInfo[] = [];
|
const newTodos: TodoInfo[] = [];
|
||||||
const tasksToRemove = new Set<string>();
|
const tasksToRemove = new Set<string>();
|
||||||
|
const allTaskSubjects = new Set<string>();
|
||||||
|
|
||||||
for (const task of tasks) {
|
for (const task of tasks) {
|
||||||
|
allTaskSubjects.add(task.subject);
|
||||||
const todo = syncTaskToTodo(task);
|
const todo = syncTaskToTodo(task);
|
||||||
if (todo === null) {
|
if (todo === null) {
|
||||||
tasksToRemove.add(task.id);
|
tasksToRemove.add(task.id);
|
||||||
@ -176,13 +179,19 @@ export async function syncAllTasksToTodos(
|
|||||||
const isInNewTodos = newTodos.some((newTodo) => todosMatch(existing, newTodo));
|
const isInNewTodos = newTodos.some((newTodo) => todosMatch(existing, newTodo));
|
||||||
const isRemovedById = existing.id ? tasksToRemove.has(existing.id) : false;
|
const isRemovedById = existing.id ? tasksToRemove.has(existing.id) : false;
|
||||||
const isRemovedByContent = !existing.id && removedTaskSubjects.has(existing.content);
|
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(existing);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
finalTodos.push(...newTodos);
|
finalTodos.push(...newTodos);
|
||||||
|
|
||||||
|
const resolvedWriter = writer ?? (await resolveTodoWriter());
|
||||||
|
if (resolvedWriter && sessionID) {
|
||||||
|
await resolvedWriter({ sessionID, todos: finalTodos });
|
||||||
|
}
|
||||||
|
|
||||||
log("[todo-sync] Synced todos", {
|
log("[todo-sync] Synced todos", {
|
||||||
count: finalTodos.length,
|
count: finalTodos.length,
|
||||||
sessionID,
|
sessionID,
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user