Reject booleans and non-finite numbers in the chat timing fields instead of coercing them

[swinney]

Objective

Reject a JSON boolean supplied for client_sent_msg_ts or client_timeout with HTTP 400, instead of silently treating true as 0.001 and false as "field omitted".

Context you need

src/interfaces/chat_app/request_validation.py normalizes both timing fields through _milliseconds_to_seconds, which guards the division against OverflowError and TypeError. bool is a subclass of int in Python, so a boolean satisfies neither guard and sails through:

$ python -c "from src.interfaces.chat_app.request_validation import parse_client_timeout as p; print(p(True), p(False))"
0.001 0

Measured on dev at dd3fb6b4 — all four combinations are accepted, none raises:

payload	result	consequence
"client_timeout": true	0.001	a 1-millisecond deadline. Any request that also supplies client_sent_msg_ts is past it on arrival, so _prepare_chat_context returns 408; on the streaming route the in-stream check ends the stream with an in-band 408 almost immediately.
"client_timeout": false	0	silently identical to omitting the field
"client_sent_msg_ts": true	0.001	persisted as 1970-01-01T00:00:00.001Z — indistinguishable from a real measurement at a glance, and not the documented 1970-01-01T00:00:00Z absent-value sentinel
"client_sent_msg_ts": false	0	silently identical to omitting the field

true is the harmful one: a client that serializes a flag into the wrong field gets 408 on every request with no indication that the field was the problem.

Why this is not already covered. The guards catch what cannot be divided (OverflowError, TypeError); a boolean divides fine. The falsey guard if not value: return 0 runs first, so False never reaches the division at all — a type check placed after it would miss false entirely. Put the boolean check before the falsey guard.

How it was found. Adversarial review of PR for #194 (docs), which had claimed "anything not numeric is rejected". That claim was corrected to name only strings, arrays and objects — the types actually rejected — so the documentation is currently accurate about a contract that has this hole. Closing the hole lets the docs say "a boolean" too.

Not a regression. This behaviour predates #175/#185; those changes added the guards that made every other non-numeric type a 400 and left this one gap.

Constraints

• Branch from origin/dev. PR with gh pr create --repo fasrc/archi --base dev. Not upstream/dev. Never commit to dev directly.
• TDD: add the failing endpoint tests first, watch them fail with a 200/408 rather than 400, then change request_validation.py.
• bash scripts/gate.sh must pass, run bare (no pipe, no redirect), with ≥80% diff coverage on changed lines. Never --no-verify.
• No Co-Authored-By or AI-attribution trailers. Short lowercase commit subject.
• Update docs/docs/api_reference.md in the same change — both timing-field rows list the rejected types, and a boolean belongs in that list once it is rejected. Do not renumber any [name]: …/app.py#Lnnn anchor (that is Make the api_reference.md app.py line anchors self-verifying #190).
• Uses OpenSpec: this changes a documented request contract, so add a delta to chat-api-request-contract rather than editing a main spec in place.
• Never merge. A human merges.

Plan

1. Add failing cases to tests/unit/test_chat_timing_field_validation.py::TestNormalizationItselfCannotRaise — its CASES list and _post harness already drive both routes with a real JSON body. Add (field, True) and (field, False) for both fields; all four must fail before the fix.
2. In _milliseconds_to_seconds, reject isinstance(value, bool) before the if not value guard, raising InvalidClientTiming with a message naming the field, consistent with the existing wording.
3. Confirm no legitimate caller sends booleans: grep -rn "client_timeout\|client_sent_msg_ts" src/interfaces/chat_app/static/ src/interfaces/chat_app/openai_compat.py — the JS client sends Date.now() and a number, and openai_compat.py synthesizes now.timestamp().
4. Add the OpenSpec delta and add "a boolean" to the rejected-type lists in both api_reference.md rows.

Commands

# reproduce (expect: 0.001 0)
python -c "from src.interfaces.chat_app.request_validation import parse_client_timeout as p; print(p(True), p(False))"

# red first, then green
python -m pytest tests/unit/test_chat_timing_field_validation.py -q

bash scripts/gate.sh
openspec validate <change-name> --strict

Acceptance criteria

• python -c "from src.interfaces.chat_app.request_validation import parse_client_timeout as p; p(True)" exits non-zero with InvalidClientTiming; same for p(False), parse_client_sent_msg_ts(True), parse_client_sent_msg_ts(False).
• Both chat routes return 400 for a boolean in either timing field, and the error names the field.
• A real number is still accepted: parse_client_timeout(600000) == 600.0 and parse_client_sent_msg_ts(1700000000000) == 1700000000.0.
• An omitted field still yields 0 — parse_client_timeout(None) == 0. Rejecting false must not break omission.
• grep -c boolean docs/docs/api_reference.md ≥ 1.
• git diff origin/dev -- docs/docs/api_reference.md | grep -c '#L[0-9]' returns 0.
• bash scripts/gate.sh passes with ≥80% diff coverage; openspec validate <change> --strict passes.

Start here

Run the reproduce command above and confirm it prints 0.001 0. Then read _milliseconds_to_seconds and note that the falsey guard precedes the division — that ordering is why the check has to go first.

---

Found by adversarial review of the #194 docs PR. Related: #190 (anchor drift), #193 (nullable timing column), #194 (the docs rows this would let us strengthen).

[swinney]

Widening this issue: non-finite numbers reach the same hole, and one of them is worse than the boolean.

Found in round 2 of the adversarial review on the #194 docs PR. _milliseconds_to_seconds guards against OverflowError and TypeError, but inf / 1000 is inf — no exception — so a literal large enough to decode as infinity passes straight through. Measured on dev at dd3fb6b4:

$ python -c "import json; print(json.loads('1e309'), json.loads('-1e309'))"
inf -inf

payload	parse_client_timeout	consequence at the deadline check
"client_timeout": 1e309	accepted, inf	elapsed > inf is always False → deadline silently disabled
"client_timeout": -1e309	accepted, -inf	elapsed > -inf is always True → 408 immediately, on every request
"client_timeout": NaN-ish	accepted, nan	every comparison False → deadline silently disabled

client_sent_msg_ts is not exposed to this: inf, -inf and nan all fail its datetime.fromtimestamp range check and return 400 already. Verified. So the non-finite gap is client_timeout-only, while the boolean gap is on both fields.

-1e309 is the sharp one — a client that formats a timeout in exponent notation and overshoots gets a hard 408 on every request, with nothing in the error naming the field.

This does not change the fix location or the shape of the fix, which is why it belongs here rather than in a separate issue: one guard in _milliseconds_to_seconds, before the falsey check, rejecting anything that is not a finite real number. math.isfinite covers inf/-inf/nan; isinstance(value, bool) covers the boolean. Both raise InvalidClientTiming naming the field.

Additions to the plan above

• Step 1 (tests): add 1e309, -1e309 and a NaN literal for client_timeout alongside the boolean cases. All must fail before the fix — note that -1e309 fails differently from the others (it produces a 408, not a 200), so assert on the 400 specifically rather than on "not 200".
• Step 2 (code): reject non-finite as well as boolean. Keep both checks ahead of if not value.
• Step 4 (docs): the client_timeout row currently discloses both gaps and links here — once this lands, replace that disclosure with the enforced contract. The client_sent_msg_ts row only needs the boolean clause removed.

Additional acceptance criteria

• parse_client_timeout(float("inf")), parse_client_timeout(float("-inf")) and parse_client_timeout(float("nan")) all raise InvalidClientTiming.
• parse_client_timeout(10**300) == 1e+297 still — a large finite value stays accepted, since the field is only compared against an elapsed interval.
• grep -c "1e309" docs/docs/api_reference.md returns 0 — the disclosure is gone because the behaviour is fixed.

[swinney]

Third widening, and the last — round 3 of the #194 review found the remaining cases, and together they show the real shape of the defect.

The root cause is one line's ordering, not a list of bad types. _milliseconds_to_seconds opens with if not value: return 0, so every falsey JSON value is silently reinterpreted as "field omitted" before any type or numeric check runs. Everything that survives that line is then accepted if value / 1000 does not raise. The guard validates divisibility, not type — which is why each round of review found another value that slips through.

Full matrix, measured through the real route (FlaskAppWrapper.get_chat_response with a raw JSON body) on dev at dd3fb6b4:

"client_timeout":	outcome	note
omitted / null / 0 / false	200, timeout 0	intended: no deadline declared
"" / [] / {}	200, timeout 0	falsey → silently read as omitted
"600000" / [600000] / {"a":1}	400	intended
600000	200, timeout 600.0	intended
true	200, timeout 0.001	1 ms deadline → nearly every request 408s
1e309	200, timeout inf	deadline disabled
NaN	200, timeout nan	deadline disabled (every comparison is False)
-1e309	200, timeout -inf	deadline expires immediately

Two additions to what is already described above:

1. Empty containers and the empty string are treated as omission. "", [], {} are falsey, so they never reach the division. A client that serializes an unset timeout as "" gets no deadline and no error — and the same three values on client_sent_msg_ts produce the epoch sentinel as though the field had been left out.
2. NaN disables the deadline too. Python's json accepts the bare NaN literal (it is not standard JSON, but Flask's loader takes it), nan / 1000 is nan, and elapsed > nan is False at both comparison sites. Same effect as inf.

Revised fix

One guard, ahead of the falsey check, accepting only a real finite number:

• reject isinstance(value, bool) — bool is an int subclass
• reject non-finite via math.isfinite
• treat only None (and an absent key) as omission; a falsey non-number is a bad value, not an absent one

Keeping 0 and false-as-omission is a decision to make explicitly rather than inherit: 0 is a legitimate "no deadline" spelling that the current docs describe, so preserve it; "", [], {} have no such justification.

Additional acceptance criteria

• "", [], {} in either timing field return 400, not 200.
• None / an absent key still yields 0, and 0 itself still yields 0.
• parse_client_timeout(float("nan")) raises InvalidClientTiming, alongside inf and -inf.
• A test drives the real route with a raw JSON body for at least "", true, NaN and -1e309 — the existing tests/unit/test_chat_timing_field_validation.py::_post harness does this; passing Python values to the parsers directly would miss that NaN is reachable only through the JSON decoder.
• grep -cE "falsey|1e309|NaN" docs/docs/api_reference.md returns 0 once the behaviour is enforced — the disclosure paragraphs come out when they stop being true.

[swinney]

Final addition — client_timeout accepts negative values, and that is the most plausible way a real client hits this.

parse_client_timeout(-1000)  ->  -1.0     # accepted
elapsed > -1.0               ->  True     # at both comparison sites, always

So "client_timeout": -1000 yields 408 on every request, same as -1e309, but at a magnitude a client could reach by accident — a subtraction that went the wrong way, a clock skew, a "remaining budget" computed after the budget was spent. Nothing in the 408 response names the field.

A fractional value (1.5 → 0.0015 s) is also accepted. That one is arguably legal rather than a bug — a sub-millisecond deadline is meaningless but not ill-formed — so treat it as a decision to make, not a defect to fix: either accept it silently as now, or require an integer.

Scope note, to keep this issue's fix from sprawling. A negative or fractional client_sent_msg_ts is not a defect and must keep working: within years 1–9999, -1000 is 1969-12-31T23:59:59Z and 1700000000123 carries ordinary millisecond precision. The sign check belongs to client_timeout only — a negative duration is meaningless, a negative instant is not. Do not add a shared sign guard to _milliseconds_to_seconds.

Additional acceptance criteria

• parse_client_timeout(-1000) and parse_client_timeout(-1) raise InvalidClientTiming.
• parse_client_sent_msg_ts(-1000) still returns -1.0, and parse_client_sent_msg_ts(1700000000123) still returns 1700000000.123 — the sign guard must not leak onto this field.
• Whatever is decided about a fractional client_timeout is recorded in the change's spec delta, so the next reader knows it was a choice.

[swinney]

🤖 Picked up by the nightly run on 2026-08-05. Will open a PR; a human reviews before merge.