Skip to content

fix(coordinator): post-audit logic-review hotfixes - #393

Merged
appergb merged 1 commit into
betafrom
fix/post-audit-logic-review
May 10, 2026
Merged

fix(coordinator): post-audit logic-review hotfixes#393
appergb merged 1 commit into
betafrom
fix/post-audit-logic-review

Conversation

@appergb

@appergbappergb commented May 10, 2026

Copy link
Copy Markdown
Collaborator

User description

Summary

Two real bugs surfaced by the 2026-05-10 end-to-end logic review of beta after the 8-PR audit-fix wave landed (full report at `docs/logic-review-2026-05-10.md`, kept local).

Bug 1: `coordinator.rs:2313` — QA path missing `.await`

PR #391 (`6171df61`) made `acquire_recording_mute` `async fn` and updated the dictation call site (`coordinator/dictation.rs:451`), but the QA call site was missed.

Effect: when `mute_during_recording = true` and the user triggers QA, the returned Future was dropped on the next line, `spawn_blocking` was never scheduled, `holders` never incremented, system audio was NOT muted (YouTube/etc. kept playing into the QA recording). Both matching `release_recording_mute(inner, "qa")` calls became no-ops (early return at `holders == 0`). The Rust compiler was emitting `unused_must_use` for this exact line.

Fix: `+ .await`. Compiler warning gone.

Bug 2: `coordinator/dictation.rs:843-849` — `focus_target` not cleared on cancel during Processing

PR #387 (`ce82fcd9`) was framed as "clear focus_target on cancel regardless of phase", but the unconditional clear only landed in `finish_cancel_session_state`. `cancel_session` deliberately skips that helper for the `Processing` branch (so `end_session` drives its own teardown), and `end_session`'s "ASR-finished, cancelled" exit set `phase = Idle` but never touched `focus_target`.

Effect: cancel-during-Processing leaves a stale `usize` focus slot in `state.focus_target` until the next `begin_session_state` overwrites it. Today bounded (no documented reader on that interval), but violates PR #387's stated contract and is a silent footgun for future readers.

Fix: in the cancelled-after-ASR exit branch, take `state.lock()` once and clear both `phase` and `focus_target` together.

Test plan

  • `cargo test --lib` 185/185 pass.
  • `cargo check` clean. `unused_must_use` warning at coordinator.rs:2313 disappears.
  • CI Linux/Windows/macOS to verify.
  • Manual for Bug 1: enable mute_during_recording, play YouTube, open QA panel, press Right Option — audio should mute and `[audio-mute] acquired by qa; holders=1` appear in `openless.log`.
  • Manual for Bug 2: start dictation → release → during ~1s ASR await window press Esc; inspect logs / state. `focus_target` should be `None` immediately after cancel-during-Processing (was: stale until next session).

PR Type

Bug fix


Description

  • Add missing .await on acquire_recording_mute in QA path

  • Clear focus_target on Processing‑phase cancel in end_session


Diagram Walkthrough

flowchart LR
QA["begin_qa_session"] -- ".await" --> Mute["acquire_recording_mute('qa') (mute engages)"]
Cancel["end_session (cancel after ASR)"] -- "clear focus_target" --> Focus["state.focus_target = None (no stale handle)"]
Loading

File Walkthrough

Relevant files
Bug fix
coordinator.rs
Await QA audio mute call

openless-all/app/src-tauri/src/coordinator.rs

  • Add .await to acquire_recording_mute(inner, "qa") to prevent dropped
    future
+1/-1
dictation.rs
Clear focus_target on Processing cancel

openless-all/app/src-tauri/src/coordinator/dictation.rs

  • Clear focus_target when cancelling during Processing phase to prevent
    stale handle
  • Use single lock acquisition to set both phase and focus_target
+9/-1

Two real bugs surfaced by the 2026-05-10 end-to-end logic review of
beta after the 8-PR audit-fix wave landed.
1. coordinator.rs:2313 — `acquire_recording_mute(inner, "qa");` was
missing `.await`. PR #391 (`6171df61`) made the function `async fn`
and updated the dictation call site at coordinator/dictation.rs:451,
but the QA call site was missed. Effect: when a user has
`mute_during_recording = true` and triggers QA via Right Option, the
returned Future is dropped on the next line, `spawn_blocking` is
never scheduled, holders never increments, and system audio is NOT
muted (e.g. YouTube playback continues into the QA recording). Both
the matching `release_recording_mute(inner, "qa")` calls become
no-ops (early return at holders == 0). The compiler was emitting
`unused_must_use` for this site (verified before this commit).
Fix: add `.await`.
2. coordinator/dictation.rs:843-849 — PR #387 (`ce82fcd9`) was framed
as "clear focus_target on cancel regardless of phase", but the only
code path that gained the unconditional clear was
`finish_cancel_session_state`. cancel_session deliberately skips
that helper for Processing (so end_session can drive its own
teardown), and end_session's "ASR-finished, cancelled" exit at
dictation.rs:843-849 set phase=Idle but never touched focus_target.
Result: cancel-during-Processing leaves a stale `usize` focus slot
in state.focus_target until the next begin_session_state overwrites
it. Today the leak is bounded (no documented reader between cancel
and next begin), but it violates PR #387's stated contract and is a
silent footgun for any future reader of focus_target on that
interval. Fix: in the cancelled-after-ASR exit branch, take the
state lock once and clear both phase and focus_target together.
Source: docs/logic-review-2026-05-10.md (subagent end-to-end review).
185/185 lib tests pass; cargo check clean (unused_must_use gone).
Manual verification checklist for both fixes in the review doc.
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

387 - Partially compliant

Compliant requirements:

  • Clear focus_target on accepted cancels, including Processing.
  • Keep the Processing -> Idle transition in end_session.

Non-compliant requirements:

  • Regression test updates are not visible in the provided diff.

Requires further human verification:

  • Run the updated coordinator state-machine tests.
  • Manually verify cancel-during-Processing behavior in a real build.

391 - Partially compliant

Compliant requirements:

  • Await acquire_recording_mute() in the QA path.

Non-compliant requirements:

  • The non-blocking implementation changes for the broader audio-mute flow are not visible in the provided diff.

Requires further human verification:

  • Confirm the associated spawn_blocking changes in the audio-mute implementation.
  • Run the macOS timing/manual verification described in the ticket.
  • Verify the CI build.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ No major issues detected

@appergb
appergb merged commit 618fc7f into betaMay 10, 2026
4 checks passed
@appergb
appergb deleted the fix/post-audit-logic-review branch May 10, 2026 01:11
@appergbappergb mentioned this pull request May 10, 2026
5 tasks
pullBot pushed a commit to yimmy23/openless that referenced this pull request May 10, 2026
10 PRs landed on beta this cycle:
- Open-Less#377 paste shortcut configurable (issue Open-Less#360)
- Open-Less#386 TS UserPreferences updateChannel alignment
- Open-Less#387 focus_target leak on Processing-phase cancel
- Open-Less#388 [严重] MacHotkeyAdapter::shutdown stops CFRunLoop + tap
- Open-Less#389 emit_capsule window.show/hide off audio thread
- Open-Less#390 QA / dictation hotkey routing race
- Open-Less#391 audio-mute spawn_blocking (async hygiene)
- Open-Less#392 hotkey supervisor + global dispatcher exit signal
- Open-Less#393 post-audit logic-review hotfixes (QA mute .await + focus_target Processing branch)
- Open-Less#394 in-process credentials cache (kills repeated Keychain prompts)
Bump 4 files: package.json, tauri.conf.json, Cargo.toml, Cargo.lock.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@appergb