-
Notifications
You must be signed in to change notification settings - Fork 57
Description
Problem
processMessage (lines 1066-1743 in src/core/bot.ts) handles too many concerns in one procedural flow with 20+ mutable closure variables acting as an implicit state machine. This makes it fragile, hard to reason about, and prone to subtle bugs when new recovery paths are added.
Current structure
The method handles all of these in a single for await loop with shared mutable state:
- Stream consumption and type routing (~80 lines)
- Foreground run identification and competing run filtering (~40 lines)
- Multi-assistant-UUID detection and message finalization (~50 lines)
- Live-edit streaming with rate limit backoff (~30 lines)
- Result text vs streamed text divergence resolution (~30 lines)
- Stale/cancelled run detection and retry (~30 lines)
- Approval conflict recovery (2 separate paths, ~50 lines)
- Empty/error result retry with orphaned approval enrichment (~50 lines)
- Missing foreground terminal result detection (~15 lines)
- Directive parsing (~10 lines)
- Final message delivery with edit-fallback-to-new (~20 lines)
- No-visible-response fallback (~10 lines)
Specific fragility points
1. response buffer is a footgun
Cleared in at least 5 places (lines 1212, 1243, 1459, 1659, 1673) and set from 3 different sources (streamed assistant chunks, result text fallback, error formatting). sentAnyMessage is supposed to be the authoritative "did we deliver" flag, but finalizeMessage() clears response silently, so later checks like hasResponse (line 1493) can be false even though text was already delivered to the user.
2. The result handler (lines 1437-1630) is the most complex section
- 5-way decision tree for whether to use result text vs streamed text
- Calls
buildResultRetryDecisiontwice (lines 1531 and 1554) with an API enrichment call in between - 3 separate retry paths (approval conflict, empty result, error result), each invalidating the session and recursively calling
processMessage - Recursion is bounded by
retriedbut state carry-across is hard to verify
3. Run ID tracking is defensive but opaque
expectedForegroundRunId is set on the first assistant or result event. Everything before that gets buffered/deferred. The system can't know which run to display until content is already streaming, so the buffer-then-flush pattern adds complexity to every downstream consumer.
4. The sentAnyMessage / response / messageId trio is an implicit state machine
Possible states (not explicitly modeled):
- No response yet
- Streaming via edits (messageId set)
- Finalized mid-stream (response cleared, sentAnyMessage true)
- Finalized at end
- Cancelled
- No-reply marker
- Error-formatted response
These states are implicit in the combination of variables, not explicit in the code.
Suggested decomposition
Split into focused components with explicit state transitions:
- StreamConsumer -- consumes raw stream, handles run filtering and deduplication, yields semantic events
- ResponseAccumulator -- tracks response text, handles multi-turn finalization, owns the
response/messageId/sentAnyMessagestate as explicit states - ResultHandler -- receives result event, makes retry/recovery decisions, returns an action rather than executing side effects inline
- DeliveryManager -- handles streaming edits, final send, edit-fallback, rate limiting
Each as a class or module with well-defined inputs/outputs rather than 20+ mutable variables in a closure.
Impact
This isn't causing user-visible bugs right now (the current code works), but every new recovery path or edge case handler added to this method increases the risk of state interaction bugs. The PR #513 fix (run filtering) added ~100 lines of state tracking to an already complex method. Future changes (e.g. multi-message responses, richer streaming, new error recovery) will compound this.