fix(session-recovery): prefer valid tool use ids
This commit is contained in:
+19
@@ -306,6 +306,25 @@ Expected: only interrupted `running` / `pending` call IDs are recovered, complet
|
|||||||
- Result: 7021 pass, 1 skip, 0 fail across 725 files.
|
- Result: 7021 pass, 1 skip, 0 fail across 725 files.
|
||||||
- Cleanup: validation worktree removed.
|
- 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
|
## 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.
|
- Product behavior change: only malformed idle events with unfinished latest assistant messages and `pending` / `running` tool parts get synthetic interrupted tool results.
|
||||||
|
|||||||
@@ -140,6 +140,7 @@ describe("session-recovery hook interrupted idle recovery", () => {
|
|||||||
{
|
{
|
||||||
type: "tool_use",
|
type: "tool_use",
|
||||||
id: "toolu_running",
|
id: "toolu_running",
|
||||||
|
callID: "prt_not_a_tool_use_id",
|
||||||
name: "bash",
|
name: "bash",
|
||||||
input: {},
|
input: {},
|
||||||
state: { status: "running" },
|
state: { status: "running" },
|
||||||
@@ -147,6 +148,7 @@ describe("session-recovery hook interrupted idle recovery", () => {
|
|||||||
{
|
{
|
||||||
type: "tool_use",
|
type: "tool_use",
|
||||||
id: "toolu_pending",
|
id: "toolu_pending",
|
||||||
|
callID: "also_not_a_tool_use_id",
|
||||||
name: "task",
|
name: "task",
|
||||||
input: {},
|
input: {},
|
||||||
state: { status: "pending" },
|
state: { status: "pending" },
|
||||||
|
|||||||
@@ -67,12 +67,21 @@ export function createSessionRecoveryHook(ctx: PluginInput, options?: SessionRec
|
|||||||
return typeof completed === "string" && completed.length > 0
|
return typeof completed === "string" && completed.length > 0
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const partHasValidToolUseID = (part: NonNullable<MessageData["parts"]>[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 => {
|
const messageHasInterruptedToolResults = (message: MessageData): boolean => {
|
||||||
return message.parts?.some((part) =>
|
return message.parts?.some((part) =>
|
||||||
(part.type === "tool" || part.type === "tool_use")
|
(part.type === "tool" || part.type === "tool_use")
|
||||||
&& (part.state?.status === "pending" || part.state?.status === "running")
|
&& (part.state?.status === "pending" || part.state?.status === "running")
|
||||||
&& typeof (part.callID ?? part.id) === "string"
|
&& partHasValidToolUseID(part)
|
||||||
&& /^(toolu_|call_)/.test(part.callID ?? part.id ?? "")
|
|
||||||
) === true
|
) === true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -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 () => {
|
it("sends only interrupted sqlite tool results when recoverStatuses is provided", async () => {
|
||||||
//#given
|
//#given
|
||||||
sqliteBackend = true
|
sqliteBackend = true
|
||||||
|
|||||||
@@ -51,10 +51,20 @@ function isValidToolUseID(id: string | undefined): id is string {
|
|||||||
return typeof id === "string" && /^(toolu_|call_)/.test(id)
|
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 {
|
function normalizeMessagePart(part: { type: string; id?: string; callID?: string; state?: { status?: unknown } }): MessagePart | null {
|
||||||
if (part.type === "tool" || part.type === "tool_use") {
|
if (part.type === "tool" || part.type === "tool_use") {
|
||||||
const toolUseID = part.callID ?? part.id
|
const toolUseID = selectValidToolUseID(part)
|
||||||
if (!isValidToolUseID(toolUseID)) {
|
if (!toolUseID) {
|
||||||
return null
|
return null
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user