Skip to content

[Engine] reject a boolean or a fractional token limit with 400 (Fix #4037) - #4314

Open
XiaoBinGan wants to merge 1 commit into
Niko1221:mainfrom
XiaoBinGan:fix/token-limit-types
Open

XiaoBinGan wants to merge 1 commit into
Niko1221:mainfrom
XiaoBinGan:fix/token-limit-types

Conversation

@XiaoBinGan

@XiaoBinGan XiaoBinGan commented Oct 27, 2026 •

Copy link
Copy Markdown

Problem and result

POST /v1/chat/completions with "max_tokens": true answers one token, 2.5 answers two, and
false answers the whole context, because both routes convert the field with int(...):
int(True) is 1, int(2.5) is 2, and False falls through the or chain to 0, which
Service.prepare reads as the remaining context. /v1/messages does the same for max_tokens. The
request therefore runs with a generation budget nobody asked for, and nothing says so.
/v1/responses already 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/responses behaves as it
did. Whole numbers, the documented 0 / -1 sentinels, 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:7148 and :7396 at the
commit it was filed against).

Approach

One helper next to check_request_sampling, which the same two handlers already call for the same
kind of 400:

def check_token_limit(req, *names) -> int:
    for name in names:
        value = req.get(name)
        if value is None:
            continue
        if isinstance(value, bool):
            raise ValueError(f"{name}: a whole number of tokens (true and false are not)")
        ...
        if value == 0:
            continue        # 0 is "the rest of the context": the next field, or no cap, decides
        return value
    return 0

The two call sites become check_token_limit(req, "max_completion_tokens", "max_tokens") and
check_token_limit(req, "max_tokens"). The first field naming a non-zero cap still wins, as the or
chain it replaces did, so nothing changes for a request that sends both. A ValueError is already the
established 400 on these paths (stop_strings, check_request_sampling, a bad tool_choice), so no
error plumbing was touched. serve/responses.py is not changed: it already validates and a
ResponsesError carries 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 posts
true, 2.5 and false as max_tokens on /v1/chat/completions and /v1/messages, and as
max_completion_tokens on chat, and asserts 400 with the field named; then posts 8, 8.0,
"8", 0, -1 and {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_security 69 tests, OK (1 skipped);
serve.test_request_accounting + serve.test_effort_paths + serve.test_vision_token_limit
30 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/terminal and the apps/host UI tests: no file in those trees is
touched.

Limits

  • A whole-valued float (8.0) is accepted, because int(8.0) changes nothing; a fractional one is
    refused. That is the line the issue drew ("reject boolean and fractional token limits").
  • A numeric string is still read, so "2.5" is a 400 (it was a ValueError from int() before, so
    the answer is the same) but the message now names the field.
  • Negative values other than -1 are not refused here. The issue asked to preserve the documented
    sentinels, not to add range checks, and Service.prepare already treats <= 0 as "the rest".

… 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.
@XiaoBinGan

Copy link
Copy Markdown
Author

Rebased onto 0.2.7 (main at 5e8a4cc): serve/server.py moved by about 450 lines in the release, so the old branch no longer applied cleanly. Same change, re-applied to the file as main has it now — two call sites become check_token_limit(), and the helper is unchanged:

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 context

Re-checked on 0.2.7's serve/server.py, not on the version the issue was filed against:

python -m unittest serve.test_request_hardening     → 11 tests, OK
python -m unittest serve.test_responses serve.test_security → 73 tests, OK (1 skipped)

The bug is unchanged in 0.2.7 — int(req.get("max_completion_tokens") or ...) was still at serve/server.py:7603 and int(req.get("max_tokens") or 0) at :7870 before this branch, and no other int(req.get(...)) coercion is left in the file.

Which of my other PRs are still needed after 0.2.7, since I checked them together: #4312 (the [ -x ] probe in bin/strata and docker/entrypoint.sh), #4316 (no seccomp entry in docs/DOCKER.md), #4317 (setup_args() still forwards the preset's lower-case tag) and #4318 (the whole-shard mark is still unprefixed by a pin) are all unchanged on main too. #4315 is not: 0.2.7's new Catalog.model_folder() fixes that issue, so I closed it rather than leave a duplicate.

@XiaoBinGan

Copy link
Copy Markdown
Author

Reproduced against the live server, not only the suite's MockEngine — since a reviewer will ask whether the mock is representative. Same container (0.2.2 image, engine 0.1.47, RTX 5090), real generation, curl over loopback:

chat, max_tokens: true   → finish: length | completion_tokens: 1 | content: None
chat, max_tokens: 2.5    → finish: length | completion_tokens: 2 | content: None
chat, max_tokens: 8      → finish: length | completion_tokens: 8
/v1/messages, max_tokens: true → stop_reason: max_tokens | usage: {input_tokens: 1, output_tokens: 1, ...}
/v1/responses, max_output_tokens: 2.5 → HTTP 400
    {"error": {"message": "max_output_tokens must be a positive whole number", ... "param": "max_output_tokens"}}

So true answered one token, 2.5 answered two, the Anthropic route did the same with stop_reason: max_tokens, and the Responses route (the one that was already right) refused the same value with a 400 that names the parameter. A whole number behaves normally in the same run.

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 serve/server.py would interrupt it. The fix's own evidence is the suite (11 tests in serve.test_request_hardening, plus serve.test_responses and serve.test_security at 73 tests OK on 0.2.7), and the code path is the request parser, before any engine call — which is why the mock and the live server agree here on everything except the number of tokens the engine was allowed to generate.

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}'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant