Skip to content

fix: remove redundant return in finally block (voice processing) - #23709

Closed
vanthinh6886 wants to merge 1 commit into
NousResearch:mainfrom
vanthinh6886:fix/return_in_finally_voice
Closed

fix: remove redundant return in finally block (voice processing)#23709
vanthinh6886 wants to merge 1 commit into
NousResearch:mainfrom
vanthinh6886:fix/return_in_finally_voice

Conversation

@vanthinh6886

Copy link
Copy Markdown
Contributor

Problem

_voice_stop_and_transcribe() in cli.py has a return statement inside a finally block (line 9303). In Python, return in finally silently swallows any exception being propagated — the exception is discarded and the function returns normally.

Python 3.14 will emit SyntaxWarning for this pattern:

SyntaxWarning: 'return' in a 'finally' block

Fix

Remove the redundant return statement. The return was intended to prevent voice auto-restart when the no-speech limit (3 consecutive) is reached, but this guard is already redundant:

# Two lines before the removed return:
self._voice_continuous = False  # ← disables continuous mode

# The auto-restart condition checks this flag:
if self._voice_continuous and not submitted and not self._voice_recording:
    # This block is unreachable when _voice_continuous is False

Setting _voice_continuous = False already makes the subsequent auto-restart condition evaluate to False, so the return was unnecessary. Removing it preserves identical control flow while eliminating the exception-swallowing bug.

Before vs After

Aspect Before After
Exception in try/except Silently swallowed by return in finally Propagated normally
Auto-restart when no-speech limit reached Blocked by return Blocked by _voice_continuous = False
Python 3.14 compatibility SyntaxWarning No warning

Tests

  • Verified syntax with ast.parse()
  • AST scanner confirms no return in finally remains
  • 1 line removed, 0 lines added

The `_voice_stop_and_transcribe()` method in cli.py had a `return`
statement inside a `finally` block (line 9303). In Python, `return`
in `finally` silently swallows any exception being propagated — the
exception is discarded and the function returns normally. Python 3.14
will emit SyntaxWarning for this pattern.

The `return` was intended to prevent the voice auto-restart logic
from running when the no-speech limit (3 consecutive) is reached.
However, this guard is already redundant: the code sets
`self._voice_continuous = False` two lines before, which makes the
subsequent auto-restart condition (`self._voice_continuous and not
submitted`) evaluate to False. Removing the `return` preserves the
same control flow while eliminating the exception-swallowing bug.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists labels May 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #21100 (and #23654, #21261). Same fix: remove return in finally block in _voice_stop_and_transcribe for Python 3.14+ SyntaxWarning.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused cleanup. The premise still holds on current main: cli.py:11328 returns from the finally block, while cli.py:11325 clears _voice_continuous before the restart guard at cli.py:11336.

Suggested changes

  • Consider adding a regression test for the third consecutive silent cycle. Current coverage tests a single restart at tests/tools/test_voice_cli_integration.py:1251-1258, but not that the third silence disables continuous mode and prevents another restart.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

The return-in-finally restart race was fixed in #73520 via @brunopirz's #52004 (co-credit @liuhao1024's earliest #21100); this stale variant is superseded. Thanks!

(Landed via #73520, merge e04c2a9ebd.) Closing.

@teknium1 teknium1 closed this Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants