diff --git a/.debugging b/.debugging index 6051974f4..01268e152 100644 --- a/.debugging +++ b/.debugging @@ -306,6 +306,25 @@ Expected: only interrupted `running` / `pending` call IDs are recovered, complet - Result: 7021 pass, 1 skip, 0 fail across 725 files. - Cleanup: validation worktree removed. +## Cubic Follow-up + +- Latest Cubic review on PR #4106 initially reported: `1 issue found`, confidence `3/5`. +- Cubic found a valid edge case in `src/hooks/session-recovery/recover-tool-result-missing.ts`: `callID ?? id` discarded recoverable `tool_use` parts when `callID` existed but was malformed and `id` was valid. +- Red tests added: + - `recoverToolResultMissing > falls back to a valid id when callID is malformed` + - The interrupted idle recovery test now uses malformed `callID` plus valid `tool_use.id`. +- Red output before the fix: + - `recoverToolResultMissing` returned `false` instead of `true`. + - `handleInterruptedToolResultsOnIdle` returned `false` instead of `true`. +- Fix: choose a valid `callID` first, then fall back to a valid `id`; apply the same validity check in the idle precheck. +- Post-fix validation: + - `bun test src/hooks/session-recovery/recover-tool-result-missing.test.ts --test-name-pattern "malformed"`: pass. + - `bun test src/hooks/session-recovery/hook.test.ts --test-name-pattern "interrupted idle recovery"`: pass. + - `bun test src/hooks/session-recovery/recover-tool-result-missing.test.ts src/hooks/session-recovery/hook.test.ts src/features/background-agent/task-completion-cleanup.test.ts src/plugin/event.test.ts --bail`: 53 pass, 0 fail. + - `bun --install=fallback /Users/yeongyu/.config/opencode/skills/typescript-programmer/scripts/check-no-excuse-rules.ts src/hooks/session-recovery/hook.ts src/hooks/session-recovery/hook.test.ts src/hooks/session-recovery/recover-tool-result-missing.ts src/hooks/session-recovery/recover-tool-result-missing.test.ts`: pass. + - `bun run typecheck`: pass. + - `bun run build`: pass. + ## Final Status Before PR - Product behavior change: only malformed idle events with unfinished latest assistant messages and `pending` / `running` tool parts get synthetic interrupted tool results. diff --git a/src/hooks/session-recovery/hook.test.ts b/src/hooks/session-recovery/hook.test.ts index aa5fc9807..f1a2c8a5a 100644 --- a/src/hooks/session-recovery/hook.test.ts +++ b/src/hooks/session-recovery/hook.test.ts @@ -140,6 +140,7 @@ describe("session-recovery hook interrupted idle recovery", () => { { type: "tool_use", id: "toolu_running", + callID: "prt_not_a_tool_use_id", name: "bash", input: {}, state: { status: "running" }, @@ -147,6 +148,7 @@ describe("session-recovery hook interrupted idle recovery", () => { { type: "tool_use", id: "toolu_pending", + callID: "also_not_a_tool_use_id", name: "task", input: {}, state: { status: "pending" }, diff --git a/src/hooks/session-recovery/hook.ts b/src/hooks/session-recovery/hook.ts index e64ea58ec..ca8f3f1d7 100644 --- a/src/hooks/session-recovery/hook.ts +++ b/src/hooks/session-recovery/hook.ts @@ -67,12 +67,21 @@ export function createSessionRecoveryHook(ctx: PluginInput, options?: SessionRec return typeof completed === "string" && completed.length > 0 } + const partHasValidToolUseID = (part: NonNullable[number]): boolean => { + const callID = part.callID + if (typeof callID === "string" && /^(toolu_|call_)/.test(callID)) { + return true + } + + const id = part.id + return typeof id === "string" && /^(toolu_|call_)/.test(id) + } + const messageHasInterruptedToolResults = (message: MessageData): boolean => { return message.parts?.some((part) => (part.type === "tool" || part.type === "tool_use") && (part.state?.status === "pending" || part.state?.status === "running") - && typeof (part.callID ?? part.id) === "string" - && /^(toolu_|call_)/.test(part.callID ?? part.id ?? "") + && partHasValidToolUseID(part) ) === true } diff --git a/src/hooks/session-recovery/recover-tool-result-missing.test.ts b/src/hooks/session-recovery/recover-tool-result-missing.test.ts index 18a7755bf..442b9aad9 100644 --- a/src/hooks/session-recovery/recover-tool-result-missing.test.ts +++ b/src/hooks/session-recovery/recover-tool-result-missing.test.ts @@ -92,6 +92,35 @@ describe("recoverToolResultMissing", () => { }) }) + it("falls back to a valid id when callID is malformed", async () => { + //#given + const { client, promptAsync } = createMockClient() + const failedAssistantWithMalformedCallID: MessageData = { + info: { id: "msg_failed", role: "assistant" }, + parts: [{ + type: "tool_use", + id: "toolu_recovered_from_id", + callID: "prt_not_a_tool_use_id", + state: { status: "running" }, + }], + } + + //#when + const result = await recoverToolResultMissing(client, "ses_1", failedAssistantWithMalformedCallID, undefined, { + recoverStatuses: new Set(["pending", "running"]), + }) + + //#then + expect(result).toBe(true) + expect(promptAsync).toHaveBeenCalledTimes(1) + const call = promptAsync.mock.calls[0]?.[0] as { + body: { + parts: Array<{ toolUseId: string }> + } + } + expect(call.body.parts.map((part) => part.toolUseId)).toEqual(["toolu_recovered_from_id"]) + }) + it("sends only interrupted sqlite tool results when recoverStatuses is provided", async () => { //#given sqliteBackend = true diff --git a/src/hooks/session-recovery/recover-tool-result-missing.ts b/src/hooks/session-recovery/recover-tool-result-missing.ts index 40e923b83..f7ae2af06 100644 --- a/src/hooks/session-recovery/recover-tool-result-missing.ts +++ b/src/hooks/session-recovery/recover-tool-result-missing.ts @@ -51,10 +51,20 @@ function isValidToolUseID(id: string | undefined): id is string { return typeof id === "string" && /^(toolu_|call_)/.test(id) } +function selectValidToolUseID(part: { id?: string; callID?: string }): string | undefined { + if (isValidToolUseID(part.callID)) { + return part.callID + } + if (isValidToolUseID(part.id)) { + return part.id + } + return undefined +} + function normalizeMessagePart(part: { type: string; id?: string; callID?: string; state?: { status?: unknown } }): MessagePart | null { if (part.type === "tool" || part.type === "tool_use") { - const toolUseID = part.callID ?? part.id - if (!isValidToolUseID(toolUseID)) { + const toolUseID = selectValidToolUseID(part) + if (!toolUseID) { return null }