fix(waves): serialize STT query params passed through the escape hatch - #111
CaptainAni187 wants to merge 1 commit into
Conversation
build_stt_stream_query coerced booleans and lists only for params it names, so a knob given via additional_query_parameters reached the handshake as "True" or as a Python list repr. The controller compares === "true", so the knob silently did nothing. request_options["additional_query_parameters"] bypassed the same coercion. Key the coercion off the value's type instead of the param name, which covers both paths and the unnamed knobs the escape hatch exists for.
|
crim doesn't review pull requests automatically here. Comment |
|
crim review |
|
crim is reviewing this pull request. Findings will be posted shortly. |
Good to mergeThe old code trusted param names like a bouncer with a guest list; now it checks the type at the door, and the escape hatch finally serializes bools that actually say "true". Nicely done, with tests that even document the bug they fixed. |
There was a problem hiding this comment.
LGTM
One-sentence assessment: A correct, well-tested bug fix that closes a real serialization gap in the STT escape hatch.
What this PR does: It replaces the name-based coercion of STT query params (which only serialized booleans/lists for a hardcoded set of known param names) with type-based coercion. Now any value routed through additional_query_parameters or request_options["additional_query_parameters"] is shaped the same way as typed params: bools become "true"/"false" and lists/tuples become comma-joined strings. This fixes the previous behavior where an escape-hatch bool reached the wire as "True" (failing the controller's === "true" check) and a list reached it as a Python repr.
Findings: No issues found.
Notes verified: bool is a subclass of int, so the isinstance(value, bool) branch correctly precedes any numeric handling and does not mis-coerce ints. _coerce is private and all three call sites were updated to the new single-argument signature. Numeric/float params (e.g. vad_threshold, max_words) still pass through untouched, matching the added tests.
build_stt_stream_querynormalises the handshake params it names: booleans go out as"true"/"false"because the controller compares=== "true", andkeywords/redact_pii/redact_pcigo out comma-joined. Anything passed throughadditional_query_parametersskipped that and went onto the query string as-is.So the two paths disagree on the same knob:
punctuate=Trueis not"true", so punctuation stays off.keywordsarrives as a Python list repr, so keyword boosting matches nothing. Neither one errors, the socket opens, the session runs, the knob is just quietly ignored.request_options["additional_query_parameters"]bypasses the same coercion instream_speech_to_text, so both documented ways of passing raw params are affected.The change
_coercenow keys off the value's type rather than the param name, and runs over the escape-hatch params too.The two frozensets listed exactly the bool-typed and list-typed params in the signature, so switching to a type check is equivalent for every named param, and it also covers the params the SDK does not name yet, which is what the escape hatch is for. A knob the server adds tomorrow now serialises correctly without an SDK release. Net effect is less code.
The alternative was to keep the name lists and look up escape-hatch keys in them. That fixes the two knobs above but still sends a raw
Truefor anything newer than the SDK, which felt like leaving the bug half-fixed.Tests
Three added to
tests/custom/test_streaming_stt_helper.py, covering both bypass paths and an unnamed knob. All three fail onmainand pass here. The existing 12 in that file are unchanged and still pass, which is what pins the equivalence for named params.Both files are
.fernignored, so a regen keeps them.I did not have an API key to run this against the live Pulse endpoint, so the wire-format claim rests on the
=== "true"comparison your own module docstring documents. Happy to redo it against a real session if you want that.