Clear the Minor findings from the hardening review - #4
Open
foodlbs wants to merge 1 commit into
Open
Conversation
Cleanup pass over the 11 Minor items logged during the hardening review. No behavior changes; three test gaps closed. - SkipperApp: url is non-nullable, so drop the dead if-guard; the six silent catchNonCancelling sites now log at Log.d so a swallowed cache/parse failure is diagnosable. - RemoteConfigParser: drop the redundant .take(256) in customButtonList (stringList already caps each entry at MAX_MATCHER_STRING). - proguard-rules: correct two misleading comments and drop the inert java.security keep — those are platform classes R8 never sees. - SkipperKitSettingsScreen: alphabetize the semantics imports. - SECURITY.md: note that the relocation mv runs from the repo root. - TeachScanner: document why packageName is unused (global throttle). - DiscoveryCoordinator: drop the unused injectable classifier param. - Tests: cover the clickable-but-blank teach filter branch, the CUSTOM gesture-path displayLabel, and addApproved's pending/dismissed cross-state promotion.
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
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
Clears the 11 Minor findings logged during the hardening review (PR #3). No behavior changes; three test gaps closed. Follow-up to #3.
Code
SkipperApp—urlis a non-null constant, so theif (url != null)guard was dead; removed and the block dedented. The sixcatchNonCancellingsites that swallowed silently (onFailure = {}) now log atLog.d, so a failed cached-config read/parse or version read is diagnosable instead of vanishing.RemoteConfigParser— dropped the redundant.take(256)oncustomButtonList's viewIds/labels. Verified first:stringListalready caps every entry atMAX_MATCHER_STRING(added in Hardening & refactor: 1-month roadmap from ownership audit #3), so the second cap was a no-op. The cap itself is unchanged.proguard-rules.pro— corrected two misleading comments and removed the inert-keep class java.security.**rule. Those are Android platform classes, never part of the app's dex, so R8 could never strip them; the rule implied a protection that didn't exist. The DataStore comment claimed R8 "can inline and rename" key strings — R8 never rewrites string literals; the keep actually guards against class shrinkage, which the comment now says.SkipperKitSettingsScreen— alphabetized the semantics imports.TeachScanner— KDoc explaining thatpackageNameis intentionally unread (the throttle is global, matching the single timestamp it replaced).DiscoveryCoordinator— removed the injectableclassifierparam and its imports. Nothing injected it;DiscoveryEngine.discoveralready defaults it (YAGNI).SECURITY.md— note that the key-relocationmvmust run from the repo root (it uses a relative source path).Tests (3 gaps closed)
TeachScannerTest— a clickable node with no id/text/desc (and a blank-after-trim one) must be filtered out. Previously only theisClickablehalf of the predicate was exercised, so a regression stripping theisNullOrBlankchecks would have gone unnoticed.SkipEngineTest—NeedsGesture.displayLabelon the CUSTOM path. The existing custom-button test always hit theClickedpath, leaving the gesture-fallback label untested.DiscoveryRepositoryTest—addApprovedon a pending key (promotes and clears from pending) and on a dismissed key (promotes). This edge changed behavior in Hardening & refactor: 1-month roadmap from ownership audit #3's lifecycle unification and was flagged as untested.Test plan
./gradlew testDebugUnitTest lintDebug assembleDebug assembleRelease— BUILD SUCCESSFULassembleReleaseincluded deliberately, since ProGuard rules changedNot included
The Critical host-hygiene items (C-1 signing key, C-2 release keystore) are maintainer actions on local key material, not code — runbooks are in
SECURITY.md. Still outstanding, along with the R8 runtime smoke test and TalkBack check.