Implement audit remediation for BLE, sync, backup, and config hygiene#494
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements security, configuration, and BLE lifecycle guardrails, including secret-scanning scripts, a new WorkoutCommandValidator, a SyncInvariantChecker for data sync validation, and privacy metadata for backups. The reviewer provided valuable feedback on ensuring thread safety in the BLE event tracker, securing state cleanup during connection teardown with try-finally blocks, preventing potential null pointer exceptions and false-positive warnings in the invariant checker, using timezone-safe ISO-8601 formatting for backup timestamps, and improving the shell script's robustness when handling file paths with spaces.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fun recordDropped(type: BleCriticalEventType) { | ||
| _snapshot.value = when (type) { | ||
| BleCriticalEventType.REP -> _snapshot.value.copy(repEventsDropped = _snapshot.value.repEventsDropped + 1) | ||
|
|
||
| BleCriticalEventType.DELOAD -> _snapshot.value.copy(deloadEventsDropped = _snapshot.value.deloadEventsDropped + 1) | ||
|
|
||
| BleCriticalEventType.ROM_VIOLATION -> | ||
| _snapshot.value.copy(romViolationEventsDropped = _snapshot.value.romViolationEventsDropped + 1) | ||
|
|
||
| BleCriticalEventType.RECONNECTION_REQUEST -> | ||
| _snapshot.value.copy(reconnectionRequestsDropped = _snapshot.value.reconnectionRequestsDropped + 1) | ||
| } | ||
| } |
There was a problem hiding this comment.
The recordDropped function is not thread-safe. Since BLE events can be received concurrently from different characteristics or connection callbacks, updating _snapshot.value directly using a read-modify-write pattern can lead to race conditions and lost updates. Use _snapshot.update { ... } from kotlinx.coroutines.flow to ensure atomic, thread-safe updates.
fun recordDropped(type: BleCriticalEventType) {
_snapshot.update { current ->
when (type) {
BleCriticalEventType.REP -> current.copy(repEventsDropped = current.repEventsDropped + 1)
BleCriticalEventType.DELOAD -> current.copy(deloadEventsDropped = current.deloadEventsDropped + 1)
BleCriticalEventType.ROM_VIOLATION -> current.copy(romViolationEventsDropped = current.romViolationEventsDropped + 1)
BleCriticalEventType.RECONNECTION_REQUEST -> current.copy(reconnectionRequestsDropped = current.reconnectionRequestsDropped + 1)
}
}
}| suspend fun shutdown() { | ||
| log.i { "Shutting down BLE connection manager" } | ||
| isExplicitDisconnect = true | ||
|
|
||
| scanJob?.cancel() | ||
| scanJob = null | ||
| stateObserverJob?.cancel() | ||
| stateObserverJob = null | ||
| pollingEngine.stopAll() | ||
| discoMode.shutdown() | ||
| handleDetector.disable() | ||
| handleDetector.reset() | ||
|
|
||
| try { | ||
| peripheral?.disconnect() | ||
| } catch (e: Exception) { | ||
| e.rethrowIfCancellation() | ||
| log.w { "Shutdown disconnect error (non-fatal): ${e.message}" } | ||
| } | ||
|
|
||
| clearConnectionState(clearScannedDevices = true) | ||
| reportConnectionState(ConnectionState.Disconnected) | ||
| } |
There was a problem hiding this comment.
If peripheral?.disconnect() throws a CancellationException (which is rethrown by e.rethrowIfCancellation()), the subsequent cleanup calls clearConnectionState and reportConnectionState will be skipped. This leaves the connection manager in an inconsistent state. Wrap the disconnect call in a try-catch block and place the state clearing and reporting in a finally block to ensure they always run.
suspend fun shutdown() {
log.i { "Shutting down BLE connection manager" }
isExplicitDisconnect = true
scanJob?.cancel()
scanJob = null
stateObserverJob?.cancel()
stateObserverJob = null
pollingEngine.stopAll()
discoMode.shutdown()
handleDetector.disable()
handleDetector.reset()
try {
peripheral?.disconnect()
} catch (e: Exception) {
e.rethrowIfCancellation()
log.w { "Shutdown disconnect error (non-fatal): ${e.message}" }
} finally {
clearConnectionState(clearScannedDevices = true)
reportConnectionState(ConnectionState.Disconnected)
}
}| override suspend fun shutdown() { | ||
| log.i { "Shutting down BLE repository" } | ||
| connectionManager.shutdown() | ||
| _scannedDevices.value = emptyList() | ||
| _heuristicData.value = null | ||
| clearDiagnostics() | ||
| repositoryJob.cancel() | ||
| } |
There was a problem hiding this comment.
Similar to the connection manager, if connectionManager.shutdown() throws a CancellationException or other exception, the repository's local state clearing and repositoryJob.cancel() will be skipped. Wrap the shutdown call in a try-finally block to guarantee that the repository scope is cancelled and state is cleared.
override suspend fun shutdown() {
log.i { "Shutting down BLE repository" }
try {
connectionManager.shutdown()
} finally {
_scannedDevices.value = emptyList()
_heuristicData.value = null
clearDiagnostics()
repositoryJob.cancel()
}
}| .groupBy { | ||
| listOf( | ||
| it.exerciseName.trim().lowercase(), | ||
| it.recordType.trim().lowercase(), | ||
| it.workoutPhase.orEmpty().trim().lowercase(), | ||
| it.achievedAt.orEmpty().trim(), | ||
| it.sessionId.orEmpty().trim(), | ||
| ).joinToString("|") | ||
| } |
There was a problem hiding this comment.
If recordType is nullable and defaults to null when not provided (as seen in some test cases), calling it.recordType.trim() directly will throw a NullPointerException during grouping. Use orEmpty() to safely handle null values.
personalRecords
.filter { it.exerciseName.isNotBlank() && !it.achievedAt.isNullOrBlank() }
.groupBy {
listOf(
it.exerciseName.trim().lowercase(),
it.recordType.orEmpty().trim().lowercase(),
it.workoutPhase.orEmpty().trim().lowercase(),
it.achievedAt.orEmpty().trim(),
it.sessionId.orEmpty().trim(),
).joinToString("|")
}| if (exercise.sessionId.isNotBlank() && exercise.sessionId != session.id) { | ||
| violations += SyncInvariantViolation( | ||
| code = "ORPHAN_PULL_EXERCISE", | ||
| entityId = exercise.id, | ||
| message = "Exercise ${exercise.id} references session ${exercise.sessionId}, expected ${session.id}", | ||
| ) |
There was a problem hiding this comment.
UUID casing can vary across platforms (e.g., iOS often generates uppercase UUIDs, while Android and backends default to lowercase). Comparing them directly with != can lead to false-positive orphan warnings. Use .equals(..., ignoreCase = true) for case-insensitive ID comparisons.
| if (exercise.sessionId.isNotBlank() && exercise.sessionId != session.id) { | |
| violations += SyncInvariantViolation( | |
| code = "ORPHAN_PULL_EXERCISE", | |
| entityId = exercise.id, | |
| message = "Exercise ${exercise.id} references session ${exercise.sessionId}, expected ${session.id}", | |
| ) | |
| if (exercise.sessionId.isNotBlank() && !exercise.sessionId.equals(session.id, ignoreCase = true)) { | |
| violations += SyncInvariantViolation( | |
| code = "ORPHAN_PULL_EXERCISE", | |
| entityId = exercise.id, | |
| message = "Exercise ${exercise.id} references session ${exercise.sessionId}, expected ${session.id}", | |
| ) | |
| } |
| val exportNowMs = KmpUtils.currentTimeMillis() | ||
| val exportedAt = KmpUtils.formatTimestamp(exportNowMs, "yyyy-MM-dd") + "T" + | ||
| KmpUtils.formatTimestamp(exportNowMs, "HH:mm:ss") + "Z" |
There was a problem hiding this comment.
Using manual string formatting and appending 'Z' can lead to incorrect timezone representations if KmpUtils.formatTimestamp formats in local time instead of UTC. A safer, cleaner, and standard way in kotlinx.datetime is to use Instant.fromEpochMilliseconds(KmpUtils.currentTimeMillis()).toString(), which automatically formats to a timezone-safe ISO-8601 UTC string.
| val exportNowMs = KmpUtils.currentTimeMillis() | |
| val exportedAt = KmpUtils.formatTimestamp(exportNowMs, "yyyy-MM-dd") + "T" + | |
| KmpUtils.formatTimestamp(exportNowMs, "HH:mm:ss") + "Z" | |
| val exportedAt = kotlinx.datetime.Instant.fromEpochMilliseconds(KmpUtils.currentTimeMillis()).toString() |
| while IFS= read -r raw_path; do | ||
| path="${raw_path//\\//}" | ||
| lower="$(printf '%s' "$path" | tr '[:upper:]' '[:lower:]')" | ||
| is_template=false | ||
| case "$lower" in | ||
| *.example|*.sample|*.template|*/templates/*) is_template=true ;; | ||
| esac | ||
|
|
||
| if [[ "$path" =~ (^|/)Supabase\.xcconfig$ ]]; then | ||
| violations+=("$path (real iOS Supabase config must stay local or be generated from CI secrets)") | ||
| continue | ||
| fi | ||
|
|
||
| if [[ "$path" =~ (^|/).*\.local\.properties$ || "$path" =~ (^|/)local\.properties$ ]]; then | ||
| violations+=("$path (local Gradle/SDK or secret properties must not be tracked)") | ||
| continue | ||
| fi | ||
|
|
||
| if [[ "$is_template" == false && ( "$path" =~ (^|/)google-services\.json$ || "$path" =~ (^|/)GoogleService-Info\.plist$ ) ]]; then | ||
| violations+=("$path (service config must be supplied outside git unless it is a template)") | ||
| continue | ||
| fi | ||
|
|
||
| if [[ "$is_template" == false && "$path" =~ \.(jks|keystore|p12|mobileprovision)$ ]]; then | ||
| violations+=("$path (signing material must be supplied from a local machine or CI secret)") | ||
| continue | ||
| fi | ||
|
|
||
| if [[ "$is_template" == false && "$path" =~ (^|/)\.env(\..*)?$ ]]; then | ||
| violations+=("$path (environment files must not be tracked)") | ||
| continue | ||
| fi | ||
| done < <(git -C "$repo_root" ls-files) |
There was a problem hiding this comment.
Reading file paths line-by-line using while read without -d '' can fail or behave unexpectedly if any tracked file names contain spaces, newlines, or special characters. It is safer and more robust to use git ls-files -z paired with read -r -d '' to handle null-terminated file paths.
while IFS= read -r -d '' raw_path; do
path="${raw_path//\\//}"
lower="$(printf '%s' "$path" | tr '[:upper:]' '[:lower:]')"
is_template=false
case "$lower" in
*.example|*.sample|*.template|*/templates/*) is_template=true ;;
esac
if [[ "$path" =~ (^|/)Supabase\.xcconfig$ ]]; then
violations+=("$path (real iOS Supabase config must stay local or be generated from CI secrets)")
continue
fi
if [[ "$path" =~ (^|/).*\.local\.properties$ || "$path" =~ (^|/)local\.properties$ ]]; then
violations+=("$path (local Gradle/SDK or secret properties must not be tracked)")
continue
fi
if [[ "$is_template" == false && ( "$path" =~ (^|/)google-services\.json$ || "$path" =~ (^|/)GoogleService-Info\.plist$ ) ]]; then
violations+=("$path (service config must be supplied outside git unless it is a template)")
continue
fi
if [[ "$is_template" == false && "$path" =~ \.(jks|keystore|p12|mobileprovision)$ ]]; then
violations+=("$path (signing material must be supplied from a local machine or CI secret)")
continue
fi
if [[ "$is_template" == false && "$path" =~ (^|/)\.env(\..*)?$ ]]; then
violations+=("$path (environment files must not be tracked)")
continue
fi
done < <(git -C "$repo_root" ls-files -z)![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8907f9578
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private fun isFinite(value: Float): Boolean = !value.isNaN() && !value.isInfinite() | ||
|
|
||
| private fun validateWeightRange(weightPerCableKg: Float, allowZero: Boolean): Result<Unit> { | ||
| val min = if (allowZero) Constants.MIN_WEIGHT_KG else Constants.JUST_LIFT_MIN_VALID_WEIGHT_KG |
There was a problem hiding this comment.
For normal non-AMRAP workouts this makes the minimum BLE-sendable weight 1 kg by reusing the Just Lift race-condition guard, but the app still exposes and stores 0.5 kg increments (WEIGHT_INCREMENT_OPTIONS_KG/DEFAULT_WEIGHT_INCREMENT_KG) and updateSetReadyWeight clamps routine weights from MIN_WEIGHT_KG (0) to the hardware max. A user can therefore configure a regular routine/set at 0.5 kg per cable, then validateProgramParams/legacy validation rejects it before sending the workout command, preventing an otherwise valid lightweight/fractional set from starting. Use the general minimum for non-Just-Lift commands and keep the 1 kg guard scoped to Just Lift.
Useful? React with 👍 / 👎.
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Another round of cleanup and this PR stays pristine. I'm starting to think the author has achieved some zen-like code harmony. Incremental Diff Analysis
📊 Overall: Like a well-maintained codebase that actually respects its own conventions. No issues found in this increment. Files Reviewed (9 files in incremental diff)
🏆 Best part: SQL-level filtering ( 📊 Overall: Clean increment. The author's attention to defensive programming (double filtering) shows caution, even if it's redundant with the SQL filter. Everything works as intended. Reviewed by laguna-m.1-20260312:free · 3,520,736 tokens |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 267e08db99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| writer.write("""{"version":$CURRENT_BACKUP_VERSION,"exportedAt":"$exportedAt","appVersion":"${Constants.APP_VERSION}","data":{""") | ||
| val exportedAt = kotlin.time.Instant.fromEpochMilliseconds(KmpUtils.currentTimeMillis()).toString() | ||
| val privacy = json.encodeToString(BackupPrivacyMetadata.serializer(), BackupPrivacyMetadata()) | ||
| writer.write("""{"version":$CURRENT_BACKUP_VERSION,"exportedAt":"$exportedAt","appVersion":"${Constants.APP_VERSION}","privacy":$privacy,"data":{""") |
There was a problem hiding this comment.
When streaming export writes privacy before data, the streaming importer cannot read these new backups because its top-level parser only handles version, exportedAt, appVersion, and data and does not skip unknown fields. For large backups (or content URIs with unknown size) that use importFromStream, after nav.nextName() reads privacy the next loop attempts to parse the privacy object as a field name and fails before reaching data. Add a privacy -> nav.skipValue() branch or a default skip for unknown top-level fields.
Useful? React with 👍 / 👎.
Summary
Testing
:shared:testAndroidHostTesttargeted regression coverage for BLE cancellation, packet validation, sync invariants, backup serialization, and diagnostics export text.spotlessCheck,validateSchemaManifest,:shared:verifyCommonMainVitruvianDatabaseMigration, and:shared:testAndroidHostTestall passed in the full local gate.git diff --checkwas clean apart from CRLF normalization warnings.