Skip to content

Audit round 5: Home on failed reads, daily streak at app start, clean-ups - #6

Merged
khashyap0803 merged 3 commits into
masterfrom
claude/audit-2026-10-r5
Oct 30, 2026
Merged

khashyap0803 merged 3 commits into
masterfrom
claude/audit-2026-10-r5

Conversation

@khashyap0803

Copy link
Copy Markdown
Owner

Round 5 of the audit, branched from master at 10900e3 (after #5 was merged). Same rules as before: one commit per fix (the three clean-ups share one, as you allowed), a JVM unit test for each fix, explicit git add paths, no amend or force-push. Test names avoid " : * ? < > |.

This PR leaves the security-rules PR (#3) alone. It does not touch firestore.rules, database.rules.json, firebase-rules-tests/ or the docs that PR changes.

Contents: 3 commits, in the order you gave them. For each one I reverted just the fix's logic (or, for the log guard, put master's files back), re-ran its tests, and confirmed they fail.

1. Changes

# Where (this branch) Bug What a student experienced Fix Test
1 ui/home/HomeFragment.kt:261, :279, :289, :632, :643, :691, :699 loadProgressFromCloud returns null both when there is no progress document and when the read failed. Home drew a new student's defaults for either. When the connection dropped, Home showed 0 XP, a 1-day streak and 0 stages in place of the student's real progress. Home reads with loadProgressResult(), and homeViewFor decides what to show:
  • Loaded: as now.
  • Absent: the new-student defaults, as now.
  • Failed: keeps the figures Home last drew. If Home never drew any, the dashboard stays blank; it never shows zeros.
A Snackbar says "Couldn't load your progress. Check your connection and try again." with Retry, and pull-to-refresh also retries. Nothing is written to Firestore on this path.

Home keeps its own home_shown_* copy of what it drew. The existing home_* keys also get Learning Path's first-launch zeros, written before any read.
HomeProgressTest (5)
2 services/UserProgressSyncService.kt:99, :125, :140, :162, :411; MainActivity.kt:91; ui/home/HomeFragment.kt:143; ui/learningpath/LearningPathFragment.kt:471; AppDate.kt:38 The daily streak was only updated when Learning Path was opened. After days away, Home showed an old streak. A student who only opened Home or Practice never got one. recordDailyVisit counts the day once per student per calendar day, whichever caller asks first.
  • MainActivity.onStart asks for any signed-in student, including when the app comes back to the foreground after midnight.
  • Home and Learning Path ask before they read progress. If the update is already running, they wait for it, so they show the new streak instead of racing the write.
  • Learning Path no longer updates the streak itself.
The rules are unchanged, moved into dailyStreak:
  • The same day changes nothing.
  • The next day adds 1.
  • A gap of 2 or more days starts again at 1.
  • The best streak only goes up.
The write is still saveStreak's streak fields.
  • A failed read writes nothing and is retried next time.
  • A write Firestore hasn't confirmed after 3s (offline) counts as made. It is already in the local cache and is sent on reconnect, and without this limit it would hold Home and Learning Path indefinitely.
  • The milestone and "streak broken" toasts are unchanged; whichever caller made the update shows them.
DailyStreakTest (12)
3a services/FirestoreManager.kt deleteAllPersonalizedStages had no callers and pointed at users/{u}/personalized_stages, where no stages are kept. n/a Deleted. none (see below)
3b services/FirestoreManager.kt:120, :136 pathReadOf's KDoc sat above retakeWrites. n/a Each comment is now on its own function. none (see below)
3c services/GeminiQuizService.kt:294; GeminiChallengeService.kt:266, :1151, :1181 Release-level logs, which survive R8, wrote model text:
  • The quiz's Log.e "Raw response (first 500 chars)".
  • The challenge service's two Log.e "Cleaned response" lines.
  • A Log.i with the model-generated problem statement.
Model reply text was in release device logs. Each now logs the length. The quiz also keeps the parse error it already logged. Log.d and Log.v are untouched. ReleaseLogIdentifierTest (+2, now 4)

On the tests

  • Fix 2 across a day boundary. DailyStreakTest covers:
    • Same day: no change.
    • Next day: +1, across a month boundary and a year boundary.
    • A gap of 2+ days: reset to 1, best streak kept.
    • A shorter new streak keeps the old best.
    • A first visit.
    • With a fake progress document that remembers writes: the day is counted once, whichever screen asks first (one read, one write); the next calendar day is counted again; a failed read writes nothing and is retried; a refused write is retried; an unconfirmed write doesn't hold the screens.
    • The toast texts.
  • Mutation checks for fix 2.
    • Dropping the once-a-day guard fails "counted once".
    • Letting a same-day visit add 1 fails "same day".
    • Waiting on the write without a limit made the offline test hang rather than fail. I gave that test a 5s limit, so the same revert now fails fast.
  • 3a and 3b have no test. A test would only restate the deletion or the comment's position.
  • The 3c guard. It fails on any Log.w/e/i/wtf that interpolates a reply-text variable: response, responseText, cleaned, cleanedResponse, cleanJson, rawResponse, reply, content, jsonArray or problemStatement.
    • Logging the variable's .length, .size or .code is allowed.
    • A bare $name is always flagged, because "$cleaned.length" prints the text.
    • Run against master's two services, it lists exactly the four lines above.
  • What the tests cover. homeViewFor, dailyStreak, recordDailyVisit and streakMessage are tested directly. The Activity and Fragment lines that call them need Robolectric to exercise, so I checked them by reading the code: the Snackbar, the home_shown_* cache, and the onStart, Home and Learning Path calls.
  • No device verification. Nothing here was run on a phone or emulator.

2. Found but not fixed

Sev Where Issue Suggested fix
Low services/GeminiChatService.kt:164, GeminiReportService.kt:732 Log.e writes the HTTP error body ($responseBody) on a non-2xx response. That is the gateway's error JSON, not a model reply, so it is outside 3c and the guard doesn't check responseBody. It is still server text in release logs. Log the length, and add responseBody to the guard.
Low Fix 2, Home's first load of the day Two progress reads: the daily update's and Home's own. While offline, each Home or Learning Path load retries the update's read. Have the daily update hand its read back to the caller.
Low Fix 1, Home with no saved figures A failed read on a device where Home never drew anything leaves the dashboard blank. Achievements aren't redrawn from the saved figures, which don't hold stars or the best streak. Save the full progress if you want achievements back offline.
Low UserProgressSyncService.syncProgress, saveProgressToCloud; LearningPathFragment.syncProgressToCloud No callers. They are the only users of the deprecated getLocalProgress, which accounts for 4 of the 20 warnings (2 warnings, each reported twice). Remove them.
Hygiene KeyPathCostHarness.kt:32, MisconceptionDiagnosticHarness.kt:168 The test harnesses write to a hard-coded C:/Users/... path, which leaves app/C:/ in a Linux checkout. One was already sitting in this branch's working tree; I deleted it. Write under build/.
Low Earlier rounds
  • Rotation and back-out show "…cancelled": LoginActivity.kt:518, :589; QuizActivity.kt:183.
  • Catch-alls swallow cancellation.
  • !! in LoginActivity.
  • Unused GEMINI_API_KEY field.
As in #5.

Out of scope as you asked, so not started: individual contestant sign-in, a server-side username lookup, the event accounts, Analytics XP, partly generated quizzes, and rotation state on the retake screen.

3. Verification

./gradlew :app:testDebugUnitTest :app:compileReleaseKotlin
BUILD SUCCESSFUL in 1m 20s
51 actionable tasks: 51 executed

Run on this branch's head after ./gradlew clean. JUnit XML totals: 224 tests, 0 failures, 0 errors, 0 skipped across 39 classes. That is master's 205 plus 19 new:

Test class Tests
HomeProgressTest (new) 5
DailyStreakTest (new) 12
ReleaseLogIdentifierTest (extended) 2 → 4

Kotlin warnings: 20, the same as master (10 deprecation warnings, each reported for both the debug and release compiles). None are new. compileReleaseKotlin passes.

Dependencies came from the repositories in settings.gradle.kts, with no mirror; they were already cached.


Generated by Claude Code

Home read progress with loadProgressFromCloud, which returns null both when
there is no progress document and when the read failed, and drew a new
student's defaults (0 XP, 1-day streak, 0 stages) for either. A dropped
connection replaced a real student's dashboard with zeros.

Home now reads with loadProgressResult. HomeFragment.homeViewFor decides:
Loaded is drawn as before; Absent draws the new-student defaults as before;
Failed keeps the figures Home last drew - kept in its own home_shown_*
prefs, because the other home_* values also get LearningPathFragment's
first-launch zeros - or, if Home never drew any, leaves the dashboard
blank. A Snackbar says "Couldn't load your progress. Check your connection
and try again." with Retry; pull-to-refresh retries too. Nothing is
written to Firestore on this path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018eTtYuuVWNwDn1mSsKbKrW
The streak was only updated when Learning Path was opened. After days away
Home showed an old streak, and a student who only opened Home or Practice
never got one.

UserProgressSyncService.recordDailyVisit counts the day once per student
per calendar day. MainActivity asks in onStart (so it also runs after the
app sat in the background past midnight), and Home and Learning Path ask
before they read progress, so whichever comes first does the update and
the others wait for it and show the new streak. Learning Path no longer
updates the streak itself. The rule is the old one, moved into
dailyStreak: same day changes nothing, the next day adds 1, a gap of two or
more days starts again at 1, and the best streak only goes up. The write is
saveStreak's streak fields. A failed read writes nothing and is retried
next time. A write Firestore hasn't confirmed after 3 seconds (offline) is
counted as made - it is already in the local cache and is sent on
reconnect - so it can't hold Home and Learning Path. The milestone and
"streak broken" toasts move with it, shown by whichever caller made the
update.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018eTtYuuVWNwDn1mSsKbKrW
- Delete FirestoreManager.deleteAllPersonalizedStages. Nothing calls it
  since the retake moved to one WriteBatch, and it pointed at
  users/{username}/personalized_stages, where no stages are kept.
- pathReadOf's KDoc sat above retakeWrites, attached to the wrong
  function. Each comment is now on its own function.
- The quiz and challenge services wrote the model's reply text to
  release-level logs, which survive R8: GeminiQuizService's "Raw response
  (first 500 chars)" Log.e, GeminiChallengeService's two "Cleaned response"
  Log.e lines, and a Log.i with the model-generated problem statement. They
  now log the length (and, for the quiz, the parse error that was already
  logged). Log.d and Log.v are untouched.

ReleaseLogIdentifierTest now also fails on any Log.w/e/i/wtf that
interpolates a reply-text variable (response, cleaned, cleanJson, content,
problemStatement, ...) other than its length, size or HTTP code.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018eTtYuuVWNwDn1mSsKbKrW
Copilot AI balanced review requested due to automatic review settings October 30, 2026 09:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants