fix: accept unlimited AMRAP reps for finite routine leases

[9thLevelSoftware]

Summary

Fixes #712.

Routine AMRAP commands send the unlimited 0xFF target, which the machine reports as repsSetTotal=252. The execution lease retains the routine's finite UI fallback target (for example 10). PR #701 only exempted AMRAP when workingRepTarget == 0, so every valid warmup and working rep was rejected as TARGET_MISMATCH.

The lease now records whether the current machine command uses the unlimited target. The freshness gate accepts sentinel 252 only when the active AMRAP command is explicitly unlimited. Finite variable-warmup executions remain protected from delayed stale unlimited packets.

Runtime evidence

Corrected Android Phantom production path:

• Runtime: isAMRAP=true, warmup target 3, lease target 10, current command unlimited
• Sequence: warmup 1/3 → 2/3 → 3/3, then working reps 1..3
• Before fix: every notification dropped as TARGET_MISMATCH; UI remained 0/3
• After fix: notifications processed, warmup gate opened, three working reps counted, and release/rest auto-stopped into Set Summary
• Review hardening rerun: same pass with explicit current-command authority

Regression coverage

• Corrected the misleading “finite UI target” test from target 0 to 10
• Added a variable-warmup regression proving a finite AMRAP warmup execution rejects delayed sentinel 252
• Verified red before implementation

CI portability follow-up

The first CI run exposed unrelated current-main common-test portability failures. Test-only fixes:

• removed commas from a Kotlin/Native-incompatible backtick test name;
• replaced common-test Dispatchers.IO references with portable Dispatchers.Default.

Verification

• Full Android host suite
• Related freshness/rep-counter/lifecycle tests
• Android main compilation
• Exact iOS main and test-target compilation
• Live Phantom warmup, working-rep, and auto-stop flow

No version bump, schema change, firmware change, portal change, or unrelated production refactor.

[chatgpt-codex-connector]

You have reached your Codex usage limits for security reviews. Please try again later.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[kilo-code-bot]

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

The Codex P2 landed. Commit e881fd4 cleanly binds the AMRAP exemption to the active command's unlimited-rep intent: a new usesUnlimitedRepTarget boolean travels ExecutionSeed → ExecutionLease (with the right defensive default of false), the gate now requires all three of isAmrap && usesUnlimitedRepTarget && repsSetTotal == 252, and a fresh regression test pins the variable-warmup case to TARGET_MISMATCH. The plumbing hits every begin*Execution entry point because they all funnel through beginExecutionLocked. Cooked correctly.

📊 Overall: Like a chef fixing a recipe mid-service — saw the order was wrong, didn't burn the dish, refired it. Fine follow-up.

Files Reviewed (3 incremental files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGate.kt
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExecutionGuard.kt
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGateTest.kt

Ponytail Review

Two optional, non-blocking notes for future cleanup:

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/ActiveSessionEngine.kt:L7755 — shrink: requiresMachine && !isBodyweightAtStart is redundant; requiresMachine is defined as !isBodyweightAtStart two lines above (L7715). Collapse to requiresMachine && !isTimedCableAtStart && variableWarmupTarget == null && (…).
• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGate.kt:L61 — shrink: The block comment still justifies workingRepTarget == 0 vs the notification sentinel but doesn't mention the new lease.usesUnlimitedRepTarget requirement. One extra sentence suffices.

Ponytail net: -2 lines.

Verdict

Approve — P2 resolved, regression test in place, plumbing complete. Optional Ponytail cleanups are not blockers.

Previous Review Summaries (2 snapshots, latest commit 026db9e)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 026db9e)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this incremental commit is also clean. I need to sit down again. I had my flamethrower warming up for round two and you hit me with a five-line Kotlin Native portability patch.

The follow-up commit swaps Dispatchers.IO for Dispatchers.Default in two race-condition tests (the JVM-only IO pool is not available on Kotlin Native) and silently defuses the backtick-quoted test-name commas in RoutineSetWeightResolverTest so K/N does not choke on the parser. No semantic change, no new edge cases, no new abstractions — exactly the kind of micro-commit that nobody notices until it is missing.

📊 Overall: Like a good prequel — short, necessary, and entirely uninterested in stealing the spotlight from the main feature.

Files Reviewed (2 from the original review; 3 incremental files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGate.kt
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGateTest.kt
• shared/src/commonTest/kotlin/com/devil/phoenixproject/domain/usecase/RoutineSetWeightResolverTest.kt (incremental)
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/RestoredRuntimeTimerRaceTest.kt (incremental)
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/WorkoutExecutionGuardRestoredRuntimeTest.kt (incremental)

Previous review (commit 483c400)

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.

A surgical bug fix that swaps a broken lease-side discriminator (workingRepTarget == 0) for the actual wire sentinel (repsSetTotal == 252), names it via a private const, and corrects the misleading regression test from target = 0 to target = 10 so it actually exercises the bug path. The historical comment is preserved and updated with #712, which is the kind of breadcrumb future maintainers will silently thank you for.

📊 Overall: Like finding a unicorn in production — a tiny, well-scoped fix that targets the real root cause instead of papering over it. Ship.

Files Reviewed (2 files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGate.kt
• shared/src/commonTest/kotlin/com/devil/phoenixproject/presentation/manager/RepNotificationFreshnessGateTest.kt

---

_{Reviewed by minimax-m3 · Input: 60.6K · Output: 7.5K · Cached: 333.3K}

_{Review guidance: REVIEW.md from base branch main}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 026db9ecc3

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

[9thLevelSoftware]

CI follow-up

The first PR run exposed two pre-existing Kotlin/Native common-test portability failures on current main:

• one backtick test name contained commas, rejected by the Native test compiler;
• common tests referenced Dispatchers.IO, which is internal on Native.

Commit 026db9e applies the smallest test-only corrections: a portable test name and Dispatchers.Default for the cross-platform concurrency tests. The exact CI command now passes locally:

./gradlew :shared:compileKotlinIosArm64 :shared:compileTestKotlinIosArm64 -Pskip.supabase.check=true

The full Android host suite also remains green. No production behavior changed in this follow-up.