fix: report keychain, toast and IPC failures honestly; pin ruff to 0.16.x by abrichr · Pull Request #29 · OpenAdaptAI/openadapt-tray

abrichr · 2026-07-28T02:20:30Z

Why now

ruff>=0.1.0 was unbounded, [tool.ruff.lint] declared no select, and CI runs a blocking uv run ruff check src tests scripts. ruff 0.16.0 grew its DEFAULT rule set from 59 rules to 413, so this repo silently inherits 354 new rules the moment a runner resolves a new ruff. The only thing keeping main green was the 0.14.13 pin in uv.lock — and .github/dependabot.yml schedules weekly pip updates. The next bump would have turned main red with 216 findings and no code change. Same failure mode already hit openadapt-consilium (0 → 54) and OmniMCP (0 → 375) today.

Rule-set decision: keep the FULL ruff 0.16 default set

No select. After safe autofix the residual was 91 findings in a 3.9k-line tree — small enough to read and judge one by one, so the real coverage is worth more than trading it away for a narrow ["E","F","I","W"]. The reasoning is written inline in pyproject.toml, and the ruff version is now bounded (ruff>=0.16,<0.17) so the default set can only change when a person raises the ceiling. uv.lock resolves 0.16.0, so CI genuinely exercises the new set.

One rule disabled, documented inline: BLE001. The tray is almost nothing but boundaries to optional, platform-owned subsystems — pystray, pynput, Pillow, keyring, desktop-notifier/Cocoa, tkinter, zenity/kdialog, osascript, PowerShell, winreg, ctypes.windll. They share no exception hierarchy, and every handler logs and degrades on purpose because a menu-bar app that dies when a notification daemon is missing is a worse product. Enumerating vendor exception types would reintroduce crash-on-first-surprise the next time a dependency added one — a behaviour change, not a style change.

Real defects this exposed

Where a blind catch was hiding a defect rather than guarding a boundary, the code was fixed, not the rule silenced.

File	What was wrong	What a user saw
`src/openadapt_tray/keychain.py`	`clear_ingest_token` documented "removed or already absent" and its own inline comment said absent = no-op success — but returned `False`. `keyring` raises `PasswordDeleteError` when there is nothing to delete, and the blanket `except Exception` caught it.	Signing out on a machine that had never signed in reported a failed sign-out. A genuine backend error was also silent.
`src/openadapt_tray/notifications.py`	`_show_windows` returned `True` unconditionally after shelling out to PowerShell, ignoring exit status (`PLW1510`). macOS and Linux paths have always checked `returncode`.	On any Windows box where the WinRT toast API is unavailable, every notification — including "N automations need attention" — was reported as delivered.
`src/openadapt_tray/ipc.py`	`IPCServer._accept_loop` caught bare `Exception` and `break`.	The IPC server could stop accepting connections with no message anywhere. Now narrowed to `OSError` (all `accept()` can raise) and it logs unless `stop()` closed the socket.
`src/openadapt_tray/icons.py`	A present-but-corrupt icon PNG was swallowed by `try/except/pass` (`S110`).	A broken asset was indistinguishable from a missing one. Now logged.
`scripts/generate_icons.py`	A cairosvg import/render failure was swallowed.	A broken cairosvg install printed "No SVG rasterizer found". Now logged.
`src/openadapt_tray/notifications.py`	A pystray balloon failure fell silently through to the toast path.	Now says why it fell back.
`src/openadapt_tray/platform/windows.py`	`_find_executable` wrapped `os.path.exists` in `try/except/pass`. `os.path.exists` swallows its own `OSError`s and returns `False`, so the handler was unreachable and only hid a failed `import os`.	Dead code removed; import hoisted.

Regression tests added for the two user-visible fixes (tests/test_keychain.py::TestClearIngestToken, new tests/test_notifications.py). Both fail against main.

Mechanical, behaviour-preserving

PEP 604/585 annotations (UP045/UP006/UP035), import sorting (I001), pass after docstrings (PIE790), OSError/TimeoutError aliases (UP024/UP041), sorted __all__ (RUF022), unused # noqa removed (RUF100 — E402/N802 are not in the 0.16 default set, so both were inert), ClassVar on mutable class attributes (RUF012), unused unpacked variables prefixed (RUF059), collapsed nested if (SIM102), explicit check=False on every subprocess.run whose returncode is already inspected (PLW1510), tz-aware datetimes that render the same local time as before (DTZ005/DTZ006), and chmod +x on the two scripts carrying a shebang (EXE001).

Verified locally

uv sync --locked --extra dev → ruff 0.16.0
uv run ruff check src tests scripts → All checks passed
uv run pytest tests -q → 153 passed, 2 skipped
uv build → wheel + sdist OK; scripts/check_release_consistency.py OK

Commit type

fix: — patch_tags = ["fix", "perf"], so this cuts a patch release. Warranted: three real behavioural defects ship in the package, not just lint config.

Not in scope

Four ad-hoc root-level scripts (generate_screenshots.py, rename_screenshots.py, test_notification*.py, examples/) still report 15 findings. They are outside the CI gate (src tests scripts) and outside this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM

…16.x Three silent failures found by reading ruff 0.16's expanded default rule set site by site, plus the config change that made those rules visible. Real defects fixed: * `keychain.clear_ingest_token` promised "removed (or already absent)" and its own inline comment said an absent entry was a no-op success -- but the code returned False. keyring raises `PasswordDeleteError` when there is nothing to delete, which the blanket `except Exception` caught. Signing out on a machine that had never signed in reported a failed sign-out. Absent now returns True; a genuine backend error still returns False and is logged (was silent). * `NotificationManager._show_windows` returned True unconditionally after shelling out to PowerShell, ignoring the exit status. On a machine where the WinRT toast API is unavailable, every notification -- including "N automations need attention" -- was reported to the caller as delivered. It now checks returncode and logs stderr, matching the macOS and Linux paths which have always done so. * `IPCServer._accept_loop` caught bare `Exception` and `break`, so the server could stop accepting connections with no message anywhere. Narrowed to `OSError` (all `accept()` can raise) and it now logs when the loop ends for any reason other than `stop()` closing the socket. Also: a corrupt icon PNG and a broken cairosvg install were both indistinguish- able from "file missing" -- now logged; a pystray balloon failure silently fell through to the toast path -- now logged; and `WindowsHandler._find_executable` had a try/except/pass around `os.path.exists`, which never raises, so the handler was unreachable and only hid a failed `import os`. Lint configuration made explicit and bounded: * `ruff>=0.1.0` -> `ruff>=0.16,<0.17`. ruff 0.16.0 grew its default rule set from 59 rules to 413, this project selects no rules, and CI runs a blocking `ruff check src tests scripts`. Only the 0.14.x pin in `uv.lock` was keeping `main` green; the next dependabot bump would have turned it red with 216 findings and no code change. `uv.lock` now resolves 0.16.0, so CI genuinely exercises the new default set. * The FULL default set is kept -- no `select`. The residual after safe autofix was 91 findings in a 3.9k-line tree, small enough to read and judge one by one, so the coverage is worth more than a narrow `["E","F","I","W"]`. * `ignore = ["BLE001"]`, documented inline. The tray is almost nothing but boundaries to optional, platform-owned subsystems (pystray, pynput, Pillow, keyring, desktop-notifier, tkinter, zenity/kdialog, osascript, PowerShell, winreg, ctypes.windll) that share no exception hierarchy; every handler logs and degrades on purpose. Where a blind catch was hiding a defect rather than guarding a boundary, the code was fixed instead. The rest is mechanical and behaviour-preserving: PEP 604/585 annotations, import sorting, explicit `check=False` on `subprocess.run` calls whose returncode is already inspected, `ClassVar` on mutable class attributes, tz-aware datetimes that render the same local time as before, and `chmod +x` on the two scripts that carry a shebang. Regression tests added for both user-visible fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM

Every defect here is one failure class: a failure rendered as a successful empty result. `return []` when we could not look. A `count` of 0 that means "the body was unreadable". A `False` that means "we never asked the user". A bool that means "we did not check". Each fix makes the distinction representable -- a raised exception, a returned reason, a different rendering -- rather than adding a log line above the same wrong value. Every fix below is mutation-checked: the production change was reverted, the new test was confirmed to FAIL, and the fix restored. hosted.py * `CountResult.from_payload` used `payload.get("count", 0)`, so a response body that lost or renamed `count` produced a confident all-clear on the one number the tray exists to report. It now raises `InvalidCountPayload`, `poll_once` reports a failed poll, and the badge keeps its last known value instead of clearing to zero. The display-only subfields stay tolerant of absence. * `_fire_notification` discarded the notifier's return value and `_handle_result` advanced `_last_count` regardless, so the honest `False` that #29 added to `_show_windows` went nowhere: a user was recorded as informed about breaks they were never shown, and the count had to RISE again before a second attempt. Delivery is now propagated and `_last_count` only advances when the notification actually landed, so the next poll retries. * `route_break_click` discarded `webbrowser.open`'s result. A click that opened nothing now returns False, and `TrayApplication.open_needs_attention` tells the user instead of leaving them in front of an unchanged screen. config.py * `TrayConfig.load` printed a warning and returned defaults when tray.json was unreadable. `deployment_lane` defaults to "cloud", so a byoc install -- where the fix must stay local -- was silently moved onto the hosted route by a corrupt settings file. A missing file still means defaults; an unreadable one raises `ConfigLoadError`. `load_or_defaults` hands back both so the caller cannot lose it, and the tray now says so on startup. menu.py * `_get_recent_captures` swallowed every exception and returned `[]`, so an unreadable captures directory rendered the same "No captures" as an empty one. It now raises, and the menu says "Could not read captures". * `_view_capture` never read the exit status of `openadapt visualize`, so a CLI that ran and failed did nothing at all and said nothing. A non-zero exit now falls back to the file browser, like a missing CLI always did. platform/ * `prompt_input` returned None and `confirm_dialog` returned False when no dialog could be shown -- identical to "the user cancelled" and "the user said no". Clicking Start Recording or Delete could therefore do nothing, forever, silently. Both now raise `DialogUnavailableError` when nothing could ask, and the answers they do return are answers the user actually gave. On Linux, zenity/kdialog exit 1 means the user declined; any other non-zero exit is the tool failing, which used to be read as the user's answer AND skipped the remaining fallbacks. On macOS, an osascript execution error is now separated from a user cancel by stderr. Callers: a recording proceeds with a default name (and says why there was no prompt); a delete is not performed and the user is told it was not performed. CI * `ruff check src tests scripts` left examples/ and the four root-level helper scripts outside every gate: 15 findings sat there while CI reported a green lint. The gate now covers the repository and those findings are fixed -- including a screenshot helper that printed a success tick regardless of whether the notification was delivered. * The gitleaks install piped curl into tar without pipefail, so the step reported tar's status rather than the download's. Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

abrichr merged commit 5331935 into main Jul 28, 2026
10 checks passed

abrichr deleted the fix/honest-failure-reporting-and-ruff-0-16 branch July 28, 2026 02:23

abrichr mentioned this pull request Jul 28, 2026

fix: make unknown outcomes representable instead of success-shaped #30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: report keychain, toast and IPC failures honestly; pin ruff to 0.16.x - #29

fix: report keychain, toast and IPC failures honestly; pin ruff to 0.16.x#29
abrichr merged 1 commit into
mainfrom
fix/honest-failure-reporting-and-ruff-0-16

abrichr commented Jul 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

abrichr commented Jul 28, 2026

Why now

Rule-set decision: keep the FULL ruff 0.16 default set

Real defects this exposed

Mechanical, behaviour-preserving

Verified locally

Commit type

Not in scope

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant