fix: restore live-decode regressions guards - #15
Conversation
Three uncommitted tweaks had crept in without justification or test coverage; reverted all three back to known-good behavior and left a comment at each site so they don't get silently re-applied: - wristOn: skinContact>50 broke the parity suite (2906/2934, all 28 misses on wrist_on) — 28 real v24 records show hr=87-97 (clearly worn) with skinContact<=50, proving skinContact is contact quality, not wear, exactly as already documented. Back to hr>0, which matches the oracle on every case. - parseRealtimeHr's RR sanity clamp (200-2500ms = 24-300bpm) had been relaxed to `v>0`, letting corrupted/out-of-range timing values straight into live RR with no guard. - _r10Motion's detrend window had been widened 9->25 with nothing backing it; ±25 approaches the 7-40 sample autocorrelation lag range it feeds, risking attenuation of genuine step periodicity. No test (parity or otherwise) exercises steps_inc/activity from R10, so there was no way to validate the change either way. dart test: 71/71 passing, including the full 2934-case parity suite.
Was the original reference the Dart decoders were ported from and kept in parity with (7 commits, right up through the latest feature work), not a one-off leftover — but decode_parity_cases.json is now the frozen, self-contained oracle dart test actually runs against, with no runtime dependency on ts/ (verified: no test/config spawns node/tsc against it). Drops the unported v25-handling WIP that was sitting in ts/live.ts uncommitted; that work is not carried over to live.dart.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds comments to the Dart live decoder and removes the TypeScript live decoder, Type-24 parser, and decoder test script. No Dart logic or public signatures change. ChangesDecoder cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Reviewed this as part of a sweep across the open PRs. Recommending rebase-or-close — this is a judgement call rather than something I should push a commit for, so I've left the branch alone. First, a correction to the summary I was working from: this PR is not "only a It's CONFLICTING, and it defends a code state that no longer existsThe two positions it argues for were both settled on 1. // This record family carries no IMU stepping window at all — that is
// "no usable IMU data", the same absence DecodedSample.activity's doc
// comment describes for a truncated R10, not a measured 0.0/0.
activity: null,
stepsInc: null,That landed in #21 along with the corresponding 629-case parity-oracle update. Resolving this branch's conflicts toward its own prose would put the fabrication back. 2. The The evidentiary argument is circularThe PR defends But the oracle's return { ts: d.ts_epoch, hr: d.hr, activity: 0, steps_inc: 0, wrist_on: d.hr > 0, rec_type: 24 }It is literally Worth being clear: On deleting
|
# Conflicts: # lib/src/control.dart # lib/src/live.dart
…docs
Conflicts resolved toward MAIN in both files, deliberately. Resolving toward
this branch's prose would have reverted hardening that has since merged:
* live.dart -- this branch wanted `activity: 0, stepsInc: 0` for the
historical family. Main has `activity: null, stepsInc: null` (protocol#21):
that family carries no IMU stepping window at all, so a zero is a
FABRICATED measurement, not an absence. Reverting would reintroduce exactly
the bug that fix removed.
* control.dart -- this branch wanted the 200-2500 ms RR bound restored over a
relaxed `v > 0`. Main ALREADY applies that bound (`kMinRrMs`/`kMaxRrMs`),
and additionally reads all four declared RR slots instead of stopping after
two, which had been silently dropping beats 3 and 4 and changing RMSSD. So
this branch's concern is already satisfied, and its version is strictly
worse.
WHAT IS LEFT after an honest resolution is the `ts/` removal plus two comments
-- ZERO behaviour change. That is the accurate description of this PR now.
Corrected one of those comments rather than leaving it: it claimed the detrend
window "was bumped to 25 without comment; reverted", but main already reads 9,
so nothing is being reverted. Rewritten as a note on why not to widen it.
Also stated the limit of this branch's own evidence at the `wristOn` seam. The
skinContact-is-quality-not-wear point is sound and worth documenting -- 28 real
v24 records read hr 87-97 with skinContact <= 50 -- but the parity oracle
cannot EVIDENCE `hr > 0`, because that column was generated by the same
`hr > 0` expression in ts/live.ts, which this PR deletes. Confirming it needs
captures with independently known wear state.
dart analyze clean; 111 passing / 4 skipped (the skips are fixture-dependent
and pass from a worktree next to whoop_hist.jsonl).
|
Conflicts resolved ( Both conflicts resolved toward
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/live.dart (1)
16-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn
nullfor compact HR packets without IMU data.
activityandstepsIncdefinenullas unavailable and0as a measured result. The0x28branch at Lines 322-323 still emits0for both fields, although it decodes only timestamp and heart rate. Downstream consumers can persist fabricated zero-motion and zero-step measurements. Returnnullfor both fields in that branch.Suggested fix
- activity: 0, - stepsInc: 0, + activity: null, + stepsInc: null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/live.dart` around lines 16 - 27, Update the 0x28 compact HR packet branch to assign null to both activity and stepsInc, since that branch decodes only timestamp and heart rate. Preserve measured zero values for branches that actually decode IMU data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/live.dart`:
- Around line 346-352: Update the wristOn provenance comment near the wristOn
logic in live.dart so it no longer references the removed ts/live.ts path.
Preserve the hr > 0 generation rule and parity-fixture limitation by documenting
them in a surviving fixture or documentation record, or cite an immutable
archived source.
---
Outside diff comments:
In `@lib/src/live.dart`:
- Around line 16-27: Update the 0x28 compact HR packet branch to assign null to
both activity and stepsInc, since that branch decodes only timestamp and heart
rate. Preserve measured zero values for branches that actually decode IMU data.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 004e146f-dcbf-43d1-a5ea-fc6f59ea6e14
📒 Files selected for processing (1)
lib/src/live.dart
CodeRabbit, correctly: the comment I added cites `ts/live.ts`, and this PR
deletes it -- so the citation would be a dangling path the moment it merges.
That is precisely the doc-rot this project fights, in a comment written to
prevent doc-rot.
The rule itself is now recorded INLINE (`wrist_on: d.hr > 0`) rather than by
reference, so the provenance survives the deletion, plus an immutable
recovery path for the original:
git show 7edcb3e:ts/live.ts | sed -n '229p'
7edcb3e is protocol main, where the file still exists; verified the line is
there and is the expression quoted.
The substantive point is unchanged: the parity oracle cannot EVIDENCE
`hr > 0`, because `decode_parity_cases.json`'s `wrist_on` column was generated
by that same expression. Confirming it needs captures with independently known
wear state.
dart analyze clean; 111 passing / 4 skipped.
|
@coderabbitai — valid, fixed in The rule is now recorded inline (
The substantive point is unchanged: |
|
Tip For best results, initiate chat on the files or code changes.
The comment now correctly states the evidence limit: 🐇 ✓ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
1 participant
Summary
parseRealtimeHr's RR sanity clamp restored to200-2500ms(24-300bpm) — a relaxation tov>0let corrupted/out-of-range timing values straight into live RR with no guard._r10Motion's detrend window restored to9— a widen to25had no test coverage and risked attenuating genuine step periodicity (the autocorrelation lag range it feeds is 7-40 samples).wristOnrestored tohr>0—skinContact>50broke the 2934-case parity suite (28 misses, all false-negative "not worn");skinContactis contact quality, not wear, exactly as already documented, and the fixture proves it (28 real v24 records with hr=87-97 clearly worn, skinContact<=50).ts/(the TypeScript reference implementation) — superseded by the frozendecode_parity_cases.jsonoracle, whichdart testalready runs against with no runtime dependency onts/.Test plan
dart test— 71/71 passing, including the full 2934-case parity suite.(Re-opened as a fresh branch/PR — the prior PR #14 covering this same work was closed without merging.)
Summary by CodeRabbit
Breaking Changes
Documentation