Skip to content

Code review: fix TTS response-pipe bug, dedupe transcription finishing, trim hot-path I/O - #157

Merged
JRufer merged 14 commits into
masterfrom
claude/ecstatic-curie-azrhcw
Sep 26, 2026
Merged

JRufer merged 14 commits into
masterfrom
claude/ecstatic-curie-azrhcw

Conversation

@JRufer

@JRufer JRufer commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Full code-review pass over the Rust workspace and the Svelte frontend, focused on correctness, speed and reuse.

Important

One-time setup is needed before merging. Every push to master runs the release workflow, and that workflow now refuses to build or publish without the update-signing key. Generate the key and add a repository variable and a secret first. It takes about two minutes; see docs/release-signing.md:

minisign -G -W -p voxctrl-update.pub -s voxctrl-update.key
  • variable VOXCTRL_UPDATE_PUBKEY: the second line of voxctrl-update.pub
  • secret VOXCTRL_UPDATE_SIGNING_KEY: the contents of voxctrl-update.key

Security

Releases are signed, and the updater verifies the signature before installing.

  • The gap: the updater only compared downloads with the SHA-256 digest GitHub reports for the same upload. That catches corruption, not tampering, because whoever can replace a release asset replaces its digest too.
  • Signing: the publish job signs every asset with minisign and uploads <asset>.minisig beside it. Before publishing, it verifies each signature against the public key, so a mismatched key pair fails the release.
  • Verifying: release builds bake in the public key (via voxctrl-update/build.rs). After the digest check, the updater stream-verifies the download against its signature before putting it in place.
  • Replay protection: the signed trusted comment names the file and the release tag, and must match exactly.
  • Unsigned releases: a release with no signature for this platform is shown as available but not installable, at check time rather than after a 100 MB download.
  • Development builds have no key and keep the digest-only behavior. The release workflow refuses to build a release without the key.
  • Verified end to end: I dry-ran the workflow's signing script with a real key pair, and the updater's verifier accepted its output. Real minisign fixtures cover genuine, tampered, wrong-key, replayed-older-tag, wrong-asset and malformed cases.
  • Caveat: the first signed release is still installed by the previous, unsigned updater. Every update after that is verified.
  • Docs: docs/release-signing.md covers setup, key rotation and manual verification. docs/privacy.md is updated.

Bugs fixed

  1. Response-pipe (FIFO) replies were dropped silently. Startup created a callback-less TTS worker, bound the FIFO responders to it, then replaced it with the real one. The responders were never rebound. Responders now look up the current worker per line, and startup creates only one worker, which also stops a prewarming engine from loading its model twice.

  2. Shortcuts with punctuation, numpad or right-hand modifier keys never fired. The recorders saved names the backends never report, and the key matcher compares names exactly:

    • . became KEY_. rather than KEY_DOT;
    • Numpad 1 became KEY_1 rather than KEY_KP1;
    • Right Ctrl became KEY_LEFTCTRL.

    Keys are now mapped by physical key (KeyboardEvent.code), and each name was checked against the evdev crate and the Windows scan-code table. The portal's accelerator translation gains the numpad keysyms. Saved bindings and stop keys using the old punctuation names are migrated on load. Old numpad bindings saved as KEY_1 can't be distinguished from the top-row key, so they are left alone.

  3. Stereo-only microphones never opened. Capture always requested mono. It now uses mono when offered, and otherwise opens at the device's channel count and downmixes on the audio thread without allocating.

  4. Config saves were not atomic. A crash mid-save could leave config.json truncated, and it would then load as all defaults. Saves now write a temp file and rename it into place; routing's TOML saves share the helper.

  5. Stale tray/overlay target label after renaming the active target. The targets cache now carries a version counter, which the tray includes in its cache key.

  6. The TTS stop-key recorder saved letter keys wrong (Q became KEYQ) because of a drifted private copy of the key mapper. It now uses the shared mapper.

  7. Inconsistent action-button styles. Five scoped copies had drifted (small text 10px vs 11px; disabled styled two different ways, or not at all). There is now one definition in tab.css, so the shortcut-banner button now looks disabled while it works.

  8. Smaller fixes: empty target labels in the tray; get_status with multi-target ids; inference errors losing their target and hotkey ids.

Performance (hot path)

  • The dictation path reads no config file. InferenceRequest carries the primary target's processing overrides and the hotkey binding, resolved once per recording from in-memory targets and bindings caches. The OpenAI settings come from the engine's in-memory config.
  • Hotkey OpenAI rewrites no longer send GET /models before every request; a 2 s connect_timeout keeps failing fast.
  • Config::load writes the file at most once, instead of once per migration.
  • Command-trigger fuzzy matching uses the allocation-free Levenshtein.
  • Messages on the TTS command queue shrink from 568 bytes, because the config update is boxed.

Structure

All of these are pure moves plus merged duplicates; each was checked mechanically so that every original line appears exactly once.

  • commands.rs (1.8k lines) → commands/ with 10 modules. generate_handler! and all paths are unchanged.
  • voxctrl-config/src/lib.rs (1.7k lines) → 12 modules. The public API is unchanged, and the test list is identical.
  • HotkeysTab.svelte (1584 → 1226 lines) and TtsTab.svelte (1555 → 1349 lines):
    • shared keys.ts, actions.ts, newOutputTarget() and hotkeys.ts;
    • new components: HotkeyBackendBanners, StopKeyRecorder, and KeyValueListEditor, which Features and TTS had as identical copies.
  • Dead code removed: the unused retry-shortcuts code, getTriggerSignature, ten unused TtsTab CSS rules, and AudioRecorder's never-read config along with voxctrl-audio's dependency on voxctrl-config.
  • voxctrl_inference::finalize is shared by the local and remote transcription paths. start_tts_worker, targets_display_label and onLevelFrame each replace several copies.

Lint

  • clippy: zero warnings across the workspace (--all-targets). Beyond mechanical fixes:
    • capture_loop takes the recorder instead of 11 arguments;
    • two identical portal branches are merged, with the same behavior;
    • tests that deliberately hold the environment lock across .await are documented and allowed;
    • the verbatim reference copy in voxctrl-text is exempted rather than rewritten.
  • svelte-check: 453 warnings → 0. Tailwind directives now live in <style lang="postcss">, and the real line-clamp warning is fixed.

Tests

New tests cover release signing (10 tests), the finalize module (7), key mapping (punctuation, numpad, right-hand modifiers, keycaps), the portal numpad keysyms, the legacy key-name migration in both config and bindings, the bindings and targets caches, stereo capture, hotkeys.ts, KeyValueListEditor and StopKeyRecorder. Two inference tests that could never fail were replaced, and each replacement was confirmed to fail when the behavior it guards is broken.

Verification

  • CI passed on every earlier commit: Linux (default features, moonshine-webgpu, vulkan) and Windows cargo check, cargo test on both platforms, and the frontend check plus tests.
  • Locally on the latest commit:
    • cargo test passes for every crate, including voxctrl-app (94);
    • cargo clippy --all-targets reports no warnings;
    • svelte-check reports 0 errors and 0 warnings;
    • vitest passes 260/260.
  • The compiled CSS was compared before and after each frontend change; the only differences are the ones listed above.

🤖 Generated with Claude Code

https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR

claude and others added 14 commits September 25, 2026 20:02
…g, trim hot-path I/O

Bug fixes
- Response pipes (FIFO responders) were bound to a TTS worker that startup
  created without callbacks and then shut down, so pipe replies were silently
  dropped; disabling TTS also left a worker running for them. Responders now
  look up the current worker per line, and startup creates a single worker.
- Config saves are now atomic (temp file + rename): a crash mid-write used to
  leave a truncated config.json that loaded back as all-defaults.
- Tray/status target label no longer shows an empty string for a target with
  no label, and get_status handles multi-target ids.
- Inference errors keep the utterance's target and hotkey ids.

Performance
- The per-utterance path no longer re-reads config.json (Config::load also
  ran migrations and a filesystem migration each time); the in-memory config
  is already kept current by save_config.
- Voice-command parsing uses the in-memory targets cache instead of re-reading
  and re-parsing targets.toml on every final and interim pass.
- Hotkey OpenAI rewrites no longer do a GET /models probe before every
  request; a 2s connect timeout keeps the fast-fail on a dead server.
- Config load writes the file at most once instead of once per migration.
- Command-trigger fuzzy matching uses voxctrl-text's allocation-free
  Levenshtein instead of a full-table copy.

Reuse / cleanup
- New voxctrl_inference::finalize module shared by the local worker and the
  remote streaming path (noise gate, prompt, post-processing, hallucination
  filter, per-hotkey OpenAI overrides), replacing two ~150-line copies.
- InferenceOutput::empty/failed constructors; OpenAiMode::from_key.
- One start_tts_worker helper replaces three copies of the callback wiring.
- routing::targets_display_label replaces four label-resolution copies.
- One write_private helper shared by config and routing.
- Overlay styles share onLevelFrame (listener + smoothing loop), which also
  stops leaking the audio-level listener on a fast unmount.
- Removed a redundant desktop-integration pass at startup, dead
  OpenAiClient availability cache, and hand-written Default impls.
- dist/ rebuilt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
negotiate_config requested one channel unconditionally. Devices that only
capture in stereo (many USB mics; WASAPI's shared-mode mix format is often two
channels) refuse that, so the microphone never opened. The stream now uses
mono when the device offers it at its default rate, and the device's own
channel count otherwise; CaptureProcessor averages interleaved frames to mono
in a reused buffer before gain, pre-roll, denoise and resampling.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
The tray/overlay target label is cached and was only rebuilt when the active
target, the binding label or the label source changed, so renaming the active
target in Settings did not show until the user switched targets. The targets
cache now goes through AppState::set_targets, which bumps a targets_version
counter; the tray includes that version in its cache key.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
test_language_is_none_for_all_whisper_devices and
test_process_uses_in_memory_config_not_disk only asserted on the config the
engine was constructed with, so neither exercised process() or could catch
the regressions they are named after.

They are replaced by tests that drive InferenceEngine::process end to end
through a recording fake backend:
- whisper_gets_only_its_own_language_setting: the language hint comes only
  from whisper_cpp.language (never moonshine.language), for every device.
- process_uses_the_engine_config_and_follows_updates: post-processing and
  the recognition prompt come from the in-memory config, and a config pushed
  through update_config applies to the next utterance.
- silence_is_gated_before_the_backend_runs.

Both replacements were checked to fail when the behaviour they guard is
broken.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
452 of svelte-check's 453 warnings were "Unknown at rule @apply/@reference"
from its plain-CSS validator reading Tailwind directives, which buried any
real warning. The 15 components that use those directives now mark their
style blocks lang="postcss" — Tailwind v4's documented setup for Svelte —
which svelte-check does not run the plain-CSS validator on. The compiled CSS
is byte-for-byte unchanged apart from the fix below.

The one real warning left is fixed: VoiceStep's note clamps with
-webkit-line-clamp only, so add the standard line-clamp alongside it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
InferenceEngine::process re-read targets.toml (and bindings.toml for hotkey
rewrites) from disk for every utterance, coupling the speech engine to
routing's file layout and doing file I/O plus TOML parsing on the hot path.

InferenceRequest now carries what finishing an utterance needs: the primary
target's processing overrides and the hotkey binding. The pipeline resolves
them once per recording (RecordingContext::capture) from AppState's
in-memory caches: the existing targets cache, and a new bindings cache that
save_bindings keeps current. The remote path and the S1-mini lookup use the
same caches, so the dictation path reads no config file at all.

finalize::post_process/post_process_config take the overrides directly;
finalize::target_processing resolves them from a target list. The engine's
tests are now hermetic, with a new test that per-request overrides apply,
and the app gains a test for the bindings cache lookup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
The updater checked downloads only against the SHA-256 digest GitHub reports
for the same upload. That catches corruption, not tampering: whoever can
replace a release asset replaces its digest too.

- The release workflow's publish job signs every asset with minisign
  (secret VOXCTRL_UPDATE_SIGNING_KEY), uploads <asset>.minisig beside it, and
  verifies each signature against the public key before publishing, so a
  mismatched key pair fails the release instead of shipping un-installable
  updates. Build jobs refuse to build a release without the public key.
- Release builds bake in the public key (repository variable
  VOXCTRL_UPDATE_PUBKEY, via voxctrl-update/build.rs) and, after the digest
  check, stream-verify the download against its signature before it is put in
  place. The signed trusted comment names the file and the release tag, and
  must match exactly, so a signed file cannot be replayed under another name
  or as a newer release.
- A release with no signature for this platform is reported as available but
  not installable at check time, not after the download.
- Development builds carry no key and keep the digest-only behavior.

Tests use real minisign fixtures (throwaway key): genuine, tampered, wrong
key, replayed older tag, wrong asset, malformed input, and the check-time
decision. docs/release-signing.md covers the one-time key setup, rotation and
manual verification; docs/privacy.md is updated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
A pure move: core, routing, tts, models, overlays, audio, openai, hotkeys,
setup and display. mod.rs re-exports every submodule, so
`use crate::commands::*`, generate_handler! and the crate::commands:: paths
used elsewhere are unchanged. Each non-blank line of the old file appears
exactly once in the new ones (checked mechanically); only the per-file
imports differ.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
A pure move: engine, audio, ui, features, openai, tts, mcp, migrate, store,
paths, validate, and the tests into tests.rs. lib.rs keeps ConfigError and
AppConfig and re-exports every module, so the crate's public API is
unchanged (the whole workspace builds against it untouched). Every line of
the old file appears exactly once in the new ones, and the test list is
identical (26 tests).

Crate-internal only: the migration helpers and parse_tolerant are pub(crate)
and Config::path is pub(crate), because the tests now live in their own file.

Also fixes a misplaced doc comment from earlier in this PR: write_private had
been inserted between find_in_path's doc comment and find_in_path itself, so
find_in_path lost its documentation. Each now carries its own.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
… helpers

Settings → Hotkeys (1584 → 1226 lines) and Settings → TTS (1555 → 1349) are
broken up, and the copies of logic they shared with other components merged:

- src/lib/keys.ts: mapBrowserKeyToEvdev, MODIFIER_KEYS, isModifiersOnly. The
  wizard, Hotkeys tab and TTS stop-key recorder each had a copy, kept in step
  by a comment. The TTS copy had drifted: it lacked the letter-key rule, so
  recording Q as the stop key saved "KEYQ", a name nothing recognises. The
  wizard's exports (and their tests) are unchanged; it re-exports these.
- src/lib/actions.ts: the autoResize textarea action (three copies).
- routing-types.ts newOutputTarget(): the new-command template (two copies).
- Settings/hotkeys.ts: the Hotkeys tab's types, gesture tables and pure
  helpers (conflict signatures, target labels).
- HotkeyBackendBanners.svelte: the shortcut-backend and KDE banners, with
  their state and styles.
- KeyValueListEditor.svelte: the word → replacement list editor, which
  Features → Snippets and TTS → Pronunciation had as two identical copies.
- StopKeyRecorder.svelte: the TTS stop-key recorder.

Also removes dead code: the unused retry-shortcuts state and handler and
getTriggerSignature in HotkeysTab, and ten TtsTab CSS rules no markup used
(Svelte could not see they were unused while the tab had a computed class).

The compiled CSS is unchanged apart from those dead rules and the four
button rules the banner component carries. New tests cover hotkeys.ts, the
list editor (binding, trimming, onchange) and the stop-key recorder,
including the letter-key regression. svelte-check: 0 errors, 0 warnings;
vitest: 256 passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
…eal names

The shortcut recorders (setup wizard, Settings → Hotkeys, TTS stop key)
saved key names the backends never report, and the key matcher compares
names exactly, so those shortcuts could not fire:

- punctuation was named after the typed character: "." became "KEY_.",
  not KEY_DOT (and Shift changed the name again: "KEY_>");
- numpad keys became the top-row digit: Numpad 1 was saved as KEY_1, while
  evdev, X11 and Windows report KEY_KP1;
- right-hand modifiers were saved as the left one (KEY_LEFTCTRL for Right
  Ctrl), while evdev, X11 and Windows report the side pressed.

mapBrowserKeyToEvdev now maps these by physical key (KeyboardEvent.code),
which is layout- and Shift-independent, to evdev names; each emitted name was
checked against the evdev crate and the Windows scan-code table. The portal's
accelerator translation gains the numpad keysyms (KP_0..KP_9, KP_Add, ...).

Saved configs are migrated on load: voxctrl_config::canonical_key_name maps
the old punctuation names (and KEY_ESCAPE, whose migration it absorbs) to
evdev names, for bindings and the TTS stop key. Numpad digits saved as KEY_1
are indistinguishable from the top-row key and are left as they are.

The wizard's keycaps show punctuation and numpad keys readably (".", "Num 7").

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
Five components carried their own scoped copy of .btn-action, and the
copies had drifted:
- .small text was 10px in the command editor and 11px in the tabs;
- :disabled faded to 50% with a not-allowed cursor in the command editor,
  to 60% with a normal cursor in General, and not at all elsewhere, so the
  shortcut-banner button did not look disabled while it was opening
  settings.

One definition now lives in the shared Settings stylesheet, using the
majority 11px and the conventional 50% / not-allowed disabled state. The
compiled CSS differs from before only in those two variants.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
clippy --all-targets is now silent on every crate (default Linux features,
and the app without the ONNX features). Beyond mechanical fixes:

- voxctrl-audio: capture_loop took eleven arguments, eight of them the
  AudioRecorder's own fields cloned one by one; it now takes the recorder.
  Its AudioConfig field was never read — every setting reaches the recorder
  through live atomics — and a blanket allow(unused_variables) had hidden
  that. Field, constructor argument and allow are gone, and so is the
  crate's dependency on voxctrl-config, which nothing else used.
- voxctrl-tts: TtsCommand::UpdateConfig boxes its TtsConfig, which was
  making every command on the TTS queue 568 bytes.
- voxctrl-hotkeys portal: two branches of the kglobalshortcutsrc rewrite
  produced the same line; merged into one (same behaviour).
- Tests holding the environment lock across .await do so deliberately (the
  environment must stay set for the whole test, and each tokio test has its
  own thread); documented on get_env_lock and allowed on those six tests.
- voxctrl-text/reference.rs is a verbatim copy kept for a differential test,
  so it is exempted rather than rewritten.
- The rest: async fn in trait impls (services, mcp tests), derived Default
  impls, strip_suffix, a c"" literal, is_empty beside a public len,
  is_none_or, sort_by_key, slice::from_ref, struct-update syntax in tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015v84hAqhDBdwy7Q49gAWBR
@JRufer
JRufer marked this pull request as ready for review September 26, 2026 11:56
@JRufer
JRufer merged commit 89897be into master Sep 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants