fix(session-recovery): handle unavailable_tool (dummy_tool) errors#2005
fix(session-recovery): handle unavailable_tool (dummy_tool) errors#2005code-yeongyu merged 13 commits intodevfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 6 files
Confidence score: 2/5
- High risk of session recovery failures:
recover-unavailable-tool.tscallspromptAsyncwithout itsthiscontext and sendstool_resultas an object, both of which can break OpenCode compatibility. hook.tstriggers duplicate session resumption (doublepromptAsync), anddetect-error-type.tshas a typo preventingNoSuchToolErrorrecognition, with tests indetect-error-type.test.tsmissing the “No such tool” case.- Pay close attention to
src/hooks/session-recovery/recover-unavailable-tool.ts,src/hooks/session-recovery/hook.ts,src/hooks/session-recovery/detect-error-type.ts,src/hooks/session-recovery/detect-error-type.test.ts- OpenCode compatibility and error detection/resumption correctness.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/session-recovery/recover-unavailable-tool.ts">
<violation number="1" location="src/hooks/session-recovery/recover-unavailable-tool.ts:83">
P1: `tool_result` content should be a string (or an array of blocks), not an object. OpenCode's message parser and the underlying API expect a string for `tool_result` errors.</violation>
<violation number="2" location="src/hooks/session-recovery/recover-unavailable-tool.ts:91">
P1: Custom agent: **Opencode Compatibility**
`client.session.promptAsync` is invoked with a lost `this` context. Extracting it via `Reflect.get` and calling it as a bare function (`promptAsync({...})`) drops the `this` binding, which will cause a runtime error inside the SDK method. The established pattern in this module (`recover-tool-result-missing.ts`, `resume.ts`) calls the method directly on the object. Use `client.session.promptAsync(...)` with a `// @ts-expect-error` suppression (matching the existing pattern) instead.</violation>
</file>
<file name="src/hooks/session-recovery/detect-error-type.test.ts">
<violation number="1" location="src/hooks/session-recovery/detect-error-type.test.ts:170">
P1: Custom agent: **Opencode Compatibility**
The `extractUnavailableToolName` tests (and its underlying implementation) do not cover the "No such tool" error pattern, which was explicitly added to `detectErrorType` above. The current regex `/unavailable tool ['"]?([^'".\s]+)['"]?/` will fail to extract the tool name for `NoSuchToolError`. This causes the recovery logic to fail to isolate the specific invalid tool, marking all tools in the request as unavailable instead.</violation>
</file>
<file name="src/hooks/session-recovery/detect-error-type.ts">
<violation number="1" location="src/hooks/session-recovery/detect-error-type.ts:92">
P1: Custom agent: **Opencode Compatibility**
Typo in error detection string prevents `NoSuchToolError` from being recognized.</violation>
</file>
<file name="src/hooks/session-recovery/hook.ts">
<violation number="1" location="src/hooks/session-recovery/hook.ts:113">
P1: Duplicate session resumption logic (double promptAsync calls).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
code-yeongyu
force-pushed
the
fix/1803-session-recovery-unavailable-tool
branch
from
952af87 to
d1d4792
Compare
|
@cubic-dev-ai please re-review |
@code-yeongyu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 9 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: cubic reviewed with no issues found and tests cover new error detection; well contained recovery fixes with added tests ensure no regressions.
|
@cubic-dev-ai please re-review |
@code-yeongyu I have started the AI code review. It will take a few minutes to complete. |
code-yeongyu
force-pushed
the
fix/1803-session-recovery-unavailable-tool
branch
from
5451eef to
e1c75fd
Compare
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/anthropic-context-window-limit-recovery/executor.test.ts">
<violation number="1" location="src/hooks/anthropic-context-window-limit-recovery/executor.test.ts:110">
P0: The removal of `.serial` from these tests and `fakeTimeouts` from `beforeEach`/`afterEach` contradicts the PR's goal of preventing timer races and introduces two critical test bugs:
1. **Global Timer Races**: Localizing `createFakeTimeouts()` inside a `try/finally` block does not make it safe for concurrent execution. Since it mutates `globalThis.setTimeout`, running these tests concurrently (if CI uses `--concurrent` or `concurrentTestGlob`) will cause them to hijack each other's timers, breaking `advanceBy()` calls and causing flaky failures.
2. **Async Call Leaks**: Tests that don't instantiate `fakeTimeouts` but still hit a path that schedules a continuation (like `promptAsync` in `setTimeout`) will now use the real native `setTimeout`. This leaks an unawaited async callback that will execute ~500ms later, potentially during a subsequent test or after suite teardown, causing unexpected mock invocations or unhandled rejections.
To safely avoid these races and leaks, restore `.serial` to all tests (or use `describe.serial` on the block) and keep `fakeTimeouts` in `beforeEach`/`afterEach` so it reliably intercepts all timers for the suite without side-effects on concurrent tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -1,4 +1,4 @@ | |||
| import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test" | |||
| import { beforeEach, describe, expect, mock, spyOn, test } from "bun:test" | |||
There was a problem hiding this comment.
P0: The removal of .serial from these tests and fakeTimeouts from beforeEach/afterEach contradicts the PR's goal of preventing timer races and introduces two critical test bugs:
-
Global Timer Races: Localizing
createFakeTimeouts()inside atry/finallyblock does not make it safe for concurrent execution. Since it mutatesglobalThis.setTimeout, running these tests concurrently (if CI uses--concurrentorconcurrentTestGlob) will cause them to hijack each other's timers, breakingadvanceBy()calls and causing flaky failures. -
Async Call Leaks: Tests that don't instantiate
fakeTimeoutsbut still hit a path that schedules a continuation (likepromptAsyncinsetTimeout) will now use the real nativesetTimeout. This leaks an unawaited async callback that will execute ~500ms later, potentially during a subsequent test or after suite teardown, causing unexpected mock invocations or unhandled rejections.
To safely avoid these races and leaks, restore .serial to all tests (or use describe.serial on the block) and keep fakeTimeouts in beforeEach/afterEach so it reliably intercepts all timers for the suite without side-effects on concurrent tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/anthropic-context-window-limit-recovery/executor.test.ts, line 110:
<comment>The removal of `.serial` from these tests and `fakeTimeouts` from `beforeEach`/`afterEach` contradicts the PR's goal of preventing timer races and introduces two critical test bugs:
1. **Global Timer Races**: Localizing `createFakeTimeouts()` inside a `try/finally` block does not make it safe for concurrent execution. Since it mutates `globalThis.setTimeout`, running these tests concurrently (if CI uses `--concurrent` or `concurrentTestGlob`) will cause them to hijack each other's timers, breaking `advanceBy()` calls and causing flaky failures.
2. **Async Call Leaks**: Tests that don't instantiate `fakeTimeouts` but still hit a path that schedules a continuation (like `promptAsync` in `setTimeout`) will now use the real native `setTimeout`. This leaks an unawaited async callback that will execute ~500ms later, potentially during a subsequent test or after suite teardown, causing unexpected mock invocations or unhandled rejections.
To safely avoid these races and leaks, restore `.serial` to all tests (or use `describe.serial` on the block) and keep `fakeTimeouts` in `beforeEach`/`afterEach` so it reliably intercepts all timers for the suite without side-effects on concurrent tests.</comment>
<file context>
@@ -106,29 +105,24 @@ describe("executeCompact lock management", () => {
- })
-
- test.serial("clears lock on successful summarize completion", async () => {
+ test("clears lock on successful summarize completion", async () => {
// given: Valid session with providerID/modelID
autoCompactState.errorDataBySession.set(sessionID, {
</file context>
code-yeongyu
force-pushed
the
fix/1803-session-recovery-unavailable-tool
branch
from
45d18ff to
0dc1a1c
Compare
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/anthropic-context-window-limit-recovery/executor.test.ts">
<violation number="1" location="src/hooks/anthropic-context-window-limit-recovery/executor.test.ts:305">
P1: Test pollution risk: removing `try...finally` means `fakeTimeouts` won't be restored if an assertion fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/hooks/anthropic-context-window-limit-recovery/executor.test.ts
Outdated
Show resolved
Hide resolved
code-yeongyu
force-pushed
the
fix/1803-session-recovery-unavailable-tool
branch
from
8fea955 to
0dc1a1c
Compare
|
@cubic-dev-ai The previous review was on a now-reverted commit. The current HEAD already has |
@code-yeongyu I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai Fixed: moved |
|
@cubic-dev-ai Please re-review. Fixed the issues identified:
|
@code-yeongyu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 18 files
Confidence score: 4/5
- Primary concern is a test-only regression: in
src/hooks/anthropic-context-window-limit-recovery/executor.test.ts, error-path tests now schedule a real 2000ms timeout, which can slow or flake the suite - Overall impact is limited to test reliability/performance rather than runtime behavior, so this looks safe to merge with minor cleanup
- Pay close attention to
src/hooks/anthropic-context-window-limit-recovery/executor.test.ts- timer scoping causes real timeouts in error-path tests.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/anthropic-context-window-limit-recovery/executor.test.ts">
<violation number="1" location="src/hooks/anthropic-context-window-limit-recovery/executor.test.ts:291">
P2: Scoping fake timers to specific tests causes a timer leak in error-path tests. Tests like 'clears lock when summarize throws exception' now schedule a real 2000ms `setTimeout`, which slows down the test suite and runs background logic after test completion. Consider restoring global fake timers or mocking the timer in all affected tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
src/hooks/anthropic-context-window-limit-recovery/executor.test.ts
Outdated
Show resolved
Hide resolved
Remove duplicated dispatchToHooks declaration that broke TypeScript parsing, and isolate chat-headers tests from marker cache collisions with unique message IDs.
Relax verbose event assertions to target custom-event logs only and run compact lock-management specs serially to avoid global timer races in CI.
Stop patching global timers in every lock-management test. Use scoped fake timers only in continuation tests so lock/notification assertions remain deterministic in CI.
Prevent cross-file mock.module leakage by restoring Bun mocks after recovery-hook test, so executor tests always run against the real module implementation.
code-yeongyu
force-pushed
the
fix/1803-session-recovery-unavailable-tool
branch
from
b12f75d to
db9df55
Compare
There was a problem hiding this comment.
2 issues found across 18 files
Confidence score: 2/5
- High risk:
recover-unavailable-tool.tscurrently returnstool_resultonly for the unavailable tool, which can trigger 400 Bad Request errors when validtool_useblocks are filtered out. - There is duplicated logic in
readPartsFromSDKFallbackthat mirrorsrecover-tool-result-missing.ts, increasing maintenance risk but not as critical as the request failure. - Pay close attention to
src/hooks/session-recovery/recover-unavailable-tool.ts- ensure alltool_useblocks get correspondingtool_resultand consider deduping shared logic.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/hooks/session-recovery/recover-unavailable-tool.ts">
<violation number="1" location="src/hooks/session-recovery/recover-unavailable-tool.ts:40">
P2: Duplicate code: `readPartsFromSDKFallback` exactly mirrors the function in `recover-tool-result-missing.ts`.</violation>
<violation number="2" location="src/hooks/session-recovery/recover-unavailable-tool.ts:89">
P0: Custom agent: **Opencode Compatibility**
Return a `tool_result` for EVERY `tool_use` block in the message, not just the unavailable one. Filtering out valid tool uses causes a 400 Bad Request error from the Anthropic API.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const matchingToolUses = unavailableToolName | ||
| ? toolUseParts.filter((part) => part.name.toLowerCase() === unavailableToolName) | ||
| : [] | ||
| const targetToolUses = matchingToolUses.length > 0 ? matchingToolUses : toolUseParts |
There was a problem hiding this comment.
P0: Custom agent: Opencode Compatibility
Return a tool_result for EVERY tool_use block in the message, not just the unavailable one. Filtering out valid tool uses causes a 400 Bad Request error from the Anthropic API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/session-recovery/recover-unavailable-tool.ts, line 89:
<comment>Return a `tool_result` for EVERY `tool_use` block in the message, not just the unavailable one. Filtering out valid tool uses causes a 400 Bad Request error from the Anthropic API.</comment>
<file context>
@@ -0,0 +1,108 @@
+ const matchingToolUses = unavailableToolName
+ ? toolUseParts.filter((part) => part.name.toLowerCase() === unavailableToolName)
+ : []
+ const targetToolUses = matchingToolUses.length > 0 ? matchingToolUses : toolUseParts
+
+ const toolResultParts = targetToolUses.map((part) => ({
</file context>
| @@ -0,0 +1,108 @@ | |||
| import type { createOpencodeClient } from "@opencode-ai/sdk" | |||
There was a problem hiding this comment.
P2: Duplicate code: readPartsFromSDKFallback exactly mirrors the function in recover-tool-result-missing.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/hooks/session-recovery/recover-unavailable-tool.ts, line 40:
<comment>Duplicate code: `readPartsFromSDKFallback` exactly mirrors the function in `recover-tool-result-missing.ts`.</comment>
<file context>
@@ -0,0 +1,108 @@
+ )
+}
+
+async function readPartsFromSDKFallback(
+ client: Client,
+ sessionID: string,
</file context>
1 participant
Summary
unavailable_toolas a newRecoveryErrorTypein the session-recovery hookdummy_tool/NoSuchToolError/unavailable toolerror patternsrecover-unavailable-tool.tsthat injects synthetic tool_results to unblock stalled sessionsProblem
When an agent session is interrupted and resumed, the model sometimes hallucinates tool calls to non-existent tools. OpenCode throws
dummy_toolerrors that the session-recovery hook couldn't handle, leaving the agent stuck in an unrecoverable loop.Fix
Added
"unavailable_tool"detection and recovery: finds the invalid tool_use block and injects a synthetic error tool_result, allowing the session to continue normally.Summary by cubic
Prevents stalled sessions by detecting and recovering from unavailable tool calls, with tool-name parsing and SDK/storage fallbacks to inject synthetic tool_results even when assistant parts are missing. Also cleans up stale hook wiring, fixes a TS parse error, preserves disable_omo_env, and hardens tests/CI; addresses Linear #1803.
Bug Fixes
Tests/CI
Written for commit db9df55. Summary will update on new commits.