fix(oauth): The dynamic client registration should be optional

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .

[jokemanfire]

The dynamic registration can be skipped, and it should be optional.

Motivation and Context

Accoding to rfc8414.

How Has This Been Tested?

No test

Breaking Changes

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

[alexhancock]

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

[jokemanfire]

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional

.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,

[vorporeal]

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

[alexhancock]

I see

[jokemanfire]

#461

[Copilot]

Pull Request Overview

This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.

• Changed registration_endpoint from required String to Option<String>
• Added graceful fallback when dynamic registration is not supported
• Improved logging to use warnings instead of errors for expected fallback scenarios

Reviewed Changes

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

File	Description
examples/servers/src/complex_auth_sse.rs	Wraps registration endpoint in Some() to match new optional type
crates/rmcp/src/transport/auth.rs	Updates struct definition, fallback logic, and error handling for optional registration

Comments suppressed due to low confidence (2)

crates/rmcp/src/transport/auth.rs:398

• Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.

 return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));

crates/rmcp/src/transport/auth.rs:408

• Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.

 Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[jokemanfire]

@4t145 Did you forget submit your review? :）

[4t145]

approved

[alexhancock]

LGTM as well after adjusting commit message to satisfy the linter

[jokemanfire]

LGTM as well after adjusting commit message to satisfy the linter

Change it while merge .