Skip to content

fix(group): persist group chats and route subchat whispers - #5847

Open
uhhgoat wants to merge 10 commits into
odysseus-dev:devfrom
uhhgoat:reopen/20260731-pr-4137-group-chat-state-persistence
Open

fix(group): persist group chats and route subchat whispers#5847
uhhgoat wants to merge 10 commits into
odysseus-dev:devfrom
uhhgoat:reopen/20260731-pr-4137-group-chat-state-persistence

Conversation

@uhhgoat

@uhhgoat uhhgoat commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Reopens and rebases the closed #4137 group-chat persistence fix against the new odysseus-dev/odysseus dev branch. The change persists group chat state server-side, hides participant child sessions from normal top-level session views, exposes participants through the parent session disclosure, routes direct child sends as private group whispers, cascades parent moves/deletes to participant children, and preserves legacy/null-owner group state rows through the current owner-filtering behavior.

Linked Issue

Fixes #4038.

Type of Change

  • Bug fix (non-breaking - fixes a confirmed issue)
  • New feature (non-breaking - adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

How to Test

Manual review flow: create a group chat with multiple personas, reload or switch away and back, confirm the parent restores group mode and participants, expand the parent row to inspect participant children, open a child raw transcript, send a whisper, and confirm the parent mirrors the exchange while child lifecycle actions remain blocked.

Validation run while reopening:


git diff --check origin/dev...HEAD
python -m py_compile routes/chat_routes.py routes/session_routes.py src/session_actions.py src/session_search.py tests/test_group_chat_state_routes.py tests/test_group_chat_storage.py tests/test_session_actions_cleanup.py tests/test_session_search.py
node --check static/app.js
node --check static/js/group.js
node --check static/js/sessions.js
node --check static/js/chat.js
node --check static/js/modelPicker.js
node --check static/js/chatRenderer.js

Local pytest was not available in the bundled Python runtime, so the focused pytest suite is left to GitHub CI.

Checklist

  • I searched existing issues and PRs for duplicates before opening the linked issue.
  • The PR links the issue it fixes.
  • I added or updated focused tests for the changed behavior.
  • I ran the validation commands listed above.
  • The change is scoped to group chat persistence, participant visibility, and direct child whisper behavior.
  • I did not add new runtime dependencies.

@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jul 31, 2026

@RaresKeY RaresKeY left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the complete latest head against current dev and found four issues, including one regression that prevents the main streaming chat route from running.

Findings

P1 Badge issue (chat runtime): Restore the independent incognito request flag

  • Problem: The stream handler previously assigned incognito beside the other request flags. This change replaces that assignment with group_internal, but the handler still passes incognito into context construction and reads it throughout the response path. There is no remaining assignment in the function.

  • Impact: Every /api/chat_stream request that reaches context construction raises UnboundLocalError before model execution, so ordinary chat UI requests fail globally.

  • Ask: Keep the existing incognito parse and assign group_internal separately. Please add a route-level streaming regression covering ordinary, incognito, and group-internal requests.

  • Location: routes/chat_routes.py:843, routes/chat_routes.py:1093

P2 Badge issue (data integrity): Make group cascade deletion atomic and check every deletion

  • Problem: Group deletion commits each child separately, then deletes the parent, then removes group state. A child deletion returning False is ignored. The underlying delete helper converts database errors into False, so a failed child can be left behind while the parent and group state disappear; a later failure can instead leave only some histories permanently deleted.

  • Impact: An ordinary group deletion during a persistence failure can silently orphan participant histories or irreversibly erase only part of the group.

  • Ask: Delete parent, participants, and group state in one checked transaction, roll back on any failure, and update in-memory state only after commit. Please add injected-failure tests for a child and the parent.

  • Location: routes/session_routes.py:378, core/session_manager.py:542

P2 Badge issue (API consistency): Mirror child whispers on /api/chat as well as /api/chat_stream

  • Problem: The child-to-parent lookup and both mirror calls exist only in the streaming route. The supported non-streaming /api/chat route still accepts and persists messages to the same child sessions but never mirrors the user or assistant message to the group parent.

  • Impact: API and non-stream clients can successfully send a whisper while the parent group transcript silently loses that exchange, so persisted history depends on the transport used.

  • Ask: Apply the same owner-scoped mirroring to /api/chat, or explicitly reject group-child use there if it is unsupported. Please add an API-level regression for both directions and for avoiding duplication on internal group requests.

  • Location: routes/chat_routes.py:701, routes/chat_routes.py:1261, routes/chat_routes.py:1756

P3 Badge issue (accessibility): Put the whisper destination in the accessible name

  • Problem: The button's visible text and accessible name remain Whisper; only its title changes to Whisper to <participant>. A title is not a reliable destination announcement when the button already has child text.

  • Impact: Screen-reader users cannot reliably determine which participant will receive the next whisper before activating the control.

  • Ask: Update an aria-label or associated accessible description with the active participant and cover the state transition in a DOM/browser accessibility test.

  • Location: static/index.html:1174, static/app.js:970

Validation

  • 35 focused persistence, storage, lifecycle, and search tests passed in the secretless runner.
  • Changed Python compilation, all changed JavaScript syntax checks, and git diff --check passed.
  • A fresh conflict check against current dev at e7eddbae passed, and source inspection confirmed that all four findings remain present.
  • All visible current-head checks pass, but none exercises the broken streaming runtime, failed child deletion, non-streaming whisper path, or accessibility tree.
  • Browser/mobile group interaction and restart migration behavior remain to be checked after the fixes.

Merge current dev, restore independent streaming flags, mirror sync whispers, make group deletion atomic, and keep the whisper control's accessible name participant-specific.
@uhhgoat

uhhgoat commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all four findings in c2480d11 (and merged current dev at 17ee856d):

  • restored independent incognito and group_internal parsing, with route-level ordinary/incognito/group-internal coverage;
  • staged parent, participants, messages/documents, gallery state, and group state in one checked transaction; rollback tests now cover a child returning False and an injected parent failure, while memory eviction and file unlinking occur only after commit;
  • mirrored both user and assistant whispers through /api/chat, with group_internal suppressing duplicates;
  • added a participant-specific aria-label plus DOM state-transition coverage and a local browser smoke check.

Validation: 5053 passed, 4 skipped for the full pytest suite; focused/adjacent regressions were 47 passed; node --check and git diff --check passed. GitHub CI is running on the pushed commit.

@github-actions

Copy link
Copy Markdown

⚠️ PR description is complete; validation evidence is still outstanding

Changed-file classification: UI-sensitive.

Author-reported runtime / visual state

  • App/runtime validation is not author-attested. Check the run box only after running it, or check the explicit not-run box and describe the gap.
  • The screenshot/clip checkbox is not checked for this UI-sensitive change.
  • The Screenshots / clips section does not contain an actual attachment or link.

Checkboxes are author attestations. GitHub Actions results remain the execution evidence for CI; this check does not prove that a local command ran.


This comment updates automatically when the description or changed files change.

@github-actions github-actions Bot added needs runtime validation Runtime validation not attested — tick the app-run box after running it, or state the gap needs visual evidence UI-sensitive change without an attested screenshot or clip from the running app and removed ready for review Description complete — ready for maintainer review labels Aug 18, 2026
@uhhgoat

uhhgoat commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the unstable pytest check in commit 060114a and merged current dev.

The group-chat regressions had drifted from two current route interfaces:

  • the shared foreground-routing query double did not implement all(), which the owner-scoped group-child lookup now uses;
  • the non-streaming whisper test still mocked llm_call_async even though /api/chat now uses llm_call_async_with_route_fallback and returns requested/actual route provenance.

The focused updates keep production behavior unchanged while exercising the current fallback-aware path.

Validation:

  • focused Docker routing/group-chat suite: 107 passed;
  • Python compileall, all 7 PR-specific JavaScript syntax checks, and git diff --check: passed;
  • GitHub: 18 checks passed, 1 expected SARIF-upload job skipped, 0 failures;
  • PR is mergeable and clean.

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

Labels

needs runtime validation Runtime validation not attested — tick the app-run box after running it, or state the gap needs visual evidence UI-sensitive change without an attested screenshot or clip from the running app

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Group chats reopen as normal chats and expose participant sessions

2 participants