Reject malformed transcription temperature instead of reinterpreting it - #4553
Open
Daksha1611 wants to merge 1 commit into
Open
Reject malformed transcription temperature instead of reinterpreting it#4553Daksha1611 wants to merge 1 commit into
Daksha1611 wants to merge 1 commit into
Conversation
parseTemperature() parsed the multipart temperature field with ovms::stof
and, when that failed, retried with ovms::stou32. ovms::stof requires the
whole field to be consumed; ovms::stou32 does not. The fallback therefore
rescued precisely the values stof had just rejected, converting malformed
input into a plausible temperature rather than returning the intended
"Invalid temperature type." error:
temperature=0.5x -> accepted as 0.0 (not 0.5)
temperature=0abc -> accepted as 0.0
temperature=1e400 -> accepted as 1.0 (stof throws out_of_range,
stou32 parses the leading 1)
temperature=5 5 -> accepted as 55.0 (stou32 erases spaces first)
The fallback had no legitimate purpose: ovms::stof already parses integer
spellings, so "1" was never reaching it. Drop it and let the existing error
path fire.
This deliberately does not add a range check. Passing a negative temperature
through for GenAI to reject is the behaviour established by openvinotoolkit#4201 ("Negative
temperature should lead to genai error in STT path") and pinned by
negativeTemperatureEnableSampling, so it is left alone.
Tests: adds four cases covering the malformed spellings above. The three
existing SttServableParseTemperatureTest cases (-1.0, 0, 1.0) are unaffected -
all three parse cleanly through ovms::stof and never reached the fallback.
3 tasks
Daksha1611
marked this pull request as ready for review
September 12, 2026 07:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠 Summary
Fixes #4552.
parseTemperature()parsed the multiparttemperaturefield withovms::stofand, on failure, retried withovms::stou32.ovms::stofrequires the whole field to be consumed;ovms::stou32does not, and it erases spaces first. The fallback therefore rescued exactly the valuesstofhad just rejected, turning malformed input into a plausible temperature instead of the intendedInvalid temperature type.error:The fallback had no purpose —
ovms::stofalready parses integer spellings, so"1"never reached it. Every input it could rescue is malformed by definition. Removing it lets the existing error path fire.This deliberately adds no range check. Passing a negative temperature through for GenAI to reject is the behaviour established by #4201 and pinned by
negativeTemperatureEnableSampling, so I left it alone rather than quietly reverse it. Happy to revisit separately if the endpoint should enforce the OpenAI audio range at the parse layer.Adds four cases for the spellings above. The three existing
SttServableParseTemperatureTestcases are unaffected —-1.0,0and1.0all parse cleanly throughovms::stofand never reached the fallback.The root cause is that
ovms::stou32alone among thestringutilsconverters does not check for a partial parse; that is #4554, and the two fixes are independent.I have no OVMS build container available, so this is not compiled against the full tree and CI will need to confirm the build. I did check the change in isolation and verified the three existing test inputs behave identically. Draft for that reason.
🧪 Checklist