fix: androidTest Hilt binding, boot rescheduling, Spotify security fixes, escapeJson dedup - #516
Merged
Merged
Conversation
The entire instrumented-test suite has been broken since before the provider-mode-selection branch: FakeSttTestModule/FakeTtsTestModule each replace() the real SttModule/TtsModule wholesale via @TestInstallIn, but the real modules also provide WhisperModelDownloader/PiperVoiceDownloader, which OfflineStackViewModel/WhisperSettingsViewModel/PiperSettingsViewModel inject directly as concrete classes. Nothing replaced those two bindings, so the whole androidTest Hilt graph failed to compile. Both downloader classes only need a Context and make no network call until .download() is explicitly invoked (which no instrumented test does), so the real classes are provided as-is rather than fakes. Unblocks manual on-device verification of Phase 20 features via the same processUserInput()-bypass pattern used previously to verify real tool-calling (STT/mic not required).
AlarmManager exact alarms don't survive reboot, so any scheduled alarm, reminder, or routine silently stopped firing until the user re-touched it — a known limitation documented but not fixed when P20.2-4 shipped. New BootRescheduler re-arms every persisted alarm (recomputing next occurrence), every upcoming reminder (reusing its stored absolute trigger time directly, no recompute needed), and every routine with a schedule. Extracted as a plain class (not inline in BootReceiver) so it's unit-testable without a real BroadcastReceiver lifecycle, and each category is wrapped independently so a failure in one (e.g. a DB error) doesn't prevent the other two from rescheduling. BootReceiver becomes @androidentrypoint to inject the three DAOs + Moshi, and uses goAsync() since rescheduling does real DB + AlarmManager work that must outlive onReceive() returning. Also removes AlarmEntity.enabled (always true, no toggle tool exists to set it false) per the earlier review's "wire up a toggle or drop the column" note — dropped per YAGNI.
Extract the private per-file escapeJson() extension (duplicated identically in 8 files added this session) into a single shared com.opendash.app.tool.escapeJson. Pre-existing duplicates in ~15 files predating this session are left untouched to avoid unrelated refactor scope creep.
An unescaped track URI containing a double quote would produce invalid JSON in the request body sent to Spotify's /me/player/play.
…references The PKCE code_verifier and CSRF state for an in-flight Spotify authorization were stored in the plaintext AppPreferences DataStore alongside non-secret settings. Move them to the encrypted SecurePreferences store used for the access/refresh tokens, since the verifier is a secret the token exchange trusts implicitly.
… dialogs Discovered by actually running the instrumented suite on-device this session (previously blocked entirely by the androidTest Hilt binding bug fixed earlier in this branch): - Add @HiltAndroidTest + HiltAndroidRule. HiltTestRunner swaps in HiltTestApplication process-wide, so any test launching the real MainActivity needs the rule to create the test Dagger component first, or the launch crashes with "component was not created". - Update EXPECTED_TEXTS for the ProviderModeScreen landing screen added by the provider-mode-selection feature (was stale, only listed ModelSetupScreen/OnboardingScreen text). - Grant RECORD_AUDIO/POST_NOTIFICATIONS via `pm grant` through UiAutomation before launch. On a genuinely fresh install those permissions trigger a system dialog that owns the foreground window, so the UiAutomator foreground check never matched. Confirmed passing via connectedStandardDebugAndroidTest against a fresh install on a running emulator.
…control byte) Fixes from a final whole-branch review of this fix batch before merge: - Bump AppDatabase.version 12 -> 13. Removing AlarmEntity.enabled in 17b3b8e changed the compiled schema without bumping the version, so Room's identity-hash check would crash app launch on any device that already created the DB under the old schema (fallbackToDestructiveMigration only runs from onUpgrade, which never fires when the version is unchanged). Invisible to this session's own verification since it only ever used fresh installs and fresh in-memory test DBs. - AlarmFireReceiver now deletes a fired one-shot alarm's DB row. Without this, a fired one-shot alarm and a still-pending one are indistinguishable in storage (both are just hour/minute with an empty repeat mask), so BootRescheduler would re-arm long-fired one-shot alarms on every reboot. - Fix a literal raw control byte embedded in JsonEscape.kt's form-feed case, replacing it with the safe unicode escape literal the 8 duplicated functions it replaced all used -- a raw control byte in source is invisible in most editors/diffs and fragile against reformatting tools. - Clean up the legacy plaintext PKCE verifier/state DataStore keys left behind by the SecurePreferences migration in efd4493, so a device that started an authorization on an earlier build doesn't keep an orphaned secret in plaintext indefinitely.
yuga-hashimoto
added a commit
that referenced
this pull request
Jul 12, 2026
…20.4 notes ## Priority 1 PR #516 already shipped BootRescheduler + the one-shot alarm cleanup fix but never updated the roadmap, leaving P20.3/P20.4 pointing at a "doesn't survive reboot" limitation that's no longer true.
yuga-hashimoto
added a commit
that referenced
this pull request
Jul 12, 2026
## Priority 1 Tracks P21.1 (AEC/noise suppression), P21.2 (androidTest Hilt fix, already shipped upstream in PR #516), and P21.3 (looping/fading/ full-screen alarm ringing) from this session's gap-review follow-through.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up fix pass on top of the already-merged Phase 20 smart-speaker-parity batch (shopping lists, reminders, in-app alarms, scheduled routines, jokes/trivia, fast-path latency, translation, sports scores, Spotify Connect). Addresses every gap flagged in that batch's own code review plus everything found while actually running the app and its instrumented test suite on-device for the first time.
FakeSttTestModule/FakeTtsTestModuleused@TestInstallIn(replaces = ...), which drops every binding the replaced module provided, silently breakingWhisperModelDownloader/PiperVoiceDownloaderinjection for the wholeandroidTestsource set.BootRescheduler, rewiredBootReceiver. Also fixes a bug found in review: a fired one-shot alarm's DB row was never deleted, so it was indistinguishable from a still-pending one and would be resurrected on every subsequent reboot —AlarmFireReceivernow deletes it.escapeJson()across the 8 new tool executors added in Phase 20 into a sharedcom.opendash.app.tool.escapeJson(pre-existing duplicates elsewhere left untouched — out of scope).trackUribefore interpolating into theplay()request body (was producing invalid JSON / a soft injection point on a"in the URI); move the PKCEcode_verifier/statefrom plaintext DataStore to encryptedSecurePreferences, with cleanup of the old plaintext keys on next auth attempt.AppLaunchE2ETestfix — this pre-existing E2E test could never actually run before (blocked by the Hilt bug above). Once unblocked, found and fixed three real issues: missing@HiltAndroidTest/HiltAndroidRule, staleEXPECTED_TEXTS(predated the provider-mode-selection screen), and unhandled RECORD_AUDIO/POST_NOTIFICATIONS permission dialogs blocking the UiAutomator foreground check on a fresh install.AlarmEntity.enabledchanged the compiled Room schema without bumping the version, which would have crashed app launch via Room's identity-hash check on any device with a pre-existing DB.JsonEscape.kt's form-feed case during the dedup, replacing it with a safe unicode escape.Test plan
./gradlew testStandardDebugUnitTest— all unit tests green./gradlew assembleDebug— both flavors build./gradlew connectedStandardDebugAndroidTeston a running emulator — Hilt graph confirmed working;AppLaunchE2ETestpasses on a genuinely fresh installAssistantProviderE2ETest,FakeSttPipelineE2ETest,LatencyBudgetE2ETest, andVoicePipelineFastPathE2ETestall hitkotlinx.coroutines.testvirtual-time timeouts when run under instrumentation — a systemic incompatibility predating this branch (from the original E2E scaffolding PR test(e2e): scaffold instrumented test layer with Hilt + UiAutomator #459), not something introduced here.🤖 Generated with Claude Code