Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 17 additions & 7 deletions packages/sdk-python/src/qwen_code_sdk/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,7 @@ def validate_query_options(options: QueryOptions) -> None:
)

if options.max_session_turns is not None and (
isinstance(options.max_session_turns, bool)
or not isinstance(options.max_session_turns, int)
or options.max_session_turns < -1
not _is_int(options.max_session_turns) or options.max_session_turns < -1
):
raise ValidationError("max_session_turns must be -1 or a non-negative integer")

Expand All @@ -147,13 +145,16 @@ def validate_query_options(options: QueryOptions) -> None:
):
raise ValidationError("path_to_qwen_executable cannot be empty")

if options.max_tool_calls is not None and options.max_tool_calls < -1:
if options.max_tool_calls is not None and (
not _is_int(options.max_tool_calls) or options.max_tool_calls < -1
):
raise ValidationError("max_tool_calls must be -1 or a non-negative integer")

if options.max_subagent_depth is not None and not (
1 <= options.max_subagent_depth <= 100
if options.max_subagent_depth is not None and (
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")
raise ValidationError("max_subagent_depth must be an integer between 1 and 100")

if options.agents:
for i, agent in enumerate(options.agents):
Expand Down Expand Up @@ -205,6 +206,15 @@ def validate_query_options(options: QueryOptions) -> None:
raise ValidationError("proxy cannot be empty")


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)
Comment on lines +209 to +215

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.



def _validate_optional_callable(
value: object,
validator: Callable[[object, type[ValidationError]], None],
Expand Down
25 changes: 25 additions & 0 deletions packages/sdk-python/tests/unit/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,31 @@ def test_rejects_invalid_max_subagent_depth() -> None:
validate_query_options(QueryOptions(max_subagent_depth=0))


@pytest.mark.parametrize("value", [True, False, 0.5])
def test_rejects_non_integer_max_tool_calls(value: object) -> None:
# The error message promises "-1 or a non-negative integer", and the value
# is stringified straight onto the CLI, so `True` would become
# `--max-tool-calls True`.
with pytest.raises(ValidationError, match="max_tool_calls"):
validate_query_options(QueryOptions(max_tool_calls=cast(Any, value)))


@pytest.mark.parametrize("value", [True, False, 2.5])
def test_rejects_non_integer_max_subagent_depth(value: object) -> None:
with pytest.raises(ValidationError, match="max_subagent_depth"):
validate_query_options(QueryOptions(max_subagent_depth=cast(Any, value)))


def test_accepts_valid_integer_limits() -> None:
# The in-range integers these options are documented to take must survive.
validate_query_options(
QueryOptions(max_tool_calls=-1, max_session_turns=-1, max_subagent_depth=1)
)
validate_query_options(
QueryOptions(max_tool_calls=0, max_session_turns=0, max_subagent_depth=100)
)


def test_rejects_agents_missing_required_fields() -> None:
with pytest.raises(ValidationError, match="missing required field"):
validate_query_options(QueryOptions(agents=[{"name": "test"}]))
Expand Down
Loading