fix(tui): clear stuck streaming when final has no active run in flight#52745
fix(tui): clear stuck streaming when final has no active run in flight#52745lyksdu wants to merge 15 commits intoopenclaw:mainfrom
Conversation
Add a function to determine if orphaned activity status should be cleared when no active chat run exists.
Greptile SummaryThis PR fixes a TUI status line bug where the footer could remain stuck on
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/tui/tui-event-handlers.test.ts
Line: 397-423
Comment:
**Test doesn't exercise the new code path**
This test sets `state.activeChatRunId = null` and then sends the final for the same `"run-orphan"` run. Inside `handleChatEvent`, the final event goes through `noteSessionRun("run-orphan")` first, and then:
```ts
if (!state.activeChatRunId && !isLocalBtwRunId?.(evt.runId)) {
state.activeChatRunId = evt.runId; // re-assigns to "run-orphan"
}
```
Because `activeChatRunId` is `null` and the run is not a btw run, it gets **re-registered as the active run** before `wasActiveRun` is captured. As a result, `wasActiveRun = true` and `setActivityStatus("idle")` is called via the **pre-existing `params.wasActiveRun` branch** — not via the new `shouldClearOrphanedActivityStatus()` path. The test would pass identically with or without the PR's changes.
To actually reach `shouldClearOrphanedActivityStatus()`, you'd need `activeChatRunId` to be non-null (pointing at a different run) so the re-assignment is skipped and `wasActiveRun` stays `false`, or the run must be a btw run (where the re-assignment is blocked by `!isLocalBtwRunId` being false). Consider adding a test case like:
```ts
// activeChatRunId pointing to a different, already-finalized run so re-assignment is skipped
state.activeChatRunId = "run-other"; // non-null, non-matching
// then finalize "run-other" first so sessionRuns is empty after run-orphan finalizes
handleChatEvent({ runId: "run-other", ..., state: "final", message: null });
// Now activeChatRunId is null (cleared by clearActiveRunIfMatch) and sessionRuns is empty
// Send streaming delta + final for run-orphan to validate shouldClearOrphanedActivityStatus()
```
This gap means `shouldClearOrphanedActivityStatus()` currently has no passing test that proves it's reachable and effective.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add tests for orphaned streaming and act..." | Re-trigger Greptile |
Update test to ensure activeChatRunId is null when final arrives inactive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e380edcd2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Refactor test setup for orphan-clear event handling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3db3b912de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Removed pendingHistoryRefresh variable and related logic to streamline event handling.
Refactor setActivityStatus to update state directly and add regression test for handling late final events after errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a57b182d51
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Refactor event handler types and functions for better clarity and functionality.
Added logic to manage pending history refresh based on active runs and session state.
Summary
Describe the problem and fix in 2–5 bullets:
streaming • … | connectedafter the assistant turn has visibly finished, when the chatfinalevent is processed withwasActiveRun === false(i.e.state.activeChatRunId !== evt.runId) while earlierdeltaevents had already calledsetActivityStatus("streaming"), and there is no concurrent in-flight run (sessionRunsempty after finalization,activeChatRunIdalreadynull).agent.wait/ LLM timeout then recovery).finalizeRunandterminateRunnow also callsetActivityStatuswhenshouldClearOrphanedActivityStatus()is true (!activeChatRunId && sessionRuns.size === 0), plus two unit tests (orphan clear vs concurrent run preserved).sessionRuns(concurrent runs still do not get their activity cleared by an inactive final).Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
activeChatRunId/ subagent lane discussions)User-visible / Behavior Changes
connected | idle) instead of staying onstreamingindefinitely when no run remains in flight.Security Impact (required)
Yes, explain risk + mitigation: N/ARepro + Verification
Environment
openclaw tuiruns)openclaw-tui)Steps
agent.wait(e.g. multi-agent search with slow or failing tools).finalfor a run may arrive whenactiveChatRunIdno longer matches thatrunId, whilesessionRunsis already empty.Expected
streamingstate when no chat run is in flight for the session.Actual (before fix)
streaming • Ns | connectedfor minutes after content looks complete.Evidence
Attach at least one:
agent.waittimeout + failover around stuck UI)Human Verification (required)
What you personally verified (not just CI), and how:
src/tui/tui-event-handlers.test.tsfor (1) inactive final clears activity when no runs remain, (2) inactive final does not clear when another run is still in flight.run-a/run-bpath in test harness; explicitactiveChatRunId = nullbefore final to simulate pointer loss.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Failure Recovery (if this breaks)
src/tui/tui-event-handlers.ts(and associated tests).tui-event-handlers.ts/tui-event-handlers.test.ts.setActivityStatusfires too aggressively, concurrent runs might briefly show idle while another run is still streaming—mitigated by requiringsessionRuns.size === 0andactiveChatRunId === null.Risks and Mitigations
activeChatRunIdis null andsessionRunsis empty could theoretically align poorly with a future gateway behavior that delaysnoteSessionRunfor a successor run.sessionRunsblocks the orphan clear. Add regression tests if gateway ordering changes.