Repository navigation
[Engine] reject a boolean or a fractional token limit with 400 (Fix #4037) - #4314
XiaoBinGan wants to merge 1 commit into
Conversation
743da95 to
c6cc453
Compare
… int() /v1/chat/completions and /v1/messages read their output cap with int(...), so "max_tokens": true became 1, 2.5 became 2 and false fell through to 0, which Service.prepare reads as the rest of the context. The request answered with a generation budget nobody asked for, silently. /v1/responses already refused these with "max_output_tokens must be a positive whole number"; this makes the two other routes agree through one helper, check_token_limit(), which also refuses a non-object body member the same way. Kept as before: whole numbers, the documented 0 and -1 sentinels (0 and -1: the rest of the context), an absent field, a whole-valued float such as 8.0, and a numeric string such as "192" that some clients send. The first field naming a non-zero cap still wins, as the `or` chain it replaces did.
c6cc453 to
1e0cdea
Compare
|
Rebased onto 0.2.7 ( max_req = max_new = check_token_limit(req, "max_completion_tokens", "max_tokens") # 0/-1: the rest
max_new = check_token_limit(req, "max_tokens") # 0/-1: the rest of the contextRe-checked on 0.2.7's The bug is unchanged in 0.2.7 — Which of my other PRs are still needed after 0.2.7, since I checked them together: #4312 (the |
|
Reproduced against the live server, not only the suite's So I did not run the fixed code against a live engine: this server is the user's, it is serving, and restarting it to load a patched Command, for repeating it (the key is read from the file this container keeps): KEY=$(cat /workspace/Strata/logs/api-key.txt)
curl -s http://127.0.0.1:8899/v1/chat/completions -H "Authorization: Bearer $KEY" \
-H 'Content-Type: application/json' \
-d '{"messages":[{"role":"user","content":"List three colours."}],"max_tokens":true,"temperature":0.7}' |
Problem and result
POST /v1/chat/completionswith"max_tokens": trueanswers one token,2.5answers two, andfalseanswers the whole context, because both routes convert the field withint(...):int(True)is 1,int(2.5)is 2, andFalsefalls through theorchain to 0, whichService.preparereads as the remaining context./v1/messagesdoes the same formax_tokens. Therequest therefore runs with a generation budget nobody asked for, and nothing says so.
/v1/responsesalready refuses the same values with"max_output_tokens must be a positive whole number", so the three routes disagreed.After this change both routes answer HTTP 400 and name the field, and
/v1/responsesbehaves as itdid. Whole numbers, the documented
0/-1sentinels, an absent field, a whole-valued float(
8.0) and a numeric string ("192", which some clients send) keep working.Related discussion: #4037 — the report, with the lines (
serve/server.py:7148and:7396at thecommit it was filed against).
Approach
One helper next to
check_request_sampling, which the same two handlers already call for the samekind of 400:
The two call sites become
check_token_limit(req, "max_completion_tokens", "max_tokens")andcheck_token_limit(req, "max_tokens"). The first field naming a non-zero cap still wins, as theorchain it replaces did, so nothing changes for a request that sends both. A
ValueErroris already theestablished 400 on these paths (
stop_strings,check_request_sampling, a badtool_choice), so noerror plumbing was touched.
serve/responses.pyis not changed: it already validates and aResponsesErrorcarries its own field name.Validation
Passed:
python -m unittest serve.test_request_hardening -v(Python 3.11.16, from a source checkout):11 tests, OK, including the new
test_a_boolean_or_fractional_token_limit_is_a_400. It poststrue,2.5andfalseasmax_tokenson/v1/chat/completionsand/v1/messages, and asmax_completion_tokenson chat, and asserts 400 with the field named; then posts8,8.0,"8",0,-1and{max_completion_tokens: 8, max_tokens: 4}and asserts 200.Passed: no regression in the neighbours that share these handlers:
serve.test_responses+serve.test_security69 tests, OK (1 skipped);serve.test_request_accounting+serve.test_effort_paths+serve.test_vision_token_limit30 tests, OK.
Failed: none.
Not run: the same requests against a live engine: every check above uses the suite's
MockEngine,which is what the existing hardening tests use. The code path is the request parser, before any engine
call, so a real model would only repeat the same 400; I did not run it against a model and I am not
claiming I did.
Not run:
python -m pytest tests/terminaland theapps/hostUI tests: no file in those trees istouched.
Limits
8.0) is accepted, becauseint(8.0)changes nothing; a fractional one isrefused. That is the line the issue drew ("reject boolean and fractional token limits").
"2.5"is a 400 (it was aValueErrorfromint()before, sothe answer is the same) but the message now names the field.
-1are not refused here. The issue asked to preserve the documentedsentinels, not to add range checks, and
Service.preparealready treats<= 0as "the rest".