Skip to content

fix(desktop): classify OAuth ticket mint failures - #58556

Closed
33hodl wants to merge 1 commit into
NousResearch:mainfrom
33hodl:fix/desktop-oauth-ticket-errors
Closed

fix(desktop): classify OAuth ticket mint failures#58556
33hodl wants to merge 1 commit into
NousResearch:mainfrom
33hodl:fix/desktop-oauth-ticket-errors

Conversation

@33hodl

@33hodl 33hodl commented Jul 4, 2026

Copy link
Copy Markdown

Summary

  • classify OAuth WebSocket ticket mint failures by status/failure type
  • keep true 401/403 auth failures as sign-in-required
  • stop labeling timeout/5xx gateway startup failures as expired sessions
  • share the classification between the boot path and the Test Remote WS path

Validation

  • node --check electron/connection-config.cjs
  • node --check electron/main.cjs
  • node --test electron/connection-config.test.cjs
  • npm run lint -- --quiet electron/connection-config.cjs electron/connection-config.test.cjs electron/main.cjs
  • npm run test:desktop:platforms

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have labels Jul 4, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying that the boot and Test Remote paths currently present the same generic reauthentication outcome.

Problems

  • The proposed timeout/5xx behavior conflicts with the current Desktop auth invariant: apps/desktop/AGENTS.md:127-130 says an OAuth WebSocket-ticket mint failure means reauthentication. The PR changes both call sites to set needsOauthLogin=false for those failures, so this needs an explicit design decision before salvage.
  • The patch targets deleted CJS files. Current main moved the implementation to apps/desktop/electron/connection-config.ts:118-149 and apps/desktop/electron/main.ts:5990-6034 in 39d09453f95e8aefc0c97e5d9b30ff341cae9ed8.

Suggested changes

  • If maintainers adopt the distinction, port the shared classifier and tests to the TypeScript targets and retain both current call sites.
  • Prefer the real error.statusCode populated by apps/desktop/electron/main.ts:5281-5285 for HTTP failures.

Automated hermes-sweeper review.

@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 15, 2026
@33hodl

33hodl commented Jul 17, 2026

Copy link
Copy Markdown
Author

Closing this superseded OAuth-classification PR in favor of the consolidated, rebased implementation in #64673: #64673. The consolidated PR includes the 401/403-only auth classification, bounded transport retry, fresh ticket resolution, session recovery, and the updated Desktop guidance.

@33hodl 33hodl closed this Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants