Skip to content

fix(gateway): ws-orphan reaper leaves the canonical Bot Chat resumable - #93077

Closed
ClintonEmok wants to merge 1 commit into
NousResearch:mainfrom
ClintonEmok:fix/canonical-bot-chat-reap
Closed

fix(gateway): ws-orphan reaper leaves the canonical Bot Chat resumable#93077
ClintonEmok wants to merge 1 commit into
NousResearch:mainfrom
ClintonEmok:fix/canonical-bot-chat-reap

Conversation

@ClintonEmok

Copy link
Copy Markdown
Contributor

What does this PR do?

Bot Mode's forever-chat is identified by (profile_name, title='Bot Chat'), not by stored session ids. When the desktop's WebSocket stayed down past _WS_ORPHAN_REAP_GRACE_S, _finalize_session(end_reason='ws_orphan_reap') ended that durable row — which archives it. On reopen, findExistingCanonicalChat located the archived row correctly but the open path rejected it; the plugin's recreate then collided with the global idx_sessions_title_unique index and forked throwaway auto-titled sessions (Tell me about yourself #N, ...). The bot appeared to have lost its memory, with hundreds of messages of history invisible from every UI path (#92687).

ws_orphan_reap is already classified as an accidental, recoverable end reason elsewhere in the codebase — promote_to_session_reset and find_latest_gateway_session_for_peer both treat it as recoverable. This PR extends that same leniency to the canonical Bot Chat at the write site: when an accidental reap reason targets a profile-scoped Bot Chat row, only the DB end-write is skipped (with an info log); in-process teardown still runs.

Explicit user boundaries are untouched: tui_close, session.close, /new still end the row normally. Ordinary sessions are unaffected, and the #60609 gateway-owner guard keeps precedence (it runs first).

Related Issue

Fixes #92687

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tui_gateway/server.py
    • CANONICAL_BOT_CHAT_TITLE = 'Bot Chat' + _is_canonical_bot_chat_row() — identity check requiring a non-empty profile_name and exact title match
    • _ACCIDENTAL_END_REASONS = {'ws_orphan_reap'} — the accidental-reason set, kept deliberately narrow
    • In _finalize_session: when the TUI owns lifecycle, the end reason is accidental, and the row is a canonical Bot Chat, skip db.end_session(...) and log why
  • tests/tui_gateway/test_canonical_bot_chat_reap.py (new) — mirrors the existing test_gateway_owned_session_reap.py conventions

How to Test

  1. python3 -m pytest tests/tui_gateway/test_canonical_bot_chat_reap.py tests/tui_gateway/test_gateway_owned_session_reap.py -q → 16 passed
  2. Repro of the original bug on main: desktop Bot Chat open → kill WS for at least the reap grace → observe state.db: canonical row gets ended_at/end_reason='ws_orphan_reap'. With this branch the row stays open while teardown still proceeds.
  3. Explicit close still works: session.close / app quit ends the row as before (covered by test).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run targeted tests (full tests/tui_gateway/: all green except one pre-existing unrelated failure also present on clean main)
  • I've added tests for my changes (required for bug fixes)
  • I've tested on my platform: macOS 26.5 (Apple Silicon)

Documentation and Housekeeping

  • I've updated relevant documentation — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure Python logic, no platform-specific paths
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 23, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

The reap guard itself is well done: exact-title + profile-scoped identity, explicit-boundary reasons still end the row, gateway-owned sources keep the #60609 guard, and the missing-row case stays legacy. The pure picker-state.ts module with atomic tmp+rename writes and corrupt-file tolerance is also solid. Points:

  1. Scope: this PR bundles two unrelated fixes — the canonical Bot Chat reap protection (Bot Mode canonical 'Bot Chat' archived by ws_orphan_reap is not resurrected on reopen — every click creates a new transient session #92687, tui_gateway/server.py) and the last-used-directory picker memory ([Feature]: Desktop — attach file dialog should remember the last-used directory #92925, Electron main). The title mentions only one. Splitting would make bisection and changelog entries cleaner; at minimum list both in the description.

  2. tui_gateway/server.py:753-764_is_canonical_bot_chat_row matches any row titled exactly "Bot Chat" with a profile, regardless of how it was created. A user who manually names their own desktop session "Bot Chat" gets accidental-reap immunity too (their row stays open across WS drops until an explicit close). Low impact since only the end-write is skipped, but scoping on source == "desktop" (or whatever the plugin mints) would tighten it.

  3. apps/desktop/electron/main.ts:14084-14088 — the remembered directory is written on every successful pick, including picks that supplied an explicit composer-cwd default. That's arguably correct (memory should track actual usage), but worth confirming it's intended versus "only remember when we had no opinion," since the two policies diverge for users who mostly pick from suggested folders.

  4. _ACCIDENTAL_END_REASONS as a frozenset is nicely extensible — if transport-loss reasons grow (e.g. timeout variants), this is the single place to add them.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Heads-up from #92687 triage: this PR and #93217 attack the same bug from complementary angles — this one prevents the reaper from end-writing the canonical Bot Chat (future protection), #93217 makes the canonical-session lookups resurrect rows already archived by recoverable reasons (heals existing damage + other accidental-archival sources). They compose cleanly; neither blocks the other.

One reviewer note on this PR as it stands: the diff also carries a complete file-picker default-path feature (picker-state.ts + tests + main.ts wiring, ref #92925) that's unrelated to the reap fix — splitting that into its own PR would let each half be reviewed and land on its own timeline.

Bot Mode's forever-chat is identified by (profile_name,
title='Bot Chat'), not by stored session ids. When the desktop's WS
stayed down past _WS_ORPHAN_REAP_GRACE_S, _finalize_session ended the
row with end_reason='ws_orphan_reap', which archives it; on reopen
findExistingCanonicalChat located the archived row but openSession
rejected it, the recreate collided with idx_sessions_title_unique, and
the bot fell back to auto-titled throwaway sessions — 511 messages of
history invisible from every UI path (NousResearch#92687).

ws_orphan_reap is already classified as an accidental, recoverable end
elsewhere (promote_to_session_reset, find_latest_gateway_session_for_peer);
extend that leniency to the canonical chat at the write site: when an
accidental reap reason targets a profile-scoped 'Bot Chat' row, skip only
the DB end-write and log why. In-process teardown still runs, explicit
user boundaries (tui_close, session.close, /new) still end it, ordinary
sessions are unaffected, and the NousResearch#60609 gateway-owner guard keeps
precedence.

Covered by unit tests mirroring test_gateway_owned_session_reap.py:
canonical row spared on reap, explicit close still ends, non-canonical
titles unaffected, gateway-owned rows keep their guard, missing rows keep
legacy behavior.

Fixes NousResearch#92687
@ClintonEmok

Copy link
Copy Markdown
Contributor Author

Review points addressed:

Scope tightening (point 2)_is_canonical_bot_chat_row now also requires source == 'desktop', so a user who manually renames an ordinary TUI/CLI session 'Bot Chat' no longer gains accidental-reap immunity. Covered by a new test; suite is 17 passed.

Point 1 (bundled scope) — fair. #93076 carries the picker half with its own description and fixes the CI lint failure this PR's head shared; once both are green the two can land independently.

Points 3 & 4 — agreed: memory-follows-usage is now documented on #93076, and the frozenset stays as the single extension point for future transport-loss reasons.

On kshitijk4poor's note re: #93217 — agreed they're complementary (this prevents future archive-writes; theirs heals already-archived rows). No conflict from my side; whichever shape maintainers prefer.

@ClintonEmok
ClintonEmok force-pushed the fix/canonical-bot-chat-reap branch from e030d25 to b02bd3d Compare August 23, 2026 21:45
@alt-glitch alt-glitch added area/sessions Session lifecycle, resume, persistence, history and removed comp/desktop Electron desktop app (apps/desktop/*) labels Aug 23, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Scope-tightening fix looks right (source == 'desktop' closes the rename-immunity hole cleanly), and splitting the picker half to #93076 resolves the bundling point — thanks for the quick turnaround. Complementarity with #93217 confirmed on my side too (it merged earlier today, so your reap-exemption now lands on top of the heal-on-read layer). Nothing further from me — LGTM for maintainer review.

@teknium1

Copy link
Copy Markdown
Contributor

Closing: the user-visible symptom (#92687, canonical Bot Chat archived by the reaper) was fixed read-side by #93217 (recoverable end-reason resurrection in hermes_state.py), and the consolidated session-recovery PR #93361 now also prevents most reap-time archives from happening at all (resume cancels pending reaps; superseded runtimes end quietly as a recoverable reason). Your write-side skip became belt-and-braces on top of two landed layers. Thanks @ClintonEmok — your diagnosis of the archive collision was right, it just got fixed from the other side first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bot Mode canonical 'Bot Chat' archived by ws_orphan_reap is not resurrected on reopen — every click creates a new transient session

5 participants