fix(tui): isolate embedded event listener failures#100117
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 10:35 PM ET / 02:35 UTC. Summary PR surface: Source -1, Tests +41. Total +40 across 2 files. Reproducibility: yes. Source inspection shows current main discards a promise returned by an async embedded event handler while the shared bus only isolates synchronous listener throws, and the PR body includes strict unhandled-rejection runtime proof for the repaired path. Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the focused synchronous-handler repair with the regression once normal maintainer review and required checks are satisfied. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main discards a promise returned by an async embedded event handler while the shared bus only isolates synchronous listener throws, and the PR body includes strict unhandled-rejection runtime proof for the repaired path. Is this the best way to solve the issue? Yes. Removing the unnecessary async boundary is narrower and more maintainable than adding a local catch-and-swallow path because it reuses the existing shared listener isolation contract. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against eb2f9bece730. Label changesLabel justifications:
Evidence reviewedPR surface: Source -1, Tests +41. Total +40 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (9 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
ProofBehaviorThe Source
this.unsubscribe = onAgentEvent((evt) => {
void this.handleAgentEvent(evt).catch(() => {});
});Test outputNotes
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
8f9a14c to
761e0dc
Compare
steipete
changed the title
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Non-maintainer proof pass on the latest head
I do not see a code-change blocker from this pass. The remaining maintainer decision is whether the posted runtime-injection proof is enough substitute for a fully interactive embedded TUI transcript; the scoped TUI guide still warns that PTY smoke does not prove every embedded backend runtime path. |
clawsweeper
Bot
added
proof: sufficient
openclaw-barnacle
Bot
added
docs
0a21c5a to
3494071
Compare
openclaw-barnacle
Bot
added
size: XS
and removed
docs
|
Merged via squash.
|
* fix(tui): suppress unhandled rejection from async agent event handler * fix(tui): isolate embedded event listener failures --------- Co-authored-by: Peter Steinberger <[email protected]>
3 participants
What Problem This Solves
EmbeddedTuiBackend.handleAgentEventwas declaredasynceven though it contains no asynchronous work. When a TUI event consumer threw synchronously, that unnecessaryasyncboundary converted the error into a rejected promise; the fire-and-forget agent-event callback did not observe it, so Node could terminate the TUI for an unhandled rejection.Why This Change Was Made
Keep the embedded event handler synchronous and call it directly from
onAgentEvent. This restores the existing shared event-bus contract:notifyListenersisolates synchronous listener failures while continuing to notify other consumers. A focused regression injects a throwing TUI consumer through the real shared listener helper.This supersedes the original noop
.catch()suggestion. Removing the redundant async boundary fixes the root cause, preserves event ordering, and reduces production code instead of adding a silent catch path.User Impact
Embedded/local TUI sessions no longer risk a process-level unhandled rejection when a TUI event consumer throws. Normal agent, chat, lifecycle, and local PTY behavior remains unchanged.
Evidence
Focused regression
The new regression starts a local run, routes an assistant event through
notifyListeners, injects a throwingbackend.onEventconsumer, and verifies the event bus contains the failure.Local TUI PTY
This includes the slower
tui --localsmoke, with only the external model endpoint mocked.Non-test runtime injection
Ran the real source backend and global agent-event bus outside Vitest with Node's strict unhandled-rejection mode. The harness installed an active embedded run, made
backend.onEventthrow, emitted an assistant event throughemitAgentEvent, waited one event-loop turn, and then stopped the backend.Without the synchronous repair, the thrown consumer error crosses the unnecessary async boundary as a rejected promise and strict Node exits nonzero. With this branch, the process remains alive because
notifyListenerscontains the synchronous listener failure.Review
Broad changed gate
Testbox-through-Crabbox provider
blacksmith-testbox:tbx_01kwqyrrwwfn8tdnw4ajh48rd8OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 pnpm check:changedScope
src/tui/embedded-backend.ts(net -1 line)src/tui/embedded-backend.test.ts