From 8a9c7713b9662a0bfb7f17f399e9ae57f7343937 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sat, 11 Jul 2026 18:33:43 +0200 Subject: [PATCH] Review fixes: specific sync exceptions, clearer names Address code-review feedback on the sync-before-lock change: - pull_shared_log now catches (SyncError, GitHubSyncError, OSError) instead of a blanket `except Exception`. crdt_sync wraps every requests transport error in GitHubSyncError, so those three are the whole realistic failure surface; the gate still fails closed on all of them, and a genuine bug now surfaces instead of being hidden as "sync unavailable". Drops the broad-exception pylint disable and the per-file BLE001 ignore in pyproject. - Rename _cli_gate._is_due -> _should_lock: too easily confused with the pure local predicate gate_is_due it wraps. The new name says what it decides. - Rename the list `satisfied` -> `satisfied_slots` in _reconcile_after_fetch; it's a list of slots, not a boolean. No behaviour change; 100% coverage retained. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01FtSmzACzJSGyEatgK9exkd --- diet_guard/_cli_gate.py | 21 ++++++++++++--------- diet_guard/_gatelock_mealflow.py | 6 +++--- diet_guard/_sync.py | 16 +++++++++++----- diet_guard/tests/test_cli_gate.py | 12 ++++++------ diet_guard/tests/test_sync.py | 14 +++++++++++--- pyproject.toml | 4 ---- 6 files changed, 43 insertions(+), 30 deletions(-) diff --git a/diet_guard/_cli_gate.py b/diet_guard/_cli_gate.py index 0345a53..563ec20 100644 --- a/diet_guard/_cli_gate.py +++ b/diet_guard/_cli_gate.py @@ -19,12 +19,15 @@ if TYPE_CHECKING: from collections.abc import Callable -def _is_due(emit: Callable[[str], None]) -> bool: - """Return the gate decision, refreshing from sync only when a lock is due. +def _should_lock(emit: Callable[[str], None]) -> bool: + """Decide whether to lock, pulling from sync first if a lock looks due. - The cheap local check runs first; the network pull happens solely on the - rare tick a lock would otherwise fire (via :func:`pull_shared_log`, which - fails closed), then the decision is re-read from the freshly written log. + This wraps the pure, local :func:`~diet_guard._gate.gate_is_due` predicate + with one network step: the cheap local check runs first, and only when it + says a lock is due do we pull the shared log (via :func:`pull_shared_log`, + which fails closed) and re-read the freshly written log. So a meal just + logged on the phone can clear the lock, while a not-due tick never touches + the network at all. """ if not gate_is_due(): return False @@ -40,8 +43,8 @@ def cmd_gate(emit: Callable[[str], None], *, check: bool, demo: bool) -> int: Three modes: ``--check`` is a headless decision (no window) whose exit code a timer reads; ``--demo`` always shows a safe demo window; bare ``gate`` shows the real lock only when one is due. Both real modes route through - :func:`_is_due`, which -- only when a lock is otherwise due -- pulls the - shared log first (see :func:`_refresh_from_sync`) so a meal just logged on + :func:`_should_lock`, which -- only when a lock is otherwise due -- pulls + the shared log first (via :func:`pull_shared_log`) so a meal just logged on the phone unlocks without a manual re-entry. A flock guard stops a second window from stacking on top of the first, and a window-opening mode first waits for the X display so a session-start launch never crashes unshown. @@ -56,10 +59,10 @@ def cmd_gate(emit: Callable[[str], None], *, check: bool, demo: bool) -> int: For ``--check``: 0 if not due, 1 if a lock is due. Otherwise 0. """ if check: - due = _is_due(emit) + due = _should_lock(emit) emit("due (a lock is warranted)" if due else "ok (no lock needed)") return 1 if due else 0 - if not demo and not _is_due(emit): + if not demo and not _should_lock(emit): emit("ok - no lock needed right now.") return 0 handle = acquire_gate_lock() diff --git a/diet_guard/_gatelock_mealflow.py b/diet_guard/_gatelock_mealflow.py index 52916ea..93c9ff6 100644 --- a/diet_guard/_gatelock_mealflow.py +++ b/diet_guard/_gatelock_mealflow.py @@ -283,9 +283,9 @@ class _GateMealFlow(_GateNutrition): def _reconcile_after_fetch(self) -> None: """Drop pending slots a freshly pulled meal now covers; unlock if none.""" still_due = set(due_slots()) - satisfied = [slot for slot in self._pending if slot not in still_due] + satisfied_slots = [slot for slot in self._pending if slot not in still_due] self._refresh_dashboard() - if not satisfied: + if not satisfied_slots: self._set_status("No new meals found in sync.") return self._pending = [slot for slot in self._pending if slot in still_due] @@ -294,7 +294,7 @@ class _GateMealFlow(_GateNutrition): return self._clear_inputs() self._refresh_slot_header() - count = len(satisfied) + count = len(satisfied_slots) meal_word = "meal" if count == 1 else "meals" self._set_status(f"Pulled {count} {meal_word} — next meal, please.") self._widgets.desc_text.focus_set() diff --git a/diet_guard/_sync.py b/diet_guard/_sync.py index 1715372..826fec8 100644 --- a/diet_guard/_sync.py +++ b/diet_guard/_sync.py @@ -13,7 +13,7 @@ from __future__ import annotations import json import logging -from crdt_sync import GitHubSyncClient, Log, merge_logs +from crdt_sync import GitHubSyncClient, GitHubSyncError, Log, merge_logs from diet_guard._constants import ( SYNC_DEVICE_ID, @@ -138,12 +138,18 @@ def pull_shared_log() -> str | None: A thin wrapper over :func:`run_sync` for callers that must never crash on a sync error: the gate's automatic pre-lock refresh and the lock screen's manual "Fetch from sync" button. Returns ``None`` on success, or a short - human-readable reason when the pull could not complete (no token, network - down, a disk error writing the merged log back), so the caller keeps its - own lock decision rather than failing open. + human-readable reason when the pull could not complete, so the caller keeps + its own lock decision rather than failing open. + + The three caught types are the whole realistic failure surface of a run: + :class:`SyncError` (no/empty token), :class:`~crdt_sync.GitHubSyncError` + (the client wraps every ``requests`` transport error in this), and + :class:`OSError` (reading the token or writing the merged log back). A bug + outside these is deliberately *not* swallowed -- it should surface, not be + silently reported as a sync outage. """ try: run_sync() - except Exception as exc: # pylint: disable=broad-exception-caught + except (SyncError, GitHubSyncError, OSError) as exc: return f"sync unavailable ({exc})" return None diff --git a/diet_guard/tests/test_cli_gate.py b/diet_guard/tests/test_cli_gate.py index 6f45531..ef088ba 100644 --- a/diet_guard/tests/test_cli_gate.py +++ b/diet_guard/tests/test_cli_gate.py @@ -8,7 +8,7 @@ from datetime import datetime, timezone from unittest.mock import MagicMock, patch from diet_guard import _cli_gate -from diet_guard._cli_gate import _is_due, cmd_gate +from diet_guard._cli_gate import _should_lock, cmd_gate from diet_guard._estimator import Nutrition from diet_guard._gate import gate_is_due from diet_guard._state import log_meal @@ -109,7 +109,7 @@ class TestCmdGate: assert "display not ready" in lines[0] -class TestIsDue: +class TestShouldLock: """The decision helper only pays the network cost when a lock is due.""" def test_not_due_skips_sync(self) -> None: @@ -119,7 +119,7 @@ class TestIsDue: patch.object(_cli_gate, "gate_is_due", return_value=False), patch.object(_cli_gate, "pull_shared_log", pull), ): - assert _is_due([].append) is False + assert _should_lock([].append) is False pull.assert_not_called() def test_due_then_cleared_by_sync(self) -> None: @@ -129,7 +129,7 @@ class TestIsDue: patch.object(_cli_gate, "gate_is_due", side_effect=[True, False]), patch.object(_cli_gate, "pull_shared_log", pull), ): - assert _is_due([].append) is False + assert _should_lock([].append) is False pull.assert_called_once() def test_due_and_still_due_after_sync(self) -> None: @@ -139,7 +139,7 @@ class TestIsDue: patch.object(_cli_gate, "gate_is_due", side_effect=[True, True]), patch.object(_cli_gate, "pull_shared_log", pull), ): - assert _is_due([].append) is True + assert _should_lock([].append) is True pull.assert_called_once() def test_pull_failure_is_reported(self) -> None: @@ -151,7 +151,7 @@ class TestIsDue: _cli_gate, "pull_shared_log", return_value="sync unavailable (x)" ), ): - assert _is_due(lines.append) is True + assert _should_lock(lines.append) is True assert "sync unavailable (x)" in lines[0] diff --git a/diet_guard/tests/test_sync.py b/diet_guard/tests/test_sync.py index df3b6d5..45ecc7b 100644 --- a/diet_guard/tests/test_sync.py +++ b/diet_guard/tests/test_sync.py @@ -213,9 +213,17 @@ class TestPullSharedLog: assert _sync.pull_shared_log() is None run_sync.assert_called_once_with() - def test_returns_reason_on_any_failure(self) -> None: - """Any run_sync error becomes a reason string, never an exception.""" - with patch.object(_sync, "run_sync", side_effect=RuntimeError("boom")): + def test_returns_reason_on_expected_failure(self) -> None: + """A real sync failure (here a network error) becomes a reason string.""" + with patch.object(_sync, "run_sync", side_effect=_sync.GitHubSyncError("boom")): reason = _sync.pull_shared_log() assert reason is not None assert "boom" in reason + + def test_unexpected_error_is_not_swallowed(self) -> None: + """A bug outside the known failure surface surfaces, not hidden.""" + with ( + patch.object(_sync, "run_sync", side_effect=KeyError("bug")), + pytest.raises(KeyError), + ): + _sync.pull_shared_log() diff --git a/pyproject.toml b/pyproject.toml index 5076468..df80097 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,10 +42,6 @@ unfixable = [] [tool.ruff.lint.per-file-ignores] "**/tests/**/*.py" = ["ARG", "D", "PLC0415", "PLR2004", "S101", "S107", "SLF001"] "**/test_*.py" = ["ARG", "D", "PLC0415", "PLR2004", "S101", "S107", "SLF001"] -# pull_shared_log must fail closed: any run_sync error (no token, network, disk -# write-back) is returned as a reason string so the gate keeps locking rather -# than failing open. A deliberately broad catch, so BLE001 does not apply here. -"**/_sync.py" = ["BLE001"] [tool.ruff.lint.pydocstyle] convention = "google"