fix: validate model output during warmup - #132
Conversation
flyworker
left a comment
There was a problem hiding this comment.
Reviewed and tested this against the two real backends on my node plus some constructed cases. The direction is right and the shape is good — scoping the checks to the synthetic greeting only, recognising both reasoning and reasoning_content, reusing checkForOpenAIError, and adding stream: false are all the right calls. Builds clean, go vet clean, both new tests pass.
One finding worth addressing before merge.
finish_reason: "length" is doing all the work, and it cannot tell "never stops" from "chatty"
I ran the exact reported failure through validateWarmupResponse:
content: ", I'm looking for a good hotel in Paris. Can you recommend one? Hello! Paris is a beautiful city and there are many"
→ CAUGHT: warmup exhausted the 64-token budget
It is caught — but only by the length rule. That text has no ChatML delimiters and no user: at the start of a line, so neither the marker check nor warmupRoleBoundary fires. The length check is the sole detector for the bug this PR exists to fix.
Now the same rule on a verbose but perfectly healthy reply:
content: "Hello! It's very nice to meet you. I'm an AI assistant, and I'm here to help with all sorts of
things — answering questions, writing and editing, working through code, explaining tricky concepts,
or just talking something over. Is there anything particular"
→ REJECTED: warmup exhausted the 64-token budget
Same rule, same message, opposite meaning. 64 tokens is roughly 45–50 words, and an instruct model with a preamble reaches that on a greeting without anything being wrong.
This matters more than a local log line, because handleWarmup returning an error becomes Success: false with the error text sent back to the platform — a claim about the operator's model, not just a note in their logs.
For calibration, both correctly-configured backends here answer "Hi" in about 8 tokens and finish with stop, so they pass comfortably:
Cydonia-24B-v4.3 (vLLM): finish_reason=stop, "Hello! How can I help you today?"
Qwen3.8-27B (llama.cpp): finish_reason=stop, "Hello! How can I help you today?"
The headroom is real for typical models. It is the verbose tail that gets caught.
Suggestion
A model that never stops will exhaust any budget; a verbose one will not. That difference is directly testable, so where the content is otherwise clean — no delimiters, no role boundary — consider re-probing once with a much larger budget (512 or so) and failing only if it hits the limit again. A well-behaved model finishes and passes; a broken template runs to the wall twice and is rejected with a stronger claim than a single truncation supports.
That keeps the detector that actually catches #131 while removing the class of false positive, at the cost of one extra request on a path that should be rare.
Two smaller things:
- With multiple choices,
warningis overwritten by each iteration, so only the last is reported. Minor, and probably never hit withn=1. - A refusal that hits the budget (
finish_reason: length, empty content, non-emptyrefusal) is rejected as a template problem by thecontent+refusal != ""branch. Also an edge case, but the diagnosis would be wrong.
flyworker
left a comment
There was a problem hiding this comment.
Re-reviewed after 283304a. This resolves the finding, and I re-ran the same cases against the new logic to confirm rather than assume:
| case | before | now |
|---|---|---|
| verbose but valid, 64-token cutoff | rejected | retry at 512 → passes |
| the runaway from #131 | rejected | retry at 512 → still truncated → rejected |
| runaway with a ChatML marker | rejected | rejected immediately, no retry |
runaway with a user: line |
rejected | rejected immediately, no retry |
| healthy short reply | passes | passes, no retry |
The false positive is gone and the bug this PR exists to catch is still caught. The ordering is right too: the marker and role-boundary checks run before the length branch, so an obviously broken template is rejected on the first probe and never spends the extra request. The retry only fires for a clean completion that happened to hit the wall, which is the one case that genuinely cannot be told apart without asking again.
The sentinel error is the right shape for this — the validator reports what it saw and the caller decides whether that warrants a second probe, rather than the validator knowing about retry policy. The test asserting the budgets were exactly [64, 512] is the detail I would have asked for.
Verified locally: go build ./..., go vet, go test ./... (8/8 packages) and go test -race ./internal/computing all clean on the branch.
One behaviour change worth naming, not blocking:
The reasoning-model exemption is gone. Previously a model that spent the whole budget on hidden reasoning produced a warning and passed; now it retries and is rejected if still truncated at 512. I think that is the better behaviour — 512 tokens to answer "Hi" is generous, and a reasoning model that cannot is worth surfacing — but it is a deliberate change from the earlier version of this PR rather than a consequence of the retry, so it is worth being sure it is intended. If a heavy reasoning model does trip it in the field, the fix is a larger retry budget rather than restoring the exemption, since the exemption also let a genuinely broken reasoning backend through.
Trivial: sendWarmup mutates the shared warmupRequest map rather than copying it. Harmless with one sequential caller, just noting it.
Approving.
Closes #131.
A backend with a broken chat template could pass warmup because the one-token limit guaranteed a cutoff and the provider discarded the completion. Warmup now allows 64 tokens and validates the response before reporting success. It rejects leaked ChatML/Llama delimiters, generated
user:/assistant:lines, invalid roles, empty completions, and error bodies returned with HTTP 200.If an otherwise clean completion reaches the 64-token limit, the provider re-probes it once with 512 tokens. A verbose healthy model can finish the larger probe and pass; a model that reaches both limits is rejected. Hidden reasoning is recognized through both
reasoningandreasoning_contentwhen checking whether a completion is empty.Validation:
go test ./...go test -race ./internal/computinggo vet ./internal/computing