fix: recording stuck-stop watchdog, keychain persistence, error feedback, overlay stop path#55
Conversation
- Route stuck-stop recovery through reset_finished_recording_state. - Clear assistive session metadata and transient mode fields consistently. - Make toggle and hold recovery share the same full reset surface.
- Share STOP_TIMEOUT between toggle and hold stop paths. - Wrap hold-path process_recording in tokio timeout and recover to Idle on expiry. - Add a paused-time regression test that hangs process_recording and verifies session cleanup. - Enable Tokio test-util for deterministic watchdog coverage.
- Mark CODESCRIBE_DATA_DIR bridge coverage as serial across the crate. - Serialize optimistic-overlay compensation tests that install shared controller/listener state. - Keep cargo test --workspace deterministic under default parallel test execution.
CODESCRIBE_DATA_DIR is a documented, production-valid override for the data directory, but is_test_env() treated it (and CI) as a Keychain kill switch. Setting it silently disabled the Keychain, so API keys and OAuth tokens stopped persisting across restarts. Drop CODESCRIBE_DATA_DIR and CI from is_test_env(); CODESCRIBE_DISABLE_KEYCHAIN is now the only user-facing opt-out. Test binaries are still detected via the existing harness signals (target/**/deps/ exe path, RUST_TEST_THREADS, cfg!(test)), so the suite never touches the real Keychain. rust.yml runs no tests, so removing CI has no effect on CI. Extract the pure policy into keychain_disabled_by_signals() and pin the regression with unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Route warmup watchdog aborts through the recording stop-side callback. - Route overlay listener errors through the same stop-side cleanup. - Keep abort cleanup idempotent so normal finalize still fires onRecordingStopped once.
…key liveness A 400 (e.g. model_not_found) means the server processed the request and the key passed authentication, so the key is live. Previously such responses fell through to the Network bucket and surfaced a misleading "could not verify" message for a working key. Classify any non-quota, non-auth client error (4xx) as Ok; keep 401/403 as Invalid, 429/402/quota as NoQuota, and 5xx/transport failures as Network. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A failed processing/transcription result was only logged via error!(), so the user got no signal. Broadcast an engine Warning over the existing IPC channel in the Err branch of handle_processed_recording_result; the bridge forwarder turns it into listener.on_error + a tray Error state on the SwiftUI surface. Reuses the wired Warning variant — no new event architecture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A paste failure early-returns from process_stopped_recording via `?`, which previously skipped the final-transcript save that followed it — losing the AI-formatted layer for that dictation. Move the needs_final_save block ahead of the paste attempt so the formatted text is recoverable from history even when delivery fails. History write is a disk-only side effect, so the user-visible side-effect order (paste) is unchanged on happy path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness improvements and bug fixes across the codebase. Key changes include surfacing transcription failures to the user via IPC warnings, implementing a watchdog timeout for hold-stop processing to recover to an Idle state if it stalls, and persisting the formatted transcript before attempting to paste to prevent data loss. Additionally, the Keychain-disable policy is refined so that setting CODESCRIBE_DATA_DIR or CI does not disable Keychain persistence, and non-auth 4xx client errors are now treated as successful key liveness checks. The macOS overlay state is also updated to properly abort recording sessions on errors. I have no additional feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Apply clippy::question_mark on stable 1.97 in parse path split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace manual Option::filter implementation in non_empty_transcript with Option::filter, per clippy::manual_filter on stable 1.97. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR type: bugfix (runtime hardening)
Problems addressed
All findings come from an adversarially-verified deep quality recon (42 findings total; the P1s below were CONFIRMED by independent verification):
finish_recording_lockedawaitedprocess_recordingwith no timeout. A hung STT final pass (e.g. Metal/Whisper stall) left the app permanently in Busy with hotkeys ignored. The toggle path already hadSTOP_TIMEOUTprotection; the hold path did not.recover_from_stuck_stopbypassed the full session reset, leakingIS_ASSISTIVE_SESSIONso the tray misreported an assistant session after recovery (both toggle and hold paths).CODESCRIBE_DATA_DIRsilently disabled the Keychain.is_test_env()treatedCODESCRIBE_DATA_DIR(andCI) as "test environment", so API keys and OAuth tokens never persisted across restarts for anyone using the documented data-dir override.error!()log. The user got no feedback at all when a recording failed to transcribe.fireWarmupWatchdog/onErrornever ran the stop-side path, sochat.dictationBlockedstayedtrueuntil the next successful dictation session.model_not_found) as network errors, even though a 4xx response proves the key passed auth.Fixes
app/controller/mod.rs: sharedSTOP_TIMEOUTconst; hold path wrapped intokio::time::timeoutwith the samerecover_from_stuck_stoprecovery; recovery now performs the fullreset_finished_recording_state.app/controller/mod.rs: transcription failure now emits the existingEngineEventWire::Warning(already wired end-to-end: tray Error state +on_erroron the Swift side); formatted transcript is persisted before paste.core/config/keychain.rs:CODESCRIBE_DATA_DIRandCIremoved fromis_test_env();CODESCRIBE_DISABLE_KEYCHAINremains the only user-facing kill switch. Test binaries are still auto-detected (cfg!(test) + cargo deps-path +RUST_TEST_THREADS), so the test suite never touches the real Keychain.core/llm/key_liveness.rs: 401/403 → invalid key, 429/402 → quota, other 4xx → key verdict OK (server answered ⇒ auth passed), 5xx/transport → network. No new enum variant; bridge/Swift untouched.macos/.../OverlayState.swift: minimalabortRecordingSessionpath used by warmup watchdog andonError— firesonRecordingStoppedexactly once (guarded byfinalized), sodictationBlockedresets after failures.bridge/src/hotkeys.rs: test-only serialization of hotkey tests that share process-wide state (pre-existing race surfaced by the new watchdog test).Test plan (commands actually run, all green at final HEAD)
New tests: paused-time watchdog regression test (hangs
process_recording, asserts Idle + cleared session fields), keychainis_test_envcoverage, key-liveness classification cases (400/401/403), Warning-on-failure and save-before-paste invariants.Smoke notes (manual, recommended before merge)
STOP_TIMEOUT, hotkeys stay alive, tray shows no stale assistant session.CODESCRIBE_DATA_DIR=/tmp/csdata+ save an API key in Settings → restart → key still present.Out of scope / backlog
Streaming-pipeline behaviorals (stale corrections, unbounded window_text, utterance joining), agent-turn cancellation not aborting spawned tools, multi-turn composer images, audio
auto_silencedefaults, thermal shedding order, FFI panic strategy — all recorded in the recon report as backlog; deliberately not touched here.🤖 Generated with Claude Code