From 79879263ba8fcd69dab11f5c2f927fec76fa6cec Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 17 Jul 2026 09:08:54 +0200 Subject: [PATCH] Support multiple workouts per day; fix the dead sync; ban silent failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The workout log stored ONE workout per calendar day, so a second same-day workout was silently dropped and could never earn credit. Multiple workouts per day: - workout_log.json: {date: entry} -> {date: [entry, ...]}. Legacy entries are wrapped on read (_log_io.load_workout_log) so their HMACs stay valid — no re-signing; every write normalizes the file, migrating it progressively. - New write chokepoint (_log_mixin.write_signed_entry) APPENDS and dedups by a stable workout_id, returning RecordResult so callers can tell a new workout from a duplicate. Re-verifying the same workout is a no-op. - count_weekly_workouts is anti-gaming: each VERIFIED workout counts individually; self-reported manual entries count at most once per day. Four real workouts in one day satisfy the week; four manual entries count as one. - Reward: the day's first counted workout gets the base +2h; an additional same-day VERIFIED workout gets +1h; an additional manual gets nothing. The old "+1h when weekly_count > 4" gate is replaced — weekly volume is the banked bonus's job and shouldn't be paid twice. - Fixed: the unlock screen labelled the +1h "#{weekly_count - 4}", which can render negative under the new rule. Removed the dead runnerup_manual type. - Flutter: the manual budget counts distinct DAYS, matching the PC. PC->phone sync (it had never worked once): - push_pc_manuals pushed from a side-store written at log-time, so anything logged before that feature existed could never publish. _manual_push now derives the crdt log directly from workout_log.json — the single source of truth — covering manual AND verified runs. The record id is the workout's own workout_id (derived for legacy entries to match the phone's format) and the HLC comes from the entry's own timestamp, so a re-push is byte-identical and the repo never churns. - The phone reads every kind (readMergedWorkoutPayloads) and lists the PC's workouts under "SYNCED FROM PC". The manual budget still sees manual-only, so a verified run can't consume a self-report slot. - Settings claimed "Connected." whenever a token STRING existed. It now verifies against the API on open, so a revoked token reports 401 instead of showing a green lie. Never fail silently: - 30 swallowed exceptions across 19 files now log WARNING with the concrete cause AND consequence — plus the token guards in _workout_sync that returned [] in total silence, the exact bug that hid the dead sync for weeks. - New pre-commit hook (scripts/check_silent_failures.py) fails any except that neither re-raises nor logs loudly. ruff's S110 only catches `except: pass`; the real pattern here was `except: log.info(); return []`. Documented in CLAUDE.md. Verified on real data: the Jul 13 run (pulled from its actual TCX over ADB) now counts alongside that day's manual squash, and all three workouts are published to kuhyx/syncs. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Fv8ULVt3Q67GwCFbM7doHB --- .pre-commit-config.yaml | 5 + CLAUDE.md | 21 ++ pyproject.toml | 6 + screen_locker/_auto_upgrade.py | 25 +- screen_locker/_compliance_state.py | 49 +-- screen_locker/_extra_benefits.py | 9 +- screen_locker/_log_io.py | 75 +++++ screen_locker/_log_mixin.py | 117 +++++-- screen_locker/_manual_push.py | 197 ++++++++---- screen_locker/_manual_sync.py | 62 ++-- screen_locker/_manual_workout.py | 48 +-- screen_locker/_manual_workout_dialog.py | 17 +- screen_locker/_phone_verification.py | 16 +- screen_locker/_runnerup_db.py | 7 +- screen_locker/_runnerup_verification.py | 71 ++--- screen_locker/_shutdown.py | 8 +- screen_locker/_shutdown_base.py | 18 +- screen_locker/_sick_dialog.py | 7 +- screen_locker/_sick_tracker.py | 8 +- screen_locker/_status.py | 61 ++-- screen_locker/_status_data.py | 67 ++-- screen_locker/_status_view_verify.py | 160 ++++++++++ screen_locker/_time_check.py | 7 +- screen_locker/_weekly_check.py | 69 +++-- screen_locker/_workout_credit.py | 69 +++-- screen_locker/_workout_sync.py | 42 ++- screen_locker/screen_lock.py | 7 +- screen_locker/status_view.py | 116 +++---- screen_locker/tests/_status_view_helpers.py | 6 +- screen_locker/tests/test_early_bird_part2.py | 4 +- screen_locker/tests/test_init_and_log.py | 7 +- .../tests/test_init_and_log_part2.py | 8 +- screen_locker/tests/test_log_io.py | 91 ++++++ .../tests/test_log_mixin_chokepoint.py | 124 ++++++++ screen_locker/tests/test_manual_push.py | 292 ++++++++++++------ screen_locker/tests/test_manual_sync.py | 58 ++-- .../tests/test_manual_workout_features.py | 142 ++++----- .../tests/test_manual_workout_status_view.py | 12 +- screen_locker/tests/test_mcp.py | 3 +- .../tests/test_runnerup_verification_part3.py | 93 ++++-- .../tests/test_screen_lock_coverage_part1.py | 8 +- .../tests/test_screen_lock_coverage_part2.py | 93 +++++- screen_locker/tests/test_status.py | 34 +- screen_locker/tests/test_status_data.py | 53 +++- screen_locker/tests/test_status_view.py | 61 +++- screen_locker/tests/test_status_view_part2.py | 88 +++++- scripts/check_file_length.py | 12 +- scripts/check_silent_failures.py | 104 +++++++ .../lib/models/manual_workout.dart | 19 +- .../lib/screens/history_screen.dart | 110 ++++++- .../lib/screens/settings_screen.dart | 16 +- .../lib/services/workout_sync_service.dart | 44 ++- .../test/models/manual_workout_test.dart | 53 ++++ .../test/screens/history_screen_test.dart | 92 +++++- .../test/screens/settings_screen_test.dart | 38 ++- .../services/workout_sync_service_test.dart | 72 +++++ 56 files changed, 2335 insertions(+), 766 deletions(-) create mode 100644 screen_locker/_log_io.py create mode 100644 screen_locker/_status_view_verify.py create mode 100644 screen_locker/tests/test_log_io.py create mode 100644 screen_locker/tests/test_log_mixin_chokepoint.py create mode 100755 scripts/check_silent_failures.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 58c0ab3..1f8bd47 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -147,6 +147,11 @@ repos: entry: python3 scripts/check_file_length.py language: system types_or: [python, shell] + - id: no-silent-failures + name: Block silent failures (swallowed exceptions must log why) + entry: python3 scripts/check_silent_failures.py + language: system + types: [python] - id: flutter-analyze name: flutter analyze (workout app) entry: bash -c 'cd stronglift_replacement/workout_app && flutter analyze' diff --git a/CLAUDE.md b/CLAUDE.md index a46a479..158090c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -3,3 +3,24 @@ If tests fail on the same issue twice in a row, STOP and ask the user how to pro ALWAYS confirm that the feature you add / bug you fixed behaves as it should by running the program after your changes (not tests!) and inspecting output comparing it with what user wanted, after confirming by yourself ask user if the program behaves as they intended After running tests fix all coverage gaps and issues, do not ignore unless specifically instructed to do so You are NOT done until you install the new version on the phone itself (flutter install --debug from the workout_app directory, then adb shell monkey -p com.kuhy.workout_app to launch). + +## NEVER fail silently + +A failure that says nothing is a bug. The PC's workout sync did nothing for +weeks because a missing token `return`ed `[]` and real errors only logged at +`INFO` — nobody could tell "nothing to do" from "it broke". + +- **Swallowed exceptions must say why, loudly.** Every `except` must either + re-`raise` or log at `warning`/`error`/`exception`/`critical` with the + concrete reason. `debug`/`info` do NOT count — an invisible log is a silent + failure with extra steps. Enforced by `scripts/check_silent_failures.py` + (pre-commit hook `no-silent-failures`); ruff's `S110` only catches + `except: pass`, which is the rare case. +- **Guard clauses that abort work must log too.** `if token is None: return` + is a silent no-op. Say what was missing and what it means. +- **Return something meaningful.** Prefer a typed result (e.g. + `_manual_push.PushResult(pushed, record_count, reason)`) over a bare + `None`/`[]`/`False`, so the caller can react instead of guessing. `reason` + should read like a sentence a human can act on. +- **No escape hatch** — `# noqa` is banned repo-wide. If a failure really is + benign, log it at `warning` and move on. diff --git a/pyproject.toml b/pyproject.toml index c10e63d..da3e27d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -75,6 +75,12 @@ unfixable = [] # (verifier_factory, temperature_fetcher, log_file, ...) — same # dependency-injection rationale as _status_data.py's gather_status below. "screen_locker/status_view.py" = ["PLC0415", "PLR0913", "SLF001", "T201"] +# The verify-and-credit flow split out of status_view.py: it deliberately +# drives the bare verifier's own mixin methods (_verify_phone_workout, +# _verify_runnerup_workout, _apply_workout_credit) — the same single-source-of +# -truth reuse status_view.py above is exempted for — and _make_bare_verifier +# imports ScreenLocker inside the function to avoid an import cycle. +"screen_locker/_status_view_verify.py" = ["PLC0415", "SLF001"] # explain_lock_decision takes one arg per real file path + sick history, and # returns early per lock-decision-chain stage (see module docstring) — both # are the deliberate design, not something to collapse into a config object. diff --git a/screen_locker/_auto_upgrade.py b/screen_locker/_auto_upgrade.py index 80df079..e75bac8 100644 --- a/screen_locker/_auto_upgrade.py +++ b/screen_locker/_auto_upgrade.py @@ -69,7 +69,11 @@ class AutoUpgradeMixin: try: status, message = self._verify_phone_workout() except (OSError, RuntimeError) as exc: - _logger.info("Auto-upgrade phone check failed: %s", exc) + _logger.warning( + "Sick-day auto-upgrade could not reach the phone (%s) — today's " + "sick_day entry stays unverified; trying RunnerUp next", + exc, + ) status, message = "error", str(exc) if status == "verified": self.workout_data["type"] = "phone_verified" @@ -82,7 +86,11 @@ class AutoUpgradeMixin: try: runnerup_status, runnerup_msg = self._verify_runnerup_workout() except (OSError, RuntimeError) as exc: - _logger.info("Auto-upgrade RunnerUp check failed: %s", exc) + _logger.warning( + "Sick-day auto-upgrade could not read RunnerUp either (%s) — " + "today stays a sick_day, NOT upgraded to a verified workout", + exc, + ) return False if runnerup_status != "verified": _logger.info( @@ -101,7 +109,11 @@ class AutoUpgradeMixin: try: status, message = self._verify_phone_workout() except (OSError, RuntimeError) as exc: - _logger.info("Early bird upgrade phone check failed: %s", exc) + _logger.warning( + "Early-bird auto-upgrade could not reach the phone (%s) — the " + "early_bird entry stays unverified; trying RunnerUp next", + exc, + ) status, message = "error", str(exc) if status == "verified": self.workout_data["type"] = "phone_verified" @@ -114,7 +126,12 @@ class AutoUpgradeMixin: try: runnerup_status, runnerup_msg = self._verify_runnerup_workout() except (OSError, RuntimeError) as exc: - _logger.info("Early bird RunnerUp check failed: %s", exc) + _logger.warning( + "Early-bird auto-upgrade could not read RunnerUp either (%s) — " + "the expired early_bird entry is NOT upgraded, so the screen " + "will lock", + exc, + ) return False if runnerup_status != "verified": _logger.info( diff --git a/screen_locker/_compliance_state.py b/screen_locker/_compliance_state.py index ee7489c..5e2e845 100644 --- a/screen_locker/_compliance_state.py +++ b/screen_locker/_compliance_state.py @@ -31,6 +31,7 @@ from screen_locker._constants import ( EARLY_BIRD_END_MINUTE, EARLY_BIRD_START_HOUR, ) +from screen_locker._log_io import load_workout_log from screen_locker._sick_tracker import is_sick_day as _is_sick_day if TYPE_CHECKING: @@ -92,29 +93,34 @@ def is_scheduled_skip_today( try: with scheduled_skips_file.open() as f: skips = json.load(f) - except (OSError, json.JSONDecodeError): + except (OSError, json.JSONDecodeError) as exc: + _logger.warning( + "Could not read scheduled skips from %s: %s — treating today as NOT " + "a scheduled skip (the lock chain continues)", + scheduled_skips_file, + exc, + ) return False return (today or _today_str()) in skips def has_logged_today(log_file: Path, *, today: str | None = None) -> bool: - """Return True if a validly-signed workout is logged for *today*.""" - if not log_file.exists(): + """Return True if a validly-signed workout is logged for *today*. + + The day may hold multiple workouts; return True if ANY of today's entries + verifies (or is acceptably unsigned when no HMAC key is configured). + """ + entries = load_workout_log(log_file).get(today or _today_str(), []) + if not entries: return False - try: - with log_file.open() as f: - logs = json.load(f) - except (OSError, json.JSONDecodeError): - return False - entry = logs.get(today or _today_str()) - if entry is None: - return False - if verify_entry_hmac(entry): - return True - if compute_entry_hmac({"_probe": True}) is None and "hmac" not in entry: - _logger.info("HMAC key unavailable — accepting unsigned entry") - return True - _logger.warning("HMAC verification failed for today's log entry") + key_unavailable = compute_entry_hmac({"_probe": True}) is None + for entry in entries: + if verify_entry_hmac(entry): + return True + if key_unavailable and "hmac" not in entry: + _logger.info("HMAC key unavailable — accepting unsigned entry") + return True + _logger.warning("HMAC verification failed for today's log entries") return False @@ -125,7 +131,14 @@ def is_early_bird_pending(pending_file: Path, *, today: str | None = None) -> bo try: with pending_file.open() as f: state = json.load(f) - except (OSError, json.JSONDecodeError): + except (OSError, json.JSONDecodeError) as exc: + _logger.warning( + "Could not read the early-bird pending marker at %s: %s — treating " + "it as not pending, so the lock chain proceeds as if no early bird " + "was claimed today", + pending_file, + exc, + ) return False if not isinstance(state, dict) or state.get("date") != (today or _today_str()): return False diff --git a/screen_locker/_extra_benefits.py b/screen_locker/_extra_benefits.py index d47bf58..a4bd7cf 100644 --- a/screen_locker/_extra_benefits.py +++ b/screen_locker/_extra_benefits.py @@ -36,7 +36,14 @@ def _load_state(state_file: Path) -> dict[str, Any]: try: with state_file.open() as f: return json.load(f) - except (OSError, json.JSONDecodeError): + except (OSError, json.JSONDecodeError) as exc: + _logger.warning( + "Could not read extra-benefits state from %s: %s — starting from " + "empty state, so the workout streak and any banked bonus hours are " + "lost for this run", + state_file, + exc, + ) return {} diff --git a/screen_locker/_log_io.py b/screen_locker/_log_io.py new file mode 100644 index 0000000..7a08e68 --- /dev/null +++ b/screen_locker/_log_io.py @@ -0,0 +1,75 @@ +"""Single normalization point for reading ``workout_log.json``. + +The log is day-keyed and now holds MULTIPLE workouts per day. This module is +the one place that reads the raw file and normalizes both shapes into a +per-day list: + +* legacy ``{date: entry}`` — the single ``entry`` is wrapped in a + one-element list, and +* new ``{date: [entry, ...]}`` — returned as-is. + +Kept dependency-free (stdlib only) so any module — including the low-level +``_log_mixin`` and ``_compliance_state`` — can import it without risking an +import cycle. Every consumer reads through :func:`load_workout_log` and +iterates the per-day list rather than assuming one entry per day. +""" + +from __future__ import annotations + +import json +import logging +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from pathlib import Path + +_logger = logging.getLogger(__name__) + + +def read_raw_log(log_file: Path) -> dict: + """Load ``workout_log.json`` verbatim (values may be dict or list), or ``{}``. + + Returns the on-disk shape unchanged. Prefer :func:`load_workout_log` for + reading; this raw form exists for the writer, which rewrites the file, and + for the one-shot migration. + """ + if not log_file.exists(): + return {} + try: + with log_file.open() as f: + data = json.load(f) + except (OSError, json.JSONDecodeError) as exc: + _logger.warning( + "Could not read the workout log at %s (%s) — treating it as EMPTY, " + "so NO workouts count and the lock will fire until it is readable", + log_file, + exc, + ) + return {} + if not isinstance(data, dict): + _logger.warning( + "Workout log at %s is a %s, not an object of dates — treating it as " + "EMPTY, so NO workouts count", + log_file, + type(data).__name__, + ) + return {} + return data + + +def load_workout_log(log_file: Path) -> dict[str, list[dict]]: + """Load ``workout_log.json`` normalized to ``{date: [entry, ...]}``. + + Accepts BOTH the legacy day-keyed shape (``{date: entry}``) and the new + multi-entry shape so old files, rolled-back writers, and mid-migration + states all read correctly. Each entry is the usual + ``{timestamp, workout_data, hmac[, workout_id]}`` dict; malformed values + are dropped. Missing/corrupt file → ``{}``. + """ + normalized: dict[str, list[dict]] = {} + for date, value in read_raw_log(log_file).items(): + if isinstance(value, list): + normalized[date] = [entry for entry in value if isinstance(entry, dict)] + elif isinstance(value, dict): + normalized[date] = [value] + return normalized diff --git a/screen_locker/_log_mixin.py b/screen_locker/_log_mixin.py index 2588bc9..2119d19 100644 --- a/screen_locker/_log_mixin.py +++ b/screen_locker/_log_mixin.py @@ -2,6 +2,7 @@ from __future__ import annotations +from dataclasses import dataclass, field from datetime import datetime, timezone import json import logging @@ -11,6 +12,8 @@ from gatelock.log_integrity import compute_entry_hmac from screen_locker import _compliance_state from screen_locker._constants import SCHEDULED_SKIPS_FILE +from screen_locker._log_io import load_workout_log +from screen_locker._manual_workout import MANUAL_WORKOUT_TYPE, manual_sync_record_id if TYPE_CHECKING: from collections.abc import Mapping @@ -19,42 +22,108 @@ if TYPE_CHECKING: _logger = logging.getLogger(__name__) -def _read_logs(log_file: Path) -> dict: - """Load ``workout_log.json`` as a dict, or ``{}`` if missing/corrupt.""" - if not log_file.exists(): - return {} - try: - with log_file.open() as f: - return json.load(f) - except (OSError, json.JSONDecodeError): - return {} +@dataclass(frozen=True) +class RecordResult: + """Outcome of appending a workout entry to the day-keyed log. + + ``appended`` is ``False`` when an entry with the same ``workout_id`` was + already present for that day — the append was a no-op, so the caller must + apply NO new credit (re-verifying the same workout is idempotent). + ``prior_entries`` is the day's entries *before* this append, which the + credit path uses to tell a first-of-day workout from an additional one. + """ + + appended: bool + prior_entries: list[dict] = field(default_factory=list) + + +def _derive_workout_id(date: str, workout_data: Mapping[str, object]) -> str | None: + """Return a stable dedup id for ``workout_data`` on ``date``. + + An explicit ``workout_id`` in ``workout_data`` wins (used by manual-sync + ingestion, which carries the wire record id). Otherwise a manual workout + reuses :func:`manual_sync_record_id` so a locally-logged manual and the + same one synced back share an id; every other type keys on + ``"{type}:{date}"`` — idempotent per day, which matches the "one verified + check per day" reality of the live phone/RunnerUp verifiers. + """ + explicit = workout_data.get("workout_id") + if explicit: + return str(explicit) + wtype = workout_data.get("type") + if not wtype: + return None + if wtype == MANUAL_WORKOUT_TYPE: + start = str(workout_data.get("start_time", "")).strip() + if start: + return manual_sync_record_id(date, start) + return f"{wtype}:{date}" + + +def _entry_workout_id(date: str, entry: Mapping[str, object]) -> str | None: + """Return an already-logged entry's dedup id, deriving it if it has none. + + Entries written before ``workout_id`` existed carry no id. Comparing only + the stored field would treat them as un-matchable, so the SAME workout + syncing back from another device would append a duplicate — which is + exactly what happened to a pre-migration manual workout once the PC began + publishing its history. Deriving the id the same way the writer would have + makes a legacy entry match its own synced copy. + """ + stored = entry.get("workout_id") + if stored: + return str(stored) + workout_data = entry.get("workout_data", {}) + if not isinstance(workout_data, dict): + return None + return _derive_workout_id(date, workout_data) def write_signed_entry( log_file: Path, date: str, workout_data: Mapping[str, object] -) -> None: - """Write one HMAC-signed workout entry into ``log_file`` keyed by ``date``. +) -> RecordResult: + """Append one HMAC-signed workout entry for ``date``, deduped by workout_id. - Shared by the live save path (today's workout) and manual-sync ingestion, - which files a synced workout under its OWN date so the weekly count places - it in the right ISO week. The log is day-keyed: one entry per date. + The single write chokepoint. Shared by the live save path (today's + workout) and manual-sync ingestion, which files a synced workout under its + OWN date so the weekly count places it in the right ISO week. The log is + day-keyed but now holds MULTIPLE entries per day: this APPENDS rather than + overwrites. If an entry with the same ``workout_id`` already exists for the + day, it is a no-op — the log itself is the idempotency store, so + re-verifying the same workout never double-records. Every write normalizes + the whole file to the list shape, progressively migrating legacy entries. + + Returns a :class:`RecordResult` so the credit path can tell an appended + workout from a duplicate and a first-of-day workout from an additional one. """ - logs = _read_logs(log_file) + logs = load_workout_log(log_file) + entries = logs.setdefault(date, []) + prior = list(entries) + + workout_id = _derive_workout_id(date, workout_data) + if workout_id is not None and any( + _entry_workout_id(date, e) == workout_id for e in entries + ): + return RecordResult(appended=False, prior_entries=prior) + entry: dict[str, object] = { "timestamp": datetime.now(tz=timezone.utc).isoformat(), "workout_data": workout_data, } + if workout_id is not None: + entry["workout_id"] = workout_id signature = compute_entry_hmac(entry) if signature is not None: entry["hmac"] = signature else: _logger.warning("HMAC key unavailable — saving unsigned entry") - logs[date] = entry + entries.append(entry) try: with log_file.open("w") as f: json.dump(logs, f, indent=2) except OSError as e: _logger.warning("Could not save workout log: %s", e) + return RecordResult(appended=True, prior_entries=prior) class LogMixin: @@ -73,15 +142,17 @@ class LogMixin: """Check if workout has been logged today with valid HMAC.""" return _compliance_state.has_logged_today(self.log_file) - def _load_existing_logs(self) -> dict: - """Load existing workout logs from file.""" - return _read_logs(self.log_file) - def _is_scheduled_skip_today(self) -> bool: """Return True if today's date is listed in the scheduled skips file.""" return _compliance_state.is_scheduled_skip_today(SCHEDULED_SKIPS_FILE) - def save_workout_log(self) -> None: - """Save today's workout data to the log file with an HMAC signature.""" + def save_workout_log(self) -> RecordResult: + """Append today's workout to the log (HMAC-signed) and report the result. + + Returns the :class:`RecordResult` from the write chokepoint so callers + (notably :meth:`WorkoutCreditMixin._apply_workout_credit`) can apply + credit only for a genuinely new workout and scale it by whether today + already had one. + """ today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") - write_signed_entry(self.log_file, today, self.workout_data) + return write_signed_entry(self.log_file, today, self.workout_data) diff --git a/screen_locker/_manual_push.py b/screen_locker/_manual_push.py index d2ff5af..67815bf 100644 --- a/screen_locker/_manual_push.py +++ b/screen_locker/_manual_push.py @@ -1,19 +1,28 @@ -"""Push PC-originated manual workouts to the shared sync repo. +"""Push the PC's workouts to the shared sync repo. -The PC manual-workout form writes its entry into ``workout_log.json`` (day-keyed, -locally). For the two-way shared budget, the PC must ALSO publish its manual -workouts to the sync repo so the phone can see them. This module keeps a small -crdt-sync log of *PC-originated* manuals (``manual_sync_log.json``, next to -``workout_log.json``) and pushes it to ``devices/pc/log.json``. +``workout_log.json`` is the single source of truth: this module derives the +crdt-sync log directly from it and pushes to ``devices/pc/log.json``, so the +phone converges on the SAME history the PC has — manual workouts *and* +machine-verified ones (StrongLifts sessions, RunnerUp runs). + +Deriving from the log (rather than a side-store written at log-time) means a +workout is published automatically no matter when or how it was recorded, +including entries logged before this module existed. Idempotence comes from two +properties: the record id is the workout's own stable ``workout_id``, and its +HLC is derived deterministically from the entry's own timestamp — so re-pushing +an unchanged log produces a byte-identical record set and no repo churn. This reverses the old "the PC only ever reads, never pushes" invariant (it had -no data of its own to contribute); manual workouts are that data. Pushing needs -a sync token with **contents:write** on the repo — a read-only token 403s, which -is swallowed (best-effort), so the local form still works offline. +no data of its own to contribute). Pushing needs a sync token with +**contents:write** on the repo — a read-only token 403s. Nothing here fails +silently: every path returns a :class:`PushResult` whose ``reason`` says exactly +what happened, and logs it. """ from __future__ import annotations +from dataclasses import dataclass +from datetime import datetime, timezone import json import logging from typing import TYPE_CHECKING @@ -30,16 +39,14 @@ from screen_locker._constants import ( SYNC_REPO_NAME, SYNC_REPO_OWNER, SYNC_TIMEOUT_SECONDS, + SYNC_TOKEN_FILE, ) -from screen_locker._manual_workout import ( - MANUAL_WORKOUT_SYNC_KIND, - _today_iso, - manual_sync_record_id, -) +from screen_locker._log_io import load_workout_log +from screen_locker._log_mixin import _derive_workout_id +from screen_locker._weekly_check import COUNTED_WORKOUT_TYPES from screen_locker._workout_sync import _DEVICES_PREFIX, read_sync_token if TYPE_CHECKING: - from collections.abc import Mapping from pathlib import Path from crdt_sync import Log @@ -47,13 +54,21 @@ if TYPE_CHECKING: _logger = logging.getLogger(__name__) _PC_DEVICE_ID = "pc" -_STORE_FILENAME = "manual_sync_log.json" _PAYLOAD_FIELD = "payload" -def _store_path(log_file: Path) -> Path: - """Return the PC manual-sync store path next to ``workout_log.json``.""" - return log_file.parent / _STORE_FILENAME +@dataclass(frozen=True) +class PushResult: + """Outcome of :func:`push_pc_workouts` — never silent. + + ``reason`` always states what happened in plain words, whether or not the + push succeeded, so a caller (and the log) can tell "nothing to do" apart + from "it broke". + """ + + pushed: bool + record_count: int + reason: str def _encode_log(log: Log) -> str: @@ -67,61 +82,97 @@ def _decode_log(text: str) -> Log: return {rid: Record.from_dict(data) for rid, data in raw.items()} -def _load_store(store_file: Path) -> dict[str, Record]: - """Load the local PC manual store, or ``{}`` if missing/corrupt.""" - if not store_file.exists(): - return {} - try: - return dict(_decode_log(store_file.read_text())) - except (OSError, ValueError, KeyError, TypeError): - _logger.warning("Corrupt PC manual store at %s — ignoring", store_file) - return {} +def _entry_wall_ms(entry: dict, date: str) -> int: + """Return the entry's timestamp in epoch ms, falling back to its date. - -def _max_hlc(log: dict[str, Record]) -> Hlc | None: - """Return the greatest HLC across the store's payload fields, if any.""" - hlcs = [ - record.fields[_PAYLOAD_FIELD][1] - for record in log.values() - if _PAYLOAD_FIELD in record.fields - ] - return max(hlcs) if hlcs else None - - -def record_pc_manual(log_file: Path, entry: Mapping[str, object]) -> str: - """Record a PC-originated manual workout into the local sync store. - - ``entry`` is the :func:`build_entry` dict the form already produced; this - wraps it in the shared sync payload (adds ``kind`` + ``date``) under a - stable ``manual:`` id and appends it to the store with a fresh monotonic - HLC. Fast and local — the network push happens separately - (:func:`push_pc_manuals`). Returns the record id. + The HLC is derived from this, so it must be stable for a given entry — the + workout's own recorded time is exactly that. A missing/unparsable timestamp + falls back to that day's midnight UTC (still stable) and says so in the log, + rather than silently inventing "now", which would churn the repo on every + push. """ - date = _today_iso() - store_file = _store_path(log_file) - log = _load_store(store_file) - payload = {**entry, "kind": MANUAL_WORKOUT_SYNC_KIND, "date": date} - record_id = manual_sync_record_id(date, str(entry.get("start_time", ""))) - hlc = Hlc.new_tick(_PC_DEVICE_ID, previous=_max_hlc(log)) - log[record_id] = Record(id=record_id, fields={_PAYLOAD_FIELD: (payload, hlc)}) - store_file.write_text(_encode_log(log)) - return record_id + raw = entry.get("timestamp") + if isinstance(raw, str): + try: + return int(datetime.fromisoformat(raw).timestamp() * 1000) + except ValueError: + _logger.warning( + "Workout entry for %s has an unparsable timestamp (%r) — using " + "that date's midnight for its sync clock", + date, + raw, + ) + else: + _logger.warning( + "Workout entry for %s has no timestamp — using that date's midnight " + "for its sync clock", + date, + ) + midnight = datetime.fromisoformat(date).replace(tzinfo=timezone.utc) + return int(midnight.timestamp() * 1000) -def push_pc_manuals(log_file: Path) -> None: - """Push the PC's manual-workout store to ``devices/pc/log.json``. +def records_from_workout_log(log_file: Path) -> dict[str, Record]: + """Derive the crdt-sync log from ``workout_log.json``. - Best-effort: no token, an empty store, or any sync error (notably a 403 - from a read-only token that lacks ``contents:write``) is swallowed so the - local form keeps working. Runs one full sync tick, so it also retries any - manuals a previous push failed to publish. + Every counted workout becomes one record keyed by its ``workout_id`` — the + same id the phone mints for its own records, so the two sides dedup to one + rather than doubling. Entries written before ``workout_id`` existed get + theirs derived the same way it would have been, so history publishes too. + The payload is the workout's own data plus the ``kind``/``date`` the wire + contract requires. + """ + log: dict[str, Record] = {} + for date, entries in load_workout_log(log_file).items(): + for entry in entries: + workout_data = entry.get("workout_data", {}) + if not isinstance(workout_data, dict): + continue + if workout_data.get("type") not in COUNTED_WORKOUT_TYPES: + continue + record_id = entry.get("workout_id") or _derive_workout_id( + date, workout_data + ) + if not record_id: + _logger.warning( + "Workout on %s (type=%r) has no derivable id — NOT synced", + date, + workout_data.get("type"), + ) + continue + payload = {**workout_data, "kind": workout_data["type"], "date": date} + hlc = Hlc.new_tick(_PC_DEVICE_ID, wall_time_ms=_entry_wall_ms(entry, date)) + log[str(record_id)] = Record( + id=str(record_id), fields={_PAYLOAD_FIELD: (payload, hlc)} + ) + return log + + +def push_pc_workouts(log_file: Path) -> PushResult: + """Publish every counted workout in ``log_file`` to ``devices/pc/log.json``. + + Runs one full sync tick, so it also retries anything a previous push failed + to publish. It never raises into the caller — but it is never silent either: + the returned :class:`PushResult` and a WARNING say why a push did not happen. """ token = read_sync_token() if token is None: - return - log = _load_store(_store_path(log_file)) + reason = f"no sync token at {SYNC_TOKEN_FILE}" + _logger.warning( + "Workouts NOT synced: %s — create a fine-grained GitHub PAT with " + "contents:write on %s/%s and save it there (chmod 600)", + reason, + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + ) + return PushResult(pushed=False, record_count=0, reason=reason) + + log = records_from_workout_log(log_file) if not log: - return + reason = f"no counted workouts in {log_file}" + _logger.warning("Workouts NOT synced: %s", reason) + return PushResult(pushed=False, record_count=0, reason=reason) + client = GitHubSyncClient( SYNC_REPO_OWNER, SYNC_REPO_NAME, @@ -138,4 +189,18 @@ def push_pc_manuals(log_file: Path) -> None: decode=_decode_log, ) except GitHubSyncError as exc: - _logger.info("PC manual push skipped (%s)", exc) + reason = f"sync error: {exc}" + _logger.warning( + "Workout sync push FAILED for %d workout(s): %s — a 403 here means " + "the token lacks contents:write on %s/%s", + len(log), + exc, + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + ) + return PushResult(pushed=False, record_count=len(log), reason=reason) + + _logger.info( + "Synced %d workout(s) to %s/%s", len(log), SYNC_REPO_OWNER, SYNC_REPO_NAME + ) + return PushResult(pushed=True, record_count=len(log), reason="pushed") diff --git a/screen_locker/_manual_sync.py b/screen_locker/_manual_sync.py index ec5c796..ef6f232 100644 --- a/screen_locker/_manual_sync.py +++ b/screen_locker/_manual_sync.py @@ -19,7 +19,8 @@ from __future__ import annotations import logging from typing import TYPE_CHECKING -from screen_locker._log_mixin import _read_logs, write_signed_entry +from screen_locker._log_io import load_workout_log +from screen_locker._log_mixin import write_signed_entry from screen_locker._manual_workout import ( MANUAL_WORKOUT_SYNC_KIND, SPORT_OTHER, @@ -28,7 +29,6 @@ from screen_locker._manual_workout import ( is_budget_exhausted, validate_manual_workout, ) -from screen_locker._weekly_check import COUNTED_WORKOUT_TYPES if TYPE_CHECKING: from collections.abc import Iterable, Mapping @@ -86,36 +86,34 @@ def reconstruct_draft(payload: Mapping[str, object]) -> ManualWorkoutDraft | Non activity_details=str(payload.get("activity_details", "")), equipment=str(payload.get("equipment", "")), ) - except (KeyError, TypeError, ValueError): + except (KeyError, TypeError, ValueError) as exc: + _logger.warning( + "Synced manual workout payload is malformed (%s: %s) — SKIPPING " + "this record, so it will not be logged or counted on the PC", + type(exc).__name__, + exc, + ) return None -def _already_ingested(logs: dict, record_id: str) -> bool: - """True if a log entry already carries this sync record id.""" - for entry in logs.values(): - if not isinstance(entry, dict): - continue - workout_data = entry.get("workout_data", {}) - if ( - isinstance(workout_data, dict) - and workout_data.get(_SYNC_ID_FIELD) == record_id - ): - return True +def _already_ingested(logs: dict[str, list[dict]], record_id: str) -> bool: + """True if any logged entry already carries this sync record id. + + Iterates the per-day lists (a day may hold several workouts); a cheap + early-out before reconstructing the draft. The write chokepoint also dedups + by ``workout_id``, so this and that are two guards on the same idempotency. + """ + for entries in logs.values(): + for entry in entries: + workout_data = entry.get("workout_data", {}) + if ( + isinstance(workout_data, dict) + and workout_data.get(_SYNC_ID_FIELD) == record_id + ): + return True return False -def _date_has_counted_workout(logs: dict, date: str) -> bool: - """True if ``date`` already holds a workout that counts (day-keyed log).""" - entry = logs.get(date) - if not isinstance(entry, dict): - return False - workout_data = entry.get("workout_data", {}) - return ( - isinstance(workout_data, dict) - and workout_data.get("type") in COUNTED_WORKOUT_TYPES - ) - - def ingest_manual_records( log_file: Path, records: Iterable[tuple[str, Mapping[str, object]]], @@ -126,9 +124,10 @@ def ingest_manual_records( For each ``(record_id, payload)`` tagged ``kind="manual_workout"``: reconstruct + re-validate the draft on the PC, enforce the rate budget, then - HMAC-sign and write it under its own ``date``. Idempotent (dedup by - ``record_id``) and non-destructive: it never overwrites an existing counted - entry for a day (the log is day-keyed — a real workout already credits it). + HMAC-sign and APPEND it under its own ``date`` (a day may hold several + workouts). Idempotent: dedup by ``record_id`` here and by ``workout_id`` in + the write chokepoint, so re-syncing the same record is a no-op. A back-dated + ingest raises the weekly count but never pushes tonight's shutdown. Returns the record ids actually ingested. """ ingested: list[str] = [] @@ -139,10 +138,7 @@ def ingest_manual_records( if not isinstance(date, str): _logger.warning("Manual record %s has no date — skipped", record_id) continue - logs = _read_logs(log_file) - if _already_ingested(logs, record_id): - continue - if _date_has_counted_workout(logs, date): + if _already_ingested(load_workout_log(log_file), record_id): continue draft = reconstruct_draft(payload) if draft is None: diff --git a/screen_locker/_manual_workout.py b/screen_locker/_manual_workout.py index 7a2a198..2c642d0 100644 --- a/screen_locker/_manual_workout.py +++ b/screen_locker/_manual_workout.py @@ -17,9 +17,8 @@ from __future__ import annotations from dataclasses import dataclass from datetime import datetime, timedelta, timezone -import json import logging -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING from screen_locker._constants import ( MANUAL_WORKOUT_BUDGET_PER_7_DAYS, @@ -30,6 +29,7 @@ from screen_locker._constants import ( MANUAL_WORKOUT_RPE_MAX, MANUAL_WORKOUT_RPE_MIN, ) +from screen_locker._log_io import load_workout_log if TYPE_CHECKING: from pathlib import Path @@ -56,42 +56,42 @@ def _parse_iso(date_str: str) -> datetime | None: """Parse ``YYYY-MM-DD`` into a UTC datetime, or return None.""" try: return datetime.strptime(date_str, "%Y-%m-%d").replace(tzinfo=timezone.utc) - except ValueError: + except ValueError as exc: + _logger.warning( + "Workout log has an unparsable date key %r (%s) — that day is " + "IGNORED when counting the manual-workout budget", + date_str, + exc, + ) return None -def _load_logs(log_file: Path) -> dict[str, Any]: - """Read ``workout_log.json``. Missing or unreadable file yields ``{}``.""" - if not log_file.exists(): - return {} - try: - with log_file.open() as f: - return json.load(f) - except (OSError, json.JSONDecodeError): - _logger.warning("Could not read workout log for manual-workout budget check") - return {} - - def count_in_window( log_file: Path, days: int, *, today: str | None = None, ) -> int: - """Return how many ``manual_workout`` entries fall in the trailing window.""" + """Return how many DAYS in the trailing window hold a ``manual_workout``. + + The manual-workout budget is counted per DAY: several self-reports on one + day consume a single slot (and only one counts toward the weekly total + anyway), so a day contributes at most one here. + """ today_str = today or _today_iso() today_dt = _parse_iso(today_str) if today_dt is None: return 0 cutoff = today_dt - timedelta(days=days) - logs = _load_logs(log_file) count = 0 - for date_str, entry in logs.items(): + for date_str, entries in load_workout_log(log_file).items(): parsed = _parse_iso(date_str) if parsed is None or not (cutoff < parsed <= today_dt): continue - workout_data = entry.get("workout_data", {}) if isinstance(entry, dict) else {} - if workout_data.get("type") == MANUAL_WORKOUT_TYPE: + if any( + entry.get("workout_data", {}).get("type") == MANUAL_WORKOUT_TYPE + for entry in entries + ): count += 1 return count @@ -163,7 +163,13 @@ def _parse_hhmm(value: str) -> datetime | None: """Parse an ``HH:MM`` string into a datetime on an arbitrary fixed date.""" try: return datetime.strptime(value.strip(), "%H:%M").replace(tzinfo=timezone.utc) - except ValueError: + except ValueError as exc: + _logger.warning( + "Manual workout time %r is not HH:MM (%s) — the workout's duration " + "cannot be computed, so the draft will fail validation", + value, + exc, + ) return None diff --git a/screen_locker/_manual_workout_dialog.py b/screen_locker/_manual_workout_dialog.py index 83561f4..feacfe5 100644 --- a/screen_locker/_manual_workout_dialog.py +++ b/screen_locker/_manual_workout_dialog.py @@ -24,7 +24,6 @@ from screen_locker._constants import ( MANUAL_WORKOUT_DESCRIPTION_MIN_CHARS, MANUAL_WORKOUT_REFLECTION_MIN_CHARS, ) -from screen_locker._manual_push import record_pc_manual from screen_locker._manual_workout_widgets import ManualWorkoutFormWidgetsMixin _logger = logging.getLogger(__name__) @@ -255,14 +254,25 @@ class ManualWorkoutDialogMixin(ManualWorkoutFormWidgetsMixin): """Read an int Spinbox var, defaulting to 0 on an invalid value.""" try: return int(self._mw_int_vars[key].get()) - except (tk.TclError, ValueError): + except (tk.TclError, ValueError) as exc: + _logger.warning( + "Manual workout field %r holds a non-integer value (%s) — " + "recording it as 0", + key, + exc, + ) return 0 def _submit_manual_workout_form(self) -> None: """Validate the form and either show an error or save + notify host.""" try: rpe = int(self._mw_rpe_var.get()) - except (tk.TclError, ValueError): + except (tk.TclError, ValueError) as exc: + _logger.warning( + "Manual workout RPE is not an integer (%s) — submitting it as " + "0, which the form validator will reject", + exc, + ) rpe = 0 sport = self._current_mw_sport() draft = _manual_workout.ManualWorkoutDraft( @@ -299,5 +309,4 @@ class ManualWorkoutDialogMixin(ManualWorkoutFormWidgetsMixin): entry = _manual_workout.build_entry(draft) # Record the manual for cross-device sync (published on the next lock # startup). Local + best-effort — never blocks saving/unlocking. - record_pc_manual(self.log_file, entry) self._on_manual_workout_saved(entry) diff --git a/screen_locker/_phone_verification.py b/screen_locker/_phone_verification.py index 3dbe2ca..bfb890a 100644 --- a/screen_locker/_phone_verification.py +++ b/screen_locker/_phone_verification.py @@ -161,7 +161,13 @@ class PhoneVerificationMixin: continue try: data = json.loads(tmp.read_text()) - except (json.JSONDecodeError, OSError): + except (json.JSONDecodeError, OSError) as exc: + _logger.warning( + "Workout JSON pulled from %s is unreadable (%s) — skipping " + "that candidate and trying the remaining phone paths", + remote, + exc, + ) continue if data.get("date") == today: return data @@ -238,7 +244,13 @@ class PhoneVerificationMixin: if resp.status != _HTTP_OK: return None return json.loads(resp.read().decode()) - except (_HTTPException, OSError, ValueError, json.JSONDecodeError): + except (_HTTPException, OSError, ValueError, json.JSONDecodeError) as exc: + _logger.warning( + "HTTP fetch of today's workout from %s failed (%s) — no data " + "from the phone's HTTP server, so verification falls through", + url, + exc, + ) return None # ── Main verification entry point ───────────────────────────────────────── diff --git a/screen_locker/_runnerup_db.py b/screen_locker/_runnerup_db.py index b5de97d..afc5b32 100644 --- a/screen_locker/_runnerup_db.py +++ b/screen_locker/_runnerup_db.py @@ -119,7 +119,12 @@ class RunnerUpDbMixin: ) row = cursor.fetchone() except sqlite3.Error as exc: - _logger.info("RunnerUp DB query failed: %s", exc) + _logger.warning( + "RunnerUp activity query against %s failed: %s — treating it as " + "no run recorded today", + db_path, + exc, + ) return None if row is None: diff --git a/screen_locker/_runnerup_verification.py b/screen_locker/_runnerup_verification.py index 363918f..52e0a7e 100644 --- a/screen_locker/_runnerup_verification.py +++ b/screen_locker/_runnerup_verification.py @@ -8,7 +8,6 @@ Root DB fallback lives in ``_runnerup_db.py``. from __future__ import annotations from datetime import datetime, timedelta, timezone -import json import logging from pathlib import Path import shutil @@ -16,17 +15,15 @@ import tempfile from typing import Any import xml.etree.ElementTree as ET -from gatelock.log_integrity import compute_entry_hmac - from screen_locker._constants import ( MIN_RUN_DISTANCE_KM, MIN_RUN_DURATION_MINUTES, RUNNERUP_ACCEPTED_SPORTS, RUNNERUP_EXPORT_DIRS, ) +from screen_locker._log_mixin import write_signed_entry from screen_locker._runnerup_db import RunnerUpDbMixin from screen_locker._time_check import check_clock_skew -from screen_locker._weekly_check import COUNTED_WORKOUT_TYPES _logger = logging.getLogger(__name__) @@ -97,7 +94,12 @@ class RunnerUpVerificationMixin(RunnerUpDbMixin): try: tree = ET.parse(tcx_path) except ET.ParseError as exc: - _logger.info("TCX parse error in %s: %s", tcx_path, exc) + _logger.warning( + "TCX file %s is not valid XML (%s) — the run it describes CANNOT " + "be verified and will not count", + tcx_path, + exc, + ) return None root = tree.getroot() @@ -155,16 +157,16 @@ class RunnerUpVerificationMixin(RunnerUpDbMixin): "RunnerUp TCX export found but could not be read", ) - def _try_fill_runnerup_for_date(self, date_str: str, logs: dict[str, Any]) -> bool: - """Try to fill one date gap from RunnerUp TCX exports, mutating logs in-place. + def _try_fill_runnerup_for_date(self, date_str: str, log_file: Path) -> bool: + """Append a verified RunnerUp entry for ``date_str`` if not already logged. - Returns True if a verified entry was written for ``date_str``. + Appends via the write chokepoint, which dedups by ``workout_id`` + (``runnerup_verified:{date}``): re-scanning a day whose run is already + recorded is a no-op, but a day that only holds, say, a manual workout + still gets the run appended alongside it (multiple workouts per day). + + Returns True if a new verified entry was appended for ``date_str``. """ - existing = logs.get(date_str, {}) - if isinstance(existing, dict): - wtype = existing.get("workout_data", {}).get("type", "") - if wtype in COUNTED_WORKOUT_TYPES: - return False for remote in self._find_runnerup_exports_for_date(date_str): data = self._pull_and_parse_tcx(remote) if data is None: @@ -172,27 +174,22 @@ class RunnerUpVerificationMixin(RunnerUpDbMixin): status, msg = self._validate_runnerup_data(data) if status != "verified": continue - entry: dict[str, Any] = { - "timestamp": datetime.now(tz=timezone.utc).isoformat(), - "workout_data": { - "type": "runnerup_verified", - "source": f"Auto-scanned: {msg}", - "distance_km": round(data["distance_m"] / 1000, 2), - "duration_minutes": round(data["duration_seconds"] / 60, 1), - }, + workout_data = { + "type": "runnerup_verified", + "source": f"Auto-scanned: {msg}", + "distance_km": round(data["distance_m"] / 1000, 2), + "duration_minutes": round(data["duration_seconds"] / 60, 1), } - signature = compute_entry_hmac(entry) - if signature is not None: - entry["hmac"] = signature - logs[date_str] = entry - _logger.info("Auto-filled RunnerUp entry for %s: %s", date_str, msg) - return True + if write_signed_entry(log_file, date_str, workout_data).appended: + _logger.info("Auto-filled RunnerUp entry for %s: %s", date_str, msg) + return True + return False return False def _scan_and_fill_week_runnerup(self, log_file: Path) -> int: - """Scan the current ISO week for RunnerUp TCX gaps and fill them. + """Scan the current ISO week for RunnerUp runs and append any not logged. - Returns the count of newly filled entries (0 if phone not connected). + Returns the count of newly appended entries (0 if phone not connected). """ if not self._has_adb_device(): _logger.info( @@ -204,27 +201,13 @@ class RunnerUpVerificationMixin(RunnerUpDbMixin): today = now.date() week_start = today - timedelta(days=today.weekday()) - try: - with log_file.open() as f: - logs: dict[str, Any] = json.load(f) - except (OSError, json.JSONDecodeError): - logs = {} - filled = 0 current = week_start while current <= today: - if self._try_fill_runnerup_for_date(current.strftime("%Y-%m-%d"), logs): + if self._try_fill_runnerup_for_date(current.strftime("%Y-%m-%d"), log_file): filled += 1 current += timedelta(days=1) - if filled > 0: - try: - with log_file.open("w") as f: - json.dump(logs, f, indent=2) - except OSError as exc: - _logger.warning("Failed to write workout log after scan: %s", exc) - return 0 - return filled # ------------------------------------------------------------------ diff --git a/screen_locker/_shutdown.py b/screen_locker/_shutdown.py index 1bc4a8a..226be6b 100644 --- a/screen_locker/_shutdown.py +++ b/screen_locker/_shutdown.py @@ -149,7 +149,13 @@ class ShutdownMixin: state = json.load(f) today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") return state.get("date") == today - except (OSError, json.JSONDecodeError): + except (OSError, json.JSONDecodeError) as exc: + _logger.warning( + "Could not read sick-day state from %s: %s — treating sick mode " + "as UNUSED today, which may allow a second sick day", + SICK_DAY_STATE_FILE, + exc, + ) return False def _save_sick_day_state( diff --git a/screen_locker/_shutdown_base.py b/screen_locker/_shutdown_base.py index 167e6b0..b65db7c 100644 --- a/screen_locker/_shutdown_base.py +++ b/screen_locker/_shutdown_base.py @@ -37,7 +37,14 @@ def get_base_hours(state_file: Path) -> tuple[int, int]: int(state.get("base_mon_wed_hour", _DEFAULT_BASE_HOUR)), int(state.get("base_thu_sun_hour", _DEFAULT_BASE_HOUR)), ) - except (OSError, json.JSONDecodeError, ValueError): + except (OSError, json.JSONDecodeError, ValueError) as exc: + _logger.warning( + "Could not read shutdown base hours from %s: %s — defaulting both " + "to %02d:00", + state_file, + exc, + _DEFAULT_BASE_HOUR, + ) return (_DEFAULT_BASE_HOUR, _DEFAULT_BASE_HOUR) @@ -63,8 +70,13 @@ def reset_to_base_if_new_day( state: dict[str, Any] = json.load(f) if state.get("last_reset_date") == today: return False - except (OSError, json.JSONDecodeError): - pass + except (OSError, json.JSONDecodeError) as exc: + _logger.warning( + "Could not read last_reset_date from %s: %s — cannot tell if " + "today was already reset, so resetting to base hours again", + state_file, + exc, + ) base_mw, base_ts = get_base_hours(state_file) diff --git a/screen_locker/_sick_dialog.py b/screen_locker/_sick_dialog.py index f9ad45b..7dcb162 100644 --- a/screen_locker/_sick_dialog.py +++ b/screen_locker/_sick_dialog.py @@ -158,7 +158,12 @@ class SickDialogMixin: onset = self._sick_onset_var.get() try: severity = int(self._sick_severity_var.get()) - except (tk.TclError, ValueError): + except (tk.TclError, ValueError) as exc: + _logger.warning( + "Sick-day severity is not an integer (%s) — submitting it as 0, " + "which the justification validator will reject", + exc, + ) severity = 0 text = self._sick_text_widget.get("1.0", "end").strip() draft = _sick_tracker.JustificationDraft( diff --git a/screen_locker/_sick_tracker.py b/screen_locker/_sick_tracker.py index a526bd3..36ece92 100644 --- a/screen_locker/_sick_tracker.py +++ b/screen_locker/_sick_tracker.py @@ -49,7 +49,13 @@ def _parse_iso(date_str: str) -> datetime | None: """Parse ``YYYY-MM-DD`` into a UTC datetime, or return None.""" try: return datetime.strptime(date_str, "%Y-%m-%d").replace(tzinfo=timezone.utc) - except ValueError: + except ValueError as exc: + _logger.warning( + "Sick history holds an unparsable date %r (%s) — that entry is " + "IGNORED when counting sick days", + date_str, + exc, + ) return None diff --git a/screen_locker/_status.py b/screen_locker/_status.py index 319fc0a..771230d 100644 --- a/screen_locker/_status.py +++ b/screen_locker/_status.py @@ -4,6 +4,7 @@ from __future__ import annotations from datetime import date, datetime, timedelta, timezone import json +import logging import sys from typing import TYPE_CHECKING @@ -13,6 +14,7 @@ from screen_locker._extra_benefits import ( has_extended_early_bird, weekly_shutdown_bonus_hours, ) +from screen_locker._log_io import load_workout_log from screen_locker._sick_tracker import load_history from screen_locker._weekly_check import ( COUNTED_WORKOUT_TYPES, @@ -25,16 +27,7 @@ if TYPE_CHECKING: from screen_locker.screen_lock import ScreenLocker - -def _load_log(log_file: Path) -> dict: - """Load the workout log dict, returning {} on any error.""" - if not log_file.exists(): - return {} - try: - with log_file.open() as f: - return json.load(f) - except (OSError, json.JSONDecodeError): - return {} +_logger = logging.getLogger(__name__) def _load_extra_benefits() -> dict: @@ -43,26 +36,38 @@ def _load_extra_benefits() -> dict: return {} try: return json.loads(EXTRA_BENEFITS_FILE.read_text()) - except (OSError, ValueError): + except (OSError, ValueError) as exc: + _logger.warning( + "Could not read extra-benefits state from %s: %s — the status view " + "will show a 0 streak and no bonus hours, which may be wrong", + EXTRA_BENEFITS_FILE, + exc, + ) return {} -def _print_day_line(d: date, entry: dict | None, sick_days: set[str]) -> bool: - """Print one day's status line. Returns True if it counted as a workout.""" +def _print_day_line(d: date, entries: list[dict], sick_days: set[str]) -> bool: + """Print one day's status line(s). Returns True if any workout counted. + + A day may hold several workouts — each is printed on its own line. + """ label = d.strftime("%a %b %d") - if entry is None: + if not entries: if d.isoformat() in sick_days: print(f" {label} ✗ sick_day") else: print(f" {label} — no entry") return False - wtype = entry.get("workout_data", {}).get("type", "?") - src = entry.get("workout_data", {}).get("source", "") - counted = wtype in COUNTED_WORKOUT_TYPES - src_str = f" ({src[:45]})" if src else "" - mark = "✓" if counted else "✗" - print(f" {label} {mark} {wtype}{src_str}") - return counted + any_counted = False + for entry in entries: + wtype = entry.get("workout_data", {}).get("type", "?") + src = entry.get("workout_data", {}).get("source", "") + counted = wtype in COUNTED_WORKOUT_TYPES + any_counted = any_counted or counted + src_str = f" ({src[:45]})" if src else "" + mark = "✓" if counted else "✗" + print(f" {label} {mark} {wtype}{src_str}") + return any_counted def run_status(locker: ScreenLocker) -> None: @@ -70,7 +75,7 @@ def run_status(locker: ScreenLocker) -> None: today = datetime.now(tz=timezone.utc).astimezone().date() monday = today - timedelta(days=today.weekday()) log_file: Path = locker.log_file - log_data = _load_log(log_file) + log_data = load_workout_log(log_file) sick_days = set(load_history().sick_days) print("=== Weekly Workout Status ===") @@ -81,7 +86,7 @@ def run_status(locker: ScreenLocker) -> None: d = monday + timedelta(days=i) if d > today: break - if _print_day_line(d, log_data.get(d.isoformat()), sick_days): + if _print_day_line(d, log_data.get(d.isoformat(), []), sick_days): before_count += 1 print() @@ -113,13 +118,15 @@ def run_status(locker: ScreenLocker) -> None: # Heat skips this month this_month = datetime.now(tz=timezone.utc).astimezone().date().strftime("%Y-%m") heat_entries = [ - (d, e) - for d, e in log_data.items() + (d, entry) + for d, entries in log_data.items() if d.startswith(this_month) - and e.get("workout_data", {}).get("type") == "heat_skip" + for entry in entries + if isinstance(entry, dict) + and entry.get("workout_data", {}).get("type") == "heat_skip" ] if heat_entries: - last_date, last_e = sorted(heat_entries)[-1] + last_date, last_e = max(heat_entries, key=lambda de: de[0]) last_temp = last_e.get("workout_data", {}).get("temperature_celsius", "?") heat_str = f"{len(heat_entries)} (last: {last_date}, {last_temp}°C)" else: diff --git a/screen_locker/_status_data.py b/screen_locker/_status_data.py index f50e1f9..777b0b2 100644 --- a/screen_locker/_status_data.py +++ b/screen_locker/_status_data.py @@ -12,7 +12,6 @@ from __future__ import annotations from dataclasses import dataclass from datetime import date, datetime, timedelta, timezone -import json from pathlib import Path from typing import TYPE_CHECKING @@ -36,6 +35,7 @@ from screen_locker._extra_benefits import ( preview_bonus_if_week_ended_now, weekly_shutdown_bonus_hours, ) +from screen_locker._log_io import load_workout_log from screen_locker._shutdown import read_shutdown_config from screen_locker._shutdown_base import get_base_hours from screen_locker._sick_tracker import ( @@ -46,6 +46,8 @@ from screen_locker._sick_tracker import ( from screen_locker._wake_state import has_workout_skip_today from screen_locker._weekly_check import ( COUNTED_WORKOUT_TYPES, + MANUAL_WORKOUT_TYPE, + VERIFIED_WORKOUT_TYPES, WEEKLY_WORKOUT_MINIMUM, count_weekly_workouts, is_relaxed_day, @@ -66,13 +68,14 @@ _MON_WED_WEEKDAYS = frozenset({0, 1, 2}) @dataclass(frozen=True) class DayStatus: - """One day's workout outcome.""" + """One day's workout outcome (a day may hold several workouts).""" date: str label: str - entry_type: str | None + entry_types: tuple[str, ...] source: str counted: bool + day_count: int is_sick_day: bool @@ -147,61 +150,57 @@ class StatusSnapshot: generated_at: str -def _day_status(day_date: date, entry: dict | None, sick_days: set[str]) -> DayStatus: - """Build a :class:`DayStatus` for one calendar day.""" +def _day_status(day_date: date, entries: list[dict], sick_days: set[str]) -> DayStatus: + """Build a :class:`DayStatus` for one calendar day from its entry list. + + ``day_count`` mirrors the weekly rule (each verified workout counts, all of + a day's manual entries count once) so the week total sums to + :func:`~screen_locker._weekly_check.count_weekly_workouts`. + """ iso = day_date.isoformat() label = day_date.strftime("%a %b %d") - if entry is None: - return DayStatus( - date=iso, - label=label, - entry_type=None, - source="", - counted=False, - is_sick_day=iso in sick_days, - ) - workout_data = entry.get("workout_data", {}) if isinstance(entry, dict) else {} - entry_type = workout_data.get("type") + types = tuple( + str(e.get("workout_data", {}).get("type", "")) + for e in entries + if isinstance(e, dict) + ) + verified = sum(1 for t in types if t in VERIFIED_WORKOUT_TYPES) + has_manual = any(t == MANUAL_WORKOUT_TYPE for t in types) + sources = [ + str(e.get("workout_data", {}).get("source", "")) + for e in entries + if isinstance(e, dict) + ] return DayStatus( date=iso, label=label, - entry_type=entry_type, - source=workout_data.get("source", ""), - counted=entry_type in COUNTED_WORKOUT_TYPES, + entry_types=tuple(t for t in types if t), + source=" · ".join(s for s in sources if s), + counted=any(t in COUNTED_WORKOUT_TYPES for t in types), + day_count=verified + (1 if has_manual else 0), is_sick_day=iso in sick_days, ) -def _load_log(log_file: Path) -> dict: - """Load the workout log dict, returning {} on any error.""" - if not log_file.exists(): - return {} - try: - with log_file.open() as f: - return json.load(f) - except (OSError, json.JSONDecodeError): - return {} - - def _week_days( log_file: Path, sick_history: SickHistory, *, today_local: datetime ) -> tuple[DayStatus, ...]: """Return this ISO week's days from Monday through today.""" - log_data = _load_log(log_file) + log_data = load_workout_log(log_file) sick_days = set(sick_history.sick_days) today_date = today_local.date() monday = today_date - timedelta(days=today_date.weekday()) days: list[DayStatus] = [] day = monday while day <= today_date: - days.append(_day_status(day, log_data.get(day.isoformat()), sick_days)) + days.append(_day_status(day, log_data.get(day.isoformat(), []), sick_days)) day += timedelta(days=1) return tuple(days) def _weekly_summary(days: tuple[DayStatus, ...]) -> WeeklySummary: - """Summarize a week's days against the weekly minimum.""" - counted = sum(1 for d in days if d.counted) + """Summarize a week's workout count against the weekly minimum.""" + counted = sum(d.day_count for d in days) return WeeklySummary( days=days, counted_count=counted, diff --git a/screen_locker/_status_view_verify.py b/screen_locker/_status_view_verify.py new file mode 100644 index 0000000..a318888 --- /dev/null +++ b/screen_locker/_status_view_verify.py @@ -0,0 +1,160 @@ +"""The status window's "Check Phone" verify-and-credit flow. + +Split out of ``status_view`` to keep that module focused on rendering. Holds +the one place the voluntary status window *writes*: a verified workout here is +appended to the log and earns its shutdown reward, exactly as the locked +screen's unlock does. +""" + +from __future__ import annotations + +from concurrent.futures import ThreadPoolExecutor # pylint: disable=no-name-in-module +from typing import TYPE_CHECKING + +from screen_locker._status_data import gather_status + +if TYPE_CHECKING: + from collections.abc import Callable + from concurrent.futures import Future + from pathlib import Path + + from screen_locker._workout_credit import WorkoutCreditResult + from screen_locker.screen_lock import ScreenLocker + + +def _make_bare_verifier(log_file: Path) -> ScreenLocker: + """Build a minimal ``ScreenLocker`` for verification calls. + + Same ``object.__new__`` bypass ``screen_lock.py`` already uses for + ``--status`` — just enough state for the verification mixins' methods + to run, no Tk lock UI, no ``__init__`` side effects. + """ + from screen_locker.screen_lock import ScreenLocker + + verifier = object.__new__(ScreenLocker) + verifier.log_file = log_file + verifier.workout_data = {} + return verifier + + +def _verify_phone_then_runnerup( + verifier: ScreenLocker, +) -> tuple[str | None, str, str, str | None]: + """Check StrongLifts first, then RunnerUp as a fallback (worker-thread safe). + + Mirrors the locked screen's StrongLifts→RunnerUp chain (``_ui_flows.py``): + a user may have run instead of lifted, so a failed phone check falls back + to today's RunnerUp export. Only calls the verifier's ADB methods — never + touches Tk — so it is safe to run inside a background thread. + + Returns ``(credited_type, status, phone_message, runnerup_message)`` where + ``credited_type`` is ``"phone_verified"`` / ``"runnerup_verified"`` on a + verified workout, or ``None`` when neither source verified. + ``runnerup_message`` is ``None`` when StrongLifts verified and RunnerUp was + never consulted. + """ + phone_status, phone_message = verifier._verify_phone_workout() + if phone_status == "verified": + return "phone_verified", phone_status, phone_message, None + runnerup_status, runnerup_message = verifier._verify_runnerup_workout() + if runnerup_status == "verified": + return "runnerup_verified", runnerup_status, phone_message, runnerup_message + return None, runnerup_status, phone_message, runnerup_message + + +class PhoneCheckMixin: + """The "Check Phone" button: verify today's workout, then credit it. + + Composed onto ``StatusWindow``, which supplies ``log_file``, + ``verifier_factory``, ``root``, ``render`` and the result/message slots + declared below. + """ + + log_file: Path + verifier_factory: Callable[[Path], ScreenLocker] + _phone_future: Future[tuple[str | None, str, str, str | None]] | None + _phone_check_result: tuple[str, str] | None + _credit_message: str | None + + def _on_check_phone_clicked(self) -> None: + """Verify today's workout (StrongLifts then RunnerUp) and apply its reward. + + Runs the two-step check in one background worker so the Tk loop stays + responsive, then polls the future. Unlike the old display-only probe, + a verified result here writes the entry and applies the shutdown bonus + (see :meth:`_on_phone_check_result`). + """ + self._phone_check_result = None + self._credit_message = None + verifier = self.verifier_factory(self.log_file) + executor = ThreadPoolExecutor(max_workers=1) + self._phone_future = executor.submit(_verify_phone_then_runnerup, verifier) + executor.shutdown(wait=False) + self._poll_phone_check() + + def _poll_phone_check(self) -> None: + """Poll the background verification future until it resolves.""" + if self._phone_future is not None and self._phone_future.done(): + self._on_phone_check_result(*self._phone_future.result()) + else: + self.root.after(500, self._poll_phone_check) + + def _on_phone_check_result( + self, + credited_type: str | None, + status: str, + phone_message: str, + runnerup_message: str | None, + ) -> None: + """Apply credit on a verified workout, else show a combined failure. + + On a verified result this writes the entry and applies the reward via + ``_apply_workout_credit`` — the same path the locked screen's + ``unlock_screen`` and the "Log Manual Workout" button use, guarded + against double-crediting on the same day. When neither source verified, + it shows both messages so the user knows why nothing counted. + """ + if credited_type is not None: + message = ( + runnerup_message + if credited_type == "runnerup_verified" + else phone_message + ) + source_label = ( + "RunnerUp" if credited_type == "runnerup_verified" else "StrongLifts" + ) + verifier = self.verifier_factory(self.log_file) + verifier.workout_data = {"type": credited_type, "source": message or ""} + credit = verifier._apply_workout_credit() + self._credit_message = "\n".join( + self._credit_result_lines(f"{source_label} verified: {message}", credit) + ) + else: + self._phone_check_result = ( + status, + f"StrongLifts: {phone_message} · RunnerUp: {runnerup_message}", + ) + self.render(gather_status()) + + def _credit_result_lines( + self, header: str, credit: WorkoutCreditResult + ) -> list[str]: + """Human-readable outcome lines for an applied workout credit. + + Shared by the "Check Phone" verify path and the "Log Manual Workout" + path so both report the reward identically: the base +2h shutdown push + for the day's first workout, the +1h bonus for an additional same-day + verified workout, and any debt cleared — or a note when this exact + workout was already recorded today (idempotent, no new credit). + """ + lines = [header] + if credit.already_counted_today: + lines.append("(Already recorded today — no additional credit.)") + return lines + if credit.shutdown_adjusted: + lines.append("Shutdown time +2h later!") + if credit.extra_bonus_delta > 0: + lines.append(f"Extra workout today! +{credit.extra_bonus_delta}h tonight") + if credit.new_debt is not None: + lines.append(f"Workout debt: {credit.new_debt}") + return lines diff --git a/screen_locker/_time_check.py b/screen_locker/_time_check.py index dc64373..4392880 100644 --- a/screen_locker/_time_check.py +++ b/screen_locker/_time_check.py @@ -35,7 +35,12 @@ def _query_ntp_offset(server: str = "pool.ntp.org") -> float | None: data, _ = sock.recvfrom(1024) t4 = time.time() except OSError as exc: - _logger.info("NTP query to %s failed: %s", server, exc) + _logger.warning( + "NTP query to %s failed: %s — the local clock CANNOT be checked for " + "tampering, so the skew check is skipped", + server, + exc, + ) return None if len(data) < _NTP_MIN_PACKET_SIZE: diff --git a/screen_locker/_weekly_check.py b/screen_locker/_weekly_check.py index 488eff0..23d8d34 100644 --- a/screen_locker/_weekly_check.py +++ b/screen_locker/_weekly_check.py @@ -12,9 +12,10 @@ current ISO week (Mon-Sun). from __future__ import annotations from datetime import datetime, timedelta, timezone -import json import logging -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING + +from screen_locker._log_io import load_workout_log if TYPE_CHECKING: from pathlib import Path @@ -26,13 +27,20 @@ WEEKLY_WORKOUT_MINIMUM: int = 4 # Python weekday(): Mon=0, Tue=1, Wed=2, Thu=3, Fri=4, Sat=5, Sun=6 _RELAXED_WEEKDAYS: frozenset[int] = frozenset({1, 2, 3}) # Tue, Wed, Thu -# Workout types that count toward the weekly minimum *and* earn the -# shutdown-time bonus (see screen_lock._try_adjust_shutdown_for_workout). -# Exported (no leading underscore) so screen_lock.py can share this single -# source of truth instead of duplicating the type check. -COUNTED_WORKOUT_TYPES: frozenset[str] = frozenset( - {"phone_verified", "runnerup_manual", "runnerup_verified", "manual_workout"}, +# VERIFIED workouts are machine-checked (a real StrongLifts session or a real +# RunnerUp run). They count toward the weekly total INDIVIDUALLY — multiple +# per day are allowed — because they can't be faked. +VERIFIED_WORKOUT_TYPES: frozenset[str] = frozenset( + {"phone_verified", "runnerup_verified"}, ) +# The single self-reported type. It counts at most ONCE per day (see +# count_weekly_workouts) so it can't be used to inflate the weekly total. +MANUAL_WORKOUT_TYPE: str = "manual_workout" +# Types that count toward the weekly minimum *and* are eligible for the base +# shutdown-time bonus (see screen_lock._try_adjust_shutdown_for_workout). +# Exported (no leading underscore) so other modules share this single source +# of truth instead of duplicating the type check. +COUNTED_WORKOUT_TYPES: frozenset[str] = VERIFIED_WORKOUT_TYPES | {MANUAL_WORKOUT_TYPE} def is_relaxed_day(*, today: datetime | None = None) -> bool: @@ -53,46 +61,53 @@ def count_weekly_workouts( *, today: datetime | None = None, ) -> int: - """Count phone-verified workouts logged in the current ISO week (Mon-Sun). + """Count counted workouts in the current ISO week under the anti-gaming rule. + + Each VERIFIED workout (:data:`VERIFIED_WORKOUT_TYPES`) counts individually, + so multiple real workouts on one day all count. Self-reported + ``manual_workout`` entries count at most ONCE per day, so they can't inflate + the total. Feeds both the weekly lock minimum (:func:`has_weekly_minimum`) + and the banked bonus. Args: log_file: Path to ``workout_log.json``. today: Override for the current local datetime (for testing). Returns: - Number of ``phone_verified`` entries whose date falls within the - current ISO week, up to and including today. + The weekly workout count (Mon-Sun, up to and including today) under the + verified-stack / manual-once-per-day rule. """ dt = today if today is not None else datetime.now(tz=timezone.utc).astimezone() week_start = (dt - timedelta(days=dt.weekday())).date() today_date = dt.date() - if not log_file.exists(): - return 0 - try: - with log_file.open() as f: - logs: dict[str, Any] = json.load(f) - except (OSError, json.JSONDecodeError): - _logger.warning("Could not read workout log for weekly count") - return 0 - count = 0 - for date_str, entry in logs.items(): + for date_str, entries in load_workout_log(log_file).items(): try: entry_date = ( datetime.strptime(date_str, "%Y-%m-%d") .replace(tzinfo=timezone.utc) .date() ) - except ValueError: + except ValueError as exc: + _logger.warning( + "Workout log has an unparsable date key %r (%s) — its workouts " + "are NOT counted toward this week's total", + date_str, + exc, + ) continue if not (week_start <= entry_date <= today_date): continue - if not isinstance(entry, dict): - continue - wtype = entry.get("workout_data", {}).get("type", "") - if wtype in COUNTED_WORKOUT_TYPES: - count += 1 + has_manual = False + for entry in entries: + wtype = entry.get("workout_data", {}).get("type", "") + if wtype in VERIFIED_WORKOUT_TYPES: + count += 1 # each verified workout counts + elif wtype == MANUAL_WORKOUT_TYPE: + has_manual = True + if has_manual: + count += 1 # all of a day's manual entries count as one return count diff --git a/screen_locker/_workout_credit.py b/screen_locker/_workout_credit.py index bed43bb..be99c74 100644 --- a/screen_locker/_workout_credit.py +++ b/screen_locker/_workout_credit.py @@ -10,14 +10,13 @@ miss (see ``project-lock-disabled-pending-manual-log`` memory). from __future__ import annotations from dataclasses import dataclass -from datetime import datetime, timezone import logging from typing import TYPE_CHECKING from screen_locker import _sick_tracker from screen_locker._weekly_check import ( COUNTED_WORKOUT_TYPES, - WEEKLY_WORKOUT_MINIMUM, + VERIFIED_WORKOUT_TYPES, count_weekly_workouts, ) @@ -77,37 +76,41 @@ class WorkoutCreditMixin: _sick_tracker.save_history(history) return new_debt - def _was_already_counted_today(self) -> bool: - """Check whether today's log entry, before this save, already counted. - - Used to guard :meth:`_apply_workout_credit` against double-crediting: - unlike the locked flow (which only ever unlocks once), the voluntary - ``StatusWindow`` entry point can save more than one workout on the - same day (the manual-workout budget allows up to 2/week), and - ``_adjust_shutdown_time_later`` is additive, not idempotent. - """ - logs = self._load_existing_logs() - today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") - entry = logs.get(today) - if not isinstance(entry, dict): - return False - workout_data = entry.get("workout_data", {}) - return workout_data.get("type") in COUNTED_WORKOUT_TYPES - def _apply_workout_credit(self) -> WorkoutCreditResult: - """Persist ``workout_data`` and apply reward credit, once per day. + """Append ``workout_data`` and apply its reward, scaled to the day. - Shared by the locked-screen flow (:meth:`ScreenLocker.unlock_screen`) - and the voluntary ``StatusWindow`` "Log Manual Workout" path. + Shared by the locked-screen flow (:meth:`ScreenLocker.unlock_screen`), + the voluntary ``StatusWindow`` "Log Manual Workout" path, and the + Check-Phone verify path. Rewards, from the day's entries *before* this + one (via the write chokepoint's :class:`RecordResult`): + + * duplicate (same ``workout_id`` already logged) → no new credit; + * first counted workout of the day → base **+2h** (cap 23:00); + * an additional same-day VERIFIED workout → **+1h** (cap midnight); + * an additional same-day manual workout → no shutdown credit + (self-reports can't stack intra-day credit — anti-gaming). + + This method always files under *today*, so its credit is the intra-day + reward; back-dated writes (manual-sync, RunnerUp backfill) go straight + through the write chokepoint and earn only the weekly count, never a + push to tonight's shutdown. """ - already_counted = self._was_already_counted_today() # sick_day is already persisted to sick_history.json by # _finalize_sick_day — workout_log.json is reserved for real outcomes. - if self.workout_data.get("type") != "sick_day": - self.save_workout_log() + if self.workout_data.get("type") == "sick_day": + return WorkoutCreditResult( + shutdown_adjusted=False, + new_debt=None, + extra_bonus_delta=0, + weekly_count=count_weekly_workouts(self.log_file), + already_counted_today=False, + ) + result = self.save_workout_log() weekly_count = count_weekly_workouts(self.log_file) - if already_counted: + + if not result.appended: + # A workout with this id was already recorded today — idempotent. return WorkoutCreditResult( shutdown_adjusted=False, new_debt=None, @@ -116,18 +119,24 @@ class WorkoutCreditMixin: already_counted_today=True, ) - shutdown_adjusted = self._try_adjust_shutdown_for_workout() - new_debt = self._clear_debt_on_verified_workout() + first_counted_today = not any( + entry.get("workout_data", {}).get("type") in COUNTED_WORKOUT_TYPES + for entry in result.prior_entries + ) - # Extra-workout bonus: +1h per workout above the weekly minimum. + shutdown_adjusted = False extra_bonus_delta = 0 - if weekly_count > WEEKLY_WORKOUT_MINIMUM: + if first_counted_today: + shutdown_adjusted = self._try_adjust_shutdown_for_workout() + elif self.workout_data.get("type") in VERIFIED_WORKOUT_TYPES: old_cfg = self._read_shutdown_config() if old_cfg and self._adjust_shutdown_time_by(1): new_cfg = self._read_shutdown_config() if new_cfg: extra_bonus_delta = new_cfg[1] - old_cfg[1] + new_debt = self._clear_debt_on_verified_workout() + return WorkoutCreditResult( shutdown_adjusted=shutdown_adjusted, new_debt=new_debt, diff --git a/screen_locker/_workout_sync.py b/screen_locker/_workout_sync.py index 4c0c4f8..9cdfded 100644 --- a/screen_locker/_workout_sync.py +++ b/screen_locker/_workout_sync.py @@ -95,6 +95,15 @@ def pull_synced_workout() -> tuple[dict | None, str | None]: """ token = read_sync_token() if token is None: + _logger.warning( + "Cannot pull the synced StrongLifts session: no sync token at %s — " + "GitHub sync is OFF, so only ADB/HTTP can verify a phone workout. " + "Create a fine-grained PAT with contents:write on %s/%s and save it " + "there (chmod 600)", + SYNC_TOKEN_FILE, + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + ) return None, None client = GitHubSyncClient( @@ -106,6 +115,14 @@ def pull_synced_workout() -> tuple[dict | None, str | None]: try: text = client.get_file_text(_LOG_PATH) except GitHubSyncError as exc: + _logger.warning( + "Could not fetch the phone's workout log %s from %s/%s: %s — " + "falling back to ADB/HTTP verification", + _LOG_PATH, + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + exc, + ) return None, str(exc) if text is None: @@ -154,6 +171,15 @@ def pull_all_manual_records() -> list[tuple[str, dict]]: """ token = read_sync_token() if token is None: + _logger.warning( + "Pulling NO synced manual workouts: no sync token at %s — GitHub " + "sync is OFF, so workouts logged on the phone will NOT count here. " + "Create a fine-grained PAT with contents:write on %s/%s and save it " + "there (chmod 600)", + SYNC_TOKEN_FILE, + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + ) return [] client = GitHubSyncClient( @@ -165,7 +191,14 @@ def pull_all_manual_records() -> list[tuple[str, dict]]: try: devices = client.list_directory(_DEVICES_PREFIX) except GitHubSyncError as exc: - _logger.info("Manual sync unavailable (%s)", exc) + _logger.warning( + "Could not list device logs under %s in %s/%s: %s — pulling NO " + "synced manual workouts, so phone-logged workouts will not count", + _DEVICES_PREFIX, + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + exc, + ) return [] merged: dict[str, tuple[dict, Hlc]] = {} @@ -174,7 +207,12 @@ def pull_all_manual_records() -> list[tuple[str, dict]]: try: text = client.get_file_text(path) except GitHubSyncError as exc: - _logger.info("Skipping device log %s (%s)", path, exc) + _logger.warning( + "Could not fetch device log %s: %s — SKIPPING that device, so " + "any manual workouts it holds will not count", + path, + exc, + ) continue if text is None: continue diff --git a/screen_locker/screen_lock.py b/screen_locker/screen_lock.py index e6afc37..5872eb4 100755 --- a/screen_locker/screen_lock.py +++ b/screen_locker/screen_lock.py @@ -40,7 +40,7 @@ from screen_locker._extra_benefits import ( ) from screen_locker._heat_skip import HeatSkipMixin from screen_locker._log_mixin import LogMixin -from screen_locker._manual_push import push_pc_manuals +from screen_locker._manual_push import push_pc_workouts from screen_locker._manual_sync import ingest_manual_records from screen_locker._manual_workout_dialog import ManualWorkoutDialogMixin from screen_locker._phone_verification import PhoneVerificationMixin @@ -219,7 +219,7 @@ class ScreenLocker( def _ingest_synced_manual_workouts(self) -> None: """Sync manual workouts: publish this PC's, ingest everyone else's.""" - push_pc_manuals(self.log_file) + push_pc_workouts(self.log_file) ingested = ingest_manual_records(self.log_file, pull_all_manual_records()) for record_id in ingested: _logger.info("Ingested synced manual workout: %s", record_id) @@ -289,9 +289,8 @@ class ScreenLocker( color="#ffaa00", ) if credit.extra_bonus_delta > 0: - extra_n = credit.weekly_count - WEEKLY_WORKOUT_MINIMUM self._text( - f"Extra workout #{extra_n}! +{credit.extra_bonus_delta}h tonight", + f"Extra workout today! +{credit.extra_bonus_delta}h tonight", font_size=20, color="#ffaa00", ) diff --git a/screen_locker/status_view.py b/screen_locker/status_view.py index c9706a3..64c6c61 100644 --- a/screen_locker/status_view.py +++ b/screen_locker/status_view.py @@ -1,26 +1,28 @@ """Read-only Tkinter status window plus a lightweight i3blocks summary CLI. Opening or refreshing this window only ever reads files already on disk -(via ``_status_data.gather_status``), with two deliberate exceptions that -only ever *display* a result and never write to ``workout_log.json``: +(via ``_status_data.gather_status``). The live Warsaw temperature (see +``_section_temperature``) is fetched via +``_temperature.fetch_current_temp_with_status`` on open and on every refresh, +in its own background thread, bounded to ``_temperature.HARD_TIMEOUT_SECONDS`` +— the same call the real locker's heat-skip check makes, so the window shows +what the lock actually sees. That fetch is display-only and never writes. -* "Check Phone" submits ``PhoneVerificationMixin._verify_phone_workout`` to a - background thread — mirroring the existing submit/poll/``Future`` idiom - used by ``_ui_flows_relaxed.py``. -* The live Warsaw temperature (see ``_section_temperature``) is fetched via - ``_temperature.fetch_current_temp_with_status`` on open and on every - refresh, in its own background thread, bounded to - ``_temperature.HARD_TIMEOUT_SECONDS`` — the same call the real locker's - heat-skip check makes, so the window shows what the lock actually sees. +Two user-initiated buttons *do* write to ``workout_log.json`` (never a silent +log): -The one exception that *does* write is "Log Manual Workout": a -user-initiated, explicit evidence-form submission (see -``_manual_workout_dialog.ManualWorkoutDialogMixin``) — never a silent log. +* "Check Phone" runs ``PhoneVerificationMixin._verify_phone_workout`` and, when + that finds nothing, ``RunnerUpVerificationMixin._verify_runnerup_workout`` as + a fallback — the same StrongLifts→RunnerUp chain the locked screen uses. On a + verified workout it writes the entry and applies the shutdown reward via + ``_apply_workout_credit`` (guarded against double-crediting on the same day), + mirroring the submit/poll/``Future`` idiom of ``_ui_flows.py``. +* "Log Manual Workout" is a user-initiated, explicit evidence-form submission + (see ``_manual_workout_dialog.ManualWorkoutDialogMixin``). """ from __future__ import annotations -from concurrent.futures import ThreadPoolExecutor # pylint: disable=no-name-in-module from pathlib import Path import sys import tkinter as tk @@ -38,14 +40,12 @@ from screen_locker._status_data import ( format_summary_line, gather_status, ) +from screen_locker._status_view_verify import PhoneCheckMixin, _make_bare_verifier from screen_locker._temperature import ( fetch_current_temp_with_status, ) from screen_locker._temperature_status_mixin import TemperatureStatusMixin from screen_locker._ui_widgets import UIWidgetsMixin -from screen_locker._weekly_check import ( - WEEKLY_WORKOUT_MINIMUM as _WEEKLY_WORKOUT_MINIMUM, -) if TYPE_CHECKING: from collections.abc import Callable @@ -58,22 +58,12 @@ if TYPE_CHECKING: _DEFAULT_LOG_FILE = Path(__file__).resolve().parent / "workout_log.json" -def _make_bare_verifier(log_file: Path) -> ScreenLocker: - """Build a minimal ``ScreenLocker`` for read-only verification calls. - - Same ``object.__new__`` bypass ``screen_lock.py`` already uses for - ``--status`` — just enough state for ``PhoneVerificationMixin`` methods - to run, no Tk lock UI, no ``__init__`` side effects. - """ - from screen_locker.screen_lock import ScreenLocker - - verifier = object.__new__(ScreenLocker) - verifier.log_file = log_file - verifier.workout_data = {} - return verifier - - -class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMixin): +class StatusWindow( + UIWidgetsMixin, + ManualWorkoutDialogMixin, + TemperatureStatusMixin, + PhoneCheckMixin, +): """Thin Tk view over a :class:`StatusSnapshot`. Formatting logic lives in ``_status_data``, not here. @@ -98,9 +88,11 @@ class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMi self.verifier_factory = verifier_factory self.temperature_fetcher = temperature_fetcher self.demo_mode = True # only affects UIWidgetsMixin._button's cursor - self._phone_future: Future[tuple[str, str]] | None = None + self._phone_future: Future[tuple[str | None, str, str, str | None]] | None = ( + None + ) self._phone_check_result: tuple[str, str] | None = None - self._manual_workout_saved_message: str | None = None + self._credit_message: str | None = None self._temp_future: Future[TemperatureCheck] | None = None self._temp_result: TemperatureCheck | None = None self._last_snapshot = snapshot @@ -127,10 +119,8 @@ class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMi status, message = self._phone_check_result color = "#00cc44" if status == "verified" else "#ff8844" self._text(f"Phone check ({status}): {message}", font_size=13, color=color) - if self._manual_workout_saved_message is not None: - self._text( - self._manual_workout_saved_message, font_size=13, color="#00cc44" - ) + if self._credit_message is not None: + self._text(self._credit_message, font_size=13, color="#00cc44") frame = self._button_row() self._button( frame, @@ -158,7 +148,7 @@ class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMi """Render today's outcome.""" del parent mark = "✓" if day.counted else ("😷" if day.is_sick_day else "—") - entry_str = day.entry_type or ( + entry_str = ", ".join(day.entry_types) or ( "sick day" if day.is_sick_day else "no entry yet" ) self._text( @@ -177,7 +167,7 @@ class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMi ) for day in week.days: mark = "✓" if day.counted else ("😷" if day.is_sick_day else "·") - entry_str = day.entry_type or ( + entry_str = ", ".join(day.entry_types) or ( "sick day" if day.is_sick_day else "no entry" ) self._text( @@ -265,34 +255,13 @@ class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMi self._text(shutdown.explanation, font_size=10, color="#666666") def _on_refresh_clicked(self) -> None: - """Clear any stale phone-check/temperature results, re-check both.""" + """Clear any stale phone-check/credit/temperature results, re-check both.""" self._phone_check_result = None + self._credit_message = None self._temp_result = None self._start_temperature_check() self.on_refresh() - def _on_check_phone_clicked(self) -> None: - """Submit a background phone-verification check and start polling it.""" - self._phone_check_result = None - verifier = self.verifier_factory(self.log_file) - executor = ThreadPoolExecutor(max_workers=1) - self._phone_future = executor.submit(verifier._verify_phone_workout) - executor.shutdown(wait=False) - self._poll_phone_check() - - def _poll_phone_check(self) -> None: - """Poll the background phone-check future until it resolves.""" - if self._phone_future is not None and self._phone_future.done(): - status, message = self._phone_future.result() - self._on_phone_check_result(status, message) - else: - self.root.after(500, self._poll_phone_check) - - def _on_phone_check_result(self, status: str, message: str) -> None: - """Display the phone-check result. Never logs — display only.""" - self._phone_check_result = (status, message) - self.render(gather_status()) - def _on_manual_workout_saved(self, entry: dict) -> None: """Persist the manual-workout entry via a bare verifier, then refresh. @@ -307,22 +276,11 @@ class StatusWindow(UIWidgetsMixin, ManualWorkoutDialogMixin, TemperatureStatusMi verifier = self.verifier_factory(self.log_file) verifier.workout_data = entry credit = verifier._apply_workout_credit() - lines = [f"Manual workout logged: {entry.get('source', '')}"] - if credit.already_counted_today: - lines.append( - "(Today already had a counted workout — no extra credit applied.)" + self._credit_message = "\n".join( + self._credit_result_lines( + f"Manual workout logged: {entry.get('source', '')}", credit ) - else: - if credit.shutdown_adjusted: - lines.append("Shutdown time +2h later!") - if credit.extra_bonus_delta > 0: - extra_n = credit.weekly_count - _WEEKLY_WORKOUT_MINIMUM - lines.append( - f"Extra workout #{extra_n}! +{credit.extra_bonus_delta}h tonight" - ) - if credit.new_debt is not None: - lines.append(f"Workout debt: {credit.new_debt}") - self._manual_workout_saved_message = "\n".join(lines) + ) self.render(gather_status()) def _on_manual_workout_cancelled(self) -> None: diff --git a/screen_locker/tests/_status_view_helpers.py b/screen_locker/tests/_status_view_helpers.py index 8356ee2..514dc6b 100644 --- a/screen_locker/tests/_status_view_helpers.py +++ b/screen_locker/tests/_status_view_helpers.py @@ -33,17 +33,19 @@ def _day( *, date: str = "2024-01-05", label: str = "Fri Jan 05", - entry_type: str | None = None, + entry_types: tuple[str, ...] = (), source: str = "", counted: bool = False, + day_count: int = 0, is_sick_day: bool = False, ) -> DayStatus: return DayStatus( date=date, label=label, - entry_type=entry_type, + entry_types=entry_types, source=source, counted=counted, + day_count=day_count, is_sick_day=is_sick_day, ) diff --git a/screen_locker/tests/test_early_bird_part2.py b/screen_locker/tests/test_early_bird_part2.py index c442217..bab02e5 100644 --- a/screen_locker/tests/test_early_bird_part2.py +++ b/screen_locker/tests/test_early_bird_part2.py @@ -50,8 +50,8 @@ class TestTryAutoUpgradeEarlyBird: with log_file.open() as f: data: dict[str, Any] = json.load(f) today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") - assert data[today]["workout_data"]["type"] == "phone_verified" - assert data[today]["workout_data"]["after_early_bird"] == "true" + assert data[today][0]["workout_data"]["type"] == "phone_verified" + assert data[today][0]["workout_data"]["after_early_bird"] == "true" def test_upgrade_fails_when_not_verified( self, diff --git a/screen_locker/tests/test_init_and_log.py b/screen_locker/tests/test_init_and_log.py index 3a6fa39..ea70a06 100644 --- a/screen_locker/tests/test_init_and_log.py +++ b/screen_locker/tests/test_init_and_log.py @@ -248,8 +248,9 @@ class TestSaveWorkoutLog: data: dict[str, Any] = json.load(f) today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") assert today in data - assert data[today]["workout_data"]["type"] == "running" - assert data[today]["hmac"] == "abc123" + # The log now holds a list of entries per day (multiple workouts/day). + assert data[today][0]["workout_data"]["type"] == "running" + assert data[today][0]["hmac"] == "abc123" def test_save_to_new_file_no_hmac_key( self, @@ -271,7 +272,7 @@ class TestSaveWorkoutLog: with log_file.open() as f: data: dict[str, Any] = json.load(f) today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") - assert "hmac" not in data[today] + assert "hmac" not in data[today][0] def test_save_to_existing_file( self, diff --git a/screen_locker/tests/test_init_and_log_part2.py b/screen_locker/tests/test_init_and_log_part2.py index 8ad324d..d4a6ee5 100644 --- a/screen_locker/tests/test_init_and_log_part2.py +++ b/screen_locker/tests/test_init_and_log_part2.py @@ -69,8 +69,12 @@ class TestAutoUpgradeSickDay: today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") with log_file.open() as f: data: dict[str, Any] = json.load(f) - assert data[today]["workout_data"]["type"] == "phone_verified" - assert data[today]["workout_data"]["after_sick_day"] == "true" + # The log is day-keyed with a LIST of workouts; the upgrade appends one. + assert len(data[today]) == 1 + entry = data[today][0] + assert entry["workout_data"]["type"] == "phone_verified" + assert entry["workout_data"]["after_sick_day"] == "true" + assert entry["workout_id"] == f"phone_verified:{today}" def test_upgrade_skipped_when_not_verified( self, diff --git a/screen_locker/tests/test_log_io.py b/screen_locker/tests/test_log_io.py new file mode 100644 index 0000000..7f45767 --- /dev/null +++ b/screen_locker/tests/test_log_io.py @@ -0,0 +1,91 @@ +"""Tests for screen_locker._log_io — the workout-log normalization layer.""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING +from unittest.mock import patch + +from screen_locker._log_io import load_workout_log, read_raw_log + +if TYPE_CHECKING: + from pathlib import Path + + +class TestReadRawLog: + """Tests for read_raw_log (verbatim on-disk shape).""" + + def test_missing_file_returns_empty(self, tmp_path: Path) -> None: + """Non-existent file → {}.""" + assert read_raw_log(tmp_path / "nope.json") == {} + + def test_returns_contents_verbatim(self, tmp_path: Path) -> None: + """Values are returned unchanged — dict or list.""" + f = tmp_path / "log.json" + f.write_text(json.dumps({"2026-07-13": {"workout_data": {"type": "x"}}})) + assert read_raw_log(f) == {"2026-07-13": {"workout_data": {"type": "x"}}} + + def test_invalid_json_returns_empty(self, tmp_path: Path) -> None: + """Corrupt JSON → {}.""" + f = tmp_path / "log.json" + f.write_text("{not json}") + assert read_raw_log(f) == {} + + def test_oserror_returns_empty(self, tmp_path: Path) -> None: + """OSError on open → {}.""" + f = tmp_path / "log.json" + f.write_text("{}") + with patch("builtins.open", side_effect=OSError("perm")): + assert read_raw_log(f) == {} + + def test_non_dict_top_level_returns_empty(self, tmp_path: Path) -> None: + """A JSON list/scalar at the top level is not a log → {}.""" + f = tmp_path / "log.json" + f.write_text(json.dumps([1, 2, 3])) + assert read_raw_log(f) == {} + + +class TestLoadWorkoutLog: + """Tests for load_workout_log (normalizes both shapes to per-day lists).""" + + def test_missing_file_returns_empty(self, tmp_path: Path) -> None: + """Non-existent file → {}.""" + assert load_workout_log(tmp_path / "nope.json") == {} + + def test_legacy_dict_value_is_wrapped_in_a_list(self, tmp_path: Path) -> None: + """The old one-entry-per-day shape reads as a one-element list.""" + entry = {"timestamp": "t", "workout_data": {"type": "runnerup_verified"}} + f = tmp_path / "log.json" + f.write_text(json.dumps({"2026-07-12": entry})) + assert load_workout_log(f) == {"2026-07-12": [entry]} + + def test_list_value_is_passed_through(self, tmp_path: Path) -> None: + """The new multi-entry shape is returned as-is, order preserved.""" + a = {"workout_data": {"type": "manual_workout"}} + b = {"workout_data": {"type": "runnerup_verified"}} + f = tmp_path / "log.json" + f.write_text(json.dumps({"2026-07-13": [a, b]})) + assert load_workout_log(f) == {"2026-07-13": [a, b]} + + def test_malformed_list_elements_are_dropped(self, tmp_path: Path) -> None: + """Non-dict entries inside a day's list are ignored.""" + good = {"workout_data": {"type": "phone_verified"}} + f = tmp_path / "log.json" + f.write_text(json.dumps({"2026-07-13": [good, "junk", 7, None]})) + assert load_workout_log(f) == {"2026-07-13": [good]} + + def test_malformed_day_value_is_skipped(self, tmp_path: Path) -> None: + """A day whose value is neither dict nor list is omitted entirely.""" + good = {"workout_data": {"type": "phone_verified"}} + f = tmp_path / "log.json" + f.write_text(json.dumps({"2026-07-13": good, "2026-07-14": "junk"})) + assert load_workout_log(f) == {"2026-07-13": [good]} + + def test_mixed_legacy_and_new_shapes(self, tmp_path: Path) -> None: + """A half-migrated file reads correctly — both shapes coexist.""" + legacy = {"workout_data": {"type": "runnerup_verified"}} + a = {"workout_data": {"type": "manual_workout"}} + b = {"workout_data": {"type": "runnerup_verified"}} + f = tmp_path / "log.json" + f.write_text(json.dumps({"2026-07-12": legacy, "2026-07-13": [a, b]})) + assert load_workout_log(f) == {"2026-07-12": [legacy], "2026-07-13": [a, b]} diff --git a/screen_locker/tests/test_log_mixin_chokepoint.py b/screen_locker/tests/test_log_mixin_chokepoint.py new file mode 100644 index 0000000..9bd3ceb --- /dev/null +++ b/screen_locker/tests/test_log_mixin_chokepoint.py @@ -0,0 +1,124 @@ +"""Tests for the workout-log write chokepoint (append + workout_id dedup).""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + +from screen_locker._log_io import load_workout_log +from screen_locker._log_mixin import write_signed_entry + +if TYPE_CHECKING: + from pathlib import Path + + +class TestWriteChokepoint: + """Tests for _derive_workout_id / write_signed_entry append+dedup.""" + + def test_explicit_workout_id_wins(self, tmp_path: Path) -> None: + """An explicit workout_id in workout_data is used verbatim.""" + log_file = tmp_path / "workout_log.json" + result = write_signed_entry( + log_file, + "2026-07-13", + {"type": "runnerup_verified", "workout_id": "custom:abc"}, + ) + assert result.appended is True + assert load_workout_log(log_file)["2026-07-13"][0]["workout_id"] == "custom:abc" + + def test_manual_without_start_time_falls_back_to_type_date( + self, tmp_path: Path + ) -> None: + """A manual entry lacking start_time keys on ``{type}:{date}``.""" + log_file = tmp_path / "workout_log.json" + write_signed_entry(log_file, "2026-07-13", {"type": "manual_workout"}) + entry = load_workout_log(log_file)["2026-07-13"][0] + assert entry["workout_id"] == "manual_workout:2026-07-13" + + def test_manual_with_start_time_uses_the_sync_record_id( + self, tmp_path: Path + ) -> None: + """A manual entry reuses the sync record id so local+synced dedup.""" + log_file = tmp_path / "workout_log.json" + write_signed_entry( + log_file, + "2026-07-13", + {"type": "manual_workout", "start_time": "14:00"}, + ) + entry = load_workout_log(log_file)["2026-07-13"][0] + assert entry["workout_id"] == "manual:2026-07-13T14:00" + + def test_workout_data_without_type_gets_no_id(self, tmp_path: Path) -> None: + """No type → no derivable id; the entry still appends.""" + log_file = tmp_path / "workout_log.json" + assert write_signed_entry(log_file, "2026-07-13", {}).appended is True + assert "workout_id" not in load_workout_log(log_file)["2026-07-13"][0] + + def test_duplicate_workout_id_is_not_appended(self, tmp_path: Path) -> None: + """Re-writing the same workout is a no-op — idempotent by workout_id.""" + log_file = tmp_path / "workout_log.json" + data = {"type": "runnerup_verified", "source": "run"} + assert write_signed_entry(log_file, "2026-07-13", data).appended is True + + second = write_signed_entry(log_file, "2026-07-13", {**data, "source": "again"}) + assert second.appended is False + # prior_entries reports the day as it already stood, and nothing is added. + assert len(second.prior_entries) == 1 + assert len(load_workout_log(log_file)["2026-07-13"]) == 1 + + def test_legacy_entry_without_id_is_not_duplicated(self, tmp_path: Path) -> None: + """A pre-workout_id entry must match its own synced copy, not double. + + Regression: the PC began publishing its history, the phone/timer synced + a legacy manual workout back, and it appended a SECOND copy because the + stored id was None. The id is now derived for comparison. + """ + log_file = tmp_path / "workout_log.json" + legacy = { + "timestamp": "2026-07-13T07:25:12+00:00", + "workout_data": { + "type": "manual_workout", + "source": "squash", + "start_time": "14:00", + }, + } + log_file.write_text(json.dumps({"2026-07-13": [legacy]})) + + # The same workout arriving back from sync, now carrying its id. + result = write_signed_entry( + log_file, + "2026-07-13", + { + "type": "manual_workout", + "source": "squash", + "start_time": "14:00", + "workout_id": "manual:2026-07-13T14:00", + }, + ) + assert result.appended is False + assert len(load_workout_log(log_file)["2026-07-13"]) == 1 + + def test_malformed_existing_entry_does_not_block_the_append( + self, tmp_path: Path + ) -> None: + """An existing entry with junk workout_data has no id, so it can't match.""" + log_file = tmp_path / "workout_log.json" + log_file.write_text( + json.dumps({"2026-07-13": [{"timestamp": "t", "workout_data": "junk"}]}) + ) + result = write_signed_entry( + log_file, "2026-07-13", {"type": "runnerup_verified"} + ) + assert result.appended is True + assert len(load_workout_log(log_file)["2026-07-13"]) == 2 + + def test_distinct_workouts_same_day_both_append(self, tmp_path: Path) -> None: + """Different workout_ids on one day stack (multiple workouts/day).""" + log_file = tmp_path / "workout_log.json" + write_signed_entry(log_file, "2026-07-13", {"type": "manual_workout"}) + result = write_signed_entry( + log_file, "2026-07-13", {"type": "runnerup_verified"} + ) + assert result.appended is True + assert len(result.prior_entries) == 1 + assert len(load_workout_log(log_file)["2026-07-13"]) == 2 diff --git a/screen_locker/tests/test_manual_push.py b/screen_locker/tests/test_manual_push.py index 2235f61..078a9e3 100644 --- a/screen_locker/tests/test_manual_push.py +++ b/screen_locker/tests/test_manual_push.py @@ -1,131 +1,241 @@ -"""Tests for pushing PC-originated manual workouts to the sync repo.""" +"""Tests for pushing the PC's workouts (from workout_log.json) to the sync repo.""" # pylint: disable=protected-access from __future__ import annotations +from datetime import datetime, timezone +import json from typing import TYPE_CHECKING from unittest.mock import MagicMock, patch -from crdt_sync import GitHubSyncError, Hlc, Record +from crdt_sync import GitHubSyncError -from screen_locker import _manual_push, _workout_sync +from screen_locker import _manual_push from screen_locker._manual_push import ( + PushResult, _decode_log, _encode_log, - _load_store, - _max_hlc, - _store_path, - push_pc_manuals, - record_pc_manual, + _entry_wall_ms, + push_pc_workouts, + records_from_workout_log, ) if TYPE_CHECKING: from pathlib import Path -_ENTRY = { +_MANUAL = { "type": "manual_workout", "source": "table tennis at Solec", - "sport": "table_tennis", "start_time": "18:00", - "end_time": "19:30", - "location_name": "Solec", "rpe": 6, } +_RUN = { + "type": "runnerup_verified", + "source": "Running: 9.8 km in 55 min", + "distance_km": 9.8, +} -def _hlc(ms: int) -> Hlc: - return Hlc(wall_time_ms=ms, counter=0, node_id="pc") +def _write_log(log_file: Path, data: dict) -> None: + log_file.write_text(json.dumps(data)) -def _record(record_id: str, ms: int) -> Record: - return Record(id=record_id, fields={"payload": ({"x": 1}, _hlc(ms))}) +def _entry(workout_data: dict, ts: str, workout_id: str | None = None) -> dict: + entry: dict = {"timestamp": ts, "workout_data": workout_data} + if workout_id is not None: + entry["workout_id"] = workout_id + return entry -class TestEncodeDecodeRoundTrip: - def test_round_trips_a_log(self) -> None: - log = {"manual:a": _record("manual:a", 100)} - assert _decode_log(_encode_log(log)) == log +class TestEncodeDecodeLog: + """The crdt log blob round-trips.""" - -class TestLoadStore: - def test_missing_file_returns_empty(self, tmp_path: Path) -> None: - assert _load_store(tmp_path / "nope.json") == {} - - def test_corrupt_file_returns_empty(self, tmp_path: Path) -> None: - store = tmp_path / "manual_sync_log.json" - store.write_text("{not json") - assert _load_store(store) == {} - - def test_valid_file_round_trips(self, tmp_path: Path) -> None: - store = tmp_path / "manual_sync_log.json" - log = {"manual:a": _record("manual:a", 100)} - store.write_text(_encode_log(log)) - assert _load_store(store) == log - - -class TestMaxHlc: - def test_empty_returns_none(self) -> None: - assert _max_hlc({}) is None - - def test_returns_greatest_hlc(self) -> None: - log = {"a": _record("a", 100), "b": _record("b", 300)} - assert _max_hlc(log) == _hlc(300) - - def test_skips_records_without_a_payload_field(self) -> None: - no_payload = Record(id="a", fields={"other": (1, _hlc(1))}) - assert _max_hlc({"a": no_payload}) is None - - -class TestRecordPcManual: - def test_writes_a_manual_record(self, tmp_path: Path) -> None: + def test_round_trips(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" - record_id = record_pc_manual(log_file, _ENTRY) - assert record_id.startswith("manual:") - assert record_id.endswith("T18:00") - store = _load_store(_store_path(log_file)) - payload, _clock = store[record_id].fields["payload"] - assert payload["kind"] == "manual_workout" - assert "date" in payload + _write_log( + log_file, {"2026-07-13": [_entry(_RUN, "2026-07-13T10:00:00+00:00")]} + ) + log = records_from_workout_log(log_file) + assert _decode_log(_encode_log(log)).keys() == log.keys() - def test_second_record_has_a_greater_clock(self, tmp_path: Path) -> None: + +class TestEntryWallMs: + """The HLC clock is derived from the entry's own timestamp — stably.""" + + def test_uses_the_entry_timestamp(self) -> None: + expected = int( + datetime(2026, 7, 13, 12, tzinfo=timezone.utc).timestamp() * 1000 + ) + ms = _entry_wall_ms({"timestamp": "2026-07-13T12:00:00+00:00"}, "2026-07-13") + assert ms == expected + + def test_unparsable_timestamp_falls_back_to_midnight(self) -> None: + ms = _entry_wall_ms({"timestamp": "not-a-time"}, "2026-07-13") + assert ms == _entry_wall_ms({"timestamp": "2026-07-13T00:00:00+00:00"}, "x") + + def test_missing_timestamp_falls_back_to_midnight(self) -> None: + expected = int(datetime(2026, 7, 13, tzinfo=timezone.utc).timestamp() * 1000) + assert _entry_wall_ms({}, "2026-07-13") == expected + + def test_fallback_is_stable_across_calls(self) -> None: + """The fallback must not be 'now' — that would churn the repo.""" + assert _entry_wall_ms({}, "2026-07-13") == _entry_wall_ms({}, "2026-07-13") + + +class TestRecordsFromWorkoutLog: + """workout_log.json is the single source of truth for what gets pushed.""" + + def test_derives_a_record_per_counted_workout(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" - record_pc_manual(log_file, {**_ENTRY, "start_time": "18:00"}) - record_pc_manual(log_file, {**_ENTRY, "start_time": "20:00"}) - store = _load_store(_store_path(log_file)) - clocks = sorted(rec.fields["payload"][1] for rec in store.values()) - assert clocks[0] < clocks[1] + _write_log( + log_file, + { + "2026-07-13": [ + _entry(_MANUAL, "2026-07-13T07:25:12+00:00"), + _entry( + _RUN, + "2026-07-13T23:10:00+00:00", + workout_id="runnerup_verified:2026-07-13", + ), + ] + }, + ) + log = records_from_workout_log(log_file) + assert set(log) == {"manual:2026-07-13T18:00", "runnerup_verified:2026-07-13"} - -class TestPushPcManuals: - def test_no_token_is_a_no_op(self, tmp_path: Path) -> None: + def test_payload_carries_kind_and_date(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" - record_pc_manual(log_file, _ENTRY) + _write_log( + log_file, {"2026-07-13": [_entry(_RUN, "2026-07-13T10:00:00+00:00")]} + ) + payload, _hlc = records_from_workout_log(log_file)[ + "runnerup_verified:2026-07-13" + ].fields["payload"] + assert payload["kind"] == "runnerup_verified" + assert payload["date"] == "2026-07-13" + assert payload["distance_km"] == 9.8 + + def test_legacy_entry_without_workout_id_still_publishes( + self, tmp_path: Path + ) -> None: + """Entries predating workout_id get the id they would have had.""" + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, {"2026-07-13": [_entry(_MANUAL, "2026-07-13T07:25+00:00")]} + ) + assert "manual:2026-07-13T18:00" in records_from_workout_log(log_file) + + def test_uncounted_types_are_not_pushed(self, tmp_path: Path) -> None: + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, + {"2026-07-13": [_entry({"type": "sick_day"}, "2026-07-13T07:00:00+00:00")]}, + ) + assert records_from_workout_log(log_file) == {} + + def test_non_dict_workout_data_is_skipped(self, tmp_path: Path) -> None: + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, + { + "2026-07-13": [ + {"timestamp": "2026-07-13T07:00:00+00:00", "workout_data": "x"} + ] + }, + ) + assert records_from_workout_log(log_file) == {} + + def test_entry_without_derivable_id_is_skipped(self, tmp_path: Path) -> None: + """A counted type with no id can't be deduped, so it isn't synced.""" + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, {"2026-07-13": [_entry(_MANUAL, "2026-07-13T07:00:00+00:00")]} + ) + with patch("screen_locker._manual_push._derive_workout_id", return_value=None): + assert records_from_workout_log(log_file) == {} + + def test_is_deterministic_across_calls(self, tmp_path: Path) -> None: + """Same log → identical records+HLCs, so a re-push is a no-op.""" + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, {"2026-07-13": [_entry(_RUN, "2026-07-13T10:00:00+00:00")]} + ) + assert _encode_log(records_from_workout_log(log_file)) == _encode_log( + records_from_workout_log(log_file) + ) + + +class TestPushPcWorkouts: + """Every outcome is reported — never a silent no-op.""" + + def test_no_token_reports_why(self, tmp_path: Path) -> None: + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, {"2026-07-13": [_entry(_RUN, "2026-07-13T10:00:00+00:00")]} + ) with patch.object(_manual_push, "read_sync_token", return_value=None): - push_pc_manuals(log_file) # no exception, no client + result = push_pc_workouts(log_file) + assert result == PushResult(pushed=False, record_count=0, reason=result.reason) + assert "no sync token" in result.reason - def test_empty_store_is_a_no_op(self, tmp_path: Path) -> None: - _workout_sync.SYNC_TOKEN_FILE.write_text("tok") + def test_empty_log_reports_why(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" - with patch.object(_manual_push, "GitHubSyncClient") as client_cls: - push_pc_manuals(log_file) - client_cls.assert_not_called() + _write_log(log_file, {}) + with patch.object(_manual_push, "read_sync_token", return_value="t"): + result = push_pc_workouts(log_file) + assert result.pushed is False + assert result.record_count == 0 + assert "no counted workouts" in result.reason - def test_pushes_when_token_and_store_present(self, tmp_path: Path) -> None: - _workout_sync.SYNC_TOKEN_FILE.write_text("tok") + def test_sync_error_reports_why(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" - record_pc_manual(log_file, _ENTRY) - client = MagicMock() - client.list_directory.return_value = [] - with patch.object(_manual_push, "GitHubSyncClient", return_value=client): - push_pc_manuals(log_file) - client.put_file_text.assert_called_once() + _write_log( + log_file, {"2026-07-13": [_entry(_RUN, "2026-07-13T10:00:00+00:00")]} + ) + with ( + patch.object(_manual_push, "read_sync_token", return_value="t"), + patch.object(_manual_push, "GitHubSyncClient", MagicMock()), + patch.object( + _manual_push, "sync_log", side_effect=GitHubSyncError("403 forbidden") + ), + ): + result = push_pc_workouts(log_file) + assert result.pushed is False + assert result.record_count == 1 + assert "403 forbidden" in result.reason - def test_swallows_a_sync_error(self, tmp_path: Path) -> None: - _workout_sync.SYNC_TOKEN_FILE.write_text("tok") + def test_successful_push_reports_count(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" - record_pc_manual(log_file, _ENTRY) - client = MagicMock() - client.list_directory.return_value = [] - client.put_file_text.side_effect = GitHubSyncError("403") - with patch.object(_manual_push, "GitHubSyncClient", return_value=client): - push_pc_manuals(log_file) # swallowed, no raise + _write_log( + log_file, + { + "2026-07-13": [ + _entry(_MANUAL, "2026-07-13T07:25:12+00:00"), + _entry(_RUN, "2026-07-13T23:10:00+00:00"), + ] + }, + ) + fake_sync = MagicMock() + with ( + patch.object(_manual_push, "read_sync_token", return_value="t"), + patch.object(_manual_push, "GitHubSyncClient", MagicMock()), + patch.object(_manual_push, "sync_log", fake_sync), + ): + result = push_pc_workouts(log_file) + assert result == PushResult(pushed=True, record_count=2, reason="pushed") + assert fake_sync.call_count == 1 + + def test_pushes_runs_not_just_manuals(self, tmp_path: Path) -> None: + """Regression: the phone must converge on the PC's runs too.""" + log_file = tmp_path / "workout_log.json" + _write_log( + log_file, {"2026-07-12": [_entry(_RUN, "2026-07-12T10:00:00+00:00")]} + ) + with ( + patch.object(_manual_push, "read_sync_token", return_value="t"), + patch.object(_manual_push, "GitHubSyncClient", MagicMock()), + patch.object(_manual_push, "sync_log", MagicMock()) as fake_sync, + ): + push_pc_workouts(log_file) + pushed = fake_sync.call_args.kwargs["local_log"] + assert "runnerup_verified:2026-07-12" in pushed diff --git a/screen_locker/tests/test_manual_sync.py b/screen_locker/tests/test_manual_sync.py index 4d4944b..8a3507a 100644 --- a/screen_locker/tests/test_manual_sync.py +++ b/screen_locker/tests/test_manual_sync.py @@ -99,7 +99,8 @@ class TestIngestManualRecords: ingested = ingest_manual_records(log_file, [record], today=_DATE) assert ingested == [record[0]] stored = json.loads(log_file.read_text()) - workout = stored[_DATE]["workout_data"] + # The log is now a list of entries per day. + workout = stored[_DATE][0]["workout_data"] assert workout["type"] == "manual_workout" assert workout["sync_record_id"] == record[0] assert count_weekly_workouts(log_file, today=_dt(_DATE)) == 1 @@ -124,7 +125,10 @@ class TestIngestManualRecords: del payload["date"] assert ingest_manual_records(log_file, [("m", payload)], today=_DATE) == [] - def test_does_not_clobber_an_existing_counted_day(self, tmp_path: Path) -> None: + def test_appends_alongside_an_existing_counted_day(self, tmp_path: Path) -> None: + """A day may hold several workouts: a synced manual is appended next to + an existing verified entry rather than being skipped (the old + one-per-day skip is gone).""" log_file = tmp_path / "workout_log.json" log_file.write_text( json.dumps( @@ -132,10 +136,12 @@ class TestIngestManualRecords: ) ) record = _manual_record(_TT_DRAFT) - assert ingest_manual_records(log_file, [record], today=_DATE) == [] - # The original phone_verified entry is untouched. + assert ingest_manual_records(log_file, [record], today=_DATE) == [record[0]] stored = json.loads(log_file.read_text()) - assert stored[_DATE]["workout_data"]["type"] == "phone_verified" + # The original phone_verified entry is untouched and the manual is added. + assert len(stored[_DATE]) == 2 + assert stored[_DATE][0]["workout_data"]["type"] == "phone_verified" + assert stored[_DATE][1]["workout_data"]["type"] == "manual_workout" def test_skips_a_malformed_payload(self, tmp_path: Path) -> None: log_file = tmp_path / "workout_log.json" @@ -163,39 +169,27 @@ class TestIngestManualRecords: with patch("screen_locker._log_mixin.compute_entry_hmac", return_value=None): ingest_manual_records(log_file, [record], today=_DATE) stored = json.loads(log_file.read_text()) - assert "hmac" not in stored[_DATE] + assert "hmac" not in stored[_DATE][0] class TestIngestHelpers: - def test_already_ingested_ignores_non_dict_entries(self) -> None: - logs = {"a": "not-a-dict", "b": {"workout_data": {"sync_record_id": "x"}}} + def test_already_ingested_finds_id_in_per_day_lists(self) -> None: + """_already_ingested takes normalized ``{date: [entry, ...]}`` and + scans every entry across all days for a matching sync record id.""" + logs = { + "a": [], + "b": [ + {"workout_data": {"type": "manual_workout"}}, + {"workout_data": {"sync_record_id": "x"}}, + ], + } assert _manual_sync._already_ingested(logs, "x") is True assert _manual_sync._already_ingested(logs, "y") is False - def test_date_has_counted_workout_variants(self) -> None: - logs = { - "d1": {"workout_data": {"type": "manual_workout"}}, - "d2": {"workout_data": {"type": "nope"}}, - "d3": "not-a-dict", - } - assert _manual_sync._date_has_counted_workout(logs, "d1") is True - assert _manual_sync._date_has_counted_workout(logs, "d2") is False - assert _manual_sync._date_has_counted_workout(logs, "d3") is False - assert _manual_sync._date_has_counted_workout(logs, "missing") is False - - -class TestReadLogs: - def test_missing_file_returns_empty(self, tmp_path: Path) -> None: - from screen_locker._log_mixin import _read_logs - - assert _read_logs(tmp_path / "nope.json") == {} - - def test_corrupt_file_returns_empty(self, tmp_path: Path) -> None: - from screen_locker._log_mixin import _read_logs - - log_file = tmp_path / "workout_log.json" - log_file.write_text("{not valid json") - assert _read_logs(log_file) == {} + def test_already_ingested_ignores_non_dict_workout_data(self) -> None: + """An entry whose workout_data isn't a dict is skipped, not matched.""" + logs = {"b": [{"workout_data": "not-a-dict"}]} + assert _manual_sync._already_ingested(logs, "x") is False def _dt(date: str) -> object: diff --git a/screen_locker/tests/test_manual_workout_features.py b/screen_locker/tests/test_manual_workout_features.py index 1aa1503..b9724ca 100644 --- a/screen_locker/tests/test_manual_workout_features.py +++ b/screen_locker/tests/test_manual_workout_features.py @@ -3,11 +3,11 @@ from __future__ import annotations -import json from typing import TYPE_CHECKING from unittest.mock import MagicMock, patch from screen_locker import _manual_workout, _sick_tracker +from screen_locker._log_mixin import RecordResult from screen_locker._sick_tracker import SickHistory from screen_locker.tests.conftest import create_locker @@ -77,69 +77,24 @@ class TestOnManualWorkoutCancelled: locker._start_phone_check.assert_called_once() -def _write_today_entry(log_file: Path, today: str, entry_type: str) -> None: - """Write a single workout_log.json entry for *today*.""" - log_file.write_text( - json.dumps( - { - today: { - "timestamp": f"{today}T08:00:00+00:00", - "workout_data": {"type": entry_type}, - } - } - ) - ) - - -class TestWasAlreadyCountedToday: - """Tests for ScreenLocker._was_already_counted_today.""" - - def test_false_when_no_log_file( - self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path - ) -> None: - locker = create_locker(mock_tk, tmp_path) - assert locker._was_already_counted_today() is False - - def test_false_when_todays_entry_missing( - self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path - ) -> None: - locker = create_locker(mock_tk, tmp_path) - locker.log_file.write_text( - json.dumps({"2020-01-01": {"workout_data": {"type": "phone_verified"}}}) - ) - assert locker._was_already_counted_today() is False - - def test_true_when_todays_entry_is_counted_type( - self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path - ) -> None: - locker = create_locker(mock_tk, tmp_path) - with patch( - "screen_locker._workout_credit.datetime", - ) as mock_dt: - mock_dt.now.return_value.strftime.return_value = "2026-07-05" - _write_today_entry(locker.log_file, "2026-07-05", "manual_workout") - assert locker._was_already_counted_today() is True - - def test_false_when_todays_entry_is_noncounted_type( - self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path - ) -> None: - locker = create_locker(mock_tk, tmp_path) - with patch("screen_locker._workout_credit.datetime") as mock_dt: - mock_dt.now.return_value.strftime.return_value = "2026-07-05" - _write_today_entry(locker.log_file, "2026-07-05", "sick_day") - assert locker._was_already_counted_today() is False - - class TestApplyWorkoutCredit: - """Tests for ScreenLocker._apply_workout_credit.""" + """Tests for ScreenLocker._apply_workout_credit under the multi-per-day rule. - def test_skips_credit_when_already_counted_today( + Credit is now driven by the :class:`RecordResult` the write chokepoint + returns (``appended`` + ``prior_entries``), not a separate + ``_was_already_counted_today`` probe (removed). + """ + + def test_skips_credit_when_duplicate_already_logged_today( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path ) -> None: + """save_workout_log reports a duplicate (appended=False) → no new credit.""" locker = create_locker(mock_tk, tmp_path) locker.workout_data = {"type": "manual_workout"} object.__setattr__( - locker, "_was_already_counted_today", MagicMock(return_value=True) + locker, + "save_workout_log", + MagicMock(return_value=RecordResult(appended=False, prior_entries=[])), ) object.__setattr__( locker, "_try_adjust_shutdown_for_workout", MagicMock(return_value=True) @@ -147,7 +102,6 @@ class TestApplyWorkoutCredit: object.__setattr__( locker, "_clear_debt_on_verified_workout", MagicMock(return_value=0) ) - object.__setattr__(locker, "save_workout_log", MagicMock()) result = locker._apply_workout_credit() @@ -157,15 +111,18 @@ class TestApplyWorkoutCredit: assert result.extra_bonus_delta == 0 locker._try_adjust_shutdown_for_workout.assert_not_called() locker._clear_debt_on_verified_workout.assert_not_called() - locker.save_workout_log.assert_called_once() # log entry still saved + locker.save_workout_log.assert_called_once() # log entry write attempted - def test_applies_credit_when_not_already_counted( + def test_first_workout_of_day_gets_base_shutdown_push( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path ) -> None: + """First counted workout of the day (empty prior_entries) → base +2h push.""" locker = create_locker(mock_tk, tmp_path) locker.workout_data = {"type": "manual_workout"} object.__setattr__( - locker, "_was_already_counted_today", MagicMock(return_value=False) + locker, + "save_workout_log", + MagicMock(return_value=RecordResult(appended=True, prior_entries=[])), ) object.__setattr__( locker, "_try_adjust_shutdown_for_workout", MagicMock(return_value=True) @@ -173,9 +130,6 @@ class TestApplyWorkoutCredit: object.__setattr__( locker, "_clear_debt_on_verified_workout", MagicMock(return_value=2) ) - object.__setattr__( - locker, "_read_shutdown_config", MagicMock(return_value=(21, 21, 5)) - ) result = locker._apply_workout_credit() @@ -183,21 +137,62 @@ class TestApplyWorkoutCredit: assert result.shutdown_adjusted is True assert result.new_debt == 2 assert result.extra_bonus_delta == 0 + locker._try_adjust_shutdown_for_workout.assert_called_once() - def test_extra_bonus_applied_above_weekly_minimum( + def test_additional_manual_workout_earns_no_shutdown_credit( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path ) -> None: + """A second manual workout on a day that already counted → no shutdown push. + + Anti-gaming: self-reports can't stack intra-day shutdown credit. The + prior day already holds a counted entry, and the new workout is manual + (not verified), so neither the base push nor the +1h extra applies. + """ locker = create_locker(mock_tk, tmp_path) locker.workout_data = {"type": "manual_workout"} + prior = [{"workout_data": {"type": "manual_workout"}}] object.__setattr__( - locker, "_was_already_counted_today", MagicMock(return_value=False) + locker, + "save_workout_log", + MagicMock(return_value=RecordResult(appended=True, prior_entries=prior)), ) object.__setattr__( - locker, "_try_adjust_shutdown_for_workout", MagicMock(return_value=False) + locker, "_try_adjust_shutdown_for_workout", MagicMock(return_value=True) + ) + object.__setattr__( + locker, "_adjust_shutdown_time_by", MagicMock(return_value=True) ) object.__setattr__( locker, "_clear_debt_on_verified_workout", MagicMock(return_value=None) ) + + result = locker._apply_workout_credit() + + assert result.shutdown_adjusted is False + assert result.extra_bonus_delta == 0 + locker._try_adjust_shutdown_for_workout.assert_not_called() + locker._adjust_shutdown_time_by.assert_not_called() + + def test_additional_verified_workout_earns_plus_one_hour( + self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path + ) -> None: + """An additional same-day VERIFIED workout earns the +1h extra bonus. + + prior_entries already holds a counted workout, so this isn't the first + of the day; because the new type is verified it takes the +1h branch, + and extra_bonus_delta reflects the shutdown-config hour delta. + """ + locker = create_locker(mock_tk, tmp_path) + locker.workout_data = {"type": "phone_verified"} + prior = [{"workout_data": {"type": "phone_verified"}}] + object.__setattr__( + locker, + "save_workout_log", + MagicMock(return_value=RecordResult(appended=True, prior_entries=prior)), + ) + object.__setattr__( + locker, "_try_adjust_shutdown_for_workout", MagicMock(return_value=True) + ) object.__setattr__( locker, "_read_shutdown_config", @@ -206,22 +201,25 @@ class TestApplyWorkoutCredit: object.__setattr__( locker, "_adjust_shutdown_time_by", MagicMock(return_value=True) ) + object.__setattr__( + locker, "_clear_debt_on_verified_workout", MagicMock(return_value=None) + ) with patch( "screen_locker._workout_credit.count_weekly_workouts", return_value=5 ): result = locker._apply_workout_credit() assert result.weekly_count == 5 + assert result.shutdown_adjusted is False assert result.extra_bonus_delta == 1 + locker._try_adjust_shutdown_for_workout.assert_not_called() + locker._adjust_shutdown_time_by.assert_called_once_with(1) def test_skips_save_for_sick_day_type( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path ) -> None: locker = create_locker(mock_tk, tmp_path) locker.workout_data = {"type": "sick_day"} - object.__setattr__( - locker, "_was_already_counted_today", MagicMock(return_value=False) - ) object.__setattr__( locker, "_try_adjust_shutdown_for_workout", MagicMock(return_value=False) ) @@ -230,6 +228,8 @@ class TestApplyWorkoutCredit: ) object.__setattr__(locker, "save_workout_log", MagicMock()) - locker._apply_workout_credit() + result = locker._apply_workout_credit() + assert result.already_counted_today is False + assert result.shutdown_adjusted is False locker.save_workout_log.assert_not_called() diff --git a/screen_locker/tests/test_manual_workout_status_view.py b/screen_locker/tests/test_manual_workout_status_view.py index 6d899cf..5eb8d7e 100644 --- a/screen_locker/tests/test_manual_workout_status_view.py +++ b/screen_locker/tests/test_manual_workout_status_view.py @@ -95,7 +95,7 @@ class TestOnManualWorkoutSaved: assert fake_verifier.workout_data == entry fake_verifier._apply_workout_credit.assert_called_once() - message = window._manual_workout_saved_message + message = window._credit_message assert message is not None assert "table tennis at Solec" in message assert "Shutdown time +2h later!" in message @@ -120,9 +120,9 @@ class TestOnManualWorkoutSaved: with patch("screen_locker.status_view.gather_status", return_value=_snapshot()): window._on_manual_workout_saved({"type": "manual_workout", "source": "x"}) - message = window._manual_workout_saved_message + message = window._credit_message assert message is not None - assert "Extra workout #1! +1h tonight" in message + assert "Extra workout today! +1h tonight" in message assert "Workout debt: 1" in message def test_already_counted_today_shows_no_extra_credit_note( @@ -145,9 +145,9 @@ class TestOnManualWorkoutSaved: with patch("screen_locker.status_view.gather_status", return_value=_snapshot()): window._on_manual_workout_saved({"type": "manual_workout", "source": "x"}) - message = window._manual_workout_saved_message + message = window._credit_message assert message is not None - assert "no extra credit applied" in message + assert "no additional credit" in message assert "Shutdown time" not in message @@ -170,7 +170,7 @@ class TestManualWorkoutSavedMessageDisplay: def test_shown_when_present(self, mock_tk: MagicMock, temp_log_file: Path) -> None: window = _make_window(mock_tk, _snapshot(), log_file=temp_log_file) - window._manual_workout_saved_message = "Manual workout logged: test" + window._credit_message = "Manual workout logged: test" window.render(_snapshot()) texts = [c.kwargs.get("text", "") for c in mock_tk.Label.call_args_list] assert any("Manual workout logged: test" in t for t in texts) diff --git a/screen_locker/tests/test_mcp.py b/screen_locker/tests/test_mcp.py index 46c8084..76904fc 100644 --- a/screen_locker/tests/test_mcp.py +++ b/screen_locker/tests/test_mcp.py @@ -47,9 +47,10 @@ def _snapshot() -> StatusSnapshot: day = DayStatus( date="2026-07-10", label="Fri Jul 10", - entry_type=None, + entry_types=(), source="", counted=False, + day_count=0, is_sick_day=False, ) return StatusSnapshot( diff --git a/screen_locker/tests/test_runnerup_verification_part3.py b/screen_locker/tests/test_runnerup_verification_part3.py index 422ec18..b98027a 100644 --- a/screen_locker/tests/test_runnerup_verification_part3.py +++ b/screen_locker/tests/test_runnerup_verification_part3.py @@ -6,6 +6,7 @@ import json from typing import TYPE_CHECKING, Any from unittest.mock import MagicMock, patch +from screen_locker._log_io import load_workout_log from screen_locker.tests.conftest import create_locker if TYPE_CHECKING: @@ -158,18 +159,18 @@ class TestScanAndFillWeekRunnerup: ) with patch( - "screen_locker._runnerup_verification.compute_entry_hmac", + "screen_locker._log_mixin.compute_entry_hmac", return_value="sig", ): result = locker._scan_and_fill_week_runnerup(log_file) assert result > 0 logs = json.loads(log_file.read_text()) - # At least one date should have been filled. + # At least one date should have been filled; each day holds a list. types = [ - v.get("workout_data", {}).get("type") - for v in logs.values() - if isinstance(v, dict) + entry.get("workout_data", {}).get("type") + for entries in logs.values() + for entry in entries ] assert "runnerup_verified" in types @@ -235,28 +236,60 @@ class TestScanAndFillWeekRunnerup: ) assert locker._scan_and_fill_week_runnerup(log_file) == 0 - def test_returns_zero_on_write_error( + def test_rescanning_an_already_filled_day_is_a_no_op( self, mock_tk: MagicMock, mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """OSError writing log after fill → returns 0 (lines 241-246).""" + """A day whose run is already logged isn't appended twice (dedup).""" locker = create_locker(mock_tk, tmp_path) + log_file = tmp_path / "log.json" + log_file.write_text("{}") - # Can't patch PosixPath.open (read-only slot), so wrap it in a - # tiny class that delegates reads but raises on writes. - real_log = tmp_path / "log.json" - real_log.write_text("{}") + object.__setattr__( + locker, + "_find_runnerup_exports_for_date", + MagicMock(return_value=["/sdcard/run.tcx"]), + ) + object.__setattr__( + locker, + "_pull_and_parse_tcx", + MagicMock( + return_value={"sport": 0, "duration_seconds": 2400, "distance_m": 6000} + ), + ) + object.__setattr__( + locker, + "_validate_runnerup_data", + MagicMock(return_value=("verified", "ok")), + ) - class _FailWrite: - def open(self, mode: str = "r", **kw): - if mode == "w": - msg = "disk full" - raise OSError(msg) - return real_log.open(mode, **kw) + with patch( + "screen_locker._log_mixin.compute_entry_hmac", + return_value="sig", + ): + first = locker._try_fill_runnerup_for_date("2026-07-13", log_file) + second = locker._try_fill_runnerup_for_date("2026-07-13", log_file) - fail_log = _FailWrite() + assert first is True + assert second is False + assert len(load_workout_log(log_file)["2026-07-13"]) == 1 + + def test_save_error_is_swallowed_not_raised( + self, + mock_tk: MagicMock, + mock_sys_exit: MagicMock, + tmp_path: Path, + ) -> None: + """An OSError saving the log only warns — the scan still completes. + + Persistence now lives in the write chokepoint (``write_signed_entry``), + which logs and swallows save errors rather than propagating them. + """ + locker = create_locker(mock_tk, tmp_path) + fail_log = tmp_path / "log.json" + fail_log.write_text("{}") object.__setattr__(locker, "_has_adb_device", MagicMock(return_value=True)) object.__setattr__( @@ -277,13 +310,20 @@ class TestScanAndFillWeekRunnerup: MagicMock(return_value=("verified", "ok")), ) - with patch( - "screen_locker._runnerup_verification.compute_entry_hmac", - return_value="sig", + with ( + patch( + "screen_locker._log_mixin.compute_entry_hmac", + return_value="sig", + ), + patch( + "screen_locker._log_mixin.json.dump", + side_effect=OSError("disk full"), + ), ): result = locker._scan_and_fill_week_runnerup(fail_log) - assert result == 0 + # The append still reports success; only the save failure is warned. + assert result >= 1 def test_handles_corrupt_log_file( self, @@ -334,16 +374,17 @@ class TestScanAndFillWeekRunnerup: ) with patch( - "screen_locker._runnerup_verification.compute_entry_hmac", + "screen_locker._log_mixin.compute_entry_hmac", return_value=None, ): result = locker._scan_and_fill_week_runnerup(log_file) assert result > 0 - # No "hmac" key when signature is None. + # No "hmac" key when signature is None; each day holds a list of entries. logs = json.loads(log_file.read_text()) - for entry in logs.values(): - assert "hmac" not in entry + for entries in logs.values(): + for entry in entries: + assert "hmac" not in entry # --------------------------------------------------------------------------- diff --git a/screen_locker/tests/test_screen_lock_coverage_part1.py b/screen_locker/tests/test_screen_lock_coverage_part1.py index f0a7228..a351096 100644 --- a/screen_locker/tests/test_screen_lock_coverage_part1.py +++ b/screen_locker/tests/test_screen_lock_coverage_part1.py @@ -243,8 +243,8 @@ class TestTryAutoUpgradeSickDayRunnerUp: assert result is True today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") data = json.loads(log_file.read_text()) - assert data[today]["workout_data"]["type"] == "runnerup_verified" - assert data[today]["workout_data"]["after_sick_day"] == "true" + assert data[today][0]["workout_data"]["type"] == "runnerup_verified" + assert data[today][0]["workout_data"]["after_sick_day"] == "true" def test_runnerup_not_verified_returns_false( self, @@ -320,8 +320,8 @@ class TestTryAutoUpgradeEarlyBirdRunnerUp: assert result is True today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") data = json.loads(log_file.read_text()) - assert data[today]["workout_data"]["type"] == "runnerup_verified" - assert data[today]["workout_data"]["after_early_bird"] == "true" + assert data[today][0]["workout_data"]["type"] == "runnerup_verified" + assert data[today][0]["workout_data"]["after_early_bird"] == "true" def test_runnerup_not_verified_returns_false( self, diff --git a/screen_locker/tests/test_screen_lock_coverage_part2.py b/screen_locker/tests/test_screen_lock_coverage_part2.py index 24db878..514ef09 100644 --- a/screen_locker/tests/test_screen_lock_coverage_part2.py +++ b/screen_locker/tests/test_screen_lock_coverage_part2.py @@ -2,6 +2,8 @@ from __future__ import annotations +from datetime import datetime, timezone +import json from typing import TYPE_CHECKING from unittest.mock import MagicMock, patch @@ -21,10 +23,32 @@ class TestUnlockScreenExtras: weekly_count: int = 5, streak: int = 0, adjust_ok: bool = True, + seed_today_type: str | None = None, ): - """Create a locker ready to call unlock_screen.""" + """Create a locker ready to call unlock_screen. + + ``seed_today_type`` pre-logs a counted workout for today under a + different ``workout_id``, so the unlock's own verified workout is an + ADDITIONAL same-day one — the case that now earns the +1h bonus. + """ log_file = tmp_path / "workout_log.json" - log_file.write_text("{}") + if seed_today_type is None: + log_file.write_text("{}") + else: + today = datetime.now(tz=timezone.utc).strftime("%Y-%m-%d") + log_file.write_text( + json.dumps( + { + today: [ + { + "timestamp": f"{today}T06:00:00+00:00", + "workout_data": {"type": seed_today_type}, + "workout_id": f"{seed_today_type}:{today}", + } + ] + } + ) + ) locker = create_locker(mock_tk, tmp_path) locker.log_file = log_file locker.workout_data = {"type": "phone_verified"} @@ -53,8 +77,10 @@ class TestUnlockScreenExtras: mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """weekly_count > 4 + adjust succeeds → extra_bonus_delta calculated (360-364).""" - locker = self._setup_unlock(mock_tk, tmp_path, weekly_count=5) + """An additional same-day verified workout earns the +1h bonus.""" + locker = self._setup_unlock( + mock_tk, tmp_path, weekly_count=5, seed_today_type="manual_workout" + ) with ( patch( @@ -77,8 +103,8 @@ class TestUnlockScreenExtras: mock_sys_exit: MagicMock, tmp_path: Path, ) -> None: - """extra_bonus_delta > 0 → _text called with 'Extra workout' (lines 375-376).""" - locker = self._setup_unlock(mock_tk, tmp_path) + """extra_bonus_delta > 0 → _text called with 'Extra workout'.""" + locker = self._setup_unlock(mock_tk, tmp_path, seed_today_type="manual_workout") # Simulate before=22, after=23 → delta=1 old_cfg = (22, 22, 5) @@ -107,6 +133,61 @@ class TestUnlockScreenExtras: assert any("Extra workout" in c for c in text_calls) + def test_no_extra_bonus_when_adjust_fails( + self, + mock_tk: MagicMock, + mock_sys_exit: MagicMock, + tmp_path: Path, + ) -> None: + """Additional verified workout but the +1h adjust fails → delta stays 0.""" + locker = self._setup_unlock( + mock_tk, + tmp_path, + seed_today_type="manual_workout", + adjust_ok=False, + ) + + text_calls: list[str] = [] + object.__setattr__(locker, "_text", lambda msg, **kw: text_calls.append(msg)) + + with ( + patch( + "screen_locker._workout_credit.count_weekly_workouts", + return_value=5, + ), + patch("screen_locker.screen_lock.current_streak", return_value=0), + patch("screen_locker._log_mixin.compute_entry_hmac", return_value=None), + ): + locker.unlock_screen() + + assert not any("Extra workout" in c for c in text_calls) + + def test_no_extra_bonus_when_new_config_unreadable( + self, + mock_tk: MagicMock, + mock_sys_exit: MagicMock, + tmp_path: Path, + ) -> None: + """Re-reading the shutdown config fails after the +1h → delta stays 0.""" + locker = self._setup_unlock(mock_tk, tmp_path, seed_today_type="manual_workout") + # old_cfg readable, new_cfg unreadable → no delta can be computed. + locker._read_shutdown_config.side_effect = [(22, 22, 5), None] + + text_calls: list[str] = [] + object.__setattr__(locker, "_text", lambda msg, **kw: text_calls.append(msg)) + + with ( + patch( + "screen_locker._workout_credit.count_weekly_workouts", + return_value=5, + ), + patch("screen_locker.screen_lock.current_streak", return_value=0), + patch("screen_locker._log_mixin.compute_entry_hmac", return_value=None), + ): + locker.unlock_screen() + + assert not any("Extra workout" in c for c in text_calls) + def test_streak_displayed_when_nonzero( self, mock_tk: MagicMock, diff --git a/screen_locker/tests/test_status.py b/screen_locker/tests/test_status.py index 1ca3eae..12f3937 100644 --- a/screen_locker/tests/test_status.py +++ b/screen_locker/tests/test_status.py @@ -7,44 +7,12 @@ from pathlib import Path from typing import TYPE_CHECKING from unittest.mock import MagicMock, patch -from screen_locker._status import _load_extra_benefits, _load_log, run_status +from screen_locker._status import _load_extra_benefits, run_status from screen_locker.tests.conftest import _make_locker if TYPE_CHECKING: import pytest -# --------------------------------------------------------------------------- -# _load_log helpers -# --------------------------------------------------------------------------- - - -class TestLoadLog: - """Tests for _load_log.""" - - def test_missing_file_returns_empty(self, tmp_path: Path) -> None: - """Non-existent file → {}.""" - assert _load_log(tmp_path / "nope.json") == {} - - def test_valid_json_returned(self, tmp_path: Path) -> None: - """Valid JSON file → contents.""" - f = tmp_path / "log.json" - f.write_text(json.dumps({"2026-06-01": {"x": 1}})) - assert _load_log(f) == {"2026-06-01": {"x": 1}} - - def test_invalid_json_returns_empty(self, tmp_path: Path) -> None: - """Corrupt JSON → {}.""" - f = tmp_path / "log.json" - f.write_text("{not json}") - assert _load_log(f) == {} - - def test_oserror_returns_empty(self, tmp_path: Path) -> None: - """OSError on open → {}.""" - f = tmp_path / "log.json" - f.write_text("{}") - with patch("builtins.open", side_effect=OSError("perm")): - assert _load_log(f) == {} - - # --------------------------------------------------------------------------- # _load_extra_benefits helpers # --------------------------------------------------------------------------- diff --git a/screen_locker/tests/test_status_data.py b/screen_locker/tests/test_status_data.py index 3f7be64..0585f56 100644 --- a/screen_locker/tests/test_status_data.py +++ b/screen_locker/tests/test_status_data.py @@ -41,7 +41,8 @@ class TestGatherStatus: snap = gather_status(**_files(tmp_path), now=_FRIDAY_NOON_UTC) assert snap.today.date == "2024-01-05" - assert snap.today.entry_type is None + assert snap.today.entry_types == () + assert snap.today.day_count == 0 assert snap.today.counted is False assert snap.today.is_sick_day is False assert snap.week.counted_count == 0 @@ -88,7 +89,8 @@ class TestGatherStatus: snap = gather_status(**files, now=_FRIDAY_NOON_UTC) assert snap.week.counted_count == 2 - assert snap.today.entry_type == "phone_verified" + assert snap.today.entry_types == ("phone_verified",) + assert snap.today.day_count == 1 assert snap.today.source == "gym" assert snap.today.counted is True assert snap.lock_explanation.fired is False @@ -255,7 +257,7 @@ class TestGatherStatus: snap = gather_status(**files, now=_FRIDAY_NOON_UTC) assert snap.week.counted_count == 0 - assert snap.today.entry_type is None + assert snap.today.entry_types == () def test_now_defaults_to_current_time_when_omitted(self, tmp_path: Path) -> None: """Covers the ``now is None`` branch — just needs to not raise.""" @@ -270,25 +272,50 @@ class TestGatherStatus: class TestDayStatus: - """Direct tests for the _day_status helper's branch on non-dict entries.""" + """Direct tests for the _day_status helper on a day's entry list.""" - def test_entry_none_not_sick(self) -> None: - day = _day_status(date(2024, 1, 5), None, set()) - assert day.entry_type is None + def test_no_entries_not_sick(self) -> None: + day = _day_status(date(2024, 1, 5), [], set()) + assert day.entry_types == () assert day.counted is False + assert day.day_count == 0 assert day.is_sick_day is False - def test_entry_none_but_sick(self) -> None: - day = _day_status(date(2024, 1, 5), None, {"2024-01-05"}) + def test_no_entries_but_sick(self) -> None: + day = _day_status(date(2024, 1, 5), [], {"2024-01-05"}) assert day.is_sick_day is True - def test_entry_not_a_dict(self) -> None: - """Corrupt log data (non-dict entry) falls back to empty workout_data.""" - day = _day_status(date(2024, 1, 5), "corrupt-string-entry", set()) - assert day.entry_type is None + def test_non_dict_entries_dropped(self) -> None: + """Corrupt log data (non-dict entries in the list) are skipped.""" + day = _day_status(date(2024, 1, 5), ["corrupt-string-entry"], set()) + assert day.entry_types == () assert day.counted is False + assert day.day_count == 0 assert day.source == "" + def test_multiple_entries_count_verified_individually_manual_once(self) -> None: + """Two verified + two manual on one day → day_count 3, all types listed.""" + day = _day_status( + date(2024, 1, 5), + [ + {"workout_data": {"type": "runnerup_verified", "source": "run"}}, + {"workout_data": {"type": "phone_verified", "source": "gym"}}, + {"workout_data": {"type": "manual_workout", "source": "tt"}}, + {"workout_data": {"type": "manual_workout", "source": "squash"}}, + ], + set(), + ) + assert day.entry_types == ( + "runnerup_verified", + "phone_verified", + "manual_workout", + "manual_workout", + ) + assert day.counted is True + # 2 verified counted individually + all manual entries count once = 3. + assert day.day_count == 3 + assert day.source == "run · gym · tt · squash" + class TestFormatSummaryLine: """format_summary_line variants.""" diff --git a/screen_locker/tests/test_status_view.py b/screen_locker/tests/test_status_view.py index 4c317fb..20978ec 100644 --- a/screen_locker/tests/test_status_view.py +++ b/screen_locker/tests/test_status_view.py @@ -2,9 +2,10 @@ from __future__ import annotations -from typing import TYPE_CHECKING +from unittest.mock import MagicMock from screen_locker._compliance_state import AutoUpgradeOpportunity +from screen_locker._status_view_verify import _verify_phone_then_runnerup from screen_locker.tests._status_view_helpers import ( _day, _lock_explanation, @@ -16,14 +17,16 @@ from screen_locker.tests._status_view_helpers import ( _week, ) -if TYPE_CHECKING: - from unittest.mock import MagicMock - class TestSectionToday: def test_counted_shows_checkmark_and_entry(self, mock_tk: MagicMock) -> None: snap = _snapshot( - today=_day(entry_type="phone_verified", counted=True, source="gym") + today=_day( + entry_types=("phone_verified",), + counted=True, + day_count=1, + source="gym", + ) ) _make_window(mock_tk, snap) texts = _texts(mock_tk) @@ -31,18 +34,23 @@ class TestSectionToday: assert any(t == "gym" for t in texts) def test_sick_day_shows_sick_mark(self, mock_tk: MagicMock) -> None: - snap = _snapshot(today=_day(entry_type=None, is_sick_day=True)) + snap = _snapshot(today=_day(entry_types=(), is_sick_day=True)) _make_window(mock_tk, snap) assert any("😷" in t and "sick day" in t for t in _texts(mock_tk)) def test_no_entry_shows_dash(self, mock_tk: MagicMock) -> None: - snap = _snapshot(today=_day(entry_type=None, is_sick_day=False)) + snap = _snapshot(today=_day(entry_types=(), is_sick_day=False)) _make_window(mock_tk, snap) assert any("no entry yet" in t for t in _texts(mock_tk)) def test_empty_source_adds_no_extra_line(self, mock_tk: MagicMock) -> None: snap = _snapshot( - today=_day(entry_type="phone_verified", counted=True, source="") + today=_day( + entry_types=("phone_verified",), + counted=True, + day_count=1, + source="", + ) ) _make_window(mock_tk, snap) # Only the main "Today (...)" line, no second call for an empty source. @@ -76,7 +84,8 @@ class TestSectionWeek: date="2024-01-01", label="Mon Jan 01", counted=True, - entry_type="phone_verified", + day_count=1, + entry_types=("phone_verified",), ), _day(date="2024-01-02", label="Tue Jan 02", is_sick_day=True), _day(date="2024-01-03", label="Wed Jan 03"), @@ -168,3 +177,37 @@ class TestSectionShutdown: texts = _texts(mock_tk) assert any("Rest of week:" in t for t in texts) assert any("Next week (speculative):" in t for t in texts) + + +class TestVerifyPhoneThenRunnerup: + """The "Check Phone" worker: StrongLifts first, then RunnerUp as fallback.""" + + def test_stronglifts_verified_short_circuits(self) -> None: + """A verified phone workout wins and RunnerUp is never consulted.""" + verifier = MagicMock() + verifier._verify_phone_workout.return_value = ("verified", "5x5 done") + + result = _verify_phone_then_runnerup(verifier) + + assert result == ("phone_verified", "verified", "5x5 done", None) + verifier._verify_runnerup_workout.assert_not_called() + + def test_falls_back_to_runnerup_when_phone_not_verified(self) -> None: + """StrongLifts miss → a verified run credits as runnerup_verified.""" + verifier = MagicMock() + verifier._verify_phone_workout.return_value = ("not_verified", "stale") + verifier._verify_runnerup_workout.return_value = ("verified", "9.8 km") + + result = _verify_phone_then_runnerup(verifier) + + assert result == ("runnerup_verified", "verified", "stale", "9.8 km") + + def test_neither_source_verified(self) -> None: + """Neither verified → no credited type, both messages carried back.""" + verifier = MagicMock() + verifier._verify_phone_workout.return_value = ("not_verified", "stale") + verifier._verify_runnerup_workout.return_value = ("too_short", "3 km") + + result = _verify_phone_then_runnerup(verifier) + + assert result == (None, "too_short", "stale", "3 km") diff --git a/screen_locker/tests/test_status_view_part2.py b/screen_locker/tests/test_status_view_part2.py index dadefe6..6146656 100644 --- a/screen_locker/tests/test_status_view_part2.py +++ b/screen_locker/tests/test_status_view_part2.py @@ -5,6 +5,7 @@ from __future__ import annotations from typing import TYPE_CHECKING from unittest.mock import MagicMock, patch +from screen_locker._workout_credit import WorkoutCreditResult from screen_locker.status_view import ( StatusWindow, _compliance_state_word, @@ -127,16 +128,93 @@ class TestRefreshAndCheckPhoneFlow: window._poll_phone_check() mock_handle.assert_not_called() - def test_result_never_calls_save_workout_log(self, mock_tk: MagicMock) -> None: - """A manual peek must never silently log a workout (see module docstring).""" + def test_failed_check_shows_combined_message_and_rerenders( + self, mock_tk: MagicMock + ) -> None: + """Neither source verified → show both messages, apply no credit.""" window = _make_window(mock_tk, _snapshot()) with patch( - "screen_locker.status_view.gather_status", return_value=_snapshot() + "screen_locker._status_view_verify.gather_status", return_value=_snapshot() ) as mock_gather: - window._on_phone_check_result("verified", "Workout verified!") - assert window._phone_check_result == ("verified", "Workout verified!") + window._on_phone_check_result(None, "no_phone", "no lifts", "no run") + assert window._phone_check_result == ( + "no_phone", + "StrongLifts: no lifts · RunnerUp: no run", + ) + assert window._credit_message is None mock_gather.assert_called_once() + def test_phone_verified_applies_credit_with_phone_message( + self, mock_tk: MagicMock, temp_log_file: Path + ) -> None: + """A verified StrongLifts result writes and credits using the phone message.""" + fake_verifier = MagicMock() + fake_verifier._apply_workout_credit = MagicMock( + return_value=WorkoutCreditResult( + shutdown_adjusted=True, + new_debt=None, + extra_bonus_delta=0, + weekly_count=1, + already_counted_today=False, + ) + ) + window = _make_window( + mock_tk, + _snapshot(), + log_file=temp_log_file, + verifier_factory=lambda _log_file: fake_verifier, + ) + with patch( + "screen_locker._status_view_verify.gather_status", return_value=_snapshot() + ): + window._on_phone_check_result( + "phone_verified", "verified", "StrongLifts ok", None + ) + assert fake_verifier.workout_data == { + "type": "phone_verified", + "source": "StrongLifts ok", + } + fake_verifier._apply_workout_credit.assert_called_once() + assert window._phone_check_result is None + message = window._credit_message + assert message is not None + assert "StrongLifts verified: StrongLifts ok" in message + assert "Shutdown time +2h later!" in message + + def test_runnerup_verified_credits_using_runnerup_message( + self, mock_tk: MagicMock, temp_log_file: Path + ) -> None: + """A verified RunnerUp fallback credits using the RunnerUp message.""" + fake_verifier = MagicMock() + fake_verifier._apply_workout_credit = MagicMock( + return_value=WorkoutCreditResult( + shutdown_adjusted=False, + new_debt=None, + extra_bonus_delta=0, + weekly_count=1, + already_counted_today=False, + ) + ) + window = _make_window( + mock_tk, + _snapshot(), + log_file=temp_log_file, + verifier_factory=lambda _log_file: fake_verifier, + ) + with patch( + "screen_locker._status_view_verify.gather_status", return_value=_snapshot() + ): + window._on_phone_check_result( + "runnerup_verified", "verified", "no lifts", "RunnerUp 5.0 km" + ) + assert fake_verifier.workout_data == { + "type": "runnerup_verified", + "source": "RunnerUp 5.0 km", + } + message = window._credit_message + assert message is not None + assert "RunnerUp verified: RunnerUp 5.0 km" in message + class TestMakeBareVerifier: def test_builds_locker_without_running_init(self, tmp_path: Path) -> None: diff --git a/scripts/check_file_length.py b/scripts/check_file_length.py index 3d25130..cccbdcb 100755 --- a/scripts/check_file_length.py +++ b/scripts/check_file_length.py @@ -1,10 +1,13 @@ #!/usr/bin/env python3 """Pre-commit hook: fail if any file exceeds MAX_LINES lines.""" +import logging import sys MAX_LINES = 400 +_logger = logging.getLogger(__name__) + def main() -> int: """Return 1 if any file exceeds the line limit, else 0.""" @@ -13,7 +16,14 @@ def main() -> int: try: with open(filepath, encoding="utf-8", errors="replace") as fh: count = sum(1 for _ in fh) - except OSError: + except OSError as exc: + _logger.warning( + "Could not read %s to count its lines (%s) — failing the check " + "rather than assuming the file is within the %d-line limit", + filepath, + exc, + MAX_LINES, + ) failed = True continue if count > MAX_LINES: diff --git a/scripts/check_silent_failures.py b/scripts/check_silent_failures.py new file mode 100755 index 0000000..a25bd2a --- /dev/null +++ b/scripts/check_silent_failures.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +"""Pre-commit hook: ban silent failures. + +A caught exception that is swallowed (not re-raised) must say WHY in the log, +loudly. Otherwise the failure is invisible: the caller sees a bland ``None`` / +``[]`` / early return and cannot tell "nothing to do" from "it broke". That is +not hypothetical — the PC's workout sync silently did nothing for weeks because +a missing token returned ``[]`` and a real error only logged at ``INFO``. + +The rule: every ``except`` handler must either + +* re-``raise`` (the failure is someone else's problem), or +* log at ``warning`` / ``error`` / ``exception`` / ``critical`` with the reason. + +``debug`` / ``info`` do not count — an invisible log is a silent failure with +extra steps. ruff's ``S110`` only catches ``except: pass``; this catches the +far more common ``except: log.info(...); return []``. + +There is deliberately no escape hatch (per-line lint suppressions are banned +repo-wide too): if a failure is truly benign, say so at ``warning`` and move on. +""" + +from __future__ import annotations + +import ast +import logging +from pathlib import Path +import sys + +_logger = logging.getLogger(__name__) + +# Log levels loud enough that a human actually sees the failure. +_LOUD_LOG_METHODS = frozenset({"warning", "error", "exception", "critical"}) + + +def _is_loud_log_call(node: ast.AST) -> bool: + """Return True if ``node`` is a ``.warning(...)``-style call.""" + return ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr in _LOUD_LOG_METHODS + ) + + +def _handler_is_loud(handler: ast.ExceptHandler) -> bool: + """Return True if the handler re-raises or logs loudly about the failure.""" + return any( + isinstance(node, ast.Raise) or _is_loud_log_call(node) + for node in ast.walk(handler) + ) + + +def _handler_label(handler: ast.ExceptHandler) -> str: + """Return a readable name for what the handler catches.""" + if handler.type is None: + return "except:" + return f"except {ast.unparse(handler.type)}:" + + +def _check_source(path: Path, source: str) -> list[str]: + """Return one message per silently-swallowing except handler in ``source``.""" + try: + tree = ast.parse(source, filename=str(path)) + except SyntaxError as exc: + _logger.warning( + "Could not parse %s (%s at line %s) — reporting it as a problem " + "rather than skipping it, since its handlers cannot be checked", + path, + exc.msg, + exc.lineno, + ) + return [f"{path}:{exc.lineno}: could not parse ({exc.msg})"] + return [ + f"{path}:{node.lineno}: {_handler_label(node)} swallows the error without " + f"logging why — re-raise, or log at warning/error/exception/critical" + for node in ast.walk(tree) + if isinstance(node, ast.ExceptHandler) and not _handler_is_loud(node) + ] + + +def main() -> int: + """Return 1 if any file swallows an exception silently, else 0.""" + problems: list[str] = [] + for filename in sys.argv[1:]: + path = Path(filename) + try: + source = path.read_text(encoding="utf-8", errors="replace") + except OSError as exc: + _logger.warning( + "Could not read %s (%s) — reporting it as a problem rather than " + "skipping it, since its handlers cannot be checked", + path, + exc, + ) + problems.append(f"{path}: could not read ({exc})") + continue + problems.extend(_check_source(path, source)) + for problem in problems: + sys.stdout.write(f"{problem}\n") + return 1 if problems else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/stronglift_replacement/workout_app/lib/models/manual_workout.dart b/stronglift_replacement/workout_app/lib/models/manual_workout.dart index 8c7afb5..e3b0368 100644 --- a/stronglift_replacement/workout_app/lib/models/manual_workout.dart +++ b/stronglift_replacement/workout_app/lib/models/manual_workout.dart @@ -351,17 +351,20 @@ class ManualBudget { month >= kManualWorkoutBudgetPer30Days; } -/// Counts manual-workout [payloads] falling in the rolling 7-/30-day windows. +/// Counts DAYS with a manual workout in the rolling 7-/30-day windows. /// -/// Each payload's own `date` (YYYY-MM-DD) is the window key, so both devices -/// compute the same shared budget over the merged record set. +/// The budget is counted per DAY, not per workout: several self-reports on one +/// day consume a single slot (and only one counts toward the weekly total +/// anyway). Each payload's own `date` (YYYY-MM-DD) is the window key, so both +/// devices compute the same shared budget over the merged record set — this +/// mirrors the PC's `count_in_window` (screen_locker/_manual_workout.py). ManualBudget countManualBudget( Iterable> payloads, DateTime now, ) { final today = DateTime(now.year, now.month, now.day); - var week = 0; - var month = 0; + final weekDays = {}; + final monthDays = {}; for (final payload in payloads) { final dateStr = payload['date']; if (dateStr is! String) continue; @@ -371,8 +374,8 @@ ManualBudget countManualBudget( .difference(DateTime(date.year, date.month, date.day)) .inDays; if (days < 0) continue; - if (days < 7) week++; - if (days < 30) month++; + if (days < 7) weekDays.add(dateStr); + if (days < 30) monthDays.add(dateStr); } - return ManualBudget(week: week, month: month); + return ManualBudget(week: weekDays.length, month: monthDays.length); } diff --git a/stronglift_replacement/workout_app/lib/screens/history_screen.dart b/stronglift_replacement/workout_app/lib/screens/history_screen.dart index 05a2856..26d9396 100644 --- a/stronglift_replacement/workout_app/lib/screens/history_screen.dart +++ b/stronglift_replacement/workout_app/lib/screens/history_screen.dart @@ -9,8 +9,10 @@ import 'dart:async'; import 'dart:convert'; import 'dart:math'; import 'package:flutter/material.dart'; +import 'package:http/http.dart' as http; import 'package:workout_app/models/exercise.dart'; import 'package:workout_app/services/storage_service.dart'; +import 'package:workout_app/services/workout_sync_service.dart'; import 'package:workout_app/widgets/calendar_widget.dart'; const _kTotal = 'Total (all workouts)'; @@ -18,14 +20,29 @@ const _kTotal = 'Total (all workouts)'; /// Screen showing workout history with per-exercise drill-down and charts. class HistoryScreen extends StatefulWidget { /// Creates a [HistoryScreen]. - const HistoryScreen({super.key}); + /// + /// [httpClient] is injected only by tests, so the synced-workout fetch can + /// be driven without real network — same pattern as `SettingsScreen`. + const HistoryScreen({super.key, this.httpClient}); + + /// Overrides the HTTP client used to read synced workouts (tests only). + final http.Client? httpClient; @override State createState() => _HistoryScreenState(); } +/// Synced kinds this phone has no local record of, so showing them can't +/// double up an existing session. +/// +/// A `phone_verified` record is the PC's mirror of a StrongLifts session this +/// phone already stores in full (and renders from `_rows`), so it is +/// deliberately excluded — otherwise the same workout would appear twice. +const _kSyncedOnlyKinds = {'runnerup_verified', 'manual_workout'}; + class _HistoryScreenState extends State { List> _rows = []; + List> _syncedRows = []; bool _loading = true; String _selected = _kTotal; List _exerciseNames = []; @@ -58,9 +75,11 @@ class _HistoryScreenState extends State { state = await StorageService.instance.getExerciseState(_selected); // coverage:ignore-end } + final synced = await _loadSyncedWorkouts(); if (mounted) { setState(() { _rows = rows; + _syncedRows = synced; _exerciseNames = names; _selectedState = state; _loading = false; @@ -68,6 +87,22 @@ class _HistoryScreenState extends State { } } + /// Workouts the PC published that this phone has no local record of. + /// + /// The PC pushes its whole `workout_log.json` (RunnerUp runs and manual + /// entries included), so pulling them here is what makes both devices show + /// the SAME history. Sorted newest-first to match the local session list. + Future>> _loadSyncedWorkouts() async { + final payloads = await WorkoutSyncService( + httpClient: widget.httpClient, + ).readMergedWorkoutPayloads(); + final synced = payloads + .where((p) => _kSyncedOnlyKinds.contains(p['kind'])) + .toList() + ..sort((a, b) => '${b['date']}'.compareTo('${a['date']}')); + return synced; + } + Future _pickExercise(String name) async { ExerciseState? state; if (name != _kTotal) { @@ -83,9 +118,14 @@ class _HistoryScreenState extends State { // ── Data helpers ────────────────────────────────────────────────────────── - /// All workout dates (YYYY-MM-DD) across all sessions. - Set get _allWorkoutDates => - _rows.map((r) => r['date'] as String).toSet(); + /// All workout dates (YYYY-MM-DD): local sessions plus synced ones. + /// + /// Synced dates are included so a day you only worked out on the PC (a + /// RunnerUp run) still marks the calendar here. + Set get _allWorkoutDates => { + ..._rows.map((r) => r['date'] as String), + ..._syncedRows.map((r) => '${r['date']}'), + }; /// Dates when the selected exercise appeared. Set _exerciseDates(String name) { @@ -234,6 +274,12 @@ class _HistoryScreenState extends State { const _SectionLabel('ALL SESSIONS'), const SizedBox(height: 8), ..._rows.map((row) => _AllSessionTile(row: row)), + if (_syncedRows.isNotEmpty) ...[ + const SizedBox(height: 16), + const _SectionLabel('SYNCED FROM PC'), + const SizedBox(height: 8), + ..._syncedRows.map((row) => _SyncedWorkoutTile(payload: row)), + ], ]; List _buildExerciseView(String name) => [ @@ -576,6 +622,62 @@ class _ChartPainter extends CustomPainter { bool shouldRepaint(_ChartPainter old) => old.points != points; } +/// One workout the PC published that this phone has no local session for. +class _SyncedWorkoutTile extends StatelessWidget { + const _SyncedWorkoutTile({required this.payload}); + + final Map payload; + + @override + Widget build(BuildContext context) { + final kind = '${payload['kind']}'; + final isRun = kind == 'runnerup_verified'; + final label = isRun ? 'Run' : 'Manual'; + final detail = '${payload['source'] ?? ''}'; + + return Container( + margin: const EdgeInsets.only(bottom: 8), + padding: const EdgeInsets.symmetric(horizontal: 14, vertical: 10), + decoration: BoxDecoration( + color: Colors.grey.shade800, + borderRadius: BorderRadius.circular(8), + border: Border.all( + color: isRun ? Colors.blue.shade800 : Colors.orange.shade900, + ), + ), + child: Row( + children: [ + Icon( + isRun ? Icons.directions_run : Icons.edit_note, + color: isRun ? Colors.blue.shade300 : Colors.orange.shade300, + size: 20, + ), + const SizedBox(width: 10), + Expanded( + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + '${payload['date']} · $label', + style: const TextStyle( + color: Colors.white, + fontWeight: FontWeight.bold, + ), + ), + if (detail.isNotEmpty) + Text( + detail, + style: TextStyle(color: Colors.grey.shade400, fontSize: 12), + ), + ], + ), + ), + ], + ), + ); + } +} + class _AllSessionTile extends StatelessWidget { const _AllSessionTile({required this.row}); diff --git a/stronglift_replacement/workout_app/lib/screens/settings_screen.dart b/stronglift_replacement/workout_app/lib/screens/settings_screen.dart index ac65bd4..0946f5f 100644 --- a/stronglift_replacement/workout_app/lib/screens/settings_screen.dart +++ b/stronglift_replacement/workout_app/lib/screens/settings_screen.dart @@ -83,10 +83,12 @@ class _SettingsScreenState extends State { _tokenController.text = syncSettings.token; _loading = false; }); - // Reflect an already-configured token immediately on open, not only - // after Connect GitHub / Save is tapped this session. + // Never claim "Connected" just because a token STRING exists — check it + // against the API. A revoked/expired token otherwise shows a reassuring + // green badge while every sync 401s and the history silently stays empty. if (syncSettings.isConfigured) { - _setSyncStatus('Connected.', _SyncStatusKind.success); + _setSyncStatus('Verifying…', _SyncStatusKind.pending); + await _verifyConnection(syncSettings.token); } } } @@ -158,8 +160,14 @@ class _SettingsScreenState extends State { ); } on GitHubSyncError catch (e) { if (!mounted) return; + // Say plainly that sync is broken. "Connected, but…" reads as success + // and is how a dead token hid behind a green badge. + final rejected = e.toString().contains('401'); _setSyncStatus( - 'Connected, but could not verify: $e', + rejected + ? 'NOT connected: GitHub rejected this token (401). Tap Connect ' + 'GitHub to re-authorize — until then nothing syncs.' + : 'NOT syncing: could not reach GitHub ($e)', _SyncStatusKind.error, ); } finally { diff --git a/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart b/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart index f10f94a..818a3a8 100644 --- a/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart +++ b/stronglift_replacement/workout_app/lib/services/workout_sync_service.dart @@ -126,9 +126,27 @@ class WorkoutSyncService { /// (highest HLC wins), for computing the shared budget. Pull-only — unlike /// [pushManual] it never writes, so showing the budget can't mutate the repo. /// Returns an empty list if sync isn't configured or the repo is unreachable. - Future>> readMergedManualPayloads() async { + Future>> readMergedManualPayloads() => + _readMergedPayloads(kind: kManualWorkoutSyncKind); + + /// Every synced workout, whatever kind — manual, StrongLifts or RunnerUp. + /// + /// The PC publishes its whole `workout_log.json` (including verified runs), + /// so this is what the history view needs to show the SAME workouts both + /// devices know about. Deliberately unfiltered: the manual-workout budget + /// uses [readMergedManualPayloads] instead. + Future>> readMergedWorkoutPayloads() => + _readMergedPayloads(); + + Future>> _readMergedPayloads({String? kind}) async { final settings = await SyncSettings.load(); - if (!settings.isConfigured) return const []; + if (!settings.isConfigured) { + debugPrint( + 'WorkoutSyncService: NOT reading synced workouts — no sync token/repo ' + 'configured in Settings, so this phone cannot see the PC history.', + ); + return const []; + } final client = GitHubClient( owner: owner, @@ -141,28 +159,36 @@ class WorkoutSyncService { for (final device in await client.listDirectory(_pathPrefix)) { final text = await client.getFileText('$_pathPrefix/$device/$_logFilename'); if (text == null) continue; - _mergeManualRecords(_decode(text), merged); + _mergeRecords(_decode(text), merged, kind: kind); } return merged.values .map((r) => (r.fields['payload']!.$1! as Map).cast()) .toList(); } on GitHubSyncError catch (error) { - debugPrint('WorkoutSyncService.readMergedManualPayloads failed: $error'); + final which = kind ?? 'any'; + debugPrint( + 'WorkoutSyncService: FAILED reading synced workouts ' + '(kind=$which) from $owner/$repo: $error — history may be incomplete.', + ); return const []; } finally { client.close(); } } - static void _mergeManualRecords(Log log, Map into) { + /// Merges records into [into], keeping the highest-HLC copy of each id. + /// + /// [kind] filters on the payload's `kind` discriminator; pass null to keep + /// every workout kind. The manual-workout budget must only ever see manual + /// self-reports (a verified run must not consume that budget), while the + /// history view wants everything — hence the filter rather than two merges. + static void _mergeRecords(Log log, Map into, {String? kind}) { for (final entry in log.entries) { final field = entry.value.fields['payload']; if (field == null) continue; final payload = field.$1; - if (payload is! Map || - payload['kind'] != kManualWorkoutSyncKind) { - continue; - } + if (payload is! Map) continue; + if (kind != null && payload['kind'] != kind) continue; final existing = into[entry.key]; if (existing == null || existing.fields['payload']!.$2 < field.$2) { into[entry.key] = entry.value; diff --git a/stronglift_replacement/workout_app/test/models/manual_workout_test.dart b/stronglift_replacement/workout_app/test/models/manual_workout_test.dart index ab2d979..66130a7 100644 --- a/stronglift_replacement/workout_app/test/models/manual_workout_test.dart +++ b/stronglift_replacement/workout_app/test/models/manual_workout_test.dart @@ -267,4 +267,57 @@ void main() { expect(durationMinutes(_tt(startTime: 'noon')), isNull); }); }); + + group('countManualBudget (per-day)', () { + Map payload(String date, String start) => { + 'date': date, + 'start_time': start, + }; + final now = DateTime(2026, 7, 16); + + test('multiple workouts on one day count as a single day', () { + final budget = countManualBudget([ + payload('2026-07-13', '14:00'), + payload('2026-07-13', '23:07'), + ], now); + expect(budget.week, 1); + expect(budget.month, 1); + }); + + test('workouts on distinct days each count', () { + final budget = countManualBudget([ + payload('2026-07-13', '14:00'), + payload('2026-07-15', '09:00'), + ], now); + expect(budget.week, 2); + }); + + test('same-day plus another day counts two days', () { + final budget = countManualBudget([ + payload('2026-07-13', '14:00'), + payload('2026-07-13', '23:07'), + payload('2026-07-15', '09:00'), + ], now); + expect(budget.week, 2); + }); + + test('ignores dates outside the 30-day window and future dates', () { + final budget = countManualBudget([ + payload('2026-05-01', '10:00'), + payload('2026-07-20', '10:00'), + ], now); + expect(budget.week, 0); + expect(budget.month, 0); + }); + + test('skips payloads with missing or unparsable date', () { + final budget = countManualBudget([ + {'start_time': '10:00'}, + {'date': 42}, + {'date': 'not-a-date'}, + ], now); + expect(budget.week, 0); + expect(budget.month, 0); + }); + }); } diff --git a/stronglift_replacement/workout_app/test/screens/history_screen_test.dart b/stronglift_replacement/workout_app/test/screens/history_screen_test.dart index 22a2428..bd20f38 100644 --- a/stronglift_replacement/workout_app/test/screens/history_screen_test.dart +++ b/stronglift_replacement/workout_app/test/screens/history_screen_test.dart @@ -1,12 +1,17 @@ import 'dart:convert'; +import 'package:crdt_sync/crdt_sync.dart'; import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:http/http.dart' as http; +import 'package:http/testing.dart'; import 'package:sqflite_common_ffi/sqflite_ffi.dart'; import 'package:workout_app/models/workout_plan.dart'; import 'package:workout_app/screens/history_screen.dart'; import 'package:workout_app/services/storage_service.dart'; +import '../fake_secure_storage.dart'; + void main() { setUpAll(() { sqfliteFfiInit(); @@ -16,6 +21,10 @@ void main() { setUp(() async { StorageService.resetForTesting(); await StorageService.init(); + // HistoryScreen pulls the PC's synced workouts, which reads the sync token + // from secure storage — fake it so tests never touch the OS keystore. + // No token => not configured => no network, and an empty synced list. + installFakeSecureStorage(); }); Future _pump(WidgetTester tester, Widget w) async { @@ -26,7 +35,53 @@ void main() { await tester.pump(); } - Widget _wrap() => const MaterialApp(home: HistoryScreen()); + Widget _wrap({http.Client? httpClient}) => + MaterialApp(home: HistoryScreen(httpClient: httpClient)); + + /// A GitHub mock serving one PC device log holding a run and a manual. + http.Client _syncMock() { + String file(String body) => jsonEncode({ + 'content': base64Encode(utf8.encode(body)), + 'sha': 'sha', + }); + Record rec(String id, String kind, int ms) => Record( + id: id, + fields: { + 'payload': ( + { + 'kind': kind, + 'date': '2026-07-13', + 'source': 'Running: 9.8 km in 55 min', + }, + Hlc(wallTimeMs: ms, counter: 0, nodeId: 'pc'), + ), + }, + ); + final log = jsonEncode({ + 'runnerup_verified:2026-07-13': rec( + 'runnerup_verified:2026-07-13', + 'runnerup_verified', + 2000, + ).toJson(), + 'manual:2026-07-13T14:00': rec( + 'manual:2026-07-13T14:00', + 'manual_workout', + 1000, + ).toJson(), + }); + return MockClient((req) async { + final path = req.url.path; + if (path.endsWith('screen-locker-sync/devices')) { + return http.Response(jsonEncode([ + {'name': 'pc', 'type': 'dir'}, + ]), 200); + } + if (path.contains('pc/log.json')) { + return http.Response(file(log), 200); + } + return http.Response('not found', 404); + }); + } // Seed a workout. DB writes must run on the real event loop: the widget-test // zone fakes async, so a sqflite-ffi write in the test body hangs (then the @@ -276,4 +331,39 @@ void main() { expect(find.byType(HistoryScreen), findsOneWidget); expect(find.text('WEIGHT OVER TIME').evaluate(), isEmpty); // total view }); + + testWidgets('shows the PC-synced workouts the phone has no session for', ( + tester, + ) async { + // The PC publishes its whole workout_log.json; without this the two + // devices show different histories (a RunnerUp run exists only on the PC). + installFakeSecureStorage(initial: {'sync.token': 'tok'}); + await _seed( + tester, + jsonEncode({ + 'exercises': [ + { + 'name': 'Squat', + 'targetSets': 3, + 'targetReps': 5, + 'targetWeight': 40.0, + 'warmupDone': false, + 'succeeded': true, + 'setResults': >[], + }, + ], + }), + date: '2026-07-11', + ); + await _pump(tester, _wrap(httpClient: _syncMock())); + + await tester.scrollUntilVisible( + find.text('SYNCED FROM PC'), + 200, + scrollable: find.byType(Scrollable).first, + ); + expect(find.text('SYNCED FROM PC'), findsOneWidget); + expect(find.textContaining('2026-07-13 · Run'), findsOneWidget); + expect(find.textContaining('2026-07-13 · Manual'), findsOneWidget); + }); } diff --git a/stronglift_replacement/workout_app/test/screens/settings_screen_test.dart b/stronglift_replacement/workout_app/test/screens/settings_screen_test.dart index ddcfa8b..7a42867 100644 --- a/stronglift_replacement/workout_app/test/screens/settings_screen_test.dart +++ b/stronglift_replacement/workout_app/test/screens/settings_screen_test.dart @@ -196,12 +196,38 @@ void main() { }); testWidgets( - 'shows Connected immediately when a token is already saved', + 'a saved token is VERIFIED on open, not blindly trusted', (tester) async { - installFakeSecureStorage(initial: {'sync.token': 'already-saved'}); - await _pump(tester, _wrap()); + // Regression: the badge used to read "Connected." whenever a token + // string existed, so a revoked token showed green while every sync + // 401'd and history silently stayed empty. + installFakeSecureStorage(initial: {'sync.token': 'revoked-token'}); + final mock = MockClient( + (req) async => http.Response('Bad credentials', 401), + ); + await _pump(tester, _wrap(httpClient: mock)); await _scrollToGitHubSync(tester); - expect(find.text('Connected.'), findsOneWidget); + expect(find.text('Connected.'), findsNothing); + expect(find.textContaining('NOT connected'), findsOneWidget); + }, + ); + + testWidgets( + 'a saved token that GitHub accepts reports verified', + (tester) async { + installFakeSecureStorage(initial: {'sync.token': 'good-token'}); + final mock = MockClient( + (req) async => http.Response( + jsonEncode({'content': base64Encode(utf8.encode('{}')), 'sha': 's'}), + 200, + ), + ); + await _pump(tester, _wrap(httpClient: mock)); + await _scrollToGitHubSync(tester); + expect( + find.textContaining('Connected and verified via GitHub'), + findsOneWidget, + ); }, ); @@ -526,9 +552,9 @@ void main() { await tester.tap(find.text('Connect GitHub')); await _pumpUntil( tester, - () => find.textContaining('could not verify').evaluate().isNotEmpty, + () => find.textContaining('NOT syncing').evaluate().isNotEmpty, ); - expect(find.textContaining('could not verify'), findsOneWidget); + expect(find.textContaining('NOT syncing'), findsOneWidget); }); }); diff --git a/stronglift_replacement/workout_app/test/services/workout_sync_service_test.dart b/stronglift_replacement/workout_app/test/services/workout_sync_service_test.dart index 88caf4f..e8188e5 100644 --- a/stronglift_replacement/workout_app/test/services/workout_sync_service_test.dart +++ b/stronglift_replacement/workout_app/test/services/workout_sync_service_test.dart @@ -266,6 +266,78 @@ void main() { expect(payloads.single['cost'], 'NEW'); }); + test('readMergedWorkoutPayloads returns the PC runs too', () async { + // The PC publishes its whole workout_log.json, so the phone must be able + // to see verified runs — not just manual self-reports — or the two + // devices never show the same history. + installFakeSecureStorage(initial: {'sync.token': 'tok'}); + final run = Record( + id: 'runnerup_verified:2026-07-13', + fields: { + 'payload': ( + { + 'kind': 'runnerup_verified', + 'date': '2026-07-13', + 'source': 'Running: 9.8 km in 55 min', + }, + Hlc(wallTimeMs: 3000, counter: 0, nodeId: 'pc'), + ), + }, + ); + final pcLog = jsonEncode({ + 'manual:a': _manual('manual:a', 'NEW', 2000).toJson(), + 'runnerup_verified:2026-07-13': run.toJson(), + }); + final (:httpClient, putCalls: _) = _mockGitHub( + contentResponses: { + 'screen-locker-sync/devices': _response(200, [ + {'name': 'pc'}, + ]), + 'screen-locker-sync/devices/pc/log.json': _fileContaining(pcLog), + }, + ); + final service = WorkoutSyncService(httpClient: httpClient); + + final all = await service.readMergedWorkoutPayloads(); + expect(all, hasLength(2)); + expect( + all.map((p) => p['kind']), + containsAll(['manual_workout', 'runnerup_verified']), + ); + }); + + test('readMergedManualPayloads still excludes runs (budget stays manual)', + () async { + // A verified run must never consume the manual self-report budget. + installFakeSecureStorage(initial: {'sync.token': 'tok'}); + final run = Record( + id: 'runnerup_verified:2026-07-13', + fields: { + 'payload': ( + {'kind': 'runnerup_verified', 'date': '2026-07-13'}, + Hlc(wallTimeMs: 3000, counter: 0, nodeId: 'pc'), + ), + }, + ); + final pcLog = jsonEncode({ + 'manual:a': _manual('manual:a', 'NEW', 2000).toJson(), + 'runnerup_verified:2026-07-13': run.toJson(), + }); + final (:httpClient, putCalls: _) = _mockGitHub( + contentResponses: { + 'screen-locker-sync/devices': _response(200, [ + {'name': 'pc'}, + ]), + 'screen-locker-sync/devices/pc/log.json': _fileContaining(pcLog), + }, + ); + final manuals = await WorkoutSyncService( + httpClient: httpClient, + ).readMergedManualPayloads(); + expect(manuals, hasLength(1)); + expect(manuals.single['kind'], 'manual_workout'); + }); + test('readMergedManualPayloads swallows a sync error', () async { installFakeSecureStorage(initial: {'sync.token': 'tok'}); final httpClient = http_testing.MockClient(