KMP phase 6E: core:data:exercise becomes a KMP module — with zero androidMain

[stslex]

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 KMP phase 6D: core:data:database becomes a KMP module, on AndroidSQLiteDriver #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 KMP phase 6B: core:data:dataStore becomes a KMP module #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 KMP phase 6D: core:data:database becomes a KMP module, on AndroidSQLiteDriver #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.ai/code/session_011kNU4XxzXp6NpCnVUQA6yH

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[github-actions]

Unit Test Results

2 066 tests  ±0   2 066 ✅ ±0   9m 39s ⏱️ + 1m 18s
  237 suites ±0       0 💤 ±0 
  237 files   ±0       0 ❌ ±0 

Results for commit 5de4686. ± Comparison against base commit 5362cc7.

♻️ This comment has been updated with latest results.

[stslex]

@claude review

[stslex]

@codex review

[claude]

Claude finished @stslex's task in 2m 18s —— View job

---

Review complete

• Gather context (PR body, diff, changed files)
• Review behaviour-change commit (ExerciseRepositoryImpl.saveItem catch fix + device test)
• Review mechanics commits (wall-clock swap, commonMain conversion, build.gradle.kts)
• Review setup-hooks.sh fix
• Post final review

I read all six commits (d59aff1, af879d8, f3df653, 91ef5f0, 351708c, 706f308) against origin/claude/database-bundled-sqlite, and checked the runtime shape of the changes, not just the diff.

1. The catch fix (d59aff1 / core/data/exercise/.../ExerciseRepositoryImpl.kt)

Correct, and I verified the load-bearing claim rather than taking it on faith:

• exercise_table has exactly one unique = true index (name, core/data/database/.../ExerciseEntity.kt:14), so the string-match on "UNIQUE constraint failed" inside saveItem's try block can't misfire on some other constraint — the try only wraps dao.insert/dao.getById/dao.update against that one table.
• The old catch (_: SQLiteConstraintException) silently discarded the exception; the new catch (e: SQLiteException) conditionally rethrows non-matching errors (if (!e.isUniqueConstraintViolation()) throw e), which is strictly more correct — it stopped hiding non-duplicate SQLite failures behind the same silent path.
• Grepped the whole module for leftover android.database.sqlite references post-conversion — none outside comments/docstrings.

No issues found here.

2. Wall-clock swap (af879d8)

Pure mechanical System.currentTimeMillis() → Clock.System.now().toEpochMilliseconds() across all 8 sites in ExerciseRepositoryImpl, SessionRepositoryImpl, TrainingRepositoryImpl. No behavior change, correctly split into its own commit as claimed.

3. commonMain conversion (f3df653)

• Confirmed via git diff --stat against the workflows directory that nothing in .github/ changed, matching the PR's claim that the CI wiring needed no new work.
• The extension function isUniqueConstraintViolation() moved from a file-private top-level fun to a private member of ExerciseRepositoryImpl — correctly explained as a SyntheticAccessor fix, and this is the right shape for that.
• Final build.gradle.kts mirrors the established KMP-module pattern (androidHostTest for Robolectric, androidDeviceTest for the device test, no androidMain source set) — consistent with the phase's earlier database-module conversion.
• Device test file (ExerciseRepositoryDuplicateNameDeviceTest.kt) is a pure rename from androidTest → androidDeviceTest, content untouched.

4. setup-hooks.sh (91ef5f0 + 706f308)

The fix (switching from cp into .git/hooks/ to git config core.hooksPath .githooks) is correct and closes a real gap — CLAUDE.md already documented the script as doing this. The follow-up commit adds proper failure handling (if ! git config ...; then ... exit 1) instead of assuming success, which is a good defensive addition given the whole point of this fix is "don't silently end up with no hook." .githooks/pre-commit is 755 in the tree, so core.hooksPath will find it executable.

Minor nit (non-blocking)

documentation/feature-specs/kmp-phase-6-data-layer.md: the new ## §10 heading (added in 351708c) is appended directly after the previous paragraph with no blank line separating them — a cosmetic Markdown formatting slip, doesn't affect rendering meaningfully but is inconsistent with the rest of the doc's spacing.

Summary

This is a clean, well-sequenced conversion. The one genuine behavior fix is narrowly scoped and its safety is verifiable from the schema (single unique index), the mechanical commits are exactly that, and the process fix (setup-hooks.sh) is a legitimate and useful catch reported transparently rather than silently folded in. No correctness, security, or architectural concerns. Nothing here needs a change before merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 706f30856e

ℹ️ 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".

[stslex]

Review classification before the next push:

• Claude Markdown spacing nit — correct. The §10 heading is directly adjacent to the final §9 list paragraph with no separating blank line. I reproduced it in the source and will add the missing blank line in this PR.

[stslex]

Implemented the correct Markdown spacing nit in 1a35bf47. After rebasing onto the updated #240, the exercise host tests, Detekt, and lint were re-executed successfully: 120 actionable tasks: 120 executed.

[stslex]

@claude review

[stslex]

@codex review

[claude]

Claude finished @stslex's task in 2m 4s —— View job

---

Review complete

• Gather context (PR body, diff, prior review comments, changed files)
• Review behaviour-change commit (ExerciseRepositoryImpl.saveItem catch fix + device test)
• Review mechanics commits (wall-clock swap, commonMain conversion, build.gradle.kts)
• Review setup-hooks.sh fix and doc updates
• Check status of prior review nits (Codex, previous @claude pass)
• Post final review

I independently re-verified the load-bearing claims rather than taking the PR body or the earlier @claude review pass on faith.

1. Catch fix + device test — confirmed correct

• ExerciseRepositoryImpl.saveItem (core/data/exercise/src/commonMain/.../ExerciseRepositoryImpl.kt:113-127) now catches androidx.sqlite.SQLiteException and rethrows unless isUniqueConstraintViolation() matches "UNIQUE constraint failed" in the message. Wall-clock swap is complete: grepped the whole module — zero System.currentTimeMillis() remain, all 8 sites now read Clock.System.now().toEpochMilliseconds() (ExerciseRepositoryImpl.kt:182,301,466, SessionRepositoryImpl.kt:235,249,351, TrainingRepositoryImpl.kt:163,245).
• ExerciseRepositoryDuplicateNameDeviceTest.kt is a real, well-isolated device test: fail-fast ImageStorage stub asserts the duplicate path never touches image I/O, and it asserts both the returned SaveResult.DuplicateName and that the original row survived untouched — not just a return-value check.
• No leftover android.database.sqlite/SQLiteConstraintException type references anywhere outside comments, and core/data/exercise has no androidMain directory at all — matches the "zero androidMain" claim.

2. commonMain conversion + build.gradle.kts — confirmed correct

build-logic/.../KmpLibraryConventionPlugin.kt registers testDebugUnitTest/assembleDebug/lintDebug/assembleDebugAndroidTest/connectedDebugAndroidTest as CI-alias tasks depending on the real KMP tasks — same pattern as the prior database-module conversion, nothing new needed here as claimed.

3. setup-hooks.sh — confirmed correct

Now calls git config core.hooksPath .githooks with a failure check instead of cp-ing into .git/hooks/, matching what CLAUDE.md already documented.

4. Both prior Codex findings are still live — neither has been fixed yet

Only the markdown-spacing nit was addressed in 1a35bf4; the other two Codex comments remain outstanding and I confirmed both by reproducing:

• P2 — .claude/skills/write-repository-test.md:215. ./gradlew :core:data:exercise:testDebugUnitTest --tests "*.<RepoName>ImplDbTest" will fail. testDebugUnitTest for this module is now a plain aggregate DefaultTask (tasks.register("testDebugUnitTest") { dependsOn(tasks.withType<Test>()) } in KmpLibraryConventionPlugin.kt:204-209) — not a Test task — so --tests isn't a registered option and Gradle will reject it. The correct filtered command is :core:data:exercise:testAndroidHostTest --tests "...". Confirmed correct, not yet fixed.
• P3 — documentation/tech-debt.md:366. Links SessionRepositoryImplFinishAtomicDbTest.kt via the old core/data/exercise/src/test/... path; the file is now at src/androidHostTest/.... Confirmed correct, not yet fixed. Worth noting while touching this line: the same table row's other link, AtomicRollbackDeviceTest.kt, points at core/data/database/src/androidTest/..., which is also stale — actual path is src/androidDeviceTest/... (broken by the earlier database-module conversion, not this PR, but adjacent enough to fix in the same edit).

Both of these are one-line path corrections, low risk, and both were already flagged by Codex — just tracking their status here since the PR's classification comment only addressed the spacing nit.

Summary

The four commits do what the PR body claims: the catch-type fix is correct and narrowly scoped, verified against the schema's single unique index; the wall-clock and commonMain conversions are mechanically complete with no stragglers; setup-hooks.sh now does what CLAUDE.md always said it did. No new correctness, security, or architectural issues found. The two outstanding items are both Codex's from the earlier pass (stale doc paths in the skill file's verification command and in tech-debt.md) — neither blocking, both quick follow-ups.
— branch: claude/exercise-kmp

[stslex]

Review classification before the next push:

• Codex filtered-test command — correct. Reproduced verbatim on the current head: :core:data:exercise:testDebugUnitTest --tests … fails during task configuration with Unknown command-line option --tests because the KMP compatibility task is an alias, not a Test task. The focused command must target testAndroidHostTest; the unfiltered repo alias remains valid.
• Codex rollback-test link — correct. A repo-wide source-path sweep found the cited exercise src/test link and the adjacent database src/androidTest link in the same active tech-debt row. Both targets moved; I will update the complete row to androidHostTest / androidDeviceTest, not only the single cited URL.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 1a35bf471a

ℹ️ 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".

[stslex]

@claude review

[stslex]

@codex review

[claude]

Claude finished @stslex's task in 2m 0s —— View job

---

Review complete

• Gather context (PR body, diff, prior review comments, changed files)
• Verify latest commit (8a9761c) fixes the two outstanding Codex nits
• Re-review catch-fix + device test commit
• Re-review mechanics commits (wall-clock swap, commonMain conversion)
• Re-review setup-hooks.sh fix
• Sweep for any new issues introduced by the fix commits
• Post final review

I re-verified independently against the current head (8a9761c), not by trusting the PR body or prior review passes.

Both outstanding Codex findings are now fixed, correctly

• .claude/skills/write-repository-test.md:215-216 — now reads ./gradlew :core:data:exercise:testAndroidHostTest --tests "*.<RepoName>ImplDbTest" with a one-line comment explaining why the alias can't take --tests. Confirmed the KMP alias task is a plain dependsOn aggregate (KmpLibraryConventionPlugin.kt), not a Test task, so this was a real bug, and the fix targets the right task.
• documentation/tech-debt.md — both flagged rows are corrected: Room3RoundTripDeviceTest.kt and AtomicRollbackDeviceTest.kt now link src/androidDeviceTest/..., and (going beyond just the cited link) SessionRepositoryImplFinishAtomicDbTest.kt in the same row now links src/androidHostTest/... too — matches the author's claim of a full-row fix, not just the single cited URL. documentation/architecture.md:626 (core/data/exercise/.../ path) was already corrected in an earlier commit (745ffb9), before Codex's comment — so that thread's diff-anchor pointed at an already-fixed line, but the tech-debt.md issue it actually described is now fixed too.
• Grepped the rest of tech-debt.md for stray androidTest/src/test paths: all remaining hits belong to non-KMP modules (app/app, feature/*, core/ui/mvi), which is correct — only core/data/exercise and core/data/database converted.

Spot-checked the core claims once more, all hold

• ExerciseRepositoryImpl.saveItem (core/data/exercise/src/commonMain/.../ExerciseRepositoryImpl.kt:113-127) catches androidx.sqlite.SQLiteException, rethrows unless isUniqueConstraintViolation() matches; that helper is a private member (not file-private top-level), correctly avoiding a synthetic-accessor lint hit.
• All 8 wall-clock sites across ExerciseRepositoryImpl, SessionRepositoryImpl, TrainingRepositoryImpl use Clock.System.now().toEpochMilliseconds() — zero System.currentTimeMillis() remain.
• Zero android.database.sqlite/SQLiteConstraintException references outside comments/docstrings anywhere in the module; no src/androidMain directory exists — "zero androidMain" is accurate.
• setup-hooks.sh now calls git config core.hooksPath .githooks with a failure check (exit 1 on failure) instead of cp-ing into .git/hooks/.
• documentation/feature-specs/kmp-phase-6-data-layer.md §10 heading now has the blank line separating it from §9's final paragraph (the earlier cosmetic nit) — confirmed via git show.

No new issues found

Nothing new surfaced from the fix commits themselves — both are narrowly-scoped doc/path corrections with no code-behavior surface. This PR is in a clean, mergeable state as far as this review can tell: the one real behavior fix is correct and proven on-device, the mechanical conversion is complete, and all review feedback from both Codex and prior Claude passes has been addressed and re-verified.
— branch: claude/exercise-kmp

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a9761c485

ℹ️ 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".

[stslex]

@codex review

[stslex]

@claude review

[claude]

Claude finished @stslex's task in 1m 30s —— View job

---

Review complete

• Gather context (PR body, diff vs origin/claude/database-bundled-sqlite, prior review threads, review comments)
• Re-verify the catch fix + device test
• Re-verify the commonMain conversion (build.gradle.kts, source-set layout)
• Re-verify setup-hooks.sh and doc fixes
• Check outstanding review threads for anything still unaddressed
• Post final review

I re-verified independently against the current head (409159e), not by trusting the PR body or the prior @claude/Codex passes.

Everything previously flagged and marked "fixed" checks out

• ExerciseRepositoryImpl.saveItem (core/data/exercise/src/commonMain/.../ExerciseRepositoryImpl.kt:113-129) catches androidx.sqlite.SQLiteException, rethrows unless isUniqueConstraintViolation() matches "UNIQUE constraint failed". ExerciseRepositoryDuplicateNameDeviceTest.kt is a genuinely strong device test: a fail-fast ImageStorage stub proves the duplicate path never touches image I/O, and it asserts both SaveResult.DuplicateName and that the original row survived untouched (rows.size == 1, same uuid) — not just a return-value check.
• All 8 wall-clock sites (ExerciseRepositoryImpl, SessionRepositoryImpl, TrainingRepositoryImpl) are Clock.System.now().toEpochMilliseconds(); zero System.currentTimeMillis() remain.
• No src/androidMain directory exists in the module; no leftover android.database.sqlite/SQLiteConstraintException references outside comments — "zero androidMain" holds.
• setup-hooks.sh now calls git config core.hooksPath .githooks with a failure check instead of cp-ing into .git/hooks/, matching what CLAUDE.md claims.
• Both prior Codex path-correction nits (.claude/skills/write-repository-test.md's --tests command, documentation/tech-debt.md's stale src/test/src/androidTest links) are fixed in the committed history and the fixes are accurate — I diffed the committed file content directly rather than trusting the classification comments.

One open finding — unaddressed, and backed by the repo's own rule

core/data/exercise/build.gradle.kts:14-19 — Codex's P1 from the 8a9761c4 review pass is still live; no commit since has touched this file. The block:

// Every production file is commonMain — the module has NO androidMain: the one platform type it
// carried (the framework SQLiteConstraintException in ExerciseRepositoryImpl's duplicate-name
// catch) was replaced by the common androidx.sqlite.SQLiteException before the conversion, and the
// paging surface (Pager/PagingData/PagingSource) lives in paging-common. The compose-bom/runtime
// and room3-runtime declarations the Android build script carried had ZERO usages (phase-6 spec §5)
// and do not return.

is HISTORY under AGENTS.md's own three-category comment rule (AGENTS.md:95-108, the section Codex's finding cites): "was replaced ... before the conversion", "the Android build script carried", "do not return" are exactly the "what a decision used to be" language the rule says never belongs in code — it belongs in the commit body and documentation/feature-specs/kmp-phase-6-data-layer.md, both of which this PR already writes to. The invariant worth keeping at the point of edit is just "no androidMain; paging lives in paging-common" (a GUARD, category 1) — the derivation of how that became true is history, category 3. Since AGENTS.md's own text was written for exactly this pattern ("Category 3 is still being produced, which is why this is a rule and not a cleanup"), this is a correct, load-bearing finding, not a style nit — worth fixing before merge rather than deferring.

The adjacent two-line comment at build.gradle.kts:26-27 ("the database module's room/sqlite deps are implementation and do not leak here") is fine as-is — that's a GUARD about the current dependency shape, not history.

Summary

The four commits do what the PR body claims, and every previously-reported issue that was marked fixed is in fact fixed in the committed diff. The one open item is the build.gradle.kts conversion-history comment Codex flagged in the last pass — real, cited against the repo's own explicit comment-category rule, and not yet classified or fixed.

[stslex]

Review classification before the next push:

• Codex build-script conversion history — correct. The current commonMain / no-androidMain invariant is load-bearing, but the former framework exception, removed declarations, and zero-usage counts are conversion history. That derivation is already recorded in documentation/feature-specs/kmp-phase-6-data-layer.md → §10 What PR E measured, “core:data:exercise is the repo’s first zero-androidMain KMP module.” I will keep the present source-set invariant plus that stable anchor in the build script.

[stslex]

@claude review

[stslex]

@codex review

[claude]

Claude finished @stslex's task in 1m 0s —— View job

---

Review complete

• Gather context (PR body, diff vs origin/claude/database-bundled-sqlite, full review thread history)
• Re-verify the catch fix + device test
• Re-verify the commonMain conversion (build.gradle.kts final state after 1e9a27d)
• Re-verify setup-hooks.sh and doc fixes
• Check all prior review threads for anything still unaddressed
• Post final review

I independently re-verified against the current head (1e9a27d), not by trusting the PR body or the prior review passes.

Everything previously flagged is fixed, and I re-confirmed each fix directly rather than trusting the classification comments

• core/data/exercise/build.gradle.kts:14-16 — Codex's P1 (conversion-history comment violating AGENTS.md's comment-category rule) is fixed. The block now reads as a present-tense invariant only: "Every production file is commonMain; this module intentionally has no androidMain source set... See kmp-phase-6-data-layer.md → §10..." — no more "was replaced," "carried," or "do not return" history language. The adjacent androidx.sqlite dependency-boundary comment (lines 23-24) is unchanged and still fine, as previously assessed.
• .claude/skills/write-repository-test.md and documentation/tech-debt.md — both Codex path-correction nits (testAndroidHostTest --tests command, src/androidHostTest/src/androidDeviceTest links) are correctly applied at HEAD. (Note: the local working tree in this sandbox currently shows an unstaged reversion of write-repository-test.md back to the old src/test/testDebugUnitTest wording — but that's uncommitted, sandbox-local state, not part of the PR; the committed content at 1e9a27d is correct and I've verified it directly with git show HEAD:....)
• documentation/feature-specs/kmp-phase-6-data-layer.md §10/§9 spacing nit — confirmed fixed.

Core claims re-verified from scratch

• ExerciseRepositoryImpl.saveItem (core/data/exercise/src/commonMain/.../exercise/ExerciseRepositoryImpl.kt:103-127) catches SQLiteException, rethrows unless isUniqueConstraintViolation() matches — correct and matches the measured on-device error text.
• Wall-clock swap: all 8 sites across ExerciseRepositoryImpl (182, 301, 466), SessionRepositoryImpl (235, 249, 351), TrainingRepositoryImpl (163, 245) use Clock.System.now().toEpochMilliseconds(); grepped the whole module — zero System.currentTimeMillis() remain.
• core/data/exercise/src has exactly three source sets: commonMain, androidHostTest, androidDeviceTest — no androidMain, matching the "zero androidMain" claim.
• Grepped for SQLiteConstraintException/android.database.sqlite outside comments/docs — zero production hits; only appears in the doc-comment explaining the fix and the device test's KDoc.
• setup-hooks.sh calls git config core.hooksPath .githooks with a failure check (exit 1), matching what CLAUDE.md documents.

No new issues found

This PR is in a clean, mergeable state. The one real behavior fix (duplicate-name catch under the bundled driver) is correct and proven on-device; the mechanical conversion (wall-clock swap, commonMain move) is complete with no stragglers; every review finding from both Codex and prior Claude passes — including the most recent build-script comment-category finding — has been fixed and I've re-confirmed the fix independently rather than trusting the classification comment.
— branch: claude/exercise-kmp

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e9a27d10b

ℹ️ 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".

[stslex]

Review classification before the next push:

• Codex SQLite KDoc derivation — correct. The portable UNIQUE constraint failed conclusion and the synthetic-accessor guard belong at the helper; error code 2067, driver rendering, and type investigation are derivation already recorded in documentation/feature-specs/kmp-phase-6-data-layer.md → §10 What PR E measured, “androidx.sqlite.SQLiteException is actual typealias … on Android.” I will keep the conclusion, guard, and stable anchor only.
• Codex device-test history — correct. The real-device bundled-driver oracle is a current test invariant; “written red first” and the pre-conversion catch narrative are history already recorded under the same §10 anchor. I will retain only the current oracle relationship and citation.

[stslex]

Follow-up self-review classification before the same push:

• Inline catch derivation — correct-and-new. The catch-site comment repeats the same typealias, driver-message, and pre-fix control-flow derivation as the reviewed helper KDoc. The behavior is self-evident from isUniqueConstraintViolation() plus the rethrow, and the derivation already lives in phase-6 §10, so I will remove the duplicate comment.
• Robolectric class count — correct-and-new. The 16 of 19 measurement in the dependency comment is derivation that can decay. I will retain only the current module-ownership guard for the Robolectric dependencies; the test inventory remains in the phase specification.

[stslex]

@claude review

[stslex]

@codex review

[claude]

Claude finished @stslex's task in 1m 13s —— View job

---

Review complete

• Gather context (PR body, diff vs origin/claude/database-bundled-sqlite, full review thread history)
• Re-verify the catch fix + device test at current HEAD
• Re-verify the commonMain conversion (build.gradle.kts final state, source-set layout)
• Re-verify setup-hooks.sh fix
• Check all prior review threads for anything still unaddressed
• Post final review

I independently re-verified against the current head (5de4686), not by trusting the PR body or prior review passes.

All prior findings (Codex + earlier Claude passes) are fixed, and I re-confirmed each directly

• core/data/exercise/build.gradle.kts:14-16 — the P1 comment-history finding is fixed. It now reads as a pure present-tense invariant: "Every production file is commonMain; this module intentionally has no androidMain source set... See kmp-phase-6-data-layer.md → §10..." — no derivation language left.
• ExerciseRepositoryImpl.kt:508-515 (isUniqueConstraintViolation KDoc) — fixed in 5de4686. Now states only the portable conclusion (shared UNIQUE-text match), the synthetic-accessor rationale for keeping it a member, and a §10 citation. The error-code/type-investigation derivation is gone.
• ExerciseRepositoryDuplicateNameDeviceTest.kt:29-33 — fixed in 5de4686. KDoc now states only the current oracle relationship to the Robolectric twin plus the §10 citation; the "written red first" / pre-conversion history sentence is removed.
• .claude/skills/write-repository-test.md and documentation/tech-debt.md — both earlier Codex path-correction nits (testAndroidHostTest --tests command, src/androidHostTest/src/androidDeviceTest links) are correct at HEAD (confirmed via git show HEAD:...).
• documentation/architecture.md:626, documentation/testing.md — all core/data/exercise paths correctly point at src/commonMain/src/androidHostTest.

Core claims re-verified from scratch at 5de4686

• core/data/exercise/src has exactly three source sets — commonMain, androidHostTest, androidDeviceTest — no androidMain.
• Grepped the whole module: zero SQLiteConstraintException/android.database.sqlite references remain anywhere (including comments).
• All 8 wall-clock sites (ExerciseRepositoryImpl.kt:176,295,460, SessionRepositoryImpl.kt:235,249,351, TrainingRepositoryImpl.kt:163,245) use Clock.System.now().toEpochMilliseconds(); zero System.currentTimeMillis() remain.
• saveItem's catch (ExerciseRepositoryImpl.kt:117-121) catches androidx.sqlite.SQLiteException and rethrows unless isUniqueConstraintViolation() matches — correct and narrowly scoped to the one unique = true index on exercise_table.
• setup-hooks.sh calls git config core.hooksPath .githooks with a failure check (exit 1), matching what CLAUDE.md documents.
• gradle/libs.versions.toml adds the androidx-sqlite (base) alias as claimed, distinct from the -framework alias.

One incidental note, not a PR issue

The local checkout in this review environment shows an unstaged working-tree modification to .claude/skills/write-repository-test.md that reverts several lines back to pre-conversion wording (src/test, testDebugUnitTest --tests, androidTest). This is sandbox-local, uncommitted state — git show HEAD:.claude/skills/write-repository-test.md confirms the committed content on the branch is correct (androidHostTest, testAndroidHostTest --tests, androidDeviceTest). Nothing to fix in the PR itself; flagging only so it isn't mistaken for a regression if seen elsewhere.

Summary

Every finding raised across this PR's review history — from both Codex and prior Claude passes, including the most recent comment-category (AGENTS.md history-in-code) findings — is fixed in the committed diff, and I re-verified each fix independently rather than trusting the classification comments. No new correctness, security, or architectural issues found. This PR is in a clean, mergeable state.
— branch: claude/exercise-kmp

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: 5de468602a

ℹ️ 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".