Remove the unused timeout parameter from OAuthClientProvider#3165
Conversation
📚 Documentation preview
|
There was a problem hiding this comment.
LGTM — a mechanical removal of a parameter that was stored but never read, with the migration guide, docs page, and doc-contract tests all updated in the same PR. The one inline note (making jwt_parameters keyword-only in the deprecated RFC7523 shim so stale positional v1 calls fail at construction) is a small hardening nit and worth a follow-up, but does not block this change.
Extended reasoning...
Overview
The PR removes the timeout parameter from OAuthClientProvider, the deprecated RFC7523OAuthClientProvider, and the two client-credentials subclasses' super().__init__ calls, plus the OAuthContext.timeout dataclass field. Docs (oauth-clients.md), the migration guide, and the two test files that asserted the old signature are updated consistently. I confirmed with a repo-wide search that nothing in src/ reads context.timeout (matching the PR's claim that it has been dead since #751), and that the shipped examples never passed timeout= to the provider.
Security risks
Although this touches the OAuth client, it removes a value that was never read, so no auth, token, PKCE, or validation logic changes — the behavioral risk is effectively zero. The only surviving concern is the positional-argument shift in the deprecated RFC7523OAuthClientProvider, already covered by the inline nit: a stale six-positional-arg v1 call now misbinds a float to jwt_parameters and fails later with a less clear error instead of at construction. No previously-working configuration silently changes behavior, and typed callers get a static error.
Level of scrutiny
This is the intentionally-breaking v2 branch, the break is documented in docs/migration.md per the repo's own guidelines, and the diff is small and mechanical. The doc-contract test (test_the_two_remaining_keyword_arguments_have_defaults) pins the exact new signature of OAuthClientProvider.__init__, so a regression here would be caught. Moderate scrutiny was appropriate given the auth module location, and the dead-code claim was verified directly rather than taken on faith.
Other factors
The base OAuthClientProvider is protected from the same positional-shift hazard by accident: the vacated slot is client_metadata_url, which is validated at construction and rejects non-URL values with a ValueError. Existing tests were updated rather than deleted, and the docs page's keyword-argument count now matches the real signature. With the nit recorded inline, nothing here needs a human design decision.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
LGTM — a clean removal of a parameter that was stored but never read, so no behavior changes. Beyond the earlier inline note (still open, but non-blocking as stated there), this pass verified that nothing in src/ reads OAuthContext.timeout and that every in-repo constructor call site — examples/, docs_src/, and the conformance client — passes arguments by keyword and never passed timeout=, so nothing in the repo is affected by the positional shift.
Extended reasoning...
Overview
The PR deletes the dead timeout parameter from OAuthClientProvider.__init__, the deprecated RFC7523OAuthClientProvider, and the OAuthContext dataclass field, updates the two internal super().__init__ calls in client_credentials.py, and adjusts the docs page, the migration guide, and the two tests that referenced the parameter. A grep of src/mcp/client/auth confirms no remaining reader of .timeout — the value was written once and never consulted, so removal is behavior-preserving for keyword callers.
Security risks
This touches OAuth client code, but only by removing an inert field; no token handling, validation, discovery, or crypto logic changes. The one hazard is the positional-argument shift already flagged inline on RFC7523OAuthClientProvider (a stale v1 positional call binds a float to jwt_parameters), which is a diagnostics regression on a deprecated shim, not a security issue — and the base OAuthClientProvider rejects the equivalent misuse at construction via is_valid_client_metadata_url. I checked all in-repo call sites (examples/clients/simple-auth-client, examples/snippets, examples/stories/oauth, docs_src/oauth_clients, .github/actions/conformance/client.py): all use keyword arguments and none passed timeout.
Level of scrutiny
Moderate: it is auth-adjacent, so I verified the removal is genuinely dead code rather than trusting the description. Once that holds, the change is mechanical. This is the intentionally-breaking v2 branch, and the breaking change is documented in docs/migration.md with a concrete before/after and a suggested replacement (anyio.fail_after in the caller's handlers), satisfying the repo's breaking-change policy.
Other factors
Tests were updated coherently: the docs-contract test test_the_two_remaining_keyword_arguments_have_defaults asserts the exact new signature of OAuthClientProvider.__init__, so any accidental signature drift would fail CI, and the stale context.timeout == 300.0 assertion was removed from test_auth.py. The docs page prose was kept in sync with the signature. The prior inline suggestion (make jwt_parameters keyword-only) remains a worthwhile follow-up but was explicitly assessed as non-blocking on a deprecated class.
d149d36 to
1aec562
Compare
There was a problem hiding this comment.
LGTM — this is a mechanical removal of a parameter that was stored but never read, and all prior review feedback has been resolved.
What was reviewed:
- Confirmed
OAuthContext.timeouthad no remaining readers: greppedsrc/mcp/client/auth, tests, docs, and examples — the onlytimeout=hits are unrelated (sse_client, callback server), so runtime behavior is unchanged. - Checked the shipped examples and docs snippets construct the providers keyword-only, so none are affected by the signature change.
- The breaking change is documented in
docs/migration.mdper repo policy, and the docs-claim test (test_the_two_remaining_keyword_arguments_have_defaults) pins the new signature.
The one concern from my earlier pass (positional-arg shift on the deprecated RFC7523OAuthClientProvider) was weighed and deliberately declined by the author with sound reasoning — not restating it here.
Extended reasoning...
Overview
The PR removes the dead timeout parameter from OAuthClientProvider, the deprecated RFC7523OAuthClientProvider, and the OAuthContext dataclass field, plus the two internal super().__init__ call sites in client_credentials.py. Docs (oauth-clients.md, migration.md) and tests (test_auth.py, test_oauth_clients.py) are updated in the same PR. The diff is small and entirely subtractive.
Security risks
This touches OAuth client code, which is nominally security-sensitive, but the change removes an inert field — the value was stored on the context and never read, so no timeout enforcement, token handling, or flow logic changes. I verified with a repo-wide grep that nothing in src/mcp/client/auth, tests, docs source, or examples still references the parameter or context.timeout. Runtime behavior of the OAuth flow is byte-for-byte identical; the only observable change is a construction-time TypeError for callers still passing timeout=, which is the intended breaking change and is documented in docs/migration.md per the repo's branching policy for the v2 line.
Level of scrutiny
Moderate — auth code warrants care, but a dead-parameter removal is about as low-risk as auth changes get. The one real design question (the positional-argument shift that moves jwt_parameters into timeout's old slot on the deprecated RFC7523 shim) was raised by my prior review pass, examined in depth, and deliberately declined by the author with sound reasoning: the class is deprecated, this is the intentionally-breaking v2 branch, typed callers get a static error, and no previously-working configuration silently changes. That is an informed maintainer decision, not an unresolved concern.
Other factors
All reviewer threads are resolved: cubic's migration.md wording nit was fixed in d149d36 and my inline comment was answered and closed. The bug hunting system found no issues this run. The docs-claim test suite pins the new constructor signature exactly (test_the_two_remaining_keyword_arguments_have_defaults), so a regression reintroducing or misordering parameters would fail CI. Migration guidance is present and even tells users where to apply a bound if they relied on the (never-functional) timeout. Given the subtractive nature, complete cleanup, resolved feedback, and clean bug-hunt pass, this is safe to approve without a human pass.
OAuthContext.timeout was assigned in the constructor but never read anywhere, so the parameter has never bounded anything since the OAuth client was added. Drop it from OAuthClientProvider and OAuthContext rather than keep a knob that silently does nothing.
1aec562 to
fdf1cfe
Compare
There was a problem hiding this comment.
LGTM — mechanical removal of a parameter that was stored but never read; no runtime behavior changes.
What was reviewed:
- Confirmed
OAuthContext.timeouthad no remaining readers anywhere undersrc/mcp/client/auth— the field was write-only, so removal is pure dead-code deletion. - Checked the two
super().__init__(server_url, client_metadata, storage, None, None)call sites inclient_credentials.py— the remaining positionals still bind toredirect_handler/callback_handlercorrectly. - Breaking change is documented in
docs/migration.mdwith a workaround (anyio.fail_afterin the handlers), and docs/tests were updated in the same PR. - Prior review threads are resolved: the migration-guide wording was fixed in d149d36, and the RFC7523 positional-shift nit is moot now that the class is removed from main (#3169).
Extended reasoning...
Overview
This PR removes the unused timeout parameter from OAuthClientProvider and the OAuthContext.timeout field, updating the two subclass super().__init__ call sites in client_credentials.py, the OAuth clients doc page, the migration guide, and two tests. The value was stored on the context but never read by any code path, so this is dead-code removal.
Security risks
The diff touches OAuth client code, which is nominally security-sensitive, but no authentication logic changes: no token handling, PKCE, state/iss validation, or credential flow is modified. A grep of src/mcp/client/auth confirms zero remaining references to timeout, so nothing could have depended on the removed field. The only observable change is a construction-time TypeError for callers still passing timeout=, which is the intended v2 breaking behavior and is documented in docs/migration.md.
Level of scrutiny
Moderate scrutiny is appropriate — auth module, breaking API change — but the change is small, mechanical, and self-contained. I verified the positional argument binding at both super().__init__ call sites after the removal (the five remaining positionals bind correctly, with client_metadata_url now defaulting), and the docs_src signature test pins the exact new parameter list, so any accidental signature drift would fail CI.
Other factors
All prior reviewer threads are resolved: cubic's migration-guide wording nit was addressed in d149d36 with the author's acknowledgment, and my earlier nit about RFC7523OAuthClientProvider's positional shift was explicitly declined with valid reasoning — and is now moot since that deprecated class was removed from main in #3169 (the PR no longer touches it). The bug hunting system found no issues this run. Tests were updated in step (test_auth.py assertion removed, docs_src signature test renamed and updated), and the migration guide entry follows the repo's breaking-change documentation convention.
OAuthClientProvider(timeout=...)was stored onOAuthContext.timeoutbut nothing ever read it — it hasn't bounded anything since the OAuth client landed in #751. This removes the parameter fromOAuthClientProviderand theOAuthContextfield.Motivation and Context
A public keyword argument that silently does nothing is worse than not having it. The auth-flow's own HTTP requests aren't bounded by this value either (separate issue); the "wait for the user to authorize" step is the caller's own
redirect_handler/callback_handler, which they can bound themselves.How Has This Been Tested?
Constructed the provider through the public import; passing
timeout=now raisesTypeError. Existing auth and docs tests updated.Breaking Changes
Yes — callers passing
timeout=must drop it. Documented indocs/migration.md.Types of changes
Checklist