fix: bound, dispatch and abstain across the record/response decoders - #19
Merged
Conversation
Eight decode-layer defects, all found by audit and each covered by a regression test that fails against the previous behaviour. records.dart - rr_count from inner[18] was used verbatim with no cap and no value bound, so a stray 0x28 read 40 int16s across offsets 19..98 and turned ppg, accel, spo2, skin-temp and ambient bytes into fabricated R-R beats feeding RMSSD. Now capped at 8 (matching live.dart, which already rejected "large count = wrong offset") and each interval gated to 200-2500 ms (matching control.dart). Applies on the trusted v24/v12 path too, and rr_count now reports what was accepted. - _round/_jsRound laundered NaN/Infinity into a finite value. A non-finite accel vector now rejects the record instead of reporting a perfectly still wrist. live.dart - decodeRecord routed only v24/v25/v10, so v7/v9/v12/v18 returned null and lost their own timestamp. All known versions now route through parseR24, whose per-version offsets and plausibility gate are unchanged. - _r10Motion returned Motion(0,0) for a short frame, which published as a measured zero-motion sample. activity/stepsInc are now nullable so "not read" is distinguishable from "measured zero". control.dart - parseRealtimeHr declared n intervals but only ever extracted two, disagreeing with live.dart's realtimeRr on identical bytes. All declared slots are read, bounded, and an implausible count is refused. - getAlarmTime decoded the epoch at a fixed offset valid only for the 7-byte simple form, misreading the 20-byte rich form this package itself writes. Now dispatches on the leading form byte and emits nothing for an unknown form. - battery had no upper bound: 0xffff decoded as 6553.5%. Out-of-range values are now omitted rather than clamped. commands.dart - cmdBuzz did not range-check pattern, so cmdBuzz(0, 300) silently shipped effect 44 with a valid CRC. Now throws. The 2934-case parity oracle is unchanged and still passes.
📝 WalkthroughWalkthroughChangesDecode and command guards
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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)
302-319: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNewly-routed historical versions still fabricate
activity: 0/stepsInc: 0instead ofnull.Broadening the route to
kKnownRecordVersions.contains(recType)now produces aDecodedSamplefor v7/v9/v12/v18 (previously these fell through tonullentirely). ButparseR24'sR24type has no activity/step-count field at all — those are R10-specific IMU-window features — yet Lines 314-315 hardcodeactivity: 0, stepsInc: 0.This is exactly the "measured zero vs. not measured" fabrication this PR's own
DecodedSampledoc (Lines 16-27) was written to eliminate: "NULL means "this record carried no usable IMU data"... That is NOT the same claim as0.0, which means "the IMU was read and the wrist was still"". For WHOOP4 historical records the IMU window was never read at all — this should reportnull, not a measured zero, consistent with the R10 fix in the same PR.🐛 Proposed fix
if (kKnownRecordVersions.contains(recType)) { final d = parseR24(b); if (d == null) return null; return DecodedSample( ts: d.tsEpoch, hr: d.hr, - activity: 0, - stepsInc: 0, + activity: null, + stepsInc: null, wristOn: d.hr > 0, recType: recType, ); }🤖 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 302 - 319, Update the DecodedSample construction in the kKnownRecordVersions branch to set activity and stepsInc to null because parseR24 does not provide IMU measurements. Preserve the parsed timestamp, heart rate, wristOn, and record type values, while keeping null results from parseR24 unchanged.
🧹 Nitpick comments (1)
test/decode_guards_test.dart (1)
62-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood bounds coverage, but the 5–8 declared-count gap isn't tested.
Tests exercise
> 8(0x28,0xff) and valid0-2counts, but nothing probesdeclaredRrCountbetween5and8— precisely the range wherekMaxRrPerRecord=8currently lets the parser read intoppg_green/ppg_red_irbytes (see therecords.dartcomment). Adding a case there would have caught the constant mismatch.🤖 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 `@test/decode_guards_test.dart` around lines 62 - 123, Add a parseR24 regression test covering a declared R-R count between 5 and 8, such as 5, using _patched(_goodV24, ...) and asserting no intervals are decoded. Anchor it in the existing “R24 R-R intervals are bounded” group and ensure the assertion verifies the parser rejects counts that would overlap ppg_green/ppg_red_ir data, not only counts above 8.
🤖 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/records.dart`:
- Around line 125-132: Update the kMaxRrPerRecord constant in records.dart from
8 to 4 so historical RR parsing rejects counts above the documented 0–4 range
and matches realtimeRr’s ceiling.
---
Outside diff comments:
In `@lib/src/live.dart`:
- Around line 302-319: Update the DecodedSample construction in the
kKnownRecordVersions branch to set activity and stepsInc to null because
parseR24 does not provide IMU measurements. Preserve the parsed timestamp, heart
rate, wristOn, and record type values, while keeping null results from parseR24
unchanged.
---
Nitpick comments:
In `@test/decode_guards_test.dart`:
- Around line 62-123: Add a parseR24 regression test covering a declared R-R
count between 5 and 8, such as 5, using _patched(_goodV24, ...) and asserting no
intervals are decoded. Anchor it in the existing “R24 R-R intervals are bounded”
group and ensure the assertion verifies the parser rejects counts that would
overlap ppg_green/ppg_red_ir data, not only counts above 8.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffba9453-fa89-4cda-969c-49787fac5e74
📒 Files selected for processing (5)
lib/src/commands.dartlib/src/control.dartlib/src/live.dartlib/src/records.darttest/decode_guards_test.dart
Comment on lines
+125
to
+132
| /// Largest R-R interval count a single 1 s historical record can plausibly | ||
| /// declare. The sibling realtime decoder (live.dart `realtimeRr`) applies the | ||
| /// same ceiling for the same reason: the wire form carries 0–4 beats, so a | ||
| /// larger count byte means we are reading the wrong offset — not a second | ||
| /// containing dozens of heartbeats. A record declaring more than this yields | ||
| /// NO intervals at all (absence), never a truncated read of whatever bytes | ||
| /// happen to follow (ppg, accel float32s, skin contact, spo2, temp, ambient). | ||
| const int kMaxRrPerRecord = 8; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm live.dart's realtimeRr ceiling and compare to kMaxRrPerRecord/_maxRealtimeRr.
rg -n -B3 -A15 'Uint8List\?\s*realtimeRr|RealtimeRr\s+realtimeRr|_Motion.*realtimeRr|realtimeRr\(' lib/src/live.dart
rg -n 'kMaxRrPerRecord' lib/src -A2 -B2
rg -n '_maxRealtimeRr' lib/src -A2 -B2Repository: OpenStrap/protocol
Length of output: 2687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant sections in records.dart and control.dart.
sed -n '1,40p' lib/src/records.dart
printf '\n---\n'
sed -n '120,140p' lib/src/records.dart
printf '\n---\n'
sed -n '318,332p' lib/src/records.dart
printf '\n---\n'
sed -n '160,212p' lib/src/control.dartRepository: OpenStrap/protocol
Length of output: 5967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant sections in records.dart and control.dart.
sed -n '120,140p' lib/src/records.dart
printf '\n---\n'
sed -n '318,332p' lib/src/records.dart
printf '\n---\n'
sed -n '160,212p' lib/src/control.dartRepository: OpenStrap/protocol
Length of output: 4330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find protocol docs, tests, and any references that describe historical RR count limits.
rg -n "0–4|0-4|kMaxRrPerRecord|rrCount|R-R interval|rr intervals|historical record" lib test README.md docs . -g '!**/.dart_tool/**' -g '!**/build/**'Repository: OpenStrap/protocol
Length of output: 6590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the TypeScript source and the historical-record tests around RR count.
sed -n '1,120p' ts/records.ts
printf '\n---\n'
sed -n '1,140p' test/decode_guards_test.dart
printf '\n---\n'
sed -n '1,80p' test/decoder_test.dartRepository: OpenStrap/protocol
Length of output: 13503
Clamp kMaxRrPerRecord to 4
Historical RR counts are documented and tested as 0–4 only, so 8 still lets 5–8 through this guard and the loop can read past the RR slots into adjacent fields. Match the ceiling used by the realtime decoder.
🤖 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/records.dart` around lines 125 - 132, Update the kMaxRrPerRecord
constant in records.dart from 8 to 4 so historical RR parsing rejects counts
above the documented 0–4 range and matches realtimeRr’s ceiling.
Contributor
Author
Good catch, and the reasoning is right — an R24 record carries no IMU window, so I tested the proposed diff rather than reasoning about it, and it breaks the frozen parity oracle:
{"ts": 1775395266, "hr": 98, "activity": 0, "steps_inc": 0, "wrist_on": true, "rec_type": 24}Two things follow:
Leaving it as a follow-up rather than silently widening the blast radius of a bug-fix PR. Flagging for a maintainer: the honest fix is |
abdulsaheel
added a commit
to OpenStrap/edge
that referenced
this pull request
Jul 26, 2026
Edge CI was failing to COMPILE, and the failure is the whole reason the v48 note existed: error • The function 'personalDynFloorFromDailySummaries' isn't defined error • The named parameter 'personalDynFloorG' isn't defined error • The getter 'activeMinutes' isn't defined for DailyStepEstimate lib/compute/ calls analytics API that only exists on the sibling PR, but pubspec.yaml still pointed at the pre-fix SHA. It compiled locally only because the gitignored pubspec_overrides.yaml redirects to ../analytics — so the green local run was green-by-omission, and CI was the only thing telling the truth. This is the same drift class as v43, caught in its loud form: a missing symbol rather than a silently different number. The v43 version of this shipped a readiness fix that the pinned SHA never contained and stayed broken for three releases. Both pins now point at the sibling PR heads — still immutable commit SHAs, never floating refs: protocol 5675b2f (OpenStrap/protocol#19) analytics 8b1aa4e (OpenStrap/analytics#30) Verified, rather than assumed, that those SHAs contain what the changelog claims: `git show 8b1aa4e:lib/src/onehz/motion/steps.dart` carries the new step API, rr_correction.dart has the signed-dRR `seg.add(x[k])`, and `git show 5675b2f:lib/src/live.dart` has kKnownRecordVersions. Also corrected the kAlgoVersion changelog. It still said the sibling fixes were NOT pinned, which stopped being true with this commit — a changelog that misdescribes what is in the build is the exact failure being guarded against, in either direction. pubspec.lock now records the real git provenance (resolved-ref) instead of the local path: entries a previous `pub get` wrote while the overrides file was present. Reproduced CI locally to confirm: with pubspec_overrides.yaml moved aside, `flutter pub get` resolves ref == resolved-ref == 8b1aa4e, `flutter analyze` is clean, and the suite is 914 passing. ON MERGE: move both pins to the resulting main commits and bump kAlgoVersion again — a PR-branch SHA can become unreachable if the branch is deleted after a squash merge.
abdulsaheel
added a commit
to OpenStrap/edge
that referenced
this pull request
Jul 26, 2026
OpenStrap/protocol#19 and OpenStrap/analytics#30 are merged, so pubspec.yaml moves off the PR-branch heads v49 pointed at and onto the resulting main commits: protocol 5675b2f -> a98cd70 analytics 8b1aa4e -> f5ccae6 A PR-branch SHA stops being the canonical location of a change once it merges, and can be orphaned outright if the branch is later deleted. kAlgoVersion 49 -> 50 even though no edge source line changed alongside it. The constant identifies the code that PRODUCED a day_result, and that code includes the pinned siblings — a device holding v49 rows built against the PR-branch SHAs must re-derive rather than serve them as equivalent. Treating "same content, different commit" as beneath a bump is the assumption that lets a stale bundle outlive a dependency change. Verified at the merge commits themselves rather than inferred from the PRs being green — the check v43 skipped: analytics f5ccae6 steps.dart has the new step API (9 symbol hits) rr_correction.dart:233 is `seg.add(x[k])` (signed dRR) advanced_stager.dart has maxAccelCarryForwardSec protocol a98cd70 live.dart has kKnownRecordVersions Reproduced CI locally with pubspec_overrides.yaml moved aside: resolved-ref matches both pins exactly, flutter analyze clean, 914 passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eight decode-layer defects found by a full-tree audit. Each is covered by a regression test that fails against the previous behaviour.
The two that affect real data
Unbounded R-R count —
records.dartusedinner[18]verbatim with no cap and no value bound. A stray0x28read 40int16s across offsets 19..98, turning ppg, accel, spo2, skin-temp and ambient bytes into fabricated R-R beats that feed RMSSD and therefore readiness.live.dartalready rejectedn > 8("large count = wrong offset") andcontrol.dartalready bounded values to 200–2500 ms — the three decoders of the same wire field had simply drifted apart. Now consistent, including on the trusted v24/v12 path, andrr_countreports what was accepted.v12 was never routed —
decodeRecordhandled only v24/v25/v10, so v7/v9/v12/v18 returned null and lost their own timestamp. Downstream that made edge fall back to capture-time, collapsing a multi-day backfill into a single day. All known versions now route throughparseR24, whose per-version offsets and plausibility gate are unchanged.The rest
_round/_jsRoundlaundered NaN/Infinity into a finite value; a non-finite accel vector now rejects the record instead of reporting a perfectly still wrist._r10MotionreturnedMotion(0,0)for a short frame and published it as a measured zero.activity/stepsIncare nullable so "not read" is distinguishable from "measured zero".parseRealtimeHrdeclarednintervals but only ever extracted two, disagreeing withrealtimeRron identical bytes.getAlarmTimedecoded the epoch at an offset valid only for the 7-byte simple form, misreading the 20-byte rich form this package itself writes. Now dispatches on the form byte.0xffffdecoded as 6553.5%. Out-of-range values are omitted rather than clamped.cmdBuzzdid not range-checkpattern, socmdBuzz(0, 300)silently shipped effect 44 with a valid CRC.Verification
dart test: 106 passing (was 71), including the 2934-case parity oracle, which is unchanged — no oracle edits, no case changed behaviour.dart analyze: clean.Note for consumers
DecodedSample.activity/stepsIncare now nullable. The only consumers inedge/analyticsread.tsand.hr, so nothing downstream breaks — but this is a public API change.Summary by CodeRabbit