Repository navigation
fix(server): keep OpenCode timeouts from crashing the error formatter - #12469
tachytelicdetonation wants to merge 4 commits into
Conversation
`openCodeRuntimeErrorDetail` called `.trim()` on `cause.message` for every `Error`, but Effect's `TimeoutError` can be constructed without a message. When an unresponsive OpenCode timed out the prompt and its cleanup abort, formatting the abort cause threw a TypeError, which replaced the useful timeout warning with a defect and skipped prompt-admission recovery. Only trim `message` when it is a string, and fall through to the existing object and `String(cause)` fallbacks otherwise. Covered by unit tests for the formatter and an adapter test for the prompt-timeout cleanup path. Fixes pingdotgg#12456 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a small, focused server bug fix with targeted coverage for message-less timeout errors and cleanup recovery, while normal formatting behavior remains unchanged. A separate unresolved High-severity concern remains about reading a potentially unstable Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe formatter now handles missing or non-string error messages. Tests cover fallback formatting and prompt cleanup recovery when the prompt or cleanup abort times out. ChangesOpenCode timeout recovery
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The timeout recovery change safely preserves diagnostic details and recovery scheduling for the covered error shapes. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The issue's repro is a prompt that times out whose cleanup abort then times out as well. The existing test reproduces the crashing error shape directly; this one drives the full sequence through the adapter and checks the warning carries the abort timeout and recovery still runs. The vendored Effect supplies a timeout message, so this sequence passes with or without the formatter fix; it pins the recovery path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Dismissing prior approval to re-evaluate 7c67a7d
| if (cause instanceof Error && typeof cause.message === "string") { | ||
| const message = cause.message.trim(); | ||
| if (message.length > 0) return message; | ||
| } |
There was a problem hiding this comment.
🟠 High provider/opencodeRuntime.ts:126
This formatter throws instead of returning a diagnostic when an Error subclass changes or throws on its second message getter access. The typeof check and subsequent .trim() read cause.message separately; read it once into a local before checking and trimming.
| if (cause instanceof Error && typeof cause.message === "string") { | |
| const message = cause.message.trim(); | |
| if (message.length > 0) return message; | |
| } | |
| if (cause instanceof Error) { | |
| const message = cause.message; | |
| if (typeof message === "string") { | |
| const trimmedMessage = message.trim(); | |
| if (trimmedMessage.length > 0) return trimmedMessage; | |
| } | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/opencodeRuntime.ts around lines 126-129:
This formatter throws instead of returning a diagnostic when an `Error` subclass changes or throws on its second `message` getter access. The `typeof` check and subsequent `.trim()` read `cause.message` separately; read it once into a local before checking and trimming.
What Changed
openCodeRuntimeErrorDetailinapps/server/src/provider/opencodeRuntime.tsassumed everyErrorhas a stringmessageand called.trim()on it. It now only trimsmessagewhen it is a string and otherwise falls through to the existing object andString(cause)fallbacks.Tests added:
opencodeRuntime.errorDetail.test.ts: the formatter returns a string for anErrorwith anundefinedor non-stringmessage, fornew Cause.TimeoutError()built without a message, and keeps its existing outputs for runtime errors, trimmed messages, SDK response shapes, and primitives.OpenCodeAdapter.test.ts: a prompt that times out whose cleanup abort then fails with a message-less error still emits the "cleanup abort did not complete" warning with a readable detail and still schedules prompt-admission recovery.OpenCodeAdapter.test.ts: the issue's full sequence, a prompt timeout followed by a cleanup abort timeout, emits the same warning carrying the abort timeout and still schedules recovery. The vendored Effect gives that timeout a message, so this test passes before and after the formatter fix; it pins the recovery path.Why
Fixes #12456.
When OpenCode is unresponsive,
sendTurntimes outsession.promptAsyncand then the cleanupsession.abort. Formatting the abort cause ran beforeschedulePromptAdmissionRecovery. Effect'sTimeoutErrorcan be constructed without a message, so the formatter threwTypeError: Cannot read properties of undefined (reading 'trim'). That defect replaced the useful timeout warning, recovery never ran, and every later message on the thread failed the same way. This follows the suggested fix in the triage and mirrors the guard already used bylegacySetupFailureDescriptioninapps/server/src/ws.ts.Verification on this branch:
mainwith the TypeError from the issue and pass with the fix.OpenCodeAdapter.test.tsand allopencodeRuntime*.test.tsfiles: 177 passed.apps/servertypecheck, lint, andvp fmt --checkare clean.Checklist
Work done with Claude Fable 5.1 in Claude Code.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests