Skip to content

fix(review): remediate 3 P0s + P1s found by adversarial review of the reliability batches - #81

Merged
GalaxyRuler merged 12 commits into
mainfrom
codex/reliability-fixes
Jul 20, 2026
Merged

fix(review): remediate 3 P0s + P1s found by adversarial review of the reliability batches#81
GalaxyRuler merged 12 commits into
mainfrom
codex/reliability-fixes

Conversation

@GalaxyRuler

Copy link
Copy Markdown
Owner

Before Submitting This PR

  • I have searched existing issues and PRs to ensure this isn't a duplicate
  • I have read CONTRIBUTING.md

Top of the stack: #78#79#80 → this. Merge bottom-up. The three PRs below should not merge without this one — it fixes P0s introduced/left by them.

Human Written Description

I had the reliability batches adversarially reviewed after they were already written and passing, and the review found three P0s that both the code review and my device testing had missed — all of them on failure paths rather than happy paths. That is the lesson here: everything worked when things went right. A password field whose role couldn't be classified on Linux got read anyway; deleting a recording whose file was locked reported success and orphaned the audio; and approving a recovered dictionary rule silently destroyed the very rule the migration had saved. This PR fixes those plus six related P1s.

P0s fixed

  1. Linux secure-field capture failed open. If an AT-SPI element lacked getRoleName or returned a non-string, the password check was skipped and the protected text was read. Now a role that cannot be classified is a secure-check failure (fails closed), matching the Windows and macOS behavior.
  2. Deleting a recording could report success while the audio stayed on disk. A file-delete failure after the row delete was only logged — no reported failure, still counted as deleted. Now the last owning row is staged in a transaction, the file is removed first, and the row is rolled back if the file cannot be deleted, with a typed failure surfaced so it can be retried. Nothing is silently orphaned. (The old test asserted the orphan as correct; it has been rewritten.)
  3. Approving a v3-recovered dictionary rule destroyed it. The candidate was consumed, then promotion early-returned because the phrase already existed, keeping replacement_of = None. Promotion now merges the recovered rule into the retained entry, and re-records rather than drops it when two distinct rules conflict.

Also fixed

  • Locked-language filler removal could delete dictionary-produced output (x → um under locked English). Dictionary output phrases are now protected from language-default filler removal; custom fillers stay authoritative.
  • Recording-ownership mutations serialized (reference-count vs concurrent insert).
  • Language migration no longer stamps invalid tuples (Single+auto, Multilingual+<2 languages) as current.
  • Model-switch rollback is conditional (CAS) so it cannot clobber a newer language change made during model load.
  • BCP-47 normalization stops applying script/region casing after a singleton subtag (en-US-u-ca-gregory).
  • Dictionary phrases are sanitized before v3 identity is established (prevents later duplicates).
  • Three non-discriminating tests made real (model-switch failure branch, settings-writer guard, frontend language mode).

Testing

  • Full Rust suite: 768 passed, 0 failed (implementation + independent verification run).
  • tsc, ESLint, Prettier, translations, settings-lock coverage, cargo-deny: all pass.
  • Every P0 has a test confirmed to fail without its fix. Independently re-proved for P0-3: reverting the merge logic to the old early-return makes approving_v3_recovered_rule_merges_into_retained_entry fail; restoring it passes.

Still wants real-device verification (cannot be proven statically): Linux AT-SPI over-block behavior on real desktop apps, locked-file deletion against the real Windows filesystem, and model-switch rollback under genuine concurrency.

AI Assistance

  • AI was used (please describe below)

If AI was used:

  • Tools used: Codex (implementation + adversarial review), Claude Code (audit, review verification, red→green proof)
  • How extensively: the batches and these fixes were AI-implemented from a human-approved plan; the P0s were found by an adversarial AI review of already-green code and independently verified against source before fixing

🤖 Generated with Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c0066f681

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +492 to +495
let _recording_mutation_guard = self
.recording_mutations
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Release the recording lock before retention cleanup

With the default retention settings (PreserveLimit, history limit 5), saving the sixth recording makes cleanup_old_entries() produce deletion candidates while this guard is still held. That cleanup path calls delete_entries_and_files(), which attempts to lock the same non-reentrant recording_mutations mutex again, so the save path deadlocks instead of completing the transcription history write. Drop the guard before running retention cleanup or use an inner deletion helper that does not reacquire it.

Useful? React with 👍 / 👎.

Comment on lines +87 to +88
if settings.adaptive_language_shortlist == snapshot.written_adaptive_language_shortlist {
settings.adaptive_language_shortlist = snapshot.previous_adaptive_language_shortlist;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep the language tuple atomic during rollback

If a user changes the selected language while load_model() is running, and that language is already present in the switch-written shortlist, change_selected_language_setting updates only selected_language/mode and leaves the shortlist equal to written_adaptive_language_shortlist. On load failure this branch restores the old shortlist while preserving the new selected language, which can leave Single mode pointing at a language absent from the shortlist and break the language picker/cycling invariants. Treat the language fields as one CAS tuple or revalidate after the partial rollback.

Useful? React with 👍 / 👎.

Batch 1 changed set_active_model to return ModelSwitchOutcome { reason }.
The Playwright mock still returned the pre-batch unit (null) shape, so
modelStore.selectModel's result.data.reason access threw during first-run
onboarding, stalling the model->shortcut step. Update the mock to the real
shape and null-guard the frontend read defensively.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Base automatically changed from codex/reliability-batch-1 to main July 20, 2026 20:34
@GalaxyRuler
GalaxyRuler merged commit d8587bc into main Jul 20, 2026
15 checks passed
@GalaxyRuler
GalaxyRuler deleted the codex/reliability-fixes branch July 20, 2026 20:35
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.

1 participant