Skip to content

fix(desktop): authenticate primary-profile session slices in OAuth mode - #70576

Closed
nrmjeremy wants to merge 1 commit into
NousResearch:mainfrom
nrmjeremy:fix/desktop-oauth-primary-session-slices
Closed

fix(desktop): authenticate primary-profile session slices in OAuth mode#70576
nrmjeremy wants to merge 1 commit into
NousResearch:mainfrom
nrmjeremy:fix/desktop-oauth-primary-session-slices

Conversation

@nrmjeremy

@nrmjeremy nrmjeremy commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Bug Description

In remote OAuth mode, the desktop sidebar's default/primary profile session list renders empty while per-profile-override tabs populate correctly. Chat in the default tab still works (WS uses a minted ticket) — only the list is blank.

Supersedes #68169 (same fix, stale after the RFC 8252 native-login refactor; its branch was deleted).

Root Cause

OAuth-mode primary connections carry token: null by design — REST is authenticated either by a native RFC 8252 bearer token or through the cookie-bound Electron session partition (fetchJsonViaOauthSession). The two remote-splice helpers that fetch the primary profile's session-list slices — fetchProfilesSessionSlice's local-primary branch and mergeRemoteProfileSessions' base aggregate — called plain fetchJson(url, primary.token), so every sidebar refresh for the primary profile got a 401 no_cookie that the .catch(() => ({ sessions: [] })) then swallowed silently. Override profiles work because they route through the auth-aware requestJsonForProfile.

Fix

  • Add fetchPrimaryGetJson(primary, url), mirroring requestJsonForProfile: for OAuth primaries, prefer the native access token (bearer) when held, else fall back to fetchJsonViaOauthSession; non-OAuth primaries keep plain fetchJson(url, token).
  • Route both helpers through it.

How to Verify

  1. Build the desktop app, then confirm the helper is wired into the packaged bundle: grep -c fetchPrimaryGetJson apps/desktop/release/mac-arm64/Hermes.app/Contents/Resources/app.asar.unpacked/dist/electron-main.mjs → 3 (definition + 2 call sites)
  2. Connect a desktop client in remote OAuth mode with per-profile overrides → the default profile's session list populates alongside the override tabs
  3. npm run typecheck (all three tsconfigs) and npx vitest run --project electron

Test Plan

  • Added regression test for this bug (the electron suite has no coverage of these helpers today)
  • Existing tests still pass — 697 electron vitest tests green
  • Manual verification of the fix — pre-refactor version of this change verified live on a remote OAuth setup (basic_auth dashboards, 800+ primary-profile sessions populating, 2026-07-21); this port verified via typecheck + full electron suite + packaged-bundle inspection (2026-07-23)

Risk Assessment

Low — two call sites changed; non-OAuth behavior is byte-identical to before, and the OAuth path mirrors the existing requestJsonForProfile pattern already used everywhere else in main.ts.

In OAuth remote mode the primary backend connection carries token: null —
REST is authenticated either by a native RFC 8252 bearer token or through
the cookie-bound Electron session partition. The two remote-splice helpers
that fetch the primary profile's session-list slices
(fetchProfilesSessionSlice's local-primary branch and
mergeRemoteProfileSessions' base aggregate) called plain
fetchJson(url, primary.token), so every sidebar refresh for the primary
profile got a 401 no_cookie that the .catch(() => ({ sessions: [] }))
swallowed silently. Result: the default/primary profile's session list
rendered empty in the desktop sidebar while per-profile-override tabs
(which route through the auth-aware requestJsonForProfile) worked.

Route both helpers through a new fetchPrimaryGetJson() that mirrors
requestJsonForProfile: for OAuth primaries prefer the native access token
(bearer) when held, else fall back to fetchJsonViaOauthSession; non-OAuth
primaries keep plain fetchJson(url, token).

Supersedes NousResearch#68169 (stale after the RFC 8252 native-login refactor; the
original branch was deleted upstream).

Verified: typecheck clean (all three tsconfigs); 697 electron vitest
tests pass; helper confirmed wired into the packaged bundle
(app.asar.unpacked/dist/electron-main.mjs). The pre-refactor version of
this fix was verified live on a remote OAuth setup on 2026-07-21 (800+
primary-profile sessions populating).
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) area/auth Authentication, OAuth, credential pools area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #67578: both repair the same OAuth-primary session-list calls that otherwise turn a null-token 401 into empty sidebar rows. #67578 is the active tested current-path implementation.

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs address the OAuth-mode primary-profile empty-session-list bug: both replace a plain fetchJson(url, primary.token) call — which 401s and is silently swallowed for OAuth primaries — with an auth-aware fetch for the two session-slice helpers. #68169 (closed, +15/-9) inlines a ternary between fetchJsonViaOauthSession and fetchJson but has no native RFC 8252 bearer-token branch. #70576 (open, +25/-8) adds a dedicated fetchPrimaryGetJson helper that prefers a held native bearer token before falling back to the cookie-authed session, restoring that missing branch.

Related pull requests

  • #68169 [closed] duplicate — (+15/-9) — superseded: routes fetchProfilesSessionSlice and mergeRemoteProfileSessions through fetchJsonViaOauthSession for OAuth primaries, but the inline ternary has no native-bearer branch, so RFC 8252 native-login users (no cookie in the OAuth partition) still hit a 401 no_cookie and blank sidebar; contributor confirmed superseded by #67578, which uses the existing fetchJsonForProfile pattern including the native-bearer branch.
  • #70576 related — (+25/-8) — open, closes the gap that got #68169 superseded: adds fetchPrimaryGetJson(primary, url), which for OAuth primaries prefers a held native access token (bearer) via ensureNativeAccessToken and only falls back to fetchJsonViaOauthSession otherwise, then routes both session-slice helpers through it — restoring exactly the native-bearer branch #68169 lacked. A contributor comment on this PR also labels it duplicate of #67578; however the diff independently reconstructs the auth-aware pattern with the native-bearer branch rather than referencing #67578, so redundancy should be confirmed against #67578's current diff before closing (see consolidation).

Duplicates

#68169 and #70576 implement the same underlying fix (auth-aware GET for the primary-profile session slices) — #68169 is a strict subset of #70576, missing the native-bearer branch. Both are also flagged by a contributor as duplicating the out-of-complex #67578.

Suggested consolidation

#68169 is already closed and confirmed superseded by #67578 (OutThisLife, 2385 commits: #67578 keeps the native-bearer branch that #68169's inline ternary drops) — no further action needed there. For #70576: despite alt-glitch's comment also flagging it as duplicate of #67578, the diff shows fetchPrimaryGetJson explicitly restores the native-bearer-first logic that was the stated reason #68169 lost to #67578, so it is not clearly redundant on the same grounds cited against #68169. Recommend diffing #70576 against #67578's current state before deciding: if #67578 already contains equivalent native-bearer handling, close #70576 as duplicate of #67578; otherwise merge #70576.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup68169 ["PRs duplicating each other"]
        P68169["PR #68169 (closed)"]
        P70576["PR #70576 (open)"]
    end
    class P68169 closed
    class P70576 open
    class P70576 target
    click P68169 "https://github.com/NousResearch/hermes-agent/pull/68169"
    click P70576 "https://github.com/NousResearch/hermes-agent/pull/70576"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 5 kB of issue/PR text, 1 kB of discussion (3 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused OAuth-session fix. This is an automated hermes-sweeper review; the exact behavior is already on current main via the consolidated remote-routing work, which preserves contributor credit including @nrmjeremy.

  • apps/desktop/electron/main.ts:10035 and :10050 route both formerly direct primary session reads through fetchPrimaryProfileSessions(..., fetchJsonForProfile).
  • apps/desktop/electron/main.ts:7567-7579 supplies the required OAuth behavior: native RFC 8252 bearer first, then the cookie-bound OAuth session.
  • apps/desktop/electron/profile-session-routing.ts:10-18 preserves the empty-list fallback, with regression coverage in apps/desktop/electron/profile-session-routing.test.ts:7-29.
  • The implementation landed in merged PR fix(desktop): repair remote profile routing, sessions, and pool lifecycle #72835 as commit 704a32187030146d83fcfa323c62bcd60c32e126 (merge 5fde131eb2a6683a3879497e69918feae33224d0).

@teknium1 teknium1 closed this Jul 30, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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.

4 participants