Audit tier1 - #51
Conversation
|
Important Review skippedToo many files! This PR contains 758 files, which is 608 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to Pro+ to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (758)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR adds synchronized settings updates, preview-only dictionary corrections, safer model migration, ordered dictation insertion, improved clipboard and AT-SPI verification, model auto-unload lifecycle handling, concurrency protections, text-processing fixes, desktop shortcut mappings, localization, and extensive Core/Linux test coverage. ChangesCore persistence and text processing
Model and dictation lifecycle
Insertion and Linux integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…s (§9 L6) The subfolder setting was joined directly onto the vault path and the directory created with no validation, so values like ../outside (or a rooted path) wrote notes outside the selected vault, and a symlinked child silently redirected writes anywhere on disk. ExecuteAsync now rejects rooted subfolders, requires the normalized join to stay at or under the normalized vault root (separator-boundary compare, so /vault-evil cannot pass for /vault), and then resolves the target component-by-component (64-link budget) before any directory is created — an in-vault symlink target is allowed, an outside target is refused, and refusal happens before CreateDirectory so no directories materialize outside the vault. Daily-note mode additionally re-checks the resolved note file itself before appending, because FileMode.Append follows an existing symlinked .md out of the vault; individual-note mode needs no such check since FileMode.CreateNew refuses existing links. Nonexistent segments are treated as literal via Path.Exists (not Directory.Exists), which keeps dangling escaping links rejected. Known residual, accepted for this tier: a symlink swapped in between check and append (TOCTOU) requires pre-existing in-vault write access. New failure strings localized in en/de/es/ru. Nine new tests cover traversal, rooted, normalize-out, prefix-bypass, nested-legitimate, in-vault and outside-vault symlinks, traversal-through-link directory creation, and the daily-note append escape; all mutation-proven.
…ump (§12 M4; closes §9 L1) Ten streaming plugins each carried their own copy of the WebSocket lifecycle — socket creation, fragmented receive assembly, send serialization, terminal signaling, receive-fault propagation, close budgets, abort, disposal — and the copies had drifted: Gladia, Soniox, Speechmatics, SmallestAI, and Reson8 treated a pre-terminal close as success (silently accepting truncated transcripts), Reson8 accepted any flush confirmation regardless of ID, ElevenLabs could double-emit its committed transcript, and xAI/ElevenLabs waited unbounded on the send lock before their bounded close. The SDK now owns the lifecycle in WebSocketSessionPump: explicit states, one send gate for startup/audio/keepalive/finalize/close, complete fragmented-message assembly with a size cap, CAS first-fault that wins atomically over cancellation, required-terminal policies (normal close or EOF before the provider's documented terminal frame is an incomplete-stream failure), bounded close with exactly-once abort, shared-disposal task with a deferred transport reaper, and subscriber isolation. Providers are adapters supplying only URI/auth, payload encoding, inbound parsing, and terminal policy, over an injectable transport so the conformance suite runs without a network. Closes §9 L1 inside the AssemblyAI adapter: finalize snapshots the residual audio buffer (previously dropped), pads it with silence to the provider's 1600-byte / 50 ms minimum chunk, and sends it under the same serialized batch as Terminate so nothing can interleave. Review hardening on top of the migration: shared finalize task made uncancellable so one caller's token cannot poison other callers; cancelled mid-batch sends mark the outbound stream indeterminate and fail later sends loudly; OpenAI finalize waits for every committed item (not just the final one); terminal-before-readiness faults startup instead of stranding it; xAI's 10 s readiness budget no longer covers the TCP/TLS handshake. Tier 1 fault-handling guarantees are pinned by the untouched oracle suites (failure propagation, disposal, eight provider protocol suites, streaming coordinator). New conformance/adapter/finalize tests are mutation-proven, and all completion awaits are bounded at 10 s so a regression fails instead of hanging CI.
… (§12 M4, absorbs §9 L1)
…sactional contract (§12 M1) File-backed state was independently implemented across Core, Linux, and plugins: Settings used a fixed .tmp plus best-effort backup copy, most stores staged through AtomicFileWrite but disagreed on synchronization, backup recovery, corruption handling, mutation-before-write ordering, rollback, and whether callers observed failure; the two memory plugins carried their own temp/replace implementations because plugins cannot reference Core. AtomicJsonStore<T> now owns the transaction layer above AtomicFileWrite (which stays the sole staging/replace/permissions implementation and gained only an internal staged-write observer for fault-injection tests): per-path process-local coordination shared across instances AND across closed generic types, serialized update transactions whose commit decision is made on the serialized form (object equality would compare an in-place mutation against itself and drop it), last-known- good backup written before the primary with backup failure aborting the transaction, explicit corrupt-file policies (preserve exact bytes once per distinct content with source permissions, or throw), backup recovery that atomically restores the primary, unreadable files surfacing instead of becoming writable defaults, cache publication and revision bump only after commit, and precise rollback — on a failed write the cache is dropped only when it actually diverged from the committed form. Migrated: Settings, History, Dictionary (+CSV partial), Profile, Prompt Actions, Snippets, Error Log, Linux preferences, plugin-host settings/secrets, both Watch Folder stores, and — via the new SDK IPluginStateStore<T>/OpenStateStore with leaf-name + containment validation and a default-throwing interface member — FileMemory and OpenAI Vector Memory (whose whole-operation serialization is restored; a dropped semaphore had let a concurrent clear be undone). Best-effort callers keep their outward policy but no longer publish uncommitted memory. SettingsChanged now fires outside the store lock as one unit under a service gate, only when the stored value actually changed. Deliberate behavior choices per the audit design: unreadable (as opposed to corrupt) settings/preferences surface the error rather than falling back to defaults-then-save, which would clobber a recoverable user file; ErrorLog and Watch Folder history retain their explicitly best-effort degrade-on-unreadable contracts. PA28's Save-idiom call sites and §12 M3's installer AtomicFileWriter are out of scope. One shared conformance matrix exercises every migrated store adapter for concurrent updates, interrupted writes (staged-hook fault injection), corrupt primaries, failed replacement, rollback, permissions, and temp uniqueness; engine behaviors are additionally unit-tested and mutation-proven.
…ementations preserved First fork-side merge from the parent repository. Resolution policy: the fork's Linux implementations win wherever the trees diverged — all plugins (including both sides' independent ElevenLabs work), the Core stores (freshly migrated to AtomicJsonStore), the streaming sessions (freshly migrated to WebSocketSessionPump), the SDK, CI workflows, and docs. The fork's deliberate exclusions stay excluded: the entire src/TypeWhisper.Windows WPF app (upstream's modifications and new files, including ones git's directory-rename detection redirected into src/TypeWhisper.Linux, were not imported), WPF plugin SettingsViews, Windows packaging workflows/scripts, and the LiveTranscript/ GraniteSpeech plugins. Upstream deleted the Profile/PromptAction stack in favor of a workflow model; the fork keeps its stack. What landed: dependabot config, THIRD-PARTY-NOTICES, TESTING_GUIDE, artifacts/ gitignore entry, ExportLabels, self-contained sync JSON models, Marian doc comments, CLI support helper, and release-note docs. Roughly 120 upstream additions that reference upstream-only APIs (user-data sync feature, plural-hotkey settings model, Windows-app source-text layout tests, Store packaging) were dropped rather than half-imported; ~/Documents/UPSTREAM-MERGE-REPORT.md lists every excluded file and the upstream features that need deliberate porting (mic-priority stabilization, wizard lifetime fix, uninstall data protection, Parakeet update path, WhisperCpp CUDA installer). Validated: full solution build 0/0, Core battery 192/192, Linux battery 91/91, PluginSystem battery 179/180 — the single failure is the known load-sensitive CloseTimeout wall-clock bound, 4/4 green in isolation.
…AI-Compatible keys become clearable (§9 L2)
A stored OpenAI-Compatible profile API key could never be removed:
GetItemsAsync deliberately never echoes secrets (null), the host
converted that null to "" on load and blindly serialized every field's
current value on save, so SetItemsAsync received "" for both an
untouched secret and one the user explicitly emptied — and mapped both
through NullIfWhiteSpace to 'keep'.
The host collection editor now tracks per-field user modification (set
only by setter-routed edits; initial population and __id identity
repair reset it) and serializes Secret-kind collection fields as null
when untouched and as their actual value — including "" — when
modified. Secret modification also participates in draft detection so
an ambient refresh cannot rebuild the field VMs and silently drop an
explicit clear. Backward-compatible: OpenAI-Compatible's api-key is the
only Secret-kind collection field in the tree (Webhook's secret field
resolves as Multiline), and plugins receiving null hit exactly the same
keep-path their NullIfWhiteSpace guards applied to the old "".
The plugin now treats null as keep, "" with a stored key as an explicit
clear — deleting the secret through the paired host op before the
profile-set swap and dropping the model catalog under the existing
credentials-change rule — "" without a stored key as a no-op, and
non-blank as replace. The api-key field description was rewritten in
all four plugin locales; the old text ('Leave blank to keep the current
key') described the unfixable pre-fix behavior.
Both sides are mutation-proven: always-serialize-Value fails the host
suite; reverting keep-on-blank fails the plugin clear test.
…12, reduced scope) The audit's shared-allocator consolidation was re-scoped with the user: recon showed every collision bug it targeted was already closed by site-specific fixes (RecorderFileNamer suffixing, Watch Folder atomic create-new, Obsidian create-new plus its interprocess append lock), so a repo-wide allocator would have added a public plugin API for one consumer without closing any defect. Two real residuals remained. Recorder transcript sidecars were written with overwrite semantics after the WAV committed, so a foreign file claiming the .txt path in that window was silently destroyed. The sidecar now goes through AtomicFileWrite.WriteAllTextCreateNew; on collision the foreign file is preserved byte-for-byte and the existing StatusTranscriptSaveFailed status is surfaced. Deliberately no independent suffix: loading and deletion assume the .wav/.txt stems match, and failing safely beats an undiscoverable sidecar. AtomicFileWrite.PublishCreateNew could betray its no-replace contract: on filesystems without hard-link support it fell back to an overwrite-capable File.Move whose existence check and rename are separate syscalls. The fallback is now renameat2(RENAME_NOREPLACE) on Linux (EntryPointNotFoundException on pre-2.28 glibc fails closed), File.Move(overwrite: false) on Windows, and a fail-closed IOException anywhere no non-replacing primitive exists. Mutation-proven: reverting the Recorder call fails the foreign-sidecar test; an overwrite-capable fallback fails the fail-closed test (which forces a non-EEXIST renameat2 failure via the staged-write observer — collision tests alone cannot reach that branch on RENAME_NOREPLACE filesystems, a vacuity found and fixed during verification).
… bypasses (§12 M2; absorbs §6 L1; co-lands PA57) Seventeen production files launched or supervised child processes with their own copies of timeout, cancellation, pipe-drain, kill, and reap logic, and the copies disagreed: probes that never drained redirected pipes (a chatty gdbus response could wedge the GNOME Window Calls check past its budget — §6 L1), an ffmpeg invocation that embedded quoted user paths in a shell-style argument string (PA57's injection condition), TTS players with hand-rolled Exited handling, and a pactl monitor with its own restart logic. ProcessRunner is now the one supervisor: typed one-shots (RunProbe / RunOneShotAsync with Exited/TimedOut/StartFailed outcomes, Discard/ Utf8/Binary capture, and an explicit post-exit pipe policy — RequireEof for parsed output, AbandonAfterGrace for children like xclip whose selection daemon inherits our pipes), long-lived sessions (line-mode pumps, idempotent Terminate/Dispose converging on one kill/reap task), and detached process/URI launches that reap the launcher parent without touching its descendants. Commands are argv-only — no shell strings. Caller cancellation kills the owned tree, reaps, then rethrows; private timeouts do the same cleanup and return TimedOut; a child that closes stdin early no longer escapes cleanup with the deadline unenforced. Plugins reach the supervisor through IPluginHostServices.Processes (default-throwing member), scoped per plugin: sessions are tracked, TerminateAll stops work on unload, and a retired scope refuses new launches while a failed deactivation leaves it usable. PA32 (process- group containment) and PA33 (kill on unsignaled failure) are NOT absorbed — internal seams only. All seventeen bypasses migrated; conformance is proven against a real controllable child process (stdin/output pressure, cancellation vs timeout races, tree kill, delayed exit, abandoned pipes, sessions, detached launch). Review hardening on top: bounded session drain so a descendant holding pipes cannot hang Completion (and leak TTS temp WAVs), the xclip AbandonAfterGrace policy restored, and the ChatGPT no-browser error path made reachable again. Key behaviors are mutation-proven, including the §6 L1 rule that a timed-out probe never reads as installed.
…stop lifecycle) Overlay (major): feedback expiry now arms at PRESENTATION, not publication — a toast that loses arbitration keeps its display budget while suppressed instead of expiring retired unseen behind a recording, which silently dropped every detection-failure toast raised mid-session. Workflow ownership resets only on Hide (explicit relinquish) and slot retirement; a state-driven blank published mid-workflow through Show/Update keeps ownership, so a streaming command's terminal outcome still ranks TerminalFeedback. Both pinned by tests proven against reverted-behavior mutants. Ducking (major): _isDucked returns to end-of-duck computation with a separate _duckInProgress re-entry guard, restoring the invariant that RestoreAudio no-ops while a duck is mid-flight — the eager flag let a concurrent restore put the original volume back and drop the entry just before the duck's queued set-volume landed, leaving the stream ducked with nothing tracking it. Speech (minors): Dispose waits a bounded 500ms for the stop worker so a well-behaved TTS provider is stopped before process exit (children are not killed by parent exit) while a hung plugin costs only the budget; the ReadBack toggle-off and superseded-launch paths complete their detached requests, closing the same per-toggle CTS leak PA74 fixed; and SpeakAsync tolerates a disposed cancellation source on a stale deferred launch.
…ardening, shared test helper, transform cleanup) - Rename the Pester rejection test to drop the scalar-categories claim the @() wrap silently promotes, and document that canonical categoryNames precedence (not manifest order) picks the legacy singular category. - Note that ComputeContentFingerprint survives only for tests fabricating a published identity (production markers come from the packaging script), and why WritePublishedIdentity deletes the marker before hashing. - Harden SettingsUpdateGuard: find the repo root via the repository-unique TypeWhisper.slnx marker and assert the src scan examined at least one file after bin/obj exclusion. - Deduplicate SetTranscriptionEngines into a linked shared helper (tests/PluginManagerTestAccess.cs) used by the integration composition and recorder view-model suites. - Wrap the audio-owner interleaving test's transform workflow in try/finally so a mid-test failure still cancels dictation, awaits the failed transform start, and stops a live transform session via a queued cancel command instead of leaking the shared audio capture.
…LI discovery, sun_path headroom - smoke-test: tarball/AppImage containers now call install_ubuntu_probe_infrastructure explicitly instead of getting dbus-run-session only through dbus-x11's transitive Depends on dbus-daemon; xvfb moves out of install_ubuntu_runtime since every caller now installs it via the probe helper, keeping the runtime list to the app's closure. - CliInstallService: dev source-root discovery walks up from AppContext.BaseDirectory (bounded to 8 levels) until a directory containing TypeWhisper.Cli appears, instead of a fixed four-parent hop that lands wrong for RID-specific or publish output layouts; packaged Cli/ candidate precedence and the test seam are preserved, and a null source root limits discovery to the packaged candidate. Tests pin the net10.0, RID, and RID/publish depths. - SettingsBackupServiceTests: shorter socket-test path segments (sock/Data/s.sock) so the bound Unix socket path stays under Linux's 108-byte sun_path limit, which the long temp-dir prefix was already brushing against.
- ProcessRunnerTests: the unsupported-stdin fault only surfaces once the private deadline completes the exit wait, and the runner then kills the child; a 500 ms deadline could kill a slow-starting child before it published its PID. Raise the deadline to 3 s so PID publication wins while the run still finishes inside the 5 s WaitAsync guard. - AppHotkeyReconcileTests: use Assert.Equal instead of message-only Assert.True so failures report expected and actual values; keep the description parameter as a case label via an explicit discard. - DictationOrchestratorStartFeedbackTests: production swallows cue exceptions, so record the lease and disposal state the cue observed and assert after the helper returns instead of asserting in the callback. - DictationSectionViewModelTests: construct SystemCommandAvailabilityService with FakeProcessRunner so the capability snapshot never probes host commands from microphone-selection tests. - DictationToggleGateTests: document that the barging assertion depends on SemaphoreSlim.Release handing the permit to the queued waiter while m_currentCount stays zero (runtime behavior, not a gate contract). - EvdevDeviceReaderTests: raise the concurrent-dispose threshold to 950 ms (serialized disposals burn two 500 ms waits, so detection is preserved) and make the poll-parking probe fail with a message naming the probe, distinguishing wchan-less/hidepid hosts from a reader that never parked, instead of timing out with a bare TaskCanceledException. - InputAccessSetupHelperTests: resolve the lock-prefix/group-remove shim commands through the PATH captured before the fixture restricts it, failing with a command-specific message when resolution misses. - TextInsertionServiceTests: refresh the suppression comment to describe the current convenience overload including optional clipboardToolName. - DictationOrchestratorStreamedCommandTests: note that Typed is cumulative across both phases so the repeated FirstDelta is intentional. - Localization catalogs: move the four Shortcuts.NativeDictation* entries to their alphabetical slot (after MoreShortcuts, before NoAutomaticSetup) identically in en/de/es/ru; values unchanged.
PR69: FormatDynamicHotkeyRejection falls back to unlocalized diagnostic text for unmapped kind/reason pairs instead of aborting the apply pass; HotkeyService/control-socket disposal moved after the HTTP quiesce so an admitted profile-toggle request cannot touch a disposed service; the evdev reader's DisposeAsync no longer closes raw fds under a worker that outlived the wake budget (the worker's own finally disposes on return); ClipboardFallbackMessage snapshots LastFailureReason once; the evdev wake-fd path verifies cancellation before reporting a cancel and surfaces a wake-without-cancel invariant breach; input_group_member restores globbing with set +f on every return; profile-toggle validation maps the three collision statuses explicitly with a neutral fallback for future statuses; clipboard tool-name identities become LinuxCapabilitySnapshot constants; DisplayName fallback documented. PR70: SetOverlayState derives publications from the coordinator's slot state instead of the orchestrator's mirror, so expired feedback cannot be resurrected from stale mirror flags; the indeterminate-commit exception now carries PublishedPath and the watch-folder records it so a failed directory sync still points history at the transcript on disk; watch-folder readiness gates only plugin-qualified selections (blank or non-plugin IDs reach the processor's immediate invalid-model error); ApplyConfiguredMicrophone persists its re-resolution only when the captured selection is still current, re-running against a newer one; OneShotTimer guards its callback so a dispatcher throw during shutdown cannot kill the process; RegistryPlugin caches the legacy category mapping on first read. PR71: known-extension requests without ffmpeg answer 503 audio_importer_unavailable from the shared transcription path (covers local-file and upload routes) instead of a generic 500, with an endpoint test; the backup walk's guarded-secret set is call-local, threaded through the traversal, so concurrent CreateBackup calls on the singleton cannot see each other's capture.
…yload AudioDuckingService.RestoreAudio returned immediately whenever it landed while DuckAudio was still running, because _isDucked only flips true in the duck's finally. Shutdown, disposal, or session loss racing a duck therefore dropped the restore entirely: the duck went on to lower every sink input and nothing ever put the volumes back, leaving the user's playback quiet for good. Record the request in _restorePending and run it from the duck's finally once the last set-volume has landed. That keeps the ordering guarantee the guard existed for -- a restore can still never overtake a queued duck -- while closing the path where the restore was silently discarded. The RPM smoke check piped rpm2cpio into cpio under `set -o pipefail`. cpio stops reading at the archive trailer, so a payload whose padding is still being written hands rpm2cpio a SIGPIPE and the release gate rejects a valid RPM. Stage the payload to a file and extract from it.
…toast, and pin overlay/registry edge cases in tests
…ection toast, overlay/registry test hardening
Audit tier 4: correctness backlog fixes (PA72-PA75, PA85, PA89, PA96-98, PA105, PA111, PA113, PA116)
Code audit tier 3.2: PA52-PA58
Audit tier3.1
Audit tier3
Both sides fixed the RPM rpm2cpio SIGPIPE independently; kept the staged extraction with tier1's temp-file cleanup. Tier1's install_fedora_runtime is gone on this side — the RPM's declared dependencies must resolve inside the container to stay validated — so the fedora smoke image now pre-bakes only probe tooling (dbus/Xvfb), the ubuntu bake also pre-installs its probe tooling, and tier1's dnf mirror-retry hardening carries into the surviving dnf invocations.
Audit tier2
The tarball flow wrote a file into the install root and demanded the installer preserve it across a reinstall, which made the second install refuse with "recorded destination was customized" and failed the gate. Nothing writes there. TypeWhisperEnvironment.BasePath is a fixed $XDG_DATA_HOME/TypeWhisper, and BundledPluginDeployer only reads bundled plugins out of the install root, deploying them to that data path. The assertions date from de9801b, when INSTALL_ROOT *was* BasePath and the installer's KEEP list was load-bearing; 4883029 moved the payload to typewhisper-app and left them behind. Assert what the split layout actually guarantees: a plain --uninstall removes the whole payload root and keeps the data dir, and --purge removes the data dir too, which nothing covered before. The second install run stays as an explicit in-place-upgrade check.
PR #51 raised 151 alerts; none is a defect. The tree is already swept to zero ReSharper inspections, and several flagged sites carry a // ReSharper disable with the reason the current form was chosen, so security-and-quality reopens decisions that are already made. Excluded, extending the rationale this file already records: useless-upcast every hit is a cast the compiler needs - (PluginCategory?)null for a switch expression's natural type, (string?)null to pick a Moq Returns overload, (Exception?)null to infer a tuple element as nullable (else CS8619) missed-using-statement finally-block disposal is structural: ownership transfers to a returned ArtifactLock, or a test disposes mid-body to assert idempotent disposal inefficient-containskey the two calls read different keys linq/*, complex-*, restructuring preferences settled by ternary, boolean-expr jb inspectcode The disposal, constant-condition and float-equality rules stay enabled - they can find real bugs - with this branch's instances dismissed one by one. Fixes the two alerts worth acting on: an uncommented empty block and a missing xmldoc summary.
The ProjectReference carried AdditionalProperties="DeployBundledLinuxPlugins= false", which makes MSBuild treat TypeWhisper.Linux as a distinct build from the plain reference TypeWhisper.Linux.Tests and TypeWhisper.PluginSystem.Tests carry. Both configurations share one obj/bin directory, so a solution build ran two instances of the project concurrently and they collided on typewhisper.pdb: "AVLN9999: the process cannot access the file ... because it is being used by another process". Intermittent, so the gate failed on one run and passed on the next. It was the solution's only global-property fork. The project-level DeployBundledLinuxPlugins property went with it: project properties do not flow to referenced projects, so it never skipped anything. The command-line -p:DeployBundledLinuxPlugins=false the packaging script and the plugin workflow already use stays the way to skip the deployment.
42033ef made the deb flow assert that the install transaction pulls in neither libgl1 nor libdbus-1-3, both demoted from Depends to Recommends. 82d774b baked the full runtime closure - those two included - into one prepared Ubuntu image so the three Ubuntu formats stop downloading ~95 MB apiece. Merging the two redesigns left the deb container starting from an image where both libraries are already installed, so the assertion could never hold, and a Depends that had gone missing would have been masked by the pre-seeded closure rather than caught. The deb now runs on the bare base image and resolves its own dependencies there, which is what that format's test exists to prove. Tarball and AppImage have no package resolver and keep using the prepared image, so the download savings stand. The apt retry setting the prepared image supplied is now a helper the deb flow calls too, or its install would race a slow mirror with no retries.
Summary
Related Issue
Test Plan
dotnet testNotes
Summary by CodeRabbit
New Features
Bug Fixes
Tests