Skip to content

fix: TryLoginAsync reported success inverted - #40

Merged
binn merged 1 commit into
mainfrom
fix/trylogin-inverted-result
Sep 12, 2026
Merged

binn merged 1 commit into
mainfrom
fix/trylogin-inverted-result

Conversation

@binn

@binn binn commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

The bug

TryLoginAsync passes user == null as the success argument of VRChatLoginResult:

return new VRChatLoginResult(user == null, null);
//                           ^^^^^^^^^^^^ this is `bool success`

So the result is inverted on both paths that don't throw:

Outcome user Reported Success Correct?
Login succeeded non-null false
LoginAsync returned null null true
LoginAsync threw false

A successful login reports failure with a null Exception. That's indistinguishable from a silent failure, so the natural conclusion is bad credentials — which is how I found it, after a while of being certain my credentials were wrong.

VRChatLoginResult's own documentation is unambiguous about the intent:

success: A value indicating whether the login operation was successful. Set to true if the login succeeded; otherwise, false.

The fix

user != null for success, plus a real exception on the failure path.

The second part is worth a note: LoginAsync returns null when the final GetCurrentUser response isn't 200 OK, and discards the status. Success = false, Exception = null left a caller with nothing to act on, log, or report to a user. It now carries an UnauthorizedAccessException so the failure is at least nameable.

Notes

  • Only wrapper/VRChat.API/Client/VRChat.cs is touched — no generated code under src/.
  • Behaviour change is intentional and is the point of the PR: anyone who worked around the inversion by testing !result.Success will need to drop the negation. Given the current behaviour makes TryLoginAsync unusable as documented, I'd expect most callers to be using LoginAsync directly instead.
  • Builds clean against src/VRChat.API/VRChat.API.csproj in Release.

Not fixed here, but related

LoginAsync swallowing the status makes 401, 403, 429 and a Cloudflare WAF block indistinguishable to a caller — they all surface as null. That matters for anything that needs to respond differently to each (re-auth vs back off vs tell the operator). Happy to open a separate PR if you'd like that changed, since it's a larger behavioural question than this one.

🤖 Generated with Claude Code

https://claude.ai/code/session_015DPXtTDGbCQ6GebnuvxV2s

TryLoginAsync passed `user == null` as the `success` argument of
VRChatLoginResult, so the result was inverted on both paths that did not
throw:

  login succeeded (user != null) -> Success = false, Exception = null
  login returned null            -> Success = true,  Exception = null

A successful login therefore reported failure with no exception to
inspect, which is indistinguishable from a silent failure -- the natural
conclusion being bad credentials.

Also replaces the null Exception on the failure path. LoginAsync returns
null when the final GetCurrentUser response is not 200 OK and discards
the status, so `Success = false, Exception = null` gave a caller nothing
to act on or report. It now carries an UnauthorizedAccessException.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015DPXtTDGbCQ6GebnuvxV2s
@binn
binn merged commit 163426a into main Sep 12, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant