feat: on-device crash log, plus CI to run the tests - #36
Conversation
feat: Add Auto mode (plan auto-approval and allowlist)
Closes #17. The app had no crash handling at all - no uncaught exception handler and no crash reporting SDK - so a crash left nothing behind but a logcat line the user could not reach. Install a default uncaught exception handler that persists the crash and then delegates to the system handler, so the process still dies normally. Crashes are stored in a new Room table (migration 6 -> 7, purely additive) capped at the 50 most recent, and are viewable, shareable, and clearable from a new Crash Log screen reachable from Settings. Everything stays on device: no SDK, no network egress. The recording path is split so its logic is unit testable without Android: CrashReportFormatter and CrashReportExporter are pure, CrashLogRecorder talks to a CrashLogSink interface, and only RoomCrashLogSink touches Room. The recorder and the handler swallow every Throwable - failing to log a crash must never replace a real, diagnosable crash with its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the previous commit raised five issues worth acting on. - Add the missing copy-to-clipboard action. Sharing a crash report was implemented but copying it was not. Uses the platform ClipboardManager rather than Compose's, matching how SystemActions already does it and avoiding the deprecated Compose clipboard API. - Cap the crash-path stall at one timeout, not two. RoomCrashLogSink gave the insert and the prune 2s each, so a wedged database could hold the crashing thread for 4s and turn a crash into an ANR. Both now share one monotonic budget, and storing the crash takes priority over pruning. - Expose crashes as a StateFlow via stateIn, matching every other view model in the package, so the list survives configuration changes without re-subscribing. - Correct the OpenDroidCrashHandler comment. The flag is a one-shot latch that records only the first crash in a process, not the re-entry guard the comment described. Renamed to recordedFirstCrash to say so. - Drop CrashReportFormatter.summarize and CrashLogDao.count(), which had no callers. CrashLogRecord.summary already covers the former. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0afcc51cf1
ℹ️ 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".
| val intent = Intent(Intent.ACTION_SEND).apply { | ||
| type = "text/plain" | ||
| putExtra(Intent.EXTRA_SUBJECT, "OpenDroid crash report") | ||
| putExtra(Intent.EXTRA_TEXT, text) |
There was a problem hiding this comment.
Share large logs outside Intent extras
When the toolbar's “Share all” action exports 50 near-limit records, the payload can exceed 800,000 characters (50 × the 16,000-character stack limit, plus messages and headers). Putting that string in Intent.EXTRA_TEXT makes startActivity marshal a payload larger than Binder's transaction buffer, causing TransactionTooLargeException instead of opening the chooser; share aggregate logs through a file-backed content URI or enforce a substantially smaller aggregate limit.
Useful? React with 👍 / 👎.
| """ | ||
| DELETE FROM crash_logs | ||
| WHERE id NOT IN ( | ||
| SELECT id FROM crash_logs ORDER BY timestamp DESC, id DESC LIMIT :keep |
There was a problem hiding this comment.
Prune by insertion order rather than wall-clock time
When the device clock moves backward—for example after correcting an erroneously future clock—the newly inserted crash has the greatest id but an older timestamp. Once 50 records already exist, this ordering excludes that new row from the retained subquery and immediately deletes it, losing the crash that just occurred; use id DESC as the retention order while keeping timestamp ordering only for presentation if desired.
Useful? React with 👍 / 👎.
Confidence Score: 5/5Safe to merge; all crash-path invariants (delegate always called, write always time-bounded, credentials always redacted before storage) are enforced in code and backed by 46 unit tests. The crash handler, recorder, sink, redactor, DAO, and view model are each well-scoped and the design decisions (runBlocking on a suspend DAO, insert-before-prune, monotonic budget, AtomicBoolean one-shot latch) are explicitly documented and correct. The Room migration is purely additive and the column definitions match the entity. The two comments left are style nits. Files Needing Attention: No files require special attention beyond the two minor style suggestions on CrashLogRecorder.kt and CrashLogScreen.kt.
|
| Filename | Overview |
|---|---|
| app/src/main/java/com/opendroid/ai/core/crash/OpenDroidCrashHandler.kt | New uncaught exception handler — installs idempotently, uses AtomicBoolean one-shot latch, and always delegates to the system handler in a finally block. |
| app/src/main/java/com/opendroid/ai/core/crash/CrashLogRecorder.kt | Builds and stores crash records; swallows all Throwables on the crash path. buildRecord is declared public with no external callers — should be internal or private. |
| app/src/main/java/com/opendroid/ai/core/crash/CrashLogRedactor.kt | Credential scrubber applied at capture time; ordered rules handle URL params, Bearer tokens, headers, JSON fields, and vendor-prefixed keys. Rule ordering and \Q...\E escaping are correct. |
| app/src/main/java/com/opendroid/ai/data/crash/RoomCrashLogSink.kt | Uses runBlocking + withTimeoutOrNull with a shared monotonic budget per crash to avoid ANR on a wedged DB; insert-before-prune ordering is correct. |
| app/src/main/java/com/opendroid/ai/data/db/OpenDroidDatabase.kt | Additive MIGRATION_6_7 creates crash_logs table and timestamp index; column definitions match CrashLogEntity. Migration registered in DatabaseModule. |
| app/src/main/java/com/opendroid/ai/ui/viewmodel/CrashLogViewModel.kt | Maps Room entities to CrashLogItem before exposing as StateFlow; exportAll re-reads from DAO for share consistency. |
| app/src/main/java/com/opendroid/ai/ui/screens/CrashLogScreen.kt | Crash list UI with expand/collapse, share, copy, and delete-all. Icons.Default.ArrowBack is deprecated — RTL layouts would render the wrong direction. |
| .github/workflows/android-ci.yml | New CI workflow: JDK 21 Temurin, shared Gradle cache (read-only for PRs), testDebugUnitTest + assembleDebug, test report upload on always(). |
| app/src/main/java/com/opendroid/ai/OpenDroidApp.kt | installCrashHandler() called first in onCreate after super (which triggers Hilt injection), wrapped in try/catch so setup failure never prevents app start. |
Sequence Diagram
sequenceDiagram
participant App as OpenDroidApp.onCreate
participant Handler as OpenDroidCrashHandler
participant Recorder as CrashLogRecorder
participant Redactor as CrashLogRedactor
participant Sink as RoomCrashLogSink
participant DAO as CrashLogDao (Room)
participant System as System UEH
App->>Handler: install(recorder)
Note over Handler: wraps existing default UEH
Note over System: crash occurs on any thread
System-->>Handler: uncaughtException(thread, throwable)
Handler->>Handler: compareAndSet(false→true)
Handler->>Recorder: record(thread, throwable)
Recorder->>Redactor: redact(message + stackTrace)
Redactor-->>Recorder: redacted strings
Recorder->>Sink: record(CrashLogRecord)
Sink->>DAO: insert() [runBlocking, 2s budget]
Sink->>DAO: pruneToMostRecent(50) [remaining budget]
Recorder-->>Handler: (returns)
Handler->>System: delegate.uncaughtException(thread, throwable)
Note over System: process killed normally
Reviews (7): Last reviewed commit: "fix: bound the aggregate share payload a..." | Re-trigger Greptile
The only workflow in the repo deployed the marketing site. Nothing compiled the app or ran a test, so app/src/test - sixteen files, four of them covering the crash log - ran only when someone remembered to. The "restore Android debug build compilation" fix a few commits back is what that gap costs. Add a workflow that runs testDebugUnitTest and assembleDebug on pull requests to main and on pushes to main. Website, docs, and markdown changes are filtered out; they have their own workflow or need no build. JDK 21 matches the jvmTarget in app/build.gradle. Only main writes the Gradle cache so pull requests share one warm cache. The job takes contents: read rather than inheriting the write permission deploy.yml needs. No secrets: gradle.properties is gitignored, so hasSigningConfig is false and assembleDebug never reads the release signing config. Test reports upload on failure so a red run is diagnosable from the run page. Lint and instrumented tests are deliberately left out. Lint would land a large untriaged backlog, and there is no androidTest source set yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI failed on its first run: Configuration `:app:debugRuntimeClasspath` contains AndroidX dependencies, but the `android.useAndroidX` property is not enabled The cause is not the crash log or the workflow. gradle.properties was gitignored, so android.useAndroidX, android.enableJetifier and org.gradle.jvmargs were absent from every fresh checkout. A clean clone of this repo could not build at all; the only reason nobody noticed is that everyone working on it already had a local copy. The ignore rule was there to protect the three RELEASE_* passwords, but it swept up settings the build cannot work without. Split the two concerns: track gradle.properties with the build settings only, and keep credentials out of the project entirely. Signing still works. app/build.gradle reads those properties with findProperty, which resolves from the Gradle home too, so they now belong in ~/.gradle/gradle.properties or in ORG_GRADLE_PROJECT_* environment variables. gradle.properties.example is rewritten to say so instead of telling people to write passwords into a file that is now published, and the three references in app/build.gradle are updated to match. *.keystore stays ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first CI run that got far enough to execute tests failed one of them: CrashReportFormatterTest > short stack traces are not truncated FAILED 138 tests completed, 1 failed The formatter is fine. The test was not. It captured a stack trace of an exception constructed inside a method named `short stack traces are not truncated`, so the trace's top frame is: at ...CrashReportFormatterTest.short stack traces are not truncated(...) Then it asserted the trace did not contain "truncated". The backtick method name puts that word in every trace the method captures, so the assertion could never hold. Assert on TRUNCATION_MARKER instead, which is what the code actually appends, and make it internal so tests can name it. The oversized case now checks endsWith rather than contains: it passed only because maxChars = 40 happened to cut the head before the method name appeared, which is luck, not a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Drop the dead assertion on an unused FakeSink in the prune-ordering test; the test passes its own ordering sink, so the assertion could never fail. - Keep the Room entity out of the UI: the view model now exposes CrashLogItem(id, CrashLogRecord) so CrashLogScreen depends on the domain model instead of CrashLogEntity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…intenance - Add light/dark theme system (CSS custom properties, persisted via localStorage, no-FOUC inline script in <head> of all 8 pages) - Add accessible theme toggle button (sun/moon icons) in the nav - Fix pre-existing dark-theme inconsistencies: hardcoded light-gray hover border on .faq-item, hardcoded light-red badge on dark bg, hardcoded rgba surfaces in navbar/glass-card/arch diagram now use theme variables - Remove dead #particles-canvas element (unused, no JS ever targeted it) - Reduce mobile section/page-header/cta-box padding for less excessive scroll on small screens - Bump theme-toggle touch target to 44px on mobile (a11y) - Verified: no horizontal overflow on any of the 8 pages at 375px in either theme, CSS braces balanced, JS syntax valid, build.sh still produces a clean dist/
…l bugs Design system rebuilt from scratch (not a re-skin): - New type pairing: IBM Plex Mono (headings/labels, display moments) + IBM Plex Sans (body, card-level headings) — replaces the generic Inter+Outfit pairing - New palette: two deliberate greens (forest for light-mode text-safe use, moss for dark-mode legibility) plus the historic Android bugdroid yellow-green kept strictly as a decorative accent — not a single bright accent on near-black - Removed the h1 gradient-text effect and the pulsing gradient-blob behind the video (both common generic-AI-template tells); solid accent color and a plain video frame instead - Bracket-framed buttons ([ Download APK ]) as the one deliberate signature device, tied to the product being a command/agent tool — reserved for real actions only, not decorative badges - Tightened radius scale (6/10/14/20px, was 8/12/16/24) — considered rectangles instead of pills everywhere - .glass-card is now a solid surface; the frosted/blur treatment is spent once, deliberately, on the architecture diagram only Nav / accessibility fix (the reported complaint): - Theme toggle moved out of the collapsible mobile menu into a persistent nav-controls cluster — visible and reachable in one tap on every page, every viewport, whether the menu is open or not (verified: in-viewport on all 8 pages x both themes) - Toggle rebuilt as a labelled button (icon + Dark/Light text), not a bare 38px icon circle - Mobile menu rebuilt from a 280px translucent blurred sidebar into a full-screen solid takeover panel with larger tap targets Real bugs found and fixed during the rebuild: - contributor.html had a full page-specific <style> block (235 lines) duplicating a mini design system with hardcoded hex colors (#3b82f6 blue, #fef3c7 amber) that ignored the theme system entirely — dark mode showed light-mode-only badge colors. Migrated into style.css as the single source of truth, retoned to the actual palette, role badges simplified to one quiet chip style instead of three clashing hues - .divider (used once, between Contributors and Testers) had no matching CSS rule anywhere and rendered as nothing — now a real rule - features.html's nav Download button was missing the icon and aria-label present on the other 7 pages — aligned - --text-muted used the identical hex in both themes, undershooting WCAG AA against bg-secondary (4.34:1 dark, 3.75:1 light) — split into two theme-appropriate values (6.02:1 / 5.41:1 verified) - navbar scroll shadow was set via inline style with a hardcoded light-mode-only rgba; now a class + theme-aware CSS variable - toggle aria-labels were in French on an all-English site; fixed - removed dead code: unused @Keyframes float, and the video-glow blob element/keyframe now that it's gone from the design Verified: contrast ratios computed against actual rendered colors (not assumed), no horizontal overflow on any of the 8 pages at 390px in either theme, toggle button confirmed in-viewport on all 16 page/theme combinations, FAQ accordion and nav dropdowns still functional, zero console/page errors, build.sh output unchanged in structure.
Website: light/dark mode + nav accessibility fix + real bug fixes
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
The crash log is a sharing sink, not just a storage one. A record is rendered on screen, copied to the clipboard, and put into an ACTION_SEND intent aimed at an arbitrary third-party app, so anything a provider echoed back into an exception message travels with it. Confirmed carrier: OpenAIProvider.kt:65 builds its failure message straight out of the raw response body, and #40 established that OpenAI's 401 body echoes the submitted key's real first 8 and last 4 characters. GeminiProvider.kt:85 puts its key in the request URL query string. Both land in Throwable.message, which CrashReportFormatter.messageOf returned verbatim and printStackTrace embeds in the stack trace, from where CrashLogRecorder writes it to crash_logs and CrashReportExporter renders it into the shared text. Scrubbing happens at capture rather than at export, so the secret never reaches the database and there is no unredacted copy to leak later. It also runs before truncation, so a secret straddling the 16,000-character cut cannot leave its head behind in a form the pattern no longer matches. CrashLogRedactor is deliberately conservative - contextual rules for URL query parameters, auth headers and JSON credential fields, plus vendor key prefixes. A blanket high-entropy-token rule would shred ordinary stack frames. It is a mitigation, not a guarantee: an unprefixed key (Cohere's are bare alphanumerics) still gets through. The project-wide rule is #40; this is the crash-log sink's share of it and should be folded in when that lands. Fixing the 11 providers that build messages out of raw response bodies is separate work on its own branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Bearer rule rewrote "Authorization: Bearer <token>" to "Authorization: Bearer [REDACTED]", and the header rule then ran over that output and matched "Bearer" itself as the header value, producing "Authorization: [REDACTED] [REDACTED]". The token was gone either way, but the "Bearer " prefix was not preserved, which CrashLogRedactorTest asserts. Add a negative lookahead so the header rule skips a value that the Bearer rule already handled, and skip an existing [REDACTED] so the rule set stays idempotent.
Two findings from review. Share all: the export concatenated every stored record, up to 50 records carrying stack traces of up to 16,000 characters each. That approaches 800,000 characters in Intent.EXTRA_TEXT, which overruns the Binder transaction buffer and throws TransactionTooLargeException instead of opening the chooser. export() now spends a character budget from the newest record down, states how many older records it left out, and truncates rather than drops the newest record when that one alone overruns the budget. Retention: pruneToMostRecent ordered by timestamp, which comes from the wall clock and can move backwards. A device correcting a clock that had been set into the future gives the newest row the largest id but an older timestamp, so the row for the crash that just happened sorted outside the keep set and was deleted immediately. Retention now orders by id; the read queries still order by timestamp for presentation.
|
Review addressed in Share large logs outside Intent extras (codex, P2) — valid. 50 records × the 16,000-character stack limit approaches 800,000 characters in
Prune by insertion order rather than wall-clock time (codex, P2) — valid. Separately, the red CI run. Local verification: |
Closes #17.
Scope
Issue #17 was title-only — no body, no comments — so the scope below was chosen and is open to correction:
What this does
The app had no crash handling at all — no uncaught exception handler, no crash reporting SDK — so a crash left nothing behind but a logcat line the user could not reach.
OpenDroidCrashHandleris installed first inOpenDroidApp.onCreate, records the crash, then delegates to the system handler so the process still dies normally. Crashes go to a newcrash_logsRoom table (migration 6 → 7, purely additive) capped at the 50 most recent, and are viewable, shareable, copyable, and clearable from the new screen.Everything stays on device: no SDK, no network egress.
Design notes
Two deliberate choices worth flagging for review:
runBlockingon a suspend DAO is load-bearing. Room dispatches suspend queries to its own executor, so its "cannot access database on the main thread" guard does not fire. Most crashes are main-thread crashes, so a blocking DAO call would fail exactly when it is needed. The write is bounded by a shared 2s monotonic budget so a wedged database turns a crash into a lost report rather than an ANR.Throwable, includingError. Failing to log a crash must never replace a real, diagnosable crash with one from the crash logger itself.The recording path is split so its logic is testable without Android:
CrashReportFormatterandCrashReportExporterare pure,CrashLogRecordertalks to aCrashLogSinkinterface, and onlyRoomCrashLogSinktouches Room.Testing
Not run locally. The machine this was written on has no JDK and no Android SDK, so nothing here was compiled and no test was executed by hand. 46 unit tests across 4 files are included but were unverified at the time of writing.
In place of a compiler, the change went through two static review passes (standards + spec). Those caught and fixed a missing copy action, a 4s worst-case crash-path stall, a
StateFlowconvention mismatch, a misleading comment, and two dead methods — see0afcc51.The "please run it before merging" ask above is now automated — see the next section. This PR's own CI run is the first execution of the suite. Read it before merging.
CI (
b5dbc5c)Writing 46 tests that nothing runs is most of the way to writing none. The repo's only workflow deployed the marketing site; nothing compiled the app or ran a test. So
app/src/test— sixteen files, four of them the crash log ones above — ran only when someone remembered to. The "restore Android debug build compilation" fix a few commits back is what that gap costs..github/workflows/android-ci.ymlrunstestDebugUnitTest assembleDebugon pull requests tomainand pushes tomain.JavaVersion.VERSION_21/jvmTarget '21'inapp/build.gradle.mainwrites the Gradle cache, so pull requests read one warm shared cache instead of each seeding its own.permissions: contents: read— deliberately not inheriting thecontents: writethatdeploy.ymlneeds forgh-pages.gradle.propertiesis gitignored, sohasSigningConfig(app/build.gradle:29) is false in CI andassembleDebugnever reads the release signing config.paths-ignoreforwebsite/**,docs/**,**.md; concurrency cancels superseded runs.if: always(), so a red run is diagnosable from the run page without reproducing it.Verified without a JVM: the workflow YAML parses,
gradlewis mode100755in the index, andgradle-wrapper.jaris tracked — so the wrapper bootstraps.Lint and instrumented tests are deliberately out of scope. A first
lintrun on a codebase that has never had one lands a large backlog needing triage or a baseline, which would block this. Instrumented tests need anandroidTestsource set that does not exist yet.Known follow-up
Tracked in #37.
The six device/app fields are a data clump:
@Embedded val device: DeviceMetadatawould collapseCrashLogEntityand both mirror-image mappers inRoomCrashLogSink(~30 lines), with column names and migration SQL unchanged. Skipped here only because it touches Room codegen and could not be compile-checked.The Room 6 → 7 migration still has no automated coverage — it needs Robolectric or an emulator, neither of which this PR sets up. That is the largest remaining hole, and the CI added here does not close it.
🤖 Generated with Claude Code