Repository navigation
feat(android): native Android client for ompweb - #333
RoyaLemonade wants to merge 9 commits into
Conversation
Kotlin/Jetpack Compose app for ompweb: sessions, live chat, composer with slash commands and dictation, files, Git and worktrees, notifications with inline answers, and the full settings surface. Connects to a server that serves GET /api/app-info (API 1); the session cookie is the only stored credential (AES-GCM, Android Keystore, bound to one origin). Plain HTTP is limited to private networks and enforced on every request and redirect hop. Includes the JVM test suite, the golden screenshot matrix, and the Gradle wrapper.
Adds the user guide (install, first connection, notifications, updates, APK verification, privacy), the RPC/frame contract the client implements, and the release notes for the current version. README and AGENTS gain the Android section: build/test/lint commands, the golden matrix, the private-network HTTP policy, the cookie model and the signing rules.
android.yml assembles, runs the JVM tests and lints on every change under android/. android-release.yml signs and publishes the APK for an android-v* tag: the key comes from repository secrets into the android-release environment, the tag must match the APK's versionName, and the release only accepts the expected signing certificate. .gitignore keeps signing material out of the tree and scopes the root-level screenshot ignore so the app's committed goldens survive; eslint ignores the vendored WebView bundles.
WalkthroughWarning Review details and warnings were omitted to fit the comment limit. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| first.host.equals(second.host, ignoreCase = true) && | ||
| first.port == second.port | ||
|
|
||
| private fun originKey(url: HttpUrl): String = |
There was a problem hiding this comment.
🔥 The Roast: originKey builds from url.host as-is. sameOrigin lowercases for comparison, but the stored/origin tags don't - if a configured origin ever comes in mixed case, tag equality (tag != originKey) could misclassify and let cookies leak across case-variants or be mishandled.
🩹 The Fix:
| private fun originKey(url: HttpUrl): String = | |
| private fun originKey(url: HttpUrl): String = | |
| "${url.scheme}://${url.host.lowercase()}:${url.port}" |
📏 Severity: warning
|
|
||
| /** Whether plain HTTP to [address] stays inside a local or private network. */ | ||
| fun isPrivatePeer(address: InetAddress): Boolean { | ||
| if (address.isLoopbackAddress || address.isSiteLocalAddress || address.isLinkLocalAddress) { |
There was a problem hiding this comment.
🔥 The Roast: You're installing a single shared PlainHttpPolicyListener instance via eventListener(...). OkHttp's listener is stateless here, but it's cleaner and safer to use a factory - avoids any accidental shared state if this ever gains fields.
🩹 The Fix:
| if (address.isLoopbackAddress || address.isSiteLocalAddress || address.isLinkLocalAddress) { | |
| .eventListenerFactory { PlainHttpPolicyListener() } |
📏 Severity: suggestion
| ): Boolean = request.url.host != "appassets.androidplatform.net" | ||
|
|
||
| override fun onRenderProcessGone( | ||
| view: WebView, |
There was a problem hiding this comment.
🔥 The Roast: shouldOverrideUrlLoading only blocks navigation unless host is appassets.androidplatform.net. With JS enabled in a sandboxed WebView that loads only bundled assets via WebViewAssetLoader, this is probably fine - but it's a little permissive if anything ever slips past the asset loader. Defense in depth wouldn't hurt.
🩹 The Fix: Explicitly deny all navigations except the asset origin (e.g., return true unless request.url.scheme == "https" && request.url.host == "appassets.androidplatform.net" and it's an expected path), or just return true to block everything.
📏 Severity: nitpick
| fun postMessage(value: String) { | ||
| post { | ||
| try { | ||
| val json = JSONObject(value) |
There was a problem hiding this comment.
🔥 The Roast: Exposing a JS interface (ompMermaid.postMessage) to a WebView with JavaScript enabled is necessary for this sandboxed renderer, but it's a high-value bridge. Good job keeping it minimal (only posts stringified JSON back), using @JavascriptInterface, blocking network loads, disabling file/content access, and handling render process crashes.
🩹 The Fix: Consider adding a ProGuard/R8 keep for this specific interface method (you have it) and ensure the interface object isn't retained longer than needed. Also consider @Keep on the anonymous class if R8 gets aggressive. Current setup looks reasonable.
📏 Severity: nitpick
| exit 1 | ||
| fi | ||
| aapt2="$(find "$ANDROID_HOME/build-tools" -name aapt2 -type f | sort -V | tail -1)" | ||
| test -n "$aapt2" |
There was a problem hiding this comment.
🔥 The Roast: version="${GITHUB_REF_NAME#android-v}" trusts the tag string directly and later interpolates it into paths (ompweb-$version.apk) and the release body. A tag like android-v1.0.17/../evil is attacker-controllable by anyone who can push tags - shell won't stop path traversal here.
🩹 The Fix:
| test -n "$aapt2" | |
| version="${GITHUB_REF_NAME#android-v}" | |
| if ! [[ "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | |
| echo "::error::Invalid version in tag $GITHUB_REF_NAME" | |
| exit 1 | |
| fi |
📏 Severity: critical
| sed -n "s/^package:.* versionName='\([^']*\)'.*/\1/p")" | ||
| if [ "$apk_version" != "$version" ]; then | ||
| echo "::error::Tag android-v$version does not match the APK's versionName '$apk_version'" | ||
| exit 1 |
There was a problem hiding this comment.
🔥 The Roast: aapt2 is discovered via find ... | sort -V | tail -1 from the whole build-tools tree. If multiple versions exist or SDK state is odd, you might pick the wrong binary. You just installed build-tools;37.0.0 - pin the exact path.
🩹 The Fix:
| exit 1 | |
| aapt2="$ANDROID_HOME/build-tools/37.0.0/aapt2" | |
| test -x "$aapt2" |
📏 Severity: warning
| # The certificate this repository's users verify against (docs/ | ||
| # android/README.md). A different keystore must not sign a release | ||
| # silently; rotate this value together with the published docs. | ||
| expected_cert_sha256="d73354b52dc7c3f0c74623c439ddb7b52e252811c6e6b5f851c9d04b7015fc87" |
There was a problem hiding this comment.
🔥 The Roast: Same thing for apksigner - find | sort -V | tail -1 is a bit of a lottery when you know the exact build-tools version you need.
🩹 The Fix:
| expected_cert_sha256="d73354b52dc7c3f0c74623c439ddb7b52e252811c6e6b5f851c9d04b7015fc87" | |
| apksigner="$ANDROID_HOME/build-tools/37.0.0/apksigner" | |
| test -x "$apksigner" |
📏 Severity: warning
| test -f "$asset.sha256" | ||
| test -f dist/release-notes.md | ||
| echo "asset=$asset" >> "$GITHUB_OUTPUT" | ||
| echo "sha_file=$asset.sha256" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🔥 The Roast: The publish job trusts the staged artifact from the build job without re-validating version/SHA-256/certificate. If artifacts are tampered with or the pipeline is compromised between jobs, you could publish the wrong APK.
🩹 The Fix: In the publish job, recompute SHA-256 of the staged APK, verify with apksigner verify --print-certs against the expected certificate SHA-256, and confirm the APK's versionName matches the tag before calling action-gh-release.
📏 Severity: warning
| Tailscale addresses ompweb runs on are plain HTTP. The app enforces | ||
| the policy itself in HttpPolicyInterceptor on every request and | ||
| redirect hop, not only in ServerAddress. --> | ||
| <base-config cleartextTrafficPermitted="true" /> |
There was a problem hiding this comment.
🔥 The Roast: Global cleartext allowed at platform level. Your HttpPolicyInterceptor does a valiant job enforcing private-network-only for http and blocking cross-origin redirects, but it's defense-in-depth in reverse - one missed call site or non-OkHttp HTTP client bypasses everything.
🩹 The Fix: Keep it only if unavoidable for .local/Tailscale, but add a build-time/test guard (e.g., custom lint rule) to ensure all network calls go through the configured OkHttpClient. Also consider tightening to domain-config if possible.
📏 Severity: warning
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (1 file)
Previous Review Summaries (5 snapshots, latest commit 5a5120f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5a5120f)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (1 file)
Previous review (commit a8f5d8c)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I had my flamethrower warmed up, but the author actually listened to the review. I need to sit down. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (2 files)
Previous review (commit 2324ec8)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I had my flamethrower warmed up, but the author actually listened to the review. I need to sit down. 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (4 files)
Previous review (commit 11aa42f)Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I had my flamethrower warmed up for rogue 📊 Overall: Like dusting a shelf that no one was using — nothing broke, nothing changed, nothing to roast. Files Reviewed (8 files)
Previous review (commit 4d2c513)Verdict: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The cookie jar does origin binding, generation checks on clear(), and best-effort persistence - proper hardening. 💀 Worst part: Unvalidated tag-derived version in the release workflow is just asking for a bad day. 📊 Overall: Solid Android client with good security instincts, but the release pipeline needs a quick tightening before shipping. Files Reviewed (X files)
Reviewed by free · Input: 31.4K · Output: 4.5K · Cached: 167.1K |
… navigation Review follow-ups on the native client: - `HttpPolicyInterceptor.install()` installs the plain-HTTP listener through `eventListenerFactory`, not as one instance shared by every call on the client, so a listener that ever gains state cannot be shared; - the Mermaid WebView only lets the bundled asset origin under `/assets/` replace the rendered document: a diagram link cannot navigate the sandbox to a foreign origin, plain HTTP, another path of the asset host, or a `data:`/`about:` document. The origin is one constant, used by both the policy and `loadDataWithBaseURL`, and `MermaidNavigationTest` covers the cases. Not changed: the cookie jar keeps using `HttpUrl.host` (OkHttp canonicalises it to lower case, so the stored tag and the comparison key already agree); the platform-wide cleartext allowance stays because every client is built through the policy interceptor; the `ompMermaid` bridge stays as it is (`@JavascriptInterface` `postMessage(String)` only, kept by the R8 rule).
…ublish Review follow-ups on the release workflow: - the tag-derived version must be `major.minor.patch`: the string reaches a file name, the release body and a URL, so a tag like `android-v1.0.17/../evil` is refused in both jobs instead of being trusted; - `aapt2` and `apksigner` come from `$ANDROID_HOME/build-tools/$RELEASE_BUILD_TOOLS` (`test -x`) instead of "latest found under build-tools", which depended on whatever the runner image carried; - the publish job re-derives the staged artifact's claims instead of trusting the build job: SHA-256 against the recorded `.sha256`, versionName via `aapt2 dump badging`, and the signing certificate via `apksigner verify --print-certs`, before `action-gh-release` runs; - the expected certificate is one workflow-level constant used by both jobs, so the two checks cannot drift apart. Committed locally only: the token of this run has no `workflow` scope, so a push of `.github/workflows/**` is refused. Not pushed.
|
Thanks — the two app findings are fixed, the other three are declined with reasons. Fixed (with tests):
Not changed:
The workflow findings from the same review (tag-version validation, pinned |
|
Workflow fixes from the review are now pushed in 2324ec8: the release tag must match ^[0-9]+.[0-9]+.[0-9]+$ (checked in both the build and the publish job), aapt2/apksigner are pinned to the installed build-tools version instead of being discovered with find, and the publish job re-verifies the APK's SHA-256, versionName and signing certificate before creating the release. |
Two bugs made both Android workflows fail before/after the Gradle build: - `android-actions/setup-android` installs `packages: tools` by default, and sdkmanager no longer serves that package, so the setup step failed in android.yml and in both release jobs. Pass `packages: platform-tools`; the Gradle SDK packages come from the existing sdkmanager step. - The release verify step tested `app/build/outputs/apk/release/app-release.apk`, but the release build names its output after the version (`outputFileName` in app/build.gradle.kts), so the file is `ompweb-<version>.apk`. Look for that name, after the validated `$version` exists.
|
The workflow fixes are pushed in
Also in
|
The install steps spelled out ompweb-1.0.17.apk, the verify block pinned V=1.0.17 and apksigner named that file, and the more-documentation list called one notes file "the current release". Every release therefore needed a README edit that could drift from the release body. Point at the release page and the workflow-generated release body instead (they carry the version, the asset names, the size and the digests) and name the release-notes pattern rather than one version. The stable signing-certificate fingerprint stays: it is a property of the project, not of a release. The release checklist records the invariant.
|
The Bun server/daemon architecture has landed on |
|
Closing: the Android client lives in its own repository ( The wire contract it depends on is documented in |
A native Android client for ompweb, written in Kotlin and Jetpack Compose (one
appmodule,applicationIdio.github.kahme247.ompweb). It is a full client, not a WebView wrapper: sessions andprojects (pinned sessions on top), the live chat transcript (streaming, tool calls, subagents,
/btw, compaction notices), the composer (slash commands, attachments, model/thinking/speedcontrols, queue edit/steer, dictation, drafts), files, Git and worktrees, notifications with inline
replies, and the settings tree. Phone, foldable and tablet layouts, light/dark themes and font
scaling; the layout matrix is pinned by 1,080 golden screenshots rendered through Robolectric on the
JVM.
The app is distributed as an APK on GitHub Releases (
android-v*tags); there is no Play Storelisting.
Server requirements
GET /api/app-info,"api": 1). The app refuses an older server with"Server too old" instead of failing later in the flow.
Build
JDK 21 and the Android SDK (platform 37).
./gradlew testDebugUnitTestruns the unit tests,including the golden screenshots;
./gradlew lintDebugruns lint. Minimum Android version is8.0 (API 26).
CI and release signing
.github/workflows/android.ymlrunsassembleDebug,testDebugUnitTestandlintDebugfor everychange under
android/..github/workflows/android-release.ymlbuilds the signed APK on anandroid-v*tag inside theandroid-releaseenvironment (required reviewers). The keystore comes from theANDROID_KEYSTORE_BASE64,ANDROID_KEYSTORE_PASSWORD,ANDROID_KEY_ALIASandANDROID_KEY_PASSWORDrepository secrets; the workflow checks the tag against the APK'sversionNameand the expected signing certificate, then attachesompweb-<version>.apkplus its.sha256to the release. No signing material is committed. Both the secrets and the environmenthave to exist in the repository settings before the first tag.
Docs
docs/android/README.md(install, first connection, notifications, privacy, verifying an APK),docs/android/protocol.md(the wire contract the app implements),docs/android/RELEASE_NOTES_1.0.17.md, an "Android app" section inAGENTS.md, and a short"Android app" section in the root README.
No screenshots are attached inline.