diff --git a/src/hooks/session-recovery/detect-error-type.test.ts b/src/hooks/session-recovery/detect-error-type.test.ts index f3fdccfa8..7652d63d5 100644 --- a/src/hooks/session-recovery/detect-error-type.test.ts +++ b/src/hooks/session-recovery/detect-error-type.test.ts @@ -61,6 +61,17 @@ describe("detectErrorType", () => { expect(result).toBe("thinking_block_modified") }) + it("#given a modified-thinking error also says expected and found #when detecting #then returns thinking_block_modified", () => { + const error = { + message: + "messages.3.content.3: Expected `thinking` or `redacted_thinking`, but found `tool_use`. `thinking` or `redacted_thinking` blocks in the latest assistant message cannot be modified.", + } + + const result = detectErrorType(error) + + expect(result).toBe("thinking_block_modified") + }) + it("#given an unrecognized error #when detecting #then returns null", () => { //#given const error = { message: "some random error" } diff --git a/src/hooks/session-recovery/detect-error-type.ts b/src/hooks/session-recovery/detect-error-type.ts index ef849a6c7..456f4f923 100644 --- a/src/hooks/session-recovery/detect-error-type.ts +++ b/src/hooks/session-recovery/detect-error-type.ts @@ -66,6 +66,10 @@ export function detectErrorType(error: unknown): RecoveryErrorType { return "assistant_prefill_unsupported" } + if (message.includes("thinking") && message.includes("cannot be modified")) { + return "thinking_block_modified" + } + if ( message.includes("thinking") && (message.includes("first block") || @@ -78,11 +82,6 @@ export function detectErrorType(error: unknown): RecoveryErrorType { return "thinking_block_order" } - // Thinking block signature corruption (Bedrock compaction) - if (message.includes("thinking") && message.includes("cannot be modified")) { - return "thinking_block_modified" - } - if (message.includes("thinking is disabled") && message.includes("cannot contain")) { return "thinking_disabled_violation" } diff --git a/src/hooks/session-recovery/hook.ts b/src/hooks/session-recovery/hook.ts index d9521f4be..4961cea19 100644 --- a/src/hooks/session-recovery/hook.ts +++ b/src/hooks/session-recovery/hook.ts @@ -194,6 +194,30 @@ export function createSessionRecoveryHook(ctx: PluginInput, options?: SessionRec let shouldKeepProcessingError = false try { + if (errorType === "thinking_block_modified") { + shouldKeepProcessingError = true + log("[session-recovery] Refusing to mutate latest assistant thinking blocks", { + sessionID, + assistantMsgID, + }) + await ctx.client.tui + .showToast({ + body: { + title: "Thinking Block Recovery", + message: "Latest assistant thinking blocks cannot be safely recovered; leaving history unchanged.", + variant: "warning", + duration: 3000, + }, + }) + .catch((error: unknown) => { + log("[session-recovery] Failed to show thinking block modified toast", { + sessionID, + error, + }) + }) + return false + } + if (onAbortCallback) { onAbortCallback(sessionID) } @@ -224,7 +248,7 @@ export function createSessionRecoveryHook(ctx: PluginInput, options?: SessionRec unavailable_tool: "Recovering from unavailable tool call...", thinking_block_order: "Fixing message structure...", thinking_disabled_violation: "Stripping thinking blocks...", - thinking_block_modified: "Stripping corrupted thinking blocks...", + thinking_block_modified: "Leaving latest thinking blocks unchanged...", "assistant_prefill_unsupported": "Prefill not supported; continuing without recovery.", } @@ -261,13 +285,6 @@ export function createSessionRecoveryHook(ctx: PluginInput, options?: SessionRec const resumeConfig = extractResumeConfig(lastUser, sessionID) await resumeSession(ctx.client, resumeConfig) } - } else if (errorType === "thinking_block_modified") { - success = await recoverThinkingDisabledViolation(ctx.client, sessionID, failedMsg) - if (success && experimental?.auto_resume) { - const lastUser = findLastUserMessage(msgs ?? []) - const resumeConfig = extractResumeConfig(lastUser, sessionID) - await resumeSession(ctx.client, resumeConfig) - } } else if (errorType === "assistant_prefill_unsupported") { shouldKeepProcessingError = true success = false diff --git a/src/hooks/session-recovery/storage/latest-assistant-message.ts b/src/hooks/session-recovery/storage/latest-assistant-message.ts new file mode 100644 index 000000000..90ff6cc86 --- /dev/null +++ b/src/hooks/session-recovery/storage/latest-assistant-message.ts @@ -0,0 +1,42 @@ +import type { PluginInput } from "@opencode-ai/plugin" +import type { MessageData } from "../types" +import { log, normalizeSDKResponse } from "../../../shared" +import { readMessages } from "./messages-reader" + +type OpencodeClient = PluginInput["client"] + +export function isLatestAssistantMessage(sessionID: string, messageID: string): boolean { + const messages = readMessages(sessionID) + for (let index = messages.length - 1; index >= 0; index -= 1) { + const message = messages[index] + if (message?.role === "assistant") { + return message.id === messageID + } + } + return false +} + +export async function isLatestAssistantMessageFromSDK( + client: OpencodeClient, + sessionID: string, + messageID: string +): Promise { + try { + const response = await client.session.messages({ path: { id: sessionID } }) + const messages = normalizeSDKResponse(response, [] as MessageData[], { preferResponseOnMissingData: true }) + for (let index = messages.length - 1; index >= 0; index -= 1) { + const message = messages[index] + if (message?.info?.role === "assistant") { + return message.info.id === messageID + } + } + } catch (error) { + log("[session-recovery] latest assistant lookup failed", { + sessionID, + messageID, + error: String(error), + }) + } + + return false +} diff --git a/src/hooks/session-recovery/storage/thinking-prepend.ts b/src/hooks/session-recovery/storage/thinking-prepend.ts index 9ccb7131c..41593951a 100644 --- a/src/hooks/session-recovery/storage/thinking-prepend.ts +++ b/src/hooks/session-recovery/storage/thinking-prepend.ts @@ -5,8 +5,8 @@ import { PART_STORAGE, THINKING_TYPES } from "../constants" import type { MessageData, StoredPart } from "../types" import { readMessages } from "./messages-reader" import { readParts } from "./parts-reader" -import { log, isSqliteBackend, patchPart } from "../../../shared" -import { normalizeSDKResponse } from "../../../shared" +import { log, isSqliteBackend, normalizeSDKResponse, patchPart } from "../../../shared" +import { isLatestAssistantMessage, isLatestAssistantMessageFromSDK } from "./latest-assistant-message" type OpencodeClient = PluginInput["client"] type StoredSignedThinkingPart = StoredPart & { @@ -28,6 +28,8 @@ type ThinkingPrependDeps = { findLastThinkingPartFromSDK: typeof findLastThinkingPartFromSDK readTargetPartIDs: typeof readTargetPartIDs readTargetPartIDsFromSDK: typeof readTargetPartIDsFromSDK + isLatestAssistantMessage?: typeof isLatestAssistantMessage + isLatestAssistantMessageFromSDK?: typeof isLatestAssistantMessageFromSDK } const thinkingPrependDeps: ThinkingPrependDeps = { @@ -38,6 +40,8 @@ const thinkingPrependDeps: ThinkingPrependDeps = { findLastThinkingPartFromSDK, readTargetPartIDs, readTargetPartIDsFromSDK, + isLatestAssistantMessage, + isLatestAssistantMessageFromSDK, } function readTargetPartIDs(messageID: string): string[] { @@ -137,6 +141,14 @@ export function prependThinkingPart( return false } + if (deps.isLatestAssistantMessage?.(sessionID, messageID) === true) { + deps.log("[session-recovery] Refusing to prepend thinking into latest assistant message", { + sessionID, + messageID, + }) + return false + } + const previousThinkingPart = deps.findLastThinkingPart(sessionID, messageID) if (!previousThinkingPart) { return false @@ -198,6 +210,17 @@ export async function prependThinkingPartAsync( messageID: string, deps: ThinkingPrependDeps = thinkingPrependDeps ): Promise { + const isLatestAssistant = deps.isLatestAssistantMessageFromSDK + ? await deps.isLatestAssistantMessageFromSDK(client, sessionID, messageID) + : false + if (isLatestAssistant) { + deps.log("[session-recovery] Refusing to patch thinking into latest assistant message", { + sessionID, + messageID, + }) + return false + } + const previousThinkingPart = await deps.findLastThinkingPartFromSDK(client, sessionID, messageID) if (!previousThinkingPart) { return false diff --git a/src/hooks/session-recovery/thinking-block-modified-recovery.test.ts b/src/hooks/session-recovery/thinking-block-modified-recovery.test.ts new file mode 100644 index 000000000..0a7665aca --- /dev/null +++ b/src/hooks/session-recovery/thinking-block-modified-recovery.test.ts @@ -0,0 +1,100 @@ +/// +import { afterEach, describe, expect, test } from "bun:test" +import { createSessionRecoveryHook } from "./hook" +import { releaseAllPromptAsyncReservationsForTesting } from "../../shared/prompt-async-gate" + +type RecoverableInfo = Parameters["handleSessionRecovery"]>[0] + +describe("session-recovery immutable thinking block errors", () => { + afterEach(() => { + releaseAllPromptAsyncReservationsForTesting() + }) + + test("#given Anthropic rejects modified latest assistant thinking blocks #when recovery handles the error #then it leaves the session history untouched", async () => { + //#given + const counts = { + abort: 0, + abortCallback: 0, + messages: 0, + promptAsync: 0, + toast: 0, + } + const info: RecoverableInfo = { + id: "msg_failed_modified_thinking", + role: "assistant", + sessionID: "ses_modified_thinking", + error: { + message: + "messages.3.content.3: `thinking` or `redacted_thinking` blocks in the latest assistant message cannot be modified. These blocks must remain as they were in the original response.", + }, + } + const ctx = { + client: { + session: { + abort: async () => { + counts.abort++ + return {} + }, + messages: async () => { + counts.messages++ + return { + data: [ + { + info: { + id: info.id, + role: "assistant", + error: info.error, + }, + parts: [ + { + id: "prt_reasoning", + type: "reasoning", + text: "signed reasoning text", + metadata: { anthropic: { signature: "sig_reasoning" } }, + }, + { + id: "prt_redacted", + type: "redacted_thinking", + signature: "sig_redacted", + }, + { + id: "prt_text", + type: "text", + text: "assistant text", + }, + ], + }, + ], + } + }, + promptAsync: async () => { + counts.promptAsync++ + return {} + }, + }, + tui: { + showToast: async () => { + counts.toast++ + return {} + }, + }, + }, + directory: "/tmp/session-recovery-modified-thinking-test", + } + const hook = createSessionRecoveryHook(ctx as never) + hook.setOnAbortCallback(() => { + counts.abortCallback++ + }) + + //#when + const result = await hook.handleSessionRecovery(info) + + //#then + expect(result).toBe(false) + expect(counts.toast).toBe(1) + expect(counts.abortCallback).toBe(0) + expect(counts.abort).toBe(0) + expect(counts.messages).toBe(0) + expect(counts.promptAsync).toBe(0) + }) +}) diff --git a/src/hooks/session-recovery/thinking-prepend-latest.test.ts b/src/hooks/session-recovery/thinking-prepend-latest.test.ts new file mode 100644 index 000000000..23bf11fc9 --- /dev/null +++ b/src/hooks/session-recovery/thinking-prepend-latest.test.ts @@ -0,0 +1,110 @@ +/// +import { existsSync, rmSync } from "node:fs" +import { randomUUID } from "node:crypto" +import { tmpdir } from "node:os" +import { join } from "node:path" +import { afterAll, describe, expect, it, mock } from "bun:test" + +const TEST_STORAGE_ROOT = join(tmpdir(), `session-recovery-latest-thinking-prepend-${randomUUID()}`) +const TEST_PART_STORAGE = join(TEST_STORAGE_ROOT, "part") + +mock.module("../../shared", () => ({ + OPENCODE_STORAGE: TEST_STORAGE_ROOT, + MESSAGE_STORAGE: join(TEST_STORAGE_ROOT, "message"), + PART_STORAGE: TEST_PART_STORAGE, + log: () => {}, + isSqliteBackend: () => false, + patchPart: async () => true, + normalizeSDKResponse: (response: { data?: TData }, fallback: TData) => response.data ?? fallback, +})) + +afterAll(() => { mock.restore() }) + +const { prependThinkingPart, prependThinkingPartAsync } = await import("./storage/thinking-prepend") + +type StoredPartRecord = { + id: string + sessionID: string + messageID: string + type: string + signature?: string + thinking?: string +} + +function cleanupParts(messageID: string): void { + rmSync(join(TEST_PART_STORAGE, messageID), { recursive: true, force: true }) +} + +describe("thinking-prepend latest assistant preservation", () => { + it("#given file-backed order recovery targets the latest assistant #when prepending thinking #then it refuses to write copied thinking", () => { + const sessionID = "ses_latest_file_backed_prepend" + const targetMessageID = "msg_latest_file_backed" + const previousThinkingPart = { + id: "prt_previous_thinking", + sessionID, + messageID: "msg_previous_assistant", + type: "thinking", + thinking: "prior signed thinking", + signature: "sig_previous", + } as const satisfies StoredPartRecord + const deps = { + isSqliteBackend: () => false, + patchPart: async () => true, + log: mock(() => {}), + findLastThinkingPart: () => previousThinkingPart, + findLastThinkingPartFromSDK: async () => null, + readTargetPartIDs: () => ["prt_target_text"], + readTargetPartIDsFromSDK: async () => [], + isLatestAssistantMessage: () => true, + isLatestAssistantMessageFromSDK: async () => false, + } + + const result = prependThinkingPart(sessionID, targetMessageID, deps) + + expect(result).toBe(false) + expect(existsSync(join(TEST_PART_STORAGE, targetMessageID))).toBe(false) + cleanupParts(targetMessageID) + }) + + it("#given sdk order recovery targets the latest assistant #when prepending thinking #then it refuses to patch copied thinking", async () => { + const sessionID = "ses_latest_sdk_prepend" + const targetMessageID = "msg_latest_sdk" + const patchPartMock = mock(async () => true) + const previousThinkingPart = { + id: "prt_previous_sdk_thinking", + type: "thinking", + thinking: "prior signed thinking", + signature: "sig_previous_sdk", + } as const + const client = { + session: { + messages: async () => ({ data: [] }), + }, + } + const deps = { + isSqliteBackend: () => false, + patchPart: patchPartMock, + log: mock(() => {}), + findLastThinkingPart: () => null, + findLastThinkingPartFromSDK: async () => previousThinkingPart, + readTargetPartIDs: () => [], + readTargetPartIDsFromSDK: async () => ["prt_target_text"], + isLatestAssistantMessage: () => false, + isLatestAssistantMessageFromSDK: async () => true, + } + const prependThinkingPartAsyncUntyped = Reflect.get( + { prependThinkingPartAsync }, + "prependThinkingPartAsync", + ) + + const result = await Reflect.apply(prependThinkingPartAsyncUntyped, undefined, [ + client, + sessionID, + targetMessageID, + deps, + ]) + + expect(result).toBe(false) + expect(patchPartMock).toHaveBeenCalledTimes(0) + }) +}) diff --git a/src/hooks/thinking-block-validator/hook.test.ts b/src/hooks/thinking-block-validator/hook.test.ts index 0601cbcfc..d2f3430d0 100644 --- a/src/hooks/thinking-block-validator/hook.test.ts +++ b/src/hooks/thinking-block-validator/hook.test.ts @@ -33,7 +33,7 @@ async function runTransform(messages: TestMessage[]): Promise { } describe("createThinkingBlockValidatorHook", () => { - it("injects signed thinking history verbatim", async () => { + it("does not copy signed thinking history into a later assistant message", async () => { //#given const signedThinkingPart: TestPart = { type: "thinking", @@ -55,10 +55,10 @@ describe("createThinkingBlockValidatorHook", () => { await runTransform(messages) //#then - expect(messages[1]?.parts[0]).toBe(signedThinkingPart) + expect(messages[1]?.parts).toEqual([{ type: "text", text: "continue" }]) }) - it("injects signed redacted_thinking history verbatim", async () => { + it("does not copy signed redacted_thinking history into a later assistant message", async () => { //#given const signedRedactedThinkingPart: TestPart = { type: "redacted_thinking", @@ -79,7 +79,7 @@ describe("createThinkingBlockValidatorHook", () => { await runTransform(messages) //#then - expect(messages[1]?.parts[0]).toBe(signedRedactedThinkingPart) + expect(messages[1]?.parts).toEqual([{ type: "tool_use" }]) }) it("skips hook when history contains reasoning only", async () => { diff --git a/src/hooks/thinking-block-validator/hook.ts b/src/hooks/thinking-block-validator/hook.ts index af7a782d2..f187d65e5 100644 --- a/src/hooks/thinking-block-validator/hook.ts +++ b/src/hooks/thinking-block-validator/hook.ts @@ -1,19 +1,3 @@ -/** - * Proactive Thinking Block Validator Hook - * - * Prevents "Expected thinking/redacted_thinking but found tool_use" errors - * by validating and fixing message structure BEFORE sending to Anthropic API. - * - * This hook runs on the "experimental.chat.messages.transform" hook point, - * which is called before messages are converted to ModelMessage format and - * sent to the API. - * - * Key differences from session-recovery hook: - * - PROACTIVE (prevents error) vs REACTIVE (fixes after error) - * - Runs BEFORE API call vs AFTER API error - * - User never sees the error vs User sees error then recovery - */ - import type { Message, Part } from "@opencode-ai/sdk" interface MessageWithParts { @@ -28,155 +12,10 @@ type MessagesTransformHook = { ) => Promise } -type SignedThinkingPart = Part & { - type: "thinking" | "redacted_thinking" - thinking?: string - signature: string - synthetic?: boolean -} - -function isSignedThinkingPart(part: Part): part is SignedThinkingPart { - const type = part.type as string - if (type !== "thinking" && type !== "redacted_thinking") { - return false - } - - const signature = (part as { signature?: unknown }).signature - const synthetic = (part as { synthetic?: unknown }).synthetic - return typeof signature === "string" && signature.length > 0 && synthetic !== true -} - -/** - * Check if there are any Anthropic-signed thinking blocks in the message history. - * - * Only returns true for real `type: "thinking"` blocks with a valid `signature`. - * GPT reasoning blocks (`type: "reasoning"`) are intentionally excluded - they - * have no Anthropic signature and must never be forwarded to the Anthropic API. - * - * Model-name checks are unreliable (miss GPT+thinking, custom model IDs, etc.) - * so we inspect the messages themselves. - */ -function hasSignedThinkingBlocksInHistory(messages: MessageWithParts[]): boolean { - return messages.some( - m => - m.info.role === "assistant" && - m.parts?.some((p: Part) => isSignedThinkingPart(p)), - ) -} - -/** - * Check if a message has any content parts (tool_use, text, or other non-thinking content) - */ -function hasContentParts(parts: Part[]): boolean { - if (!parts || parts.length === 0) return false - - return parts.some((part: Part) => { - const type = part.type as string - // Include tool parts and text parts (anything that's not thinking/reasoning) - return type === "tool" || type === "tool_use" || type === "text" - }) -} - -/** - * Check if a message already carries a thinking/reasoning block anywhere. - */ -function hasThinkingBlock(parts: Part[]): boolean { - if (!parts || parts.length === 0) return false - - return parts.some((part) => { - const type = part.type as string - return type === "thinking" || type === "redacted_thinking" || type === "reasoning" - }) -} - -/** - * Find the most recent Anthropic-signed thinking part from previous assistant messages. - * - * Returns the original Part object (including its `signature` field) so it can - * be reused verbatim in another message. Only `type: "thinking"` blocks with - * both a `signature` and `thinking` field are returned - GPT `type: "reasoning"` - * blocks are excluded because they lack an Anthropic signature and would be - * rejected by the API with "Invalid `signature` in `thinking` block". - * Synthetic parts injected by a previous run of this hook are also skipped. - */ -function findPreviousThinkingPart(messages: MessageWithParts[], currentIndex: number): SignedThinkingPart | null { - // Search backwards from current message - for (let i = currentIndex - 1; i >= 0; i--) { - const msg = messages[i] - if (msg.info.role !== "assistant") continue - if (!msg.parts) continue - - for (const part of msg.parts) { - // Only Anthropic thinking blocks - type must be "thinking", not "reasoning" - if (!isSignedThinkingPart(part)) continue - - return part - } - } - - return null -} - -/** - * Prepend an existing thinking block (with its original signature) to a - * message's parts array. - * - * We reuse the original Part verbatim instead of creating a new one, because - * the Anthropic API validates the `signature` field against the thinking - * content. Any synthetic block we create ourselves would fail that check. - */ -function prependThinkingBlock(message: MessageWithParts, thinkingPart: SignedThinkingPart): void { - if (!message.parts) { - message.parts = [] - } - - message.parts.unshift(thinkingPart) -} - -/** - * Validate and fix assistant messages that have tool_use but no thinking block - */ export function createThinkingBlockValidatorHook(): MessagesTransformHook { return { - "experimental.chat.messages.transform": async (_input, output) => { - const { messages } = output - - if (!messages || messages.length === 0) { - return - } - - // Skip if there are no Anthropic-signed thinking blocks in history. - // This is more reliable than checking model names - works for Claude, - // GPT with thinking variants, or any future model. Crucially, GPT - // reasoning blocks (type="reasoning", no signature) do NOT trigger this - // hook - only real Anthropic thinking blocks do. - if (!hasSignedThinkingBlocksInHistory(messages)) { - return - } - - // Process all assistant messages - for (let i = 0; i < messages.length; i++) { - const msg = messages[i] - - // Only check assistant messages - if (msg.info.role !== "assistant") continue - - // Check if message has content parts but no thinking block yet. - if (hasContentParts(msg.parts) && !hasThinkingBlock(msg.parts)) { - // Find the most recent real thinking part (with valid signature) from - // previous turns. If none exists we cannot safely inject a thinking - // block - a synthetic block without a signature would cause the API - // to reject the request with "Invalid `signature` in `thinking` block". - const previousThinkingPart = findPreviousThinkingPart(messages, i) - - if (previousThinkingPart) { - prependThinkingBlock(msg, previousThinkingPart) - } - // If no real thinking part is available, skip injection entirely. - // The downstream error (if any) is preferable to a guaranteed API - // rejection caused by a signature-less synthetic thinking block. - } - } + "experimental.chat.messages.transform": async () => { + return }, } } diff --git a/src/plugin/messages-transform-thinking-block.test.ts b/src/plugin/messages-transform-thinking-block.test.ts index 81bd43092..13f398848 100644 --- a/src/plugin/messages-transform-thinking-block.test.ts +++ b/src/plugin/messages-transform-thinking-block.test.ts @@ -97,4 +97,53 @@ describe("messages transform thinking block integration", () => { expect(resumedMessage?.parts[1]).toBe(thinkingAfterAnswer) expect(countThinkingParts(resumedMessage?.parts ?? [])).toBe(1) }) + + it("#given prior signed thinking and a latest assistant turn without thinking #when messages transform runs #then it does not copy thinking into the latest turn", async () => { + //#given + const priorThinkingPart: TestPart = { + type: "thinking", + thinking: "prior plan", + signature: "sig-prior", + } + const latestTextPart: TestPart = { type: "text", text: "continue" } + const latestToolPart: TestPart = { + type: "tool_use", + id: "toolu_latest", + name: "bash", + } + const messages = [ + { + info: { id: "msg_user", role: "user", sessionID: "ses_latest_preserve" }, + parts: [{ type: "text", text: "start" }], + }, + { + info: { id: "msg_prior", role: "assistant", sessionID: "ses_latest_preserve" }, + parts: [priorThinkingPart, { type: "tool_use", id: "toolu_prior", name: "bash" }], + }, + { + info: { id: "msg_prior_result", role: "user", sessionID: "ses_latest_preserve" }, + parts: [ + { + type: "tool_result", + toolUseId: "toolu_prior", + tool_use_id: "toolu_prior", + content: [{ type: "text", text: "done" }], + }, + ], + }, + { + info: { id: "msg_latest", role: "assistant", sessionID: "ses_latest_preserve" }, + parts: [latestTextPart, latestToolPart], + }, + ] satisfies TestMessage[] + + //#when + await runMessagesTransform(messages) + + //#then + const latestMessage = messages.find((message) => message.info.id === "msg_latest") + expect(latestMessage?.parts[0]).toBe(latestTextPart) + expect(latestMessage?.parts[1]).toBe(latestToolPart) + expect(countThinkingParts(latestMessage?.parts ?? [])).toBe(0) + }) })