Skip to content

fix: detect OAuth token expiry and show actionable error message#371

Open
YunshuGao wants to merge 1 commit into
YishenTu:mainfrom
YunshuGao:fix/oauth-token-expired-error
Open

fix: detect OAuth token expiry and show actionable error message#371
YunshuGao wants to merge 1 commit into
YishenTu:mainfrom
YunshuGao:fix/oauth-token-expired-error

Conversation

@YunshuGao

Copy link
Copy Markdown

Summary

  • Add isAuthenticationError() detection for OAuth token expiry, invalid API key, and 401 authentication errors
  • Catch auth errors early in ClaudianService.ts before session-retry logic (retrying with an expired token is pointless)
  • Show a clear, actionable error message: "run claude auth login to re-authenticate" instead of raw API JSON
  • Fire an Obsidian Notice toast (10s) so the error is visible to users

Problem

When the Claude API returns a 401 authentication_error (e.g. "OAuth token has expired"), the plugin doesn't recognise it as a specific error type. It falls through to generic error handling, showing a raw JSON API error like:

Error: authentication_failedFailed to authenticate. API Error: 401
{"type":"error","error":{"type":"authentication_error","message":"OAuth token has expired. Please obtain a new token or refresh your existing token."}}

This is confusing and gives users no guidance on how to fix it.

Root Cause

isSessionExpiredError() in session.ts only matches patterns like "session expired" / "session not found". OAuth/authentication errors don't match any of those patterns.

Test plan

  • Simulate an expired OAuth token and verify the new error message appears
  • Verify the Obsidian Notice toast is shown
  • Verify that session expiry recovery still works as before
  • Verify normal query flow is unaffected

Previously, when the Claude API returned a 401 authentication_error
(e.g. "OAuth token has expired"), the plugin did not recognise it as
a specific error type. It fell through to generic error handling,
showing a raw JSON API error that was confusing and unhelpful.

Changes:
- Add isAuthenticationError() in session.ts with patterns for OAuth
  expiry, invalid API key, and 401 authentication errors
- Catch auth errors early in ClaudianService.ts before session-retry
  logic (retrying with an expired token is pointless)
- Show clear message: "run claude auth login to re-authenticate"
- Fire an Obsidian Notice toast so the error is visible

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@YishenTu YishenTu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Code Review

The approach is sound — intercepting authentication errors before session-retry logic makes sense since retrying with expired credentials is pointless. The detection function follows the same pattern as isSessionExpiredError, which is good. A few issues to address:

Must fix

  1. No testsisAuthenticationError() has zero unit tests despite the project's TDD mandate and an existing test suite for the sister function isSessionExpiredError in tests/unit/utils/session.test.ts.

  2. Overly broad compound pattern{ includes: ['token', 'expired'] } in session.ts overlaps with session-expired patterns. An error like "session token has expired" would be caught as a non-recoverable auth error instead of a recoverable session error, since isAuthenticationError is checked first at every call site. Should be narrowed (e.g., require 'oauth' too) or removed since 'oauth token has expired' already matches the simple pattern list.

Should fix

  1. 6× duplicated error message strings — The long error message and Notice text are copy-pasted across ClaudianService.ts. Extract to constants to prevent drift and simplify future wording changes.

  2. Inconsistent closePersistentQuery — The persistent query path calls closePersistentQuery('authentication error') on auth errors, but the crash recovery path in startPersistentQueryConsumer doesn't. After a failed ensureReady() with auth error, the persistent query may be in a half-initialized state.

Nit

  1. String handling inconsistencyisAuthenticationError handles typeof error === 'string' but the adjacent isSessionExpiredError only handles Error instances. Worth aligning or documenting why they differ.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants