fix(sdk-python): validate max_tool_calls and max_subagent_depth as integers - #7548
Conversation
…tegers max_session_turns rejects bools and non-ints before its range check, but the two sibling limits only range-checked, so values their own error messages rule out were accepted and stringified onto the CLI: max_tool_calls=True -> --max-tool-calls True max_tool_calls=2.5 -> --max-tool-calls 2.5 max_subagent_depth=True -> --max-subagent-depth True bool is the sharp edge: it subclasses int, so True passes both isinstance and the range comparison as 1. Extract the bool-aware check the max_session_turns branch already performed into _is_int and apply it to all three. max_session_turns keeps its exact previous behavior; the expression is the same, only named.
|
Thanks for the PR! Template looks good ✓ Problem: observed and concretely demonstrated. The table in the PR body shows Direction: aligned. The SDK's error messages already promise integer validation ("must be -1 or a non-negative integer", "must be between 1 and 100") but only one of the three options enforced it. Making the validation match the documented contract is straightforward correctness work. Size: not applicable (sdk-python, not core infrastructure). Approach: minimal and well-scoped. Extracting the existing inline bool-aware check from Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且有具体演示。PR 正文中的表格展示了 方向：对齐。SDK 的错误信息已承诺整数校验（"must be -1 or a non-negative integer"、"must be between 1 and 100"），但三个选项中只有一个执行了该契约。让校验与文档承诺一致是直接的修复。 规模：不适用（sdk-python，非核心基础设施）。 方案：最小且范围恰当。将 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: I'd extract the inline bool-aware check from What the PR does: exactly that. No critical blockers. No convention violations. The docstring on Before (main branch)After (PR #7548 applied)Test case verification (all PR test cases exercised manually)Note: tmux was unavailable on this CI runner; tests were executed directly against the SDK source with the PR patch applied, then reverted. The behavioral evidence is equivalent. 中文说明代码审查独立方案： 将 PR 实际做法： 完全一致。 无关键阻塞项。无规范违反。 修复前（main 分支）
修复后（PR #7548 应用后）上述输入均被正确拒绝，并返回清晰的 测试用例验证PR 中所有新增及既有测试用例均手动执行通过。 注：本 CI 运行器无 tmux；测试直接对 SDK 源码执行（应用 PR 补丁后），随后还原。行为证据等价。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean, minimal fix for a real validation gap; before/after confirms it works exactly as promised. This is what a good SDK fix looks like: the error messages already promised integer validation, one sibling enforced it, two didn't. The PR extracts the existing check, names it, and applies it uniformly. No new abstractions, no scope creep, no behavior change for valid inputs. The tests are precise about which cases are newly covered vs. already caught by accident — that kind of care tells me the author actually thought about the boundary conditions rather than just throwing a type check over the fence. The before/after output speaks for itself: Ships it. ✅ 中文说明置信度：5/5 — 干净、最小的修复，针对真实的校验缺口；修复前后对比确认其行为完全如承诺。 这是一个好的 SDK 修复的样子：错误信息已承诺整数校验，一个同类选项执行了，另外两个没有。PR 提取已有检查、命名、统一应用。无新抽象、无范围蔓延、对合法输入无行为变化。测试精确区分了新增覆盖与偶然已捕获的用例——这种细致说明作者确实思考了边界条件，而非随手丢一个类型检查。 修复前后输出说明一切： 可以合并 ✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| not _is_int(options.max_subagent_depth) | ||
| or not (1 <= options.max_subagent_depth <= 100) | ||
| ): | ||
| raise ValidationError("max_subagent_depth must be between 1 and 100") |
There was a problem hiding this comment.
[Suggestion] The error message says "must be between 1 and 100", but 2.5 is between 1 and 100. A user who passes a float gets a misleading message that doesn't indicate the actual problem (non-integer type). The same applies to True/False — they satisfy the range check numerically.
Consider updating the message to mention the integer requirement:
| raise ValidationError("max_subagent_depth must be between 1 and 100") | |
| raise ValidationError("max_subagent_depth must be an integer between 1 and 100") |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fair point, and applied — max_subagent_depth must be an integer between 1 and 100. You're right that the old wording was actively unhelpful for 2.5: it named a constraint the value already satisfies, so the message pointed away from the real problem.
Left max_tool_calls alone since it already says "must be -1 or a non-negative integer", which names the type.
Test matchers assert on the option name rather than the full string, so nothing else moved: 123 passed / 29 skipped, ruff check + format clean, mypy clean on 9 files.
| def _is_int(value: object) -> bool: | ||
| """True for a real integer. | ||
|
|
||
| ``bool`` subclasses ``int``, so ``isinstance(True, int)`` is True and a | ||
| bare isinstance check would let ``max_tool_calls=True`` through as 1. | ||
| """ | ||
| return isinstance(value, int) and not isinstance(value, bool) |
There was a problem hiding this comment.
[Suggestion] This predicate duplicates logic already present in _as_optional_int in types.py (line 258: isinstance(raw, bool) or not isinstance(raw, int)), which guards the same three fields during JSON deserialization. If a future change adjusts what counts as a valid integer (e.g., accepting numpy.int64), both sites must be updated — missing one creates inconsistent behavior between the programmatic and deserialization paths.
Consider consolidating: move _is_int to types.py (or import it there) and refactor _as_optional_int to use it.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Good catch that the two sites exist — I'd rather not consolidate them here, though, because they aren't quite the same check and merging them would change behavior beyond this fix.
_as_optional_int is a deserialization coercion: it runs on untrusted Mapping data in QueryOptions.from_dict and raises TypeError. _is_int is a predicate used by validate_query_options, which raises ValidationError and composes with a range test in the same condition. Having _as_optional_int call _is_int would mean types.py importing from validation.py, which imports QueryOptions from types.py — a cycle. The other direction (moving _is_int into types.py and importing it back) works, but it makes a private helper of the type module part of the validation module's contract for a two-line predicate.
The divergence risk you describe is real but bounded: if numpy.int64 ever needed accepting, the deserialization path would have to change anyway, since _as_optional_int also calls int(raw) on the result and the two paths have different error types. I'd rather leave both explicit and obvious than share a helper across that boundary — and if the project would prefer them unified, that's a refactor worth its own PR rather than folding into a validation fix.
A float like 2.5 is between 1 and 100, so the old message described a constraint the value already satisfied and gave no hint that the type was the problem.
Review & Local Verification Report代码审查设计评价：Python 类型系统的经典陷阱修复。 问题： Python 中
修复： def _is_int(value: object) -> bool:
return isinstance(value, int) and not isinstance(value, bool)覆盖范围扩展：
测试： 参数化测试覆盖 结论LGTM。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
ZijianZhang989
left a comment
ZijianZhang989
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
6 participants
What this PR does
max_tool_callsandmax_subagent_depthonly range-checked their input, while the siblingmax_session_turnsalso rejects bools and non-integers. This applies the same integer check to all three by extracting the onemax_session_turnsalready performed.Why it's needed
Each of these options states its contract in its own error message — "must be -1 or a non-negative integer", "must be between 1 and 100" — but only
max_session_turnsenforced the integer part. The value is then stringified straight onto the CLI intransport.py, so an accepted non-integer becomes a malformed argument rather than a clearValidationErrorat the SDK boundary.Verified against
validate_query_optionsonmain:mainmax_tool_calls=True--max-tool-calls Truemax_tool_calls=2.5--max-tool-calls 2.5max_subagent_depth=True--max-subagent-depth Truemax_subagent_depth=2.5--max-subagent-depth 2.5max_session_turns=Truemax_session_turns=2.5boolis the sharp edge: it subclassesint, soTruesatisfiesisinstance(x, int)and passes every range comparison as1.max_subagent_depth=Truetherefore slips through1 <= x <= 100as a valid depth of 1. (Falseis caught there by accident, since0fails the lower bound — but only by accident.)The bool-aware check already existed inline in the
max_session_turnsbranch; this names it_is_intand reuses it.max_session_turnskeeps its exact previous behavior — same expression, just extracted.Reviewer Test Plan
How to verify
python -m pytest -c packages/sdk-python/pyproject.toml packages/sdk-python/tests -q→ 123 passed, 29 skipped.python -m mypy --config-file packages/sdk-python/pyproject.toml packages/sdk-python/src→ success, 9 source files.python -m ruff check/ruff format --check→ clean on both changed files.validation.pyfails 5 of the new cases:max_tool_calls[True],[False],[0.5]andmax_subagent_depth[True],[2.5].max_subagent_depth[False]passes before and after, becauseFalse == 0already failed the range check — the parametrization is deliberately precise about which cases are actually new.test_accepts_valid_integer_limitspins the other direction:-1,0,1and100still validate across all three options, so this cannot become an over-strict regression.test_rejects_invalid_max_tool_calls(-2),test_rejects_invalid_max_subagent_depth(0) and the threemax_session_turnstests are untouched and still pass.Evidence (Before & After)
QueryOptions(max_tool_calls=True)validates, and the CLI is invoked with--max-tool-calls True.ValidationError: max_tool_calls must be -1 or a non-negative integer— the message the option already promised.Tested on
macOS, CPython 3.12/3.13: full
sdk-pythonsuite, mypy, ruff check and ruff format all pass locally, with proven fail-before/pass-after. Type checking with no platform-dependent behavior, so no manual QA is required; thesdk-pythonworkflow covers the CI matrix.Environment (optional)
CPython 3.12.10 / 3.13.3;
packages/sdk-python; pytest via the repo's ownpyproject.tomlconfig.Risk & Scope
Trueor a float for these options now gets aValidationErrorinstead of a confusing downstream CLI failure. Callers passing integers — the documented type — are unaffected, whichtest_accepts_valid_integer_limitspins.validate_query_optionsthat share the same contract. It is one gap (integer options not validated as integers), not three separate ones, so they are fixed together rather than split across PRs. Other validators are untouched.Linked Issues
None — found by comparing
max_tool_callsagainst the strictermax_session_turnscheck a few lines above it.中文说明
本 PR 的作用
max_tool_calls与max_subagent_depth只做了范围检查，而同类的max_session_turns还会拒绝布尔值与非整数。本 PR 把max_session_turns已有的那段整数判断提取出来，并应用于全部三者。为什么需要
这些选项各自在错误信息中声明了自己的契约——「must be -1 or a non-negative integer」「must be between 1 and 100」——但只有
max_session_turns真正执行了其中的整数部分。该值随后在transport.py中被直接字符串化拼到 CLI 上，于是被放行的非整数会变成一个畸形参数，而不是在 SDK 边界抛出清晰的ValidationError。在
main上针对validate_query_options验证：mainmax_tool_calls=True--max-tool-calls Truemax_tool_calls=2.5--max-tool-calls 2.5max_subagent_depth=True--max-subagent-depth Truemax_subagent_depth=2.5--max-subagent-depth 2.5max_session_turns=Truemax_session_turns=2.5bool是最尖锐的一处：它是int的子类，因此True既满足isinstance(x, int)，又以1的身份通过所有范围比较。于是max_subagent_depth=True会作为「深度 1」溜过1 <= x <= 100。（False在那里被拦下纯属偶然——0未过下界而已。）这段能识别布尔的判断本就以内联形式存在于
max_session_turns分支中；本 PR 将其命名为_is_int并复用。max_session_turns的行为与此前完全一致——表达式相同，只是被提取出来。复核测试计划
如何验证
python -m pytest -c packages/sdk-python/pyproject.toml packages/sdk-python/tests -q→ 123 passed、29 skipped。python -m mypy --config-file packages/sdk-python/pyproject.toml packages/sdk-python/src→ 成功，9 个源文件。python -m ruff check/ruff format --check→ 两个改动文件均干净。validation.py，新增用例中有 5 个失败：max_tool_calls[True]、[False]、[0.5]以及max_subagent_depth[True]、[2.5]。max_subagent_depth[False]在修复前后均通过，因为False == 0本就未过范围检查——参数化刻意精确区分了哪些用例才是真正新增覆盖的。test_accepts_valid_integer_limits固定了反方向：-1、0、1、100在三个选项上仍然通过校验，因此本改动不会演变成过度严格的回归。test_rejects_invalid_max_tool_calls（-2）、test_rejects_invalid_max_subagent_depth（0）以及三个max_session_turns测试未作改动且仍然通过。证据（修复前后对比）
QueryOptions(max_tool_calls=True)校验通过，CLI 被以--max-tool-calls True调用。ValidationError: max_tool_calls must be -1 or a non-negative integer——正是该选项早已承诺的那句提示。测试环境
macOS，CPython 3.12/3.13：完整
sdk-python套件、mypy、ruff check 与 ruff format 本地全部通过，并验证了 fail-before/pass-after。纯类型检查，无平台相关行为，因此无需人工 QA；sdk-pythonworkflow 覆盖 CI 矩阵。运行环境（可选）
CPython 3.12.10 / 3.13.3；
packages/sdk-python；pytest 使用仓库自带的pyproject.toml配置。风险与影响范围
True或浮点数的调用方，现在会收到ValidationError，而不再是下游一个令人困惑的 CLI 失败。传入整数（即文档所载类型）的调用方不受影响，这一点由test_accepts_valid_integer_limits固定。validate_query_options中共享同一契约的三个整数上限。这是一个缺口（整数选项未按整数校验），而非三个彼此独立的缺口，因此一并修复而不拆成多个 PR。其他校验函数未作改动。关联 Issue
无——通过将
max_tool_calls与其上方几行更严格的max_session_turns判断对照发现。