Skip to content

fix(desktop): scope sessions to selected profile - #70630

Closed
TheAngryPit wants to merge 1 commit into
NousResearch:mainfrom
TheAngryPit:codex/fix-profile-sidebar-scope
Closed

fix(desktop): scope sessions to selected profile#70630
TheAngryPit wants to merge 1 commit into
NousResearch:mainfrom
TheAngryPit:codex/fix-profile-sidebar-scope

Conversation

@TheAngryPit

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes the Hermes Desktop profile rail a consistent session-sidebar workspace boundary.

Concrete profiles now receive only their own recents, cron sessions, Messaging sessions, pagination state, and pins. Cross-profile aggregation remains available only through the explicit All Profiles scope. The same invariant is enforced by the batched backend endpoint, the legacy fallback, and the Electron remote-backend splice.

Related Issue

Fixes #70629

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Tests (adding or improving test coverage)

Changes Made

  • Scope all batched sidebar slices in hermes_cli/web_server.py.
  • Preserve the selected scope in Desktop compatibility and remote-routing paths.
  • Prevent previous-profile responses from overwriting the new profile after a switch.
  • Scope full-text search through the selected backend/profile and bind each
    response to the scope that produced it.
  • Reset Messaging pagination totals/truncation on scope changes.
  • Filter all session categories and pin resolution at the concrete-profile boundary.
  • Add deterministic backend, store, race, pagination, pin, and Electron routing regressions.

No session database, profile config, credential, migration, or gateway behavior is changed.

How to Test

  1. Run the focused backend tests for /api/profiles/sessions/sidebar.
  2. Run the focused Desktop/Electron Vitest files.
  3. Select each concrete profile and verify that only its local, cron, and Messaging rows appear.
  4. Select All Profiles and verify that rows aggregate with profile provenance.
  5. Switch profiles while a sidebar or search request is delayed and verify that
    the old response is ignored.

Automated proof

  • 2 passed backend endpoint tests.
  • 105 passed focused Desktop/Electron tests.
  • Desktop typecheck passed.
  • ESLint passed for all changed Desktop/Electron files.
  • Structured Codex Auto Review: clean, with no accepted/actionable findings.

Checklist

Code

  • I searched for existing issues and PRs covering the exact bug.
  • The PR contains only changes related to profile-scoped sidebar sessions.
  • Regression tests were added for the affected paths.
  • Tested on macOS using synthetic fixtures; no private transcripts were used.

Documentation & Housekeeping

  • No config keys or migrations were added.
  • Cross-platform impact was considered: the state/routing logic is platform-neutral and Electron routing tests cover the remote path.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) comp/cli CLI entry point, hermes_cli/, setup wizard area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state needs-decision Awaiting maintainer decision before any implementation labels Jul 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #42934 and #63618 address the active-profile Messaging sidebar path. This PR also changes the backend slices, remote Electron routing, search, pins, pagination, and stale-response handling, so it is broader rather than a duplicate. Please choose whether to consolidate the overlapping profile-scope work.

@zakhounet

zakhounet commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Really nice to see the sidebar scoping pulled together here — routing recents/cron/messaging through sidebarSessionSliceProfiles reads like the right call for #70629.

While you're in this fan-out, one small heads-up you might want to fold in, since it sits a little below your changes in the same intercept. There's an adjacent one, #67600 (P2): the default sidebar comes back empty on remote + OAuth-gated backends. It's a different axis from #70629 — not which profile a slice targets, but how the batched slice authenticates.

On current main (a61183b56), the two batched-slice dispatches still go out through the loopback-token path:

main.ts:9229  return fetchJson(`${primary.baseUrl}/api/profiles/sessions?${searchParams}`, primary.token, { … })
main.ts:9249  const base = (await fetchJson(`${primary.baseUrl}/api/profiles/sessions?${searchParams}`, primary.token, { … })

fetchJson sends X-Hermes-Session-Token (main.ts:3824), while the per-profile branches just above use the OAuth-aware fetchJsonForProfile (9165/9183/9204). With an OAuth-gated primary + per-profile remote overrides, the batched default slice gets a 401 there, and since the resolver is meant to always return data, it resolves to an empty slice — which is #67600's empty default.

Since this PR reworks the slice params but (reasonably) leaves those dispatch lines alone, that case would still reproduce after it merges. If it's helpful, the fix is small and complements what you have: send the batched slices through fetchJsonForProfile(null, …) like the sibling paths, and let a transport error surface (e.g. into an errors[]) instead of collapsing to an empty slice, so a 401 doesn't read as "no sessions". Glad to push that onto this branch or as a quick follow-up — full root-cause writeup is on #67600.

One clarification on that swap, since the natural concern would be regressing token-auth setups: fetchJsonForProfile isn't OAuth-only — it dispatches on the connection's detected auth mode. requestJsonForProfile (main.ts:7096-7114 on a61183b56) resolves the profile's backend and only takes the bearer/cookie route when conn.authMode === 'oauth' (as probed from the public /api/statusauth_required); otherwise it falls through to the exact same fetchJson(url, conn.token) call these two sites make today. So loopback/--insecure/token setups keep byte-identical behavior — the change only has an effect when the primary is OAuth-gated, which is precisely the case that 401s today. It's also already the transport the sibling per-profile branches in this same intercept use, so this would just align the last two call sites with the path the rest of the fan-out relies on. FWIW, I've been running this exact change locally since Jul 22 on the affected setup (remote + OAuth-gated primary + per-profile overrides): the empty default sidebar is fixed, and token-authenticated flows elsewhere are unaffected.

Thanks again for consolidating all of this.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for consolidating the selected-profile sidebar boundary. The underlying bug is still present on current main: hermes_cli/web_routers/profiles.py:290-303 scopes recents but appends cron and Messaging rows for every profile, while use-session-list-actions.ts:84-121 also refreshes/pages Messaging with profile='all'.

Problems

  • The backend hunk targets the pre-extraction hermes_cli/web_server.py; current main moved this route to hermes_cli/web_routers/profiles.py:195 in 27b1377b4c5284d0fc16ed5df4c27c507b86559d. The scope condition needs to be applied at the current loop.
  • Current main already has apps/desktop/src/app/chat/sidebar/session-index.ts:17 for pin resolution. Adding session-pin-index.ts would duplicate the same ID/lineage indexing policy.

Suggested changes

  • Salvage the scope boundary across the current router, compatibility fallback, and Electron slice fan-out.
  • Extend the existing session index rather than adding a parallel helper.
  • Preserve the current OAuth-aware primary routing introduced by 704a32187030146d83fcfa323c62bcd60c32e126, which addresses the adjacent concern raised in the PR discussion.

Automated hermes-sweeper review.

import { normalizeProfileKey } from '@/store/profile'

export function buildPinnedSessionIndex(
profileScope: string,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main already has session-index.ts:17 for the same live-ID and lineage-root pin resolution policy. During salvage, extend that helper or pass it scoped arrays rather than adding a second index implementation that can drift.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@ctaylor86

Copy link
Copy Markdown
Contributor

Fresh confirmation against current main 75901a295dc44f359f2b6336e03cd3eedbcf9d4c: the concrete-profile Desktop boundary is still inconsistent. Messaging platform sections remain cross-profile because the independent refresh/paging calls still request profile=all, the batched router still aggregates Messaging across every profile, and pinned-session state is not switched with the selected profile. The visible result is that switching between two concrete profiles can retain both the prior profile's pins and platform folders such as Telegram/API/Home Assistant even when the selected profile has none.\n\nThis PR remains the right ownership anchor because it covers both observed symptoms and the full sidebar boundary rather than only the Messaging fetch. The current-main salvage should follow the existing review guidance: move the router change to hermes_cli/web_routers/profiles.py, reuse the existing session-index.ts pin/lineage policy, and preserve the OAuth-aware Electron routing now on main. I found the same current integration points while preparing a local fix and am not opening a competing PR. Happy to provide fresh verification on a rebased head.

@TheAngryPit

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #83052, which merged the later integrated sidebar/profile implementation and explicitly closed #70629.

#83052 carries the final implementation and authorship from @OutThisLife. It scopes cron and Messaging to the selected concrete profile or explicit all scope while extending the cross-profile sidebar, grouping, filtering, and project-tree behavior.

This PR remains useful as prior diagnosis and regression material, but it is no longer the merge vehicle. Thanks to everyone who reviewed and consolidated the work.

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

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

[Bug]: Concrete Desktop profiles mix Messaging sessions and can appear empty after switching

5 participants