fix(mock-provider): strict request booleans, bounded recursion, multipleOf
Second CodeRabbit pass. All four findings reproduced first; none is reachable
from a Honcho response model, but two trace back to the previous commit.
- `include_usage` and `stream` were plain `bool`, which Pydantic coerces from
"yes"/"on"/"true"/"1". The comment added last commit claimed a string had to
fail here, and it did not — the test only passed because "definitely" is not
a recognised bool literal. Both are StrictBool now, matching why `dimensions`
is StrictInt, and the tests cover the truthy strings that actually coerced.
- `_generate_array` returned the prefix alone when `items` was absent, so
prefixItems plus a larger minItems undershot its own schema. Absent `items`
leaves those positions unconstrained rather than disallowed, so the shortfall
is filled to minItems — a bare `{"type": "array"}` still generates nothing.
- A required, non-nullable recursive $ref hit RecursionError: MAX_DEPTH only
terminates a cycle that offers a `default` or a nullable branch, and
`_generate_object` keeps descending into required properties. HARD_MAX_DEPTH
degrades to an empty container instead, since a mock must not turn its own
defect into a 500. Bounded, not plumbed into an error response — the
unreachable path does not justify touching the request path.
- `_bounded_int` ignored `multipleOf` while honouring minimum, maximum and both
exclusive bounds; 9 of 12 sampled paths produced a non-multiple. Values now
snap onto a multiple inside the bounds, and an unsatisfiable window keeps the
bounds. A fractional `multipleOf` is still ignored, as documented.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Eugene Eisenstein
parent
00b7da371b
commit
786c74f32f
|
|
@ -31,6 +31,14 @@ from src.mock_provider.coerce import as_dict, as_int, as_list, as_str
|
|||
# inside conclusions, so a $ref cycle is normal input, not a malformed schema.
|
||||
MAX_DEPTH = 6
|
||||
|
||||
# Absolute cap. Past MAX_DEPTH a cycle is expected to terminate on a `default`
|
||||
# or a nullable/optional branch; a required, non-nullable self-reference has
|
||||
# neither and would recurse until Python raises RecursionError. Degrading to an
|
||||
# empty container may violate the schema, but a mock must not turn its own
|
||||
# defect into a 500. Set well clear of MAX_DEPTH so no schema that terminates
|
||||
# on its own ever reaches it.
|
||||
HARD_MAX_DEPTH = MAX_DEPTH * 4
|
||||
|
||||
_WORDS = (
|
||||
"synthetic",
|
||||
"placeholder",
|
||||
|
|
@ -171,9 +179,14 @@ def _generate(
|
|||
return _generate(_pick_branch(branches, root, depth), root, path, depth)
|
||||
|
||||
kind = _infer_type(resolved)
|
||||
# Only the two recursive kinds need the absolute cap; scalars terminate.
|
||||
if kind == "object":
|
||||
if depth >= HARD_MAX_DEPTH:
|
||||
return {}
|
||||
return _generate_object(resolved, root, path, depth)
|
||||
if kind == "array":
|
||||
if depth >= HARD_MAX_DEPTH:
|
||||
return []
|
||||
return _generate_array(resolved, root, path, depth)
|
||||
if kind == "integer":
|
||||
return _bounded_int(resolved, path)
|
||||
|
|
@ -264,14 +277,15 @@ def _generate_array(
|
|||
count = min(count, max_items)
|
||||
if depth >= MAX_DEPTH:
|
||||
count = min_items or 0
|
||||
if items is None:
|
||||
return prefix
|
||||
|
||||
# `items` describes the positions after the prefix, so only the shortfall is
|
||||
# filled homogeneously.
|
||||
# filled. With `items` absent those positions are unconstrained rather than
|
||||
# disallowed: an empty schema stands in, and the target drops to whatever
|
||||
# minItems demands, so a bare `{"type": "array"}` still generates nothing.
|
||||
trailing = items if items is not None else {}
|
||||
target = count if items is not None else min(count, min_items or 0)
|
||||
return prefix + [
|
||||
_generate(items, root, f"{path}[{len(prefix) + i}]", depth + 1)
|
||||
for i in range(max(0, count - len(prefix)))
|
||||
_generate(trailing, root, f"{path}[{len(prefix) + i}]", depth + 1)
|
||||
for i in range(max(0, target - len(prefix)))
|
||||
]
|
||||
|
||||
|
||||
|
|
@ -320,9 +334,37 @@ def _bounded_int(schema: dict[str, Any], path: str) -> int:
|
|||
|
||||
if low is not None and high is not None:
|
||||
span = high - low
|
||||
return low + (_seed(path) % (span + 1) if span > 0 else 0)
|
||||
if low is not None:
|
||||
return low + (_seed(path) % 8)
|
||||
if high is not None:
|
||||
return high - (_seed(path) % 8)
|
||||
return _seed(path) % 100
|
||||
value = low + (_seed(path) % (span + 1) if span > 0 else 0)
|
||||
elif low is not None:
|
||||
value = low + (_seed(path) % 8)
|
||||
elif high is not None:
|
||||
value = high - (_seed(path) % 8)
|
||||
else:
|
||||
value = _seed(path) % 100
|
||||
|
||||
return _snap_to_multiple(value, as_int(schema.get("multipleOf")), low, high)
|
||||
|
||||
|
||||
def _snap_to_multiple(
|
||||
value: int, multiple: int | None, low: int | None, high: int | None
|
||||
) -> int:
|
||||
"""Move ``value`` onto a multiple of ``multiple``, staying within bounds.
|
||||
|
||||
A fractional ``multipleOf`` is ignored: ``as_int`` rejects it, and honouring
|
||||
it would mean returning a non-integer from an ``integer`` schema.
|
||||
"""
|
||||
if multiple is None or multiple <= 0:
|
||||
return value
|
||||
|
||||
# Floor division, so a negative value snaps down to the next multiple below.
|
||||
snapped = (value // multiple) * multiple
|
||||
if low is not None and snapped < low:
|
||||
snapped = -(-low // multiple) * multiple # smallest multiple >= low
|
||||
if high is not None and snapped > high:
|
||||
snapped = (high // multiple) * multiple # largest multiple <= high
|
||||
|
||||
# No multiple exists in the window, so the schema is unsatisfiable. An
|
||||
# in-range value breaks the constraint the caller is less likely to check.
|
||||
if (low is not None and snapped < low) or (high is not None and snapped > high):
|
||||
return value
|
||||
return snapped
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ from __future__ import annotations
|
|||
|
||||
from typing import Annotated, Any, ClassVar, Literal
|
||||
|
||||
from pydantic import BaseModel, ConfigDict, Field, StrictInt
|
||||
from pydantic import BaseModel, ConfigDict, Field, StrictBool, StrictInt
|
||||
|
||||
|
||||
class MockRequest(BaseModel):
|
||||
|
|
@ -38,9 +38,10 @@ class ChatMessage(MockRequest):
|
|||
|
||||
class StreamOptions(MockRequest):
|
||||
# Typed rather than left as a dict because the usage chunk is conditional on
|
||||
# it: `{"include_usage": "yes"}` must fail like the real API does rather than
|
||||
# read as truthy and emit a chunk the caller never asked for.
|
||||
include_usage: bool = False
|
||||
# it. StrictBool for the same reason `dimensions` is StrictInt: plain `bool`
|
||||
# coerces "yes"/"on"/"true"/"1", so a string would quietly decide the shape
|
||||
# of the stream instead of failing the way the real API does.
|
||||
include_usage: StrictBool = False
|
||||
|
||||
|
||||
class ChatCompletionRequest(MockRequest):
|
||||
|
|
@ -48,7 +49,10 @@ class ChatCompletionRequest(MockRequest):
|
|||
messages: list[ChatMessage] = []
|
||||
response_format: dict[str, Any] | None = None
|
||||
tools: list[dict[str, Any]] | None = None
|
||||
stream: bool = False
|
||||
# StrictBool because this one field decides between two response *shapes* —
|
||||
# a JSON body or an SSE stream — so coercing a string here is the difference
|
||||
# between a working client and one that hangs waiting for events.
|
||||
stream: StrictBool = False
|
||||
stream_options: StreamOptions | None = None
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -20,9 +20,10 @@ import pytest
|
|||
from fastapi.testclient import TestClient
|
||||
from pydantic import BaseModel, Field
|
||||
|
||||
from src.mock_provider.coerce import as_dict
|
||||
from src.mock_provider.embeddings import content_to_embedding
|
||||
from src.mock_provider.main import app
|
||||
from src.mock_provider.schema_gen import generate
|
||||
from src.mock_provider.schema_gen import HARD_MAX_DEPTH, MAX_DEPTH, generate
|
||||
from src.utils.representation import PromptRepresentation
|
||||
|
||||
# A $ref/$defs schema, which is what Pydantic emits for any nested model and the
|
||||
|
|
@ -313,6 +314,113 @@ def test_prefix_items_are_followed_by_homogeneous_items() -> None:
|
|||
assert all(isinstance(value, bool) for value in result[2:])
|
||||
|
||||
|
||||
def test_min_items_is_met_when_items_is_omitted() -> None:
|
||||
"""Absent `items` leaves trailing positions unconstrained, not disallowed."""
|
||||
schema: dict[str, Any] = {
|
||||
"type": "array",
|
||||
"prefixItems": [{"type": "string"}],
|
||||
"minItems": 3,
|
||||
}
|
||||
result = generate(schema)
|
||||
|
||||
assert len(result) == 3
|
||||
assert isinstance(result[0], str)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("constraints", "multiple"),
|
||||
[
|
||||
({"minimum": 0, "maximum": 100, "multipleOf": 10}, 10),
|
||||
({"minimum": 7, "maximum": 9, "multipleOf": 4}, 4),
|
||||
({"minimum": -100, "maximum": 0, "multipleOf": 25}, 25),
|
||||
({"minimum": 5, "multipleOf": 3}, 3),
|
||||
({"maximum": -5, "multipleOf": 3}, 3),
|
||||
({"multipleOf": 6}, 6),
|
||||
],
|
||||
)
|
||||
def test_multiple_of_is_honoured_within_bounds(
|
||||
constraints: dict[str, Any], multiple: int
|
||||
) -> None:
|
||||
"""Path-seeded values land off the multiple unless snapped back onto it."""
|
||||
low = constraints.get("minimum")
|
||||
high = constraints.get("maximum")
|
||||
|
||||
# Several paths, because a single one can satisfy the constraint by luck.
|
||||
for index in range(12):
|
||||
schema: dict[str, Any] = {
|
||||
"type": "object",
|
||||
"properties": {f"f{index}": {"type": "integer", **constraints}},
|
||||
"required": [f"f{index}"],
|
||||
}
|
||||
value = generate(schema)[f"f{index}"]
|
||||
|
||||
assert value % multiple == 0, f"{value} is not a multiple of {multiple}"
|
||||
if low is not None:
|
||||
assert value >= low
|
||||
if high is not None:
|
||||
assert value <= high
|
||||
|
||||
|
||||
def test_unsatisfiable_multiple_of_stays_within_bounds() -> None:
|
||||
"""No multiple of 10 lies in [3, 7], so the bounds win over the multiple."""
|
||||
schema: dict[str, Any] = {
|
||||
"type": "integer",
|
||||
"minimum": 3,
|
||||
"maximum": 7,
|
||||
"multipleOf": 10,
|
||||
}
|
||||
result = generate(schema)
|
||||
assert 3 <= result <= 7
|
||||
|
||||
|
||||
def test_required_recursive_ref_terminates_instead_of_overflowing() -> None:
|
||||
"""A required, non-nullable cycle has no `default` or null branch to stop on.
|
||||
|
||||
MAX_DEPTH alone does not save it — `_generate_object` keeps descending into
|
||||
required properties — so the absolute cap has to.
|
||||
"""
|
||||
schema: dict[str, Any] = {
|
||||
"$defs": {
|
||||
"Node": {
|
||||
"type": "object",
|
||||
"properties": {"child": {"$ref": "#/$defs/Node"}},
|
||||
"required": ["child"],
|
||||
}
|
||||
},
|
||||
"$ref": "#/$defs/Node",
|
||||
}
|
||||
node = as_dict(generate(schema))
|
||||
|
||||
depth = 0
|
||||
# The cap returns {}, so an empty dict is the terminator.
|
||||
while node:
|
||||
node = as_dict(node["child"])
|
||||
depth += 1
|
||||
assert depth <= HARD_MAX_DEPTH, "absolute depth cap did not hold"
|
||||
assert depth > MAX_DEPTH, "should descend past the soft cap before stopping"
|
||||
|
||||
|
||||
def test_required_recursive_array_terminates_instead_of_overflowing() -> None:
|
||||
"""minItems >= 1 keeps `_generate_array` from emptying out at the soft cap."""
|
||||
schema: dict[str, Any] = {
|
||||
"$defs": {
|
||||
"Node": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"kids": {
|
||||
"type": "array",
|
||||
"items": {"$ref": "#/$defs/Node"},
|
||||
"minItems": 1,
|
||||
}
|
||||
},
|
||||
"required": ["kids"],
|
||||
}
|
||||
},
|
||||
"$ref": "#/$defs/Node",
|
||||
}
|
||||
generate(schema) # must not raise RecursionError
|
||||
|
||||
|
||||
def test_generation_is_stable_across_calls() -> None:
|
||||
assert generate(PROBE_SCHEMA) == generate(PROBE_SCHEMA)
|
||||
|
||||
|
|
@ -596,15 +704,37 @@ def test_stream_without_include_usage_emits_no_usage_chunk(
|
|||
assert "[mock]" in content
|
||||
|
||||
|
||||
def test_non_boolean_include_usage_is_rejected(client: TestClient) -> None:
|
||||
"""The usage chunk is conditional on this, so a wrong type must 400."""
|
||||
@pytest.mark.parametrize("value", ["definitely", "yes", "on", "true", "1", 1])
|
||||
def test_non_boolean_include_usage_is_rejected(client: TestClient, value: Any) -> None:
|
||||
"""The usage chunk is conditional on this, so a wrong type must 400.
|
||||
|
||||
The truthy strings matter more than the nonsense one: plain `bool` coerces
|
||||
"yes"/"on"/"true"/"1", so without StrictBool a string would silently decide
|
||||
whether the stream carries usage.
|
||||
"""
|
||||
response = client.post(
|
||||
"/v1/chat/completions",
|
||||
json={
|
||||
"model": "mock-model",
|
||||
"messages": [{"role": "user", "content": "x"}],
|
||||
"stream": True,
|
||||
"stream_options": {"include_usage": "definitely"},
|
||||
"stream_options": {"include_usage": value},
|
||||
},
|
||||
)
|
||||
|
||||
assert response.status_code == 400
|
||||
assert response.json()["error"]["type"] == "invalid_request_error"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("value", ["yes", "true", "1", 1])
|
||||
def test_non_boolean_stream_is_rejected(client: TestClient, value: Any) -> None:
|
||||
"""`stream` picks between a JSON body and an SSE stream, so it must be exact."""
|
||||
response = client.post(
|
||||
"/v1/chat/completions",
|
||||
json={
|
||||
"model": "mock-model",
|
||||
"messages": [{"role": "user", "content": "x"}],
|
||||
"stream": value,
|
||||
},
|
||||
)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue