Audit tier1 - #51
Open
csmashe wants to merge 342 commits into
Open
Conversation
|
Important Review skippedToo many files! This PR contains 574 files, which is 424 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 (574)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ms (audit §4 M15)
…ownership tokens (audit §5 H2)
…de changes (audit §4 M6)
…imeouts (audit §4 M16)
…ting recording ownership (audit §5 M3)
…ed default-voice fallback (audit §5 M7)
… or ambiguous (audit §5 M4)
FindSystemLauncher only scanned /usr/share/applications and the system-wide Flatpak export dir, so a browser installed with `flatpak install --user` — what GNOME Software defaults to — was never found. Setup then patched nothing, and because HasInstalledLauncher also missed it, IsFullyConfigured succeeded vacuously: the UI reported success while AT-SPI URL detection kept failing. Zen on this box was exactly that case. Discovery now walks XDG_DATA_DIRS in its declared order, led by the per-user Flatpak export dir, since that copy is the one the application menu launches. Spec defaults apply only when the variable is unset; a root the session omitted is one whose launchers the desktop does not read, so it is not reintroduced. Flatpak's own export roots are the exception — they are placed in the variable by hooks that miss some session types. Also resolve XDG_DATA_HOME for the shadow-launcher and backup dirs: a shadow written under a data home the session ignores never reaches the menu. A relative value is treated as unset in both places, including the sibling KdeShortcutWriter this resolution was modelled on. The Chromium name list gained the Flatpak app IDs, mirroring the Firefox list, since a Flatpak-only install ships no native launcher.
Brings back the two review fixes made on review/tier1-01. Three conflicts against later slices, resolved toward the newer audit-tier1 APIs: - DictationOrchestrator: kept the fix's per-step try/catch isolation but called RollBackStartedRecording(captureSession) — later slices made the rollback capture-session scoped. Dropped the fix's `_audio.LiveFrameSink = null`: that property is gone, and the session-scoped sink is already cleared by StopRecording during the rollback. - SettingsBackupService: took the fix's ReadBounded helper over the older inline bounded-read loop; same cap, less duplication. - TargetAppCorrectionLearningService: took the fix's explanatory comment. Also repaired a silent auto-merge break: a presenter test call site kept the two-argument constructor after IErrorLogService became required. 1683 Linux tests pass.
Addresses the QA findings on this branch and the issues surfaced by the follow-up adversarial/standard review loop. Data safety: - LocalModelStorageService: never delete a migrated source based on resemblance. Track the targets this run actually wrote, stamped with size and mtime, and delete only against an unchanged stamp. Previously any pre-existing file at the target counted as "already migrated" and the source was removed. - ProfileService: refuse to save while profiles.json failed to load, and retry the load in EnsureCacheLoaded so a transient failure or a repaired file recovers without a restart. - PluginHostServices: do not cache an empty store after an unreadable file, and clear the load-failed flag on every recovery path. - FileMemoryPlugin: write memories owner-only on every write, including the first, and commit with a single atomic overwriting rename. - AtomicFileWrite: fsync the temp file before it becomes the destination, and close the check-then-move race on the replace path. SettingsService: - Load under the write lock; add Reload so restore-from-backup is atomic. - Publish SettingsChanged outside the lock but in commit order, via a single elected drainer, with per-subscriber exception isolation. ProcessRunner: - Caller cancellation now kills the process tree and propagates instead of being reported as NotStarted; disposal of a blocked stdin writer can no longer displace the cancellation or timeout result. UI and misc: - Surface save failures in the Snippets, Prompts, and Profiles sections (en/de/es/ru), and show watch-folder warnings on succeeded runs. - Marshal HttpApiService/settings notifications to the UI thread, guarded against persisting a stale snapshot. - Cache correction regexes per pattern with CultureInvariant matching. - Order-gate and orchestrator: wait for the insertion turn before the status/focus handoff, and never queue an unreserved session. Tests: 2131 passing. ReSharper: 0 issues at HINT severity.
QA review fixes: - PluginManager.Dispose: one shutdown budget for the whole plugin pass instead of a full timeout per plugin, which multiplied by plugin count. - WatchFolderService: observe the rescan worker so an unexpected fault marks the run failed instead of dying silently while it reports healthy. - WatchFolderService: carry the committed export path into the failure history record, so a post-CommitExport failure still tells the user where the transcript landed (critical when DeleteSource removed the original). - Advanced/General section view models: post SettingsChanged through the UI thread; the HTTP API and model manager save off-thread and Refresh mutates ObservableCollections. - PluginsSectionViewModel: give the plugin boundary a real cancellation token instead of CancellationToken.None, disposed once the boundary completes. - SystemCommandAvailabilityService: kill the nvidia-smi child on every early exit; disposing the Process never stopped it. - HistorySection: replace the hard-coded MM/dd/yyyy timestamp with a culture-driven short date plus 24-hour time. ReSharper inspections: 135 -> 0 at HINT severity (fresh cache). Fixed 83 (target-typed new() style headers, static helpers, IsType/exactMatch, trailing commas, const locals, Contains, merged try, MemberData discovery enumeration); suppressed 52 with reason comments where the suggestion was subjective or would not compile. Build clean, 2826 tests pass.
Addresses review findings on GnomeShortcutWriter, verified against the code and hardened over an adversarial plus three standard Codex passes. - RemoveAsync now drops every occurrence of the managed path. List.Remove took only the first, so an externally duplicated entry survived removal and left the shortcut registered. - A failed or cancelled install no longer leaves a half-configured entry in GNOME's Settings UI: WriteAsync unpublishes the path it just added when a name/command/binding write doesn't land. It skips the cleanup for a path that was already listed, and for one another writer has since configured in full, so rollback can't delete a working shortcut. - Cleanup failures are reported rather than swallowed. MutateListAsync signals normal failures through its return value, so the outcome is now inspected: the result carries a warning naming the leftover entry, and any backup the cleanup wrote is merged into FilesChanged. - Snapshots reserve their filename with FileMode.CreateNew and retry on a colliding timestamp, so a backup is never overwritten. Only the create step retries; a write that fails afterwards deletes its partial file instead of leaving something that reads as a valid snapshot. - Install and remove are serialized per instance. The writer is a DI singleton and the Shortcuts panel exposes the two as separate commands, so they could otherwise interleave over the same path. Cross-process edits remain handled by the existing read/confirm/retry loop, which is as close to atomic as gsettings allows. Solution-wide ReSharper inspections are at zero against a fresh cache.
BrowserAccessibilitySetupHelper and KdeShortcutWriter each resolved XDG_DATA_HOME inline, including the rule that a relative value is invalid and must be ignored. Duplicating that rule meant a third consumer could easily miss it, and it left a comment in one file pointing at the other for the reasoning. The shared resolver uses GetFolderPath's DoNotVerify option: the default returns an empty string when HOME names a directory that is not on disk, which made the fallback itself relative — the very thing the rooted check exists to prevent. An unset HOME was never affected, since that path falls back to the passwd entry. Both env-mutating test classes now touch TypeWhisperEnvironment.BasePath before redirecting HOME. BasePath freezes on first access and derives from the same variables, so whichever test reached it first would otherwise pin the process-wide production root to a temp directory, silently weakening the TestPaths.EnsureIsolated guard the rest of the suite relies on.
Callers invoke AddEntry from inside their own catch blocks, so a throwing subscriber would escape and take down the operation that was trying to report a failure. Raise the event per subscriber with isolated handling, and keep the diagnostic itself best-effort: interpolating the exception calls a virtual ToString, and Trace listeners can fail. This also stops one bad subscriber from starving the ones registered after it. Adds ErrorLogServiceTests covering a throwing subscriber, an exception whose ToString and Message both throw, and the same guarantee on ClearAll.
Both view models subscribed to SettingsChanged with a bare += Refresh, so they wrote bound properties on whichever thread called Save. The dictation path and the model-storage migration both save from background threads. Route the refresh through an injected post delegate that defaults to the dispatcher, running inline when already on the UI thread so a save from the UI still refreshes synchronously. The delegate is injected rather than calling Dispatcher.UIThread directly because that dispatcher binds to whichever thread touches it first and nothing pumps it under the test runner, which made the existing Appearance tests fail under the parallel runner while passing in isolation. Follows the same seam LearnedCorrectionsNotificationService already uses.
Once the initiation response yields a valid id the transcription job exists server-side, but result_url extraction and the HTTPS check threw before returning an InitiatedJob — and TranscribeAsync only enters the try/finally that calls DeleteJobBestEffortAsync once it has one. A missing, malformed, or non-HTTPS result_url therefore orphaned the job on Gladia's side. Wrap that stretch so a failure deletes the job before rethrowing, which leaves the original protocol exception intact. The two existing theories were asserting the bug: their handlers threw "Unexpected request: DELETE" on any cleanup call. They now count DELETEs instead — one for a valid id with a bad result_url, none for the no-id case, where there is nothing to clean up. Reverting the plugin change fails exactly those four cases. Build clean, ReSharper 0 at HINT (fresh cache), 2826 tests pass.
Making the Advanced and Appearance refreshes UI-thread-deferred introduced a regression: every property Refresh writes has a save-hook guarded only by equality against _settings.Current, and that guard is sound only while the refresh runs during the save. Once queued, Current has already advanced, so the older snapshot looks like a genuine user edit and overwrites the newer commit. Read Current when the post runs instead of capturing the event payload, so queued refreshes coalesce onto the newest commit, and add a hydration flag — saved and restored rather than cleared, so a nested refresh cannot un-guard the outer one — that every save-hook checks: 4 in Appearance, 8 in Advanced. Route the plugin and provider callbacks through the same post delegate too. They mutate bound properties from background threads and touch the properties the flag guards, so they have to be serialized with Refresh rather than race it. Note this deliberately stops hydration from writing normalized values back. That write-back also persisted MemoryEnabled=false whenever a plugin was merely unavailable, discarding the user's preference; the remaining cases are normalized at the point of use instead.
Reject the redaction placeholder when no stored value backs it. GetItems renders every stored header value as "<stored securely>", so duplicating a webhook row or renaming a header carried that text back to a name with nothing behind it and stored it verbatim - the webhook then sent the placeholder as its real header value and the settings UI redisplayed it looking exactly like a kept secret. This reverses the "a placeholder on a new header is a literal value" clause of 2d17b7f; the test that pinned it now asserts rejection. Move the CLI installation off the UI thread. It copies the ~17 MB binary and runs it once to verify on a 10-second deadline, all of which froze the window. Commit the launcher by rename with its mode verified first, so an interrupted install cannot leave a truncated script - and a chmod that fails cannot replace a working launcher with a non-executable one - and handle the TimeoutException that CLI verification can raise, which was previously unhandled. Stop the transcribe spool cleanup from masking results: File.Delete in the finally could throw over a successful transcription, and in the catch it could replace the spool failure. It is now best-effort but reports the path on stderr, since the file holds the whole recording. Fail closed explicitly when the Unix-socket peer credential read throws rather than unwinding into Kestrel, dedupe FormatBudget into ConsoleOutput, and correct the PluginSelectionExtensions suppression comment, which claimed a boxed struct enumerator that does not exist - foreach over a string compiles to indexer access. Tests: drop a shared temp-directory file count that raced other processes, handle teardown faults in the stub DisposeAsync paths so they cannot mask an assertion failure or leak the temp directory, and move Dispose(api) into finally to match QuickCommandTimeoutTests.
ProvidersChanged was the one subscription set up before Refresh applied the persisted settings. RefreshSpokenFeedbackProviders falls back to the default provider when the selected one is absent, and that write is not under the hydration guard, so a notification arriving in that window would reconcile against an un-hydrated selection and persist the default over the user's saved provider. Moving it below hydration also lines it up with the SettingsChanged and PluginStateChanged subscriptions, which already ran after Refresh.
14 conflicting files. Slice 2 was cut 235 commits behind the tip, so most conflicts were the same problem solved twice; resolved toward whichever design the rest of audit-tier1 already assumes, keeping slice 2's fixes wherever they add behavior rather than duplicate it. Kept audit-tier1's design (slice 2 re-solved it independently): - ProfilesSectionViewModel: UiOperationGuard over slice 2's TryMutate — same logging, rollback and status, but catches only expected failure kinds instead of bare Exception. ProfilesSection.axaml reverted with it, since its error banner bound HasError/ErrorText from the dropped design. - ProcessRunner: audit §6 M1 covers the same cancellation hardening with five tests; the two rewrites had incompatible block structure. - AboutSectionViewModel: staged restore supersedes RestoreBackup + reload. - AdvancedSectionViewModel: _isProgrammaticRefresh over _hydratingFromSettings. Kept slice 2's work (genuinely new): - LocalModelStorageService: the MigratedTargets provenance ledger, which the auto-merged body already threads through every helper. This reverses audit-tier1's contract — migration now refuses to overwrite an unrelated target and never deletes a source it cannot prove it wrote, so the two tests asserting replace-and-delete were removed in favour of slice 2's three. Kept audit-tier1's TryCleanUp wrapper on top; dropped the now unreferenced FilesHaveIdenticalContent. - SettingsService.Reload(), which the merged ISettingsService declares. - FileMemoryPlugin owner-only permissions on every write. - GeneralSectionViewModel UI-thread marshalling, plus audit-tier1's LanguageChanged handler that slice 2's cut predated. - App.axaml.cs: seed-failure guard spliced into the newer reconcile design. - ProfileService: union ctor carrying both the atomic-write seam and errorLog. - New tests for unreadable profile files and settings publication ordering. Full solution green: 514 Core, 1700 Linux, 897 PluginSystem, 108 CLI.
Fixes the settings-UI row collision where the "normalize numbers" toggle sat on the same two grid rows as the realtime-streaming live preview, and stops reporting a Translate task downstream when the engine does not support translation (dictation, file transcription and the HTTP API all forced number normalization to treat source-language text as English). Installer: the unsafe-INSTALL_ROOT check now guards the reinstall cleanup loop as well as uninstall, and the KEEP list is defined once instead of twice, since two copies could drift and silently delete user data. The tarball smoke test now plants a fixture on a KEEP path and asserts it survives both cleanup loops. Number parsers: a bare singular "millón"/"Million" is a noun, not a number, and "menos" no longer licenses the articles "un"/"una" — it also means "except/less", so "todos menos un estudiante" was becoming "todos -1 estudiante". Regression tests cover both. Also drains-by-not-redirecting pactl stderr (a full pipe buffer would block the child and stall the device-change stream), clears the stale native-audio failure reason after a later successful init, and matches the test time provider's TimestampFrequency to the ticks it hands out. Closes the whole-solution ReSharper pass at 0 issues (HINT severity, fresh cache): trailing commas per .editorconfig, pattern merges, a dead public NormalizeSegments overload removed, and reasoned suppressions where the suggested rewrite would read worse.
Five conflicting files, resolved per hunk rather than per file — this slice contained both genuine fixes and re-solutions of problems audit-tier1 had already addressed. Kept slice 3's work: - ShortcutDispatcher: its rekeying of _pendingSelectionWorkflows from KeyCode to SelectionWorkflowId. The KeyCode map is the older design and cannot hold two selection workflows that share a trigger key under different modifiers; slice 3 ships three tests covering that and the rebound-key-after-ClearShortcuts guards. Kept audit-tier1's ReSharper annotations on the loops. - GnomeShortcutWriter: the recoverable-mutation try wrapper. - UpdateCheckService: the guard so a failed preferences write costs only rate-limit bookkeeping, not the check result. - HttpApiService: _requestDispatcher.Dispose(), which audit-tier1 never called. Dropped the accompanying _cts/_listenTask drain — the Kestrel migration removed both fields. Kept audit-tier1's: - App.axaml.cs reconcile revision guard; dropped the now-dead snapshot locals, since the merged body reads both services under reconcileLock. Full solution green: 514 Core, 1715 Linux, 897 PluginSystem, 108 CLI.
Four conflicting files. Kept slice 4's work: - AtomicFileWrite: its PublishReplace/PublishCreateNew helpers, which keep the destination-mode copy and additionally survive a concurrent writer creating the destination between the check and the link. Kept audit-tier1's FlushToDisk call, which slice 4's arm dropped — without it the durability flush is silently lost. - GemmaLocalPlugin: the SelectedModelId selection lock, alongside audit-tier1's model-routing guard (unrelated fields, both kept). Kept audit-tier1's: - ObsidianPlugin: same trailing-separator fix on both sides; audit-tier1's falls back to the vault key rather than showing a full path in the picker. - RecentTranscriptionsService.StatusTextFor: localized catalog lookups over slice 4's snapshot read and hardcoded English failure string. Corrected both sides in IsError: slice 4 inverted it so an unrecognized result fails closed (right call), but that swept ActionHandled in with the failures even though a plugin action consuming the text is a success. audit-tier1's allowlist was wrong the other way, omitting ActionFailed and ActionUnavailable. Kept the fail-closed shape with ActionHandled as success. Full solution green: 514 Core, 1715 Linux, 899 PluginSystem, 108 CLI.
Slice 5's only commit re-applied the 714 trailing commas that ec14e81 already carries into audit-tier1 (via slices 6a/6b), so all 17 conflicts were the same formatting landing twice over code audit-tier1 had since moved on from. Verified slice 5 introduces no symbol its cut point lacked, then took audit-tier1 throughout. Notable: this drops s_defaultEnabledPluginIds, which slice 5 still carried. That is correct — 4b7ff31 removed it upstream when plugin manifests became authoritative for locality and category. Full solution green: 514 Core, 1715 Linux, 899 PluginSystem, 108 CLI.
Brings in XdgPaths centralization and per-user Flatpak browser launcher discovery. Eight of the nine conflicts were the trailing-comma sweep again (files untouched by either real commit) and took audit-tier1's side. BrowserAccessibilitySetupHelper resolved by hand: - Launcher names: union, keeping slice 6's Flatpak application IDs (com.vivaldi.Vivaldi, com.opera.Opera) alongside the existing entries. - s_flatpakExportRoots: took slice 6's single "/var/lib/flatpak/exports/share". The merged consumer does Path.Join(root, "applications"), so audit-tier1's entries would have resolved to .../applications/applications and matched nothing. Its /usr/share entry is also deliberately absent per the comment there — system roots the session omitted are not force-appended. Full solution green: 514 Core, 1735 Linux, 899 PluginSystem, 108 CLI.
Second half of the mechanical trailing-comma split. No conflicts and no content change — ec14e81 already carries these commas into audit-tier1, and slice 7 added no commits of its own beyond the split's rejoin merge.
33 conflicts, nearly all from slice 9's ReSharper sweep colliding with the one audit-tier1 already ran — the two runs wrote different justification wordings for the same disable pragmas. Took audit-tier1's wordings, which are the more specific of the two. Real resolutions: - AdvancedSectionViewModel: adopted slice 9's UI-thread marshalling for SettingsChanged (Refresh mutates the provider/voice ObservableCollections and the event can fire off the HTTP API or model-manager threads), kept on top of the subscribe-after-hydration ordering resolved in slice 2. - GeneralSectionViewModel: kept audit-tier1's arm, which re-reads Current inside the post so queued refreshes coalesce onto the newest commit, rather than posting the captured payload. - WatchFolderServiceTests: slice 9 moved a local function above its uses to satisfy MoveLocalFunctionAfterJumpStatement; the merge produced both copies, so kept audit-tier1's single declaration (legal C# either way). - UiOperationGuardTests: kept slice 9's added explanation comment. The Gladia result_url job-deletion fix merged without conflict and is present (DeleteJobBestEffortAsync). Full solution green: 514 Core, 1735 Linux, 899 PluginSystem, 108 CLI.
…atches process.Start() raises Win32Exception when exec itself fails - the install directory mounted noexec, or a copy that is not a valid binary - which happens after the chmod-and-verify step has already passed. That type is outside the catch filter InstallCliAsync uses, so it escaped the command and took the app down instead of reaching CliStatusText. Normalize it to InvalidOperationException, keeping the original as inner, so it lands on the same path as every other verification failure. Narrow the stub teardown catches from bare SocketException to the shutdown error codes. Disposal cancels before awaiting, so a cancellation-state guard would always pass and suppress every socket error, letting a genuine listener or protocol fault masquerade as a shutdown reset and the test go green. The predicate lives in one place rather than in each stub. UnixSocketApiClientTests was the third stub still missing the teardown handling the other two received: its ServeAsync has no internal try/catch, and _cts.Dispose() plus the temp-directory delete sat after the try, so an unexpected fault both masked the assertion and leaked the directory. Both now run from a finally.
QA review, verified against current code before acting: - Gladia leaked the server-side job whenever polling failed, timed out or was cancelled, leaving the user's uploaded audio on the provider. Moved the try/finally to cover polling. Two tests asserted the old contract and were updated; note this means a cancelled dictation now awaits a bounded DELETE. - LLM call provenance looked the owning plugin up by selection ID. For an OpenAI-Compatible profile role that is the profile's ID, which matches no manifest, so RanLocally was always recorded false. Use PluginId. - Sherpa's Canary parse called GetString() on "text"/"lang" without checking the kind, so a numeric or boolean value threw InvalidOperationException past the JsonException handler. - The OpenAI realtime receive loop swallowed all cancellation, stranding finalize's waiters when it came from an aborted socket or a subscriber. - The ChatGPT SSE reader failed a response.completed event with a missing status; the sibling xAI client already rejects only an explicit mismatch. - OpenAI-Compatible enumerated _additionalProfiles across awaits while a settings save replaced it wholesale. Snapshot under the existing lock, and re-resolve a fetched catalog by ID so it is not applied to an orphaned profile or one whose endpoint or key has since changed. Skipped, with reasons in the review notes: the PluginSelectionExtensions legacy overloads, the shared SDK JSON helper, and MapLegacyCategory aliases for forms no manifest ever used. Test fixes: the streaming fault-propagation handshake only proved a frame was dequeued, not that the session had captured the fault, so SendAudioAsync could legitimately succeed; FakeWebSocket state is now lock-guarded. Gladia's malformed-status theory gets a bounded poll window. ReSharper inspections: 132 at HINT severity to 0, verified against a fresh --caches-home. Fixed 13 (CA1068 parameter order, CA2249, two static members, two unresolvable crefs, constants, trailing commas, xUnit2032). Suppressed 118 with per-site reasons, matching the convention already in tree.
pactl exits whenever the sound server does, and AudioRecordingService latches _watcherStarted, so a PipeWire/PulseAudio restart silently ended live default-device following for the rest of the session — the read loop already cleared its state for a restart nobody ever triggered. The watcher now reconnects itself. A run that survived reconnects at once; one that died young backs off (1s doubling to 16s) because the first attempt after a restart routinely lands before the server is listening again, bounded by MaxAutoRestarts so a permanently broken server cannot respawn pactl forever. One task owns the backoff sequence, so a failed launch — which starts no read loop — cannot silently end recovery. Each reconnect signals the dispatcher once: pactl does not replay events, so a default that changed while the subscription was down would otherwise go unnoticed. Lifecycle fixes this needed: the self-teardown path now disposes the CancellationTokenSource (only Stop() did, so every EOF leaked one) and kills the subscription before disposing it (Process.Dispose does not terminate the child, so a read error with pactl still alive orphaned it). Reconnects publish their replacement without releasing the lock, and Stop() clears the intent first and bumps a session counter, so neither a reconnect racing Stop nor a retry worker sleeping across a Stop/Start cycle can resurrect a stopped watcher or start a second retry chain. Start() no longer overwrites the callback while a run is live, keeping its documented idempotence. Four tests cover recovery, the bounded give-up, self-reconnect and the Stop-versus-reconnect race.
…ests A launch failure during Start() logged and stayed down forever, while the same failure at runtime retried on the backoff schedule. Extracted that schedule into ScheduleRetryLocked and called it from both, so the startup path can no longer leave the watcher silently dead. Start() bumps the session first, which retires any sleeping worker, so it cannot produce a duplicate one. PactlWatcher_Restarts_AfterReadLoopExitsOnEof predates the auto-reconnect and had started relying on it not firing: its instant-EOF fakes now schedule a one-second retry that could bump the count under the assertions. Both reconnect knobs are pinned out of reach so it tests the explicit restart again, and the cross-thread counter is interlocked on both sides. PactlWatcher_Reconnects_WhenASurvivingSubscriptionEnds claimed the factory throw ended the chain; SpawnLocked catches it and falls into the retry budget, so the count kept climbing and the assertion had been weakened to match. The third subscription now stays open, ending the chain at exactly three.
QA findings, each checked against current code first: - Gladia created the job before validating result_url, so a missing, non-string or non-HTTPS value leaked the uploaded audio server-side. Delete the job once its id is known and any later initiation step fails. - SelectProfileModel mutated the profile FindAdditional returned. That is a snapshot, so a concurrent save could orphan it and drop the user's model choice on the next persist. Re-resolve by id under the lock. - _additionalApiKeys was read by the transcription/LLM paths while the settings-save and activation paths rewrote it. A Dictionary read racing a resize can loop or return the wrong entry, so it now shares the profile lock. - Failed webhook activation left Service pointing at a disposed instance when the event-bus subscribe threw. Codex review on top of those: - SetItemsAsync applied each secret to the live cache as it was written but only swapped the profiles in afterwards, so a later secret failure left a new credential paired with the old BaseUrl. Stage the keys and publish both together. - Publishing and reading them as two lock acquisitions had the same window, so profiles and credentials are now written and resolved in one critical section. Tightened the watch-folder worker-failure assertion to an exact ArgumentException match, verified against the actual exception. Skipped: rejecting an orphaned "<stored securely>" header placeholder. SetItems_NewHeaderPlaceholderStoresLiteralValue pins the opposite contract by name, so flipping it is a decision, not a fix. Worth revisiting - duplicating a webhook row can persist the placeholder as a real header value. ReSharper: 0 at HINT severity, verified against a fresh cache. The refactor orphaned five now-unreachable helpers; deleted them.
35 conflicts, again dominated by two independent ReSharper sweeps writing different justification wordings for the same pragmas. Took audit-tier1's wordings and kept slice 10's substantive changes: - SherpaDecodeCoordinator: the ValueKind guard on "lang" (GetString() throws on a non-string element, so a canary payload with a numeric or boolean lang faulted the whole decode), and the <= tie-break in the quiet-cut search, which keeps the latest candidate so a digitally silent window yields the longest chunk rather than the shortest. - StreamingProviderFailurePropagationTests: lock-guarded CloseStatus/ CloseStatusDescription/State on the fake socket. Kept audit-tier1's PluginLoader form. Slice 10 restructured that method — nesting validation under `categories is not null` with legacy inference as the fallthrough — which is equivalent, but the merge spliced its inverted condition onto audit-tier1's body. That would have returned legacy metadata for plugins that declare categories and then dereferenced a null. Full solution green: 514 Core, 1735 Linux, 903 PluginSystem, 108 CLI.
Clean merge, no conflicts. Brings in the audit §10 QA/Codex review findings and the CLI verification launch-failure surfacing with narrowed socket teardown catches.
Final slice, clean merge. Brings in the tier1-11 QA review findings plus the default-device watcher reconnecting when its subscription ends and a retry for a failed initial pactl launch.
Each of the three Ubuntu smoke containers installed the same ~95 MB of GUI and audio dependencies, and that download ran inside the 10-minute per-format timeout. When archive.ubuntu.com dropped to ~140 kB/s the apt work alone outlasted the budget, so the run was killed before a single assertion executed - twice, at a different container each time. Bake the dependencies into one prepared image per distribution up front, outside the timeout, so the per-format containers touch no package manager and the budget covers only the install and execute checks. Add apt and dnf retries so a dropped connection retries instead of failing the run, and raise the workflow step budget to cover the remaining one-time download. The release workflow gated on the same script and would have failed a real release identically. Also stage the rpm payload through a file instead of a pipe: cpio stops at the archive trailer and closes the pipe while rpm2cpio is still writing padding, so pipefail failed the run despite a complete extraction.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related Issue
Test Plan
dotnet testNotes
Summary by CodeRabbit
New Features
Bug Fixes
Tests