ReviewOS

craft-native/craft

Nothing in CI compiles the Kotlin templates, and #156 shipped three type errors because of it

#163
Closed glennmichael123 opened this 10 days ago ·
10 days ago

packages/android/templates/*.kt.template is never compiled by anything in this repository. The only workflow that touches Android is mobile-e2e.yml, which is workflow_dispatch-only and whose Android job is guarded by if [ -x "packages/android/gradlew" ] — a file that does not exist. packages/android has no Gradle project at all: it is README.md, build.ts, package.json, src/ and templates/.

So every edit to CraftBridge.kt, CraftNative.kt and the six other templates is verified by reading it, and by nothing else.

What that cost, concretely

#156 rewrote 58 evaluateJavascript call sites to route through a new helper:

private fun jsQuote(value: String?): String = JSONObject.quote(value ?: "null")

Three of those sites do not pass a String:

siteexpressionactual type
addContactContentUris.parseId(results[0].uri!!)Long
downloadFiledownloadManager.enqueue(request)Long
authenticateonAuthenticationError(code, errString)CharSequence

String interpolation accepted all three; a String? parameter does not. The template would not compile, every check on the PR was green, and it merged.

Fixed in the follow-up by widening the parameter to Any? — which is what interpolation always was — but the fix is not the point. The point is that a whole language in this repository has no compiler behind it, and the bug that found it was a mechanical rewrite of the sort that will happen again.

What would catch it

A Gradle project under packages/android that substitutes the template placeholders and compiles the result. It needs the Android SDK, which android-actions/setup-android@v3 already provides and mobile-e2e.yml already uses, and it needs and the other placeholders filled with something — the same substitution build.ts does when it generates an app.

assembleDebug on a generated app is the honest check, since that is what a user's build does. compileDebugKotlin alone is cheaper and catches this entire class.

Why this is not filed as a patch

It adds an Android SDK download and a Gradle build to CI, which is minutes on every run, and where it goes — its own workflow, a job in ci.yml, or a nightly — is a call about what the project wants to pay. Worth deciding deliberately.

The interim guard

android_reply_escaping_test.zig now asserts the signature is Any? and says why, so narrowing it back fails a test rather than a user's build. That covers exactly one regression and nothing else — it is a note, not a substitute.

10 days ago

Closing: CI now compiles the Kotlin templates, on every pull request and every push to main.

Where. ci.yml runs on pull_request and push to main with no path filter. Its android-builder job has a step, "Compile generated Kotlin projects", which runs bun run test:templates (packages/android/scripts/compile-templates.ts). It was added in 6f3a4d7 on 2026-09-14. The job sets up JDK 17, the Android SDK (platforms;android-36, build-tools;36.0.0) and Gradle.

What it compiles. Two projects are generated through the real init(), with every placeholder substituted, the same way an app is generated:

  • minimal, whose name is Craft & "Kotlin" $Build so escaping in the templates is exercised too;
  • capabilities, with every capability on: Firebase, Health Connect, background location, deep links and the rest. Conditional imports and conditional source templates therefore cannot hide from the compiler.

Each runs :app:assembleDebug :app:assembleRelease :app:testDebugUnitTest, and the unit test compiles CraftPermissionPolicyTest.kt against the generated bridge. The issue suggested assembleDebug on a generated app as the honest check. This does that for both configurations, plus the release build.

Evidence that it catches exactly this class of bug. Today, on #210, a template edit added import android.app.PendingIntent to CraftBridge.kt.template, which already imported it further down. Every other check was green. That step failed the PR (run 35099434537, job 104804910883):

e: file:///tmp/craft-android-templates-4Nxz7D/minimal/app/src/main/java/dev/craft/fixture/minimal/CraftBridge.kt:5:20
   Conflicting import: imported name 'PendingIntent' is ambiguous.
BUILD FAILED in 40s

It was fixed before merge. Before this job existed, that would have shipped the way #156's three type errors did.

In addition, mobile-e2e.yml's Android emulator job builds a generated app with Gradle and runs it on an emulator, twice per run.

The interim guard mentioned in the issue, android_reply_escaping_test.zig asserting jsQuote(value: Any?), is left in place. It costs nothing and says why the signature is what it is.

Sign in to comment on this issue.