fix(desktop): surface global-hotkey registration conflicts instead of swallowing them (swarm slice 1/N)#10193
Merged
Conversation
… swallowing them RegisterEventHotKey returns eventHotKeyExistsErr (-9878) when another app — or a macOS System Settings > Keyboard > Shortcuts entry, even a disabled one — already owns the (keyCode, modifiers) pair globally. GlobalShortcutManager swallowed this with a bare NSLog, and registerAskOmi then logged "Registered Ask Omi shortcut" unconditionally, so the failure was both silent and actively misleading: the Ask Omi shortcut (default Cmd+O, a heavily-owned combo) never fired and we had no signal. This is a property of the user's machine, not our code. Per docs/agents/fallback-telemetry.md this is a hard-terminal failure (no provider/mode switch, nothing to fail open to), so it routes through the incident mechanism, not recordFallback. Extract the OSStatus decision into a pure HotKeyRegistrationOutcome classifier — the controllable seam, since the real Carbon call cannot be made to return -9878 hermetically — and surface conflicts via the existing recordUserVisibleIssue path (area=startup, failure_class=hotkey_conflict, phase=startup). osstatus/keycode/modifiers are preserved through the closed incident-extra allowlist and cloud snapshot keys. The success-path branch logic is unchanged. First evidence-led slice of the PTT/chat silent-failure swarm: converts a silent failure into a reported one (the swarm's primary metric). User-visible conflict surfacing in shortcut settings is intentionally a follow-up. Verification: GlobalShortcutManagerRegistrationTests (5 cases) cover the classifier purity (noErr / -9878 / other) and the telemetry contract (incident snapshot fields + extras survive the allowlist filter). Existing DesktopDiagnosticsManagerTests / ShortcutSettingsTests / PushToTalkShortcutActivationTests (27 cases) stay green. The real conflict condition cannot be staged on our machines — it depends on what else the user has installed — so the controllable-seam test is the prescribed verification per the swarm brief's evidence standard. Machine configuration: a Mac where another app (Raycast/Alfred/Magnet/ BetterTouchTool/…) or a System Settings > Keyboard > Shortcuts entry (even a disabled one) has registered the Ask Omi combination globally. API basis: Carbon/HIToolbox eventHotKeyExistsErr == -9878 (CarbonEvents.h). Expected telemetry: a desktop_health_event user_visible_issue with failure_class=hotkey_conflict at app launch (Sentry-gated by !isNonProduction + 60s dedup), where today there is none. Failure-Class: none No existing failure class fits the silent-OS-boundary archetype, and `new` cannot be bootstrapped pre-merge (a new class definition requires a non-empty evidence_prs list of merged PR numbers). First instance of the swarm's silent-OS-boundary class; registerable as FC-silent-os-boundary-failure once this PR supplies the first evidence number.
Git-on-my-level
added
the
no-changelog-needed
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f4bafec1a
ℹ️ 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
+125
to
+129
| DesktopDiagnosticsManager.shared.recordHotkeyRegistrationFailed( | ||
| osStatus: Int(status), | ||
| keycode: keyCode, | ||
| modifiers: modifiers, | ||
| isConflict: outcome == .alreadyInUse) |
There was a problem hiding this comment.
When a user re-enables Ask Omi by selecting a preset/custom shortcut, the existing settings flow sets askOmiEnabled = true before assigning the new askOmiShortcut, and that first setter posts askOmiShortcutChanged. This new failure branch can therefore capture a hotkey_conflict incident for the stale saved combo even though the immediately following registration for the user's chosen combo succeeds, polluting Sentry/PostHog with false conflicts. Please record the incident only after the final shortcut value is applied or suppress these intermediate re-registration failures.
Useful? React with 👍 / 👎.
…ailure An independent review of the pure classifier landed in the prior commit found a correctness regression at the noErr/no-ref boundary. classifyRegistration(noErr) returned .registered unconditionally, so when RegisterEventHotKey reports noErr but stores a nil EventHotKeyRef (a real Carbon edge), registerHotKey's else branch recorded a registration failure while registerHotKey returned .registered — and registerAskOmi then logged "Registered Ask Omi shortcut" for a shortcut that will never fire. The failure was both recorded and claimed as success in the same call. The pre-refactor code treated status==noErr with a nil ref as failure; this restores that semantics. Narrow fix: registration is successful ONLY when the Carbon status is noErr AND a non-nil EventHotKeyRef is stored. classifyRegistration now takes ref-presence as a second pure input (refPresent: Bool), keeping it the controllable test seam — the real Carbon call cannot be made to return noErr with a nil ref hermetically. The conflict flavor (eventHotKeyExistsErr) is preserved regardless of ref so the isConflict telemetry discrimination is unchanged. Behavioral regression coverage for the exact boundary is added (testClassifyNoErrWithNilRefIsOtherFailure) and the existing pure-classifier cases updated for the new refPresent parameter. Verification: GlobalShortcutManagerRegistrationTests (6 cases) pass, including the new noErr/nil-ref boundary case. make preflight (18/18 diff-scoped checks) green; SwiftLint 0 violations; swift-format clean. Failure-Class: none No existing failure class fits the silent-OS-boundary archetype (same as the prior commit): the available classes concern deploy/Cloud Run/dict-merge/module boundaries, none a Carbon noErr/nil-ref edge. `new` cannot bootstrap pre-merge.
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
1 participant
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.
What
Converts a silent OS-boundary failure into a reported one.
GlobalShortcutManager.registerHotKeyswallowed theRegisterEventHotKeyOSStatuswith a bareNSLog, andregisterAskOmithen logged"Registered Ask Omi shortcut"unconditionally — so when Carbon rejected the combo the failure was both silent and actively misleading (the log claimed success). This is the first, evidence-led slice of the PTT/chat silent-failure swarm (Lane A): it makes the swarm's primary metric — silent failures converted to reported ones — move by one.Swarm context — this is slice 1 of N
Six independent read-only scouts investigated the swarm axes (synthesized findings in the commit/PR comments). This PR ships exactly one highest-confidence candidate. The rest are deliberately deferred:
NSLogOS-boundary failure site in the PTT/shortcut/audio paths.NSEvent.flagsChanged, which SEI does not suppress (only.keyDown/.keyUpare). All four PTT presets are modifier-only, so the dominant path is unaffected. Filing the minority custom-key carve-out + the unrelated SEI barge-in sidebar as issues.retryOrGiveUpsilent give-uponSilentMicDetected, no enum extension), but broader (3 files, touches the live PTT capture path). Better as its own slice.AgentBridgesession-token refresh (chat_bridge)recordFallbackcase (credential switch), single call site, but has no hermetic unit-test seam today; verification needs a newtier: faultflow (clonechat-fault-5xx.yamlasstatus:401). The fault-injector scout confirmed fault flows belong after a Lane A fix.sampleRate = 16000resampledFrameCapacity) is already guarded + tested (AudioCaptureResampleCapacityTests.swift). #9649/#10105 were prior instances of that boundary. No third patch warranted.omi-fault-inject.shis HTTP-only by design; non-HTTP faults belong as bridge actions driving the app's own seams. Lands after a Lane A fix for the flow to prove.No
ChatProvider.swiftorFloatingControlBarWindow.swiftchanges. No notch typed chat. No speculative retries / timing tweaks / blanket fallbacks.The change
GlobalShortcutManager: extract a pureHotKeyRegistrationOutcomeclassifier over the CarbonOSStatus(.registered/.alreadyInUse/.otherFailure).registerHotKeyroutes the failure branch to the incident path; the misleading success log is gated on.registered. Success-path branch logic is unchanged.DesktopDiagnosticsManager: newrecordHotkeyRegistrationFailed(osStatus:keycode:modifiers:isConflict:)wrapper around the existing privaterecordUserVisibleIssue. Three closed-set additions:hotkey_conflicttosafeIncidentLabel;osstatus/keycode/modifierstoallowedIncidentExtraKeysandcloudIncidentSnapshotKeys(so the Carbon detail reaches both the snapshot and the Sentry attachment). These are labels/keys on an existing event, not new PostHog events or one-off counters.Telemetry-contract choice: incident, not fallback
Per
docs/agents/fallback-telemetry.md,recordFallbackis for branches that switch provider/mode/correctness or fail open. A hotkey that won't register is a hard-terminal failure (no continue, nothing to switch to) → the incident mechanism (recordUserVisibleIssue→ PostHoguser_visible_issue+ Sentry.warning), exactly likerecordPTTSilentTurn.recordFallbackis deliberately not used.Evidence standard
keyCode 31,ShortcutSettings.defaultAskOmiShortcut), the universal File → Open combo and therefore unusually collision-prone.eventHotKeyExistsErr == -9878(CarbonEvents.h). Hammerspoon issue Transcription dies every few seconds, device reconnects #1261 reproduces this exactly (RegisterEventHotKey failed: -9878when the combo is owned by System Settings — including unticked entries — or another app). Pure machine axis, not our code.GlobalShortcutManagerRegistrationTests(5 cases) — classifier purity (noErr→.registered;-9878→.alreadyInUse;-50/-9870→.otherFailure) and the telemetry contract (the incident snapshot carriesarea=startup,failure_class=hotkey_conflict,phase=startup, andosstatus/keycode/modifierssurvive the closed allowlist filter). ExistingDesktopDiagnosticsManagerTests/ShortcutSettingsTests/PushToTalkShortcutActivationTests(27 cases) stay green. The real conflict cannot be staged on our machines (it depends on what else the user has installed), so the controllable-seam test is the prescribed verification per the swarm brief's evidence standard. A full named-bundle launch is not additionally informative for this slice: the success path is provably unchanged (if outcome == .registered⟺ oldif status == noErr), so the unit test already proves a normal registration does not trip a false-positivehotkey_conflictincident.desktop_health_eventuser_visible_issuewithfailure_class=hotkey_conflict(Sentry-captured on prod/beta, 60 s de-duped perarea:failureClass), where today there is none. On unaffected machines: no new incident; the previously-unconditional"Registered Ask Omi shortcut"log now only prints on real success.Product invariants affected
GlobalShortcutManager.swiftfalls under thedesktop/macos/Desktop/Sources/FloatingControlBar/**glob. This change adds registration-failure telemetry only; it does not touch the shared transcript, kernel projection, floating viewport, journal write-path, or introduce any second store — so the chat-continuity contract is unaffected and no guard-test update is needed.Definition of Done
xcrun swift buildvia the test target's@testable import Omi_Computer)check_desktop_test_quality.pycleanscripts/pr-preflight --suggest→ INV-CHAT-1 path-match named below (contract unaffected)Failure-Classdeclared below and validated withscripts/failure-classTODO/FIXME/HACK; no new CI gate/ratchet/guideFailure-Class: none
No existing failure class fits the silent-OS-boundary archetype (the registry covers backend/release/runtime contracts, none for hotkey or OS-boundary observability), and
newcannot be bootstrapped pre-merge: a new class definition requires a non-emptyevidence_prslist of merged PR numbers. This is the first instance of the swarm's silent-OS-boundary class; a follow-up can registerFC-silent-os-boundary-failureonce this PR lands and supplies the first evidence number.