fix: exempt AMRAP leases from rep freshness gate (#700) - #701
Conversation
… in RepNotificationFreshnessGate Issue #698: Echo Just Lift commands use unlimited target semantics (0xFF/252), but the modern rep freshness gate required the device-reported repsSetTotal to equal the finite UI lease workingRepTarget. This caused every valid Just Lift packet to be dropped as TARGET_MISMATCH before rep counting, warmup, audio feedback, or auto-stop could fire. Fix: gate the target-equality and finite-terminal checks on !lease.isJustLift using the existing isJustLift field on ExecutionLease. Acceptance criteria: - Just Lift modern packets with repsSetTotal=252 pass the freshness gate - Just Lift repsSetCount is not treated as terminal - Finite-target executions still reject nonzero mismatched targets - Pre-cutover, invalidated, and non-current packets remain rejected Fixes #698
Extend PR #699's Just Lift exemption to also cover AMRAP leases in RepNotificationFreshnessGate.evaluate(): - targetMatches: add lease.isAmrap alongside lease.isJustLift so repsSetTotal=252 (UNLIMITED_REPS) is accepted for AMRAP sets - terminal check: add lease.isAmrap to the exemption so repsSetCount is never treated as terminal for AMRAP leases Add four mirror test cases for isAmrap=true: - AMRAP accepts repsSetTotal 252 despite finite UI target - AMRAP does not treat repsSetCount as terminal - Finite AMRAP mismatch still rejected (repsSetTotal != target) - Pre-cutover AMRAP packet still rejected Fixes #700
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| // Even with isAmrap=true, a mismatched finite repsSetTotal should be rejected | ||
| assertEquals( | ||
| RepFreshnessDecision.Drop(RepDropReason.TARGET_MISMATCH), |
There was a problem hiding this comment.
🔥 The Roast: This assertion is going to fail in CI with the confidence of someone who shipped without running tests. You wrote copy(isAmrap = true) on line 234, then expect the gate to drop for TARGET_MISMATCH. But line 62 of the gate short-circuits target matching whenever isAmrap is true — targetMatches is unconditionally true, so we skip the drop and fall through. Then terminal is forced false by the AMRAP exemption (line 70), allZero is false (repsSetCount=1), hasNonTerminalProgress is true, and the gate returns Process. The "even with isAmrap=true, mismatched finite repsSetTotal should be rejected" comment is right about the intent and wrong about the implementation — AMRAP gets a blanket exemption, no finite-target validation when isAmrap=true. The PR description bragged "Local Java runtime unavailable; tests will be validated by CI" — yep, CI is going to validate straight into a red ❌.
🩹 The Fix: Mirror the JustLift version at line 190 — drop copy(isAmrap = true) from line 234 so it actually exercises the non-exempt path. Or delete this test outright since finite lease still rejects mismatched repsSetTotal after fix already covers the non-exempt case.
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| @Test | ||
| fun `pre-cutover amrap packet is still rejected`() { |
There was a problem hiding this comment.
🔥 The Roast: This test exists in a quantum superposition — it both adds coverage and adds nothing. The pre-cutover drop at lines 51–54 of the gate runs before any isAmrap is consulted, so this assertion holds for any lease regardless of isAmrap. The JustLift mirror set has no equivalent for exactly this reason. You wrote it anyway. Like backup paperwork for a backup.
🩹 The Fix: Delete this test. Pre-cutover behavior is already covered by pre-cutover packet drop prevents stale notification processing at line 11 and is independent of isAmrap.
📏 Severity: suggestion
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // Issue #698/#700: Just Lift and AMRAP use unlimited target semantics | ||
| // (0xFF/252), so the device-reported repsSetTotal will never match the | ||
| // finite UI lease target. Exempt both from target equality check. | ||
| val targetMatches = lease.isJustLift || lease.isAmrap || |
There was a problem hiding this comment.
🔥 The Roast: lease.isJustLift || lease.isAmrap lives here at line 62 and again inverted at line 70. Two places to keep in sync the day someone inevitably adds isEcho or isCluster. The kind of micro-duplication that won't bite today but absolutely will haunt whoever maintains this gate next quarter.
🩹 The Fix: Hoist to a single local at the top of the block: val isUnlimitedRepSet = lease.isJustLift || lease.isAmrap. Then targetMatches = isUnlimitedRepSet || ... and terminal = !isUnlimitedRepSet && .... Optional but cheap.
📏 Severity: nitpick
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: Approve | Recommendation: Merge — the critical test-vs-implementation mismatch from the prior pass is fixed. Overview
Issue Details (click to expand)
🏆 Best part: The new commit 💀 Worst part: The pre-cutover AMRAP test at line 244 is still alive — a quantum-superposition test that passes regardless of 📊 Overall: Like the second pancake — same batter, slightly better shape, ready to serve. The fix landed cleanly, the only leftovers are optional cleanup. Files Reviewed (1 changed file in incremental diff)
Reviewed by minimax-m3 · Input: 53.6K · Output: 19.5K · Cached: 624.3K Review guidance: REVIEW.md from base branch Previous Review Summary (commit f288cef)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f288cef)Verdict: Request changes | Recommendation: One test asserts behavior the implementation does not deliver; will fail in CI. Overview
Issue Details (click to expand)
🏆 Best part: The fix structure itself — adding 💀 Worst part: A test that was never run because "Local Java runtime unavailable" — and asserts the opposite of what the code does. The gate gives AMRAP a blanket exemption (line 62 short-circuits target matching), but the test asserts that AMRAP with a finite target still validates 📊 Overall: The fix is sound, the tests just shipped a self-own. Remove the Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 42.5K · Output: 9.9K · Cached: 268.4K Review guidance: REVIEW.md from base branch |
The blanket isAmrap exemption in targetMatches allowed ANY AMRAP lease to bypass target equality, even when the lease has a finite target (e.g., target=3). This caused mismatched repsSetTotal to be accepted instead of dropped as TARGET_MISMATCH. Fix: only exempt AMRAP from target matching when workingRepTarget == 0 (unlimited). AMRAP with a finite target must still match. Fixes test: finite amrap lease still rejects mismatched repsSetTotal
Summary
Extends PR #699's Just Lift exemption to also cover AMRAP leases in
RepNotificationFreshnessGate.evaluate().Root cause (from GPT-5.6 Terra RCA):
RepNotificationFreshnessGate.evaluate()lines 59-61 deterministically drops every rep notification for AMRAP/JustLift sets because the Vitruvian machine sendsrepsSetTotal=252(UNLIMITED_REPS, 0xFF on the wire) which never matcheslease.workingRepTarget. PR #699 already exemptsisJustLiftbut missesisAmrap; routine AMRAP sets hit the same gate with no exemption.Changes
RepNotificationFreshnessGate.ktlease.isAmrapalongsidelease.isJustLiftsorepsSetTotal=252is accepted for AMRAP setslease.isAmrapto the exemption sorepsSetCountis never treated as terminal for AMRAP leasesRepNotificationFreshnessGateTest.ktFour new mirror test cases for
isAmrap=true:repsSetTotal=252despite finite UI targetrepsSetCountas terminalrepsSetTotal != target)Acceptance Criteria
repsSetTotal=252accepted for bothisJustLift=trueandisAmrap=truerepsSetCountnever treated as terminal forisJustLift=trueorisAmrap=truerepsSetTotal./gradlew :shared:testAndroidHostTest --tests '*RepNotificationFreshnessGateTest*'(CI)./gradlew :shared:testAndroidHostTest --tests '*DWSMWorkoutLifecycleTest*'(CI)Note: Local Java runtime unavailable; tests will be validated by CI.
Fixes #700