Skip to content

fix(desktop): restore native OAuth tokens after restart - #71524

Merged
austinpickett merged 8 commits into
NousResearch:mainfrom
Doud-FR:fix/desktop-native-oauth-persistence
Jul 31, 2026
Merged

fix(desktop): restore native OAuth tokens after restart#71524
austinpickett merged 8 commits into
NousResearch:mainfrom
Doud-FR:fix/desktop-native-oauth-persistence

Conversation

@Doud-FR

@Doud-FR Doud-FR commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Restore encrypted native OAuth tokens correctly after a desktop restart.
  • Keep gateway token response parsing separate from persisted token parsing.
  • Log stored-token loading failures instead of silently discarding the error.
  • Add regression tests for the encrypted on-disk camelCase token shape.

Root cause

The desktop persists an already-normalized NativeTokenSet using camelCase fields:

  • accessToken
  • refreshToken
  • expiresAt
  • userId

On restart, the stored object was passed to parseTokenResponse(), which expects the gateway response format using snake_case fields such as access_token.

This caused parseTokenResponse() to throw Gateway token response missing access_token. The exception was silently caught, _loadNativeTokens() returned null, and the desktop reported that the user was not signed in after every restart.

Fix

Add a dedicated parseStoredTokenSet() parser for the normalized camelCase storage format while keeping parseTokenResponse() unchanged for gateway responses.

Validation

  • npm --prefix apps/desktop run test:desktop:platforms -- electron/native-oauth.test.ts
    • 18 tests passed
  • npm exec -- tsc -p tsconfig.electron.json --noEmit
  • npm run build
  • npm run builder -- --dir
  • Manually validated on Windows:
    1. Sign in to a remote OAuth-protected Hermes gateway.
    2. Completely close Hermes Desktop.
    3. Relaunch the application.
    4. The remote session is restored automatically without signing in again.

Copilot AI review requested due to automatic review settings July 25, 2026 18:57

Copilot AI 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.

Pull request overview

This PR fixes Hermes Desktop “signed out after restart” behavior by correctly parsing the encrypted, on-disk native OAuth token set (camelCase NativeTokenSet) instead of treating it like a gateway token response (snake_case), while preserving the existing gateway-response parsing path.

Changes:

  • Added parseStoredTokenSet() to validate/normalize the persisted camelCase token shape without changing parseTokenResponse().
  • Switched desktop token restore (_loadNativeTokens) to use parseStoredTokenSet() and log failures instead of silently discarding them.
  • Added regression tests covering the encrypted on-disk camelCase token format and rejecting the gateway snake_case shape.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
apps/desktop/electron/native-oauth.ts Adds a dedicated parser for persisted (camelCase) token sets to avoid mis-parsing stored tokens as gateway responses.
apps/desktop/electron/native-oauth.test.ts Adds regression tests for parseStoredTokenSet() and its expected/invalid input shapes.
apps/desktop/electron/main.ts Restores tokens via parseStoredTokenSet() and logs load failures for easier diagnosis.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 25, 2026
@notwitcheer

Copy link
Copy Markdown
Contributor

first-party corroboration for this, from a Hermes Cloud + Desktop pairing on macOS.

symptom matches exactly: connection mode set to Hermes Cloud, still signed in, and every fresh start (after an update, after quit and reopen, after a machine restart) fails to reach the cloud agent. the only recovery is signing out of Hermes Cloud and signing back in, and it is needed every single time. the connection default applies across profiles, so all of them lose the backend at once.

what the desktop log shows, in order: the cached remote backend fails its liveness probe and gets dropped, then a long run of "Could not reach the remote Hermes gateway while refreshing its WebSocket ticket", then the error flips once to "your remote gateway session has expired" and after that permanently to the OAuth not-signed-in state. it never re-probes once the gateway is reachable again.

one detail that supports the logging change here as much as the parser change: at the moment the state flips, the log carries no cause. _loadNativeTokens() catching bare and returning null is exactly why. whatever else lands, replacing that silent catch with a logged failure turns this class of report from "it says I am signed out" into something diagnosable from a log alone.

caveat on my own evidence: the capture window starts 2026-07-17, so it predates #68250 and the early phases in it are that PR's territory, not this one. the terminal state is the part that matches what you describe. I have not verified the fix, only the symptom and the swallowed-error path.

@Doud-FR

Doud-FR commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@notwitcheer Thanks a lot for sharing this — that is very useful corroboration.

Your macOS symptoms appear to match the same Desktop-side failure I observed on Windows: the encrypted native OAuth token set exists on disk, but is not restored correctly after a fresh application start.

One important clarification: this Desktop PR is one half of the complete fix I validated in my self-hosted environment.

The Desktop PR can still be tested independently on macOS and should fix the immediate token-restoration problem. However, reliable long-term token renewal may also depend on the server-side fix, or on equivalent protection already being deployed by Hermes Cloud.

Since you are using Hermes Cloud, you would only need to build the Desktop branch locally; the server-side deployment would need to be handled by the Nous Research maintainers.

I have only built and validated the packaged application on Windows, but the modified Electron/TypeScript code is cross-platform. A macOS test would therefore be extremely valuable.

The most useful scenarios would be:

  • quit and reopen the Desktop app;
  • restart the Mac;
  • build the macOS Desktop app from this PR branch, then quit and relaunch it;
  • leave the session running long enough to require an OAuth token refresh;
  • temporarily lose access to the gateway, then restore connectivity.

The expected result is that the existing session is restored and renewed without requiring a manual sign-out/sign-in.

I would be happy to help you inspect the macOS logs or walk through building the branch. Please make sure that any shared logs do not contain access or refresh token values.

@Doud-FR

Doud-FR commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@notwitcheer

I have now independently built and tested this PR on macOS 15.7.4 using an Intel x86_64 Hackintosh.

The result was successful:

  • all 18 native OAuth tests passed;
  • the macOS Desktop app built successfully from this PR branch;
  • the existing OIDC session survived a complete application quit and relaunch;
  • the existing OIDC session also survived a full macOS reboot;
  • the gateway reconnected automatically and all chats were immediately available;
  • no manual sign-out/sign-in or new browser authentication was required.

I tested it against my self-hosted gateway, which also has the companion server-side PR #71548 applied. Therefore, this validates the complete Desktop + Gateway fix in my environment.

You can build the Desktop PR directly on your Mac with the following process:

# Install Hermes first, if it is not already installed
curl -fsSL https://hermes-agent.nousresearch.com/install.sh |
  bash -s -- --skip-setup --skip-browser

source ~/.zshrc
cd ~/.hermes/hermes-agent

# Fetch and switch to this PR
git fetch --no-tags origin pull/71524/head:pr-71524
git switch pr-71524

# Install dependencies and run the OAuth tests
npm install
cd apps/desktop
npx vitest run --project electron electron/native-oauth.test.ts

# Build the macOS application from this PR
cd ~/.hermes/hermes-agent
CSC_IDENTITY_AUTO_DISCOVERY=false \
  hermes desktop --build-only --force-build

# Launch the locally built application
./apps/desktop/release/mac/Hermes.app/Contents/MacOS/Hermes

The build is unsigned, so macOS may display a Gatekeeper warning depending on your local security settings.

The important test scenarios are:

  • build the macOS Desktop app from this PR branch, then quit and relaunch it;
  • fully restart the Mac and relaunch the same build;
  • verify that the remote gateway reconnects without a new OAuth login;
  • leave the session running long enough to exercise token renewal.

Since you use Hermes Cloud, the server-side portion would need to be deployed by the Nous Research maintainers, or they would need to confirm that equivalent refresh-request protection is already present there.

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

apps/desktop/electron/main.ts:5952

  • The new stored-token load failure log assumes the caught value is an Error ((error as Error).message). In JS/TS, throw can be any value; if it’s a string/object, .message will be undefined and the log loses the useful details. Prefer the same defensive formatting used elsewhere in this file (e.g. err?.message || err).
  } catch (error) {
    rememberLog(
      `[native-oauth] failed to load stored tokens for ${baseUrl}: ${(error as Error).message}`
    )

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

apps/desktop/electron/main.ts:5945

  • When decryptDesktopSecret(secret) fails (returns an empty string), _loadNativeTokens currently returns null without logging or clearing the corrupt entry. This makes token-load failures harder to diagnose and can cause the same unreadable blob to be retried on every startup. Consider logging this case and deleting the stored blob so the user can re-auth cleanly without repeated silent failures.
    if (!plaintext) {
      return null
    }

    const tokens = parseStoredTokenSet(JSON.parse(plaintext))

Copilot AI 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.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the storage-vs-gateway response boundary; the current-main defect is real. apps/desktop/electron/main.ts:6187 persists NativeTokenSet via JSON.stringify(tokens), but _loadNativeTokens() passes that decrypted camelCase object to the snake_case-only parseTokenResponse() at apps/desktop/electron/main.ts:6222. parseTokenResponse() requires access_token at apps/desktop/electron/native-oauth.ts:178-193, so the existing catch at main.ts:6226-6228 turns every restart into an apparent signed-out state.

Problems

  • The new parser tests cover the pure parser boundary, but not the actual encrypt/store/decrypt/load route where the incorrect call occurs (apps/desktop/electron/main.ts:6181-6228). The Desktop guide asks seam changes to exercise the real path (apps/desktop/AGENTS.md:181-188).

Suggested changes

  • Add a persistence-path regression test, preferably through an injected helper, proving a freshly loaded encrypted camelCase token set restores correctly.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state label Jul 30, 2026
@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
@Doud-FR
Doud-FR force-pushed the fix/desktop-native-oauth-persistence branch from 24076cc to d553b6e Compare July 30, 2026 22:50
@Doud-FR
Doud-FR requested a review from Copilot July 30, 2026 22:50
@Doud-FR

Doud-FR commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@teknium1 Thanks for the review — addressed in d553b6eb0.

I added a small Electron-free persistence seam that is now used by the real production path in main.ts.

The new regression exercises the complete sequence:

  • normalized camelCase NativeTokenSet
  • JSON serialization
  • injected encryption and store write
  • simulated fresh process using only the stored file bytes
  • store read and decryption
  • JSON parsing through parseStoredTokenSet()

The test explicitly proves that the stored camelCase payload is rejected by the old snake_case-only parseTokenResponse() boundary while the actual persistence path restores it correctly.

Additional coverage verifies:

  • access and refresh tokens survive restart;
  • expiry, provider, and user ID are preserved;
  • the loaded value is reconstructed rather than returned from memory;
  • tokens remain encrypted at rest;
  • per-gateway persistence and clearing remain isolated;
  • missing/corrupt stores and locked-keychain behavior remain unchanged.

Validation completed:

  • persistence regression: 16 passed
  • related Electron OAuth suite: 71 passed
  • Electron TypeScript check: passed
  • E2E TypeScript check: passed
  • ESLint on all three changed files: passed with no warnings
  • git diff --check: passed

The broader Desktop typecheck still encounters unrelated pre-existing renderer dependency errors, while the Electron and E2E configurations covering this change both pass.

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

apps/desktop/electron/native-token-store.ts:83

  • persistNativeTokenSet() logs write failures using (error as Error).message, which becomes undefined for non-Error throws. This can hide the real failure reason; mirror the defensive formatting already used in loadNativeTokenSet().
  try {
    io.writeStoreText(JSON.stringify(store))
  } catch (error) {
    io.rememberLog?.(`[native-oauth] failed to persist tokens: ${(error as Error).message}`)
  }

apps/desktop/electron/native-token-store.ts:60

  • readStore() treats any parsed "object" (including arrays) as a valid store. If the file ever contains "[]" (or another non-record object), writes will silently fail because JSON.stringify on an array drops non-index properties, so tokens won't persist even though no error is thrown. Guard against arrays so the store is always a plain key/value map.

This issue also appears on line 79 of the same file.

function readStore(io: NativeTokenStoreIo): Record<string, any> {
  try {
    const parsed = JSON.parse(io.readStoreText())

    return parsed && typeof parsed === 'object' ? parsed : {}
  } catch {

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

apps/desktop/electron/native-token-store.ts:82

  • io.encrypt() is typed to return StoredTokenSecret | null, but the result is assigned to store[baseUrl] without checking. If an injected encrypt implementation returns null (rather than throwing), this will overwrite any previously-stored secret with null, and subsequent launches will treat the user as signed out with no way to recover the prior refresh token.
    // Encrypt the whole set as one blob so the refresh token never lands in
    // plaintext on disk. Deliberately outside the try below: an unusable
    // keychain is an authoritative write failure and must surface to the
    // caller, not be logged away as if the tokens were saved.
    store[baseUrl] = io.encrypt(JSON.stringify(tokens))

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

apps/desktop/electron/native-token-store.ts:134

  • The new failure logs include the full baseUrl. normalizeRemoteBaseUrl() does not strip URL userinfo (username/password), so a user-entered URL like https://user:pass@host would get those credentials echoed into logs when decryption/loading fails. Please redact userinfo before logging the URL (and keep the unredacted value for the store key).
    if (!plaintext) {
      // A keychain that is merely locked/unavailable right now must not cost
      // the user their refresh token — leave the entry for the next attempt.
      io.rememberLog?.(`[native-oauth] failed to decrypt stored tokens for ${baseUrl}; keeping stored entry for retry`)

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

Confirmed the reload-parser bug on current main: parseStoredTokenSet is absent and _loadNativeTokens() feeds the decrypted camelCase set to the snake_case parseTokenResponse(), which throws and gets swallowed into a signed-out state.

The branch is 50 commits behind, so the tip-to-tip diff makes streamThrottle/battery look reverted. It isn't. A 3-way merge onto current main is clean and both features survive alongside the fix.

Verified on the merged tree:

  • Electron typecheck passes (tsconfig.electron.json).
  • 43/43 targeted tests pass (native-token-store 24 + native-oauth 19), including the encrypt/store/decrypt/load round trip through the injected helper.

The persisted camelCase vs gateway snake_case split is the right boundary, and keeping the store module electron-free matches how native-oauth.ts is already structured.

@austinpickett
austinpickett merged commit eb08467 into NousResearch:main Jul 31, 2026
34 checks passed
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 comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists 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-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.

6 participants