fix(voice): stop the directive turning an announcement into the answer - #5550
fix(voice): stop the directive turning an announcement into the answer#5550YellowSnnowmann wants to merge 1 commit into
Conversation
Asking for a spoken sentence before tool use made that sentence the whole reply. A message carrying only text and no tool call is the end of a turn, so a model that dutifully said "let me pull up your inbox - I'll drop the summary in your chat" ended there, and the harness delivered it as the final answer. The caller got a promise and no summary. Seen live, with the model echoing the directive's own example almost verbatim. The clause existed to get audio out early, back when the relay's filler was inaudible until the turn closed. That is the relay's job now, and it does it without depending on the model choosing to speak - so the directive says the opposite: call the tool, answer from the result, and do not narrate the intention. Also clears three comments that still described the preface as a thing that happens.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe voice relay now provides acknowledgements independently. ChangesVoice directive behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized voice-directive change prevents a tool-backed turn from ending with only an announcement and preserves answering from the tool result; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
CodeGhost21
left a comment
CodeGhost21
left a comment
There was a problem hiding this comment.
Approving. This is the highest-value change of the four and the mechanism is convincing.
The core argument — a reply carrying only text and no tool call is the end of a turn, so an instruction to announce work before doing it makes the announcement the final answer — is correct, and the failure mode it produces is the nasty kind: the more faithfully the model follows the instruction, the more reliably the user gets nothing. The delivered text matching the directive's own worked example almost verbatim is strong evidence, and I appreciate that the description says plainly that a transcript proving zero tool calls on that turn was not captured, rather than overstating it. Given this is prompt text with no API or schema surface, that level of confidence is proportionate to the risk — it is trivially revertible if the read turns out to be wrong.
Two things done right that are easy to get wrong:
The replacement explains itself rather than just prohibiting. "Do NOT announce what you are about to do: a reply that only says what you are going to do ends your turn…" gives the model the reason, which is what stops it substituting a hedge for the banned behaviour. A bare prohibition here would likely have produced a different flavour of the same stall.
The removed clause's original justification was retired, not ignored. The preface existed to get audio out early while the relay's filler was inaudible; that is no longer true now the relay speaks a terminated sentence at ~700ms, and crucially the relay does it without depending on the model choosing to speak. Removing the clause without that having changed would have reintroduced the silence. The doc comment above VOICE_ACK_DEADLINE_SECS was updated to say so, so the next reader will not "restore" it.
Non-blocking notes:
The test is deliberately brittle, which I think is right here. the_directive_forbids_announcing_work_instead_of_doing_it asserts on the literal substring "Do NOT announce" plus two banned phrases. Any reword of the directive breaks it. That is the correct trade for a constant whose exact wording is the behaviour — but it is worth a one-line comment saying so, or the next person will "fix" the test by loosening the assertion, which would let the regression back in.
One stale phrase left behind. The doc block above VOICE_HANDOFF_LINE still reads "Both delivery paths honour the promise" while this PR removes promise language elsewhere. Cosmetic, and #5549 rewrites that exact line anyway, so it resolves itself on merge.
On the red check: Rust Feature-Gate Smoke (gates off) is unrelated to this PR. The Rust tests pass; the job fails afterwards at the dependency-graph ratchet with dep-sim: FAIL — expected 281 names, got 279. main is red with the identical failure, as are unrelated open PRs — .github/workflows/ci-lite.yml:518 pins --expect-names 281 after the graph lost two names, and needs bumping on main separately.
Merge ordering: this and #5549 both rewrite the doc block above VOICE_HANDOFF_LINE and insert a test at the same anchor in mod tests, so they conflict — whichever lands second needs a rebase.
|
The red check here is Rust Feature-Gate Smoke (gates off), and it's not from this PR — it's a pre-existing break on Every test in that job passes; the step exits 1 later, on the dependency-count guard:
Evidence it's on
Fix belongs on |
2 participants
Summary
Problem
Asking for a summary produced no summary. The final answer delivered to chat was:
That is the directive's own example, echoed almost verbatim:
The mechanism: a reply carrying only text and no tool call is the end of a turn. The directive said "first say one short spoken sentence… then proceed", so a model that complied emitted the sentence, ended its turn, and the harness delivered that sentence as the final answer. The caller got a promise and never got the inbox. The more obediently the model followed the instruction, the more reliably it failed.
Solution
The clause existed for a latency reason that no longer holds. It was there to get audio to the caller early, back when the relay's spoken filler was inaudible until the turn closed (tinyhumansai/backend#1275 — an ellipsis-terminated filler is buffered by the provider, which segments on sentence boundaries). The relay now speaks a terminated sentence ~700ms in, and it does so without depending on the model choosing to speak first.
So the directive now says the opposite, and says why, because a bare prohibition invites the model to hedge instead:
Submission Checklist
N/A: behaviour-only change## Related—N/A: no matrix rows affectedN/A: no release-cut surface changeCloses #NNN— see## RelatedImpact
Desktop only; prompt text on the voice path. No API, schema, or storage change.
Expect less speech early in a tool-backed turn, not more: the model no longer says anything until it has something to say, and the acknowledgement comes from the relay instead. That is the intended trade — an early sentence from the model costs the answer itself.
Not fixed here: the ~8s before a first token, which is the desktop rebuilding its orchestrator per turn. This PR is about the turn producing an answer at all.
Confidence: the mechanism is inferred from the delivered text matching the directive's example almost word for word, plus the fact that a text-only reply terminates a turn. I have not captured a transcript proving the model emitted zero tool calls on that specific turn — that would need the desktop log from the failing call.
Testing
GGML_NATIVE=OFF cargo test --features voice --lib openhuman::voice::realtime_harness— 15 passedcargo clippy --features voice --lib,cargo fmt --checkcleanRelated
Closes #5552
Relates to #5399. Pairs with tinyhumansai/backend#1275 (the relay filler that makes the preface unnecessary) and #5549.
Summary by CodeRabbit
Bug Fixes
Tests