docs: 2026-05-10 audit + beta logic review snapshots
Snapshot two audit artifacts produced 2026-05-10: - docs/audit-2026-05-10-validated.md — 21 audit items run against the live source tree at openless-all/app/src-tauri/src/ with a CONFIRMED / FALSE_POSITIVE / PARTIAL verdict per item plus the exact file:line range that supports each verdict. - docs/logic-review-2026-05-10.md — 5-path logic review of origin/beta @400097adcovering startup, press/release, end-of- session, capsule emit, and shutdown. Notable: at400097adthe emit_capsule fix from PR #389 was NOT yet an ancestor, so the cpal audio-callback → AppKit/Win32 SIGTRAP risk was still live on that snapshot (subsequent merges have addressed it; this doc remains the dated record). Both files are pure snapshots — no code changes here.
This commit is contained in:
baiqing committed
1 parent
fcb13f5c23
commit
a74a5472b0
2 files changed
+859
No files matched your search
@@ -0,0 +1,700 @@
|
||||
# Audit Validation — 2026-05-10
|
||||
|
||||
Validation of the 21 audit items against the current source tree at
|
||||
`openless-all/app/src-tauri/src/`. Every CONFIRMED finding cites the
|
||||
exact file and line range read from the working copy on 2026-05-10.
|
||||
|
||||
## Summary
|
||||
|
||||
| ID | Severity | Status | One-line |
|
||||
|----|----------|--------|----------|
|
||||
| 2.2.1 | — | CONFIRMED | TS `UserPreferences` interface and `mockSettings` both miss `updateChannel` |
|
||||
| 3.1.1 | 严重 | CONFIRMED | `MacHotkeyAdapter` does not override `HotkeyAdapter::shutdown`; `CFRunLoopRun` runs forever |
|
||||
| 3.1.2 | 中 | CONFIRMED | `hotkey_supervisor_loop` is `loop { ... sleep(3s) }` with no exit signal |
|
||||
| 3.1.3 | 中 | CONFIRMED | `start_listener_thread` spawns the listener and drops the `JoinHandle` |
|
||||
| 3.2.1 | 高 | FALSE_POSITIVE | Channel is `std::sync::mpsc::channel()` (unbounded async); `tx.send` does not block |
|
||||
| 3.2.2 | 高 | CONFIRMED | `emit_capsule` calls `window.show()/hide()` from the cpal `process_callback` thread |
|
||||
| 3.2.3 | 中 | CONFIRMED | `inner.inserter.insert(...)` runs sync `arboard`+`enigo` from async `end_session` |
|
||||
| 3.2.4 | 中 | CONFIRMED | `AudioMuteGuard::activate` shells out to `osascript` / `wpctl` / `pactl` synchronously |
|
||||
| 3.2.5 | 低 | CONFIRMED (Linux only) | `probe_input_stream` calls `thread::sleep(120ms)` from the async permission gate |
|
||||
| 3.3.1 | 高 | CONFIRMED | `handle_pressed_edge` routes to QA when `panel_visible=true` regardless of dictation phase |
|
||||
| 3.3.2 | 中 | PARTIAL | Two bridge loops both touch `state` for the same modifier event; non-fatal contention, no integrity bug |
|
||||
| 3.3.3 | 中 | FALSE_POSITIVE | Cancelled doesn't reset coordinator latch, but the OS-side `Shared::trigger_held` already gates auto-repeat |
|
||||
| 3.3.4 | 中 | CONFIRMED | `open_qa_panel` always emits `CapsuleState::Idle`, clobbering any in-flight dictation capsule |
|
||||
| 3.3.5 | 低 | CONFIRMED | `finish_cancel_session_state` skips `focus_target = None` when `phase == Processing` |
|
||||
| 3.3.6 | 低 | FALSE_POSITIVE | `take_current_prepared_windows_ime_session_for_restore` removes the slot on first call; second call is a true no-op |
|
||||
| 3.4.1 | 中 (advisory) | ADVISORY_ONLY | `Inner` carries 16 `Mutex` + 4 `AtomicBool` fields (20 concurrent fields) |
|
||||
| 3.4.2 | 中 (advisory) | ADVISORY_ONLY | 66 of 67 `Ordering` usages in coordinator/hotkey are `SeqCst` |
|
||||
| 3.4.3 | 低 (advisory) | ADVISORY_ONLY | ~102 `unsafe`/`unsafe fn`/`unsafe impl`/`unsafe extern` sites; many lack SAFETY comments |
|
||||
| 3.4.4 | 低 | CONFIRMED | `start_dispatcher` in `global_hotkey_runtime.rs` is `loop {}` with no exit |
|
||||
| 20 (NEW) | — | FALSE_POSITIVE | `read_or_default` already falls back to `UserPreferences::default()` on decode failure; `expect()` only fires on filesystem errors |
|
||||
| 2.3.3 | — | CONFIRMED (no action) | All four backend events (`capsule:state`, `qa:state`, `qa:level`, `vocab:updated`) have matching frontend listeners |
|
||||
|
||||
**Tally**: 11 CONFIRMED · 4 FALSE_POSITIVE · 1 PARTIAL · 3 ADVISORY_ONLY · 1 CONFIRMED-no-action · 1 CONFIRMED-Linux-only
|
||||
|
||||
## Recommended PR groupings
|
||||
|
||||
Group by file to minimize merge conflict risk. Suggested order:
|
||||
|
||||
1. **PR A — `hotkey.rs` lifecycle** (3.1.1, 3.1.3): add `MacHotkeyAdapter::shutdown` (post a synthetic `CFRunLoopStop`/`CFRunLoopWakeUp` from `Drop`) and store the listener `JoinHandle` so panics surface. Same file, same review.
|
||||
2. **PR B — TS type alignment** (2.2.1): add `updateChannel: UpdateChannel` to `src/lib/types.ts` and `mockSettings` in `src/lib/ipc.ts`. One file pair, trivial.
|
||||
3. **PR C — coordinator hotkey supervisor exits** (3.1.2, 3.4.4): add an `AtomicBool` shutdown flag to `hotkey_supervisor_loop` and `global_hotkey_runtime::start_dispatcher`. Same module concern, no overlap with PR A.
|
||||
4. **PR D — async hygiene** (3.2.3, 3.2.4, 3.2.5): wrap `inserter.insert`, `AudioMuteGuard::activate`, and `probe_input_stream` in `tokio::task::spawn_blocking`. Touches `coordinator/dictation.rs`, `coordinator/resources.rs`, and `coordinator.rs` — coordinate with PR E to avoid shared-line conflicts.
|
||||
5. **PR E — QA / dictation routing race** (3.3.1, 3.3.4): make `handle_pressed_edge` consult dictation phase before routing to QA, and skip the `Idle` capsule clobber in `open_qa_panel` when dictation is active. Same file (`coordinator/qa.rs` + `coordinator/dictation.rs`).
|
||||
6. **PR F — capsule UI thread marshaling** (3.2.2): bounce `window.show()/hide()` through `app.run_on_main_thread`; emit-only path can stay (Tauri marshals events internally). Touches `coordinator.rs::emit_capsule`. Independent of PR E.
|
||||
7. **PR G — focus_target leak fix** (3.3.5): in `finish_cancel_session_state`, also clear `focus_target` when the cancelled phase is `Processing`. Pure `coordinator_state.rs` edit.
|
||||
|
||||
Advisory items (3.4.1 / 3.4.2 / 3.4.3) need no PR; they are tracked here for future hardening.
|
||||
|
||||
## Detail per item
|
||||
|
||||
### 2.2.1 — `updateChannel` missing in TS types
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/types.rs:216-219`, `openless-all/app/src/lib/types.ts:118-177`, `openless-all/app/src/lib/ipc.ts:46-79`
|
||||
|
||||
**Evidence (Rust source of truth)**:
|
||||
```rust
|
||||
// types.rs:216-219
|
||||
/// Auto-update 渠道偏好。stable = 跟正式版（默认）；beta = Settings 里多
|
||||
/// 一个手动下载 Beta 的入口。不影响 plugin-updater 的自动检查路径。
|
||||
#[serde(default)]
|
||||
pub update_channel: UpdateChannel,
|
||||
```
|
||||
|
||||
**Evidence (TS gap)**: `UserPreferences` ends at `startMinimized: boolean;` (line 176). No `updateChannel` field. `mockSettings` (ipc.ts:46-79) ends at `startMinimized: false,`. No `updateChannel` key.
|
||||
|
||||
**Notes**: Channel state today is read/written via separate `getUpdateChannel` / `setUpdateChannel` IPC commands (`ipc.ts:170-176`), so `getSettings()` still works — the TS shape is just lying about what the Rust backend actually serializes. Setting via `setSettings(prefs)` round-trips through Rust's `UserPreferencesWire` which `#[serde(default)]`-fills the field, so currently no data corruption, but the type is incorrect and any consumer that destructures `UserPreferences` will silently miss the field. Trivial fix.
|
||||
|
||||
---
|
||||
|
||||
### 3.1.1 — `MacHotkeyAdapter::shutdown` empty [严重]
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/hotkey.rs:111` (default), `:301-325` (`MacHotkeyAdapter` impl), `:419-458` (`run_listen_loop`), contrast `:769-776` (Windows shutdown).
|
||||
|
||||
**Evidence (default)**:
|
||||
```rust
|
||||
// hotkey.rs:102-112
|
||||
pub trait HotkeyAdapter: Send + Sync {
|
||||
fn kind(&self) -> HotkeyAdapterKind;
|
||||
fn update_binding(&self, binding: HotkeyBinding);
|
||||
fn update_modifier_shortcuts(...);
|
||||
fn reset_held_state(&self);
|
||||
fn shutdown(&self) {}
|
||||
}
|
||||
```
|
||||
|
||||
**Evidence (mac adapter impl is silent on `shutdown`)**:
|
||||
```rust
|
||||
// hotkey.rs:305-325
|
||||
impl HotkeyAdapter for MacHotkeyAdapter {
|
||||
fn kind(&self) -> HotkeyAdapterKind { ... }
|
||||
fn update_binding(&self, binding: HotkeyBinding) { ... }
|
||||
fn update_modifier_shortcuts(...) { ... }
|
||||
fn reset_held_state(&self) { reset_shared_held_state(&self.shared); }
|
||||
// <-- no shutdown override
|
||||
}
|
||||
```
|
||||
|
||||
**Evidence (no exit path)**:
|
||||
```rust
|
||||
// hotkey.rs:454-457
|
||||
log::info!("[hotkey] CGEventTap 已启动");
|
||||
let _ = status_tx.send(Ok(()));
|
||||
CFRunLoopRun();
|
||||
// CFRunLoopRun never returns absent CFRunLoopStop; the listener thread leaks.
|
||||
```
|
||||
|
||||
**Notes**: `Drop for HotkeyMonitor` does call `self.adapter.shutdown()` (line 170-174), but Mac falls through to the empty default. On every preference-driven monitor swap the old `CFRunLoop` thread + tap leak. Visible on macOS as a steady leak of background threads on long-running sessions that change hotkey bindings.
|
||||
|
||||
**Fix sketch**: Store the `CfRunLoopRef` returned by `CFRunLoopGetCurrent()` (currently captured in `run_listen_loop` only) in the `MacHotkeyAdapter`, plus the `CfMachPortRef`. On `shutdown`, call `CGEventTapEnable(tap, false)` then `CFRunLoopStop(rl)`. Both are FFI-safe to call from any thread. Mirror the Windows pattern (`PostThreadMessageW(thread_id, WM_QUIT)`). Storing those refs needs `CallbackContext` exposed to the adapter, easiest via shared `parking_lot::Mutex`.
|
||||
|
||||
---
|
||||
|
||||
### 3.1.2 — `hotkey_supervisor_loop` no-exit
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator.rs:766-833`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator.rs:766-833
|
||||
fn hotkey_supervisor_loop(inner: Arc<Inner>) {
|
||||
let mut attempts: u32 = 0;
|
||||
let capability = HotkeyMonitor::capability();
|
||||
loop {
|
||||
let prefs = inner.prefs.get();
|
||||
if inner.hotkey.lock().is_some() { return; }
|
||||
// ... try start, on failure:
|
||||
std::thread::sleep(std::time::Duration::from_secs(3));
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: The only successful exit is when the hotkey is already installed (line 772-774). On error the supervisor keeps spinning. There is no `AtomicBool` / `Sender<()>` shutdown signal exposed for app shutdown. Same pattern repeats in `qa_hotkey_supervisor_loop`, `combo_hotkey_supervisor_loop`, `translation_hotkey_supervisor_loop`, `action_hotkey_supervisor_loop`. Worth a single shared shutdown flag.
|
||||
|
||||
---
|
||||
|
||||
### 3.1.3 — `start_listener_thread` drops `JoinHandle`
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/hotkey.rs:196-229`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// hotkey.rs:218-228
|
||||
let (status_tx, status_rx) = mpsc::channel::<Result<T, HotkeyInstallError>>();
|
||||
std::thread::Builder::new()
|
||||
.name(thread_name.into())
|
||||
.spawn(move || run_listen_loop(thread_shared, tx, status_tx))
|
||||
.map_err(|e| install_error("spawn_failed", format!("hotkey 线程启动失败: {e}")))?;
|
||||
|
||||
match status_rx.recv_timeout(Duration::from_secs(3)) { ... }
|
||||
```
|
||||
|
||||
**Notes**: The `Result<JoinHandle, _>` from `spawn(...)` is `?`'d for the spawn error, but the `JoinHandle` itself is silently dropped (the spawn return value isn't bound). `ListenerThread<T>` only stores `shared` and a single `startup` value. If the listener panics (e.g. `parking_lot::RwLock` poisoning, FFI bug), there is no path for the supervisor to learn about it — the channel just stops receiving. Storing the handle and using `JoinHandle::is_finished()` (Rust 1.61+) or pairing with a "thread alive" `AtomicBool` would let the supervisor restart the listener on panic.
|
||||
|
||||
---
|
||||
|
||||
### 3.2.1 — Blocking `tx.send` in event-tap callback
|
||||
**Status**: FALSE_POSITIVE
|
||||
**Files**: `openless-all/app/src-tauri/src/hotkey.rs:183-187`, `:218`, `coordinator.rs:650`, `:781`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// hotkey.rs:183-187
|
||||
fn send_or_log(tx: &Sender<HotkeyEvent>, evt: HotkeyEvent) {
|
||||
if let Err(e) = tx.send(evt) {
|
||||
log::warn!("[hotkey] 事件发送失败: {e}");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
```rust
|
||||
// coordinator.rs:650 (and :781)
|
||||
let (tx, rx) = mpsc::channel::<HotkeyEvent>();
|
||||
```
|
||||
|
||||
**Notes**: `std::sync::mpsc::channel()` is the **unbounded asynchronous** variant — `Sender::send` only blocks-and-fails when the receiver has been dropped, in which case it returns `Err(SendError)` immediately. There is no "rendezvous" backpressure. The only way `tx.send` would block long enough to trip `kCGEventTapDisabledByTimeout` is if std mpsc internally allocated under contention, which would be milliseconds at worst, not the seconds-scale macOS uses for the tap-disabled timeout. The audit conflated `mpsc::channel()` with `mpsc::sync_channel(0)` (rendezvous). No fix needed.
|
||||
|
||||
---
|
||||
|
||||
### 3.2.2 — `emit_capsule` runs `show()/hide()` on cpal callback thread
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator/dictation.rs:336-365`, `coordinator.rs:3617-3660`, `recorder.rs:458-490`
|
||||
|
||||
**Evidence (call site is the audio callback)**:
|
||||
```rust
|
||||
// coordinator/dictation.rs:336-365
|
||||
let level_handler: Arc<dyn Fn(f32) + Send + Sync> = Arc::new(move |level| {
|
||||
// ...phase guard, throttle to ~30 Hz...
|
||||
emit_capsule(
|
||||
&inner_for_level,
|
||||
CapsuleState::Recording,
|
||||
level,
|
||||
elapsed,
|
||||
None,
|
||||
None,
|
||||
);
|
||||
});
|
||||
// ...
|
||||
match Recorder::start(microphone_device_name, consumer, level_handler) { ... }
|
||||
```
|
||||
|
||||
**Evidence (callback thread is cpal's `process_callback`)**:
|
||||
```rust
|
||||
// recorder.rs:458-482 (process_callback)
|
||||
fn process_callback(...) {
|
||||
// ...resampling, RMS computation...
|
||||
level_handler(level); // synchronously invoked from cpal audio thread
|
||||
}
|
||||
```
|
||||
|
||||
**Evidence (`emit_capsule` directly touches the window)**:
|
||||
```rust
|
||||
// coordinator.rs:3637-3656
|
||||
if let Some(window) = app.get_webview_window("capsule") {
|
||||
let visible = !matches!(state, CapsuleState::Idle);
|
||||
maybe_position_capsule_bottom_center(inner, &window, payload.translation);
|
||||
if show_capsule && visible {
|
||||
if !show_capsule_window_no_activate(&app, &window) {
|
||||
let _ = window.show();
|
||||
}
|
||||
// ...
|
||||
} else {
|
||||
hide_capsule_window_if_present();
|
||||
let _ = window.hide();
|
||||
}
|
||||
}
|
||||
let _ = app.emit_to("capsule", "capsule:state", payload);
|
||||
```
|
||||
|
||||
**Notes**: `app.emit_to` is fine — Tauri's event bus is thread-safe and marshals to the JS runtime internally. The risk is `window.show() / window.hide()` and the position helper, all of which call NSWindow / HWND APIs that expect the main thread. On macOS this can stall the audio callback (NSWindow ops grab the AppKit run loop), risking `kAudioUnitErr_TooManyFramesToProcess` if the callback misses its deadline. The throttle to ~30 Hz mitigates frequency but doesn't change the per-call risk. Worth bouncing the window-touching half through `app.run_on_main_thread`.
|
||||
|
||||
**Fix sketch**: Split `emit_capsule` into `emit_capsule_event` (just `app.emit_to`, safe everywhere) and `apply_capsule_window_state` (called inside `app.run_on_main_thread` or only from already-main paths). The level-handler path only needs the event; window show/hide already happens in begin/end/cancel which run on the tokio runtime where `run_on_main_thread` is cheap.
|
||||
|
||||
---
|
||||
|
||||
### 3.2.3 — Sync inserter from async `end_session`
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator/dictation.rs:900-913`, `insertion.rs:42-56`, `:74-95`, `:136-149`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator/dictation.rs:900-913
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
{
|
||||
inner.inserter.insert(&polished, restore_clipboard)
|
||||
}
|
||||
// ...
|
||||
} else if allow_non_tsf_insertion_fallback {
|
||||
inner.inserter.copy_fallback(&polished)
|
||||
}
|
||||
```
|
||||
|
||||
```rust
|
||||
// insertion.rs:42-46 (non-macOS impl)
|
||||
pub fn insert(&self, text: &str, restore_clipboard_after_paste: bool) -> InsertStatus {
|
||||
if text.is_empty() { return InsertStatus::CopiedFallback; }
|
||||
insert_with_clipboard_restore(text, restore_clipboard_after_paste)
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: `end_session` is `async`. On Linux (and on macOS too — `simulate_paste()` is FFI-light but still sync), `insert` calls `arboard::Clipboard::new()` which can block on X11/wayland for tens of ms, then `enigo` keystroke synthesis which is also sync. Blocking the tokio worker for 50–200 ms isn't catastrophic but contributes to latency under load. macOS path uses `CGEventPost` via FFI — fast, non-blocking in practice; mostly a Linux/Windows concern.
|
||||
|
||||
**Fix sketch**: Wrap the platform-specific `inserter.insert` / `inserter.copy_fallback` in `tokio::task::spawn_blocking(move || ...).await.unwrap_or(InsertStatus::Failed)`. `TextInserter` is `Sync`, so the move only needs `&self` cloned via `Arc` (already in `Inner`).
|
||||
|
||||
---
|
||||
|
||||
### 3.2.4 — `AudioMuteGuard` shells out synchronously
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/audio_mute.rs:127-152` (macOS), `:202-249` (Linux), `coordinator/resources.rs:112-131`, `coordinator/dictation.rs:369`
|
||||
|
||||
**Evidence (macOS `osascript`)**:
|
||||
```rust
|
||||
// audio_mute.rs:143-151
|
||||
let output = Command::new("osascript")
|
||||
.args(["-e", script])
|
||||
.output()
|
||||
.map_err(|e| format!("set output mute failed: {e}"))?;
|
||||
```
|
||||
|
||||
**Evidence (Linux `wpctl`/`pactl` — same shape)**:
|
||||
```rust
|
||||
// audio_mute.rs:215-223
|
||||
let output = Command::new("wpctl")
|
||||
.args(["set-mute", "@DEFAULT_AUDIO_SINK@", value])
|
||||
.output()
|
||||
.map_err(|e| format!("wpctl set-mute failed: {e}"))?;
|
||||
```
|
||||
|
||||
**Evidence (called from async `begin_session`)**:
|
||||
```rust
|
||||
// coordinator/dictation.rs:369
|
||||
acquire_recording_mute(inner, "dictation");
|
||||
match Recorder::start(microphone_device_name, consumer, level_handler) { ... }
|
||||
```
|
||||
|
||||
```rust
|
||||
// coordinator/resources.rs:117-127
|
||||
if mute.holders == 0 {
|
||||
match crate::audio_mute::AudioMuteGuard::activate() {
|
||||
Ok(guard) => { mute.guard = Some(guard); ... }
|
||||
Err(err) => { ... }
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: `osascript` typically takes 100–300 ms to spawn + execute on macOS (AppleScript runtime startup). On a hot-key press → begin_session, this delays the recording start by exactly that amount, on the tokio worker thread. Windows path uses native COM (`IAudioEndpointVolume::SetMute`) which is fast and OK. Linux `wpctl`/`pactl` is similar to macOS osascript in cost.
|
||||
|
||||
**Fix sketch**: Wrap `AudioMuteGuard::activate()` in `tokio::task::spawn_blocking`. Since `acquire_recording_mute` itself is sync and called from `begin_session` (async), the cleanest path is making `acquire_recording_mute` async and `.await`-ing the spawn_blocking. Drop path (`PlatformMuteGuard::restore`) also runs `osascript` and is currently called from `Drop` in `release_recording_mute`; moving that to a detached `tokio::spawn_blocking` is sufficient (release path doesn't need to await).
|
||||
|
||||
---
|
||||
|
||||
### 3.2.5 — `thread::sleep(120ms)` in async permission probe
|
||||
**Status**: CONFIRMED (Linux/non-macOS path only)
|
||||
**Files**: `openless-all/app/src-tauri/src/permissions.rs:323-357`, `coordinator/dictation.rs:137`, `coordinator.rs:1732-1763`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// permissions.rs:343-356
|
||||
let stream = match sample_format {
|
||||
SampleFormat::F32 => build_probe!(f32),
|
||||
// ...
|
||||
}?;
|
||||
stream.play().map_err(|e| e.to_string())?;
|
||||
std::thread::sleep(Duration::from_millis(120));
|
||||
drop(stream);
|
||||
Ok(())
|
||||
```
|
||||
|
||||
**Evidence (called from async)**:
|
||||
```rust
|
||||
// coordinator/dictation.rs:137
|
||||
if let Err(message) = ensure_microphone_permission(inner) { ... }
|
||||
```
|
||||
|
||||
```rust
|
||||
// coordinator.rs:1732-1763
|
||||
fn ensure_microphone_permission(_inner: &Arc<Inner>) -> Result<(), String> {
|
||||
#[cfg(target_os = "windows")] { ... return Ok(()); } // Windows skips probe
|
||||
let status = permissions::check_microphone();
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: On macOS `check_microphone` uses `AVAudioApplication` / `AVCaptureDevice` and never reaches `probe_input_stream` (that helper is in the `cfg(not(target_os = "macos"))` module). So this 120 ms blocking sleep only fires on **Linux** (and any other non-macOS, non-Windows path) when probing mic permission. On Linux dictation, every `begin_session` blocks the tokio worker for 120 ms before the recorder even starts.
|
||||
|
||||
**Fix sketch**: `tokio::time::sleep(Duration::from_millis(120)).await` is the correct call but requires turning `probe_input_stream` async. Alternatively keep it sync and wrap the whole `check_microphone()` non-macOS path in `tokio::task::spawn_blocking`. The latter is mechanically simpler.
|
||||
|
||||
---
|
||||
|
||||
### 3.3.1 — Pressed edge routes to QA without checking dictation phase
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator/dictation.rs:11-22`, `coordinator/qa.rs:64-77`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator/dictation.rs:11-22
|
||||
pub(super) async fn handle_pressed_edge(inner: &Arc<Inner>) {
|
||||
let was_held = inner.hotkey_trigger_held.swap(true, Ordering::SeqCst);
|
||||
if !was_held {
|
||||
// 路由：QA 浮窗可见时，rightOption 边沿走 QA；否则走主听写。详见 issue #118 v2。
|
||||
let panel_visible = inner.qa_state.lock().panel_visible;
|
||||
if panel_visible {
|
||||
handle_qa_option_edge(inner).await;
|
||||
} else {
|
||||
handle_pressed(inner).await;
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Evidence (QA path doesn't stop dictation)**:
|
||||
```rust
|
||||
// coordinator/qa.rs:64-77
|
||||
pub(super) async fn handle_qa_option_edge(inner: &Arc<Inner>) {
|
||||
let phase = inner.qa_state.lock().phase;
|
||||
log::info!("[coord] QA option edge (phase={phase:?})");
|
||||
match phase {
|
||||
QaPhase::Idle => { let _ = begin_qa_session(inner).await; }
|
||||
QaPhase::Recording => { let _ = end_qa_session(inner).await; }
|
||||
QaPhase::Processing => {}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: `panel_visible` flips true via `open_qa_panel`, which is triggered by the QA hotkey (`Cmd+Shift+;` by default). If the user opens the QA panel mid-dictation (dictation `phase = Listening`, mic open, ASR session live), the next dictation-hotkey press routes into `begin_qa_session`. `begin_qa_session` will call `Recorder::start` again on the same mic device. cpal will reject the second `build_input_stream` on most platforms, but on Linux/PipeWire it sometimes succeeds and you end up with two concurrent capture streams. Even where it fails, the dictation session's recorder is still running and the user has no UX path to stop it from the QA panel.
|
||||
|
||||
**Fix sketch**: In `handle_pressed_edge`, check `inner.state.lock().phase`. If `Listening` or `Starting`, route to `handle_pressed` (which is the dictation toggle path) regardless of `panel_visible`, and either close the QA panel or refuse to open it while dictation is active. Decision belongs to product, but the *technical* race is real.
|
||||
|
||||
---
|
||||
|
||||
### 3.3.2 — Dual TranslationModifier handlers
|
||||
**Status**: PARTIAL
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator.rs:1130-1139`, `:1376-1384`, `:1402-1410`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator.rs:1130-1139 (translation_hotkey_bridge_loop, runs on its own thread)
|
||||
fn translation_hotkey_bridge_loop(inner: Arc<Inner>, rx: mpsc::Receiver<ComboHotkeyEvent>) {
|
||||
while let Ok(evt) = rx.recv() {
|
||||
if inner.shortcut_recording_active.load(Ordering::SeqCst) { continue; }
|
||||
if matches!(evt, ComboHotkeyEvent::Pressed) {
|
||||
mark_translation_modifier_seen(&inner);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// coordinator.rs:1402-1410 (hotkey_bridge_loop, separate thread)
|
||||
HotkeyEvent::TranslationModifierPressed => {
|
||||
let translation_hotkey = inner_cloned.prefs.get().translation_hotkey;
|
||||
if is_builtin_translation_shift(&translation_hotkey)
|
||||
|| crate::shortcut_binding::legacy_modifier_trigger(&translation_hotkey)
|
||||
.is_some()
|
||||
{
|
||||
mark_translation_modifier_seen(&inner_cloned);
|
||||
}
|
||||
}
|
||||
|
||||
// coordinator.rs:1376-1384
|
||||
fn mark_translation_modifier_seen(inner: &Arc<Inner>) {
|
||||
let phase = inner.state.lock().phase;
|
||||
if matches!(phase, SessionPhase::Starting | SessionPhase::Listening) {
|
||||
inner.translation_modifier_seen.store(true, Ordering::SeqCst);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: Both bridge loops run on independent `std::thread`s and both ultimately call `mark_translation_modifier_seen`, which locks `inner.state`. They never run *racing on integrity* — they both write the same `AtomicBool::store(true)`, idempotent. The audit's framing of "Both lock `inner.state` independently" is technically true but not a bug — `state` is a `Mutex`, only one acquires at a time, and both write the same flag. Worst case is one log-line of `[coord] translation modifier seen during ...` printed twice for one Shift press. Not worth a code change unless 3.3.1's fix touches the same code.
|
||||
|
||||
---
|
||||
|
||||
### 3.3.3 — Cancelled doesn't reset `hotkey_trigger_held`
|
||||
**Status**: FALSE_POSITIVE
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator.rs:1399-1401`, `hotkey.rs:530-538`, `coordinator/dictation.rs:11-22`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator.rs:1399-1401
|
||||
HotkeyEvent::Cancelled => {
|
||||
cancel_session(&inner_cloned);
|
||||
}
|
||||
```
|
||||
|
||||
**Why it doesn't actually wedge**: `HotkeyEvent::Cancelled` is emitted by the OS-side hotkey monitor only when the user presses **Esc** (`hotkey.rs:565-570` for macOS, `:867-871` for Windows), not when the dictation trigger key is released. The dictation trigger's "is currently held" state lives in `Shared::trigger_held` inside the platform monitor (`hotkey.rs:117`). That atomic gates re-emission of Pressed via the `is_active && !was_held` check (`hotkey.rs:530-538`). So even if `Inner::hotkey_trigger_held` stays `true` in the coordinator after Esc, the next Pressed edge from the OS will only fire when the user releases and re-presses the trigger key — and the OS path will set `was_held=false` again before sending Pressed. The coordinator's `hotkey_trigger_held` swap on the next Pressed will return `false` (because `handle_released_edge` has run between the previous press and this one, OR the user never released, in which case no new Pressed is queued).
|
||||
|
||||
The audit confused two layers: the OS-edge dedupe in `hotkey.rs::Shared::trigger_held` (which is the gating thing) and the coordinator's `Inner::hotkey_trigger_held` (which is just a bookkeeping latch tied to Pressed/Released edges that already came in). Cancelled doesn't change either's correctness.
|
||||
|
||||
**Notes**: There's a *cosmetic* asymmetry — after Esc, `Inner::hotkey_trigger_held=true` until the user releases the trigger. If the user keeps holding past the Esc, `Released` fires later and resets it. Defensive cleanup would be to also reset on Cancelled, but it doesn't fix any user-visible bug.
|
||||
|
||||
---
|
||||
|
||||
### 3.3.4 — `open_qa_panel` clobbers Done capsule
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator/qa.rs:79-104`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator/qa.rs:79-104
|
||||
pub(super) fn open_qa_panel(inner: &Arc<Inner>) {
|
||||
{
|
||||
let mut state = inner.qa_state.lock();
|
||||
state.panel_visible = true;
|
||||
state.phase = QaPhase::Idle;
|
||||
// ...
|
||||
}
|
||||
// 先把胶囊清干净，避免主听写上一次 Done 状态残留的 message/insertedChars
|
||||
// 在 QA Done 阶段被 capsule UI 错误复用（"已之一粘贴这个 0" 那种）。
|
||||
emit_capsule(inner, CapsuleState::Idle, 0.0, 0, None, None);
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: The comment shows the design intent is *intentional* — sweep stale Done state from a previous dictation. But it sweeps *any* in-flight capsule too. If the user opens the QA panel within the ~1.5 s `CAPSULE_AUTO_HIDE_DELAY_MS` window after dictation finishes, they lose the "已粘贴 N 字" toast. More importantly, if dictation is still in `Polishing` or `Inserting` phase (LLM hasn't returned yet), opening QA hides the polish-progress capsule mid-flight. The user sees their dictation "vanish" until insertion completes.
|
||||
|
||||
**Fix sketch**: Before calling `emit_capsule(Idle, ...)`, check `inner.state.lock().phase`. Only sweep if dictation is in `Idle`. If dictation is mid-flight, leave the capsule visible — QA panel doesn't need the capsule cleared to function. Pairs cleanly with the 3.3.1 fix (same source files).
|
||||
|
||||
---
|
||||
|
||||
### 3.3.5 — `focus_target` leaks on Processing-phase cancel
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator_state.rs:155-173`, `:347-374` (test that already proves the gap)
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator_state.rs:168-173
|
||||
pub(crate) fn finish_cancel_session_state(state: &mut SessionState, decision: CancelDecision) {
|
||||
if decision.phase != SessionPhase::Processing {
|
||||
state.phase = SessionPhase::Idle;
|
||||
state.focus_target = None;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Evidence (existing test acknowledges the gap)**:
|
||||
```rust
|
||||
// coordinator_state.rs:370-372
|
||||
if matches!(initial, SessionPhase::Starting | SessionPhase::Listening) {
|
||||
assert!(state.focus_target.is_none(), "initial={initial:?}");
|
||||
}
|
||||
// Note: no assertion that Processing-phase cancel clears focus_target.
|
||||
```
|
||||
|
||||
**Notes**: When cancel hits `Processing`, `finish_cancel_session_state` deliberately keeps `phase=Processing` (the comment says "交给 end_session 自己收尾"), but it also keeps `focus_target` populated. `end_session` does eventually reset state via the `proceed_to_insert=false` branch (`coordinator/dictation.rs:862-878`) which sets `phase=Idle` but doesn't touch `focus_target`. Net result: stale `focus_target` (a `Vec<PreparedWindowsImeSessionSlot>` index, basically a plain `usize`) lives on into the next `begin_session_state`, which overwrites it (`coordinator_state.rs:80`). So the leak is bounded — next session clobbers it. Real risk is only between cancel and next begin, where `restore_focus_target_if_possible(focus_target)` could pick up a stale value if anyone reads it. Code review didn't surface a reader on that interval, so this is a low-impact correctness gap, not a user-facing bug.
|
||||
|
||||
**Fix sketch**: In `finish_cancel_session_state`, set `state.focus_target = None` unconditionally (before the phase check). The Processing branch's existing semantic — "let `end_session` collapse to Idle" — doesn't depend on `focus_target` staying set.
|
||||
|
||||
---
|
||||
|
||||
### 3.3.6 — Double-restore of prepared Windows IME session
|
||||
**Status**: FALSE_POSITIVE
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator.rs:1594-1633`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// coordinator.rs:1594-1602
|
||||
fn take_matching_prepared_windows_ime_session(
|
||||
slots: &mut Vec<PreparedWindowsImeSessionSlot>,
|
||||
session_id: SessionId,
|
||||
) -> Option<PreparedWindowsImeSession> {
|
||||
let index = slots
|
||||
.iter()
|
||||
.position(|slot| slot.session_id == session_id)?;
|
||||
Some(slots.remove(index).prepared)
|
||||
}
|
||||
```
|
||||
|
||||
```rust
|
||||
// coordinator.rs:1620-1633
|
||||
fn restore_prepared_windows_ime_session(inner: &Arc<Inner>, session_id: SessionId) {
|
||||
let state = inner.state.lock();
|
||||
let prepared = {
|
||||
let mut slot = inner.prepared_windows_ime_session.lock();
|
||||
take_current_prepared_windows_ime_session_for_restore(
|
||||
&mut slot, session_id, state.session_id,
|
||||
)
|
||||
};
|
||||
if let Some(prepared) = prepared { inner.windows_ime.restore_session(prepared); }
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: First call to `restore_prepared_windows_ime_session` for a given `session_id` does `slots.remove(index)` regardless of the freshness check on `current_session_id`. The slot is gone after that. Second call's `slots.iter().position(...)` returns `None`, the `?` short-circuits, the function silently no-ops. So even if `cancel_session → end_session` (or vice versa) both invoke `restore_prepared_windows_ime_session` with the same `session_id`, the IME state is restored at most once. The audit's worry is unfounded.
|
||||
|
||||
---
|
||||
|
||||
### 3.4.1 — Inner has many lock fields
|
||||
**Status**: ADVISORY_ONLY
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator.rs:91-141`
|
||||
|
||||
**Inventory**: 16 `Mutex<...>` + 4 `AtomicBool` (excluding `Arc<...>` indirection counts) plus an `Arc<Mutex<Vec<...>>>` for the windows IME slot vector. Specifically:
|
||||
|
||||
| Field | Type |
|
||||
|---|---|
|
||||
| `app` | `Mutex<Option<AppHandle>>` |
|
||||
| `state` | `Mutex<SessionState>` |
|
||||
| `asr` | `Mutex<Option<SessionResource<ActiveAsr>>>` |
|
||||
| `recorder` | `Mutex<Option<SessionResource<Recorder>>>` |
|
||||
| `recording_mute` | `Mutex<SharedRecordingMuteState>` |
|
||||
| `hotkey` | `Mutex<Option<HotkeyMonitor>>` |
|
||||
| `hotkey_status` | `Mutex<HotkeyStatus>` |
|
||||
| `hotkey_trigger_held` | `AtomicBool` |
|
||||
| `shortcut_recording_active` | `AtomicBool` |
|
||||
| `combo_hotkey` | `Mutex<Option<ComboHotkeyMonitor>>` |
|
||||
| `translation_hotkey` | `Mutex<Option<ComboHotkeyMonitor>>` |
|
||||
| `switch_style_hotkey` | `Mutex<Option<ComboHotkeyMonitor>>` |
|
||||
| `open_app_hotkey` | `Mutex<Option<ComboHotkeyMonitor>>` |
|
||||
| `translation_modifier_seen` | `AtomicBool` |
|
||||
| `qa_hotkey` | `Mutex<Option<QaHotkeyMonitor>>` |
|
||||
| `qa_state` | `Mutex<QaSessionState>` |
|
||||
| `capsule_layout` | `Mutex<Option<CapsuleLayoutState>>` |
|
||||
| `qa_asr` | `Mutex<Option<Arc<VolcengineStreamingASR>>>` |
|
||||
| `qa_recorder` | `Mutex<Option<Recorder>>` |
|
||||
| `qa_stream_cancelled` | `Arc<AtomicBool>` (one of two AtomicBools-in-Arc) |
|
||||
| `prepared_windows_ime_session` (windows-only) | `Arc<Mutex<Vec<PreparedWindowsImeSessionSlot>>>` |
|
||||
|
||||
No deadlock pattern was found in the read paths — most call sites take one lock at a time. `mark_translation_modifier_seen` and `cancel_session` both touch `inner.state`, but in disjoint critical sections. Documenting only.
|
||||
|
||||
---
|
||||
|
||||
### 3.4.2 — Heavy `Ordering::SeqCst` use
|
||||
**Status**: ADVISORY_ONLY
|
||||
**Files**: `coordinator.rs`, `coordinator/*.rs`, `hotkey.rs`
|
||||
|
||||
**Evidence**: `grep -rn "Ordering::SeqCst" coordinator.rs coordinator/ hotkey.rs | wc -l` → 66. Total `Ordering::*` uses in those files: 67 (one `Relaxed` in `recorder.rs::process_callback`, 66 SeqCst).
|
||||
|
||||
**Notes**: Most uses are simple set/load on independent `AtomicBool`s where `Ordering::Relaxed` would suffice. A few that gate cross-thread visibility (`hotkey_trigger_held` swap in `handle_pressed_edge` synchronizing with the audio thread reading session state) might justify Acquire/Release. `SeqCst` is correct everywhere — just over-strong. Not a bug.
|
||||
|
||||
---
|
||||
|
||||
### 3.4.3 — Many `unsafe` blocks, audit SAFETY comments
|
||||
**Status**: ADVISORY_ONLY
|
||||
**Files**: cross-tree (predominantly `hotkey.rs`, `insertion.rs`, `windows_ime_*.rs`, `permissions.rs`)
|
||||
|
||||
**Evidence**: `grep -rn "unsafe " src/ --include="*.rs" | grep -E "unsafe \{|unsafe fn|unsafe impl|unsafe extern"` → 102 sites.
|
||||
|
||||
**Notes**: Almost all are platform FFI (CoreFoundation/CoreGraphics on macOS, win32 on Windows, msg_send! on macOS objc2). Sample inspected (`insertion.rs::send_text` near line 340 and `post_cmd_v` near line 420) — function-level invariants are documented at module level, but inline `// SAFETY:` comments are sparse. Same for `hotkey.rs::run_listen_loop` which leaks `Box::into_raw` for FFI context — a `// SAFETY: ctx is dropped only inside the listener after CFRunLoopRun returns; reentrancy guarded by ...` comment would help. Documentation-grade improvement, no soundness bug detected.
|
||||
|
||||
---
|
||||
|
||||
### 3.4.4 — Global hotkey dispatcher loop has no exit
|
||||
**Status**: CONFIRMED
|
||||
**Files**: `openless-all/app/src-tauri/src/global_hotkey_runtime.rs:94-107`
|
||||
|
||||
**Evidence**:
|
||||
```rust
|
||||
// global_hotkey_runtime.rs:94-107
|
||||
fn start_dispatcher(runtime: Arc<GlobalHotkeyRuntime>) {
|
||||
std::thread::Builder::new()
|
||||
.name("openless-global-hotkey-dispatch".into())
|
||||
.spawn(move || {
|
||||
let receiver = GlobalHotKeyEvent::receiver();
|
||||
loop {
|
||||
match receiver.recv_timeout(Duration::from_millis(250)) {
|
||||
Ok(event) => runtime.dispatch(event),
|
||||
Err(_) => continue,
|
||||
}
|
||||
}
|
||||
})
|
||||
.expect("spawn global hotkey dispatcher");
|
||||
}
|
||||
```
|
||||
|
||||
**Notes**: `GlobalHotKeyEvent::receiver()` is process-global from the upstream `global-hotkey` crate. The 250 ms timeout means the loop wakes regularly but never checks an exit flag. On app shutdown the thread leaks; harmless for a single-instance app but trips `tokio::test` and any future `Drop`-based teardown (e.g. integration tests that spin coordinator up/down).
|
||||
|
||||
**Fix sketch**: Pair with `Inner` shutdown flag added in 3.1.2 fix, or use a `parking_lot::RwLock<Option<...>>` "dispatcher alive" gate inside `GlobalHotkeyRuntime` that the loop reads each iteration. Same shape as the Windows hotkey thread's `WM_QUIT` plumbing.
|
||||
|
||||
---
|
||||
|
||||
### 20 (NEW) — `PreferencesStore::new().expect(...)` panics on bad prefs
|
||||
**Status**: FALSE_POSITIVE
|
||||
**Files**: `openless-all/app/src-tauri/src/coordinator.rs:169`, `:210`, `persistence.rs:790-811`, `:146-156`, `types.rs:368-422`
|
||||
|
||||
**Evidence (deserialization fallback already exists)**:
|
||||
```rust
|
||||
// persistence.rs:790-811
|
||||
impl PreferencesStore {
|
||||
pub fn new() -> Result<Self> {
|
||||
let dir = data_dir()?;
|
||||
ensure_dir(&dir)?;
|
||||
let path = dir.join(PREFERENCES_FILE);
|
||||
let prefs = if path.exists() {
|
||||
read_or_default::<UserPreferences>(&path).unwrap_or_else(|e| {
|
||||
log::warn!(
|
||||
"[prefs] load {} failed, using defaults: {}",
|
||||
path.display(),
|
||||
e
|
||||
);
|
||||
UserPreferences::default()
|
||||
})
|
||||
} else {
|
||||
UserPreferences::default()
|
||||
};
|
||||
Ok(Self { path, state: Mutex::new(prefs), })
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
```rust
|
||||
// persistence.rs:146-156 (read_or_default)
|
||||
fn read_or_default<T: for<'de> Deserialize<'de> + Default>(path: &Path) -> Result<T> {
|
||||
if !path.exists() { return Ok(T::default()); }
|
||||
let bytes = fs::read(path).with_context(|| format!("read failed: {}", path.display()))?;
|
||||
if bytes.is_empty() { return Ok(T::default()); }
|
||||
serde_json::from_slice::<T>(&bytes)
|
||||
.with_context(|| format!("decode failed: {}", path.display()))
|
||||
}
|
||||
```
|
||||
|
||||
**Why the audit was wrong**: The custom `Deserialize for UserPreferences` (types.rs:368-422) does call `default_dictation_hotkey_from_legacy(...).map_err(serde::de::Error::custom)?` which can return a serde error for `trigger == Custom` without `customComboHotkey`. That error bubbles through `serde_json::from_slice::<UserPreferences>` to `read_or_default`, which propagates it as `Result::Err`. But `PreferencesStore::new` then catches it at `.unwrap_or_else(|e| { log::warn!(...); UserPreferences::default() })`. So the `expect("preferences store init")` at coordinator.rs:169 only fires if `data_dir()?` or `ensure_dir(&dir)?` fails — i.e. the OS-level Application Support directory cannot be created/accessed, which is a legitimate fail-fast condition (no preferences-file storage, no point continuing).
|
||||
|
||||
The audit conflated "deserialization fails" with "PreferencesStore::new returns Err". In the current code those are different.
|
||||
|
||||
**Notes**: No fix needed. The "bad prefs file" case is already handled silently (log + default). If you want belt-and-braces against panic on the truly-impossible filesystem failure, you can add a final `.unwrap_or_else` that logs and returns a fully-default in-memory store, but that's defensive coding for a case where the user's machine is so broken that Application Support is unwritable.
|
||||
|
||||
---
|
||||
|
||||
### 2.3.3 — Event-name alignment between backend emit and frontend listen
|
||||
**Status**: CONFIRMED (no action)
|
||||
**Files**: `coordinator.rs:3659`, `coordinator/qa.rs:94-101`, `coordinator/dictation.rs:929-931`, `src/components/Capsule.tsx:293`, `src/pages/QaPanel.tsx:55,116`, `src/pages/Vocab.tsx:51`
|
||||
|
||||
**Notes**: Backend emits `capsule:state`, `qa:state`, `qa:level`, `vocab:updated`. Frontend listens to all four under matching names (Capsule, QaPanel, Vocab respectively). No mismatch. As stated in the audit prompt — already verified, retained here for completeness.
|
||||
|
||||
## Files referenced
|
||||
|
||||
- `openless-all/app/src-tauri/src/types.rs` (lines 57, 200-525, especially 216-219, 277-325, 368-422)
|
||||
- `openless-all/app/src-tauri/src/hotkey.rs` (lines 1-250, 280-572, 698-870, 1126-1190)
|
||||
- `openless-all/app/src-tauri/src/coordinator.rs` (lines 91-141, 156-313, 640-833, 837-940, 1130-1139, 1376-1419, 1582-1636, 3617-3700)
|
||||
- `openless-all/app/src-tauri/src/coordinator/dictation.rs` (lines 1-160, 320-410, 810-1050)
|
||||
- `openless-all/app/src-tauri/src/coordinator/qa.rs` (full file, 1-124)
|
||||
- `openless-all/app/src-tauri/src/coordinator/resources.rs` (lines 1-160)
|
||||
- `openless-all/app/src-tauri/src/coordinator_state.rs` (full file, 1-485)
|
||||
- `openless-all/app/src-tauri/src/global_hotkey_runtime.rs` (full file, 1-107)
|
||||
- `openless-all/app/src-tauri/src/audio_mute.rs` (lines 1-263)
|
||||
- `openless-all/app/src-tauri/src/permissions.rs` (lines 200-360)
|
||||
- `openless-all/app/src-tauri/src/insertion.rs` (lines 1-150, 300-450)
|
||||
- `openless-all/app/src-tauri/src/persistence.rs` (lines 146-156, 785-811)
|
||||
- `openless-all/app/src-tauri/src/recorder.rs` (lines 28-490)
|
||||
- `openless-all/app/src-tauri/src/commands.rs` (lines 975-1000)
|
||||
- `openless-all/app/src/lib/types.ts` (lines 118-275)
|
||||
- `openless-all/app/src/lib/ipc.ts` (lines 40-176)
|
||||
- `openless-all/app/src/components/Capsule.tsx` (line 293)
|
||||
- `openless-all/app/src/pages/QaPanel.tsx` (lines 55, 116-120)
|
||||
- `openless-all/app/src/pages/Vocab.tsx` (line 51)
|
||||
@@ -0,0 +1,159 @@
|
||||
# OpenLess beta — Logic Review (commit 400097ad)
|
||||
|
||||
Audit branch: `origin/beta` @ `400097ad` (= "Merge PR #391 fix/audit-async-hygiene").
|
||||
Sources: `openless-all/app/src-tauri/src/{lib.rs, coordinator.rs, coordinator/{dictation,qa,resources}.rs, coordinator_state.rs, hotkey.rs, audio_mute.rs}`, `openless-all/app/src/lib/{ipc.ts,types.ts}`.
|
||||
|
||||
> Caller's premise correction: the prompt asserts "PR #389 (emit_capsule main thread) is pending merge but the same change is already cherry-picked-equivalent on the current code." This is **false** at `400097ad`. Commits `faf02ad4` and `84ee3d96` exist on a side branch but are NOT ancestors of `400097ad` (verified with `git merge-base --is-ancestor`). The audio-thread → AppKit/Win32 SIGTRAP risk that PR #389 was written to fix is therefore **still live on this beta**. See P4.
|
||||
|
||||
## Summary
|
||||
|
||||
| Path | Verdict | Issues found |
|
||||
|------|---------|--------------|
|
||||
| P1 startup | OK | All 6 listeners paired (start in `setup`/`Ready`, stop in `Exit`); tray watcher signaled. |
|
||||
| P2 press/release | OK with 1 ⚠️ | Routing + dedup correct; `acquire_recording_mute` correctly awaited in dictation start. |
|
||||
| P3 end-of-session | 1 🚩 | `cancel_session` during Processing leaves `focus_target` set until next `begin_session` overwrites it. PR #387 contract is incomplete for the Processing branch. |
|
||||
| P4 capsule UI emit | 1 🚩 (CRITICAL) | `emit_capsule` calls `window.show/hide` + `show_capsule_window_no_activate` directly from the cpal audio callback at ~30 Hz on `400097ad`. PR #389 is **not** merged here. |
|
||||
| P5 shutdown | 1 🚩 | `acquire_recording_mute` at QA path (`coordinator.rs:2313`) is missing `.await` — return value is a dropped Future. PR #391 hygiene fix is incomplete. Compiler emits `unused_must_use`. |
|
||||
|
||||
Net: **3 real bugs (🚩)**, 1 cross-PR composition concern, plus minor smells. Two of the three are direct consequences of merge-incomplete state of PR #389/#391. The Processing-cancel `focus_target` leak is a code-path PR #387 missed.
|
||||
|
||||
## Findings (per path)
|
||||
|
||||
### P1 — Startup
|
||||
|
||||
- OK `lib.rs:316-358` — `RunEvent::Exit` stops all 6 hotkey listeners + signals tray watcher; matches the 6 starts at `lib.rs:226` (dictation, in `setup`) and `lib.rs:320-325` (QA / combo / translation / switch_style / open_app, in `RunEvent::Ready`).
|
||||
- OK `coordinator.rs:344-358, 371-373, 383-385, 395-397, 407-409, 1313-1347` — every `stop_*_listener` for `global-hotkey`-backed monitors marshals the `Drop` to `app.run_on_main_thread`, matching the issue #169 contract for Carbon `RemoveEventHotKey`. `take_combo_hotkey_on_main_thread` / `take_translation_hotkey_on_main_thread` / `take_action_hotkey_on_main_thread` are the helpers; `stop_qa_hotkey_listener` inlines the same pattern.
|
||||
- OK `coordinator.rs:330-332` + `hotkey.rs:344-355` — dictation `stop_hotkey_listener` is a plain `inner.hotkey.lock().take()`, but the Drop chain is `HotkeyMonitor::drop` → `MacHotkeyAdapter::shutdown` → `CGEventTapEnable(false)` + `CFRunLoopStop(rl)`. PR #388 fix in place; the comment at `hotkey.rs:311-314` correctly notes both APIs are documented thread-safe, so the lack of main-thread marshalling here is intentional.
|
||||
- ℹ️ `coordinator.rs:316-318` + `global_hotkey_runtime.rs:60-63` — `request_shutdown` is `#[allow(dead_code)]` and never set in production; supervisor loops poll it but nothing flips it (matches PR #392's "passive infrastructure" promise).
|
||||
- ℹ️ `global_hotkey_runtime.rs:19, 41-55` — `GlobalHotKeyManager` lives in `OnceCell<Arc<...>>` and is therefore never Dropped in production. On macOS this means the Carbon event handler is reaped only at process exit. Acceptable but worth noting if anyone tries to add hot-restart later.
|
||||
|
||||
### P2 — Press/release
|
||||
|
||||
- OK `coordinator/dictation.rs:11-28` — `handle_pressed_edge` swap-dedups via `inner.hotkey_trigger_held.swap(true, SeqCst)`; routing checks `panel_visible && !dictation_active` (PR #390 fix). The `dictation_active = !matches!(phase, SessionPhase::Idle)` snapshot is the correct guard: it lets a hotkey press during an in-flight dictation flow fall through to `handle_pressed`, even when the QA panel happens to be visible.
|
||||
- OK `coordinator/dictation.rs:53-67` — `handle_released_edge` symmetric: `panel_visible && !dictation_active → return`. If dictation_active was true when pressed (so it routed to `handle_pressed`), released will also bypass the QA short-circuit and reach `handle_released`. No mismatched-edge leak.
|
||||
- OK `coordinator/dictation.rs:30-51, 69-85` — Hold/Toggle phase matrix correct: `(Toggle, Idle) → begin`, `(Toggle, Listening) → end`, `(Toggle, Starting) → request_stop_during_starting`, `(Hold, Idle) → begin`, `(Hold, Listening released) → end`, `(Hold, Starting released) → request_stop_during_starting`. Other combinations no-op.
|
||||
- OK `coordinator/dictation.rs:87-96` + `coordinator_state.rs:87-93` — `request_stop_during_starting_state` only flips `pending_stop` when phase is exactly `Starting`; `finish_starting_session_state` consumes the bit at the Listening transition (`coordinator_state.rs:118-124`) and triggers immediate `end_session` via `BeginOutcome::PendingStop` at `dictation.rs:587-590`.
|
||||
- OK `coordinator/dictation.rs:451` — `acquire_recording_mute(inner, "dictation").await;` is properly awaited (PR #391 fix applied to dictation path).
|
||||
- ⚠️ `coordinator/dictation.rs:418-447` (level_handler) — runs on the cpal audio callback thread and calls `emit_capsule` synchronously. See P4 for the concrete bug; flagged here because P2's recorder-start path is the producer.
|
||||
|
||||
### P3 — End-of-session pipeline
|
||||
|
||||
- OK `coordinator/dictation.rs:595-602` + `coordinator_state.rs:178-184` — `start_processing_if_listening` only transitions `Listening → Processing`; if phase is anything else (`Idle`, `Starting`, `Inserting`, already-Processing), `end_session` returns Ok(()) immediately. Guards against stale pending_stop or duplicate IPC.
|
||||
- OK `coordinator/dictation.rs:607-620` — recorder + ASR are taken with session-id matching (`take_recorder_for_session` / `take_asr_for_session`), so a stale callback that lands after a session has been re-bumped won't pick up the wrong recorder.
|
||||
- OK `coordinator/dictation.rs:984-1005` — atomic Inserting transition: same `state.lock()` checks `cancelled` and flips phase to `Inserting`. Once `Inserting`, `cancel_session` rejects (`coordinator_state.rs:155-159` — `Idle | Inserting` → `None`). This is the audit HIGH #2 contract.
|
||||
- OK `coordinator/dictation.rs:1007-1031` — `paste_shortcut = prefs.paste_shortcut` flows into `inner.inserter.insert(&polished, restore_clipboard, paste_shortcut)` for non-Windows and into `insert_with_windows_ime_first(..., paste_shortcut, ime_target)` for Windows. PR #377 wiring confirmed; corresponding signature in `coordinator.rs:1673-1680, 1731-1745` and `insertion.rs:43-89`.
|
||||
- OK `coordinator/dictation.rs:1122-1126` — happy-path end_session clears `state.focus_target = None` before scheduling capsule idle.
|
||||
|
||||
- 🚩 `coordinator/dictation.rs:843-849` + `coordinator/dictation.rs:1153-1178` + `coordinator_state.rs:171-176` — **`focus_target` is not cleared when cancel hits during `Processing`.**
|
||||
- `cancel_session` in the `Processing` branch (`dictation.rs:1171-1173`) deliberately does NOT call `finish_cancel_session_state`, leaving phase + focus_target as-is so `end_session` can finish unwinding.
|
||||
- `end_session`'s "ASR-finished, cancelled" exit (`dictation.rs:845-849`) restores Windows IME, sets `phase = Idle`, returns Ok(()) — but never touches `focus_target`.
|
||||
- PR #387 (`ce82fcd9`) was framed as "clear `focus_target` on cancel regardless of phase", but the only code path that gained the unconditional clear is `finish_cancel_session_state` at `coordinator_state.rs:172`, which the Processing branch skips.
|
||||
- Concrete consequence: between cancel-mid-Processing and the next `begin_session`, the cached AX `focus_target` (a stale `usize` slot) is reachable by anyone reading `state.focus_target` (logs, debug dumps, future readers). It's overwritten by `begin_session_state` at `coordinator_state.rs:80`, so user-visible insertion uses the right value. Severity: minor leak / contract violation rather than user-visible breakage. Tests at `coordinator_state.rs:362-385` only validate the cancel happy paths via `finish_cancel_session_state`, so the regression slipped past PR #387's guard test.
|
||||
|
||||
- ⚠️ `coordinator/dictation.rs:1175` — even when cancel fires during `Processing`, the user immediately sees `CapsuleState::Cancelled`, but `end_session` may still be inside the ASR await for several seconds. The phase is still `Processing` until `end_session` reaches the `state.cancelled` check, so a fast retry-press will be quietly dropped (`begin_session_state` requires `Idle`). Not a bug per se — matches the design comment at `dictation.rs:1169-1174` — but worth a UX note.
|
||||
|
||||
- ⚠️ `coordinator/dictation.rs:1153-1163` — `cancel_session` swallows the result of `begin_cancel_session_state` for `Inserting`, only logging "cancel ignored". Acceptable, but there's no UI signal back to the user that their Esc didn't take. Minor.
|
||||
|
||||
### P4 — Capsule UI emission
|
||||
|
||||
- 🚩🚩 **`coordinator.rs:3684-3727`** — **`emit_capsule` does NOT marshal `window.show/hide` to the main thread on `400097ad`.** Verbatim from current source:
|
||||
|
||||
```rust
|
||||
fn emit_capsule(...) {
|
||||
...
|
||||
if let Some(window) = app.get_webview_window("capsule") {
|
||||
...
|
||||
let visible = !matches!(state, CapsuleState::Idle);
|
||||
maybe_position_capsule_bottom_center(inner, &window, payload.translation);
|
||||
if show_capsule && visible {
|
||||
if !show_capsule_window_no_activate(&app, &window) {
|
||||
let _ = window.show();
|
||||
}
|
||||
#[cfg(target_os = "macos")]
|
||||
crate::restore_main_window_key_if_active(&app);
|
||||
} else {
|
||||
hide_capsule_window_if_present();
|
||||
let _ = window.hide();
|
||||
}
|
||||
}
|
||||
let _ = app.emit_to("capsule", "capsule:state", payload);
|
||||
}
|
||||
```
|
||||
|
||||
This is the *pre-PR-#389* shape. PR #389's fix (`faf02ad4`, then `84ee3d96`) wraps the `if let Some(window)` block in `app.run_on_main_thread(move || { ... })`. Verified that neither commit is an ancestor of `400097ad`:
|
||||
|
||||
```
|
||||
$ git merge-base --is-ancestor faf02ad4 400097ad ; echo $? → 1 (NOT ancestor)
|
||||
$ git merge-base --is-ancestor 84ee3d96 400097ad ; echo $? → 1 (NOT ancestor)
|
||||
```
|
||||
|
||||
Reproduction reasoning: `coordinator/dictation.rs:418-447` builds `level_handler` as `Arc<dyn Fn(f32) + Send + Sync>` and hands it to `Recorder::start`. cpal calls it from the audio process callback thread; the handler then calls `emit_capsule(...)` (line 439-446). On macOS, `WebviewWindow::show()` / `hide()` and the `show_capsule_window_no_activate` (which calls `NSWindow.orderFrontRegardless`) hit AppKit assertions (`dispatch_assert_queue_fail` → SIGTRAP) when invoked off the main thread. The 33 ms throttle at `dictation.rs:417, 426-432` only limits frequency — every individual call is at the same thread-safety risk.
|
||||
|
||||
The same risk applies to QA's level_handler at `coordinator.rs:2282-2309`, which also calls `emit_capsule` directly from the cpal callback (line 2301-2308).
|
||||
|
||||
Severity: high. SIGTRAP would crash the app on long recordings; less catastrophic outcomes are stuttering audio (the audio callback misses its deadline waiting for AppKit) and `kAudioUnitErr_TooManyFramesToProcess`. PR #389 needs to land or be cherry-picked before this beta is shipped.
|
||||
|
||||
- OK `coordinator.rs:3726` — `app.emit_to("capsule", "capsule:state", payload)` stays on the calling thread; Tauri's event bus is internally thread-safe. No change needed regardless of PR #389.
|
||||
- OK `coordinator.rs:3739-3765` — `maybe_position_capsule_bottom_center` is the OS-level call inside the `if let Some(window)` block; it would be moved into the same `run_on_main_thread` closure once PR #389 lands.
|
||||
|
||||
### P5 — App shutdown
|
||||
|
||||
- OK `lib.rs:347-355` (RunEvent::Exit) — calls `stop_hotkey_listener` (Mac CGEventTap path, safe to invoke any thread per `hotkey.rs:344-355`), plus the 5 `global-hotkey`-backed `stop_*_listener`s that all marshal via `app.run_on_main_thread` (`coordinator.rs:344-358, 1313-1347`).
|
||||
- OK `lib.rs:348` — `TRAY_MICROPHONE_WATCHER_STOPPING.store(true, Relaxed)` correctly signals the watcher loop spawned at `lib.rs:540-548`.
|
||||
- ⚠️ `coordinator.rs:344-358, 1313-1347` — `app.run_on_main_thread` is fire-and-forget; the queued `Drop` may not run before the process exits. In practice this is fine (process exit reaps everything), but if Tauri's main-loop teardown beats the queued closure, Carbon `RemoveEventHotKey` is skipped. Same model as the pre-existing PR #169 fix for `qa_hotkey`, so flagging as an inherited limitation, not a regression.
|
||||
|
||||
- 🚩 **`coordinator.rs:2313` — `acquire_recording_mute(inner, "qa");` is missing `.await`.**
|
||||
- PR #391 (`6171df61`) made `acquire_recording_mute` `async fn` (`coordinator/resources.rs:122`) and updated the dictation call site to `.await` (`coordinator/dictation.rs:451`). The QA call site was missed.
|
||||
- Effect: the function returns an `impl Future` that is dropped on the next line. `spawn_blocking` is never scheduled, so `mute.holders` doesn't increment, the system audio mute is never engaged for QA, and the `[audio-mute] acquired by qa` log is never written. The corresponding `release_recording_mute(inner, "qa")` calls (e.g. `resources.rs:194`, `coordinator.rs:2324`) decrement holders that were never incremented (early `return` at `resources.rs:174` because `holders == 0`).
|
||||
- Compiler confirms it: `cargo check --manifest-path openless-all/app/src-tauri/Cargo.toml` emits
|
||||
|
||||
```
|
||||
warning: unused implementer of `futures_util::Future` that must be used
|
||||
--> src/coordinator.rs:2313:5
|
||||
2313 | acquire_recording_mute(inner, "qa");
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
= note: futures do nothing unless you `.await` or poll them
|
||||
```
|
||||
|
||||
- User-visible consequence: when the user opted into "Mute system output during recording" and triggers QA via Option, system audio is NOT muted (e.g. a YouTube playback continues). Dictation behaves correctly. Fix: insert `.await` on the call.
|
||||
|
||||
- OK `coordinator/resources.rs:184-188` — `release_recording_mute` falls back to synchronous `work()` when no tokio handle is present, so the recorder error monitor (a plain `std::thread::spawn`) can release safely. Drop of `AudioMuteGuard` shells out to `osascript` / `wpctl`, blocking on a std thread is OK.
|
||||
|
||||
## Cross-PR composition risks
|
||||
|
||||
1. **PR #391 + #389 incomplete merge.** The current `400097ad` contains PR #391 (which added the async hygiene the rest of the audit fixes assume) but is missing PR #389. Net result: the audio-thread → AppKit risk PR #389 fixes is still present *and* the QA codepath has a half-applied PR #391 (missing `.await` at `coordinator.rs:2313`). Both must land before this commit becomes a release candidate.
|
||||
2. **PR #387 + Processing branch interaction.** PR #387 introduced `state.focus_target = None` inside `finish_cancel_session_state`, but `cancel_session` deliberately skips that helper for `Processing` (`dictation.rs:1171-1173`) so the scheduled `end_session` can drive its own teardown. `end_session`'s cancelled-after-ASR exit (`dictation.rs:845-849`) was not updated to clear `focus_target`. The contract "clear focus_target on cancel regardless of phase" is therefore violated for cancel-during-Processing. Fix can be either (a) clear `focus_target` in the cancel-after-ASR branch of `end_session`, or (b) move the clear into `cancel_session` even for Processing (does not interfere with `end_session`'s own writes).
|
||||
3. **PR #390 + multi-bridge `hotkey_trigger_held`.** The dedup atomic is process-global. With both `hotkey` and `combo_hotkey` monitors running, the legacy modifier-only adapter and the custom-combo adapter share `inner.hotkey_trigger_held`. Currently exclusive (custom-combo only runs when trigger == Custom — see `coordinator.rs:413-426`), so no cross-contamination, but anything that allows them to coexist would corrupt the dedup. ℹ️ informational.
|
||||
4. **PR #392 (passive flag) is dormant**, as the prompt indicates. No interaction risk; calling out only that supervisor loops gracefully ignore `shutdown=false` so future RunEvent::Exit hookup is safe to land in a follow-up PR.
|
||||
|
||||
## Manual-verification checklist for the user
|
||||
|
||||
After cherry-picking PR #389 + fixing the missing `.await` in P5, verify on a running build:
|
||||
|
||||
**P4 — capsule main-thread (PR #389 confirmation)**
|
||||
- [ ] macOS arm64 build, dev profile, run a 3-minute continuous toggle dictation. App must not crash with SIGTRAP / `dispatch_assert_queue_fail`. Tail `~/Library/Logs/OpenLess/openless.log` while recording.
|
||||
- [ ] On macOS, capsule still appears once the first PCM frame is captured (50–200 ms after Recorder::start) and disappears 1.5 s after Done/Cancelled/Error.
|
||||
- [ ] On Windows, no SendMessage deadlock against the GUI thread during recording start/stop (capsule transitions complete within ~50 ms of phase changes).
|
||||
|
||||
**P5 — QA mute fix**
|
||||
- [ ] Set `prefs.muteDuringRecording = true` (Settings → Recording).
|
||||
- [ ] Play YouTube in Safari/Chrome.
|
||||
- [ ] Open QA panel via `Cmd+Shift+;`. Press Right Option to start QA recording. Audio playback **must** mute. Log line `[audio-mute] acquired by qa; holders=1` must appear in `openless.log`. (Without the fix, no log line + audio keeps playing.)
|
||||
- [ ] Release Right Option. Audio playback resumes; log line `[audio-mute] released by qa; holders=0` + `system output mute restored after recording` must appear.
|
||||
|
||||
**P3 — focus_target on Processing-cancel (PR #387 completeness)**
|
||||
- [ ] Start dictation, speak briefly, release hotkey to enter Processing. While ASR is awaiting result (within ~1 s window), press Esc to trigger `cancel_dictation`. End_session bails at `cancelled` check.
|
||||
- [ ] Inspect debug logs / state dump (or add a temporary log at `dictation.rs:849`): `state.focus_target` should be `None`. With current `400097ad` it remains `Some(...)`.
|
||||
- [ ] Start a fresh dictation in a different window. Insertion should still target the new window — this works today via `begin_session_state` overwrite (`coordinator_state.rs:80`), so the bug is silent until something else reads stale `focus_target` between the cancel and the next begin.
|
||||
|
||||
**P2 — pressed/released routing (PR #390 confirmation)**
|
||||
- [ ] Open QA panel (`Cmd+Shift+;`). Confirm Option starts QA recording.
|
||||
- [ ] Close QA panel. Start a normal dictation (Option). While dictation is running, open QA panel via `Cmd+Shift+;` (panel becomes visible while dictation_active=true). Press and release Option once — dictation must end normally (insert text), QA must NOT capture this Option edge.
|
||||
- [ ] Hold mode: Set HotkeyMode::Hold. Hold Option for 2 s, release. End_session must trigger on release (not on press).
|
||||
- [ ] Toggle mode: Set HotkeyMode::Toggle. Tap Option twice rapidly during Starting phase (within the 50–200 ms cpal init window) — verify `request_stop_during_starting` queues then end_session fires on the Listening transition (search log for `applying pending_stop edge → end_session immediately`).
|
||||
|
||||
**P1 — listener teardown sanity**
|
||||
- [ ] Quit the app via tray menu. `openless.log` should show ordered `stop_*_listener` calls; no panic / SIGTRAP at exit.
|
||||
- [ ] Relaunch and grant Accessibility (after prior reset). Confirm `[hotkey] CGEventTap 已启动` returns within 3 s; first hotkey press still works without app restart.
|
||||
|
||||
If any of the P4/P5 checks fail, this beta is **not** build-quality.
|
||||
Reference in new issue
Block a user