Skip to content

Make the deeply nested body test independent of stack size #3610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maxisbey merged 1 commit into from
Oct 1, 2026
Merged

Make the deeply nested body test independent of stack size #3610

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions tests/server/test_streamable_http_modern.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,8 +1139,9 @@ async def broken_list(ctx: ServerRequestContext, params: PaginatedRequestParams


async def test_modern_post_with_deeply_nested_body_is_parse_error_not_a_crash() -> None:
"""Deep nesting makes json.loads raise RecursionError; still an unparseable body: 400 + PARSE_ERROR."""
body = b"[" * 100_000 + b"]" * 100_000
"""Unterminated deep nesting makes json.loads raise RecursionError or, where the stack is deep enough
to reach the end of input, JSONDecodeError; an unparseable body either way: 400 + PARSE_ERROR."""
Comment on lines +1142 to +1143

@claude claude Bot Oct 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Readers of this test get a docstring that explains the non-obvious reason for the body shape instead of a comment on the line that needs it. The docstring at tests/server/test_streamable_http_modern.py:1142-1143 spends its text on why json.loads raises RecursionError versus JSONDecodeError, which is the constraint behind the b"[" * 100_000 body at line 1144, not the behaviour under test. Fix: keep the docstring to the behaviour (unparseable body -> 400 + PARSE_ERROR, SDK-defined) and move the stack-depth rationale to a one-line comment next to line 1144, matching the sibling test in test_func_metadata.py.

Why this was flagged

The convention in .claude/skills/test-quality/SKILL.md:108 says comments live next to the line they explain, not in docstrings, and SKILL.md:16 asks docstrings for 1-2 sentences of behaviour. The new docstring at tests/server/test_streamable_http_modern.py:1142-1143 is a 40-word compound sentence whose first clause is an explanation of interpreter stack behaviour, a constraint on the fixture body at line 1144, not behaviour. When the body is next edited (e.g. someone 'fixes' the missing closing brackets because the test name says deeply nested), the rationale in the docstring is easy to miss and the stack-size dependence from issue #3146 returns. The dismissing finder counted sentences rather than checking what the sentence explains. Remedy: trim the docstring to the behaviour and put the unterminated-body reason as a comment beside line 1144.

Verification: nit. Cosmetic placement issue; nothing fails at runtime. /home/claude/python-sdk/.claude/skills/test-quality/SKILL.md:108 reads "Comments live next to the line they explain, not in docstrings". The docstring at tests/server/test_streamable_http_modern.py:1142-1143 carries the rationale for why line 1144 is b"[" * 100_000 with no closing brackets, placed in the docstring rather than beside it.

body = b"[" * 100_000
async with _asgi_client(_x_mcp_server()) as http:
response = await http.post("/mcp", content=body, headers={"content-type": "application/json"})
assert response.status_code == 400
Expand Down
Loading