KMP phase 6, the driver flip: Android moves to BundledSQLiteDriver - #240
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
278b99e to
7459530
Compare
|
@claude review |
|
@codex review |
|
Claude finished @stslex's task in 2m 21s —— View job Review complete
This is a clean, narrowly-scoped driver flip — every code-side One real issue: two now-stale comments this PR should have updated but didn't, both directly contradicted by this PR's own change:
Neither is a functional bug (build correctness doesn't depend on comment text), but both are exactly the kind of drift this PR's own stated philosophy ("measured, not assumed"; a device-restore risk register that flags rather than silently carries) argues against leaving behind, and both are one-line fixes. No other issues found — dependency wiring ( |
![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: 74595302d0
ℹ️ 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".
|
Review classification before the next push:
|
7459530 to
a00c91f
Compare
|
Implemented the correct stale-comment findings in |
|
@claude review |
|
@codex review |
|
Claude finished @stslex's task in 1m 22s —— View job Review complete
This is a re-review after
No new issues introduced by the fix commit — it's comment-only, and every comment now matches the code it sits next to. On the outstanding Codex P1 finding ( No other issues found in this pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a00c91fbe8
ℹ️ 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".
|
Review classification before the next push:
|
…d, behind its own bisect point buildAppDatabase moves to BundledSQLiteDriver (SQLite 3.50.x on every device; main-db and WAL formats frozen, so existing installations open unchanged), and the flip's oracle moves with it: all 5 device-test classes and the app:app in-memory provider (MetroTestRule) now run the production driver. The Robolectric surfaces deliberately do NOT flip, and the reason is measured, not assumed: the bundled android variant carries Android-ABI natives only, and loading it on a desktop JVM dies with UnsatisfiedLinkError (probed on ExerciseDaoGetAllTest before deciding). RepositoryTestEnv, BaseDatabaseTest and the snapshot host test pin AndroidSQLiteDriver with the reason in place — Robolectric was already inadmissible as a driver oracle (two recorded false negatives on rollback). Pre-flip audit of all 105 @query sites + every raw-SQL site found zero version-sensitive constructs and zero double-quoted string literals (the classic DQS breakage); the one pre-existing arbitrary-row query (getRecentSessionsForExercise's bare column under GROUP BY) is recorded, not regressed. SessionDao's PR-batch comment now states ROW_NUMBER() as available-but-not-taken; the snapshot package's framework-SQLite peek and checkpoint interop is documented at the builder and remains the device-restore pass's territory. Gate, under the flipped driver: 28/28 device tests green on Pixel_6_API_34 (the 7 migration cases apply Migration6 to a real v5 file and validate against 6.json), the same 28 having run green under AndroidSQLiteDriver one commit earlier — both drivers, as the spec's §6 requires. app:app device Regression 41/41 (35 pinned + #232's six). Host tests unchanged: database 128, exercise 212. Full forced gate 3248/3248, detekt separate 56/56, repo counts identical to the parent commit (2230+165, 0 failures), goldens 456/0 touched, schemas untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011kNU4XxzXp6NpCnVUQA6yH
a00c91f to
5362cc7
Compare
|
@claude review |
|
@codex review |
|
Claude finished @stslex's task in 1m 8s —— View job Review complete
This is a re-review after
Both stale-comment fixes from the earlier passes hold up against the current code:
On the outstanding Codex P1 ( No new issues found in this pass — the outstanding items from earlier reviews are all either fixed and verified, or correctly deferred with a live implementation already staged in the next stacked PR. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…teDriver (#239) **KMP phase 6, increment D: `core:data:database` becomes a KMP module, on `AndroidSQLiteDriver`.** The centrepiece of phase 6's second half (#232 carried the spec; #235 was this PR's step 1). The port only — the `BundledSQLiteDriver` flip is deliberately NOT here; it follows as its own PR with its own gate, so a SQLite regression and a source-set regression stay distinguishable under bisect. **The schema did not move: the regenerated `6.json` is byte-identical, `identityHash 9a848458f02ef4c4ace9841b9f2e3d42`.** ## The shape — five commits, bisect-green each 1. **`epochToIso` leaves `java.time`** — `kotlin.time.Instant.fromEpochMilliseconds(ms).toString()`, the zero-new-dependency stdlib replacement, measured byte-identical against `java.time` for whole-second / zero / fractional / negative epochs. Pinned twice: new literal-string tests, and `DatabaseJsonExporterImplTest` keeps `java.time` on the test side **deliberately** as a cross-library oracle. Mutation M-ISO1 (`fromEpochSeconds`) reds 4 tests — the epoch-zero pin alone survives, since 0 s and 0 ms name the same instant; reverted. This makes `WorkoutExportMapper` commonMain-eligible: androidMain ends at 4 files, not the spec's 5. 2. **Fifteen reflective builder sites take the generic form** — `Room.databaseBuilder<AppDatabase>(context, name)` etc., while the module is still Android-only (the reified overload falls back to reflection until `@ConstructedBy` exists — so this commit is pure mechanics, bisectable on its own). The 16th site was already the Room 3 KClass form; the spec's total said 15, measured 16. Phase-7 note: with `Class<T>` gone, a bare `ApplicationProvider.getApplicationContext()` argument deadlocks inference against the builder's factory overloads — pin as `getApplicationContext<Application>()`. 3. **The device-test component and its two CI names** — `withDeviceTest` unconditional in the KMP convention (classic-AGP parity: every library carries the component whether or not sources exist) plus `assembleDebugAndroidTest → assembleAndroidDeviceTest` **and** `connectedDebugAndroidTest → connectedAndroidDeviceTest`. **The second alias is a finding, not a plan item**: `ui_tests.yml` invokes `connectedDebugAndroidTest` literally, so the pre-diagnosed assemble alias alone would build a device-test APK the weekly suite never runs — the **fifth** silent-vanish instance of this arc. Proven both directions: on the parent commit both repo-wide graphs contain **0** `*AndroidDeviceTest` tasks; with the commit, both are fully wired for every KMP module, and an **empty** KMP device-test APK runs green on-device under the Regression annotation filter (the exact ui_tests.yml invocation), so the three source-less KMP modules cost the weekly suite nothing. `RoomLibraryConventionPlugin` keeps one plugin id and branches (KMP: room bundle + `paging-common` in commonMain, `sqlite-framework` in androidMain, `kspAndroid`/`kspIosSimulatorArm64`, room-testing on the device suite). 4. **The conversion** — 51 files commonMain / 4 androidMain (`buildAppDatabase`, `snapshot/`×3: Context, framework SQLite, `java.io.File`), 26 test classes → `androidHostTest`, 5 → `src/androidDeviceTest/kotlin`; `@ConstructedBy` + a **bare** `expect object AppDatabaseConstructor` (see below); detekt.yml gains `**/androidDeviceTest/**` beside every host-test glob, in this commit because that is when the sources exist (spec §3.4). 5. **The spec records what PR D measured** — new §9 in `kmp-phase-6-data-layer.md`; two of the spec's own §3 claims dated in place. ## What the first instrumented conversion surfaced — each measured, each fixed at the root - **Room-KMP puts no schemas on the device-test APK.** All 7 `MigrationTestHelper` tests failed on device, loudly: `FileNotFoundException … Missing file: …/5.json`. The classic integration's androidTest-assets copy has no KMP counterpart, so the Room convention's KMP branch wires `schemas/` as a static device-test asset via the variant API — **which is null until `androidResources.enable = true`**: AGP-KMP defaults the entire asset pipeline off (measured: `sources.assets == null` before, six schemas inside the APK after). - **`checkAndroidDeviceTestAarMetadata` fails on the first KMP device APK that depends on an Android-convention AAR** ("requires core library desugaring"). The KMP convention now mirrors `configureKotlinAndroid`'s repo-wide desugaring (flag + `desugar_jdk_libs`). - **Lint does not classify KSP output as generated on KMP.** `checkGeneratedSources = false` is already set and honored on classic modules; here a consumer with `checkDependencies = true` red **1087 RestrictedApi + 6 SyntheticAccessor — 100% under `build/generated`, zero in handwritten code**. Encoded in `lint.xml` as per-issue `**/build/generated/**` ignores. Mutation M-LINT1 (break the path) brings back exactly 1093; reverted. - **The repo's own gates policed this PR, twice.** `NoActualForExpectSuppressionRule` rejected the `NO_ACTUAL_FOR_EXPECT` suppression I first wrote on the expect object — and it is right: the metadata compilation never demands an actual, the platform compilations get Room's generated ones, and without the suppression a codegen failure reds the build instead of vanishing. And detekt, now seeing device tests through the KMP source-set model, surfaced two pre-existing `Wrapping` violations in `AtomicRollbackDeviceTest` — **`src/androidTest` was never full-detekt-checked on classic modules** (default sources are `src/main` + `src/test`), the same shape as #235's testFixtures hole. - **Room 3 schema export is write-on-diff** — the processor writes into the plugin's `schemaOutput` only when the generated schema differs from `schemaInput` (= the committed `schemas/`), so a production build can never silently rewrite the schema directory. The §6 gate ran as prescribed: schemaInput redirected to an empty dir via an out-of-tree init script, fresh `6.json` exported, byte-identical. - **`setQueryCoroutineContext` deliberately NOT added** (deviation from spec §3, argued): the Android builder runs without it today, adding it would change which context Room queries use — not "the port only" — and nothing constructs a database off-Android until phase 7's iOS composition root, which is where it belongs, next to iOS's driver choice. - One commonMain-ism: `String.toByteArray(Charsets.UTF_8)` is JVM-only → `encodeToByteArray()` (one site). ## A counting correction, in the open The repo-wide unit-test invariant number (2352 at #235) was produced by a `test-results/testDebugUnitTest` glob — which **never saw KMP modules' `testAndroidHostTest` results** (core:core 32, dataStore 5). Execution was never affected (CI's alias runs them); only the counting was blind, on both sides of every comparison, so past deltas stayed honest. From this PR on the number is reported in both scopes. ## Gates All `--rerun-tasks --no-build-cache --no-configuration-cache`; baseline measured on dev@c81c4490 the same way. | Gate | Result | |---|---| | full forced gate (assembleDebug testDebugUnitTest verifyPaparazziDebug lintDebug assembleDebugAndroidTest) | ✅ `3248 actionable tasks: 3248 executed`, 0 failed (dev baseline: 3080/3080; the +168 is the KMP module's task set + device-test components) | | detekt (separate invocation) | ✅ `56 actionable tasks: 56 executed` | | unit tests | ✅ **2230 tests / 230 classes** (`testDebugUnitTest` scope) + **165 / 33** (`testAndroidHostTest` scope) = **2395**, 0 failures, 0 skipped, 0 stale XMLs. Accounting vs 2354 baseline: +4 new epochToIso pins, +37 previously uncounted KMP host tests, −0 lost; database's 128 moved scope, not existence | | device suite | ✅ **`Starting 28 tests on Pixel_6_API_34(AVD)`, 28/28 green** — via the `connectedDebugAndroidTest` alias under the exact ui_tests.yml Regression annotation filter; red direction observed live (7 migration failures while schemas were missing from assets) | | instrumented suite gate | ✅ `:core:data:database: 5 instrumented source files, 206 classpath entries scanned, 0 missing` — resolved through `androidDeviceTestRuntimeClasspath` | | schema identityHash | ✅ regenerated-from-empty-input `6.json` **byte-identical** to committed; `git status schemas/` empty | | iOS | ✅ `compileKotlinIosSimulatorArm64` + `kspKotlinIosSimulatorArm64` + klib in the same repo-wide graph (the `assembleDebug` alias pulls them) | | aliases both directions | ✅ parent commit: 0 `*AndroidDeviceTest` tasks in either repo-wide graph; this branch: fully wired | | goldens | ✅ **456 on disk, 0 touched** (446 in older docs is stale — #236/#237 added 10 on dev before this branch) | Stacked work: #240 (the `BundledSQLiteDriver` flip) sits on top of this branch; PR E (`core:data:exercise` → KMP) will stack on #240. Waiting on review here does not block that work. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011kNU4XxzXp6NpCnVUQA6yH
…roidMain (#241) **KMP phase 6, increment E: `core:data:exercise` becomes a KMP module — with zero androidMain. Stacked on #240** → #239. Review in that order. The last code increment of phase 6. Four commits: the one genuine behaviour change (red-first, on device), the wall-clock swap, the pure-mechanics conversion, and the hook-deployment fix the conversion's own near-miss surfaced. ## 1. The behaviour change the spec predicted, observed live before it was fixed Spec §5 called `catch (SQLiteConstraintException)` in `ExerciseRepositoryImpl.saveItem` *control flow, not a type name*: under a Room 3 driver the constraint violation arrives as a different type, the catch silently stops matching, and the exception escapes — with no compile error. That is exactly what happened. A new on-device duplicate-name test (this module's first device test, `@Regression`, in-memory DB under the flipped `BundledSQLiteDriver`) ran **red against the unfixed repository**, escaping with: ``` android.database.SQLException: Error code: 2067, message: UNIQUE constraint failed: exercise_table.name ``` The fix is smaller than the spec's own sketch, because of a fact worth keeping: **on Android, `androidx.sqlite.SQLiteException` is `actual typealias … = android.database.SQLException`** (read from the 2.7.0 sources jar). So one common catch covers the framework driver's `SQLiteConstraintException` (a subtype), the bundled driver's own throw (measured above), and iOS's native actual — no expect/actual seam needed at all. The constraint check gates on the one string both engines emit (`UNIQUE constraint failed`), because the common type has no code field and the framework subtype is not nameable from common code. Proven in all four directions: device red on the old catch → green on the new; the Robolectric twin (framework driver) green throughout; mutation **M-DUP1** (never-matching string) reds **both** legs — 1 of 54 host tests, 1 of 1 device — reverted. ## 2. Mechanics - **Eight `System.currentTimeMillis()` sites** → `kotlin.time.Clock.System.now().toEpochMilliseconds()` (stdlib, same wall clock; its own commit so the conversion stays pure moves). - **The conversion**: all 42 production files → commonMain. **No androidMain** — after the catch fix, the module had no platform type left; the paging surface lives in `paging-common` (verified `iosSimulatorArm64` published). 19 test classes + seed helper → `androidHostTest`, the device test → `src/androidDeviceTest/kotlin`. The dead `compose-bom`/`compose-runtime`/`room3-runtime` declarations (zero usages, spec §5) do not return; `paging-runtime-ktx` → `paging-common`; the new `androidx-sqlite` (base) alias supplies the exception type the database module's `implementation` deps rightly don't leak. - The whole PR-D convention machinery ran unchanged for the second module: device-test component, both CI aliases, schema-asset wiring (n/a here), desugaring, detekt device-test globs. **Nothing new was needed** — the first conversion paid; the second rode. ## 3. A process finding, in the open `setup-hooks.sh` **copies** `.githooks/pre-commit` into `.git/hooks/` — it does not set `core.hooksPath`, though CLAUDE.md says it does. Consequences, both real: a clone that never ran the script has **no hook at all** (this session's clone — caught when a red detekt run and a passing commit coexisted; the separate forced detekt invocations in this arc's process were the actual gate all along), and a clone that ran it **before #235 still carries the ACM filter** the fixture-move PR proved blind to renames. Fixed here: the script now sets `core.hooksPath` to the tracked `.githooks/`, so hook fixes deploy with `git pull` instead of with a ritual — which is what CLAUDE.md already claimed. Reported rather than silently corrected. ## 4. The §7 riders, re-priced with numbers (closing the phase brief's ask) - **`java.time`**: production `core/data` is now clean — `WorkoutExportMapper`'s line was taken in #239. What remains: 1 test file (`DatabaseJsonExporterImplTest`, deliberately kept as a cross-library oracle) and 14 `feature:exercise-chart` files, which are Phase 7's by the spec's own split. **This rider is discharged for phase 6.** - **`java.io.File` → `okio.Path`**: the dependency half is now free (okio rides the graph via `datastore-preferences-core` since #233; using it in signatures needs only a toml alias). The remaining cost is unchanged in kind: a signature ripple across 7 modules (backup:api, google-drive, worker, database, recovery, settings, app:app). And its urgency **dropped** with this phase: after #239 every `File`-typed surface sits in an androidMain of a KMP module — nothing common is blocked by it. Still correctly deferred until a real iOS backup implementation makes the right shape observable; `android.net.Uri` still has no common analogue (SAF). ## Gates All `--rerun-tasks --no-build-cache --no-configuration-cache`. | Gate | Result | |---|---| | device: duplicate-name under bundled | ✅ red on old catch (escape above) → 1/1 green via the `connectedDebugAndroidTest` alias under the ui_tests.yml Regression filter | | host tests | ✅ exercise 212/19 (now `testAndroidHostTest` scope), feature:exercise 113/9, exercise-chart 66/8, 0 failures | | full forced gate (assembleDebug testDebugUnitTest verifyPaparazziDebug lintDebug assembleDebugAndroidTest) | ✅ `3265 actionable tasks: 3265 executed`, 0 failed | | detekt (separate invocation) | ✅ `57 actionable tasks: 57 executed` — the 57th is the suite gate over this module's new device test, previously NO-SOURCE | | repo-wide unit tests | ✅ **2018 / 211** (`testDebugUnitTest` scope) + **377 / 52** (`testAndroidHostTest` scope) = **2395**, 0 failures, 0 skipped — identical total to #239/#240; exercise's 212 moved scope, not existence | | iOS | ✅ exercise klib + `kspKotlinIosSimulatorArm64` in the repo-wide `assembleDebug` graph | | goldens | ✅ 456, 0 touched | 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011kNU4XxzXp6NpCnVUQA6yH
1 participant
KMP phase 6, the driver flip: Android moves to
BundledSQLiteDriver. Stacked on #239 — review that first; this is one commit on top, deliberately separate so a SQLite-behaviour regression and a source-set regression stay distinguishable under bisect, and so this half reverts alone. #241 (core:data:exercise → KMP) stacks on top of this PR.What flips, what doesn't, and why the line is where it is
buildAppDatabase)InMemoryDatabaseProvider(app:app androidTest viaMetroTestRule)RepositoryTestEnv+ host tests (BaseDatabaseTest, snapshot test)UnsatisfiedLinkError. Robolectric is not an admissible driver oracle in this repo anyway (AtomicRollbackDeviceTestKDoc: two false negatives)The Room convention's KMP branch now declares
sqlite-bundledfor androidMain; the database module pinssqlite-frameworkon its host tests explicitly, next to the reason.The gate — the migration suite on a real device, under the flipped driver
BundledSQLiteDriveron Pixel_6_API_34 (arm64), collected count printed (Starting 28 tests), via theconnectedDebugAndroidTestalias under the exact ui_tests.yml Regression filter. That includes all 7AppDatabaseMigrationTestcases — a real v5 file seeded on disk,Migration6applied, schema validated against6.json— plus the atomicity-rollback oracle (both production transaction shapes), invalidation across connections, and the on-disk round-trip.AndroidSQLiteDriverin KMP phase 6D: core:data:database becomes a KMP module, on AndroidSQLiteDriver #239 — the spec's "under both drivers" requirement, satisfied across the two PRs by construction.Risk register (from the pre-flip SQL audit of all 105
@Querysites + every raw-SQL site)SQLITE_DQS=0breakage): every SQL literal is single-quoted. Zero version-sensitive constructs in use — no window functions, CTEs, upserts (@Insert(REPLACE/IGNORE)compile to the pre-upsert forms, frozen since 3.6.19), date/time/json/math functions,RETURNING, orIIF.SessionDao.getRecentSessionsForExerciseprojects a bare column underGROUP BYwith no aggregate — legal and arbitrary in both SQLite builds; a planner change may pick a different in-group row when one session holds the same exercise twice. Not a flip regression; noted for the record.snapshot/package opens the live DB via framework SQLite (peek + pre-migration checkpoint) while Room holds bundled connections. Main-db and WAL formats are frozen, so reads are safe by format; the concurrent-open path leans on POSIX locking shared by both libraries. This is the residual risk the device-restore pass (Ilya's gate, per the recovery spec) covers end-to-end; flagged here rather than silently carried.ROW_NUMBER()becomes available —SessionDao's PR-batch comment now records the rewrite as available but not taken (this PR is behaviour-neutral by design).Gates
Starting 28 tests on Pixel_6_API_34(AVD)3248 actionable tasks: 3248 executed, 0 failed56 actionable tasks: 56 executed🤖 Generated with Claude Code
https://claude.ai/code/session_011kNU4XxzXp6NpCnVUQA6yH