oauth: return error instead of (nil, nil) from getServerMetadata (#903)

[pjdurden]

Fixes #903. getServerMetadata returned (nil, nil) on a non-2xx discovery response, so callers (RegisterClient, token/auth URL builders) dereferenced a nil *AuthServerMetadata and panicked. It now returns an explicit error instead; callers already handle the error path. Adds regression tests covering getServerMetadata and RegisterClient.

Summary by CodeRabbit

• Bug Fixes
  • OAuth authorization handler now returns a clear error when authorization server metadata is unavailable after discovery, avoiding nil responses and preventing downstream failures.
• Tests
  • Added regression tests that simulate unavailable metadata to ensure metadata retrieval reports errors and client registration does not panic.

[mark-iii-labs-huly]

Connected to Huly®: MCP_G-468

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2ccba960-1db5-47dc-8d52-e1e8409de2bd

📥 Commits

Reviewing files that changed from the base of the PR and between bfde902 and 9624e5b.

📒 Files selected for processing (1)

• client/transport/oauth.go

🚧 Files skipped from review as they are similar to previous changes (1)

• client/transport/oauth.go

---

Walkthrough

getServerMetadata now records and returns a non-nil error when discovery finishes without producing metadata (avoiding a (nil, nil) return). New regression tests exercise a 404 metadata discovery endpoint and assert GetServerMetadata returns an error and RegisterClient does not panic.

Changes

OAuth nil metadata fix

Layer / File(s)	Summary
Nil metadata check in getServerMetadata client/transport/oauth.go	getServerMetadata now detects when h.serverMetadata remains nil after the discovery once and sets/returns a non-nil metadataFetchErr (includes discovery URL when configured) to avoid returning (nil, nil).
Regression tests for unavailable metadata client/transport/oauth_nil_metadata_test.go	Adds a test helper and two tests that use an httptest server returning 404 to verify GetServerMetadata returns an error and RegisterClient does not panic and returns an error.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• mark3labs/mcp-go#801: Also modifies OAuthHandler.getServerMetadata discovery/fallback handling to prevent returning (nil, nil) by adding explicit error detection when metadata is unavailable.
• mark3labs/mcp-go#875: Refactors OAuthHandler.getServerMetadata handling of non-2xx discovery results and no-op fetches that can produce nil metadata.
• mark3labs/mcp-go#669: Changes the metadata discovery/fallback ordering and intersects with the same discovery/error handling code paths.

Suggested labels

type: bug, priority: high

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: fixing getServerMetadata to return an error instead of (nil, nil) when metadata discovery fails.
Description check	✅ Passed	The description covers the bug fix and mentions test additions, but lacks a complete structured template with explicit sections like Type of Change and Checklist.
Linked Issues check	✅ Passed	Code changes directly address issue #903 by making getServerMetadata return an explicit error on failed discovery, preventing nil pointer dereferences in callers like RegisterClient.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the OAuth metadata discovery issue: error handling improvements in oauth.go and new regression tests in oauth_nil_metadata_test.go.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes a panic path in OAuth metadata discovery by ensuring callers receive an explicit error when discovery yields no metadata, and adds regression tests to prevent returning (nil, nil).

Changes:

• Add regression tests covering unavailable metadata discovery and ensuring RegisterClient doesn’t panic.
• Update getServerMetadata to return a non-nil error when serverMetadata is still nil.

Reviewed changes

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

File	Description
client/transport/oauth_nil_metadata_test.go	Adds regression tests for the “nil metadata + nil error” discovery failure mode.
client/transport/oauth.go	Surfaces an explicit error when metadata discovery returns no metadata to prevent nil deref panics.

---

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