fix: 19 correctness bugs from deep codebase review - #268
Merged
Conversation
claudiusthebot
approved these changes
May 29, 2026
claudiusthebot
left a comment
claudiusthebot
left a comment
Collaborator
There was a problem hiding this comment.
All 19 bugs verified — fixes correct. Pushed a prettier commit (421d63b) on top to clear the Code Quality check (3 files: stream.ts, errors.ts, gateway.ts).
Highlights worth noting:
thinking_delta→currentThinkingTextfix is the sharpest one: the old code passedcurrentBlockText(response accumulator) to the thinking callback, meaning thinking content was silently dropped in real time. Clean fix.isFreeModel||→&&is a correctness trap: any model with free input but paid output (or vice versa) would have been labelled free. Good catch.dreaminglock bounded wait is the right shape — mirrors the heartbeat pattern. A backend ignoring abort indefinitely would have killed the dream loop permanently.fastestResponseMs === nullguard:Infinity → nullin JSON round-trips is a classic JS footgun. Good.
Potential merge-order note:
src/backend/remote-server/session-helpers.tsis also touched by PR #260 (addsid?: stringtoRemoteAssistantInfo). The changes are on nearby but different lines — should merge cleanly, but worth a rebase check if #260 lands first.src/core/cron.tsis also touched by PR #252 (thetimer!non-null assertion fix on a different function). Same situation — different lines, should be fine.src/frontend/telegram/formatting.tsis also touched by PR #252 (URL double-escaping fix, different line). These two fixes are complementary — both should land.
CI will re-run on the prettier commit. 33/34 checks were green already; Code Quality should flip to ✅ shortly.
Collaborator
|
Merge order note (from heartbeat #564, 07:45Z) 8 open PRs, some with overlapping fixes. Recommended merge sequence:
TL;DR: PRs #257 and #260 both fix the same |
## Critical / High
**`sessions.ts` — `fastestResponseMs` permanently stuck after restart**
`Infinity` serializes to `null` in JSON. The migration guard only checked
`=== undefined` and `=== 0`, so after any restart the field stayed `null`
and was never updated again (`durationMs < null` is always false).
Added `=== null` to the guard.
**`claude-sdk/stream.ts` — `thinking_delta` sent wrong text to callback**
`onStreamDelta(state.currentBlockText, "thinking")` passed the accumulated
*response* text rather than the thinking text. Added `currentThinkingText`
to `StreamState`, accumulate thinking deltas there, and pass it to the
callback.
**`claude-sdk/handler.ts` — session name overwritten on flow-violation retry**
`session.turns === 0` is still true on the first retry (incrementTurns
hasn't fired). The recursive call with `text = violation.reminder` would
extract a session name from the reminder string. Guard with
`!_internal.flowRetries`.
**`kilo/models.ts` — `isFreeModel` uses `||` instead of `&&`**
A model with one zero cost and one non-zero cost was incorrectly labeled
free. Changed cost check to `(costInput === 0 && costOutput === 0)`.
**`openai-agents/discovery.ts` — `discoveryAt` not set on failed discovery**
Only set in `.then()`, not `.finally()`. On a network failure, `discoveryAt`
stayed `null` so `awaitDiscovery()` was called again on every model-picker
open, each time incurring a 3-second soft timeout. Moved to `.finally()`.
**`telegram/callbacks.ts` — `ctx.from.id` crashes on channel post callbacks**
Channel posts have no `from` field. Added optional chaining: `ctx.from?.id`.
**`telegram/formatting.ts` — link label text not HTML-escaped**
`[<b>bold</b>](url)` would inject raw HTML into Telegram messages.
Applied `escapeHtml()` to the link label as well as the URL.
## Medium
**`discord/actions.ts` — `edit_message` guard allowed 4 000-char edits**
The guard compared against `DISCORD_MAX_TEXT * 2` (4000) but Discord's
actual edit limit is 2000. Changed to `DISCORD_MAX_TEXT`.
**`cli.ts` — `isConfigured` always returned `false` for Discord frontend**
The default `return false` case matched Discord, causing the CLI to
report Discord as unconfigured even after successful setup.
**`errors.ts` — bare `overflow` regex too broad**
Matched JavaScript `Maximum call stack size exceeded`, numeric overflow
errors, etc., misclassifying them as non-retryable `context_length`.
Changed to require `context.{0,10}overflow`.
**`cron.ts` — `warnedBadSchedule.clear()` caused a re-warning burst**
Clearing all 200 entries at the cap caused every previously-warned job
to re-warn on the very next tick. Now evicts only the oldest entry.
**`gateway.ts` — `Number("") === 0` could match chat context 0**
When `_chatId` is absent, `rawChatId` is `""` and `Number("") === 0`.
Added `rawChatId !== ""` guard to the ambient-context routing branch.
**`dream.ts` — `dreaming` lock held forever if backend ignores abort**
`await agentPromise.catch(() => {})` with no timeout meant that a backend
ignoring its abort signal would block `executeDream` forever, permanently
preventing any future dream runs. Added a `DREAM_ABORT_GRACE_MS` (30s)
bounded wait mirroring the heartbeat pattern.
**`remote-server/lifecycle.ts` — health-check fetch had no timeout**
A server that accepted TCP but never sent a response would stall
`ensureRemoteServer` indefinitely, blocking every subsequent chat turn.
Added `AbortSignal.timeout(5_000)`.
**`opencode/model-provider.ts` — `getProviderModels` was 0-indexed**
All other backends use 1-indexed pagination. `page=1` from a caller
would skip the first page of results. Changed to `page = 1` and
`start = (page - 1) * pageSize`.
## Low
**`triggers.ts` — `commandForBash()` re-probed bash on every spawn**
`spawnSync` was called on every trigger invocation. Cached the result
in a module-level variable after the first successful probe.
**`kilo/handler.ts` / `opencode/handler.ts` — redundant dynamic import**
`setSessionId` was already available from the static `sessions.js` import
at the top of each file. Replaced the dynamic `import()` with the static
binding.
**`remote-server/session-helpers.ts` — rename confusing intermediate variable**
`messageId` was typed as `{ id?: string }` (the info object, not the id
itself). Renamed to `messageInfo` to match what it actually holds.
https://claude.ai/code/session_01M8utiWqUBerWtQ45jdeXgH
claudiusthebot
force-pushed
the
claude/eager-sagan-Fv6mV
branch
from
June 6, 2026 15:19
421d63b to
1f23c6a
Compare
claudiusthebot
enabled auto-merge (squash)
June 6, 2026 15:19
This was referenced Jun 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Deep review of the full codebase (all ~250 source files) by six parallel static-analysis passes surfaced 19 confirmed bugs. All are fixed here; all 2998 tests pass after.
Critical / High
sessions.ts—fastestResponseMspermanently stuck atnullafter restartInfinityserialises tonullin JSON (JSON.stringify({x: Infinity})→{"x":null}). The migration guard at startup only checked=== undefinedand=== 0, so after any process restartfastestResponseMswas loaded asnulland stayed there forever —durationMs < nullis alwaysfalsein JS. Added=== nullto the guard.claude-sdk/stream.ts—thinking_deltacallback passed response text, not thinking textonStreamDelta(state.currentBlockText, "thinking")fired withcurrentBlockText— which accumulates response text deltas, not thinking deltas. Thinking content was silently dropped. Added acurrentThinkingTextfield toStreamState, accumulatethinking_delta.thinkinginto it, and pass that to the callback.claude-sdk/handler.ts— session name overwritten with flow-violation reminder on first retrysession.turns === 0is stilltrueon the first flow-violation retry (becauseincrementTurnshasn't fired yet). The recursive call passestext: violation.reminder, soextractSessionName(violation.reminder)would produce a session name like"[FLOW VIOLATION] Your previous...". Guard with!_internal.flowRetries.kilo/models.ts—isFreeModeluses||instead of&&for cost checkopenai-agents/discovery.ts—discoveryAtnot set on failed discoverystate.discoveryAt = Date.now()was only in the.then()path. On a network failure,discoveryAtstayednull, causinghasAttemptedDiscovery()to returnfalseon every subsequent call andawaitDiscovery()to incur a 3-second soft timeout on every model-picker open when the endpoint is unreachable. Moved to.finally().telegram/callbacks.ts—ctx.from.idcrashes on channel post callbacksChannel posts have no
fromfield —ctx.fromisundefinedfor them. Added optional chaining:ctx.from?.id.telegram/formatting.ts— Markdown link label not HTML-escapedMedium
discord/actions.ts—edit_messageguard allowed 4000-char editsThe early-rejection guard compared against
DISCORD_MAX_TEXT * 2(4000), but Discord's API limit for edits isDISCORD_MAX_TEXT(2000). Text between 2001–4000 chars passed the guard but was silently truncated at the send step. Changed the guard toDISCORD_MAX_TEXT.cli.ts—isConfiguredalways returnedfalsefor Discord frontenderrors.ts— bareoverflowregex too broad/overflow/imatchedMaximum call stack size exceeded, numeric overflow, buffer overflows, etc., silently misclassifying them as non-retryablecontext_lengtherrors and suppressing retries that might otherwise succeed. Tightened tocontext.{0,10}overflowso only context-related overflow messages match.cron.ts—warnedBadSchedule.clear()caused a re-warning burst at the capWhen the 200-entry cap was hit,
clear()removed all entries. On the very next tick, every previously-warned job was no longer in the set and re-logged a warning — producing a burst of 200 log lines. Now evicts only the oldest entry viaSetinsertion-order iteration.gateway.ts—Number("") === 0could match chat context 0When
_chatIdis absent,rawChatId = ""andNumber("") === 0.!isNaN(0)istrue, so if any chat with numeric ID0ever appeared inchatContexts, a bare tool call with no_chatIdwould route to it. The explicit-routing branch already guards withrawChatId !== ""; added the same guard to the ambient-context branch.dream.ts—dreaminglock held forever if backend ignores abort signalawait agentPromise.catch(() => {})with no timeout meant a backend that ignored its abort signal would blockexecuteDreamindefinitely. Sincedreaming = falseis inexecuteDream'sfinallyblock, it would never reset, silently killing all future dream runs until a process restart. Added aDREAM_ABORT_GRACE_MS(30 s) bounded wait, mirroring the pattern inheartbeat.ts.remote-server/lifecycle.ts— health-checkfetchhad no timeoutA server that accepted TCP connections but never sent an HTTP response would stall
reuseExistingServer— and thereforeensureRemoteServer, called on every chat turn — indefinitely. AddedAbortSignal.timeout(5_000).opencode/model-provider.ts—getProviderModelswas 0-indexed while all other backends use 1-indexedpage = 0withstart = page * pageSizemeant a caller passingpage=1(the standard from other backends) skipped the firstpageSizemodels entirely. Changed topage = 1/start = (page - 1) * pageSize.Low
triggers.ts—commandForBash()re-probed bash on every trigger spawnspawnSync(cmd, ["-lc", "exit 0"])was called synchronously on the hot path of every bash trigger invocation. The result is the same every time (bash either exists or it doesn't). Cached it in a module-level variable after the first probe.kilo/handler.ts/opencode/handler.ts— redundant dynamicimport()ofsetSessionIdBoth files already statically import from
../../storage/sessions.js.setSessionIdwas the one export not included in the static import, causing aawait import(...)at runtime. AddedsetSessionIdto the static import and removed the dynamic call.remote-server/session-helpers.ts— confusing variable namemessageIdfor the info blobThe intermediate variable was named
messageIdbut heldmessage.info(the whole info object). Renamed tomessageInfoto match what it actually holds and eliminate a misleading hint that it contained just an ID.Test plan
npm test— 2998 tests pass, 34 skipped (same skipped count asmain)npm run typecheck— cleanhttps://claude.ai/code/session_01M8utiWqUBerWtQ45jdeXgH
Generated by Claude Code