[Grapette.L (Lesereingrape)]

Motivation & Context

MCPTool documented request_timeout as "Timeout in seconds for MCP requests" but never validated it, and its single consumer folded the value through truthiness:

# python/packages/core/agent_framework/_mcp.py:2007 (main @ 726dcff8)
timedelta(seconds=self.request_timeout) if self.request_timeout else None

So request_timeout=0 — and likewise a negative value — became read_timeout_seconds=None. In the MCP SDK that is the unbounded setting, not the strictest one: mcp/shared/session.py:192 documents read_timeout_seconds as # If none, reading will never time out, and :284-291 passes it to anyio.fail_after(timeout), which is a no-op for None. A caller who configured the tightest possible timeout got the opposite behaviour: an MCP server that accepts the connection but never answers a request blocks that agent forever, with no McpError to catch and no timeout to retry against.

MCPStdioTool, MCPStreamableHTTPTool and MCPWebsocketTool each forward their own request_timeout into MCPTool.__init__ unchanged (:3684, :3963, :4497), so all three transports inherit the base-class behaviour and the base-class guard covers all of them. Reported and reproducible in #9313.

Description & Review Guide

• What are the major changes? Three small edits in python/packages/core/agent_framework/_mcp.py (7 lines changed total):

  1. a guard in MCPTool.__init__, placed directly after the existing max_host_payload_size_bytes guard (:998), so the two constructor knobs that take "a positive size/count, or None to opt out" are validated the same way;
  2. the consumer at :2007 now checks is not None instead of truthiness, so a positive value of any magnitude reaches ClientSession as written;
  3. the request_timeout docstring states the contract ("Must be positive, or None to leave requests without a timeout").

  Plus two tests in python/packages/core/tests/core/test_mcp.py, placed next to test_mcp_host_payload_size_limit_must_be_positive_or_none, which is the same shape of test for the sibling knob.

if request_timeout is not None and request_timeout <= 0:
    raise ValueError("request_timeout must be positive or None.")

• What is the impact of these changes? None keeps its exact current meaning (no timeout), and every positive value is unaffected — the consumer change is a no-op for those, since 0 was the only falsy value that could reach it as an int. What changes is the previously-silent group: request_timeout=0 and negative values now raise ValueError at construction instead of quietly disabling the timeout. That is a behaviour change for a caller who was relying on 0 meaning "no timeout", but None is the documented way to express that, and the value reads as a timeout in seconds, so 0 is far more likely to be a typo or a mis-set config. Failing at construction also localises the mistake to the line that set it, rather than surfacing much later as an unbounded await.

• What do you want reviewers to focus on? Whether rejecting non-positive values at construction is the right contract, versus treating 0 as "no timeout" and letting it through. I followed the in-repo precedent in this same module: max_host_payload_size_bytes already raises "max_host_payload_size_bytes must be positive or None." (:1000), and MCPTaskOptions.__post_init__ rejects non-positive durations for default_ttl and max_task_wait (:785-788), so a "positive, or None to opt out" knob is validated at construction here rather than reinterpreted downstream. If the team would rather have 0 mean "no timeout", the consumer change alone (is not None, with 0 mapped to None) delivers that, and the tests can be re-pointed at the resulting read_timeout_seconds value instead of the raised error.

Local verification, on main at 726dcff8:

• Before the source change, the new validation test fails on main (Failed: DID NOT RAISE ValueError, tests/core/test_mcp.py:1055); the second new test already passes on main and is kept to pin the positive-value path so the is not None change cannot regress it.
• After it, all four MCP test modules pass: test_mcp.py (398), test_mcp_http_auth.py (91), test_mcp_observability.py (15), test_mcp_skills.py (615) — 1,119 tests, no failures.
• The repo's pinned linter/formatter, ruff==0.16.10, reports no findings on both changed files (ruff check --no-fix) and ruff format --diff reports both already formatted.
• The full tests/core suite was also run: 4 tests fail, and all 4 fail identically on a pristine 726dcff8 checkout without this change — two need the aiohttp extra, one is a long-session-id temp path that exceeds the Windows path limit, and test_feature_stage.py::test_feature_id_allows_lowercase_values is unrelated to MCP. Nothing in the MCP surface fails.

Related Issue

Fixes #9313

I checked for competing open PRs before filing: no open PR in this repository mentions request_timeout, and the last commit to touch _mcp.py is 84d13537 (#8972), so the hunks in this change are untouched on current main.

AI Assistance

• No material AI assistance was used.
• This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: the guard, the consumer/docstring edits, both tests and this description were drafted by the Qoder coding assistant working from the repro written up in #9313, and were then run locally through the checks listed above (pinned ruff==0.16.10 plus the four MCP test modules on 726dcff8, before and after the source change). The diff is deliberately small and self-contained so it is quick to audit. The checkbox above is left for the human submitter to tick, since accepting responsibility is theirs to state.

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
• This is not a breaking change. No breaking change label requested: None and every positive value behave exactly as before; only values that previously fell through to the unbounded branch now raise. Callers passing 0 do see a new error, which is the point of the fix and is noted under "What is the impact of these changes?" above.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Eduard van Valkenburg (eavanvalkenburg)]

/review

[github-actions]

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (2 commit(s)): 444031688dd6, ab00244a5c8d
Model: gpt-5.6-sol

Overview

The change correctly rejects non-positive constructor inputs and explicitly forwards positive values to the MCP SDK; the added tests cover both validation and the initial connection path. All three transport subclasses share the base guard, and no repository production caller supplies this option from untrusted input. One lifecycle gap remains: the public mutable attribute can bypass construction-time validation, and the changed consumer then turns a later zero assignment into immediate request failures.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/_mcp.py

[github-actions]

request_timeout remains a public mutable attribute, so assigning 0 after construction bypasses the new guard. On the next connect or reconnect, this branch passes timedelta(0) to ClientSession, making every MCP request expire immediately instead of raising the documented configuration error. Please preserve the positive-or-None invariant for later assignments, or revalidate the value before creating the session.

[Grapette.L (Lesereingrape)]

Good catch — the invariant only held for the constructor. _connect_on_owner() now revalidates request_timeout immediately before the framework-owned Client is built, so a later tool.request_timeout = 0 raises the same ValueError instead of opening a session whose requests expire immediately. Added test_connect_revalidates_request_timeout_cleared_after_construction, which also asserts the SDK Client is never constructed.

For context on the line you quoted: the branch also merged main, which now passes the timeout as Client(read_timeout_seconds=...) rather than a timedelta into ClientSession; the guard follows that. Local run on this base: 426 passed, 2 skipped for tests/core/test_mcp.py, ruff check and ruff format --check clean.

[Grapette.L (Lesereingrape)]

Follow-up on the AI-review thread: request_timeout is now revalidated in _connect_on_owner() before the framework-owned Client is built, so assigning 0 after construction raises the same ValueError instead of opening a session whose requests expire immediately (69b0b89), with a regression test asserting the Client is never constructed.

The checks for this head are parked as action_required, so they still need a maintainer to release them. Local run on this base: 426 passed, 2 skipped in tests/core/test_mcp.py; ruff check and ruff format --check clean.

[Eduard van Valkenburg (eavanvalkenburg)]

The constructor and reconnect-time validation consistently enforce a positive timeout or None, while preserving the existing positive and unbounded paths. The regression covers post-construction mutation before SDK client creation.