fix: continue OAuth metadata discovery on 5xx responses #1632
Claude / Claude Code Review
completed
Mar 27, 2026 in 9m 57s
Code review found 2 potential issues
Found 4 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/client/src/client/auth.ts:1004-1005 |
Stale JSDoc on deprecated discoverOAuthMetadata |
| 🟣 Pre-existing | packages/client/src/client/auth.ts:1031-1035 |
Response body not consumed before fallback in discoverMetadataWithFallback |
Annotations
Check warning on line 1005 in packages/client/src/client/auth.ts
claude / Claude Code Review
Stale JSDoc on deprecated discoverOAuthMetadata
The JSDoc for the deprecated `discoverOAuthMetadata()` (lines 1044-1045) says "Any other errors will be thrown as exceptions," but after broadening `shouldAttemptFallback` to `!response.ok`, a 5xx on a path-aware URL now triggers root fallback — and if the root returns 404, the function silently returns `undefined` instead of throwing. The doc should note that non-OK responses on path-aware URLs may trigger fallback before throwing.
Check notice on line 1035 in packages/client/src/client/auth.ts
claude / Claude Code Review
Response body not consumed before fallback in discoverMetadataWithFallback
Nit: In `discoverMetadataWithFallback()`, when the fallback path is taken (line 1033-1035), the first response is overwritten without consuming its body. Compare with `discoverAuthorizationServerMetadata()` (line 1199) which correctly calls `response.text?.().catch(() => {})` before continuing. This is a pre-existing issue (same pattern existed for 4xx before this PR), but adding `await response?.text?.().catch(() => {})` before the fallback assignment would be a trivial consistency fix.
Loading