fix: exempt Just Lift from rep freshness gate target mismatch (#698) - #699
fix: exempt Just Lift from rep freshness gate target mismatch (#698)#6999thLevelSoftware wants to merge 6 commits into
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
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge VerdictApprove. The previous warning (auto-start grab predating lease activation) is resolved by reading Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. Notes
🏆 Best part: The fix uses the existing 💀 Worst part: The comment says "The polling restart preserves that confirmed detector state," which is slightly back-to-front — polling restart doesn't preserve detector state; the 📊 Overall: Like the third sequel that actually fixes the plot hole from the second movie. Five Codex P1 follow-ups, five clean increments. Whoever's writing this is either very good or very lucky, and I'm running out of material either way. Files Reviewed (2 files)
Previous Review Summaries (5 snapshots, latest commit 80b94a9)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 80b94a9)Verdict: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The regression test is focused and the production change is a single small movement-evidence branch. Oh wait, this part is actually disciplined; somebody hide the confetti. 💀 Worst part: The test simulates 📊 Overall: Like adding a doorbell and discovering the lock is on the other side — the intent is right, but the first real rep can still walk straight past it. Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit e3bb615)Verdict: No Issues Found | Recommendation: Merge Oh wait, this increment lands the third Codex P1 clean and even has tests that read like a bouncer checking IDs at the door: "no zero baseline? no movement? no entry." I had my flame-thrower idling and you're out here shipping a guard clause. Overview
The new
The two new regression tests are properly minimal and the DWSM lifecycle helper now takes 🏆 Best part: The new drop reason and its single-line guard sit next to a pre-existing 💀 Worst part: Nothing to roast. The PR author has, against all odds, shipped an increment I cannot embarrass. 📊 Overall: Like the third season of a show that started shaky and is now quietly out-writing prestige dramas. Three P1 follow-ups, three clean increments. Suspiciously competent. Files Reviewed (2 files)
Previous review (commit e834407)Verdict: No Issues Found | Recommendation: Merge Oh wait, this increment actually lands the P1 fix and retires the previous nitpick in the same stroke. I had my soapbox warmed up for a long speech about operator precedence and now I'm out of material. Overview
The previous review's only finding ( Trace verification against the new code:
All branches preserved, all comments updated to mention the new "including one whose target happens to equal the UI lease target" case. 🏆 Best part: The diff is purely subtractive in spirit — it splits one gnarly boolean into two clean branches, deletes a shadowed clause, and rewrites one test assertion. That's what a P1 follow-up should look like. 💀 Worst part: There's literally nothing left to roast, which is the most offensive thing a PR can do to a reviewer with a flamethrower. 📊 Overall: Like a director's cut that fixes the original plot hole and patches the deleted scene's continuity error — and does it in fewer shots than the original take. Files Reviewed (2 files)
Previous review (commit b7fee41)Verdict: 1 Optional Nitpick | Recommendation: Merge VerdictComment only. The Codex P1 follow-up correctly restricts the Just Lift exemption to the documented unlimited encodings; one redundant clause snuck in alongside the fix. Correctness / Safety FindingsNo correctness or safety findings. Ponytail Review
val targetMatches = lease.isJustLift && notification.repsSetTotal == UNLIMITED_REPS_SET_TOTAL ||
notification.repsSetTotal == 0 ||
notification.repsSetTotal == lease.workingRepTargetPonytail net: -1 line. Suggested Minimal PatchDrop the inner Final Merge GuidanceCan merge as-is; the Ponytail nit is optional cleanup. 🏆 Best part: The P1 fix lands exactly where the previous review predicted — gating the Just Lift exemption to 💀 Worst part: One redundant 📊 Overall: Like a director's cut that fixes the original plot hole — but accidentally leaves one continuity error on the editing-room floor. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit b88dc44)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. VerdictApprove. Minimal, surgical fix with regression coverage and zero scope creep. Correctness / Safety FindingsNo correctness or safety findings. Ponytail ReviewPonytail: Lean already. Ship. Ponytail net: 0 lines. Suggested Minimal PatchNo patch needed. Final Merge GuidanceCan merge as-is. Notes
🏆 Best part: 💀 Worst part: Honestly? Nothing. This is the kind of fix that makes code review boring, which is the highest compliment I can give a bugfix. 📊 Overall: Like a surgeon with a scalpel — one precise cut, three stitches, patient walks out fine. Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 46.6K · Output: 6.9K · Cached: 463.4K Review guidance: REVIEW.md from base branch |
![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: b88dc44e86
ℹ️ 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".
Address Codex review: accept only the known unlimited representation (252) or zero for Just Lift leases, rejecting stale packets from prior finite-target sets that could corrupt reps and feedback. - Add UNLIMITED_REPS_SET_TOTAL constant (252) - Gate targetMatches on specific unlimited value, not blanket isJustLift - Add test: just lift rejects stale finite repsSetTotal from prior execution
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?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: b7fee41271
ℹ️ 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".
| (notification.repsSetTotal == UNLIMITED_REPS_SET_TOTAL || | ||
| notification.repsSetTotal == 0)) || | ||
| notification.repsSetTotal == 0 || | ||
| notification.repsSetTotal == lease.workingRepTarget |
There was a problem hiding this comment.
When the preceding finite set has the same target as the Just Lift UI target (for example, both are 10), this final equality branch still accepts its delayed packet even though a Just Lift packet should report only 252 or zero. KableBleRepository.parseRepNotification() timestamps packets at receipt, and the new terminal exemption then classifies the stale nonzero count as progress, so RepCounterFromMachine imports it into the new session. Fresh evidence in this revision is that the narrowed Just Lift condition is still followed by the mode-agnostic repsSetTotal == lease.workingRepTarget fallback; exclude that fallback when lease.isJustLift is true.
Useful? React with 👍 / 👎.
Address Codex P1 on #699: a delayed finite-set packet whose repsSetTotal happened to equal the Just Lift UI target still passed the generic equality fallback. Restrict Just Lift to only the documented unlimited target (252) or zero, and add a matching-target stale-packet regression test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e834407ac7
ℹ️ 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".
Address Codex P1 on #699 by holding nonzero unlimited Just Lift notifications until a zero baseline or observed movement arms the new execution. Update the Issue 267 lifecycle fixture to emit the documented unlimited target (252) and baseline so CI models the device protocol.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3bb615565
ℹ️ 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".
| if (lease.isJustLift && hasNonTerminalProgress) { | ||
| return RepFreshnessDecision.Drop(RepDropReason.PROGRESS_BEFORE_EVIDENCE) |
There was a problem hiding this comment.
When a Just Lift set receives no all-zero rep packet and the user pulls continuously above the velocity threshold, HandleStateDetector transitions directly from Released to Grabbed, while ActiveSessionEngine calls observeMovement() only for HandleState.Moving (the low-velocity state). The lease therefore remains AwaitingEvidence, and this branch drops the first progress packet and every subsequent cumulative packet, leaving the entire set at zero reps. Treat the confirmed Grabbed transition as movement evidence, or otherwise ensure ordinary first-rep progress can arm the gate without requiring an optional zero packet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 80b94a9: a confirmed Released → Grabbed transition now arms the Just Lift freshness gate, so a normal zero-less first-rep path is accepted. Added a lifecycle regression that starts from that direct transition and verifies unlimited (252) warmup progress reaches completion.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| // Moving would leave the freshness gate unarmed without a zero packet. | ||
| val hasFreshJustLiftGrab = activityState == HandleState.Grabbed && | ||
| currentLease?.isJustLift == true | ||
| if ((activityState == HandleState.Moving || hasFreshJustLiftGrab) && |
There was a problem hiding this comment.
🔥 The Roast: This only arms on a new Grabbed emission, but the normal Just Lift auto-start path can already be in Grabbed before the new lease is activated. The grab that starts the auto-start countdown is emitted before activation; startActiveWorkoutPolling() restarts polling with forAutoStart=false, and the polling engine does not reset the detector, so there is no second Grabbed event after repFreshnessGate.resetFor(). The first unlimited rep packet is then dropped with PROGRESS_BEFORE_EVIDENCE, and the rest of the set stays at zero reps — a very confident way to count absolutely nothing.
🩹 The Fix: After activating the Just Lift lease, inspect the current handle state and call repFreshnessGate.observeMovement(activeLease) when it is Grabbed (or carry the already-confirmed grab evidence across the auto-start boundary). Add a regression test where Grabbed is set before workout activation and no post-activation state transition occurs.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in commit 1fafc9d: immediately after activating a Just Lift lease, the engine now carries an already-confirmed Grabbed handle state into the freshness gate before polling restarts. Added a zero-less unlimited-warmup lifecycle regression where the confirmed grab predates lease activation.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Fixes the rep freshness gate dropping all Just Lift packets as
TARGET_MISMATCH, which prevented warmup rep counting, audio feedback, and auto-stop on Just Lift sessions.Root Cause
Echo Just Lift commands use unlimited target semantics — the firmware encodes the target as 0xFF, which the parser reports as
repsSetTotal=252. However,RepNotificationFreshnessGate.evaluate()required the device-reported target to equal the finite UIlease.workingRepTarget(e.g. 10). Every valid Just Lift packet was dropped at lines 59-61 before rep counting, warmup, audio, or auto-stop could fire.Fix
Gate the target-equality and finite-terminal checks on
!lease.isJustLiftusing the existingisJustLiftfield onExecutionLease:repsSetTotal == workingRepTargetcomparisonrepsSetCount >= workingRepTargetterminal checkNo parser conversion, BLE command change, or broad mode refactor. Minimal scope — only
RepNotificationFreshnessGate.evaluate()is affected.Acceptance Criteria
repsSetTotal=252pass the freshness gaterepsSetCountis not treated as terminalTests
Three new test cases in
RepNotificationFreshnessGateTest:repsSetTotal=252despite finite UI targetrepsSetCountas terminalrepsSetTotalafter fixFixes #698