Separate the two enumeration terminators: don't stop a key walk on validKey=0 (#761, #103)

[vishk23]

Read-only. No new opcode, no allowlist change, no storage, no strings.

1. The file says the layout is unverified, and the terminator came with the layout

FeatureFlagProbe.swift has said this since #872:

Unverified on 5/MG. The hardware result behind this layout is a WHOOP 4.0 with an R19-era key list

The record layout came off a WHOOP 4.0. So did its terminator semantics:

public var isExhausted: Bool { !validKey || index == 0xFF }

That is a disjunction — the walk stops on either condition — and it has never been tested as one. It only matters if validKey = 0 might mean something other than "end of list". It might: the obvious alternative is that it marks an empty or retired SLOT, with the list continuing past it. Under that reading a walk that stops there is truncated, and every enumeration anyone has published — including this project's — is short by however many keys sat behind the first hole.

That is the same failure @ryanbr's #874 fixed one condition over. #874 split isSkippable out of isExhausted because our decode declining a name was ending the strap's enumeration. This is the same shape: a condition we do not understand, ending the walk, and reporting the result as complete.

2. On real hardware the two conditions have never been separated

Both walks, on the same WHOOP 5 MG (WS50_r03):

walk	what the strap served	what ended it
117/118 (#872)	16 replies, every one validKey = 1 with a decodable name; indices 1–10 then 13–18 (the strap skipped 11 and 12 itself, and never sent validKey = 0 for them)	the client. steps >= announcedCount. Neither 0xFF nor validKey = 0 was ever served
115/116 (#898's code)	7 keys, then a final reply carrying index = 255 AND validKey = 0 on the same frame	the strap — but both conditions at once

Read those two rows together:

• On 117/118 no terminator was observed at all. That list is not known to end at 16; we stopped asking at 16. "16 keys" is a client-side number.
• On 115/116 both terminators fired simultaneously, so that run cannot say which one the firmware meant. It is not evidence for either reading.

Neither run distinguishes them. This PR is the read-only experiment that does.

3. What changed

isExhausted is now index == 0xFF only. That marker is the one thing a strap has served here unambiguously. validKey = 0 without it becomes isEmptySlot: recorded, stepped over, and the next-record verb sent again. What comes back is the evidence:

what the strap does next	conclusion the report prints
names more keys, or reaches 0xFF	DECISIVE — validKey=0 is an EMPTY/RETIRED SLOT … a walk that stopped on validKey=0 would have been truncated here
repeats the same index with validKey = 0	DECISIVE — validKey=0 is a TERMINATOR … the cursor is parked and there is nothing past it
runs into one of our caps first	INCONCLUSIVE — … a client-side bound ended the run before the strap did

The parked-cursor case is why this costs almost nothing: if validKey = 0 really is the terminator, the firmware parks and repeats, and two round-trips settle it.

Every stop is now named, and client-side stops say so. A new stopCode (endMarker, emptySlotCursorParked, emptySlotRunCap, stepCap, announcedCountOvershoot, timeout, unsupported, parseFailure) sits beside the prose reason, and the three client-side ones also append — INCOMPLETE: the walk ended on a client-side bound, not on the strap's own end marker to the verdict — the one line that gets pasted into an issue. A truncated walk can no longer read as a complete one.

The announced count no longer has the last word. It was the only thing that ended the 117/118 walk. The walk now takes countOvershootAllowance = 4 further replies past it, so the strap gets to end its own list. Calibration: that strap announced 16 and served indices up to 18, so four is twice the observed excess. Bounded by the existing 128-reply cap.

The bounds, all client-side, all documented as such: 8 consecutive empty slots (the largest hole ever observed was 2, strap-side), a repeated index inside such a run, announced count + 4, and the pre-existing 128 replies.

#874's discipline is untouched. isSkippable is byte-for-byte what it was: a name OUR parser declines is counted, stepped over, and never reported as the strap's behaviour. The verdict still refuses to say "named none" when the strap did name them.

4. Raw record bytes, on every reply

Every trace line now carries raw=<hex> of the record it was decoded from, and a reply that fails to decode logs the whole frame:

START_FF_KEY_EXCHANGE(117) → revision=1 count=2 result=SUCCESS(1) raw=01 02 00
SEND_NEXT_FF(118) → index=0 validKey=true key="enable_r22_packets" result=SUCCESS(1) raw=01 00 01 65 6e 61 62 …
SEND_NEXT_FF(118) → index=255 validKey=false result=SUCCESS(1) raw=01 ff 00 00 00 00 00  (index=0xFF AND validKey=0 on the same reply — both terminator conditions at once)

Parsed fields are a claim about the layout; they cannot re-check or contradict it. Findings here have had to be re-run because only parsed output was kept. Hex is capped at 64 bytes and the true length is always printed, so the elision is visible.

5. The count field, and what it does not establish

parseStart reads record[1] | record[2] << 8. If the field is one byte, record[2] is padding and that read is wrong.

Nothing observed settles it. Every count seen has had record[2] == 0x00 — exactly where a u16 read and a single-byte read return the same number. So the report now carries both, and says which case it is:

Announced count: u16 LE read = 16; single-byte read = 16 (high byte 0x00) — the two readings AGREE,
so this reply does not establish the field's width. Keys yielded: 16

A nonzero high byte renders the two readings DISAGREE — and is itself the finding that decides the field. It already fails closed: an implausible u16 is refused as a loop bound by countIsPlausible. And announced-versus-yielded is now reconciled explicitly (keys + skipped + empty slots), with a MISMATCH line when it does not add up.

6. Both namespaces, by construction

The walk is namespace-parameterised (FeatureFlagProbe.Namespace, with featureFlagNamespace = 117/118 and deviceConfigNamespace = 115/116) and parseStart / parseNext take an expecting: opcode. One walk implementation, two namespaces — the terminator rules cannot be corrected in one and left wrong in the other. Tested end-to-end on real 115/116 frames, including the case that matters: an empty slot followed by a named key, which the old rule discarded.

7. Standalone, not stacked on #898 — and why

Standalone, off main. #898 is a 3,100-line PR about a different question (which device-config keys exist). This is a ~60-line semantic correction to a decoder already on main, and it should not wait behind that review — nor should #898's review absorb it.

It reaches #898 anyway, because #898's DeviceConfigReadProbe.noteEnumerationNext classifies its 115/116 replies with these exact properties (r.isExhausted, r.isSkippable). Rebasing #898 on this gives its walk the corrected rule with no further work.

I checked the merge rather than assuming it: the only conflict is two doc comments. Both branches add expecting: to parseStart/parseNext with the identical signature and identical default, so the code merges clean and whichever lands second resolves by picking a comment. #913 and #914 touch only verdict/report prose and do not overlap this at all.

8. What VK's strap will say

> 16 feature-flag keys or > 7 device-config keys means a key exists that no NOOP client has ever observed, and the completeness both namespaces currently claim — which is load-bearing for #103's conclusion that no SpO2-related key is present — is wrong.

Exactly 16 and 7 under the corrected rule is an equally decisive result, and equally worth publishing. It converts "16 keys, and our stop rule was never tested" into "16 keys, and the strap itself declined to name a seventeenth when asked four more times". That is what makes the #103 negative an actual closed door instead of an artefact of where we stopped asking. Either way, the Terminator: and Stop code: lines say which of the two conditions the firmware means — a fact nobody currently has for any WHOOP firmware.

Verification

• swift test (WhoopProtocol) — 442 passed / 0 failures. FeatureFlagProbeTests 27 → 44.
• Android testFullDebugUnitTest — 3213 tests, 0 failures, 0 errors, 5 skipped. FeatureFlagProbeTest 27 → 44. Counts read from the JUnit XML, not the exit code: an earlier run of this same command printed BUILD FAILED while the wrapper exited 0, so exit status is not evidence here. (assembleFullDebug run first, as CI does — BUILD SUCCESSFUL in 15m 57s; Android CI does not run on PRs.)
• Tools/doc_comment_lint.py and Tools/i18n_audit.py --ci clean. No new strings.
• New coverage both sides, in lockstep: continue-past-validKey=0 with keys collected after it, stop-on-0xFF, both-terminators-on-one-reply (VK's 115/116 case), 0xFF-with-validKey=1 (the other separation), parked cursor, consecutive-empty cap, valid entry resetting the run, announced-count overshoot, the 128 cap, an UNSUPPORTED(3) refusal on both the START and a NEXT reply, a skippable undecodable name mid-list, an undecodable name AFTER an empty slot (still decisive — Don't let our own decode strictness end the strap's enumeration #874's rule applied to the conclusion, not just the walk), count-versus-yielded mismatch and its clean case, both count readings agreeing and disagreeing, raw record hex, raw frame hex on a decode failure, hex elision, 115/116 end-to-end, cross-namespace opcode rejection, and a constants/stop-code parity test pinning the two platforms to the same numbers and spellings.
• Both goldens updated in lockstep — the full report text is asserted byte-for-byte on both platforms.
• The two test files are 44 tests each with 1:1 name parity — the only differing name is the deliberately mirrored walkBoundsMatchTheSwiftTwin / walkBoundsMatchTheKotlinTwin, which pins both platforms to the same bound constants and the same stop-code spellings.
• UNVERIFIED on hardware: whether a 5/MG serves anything past a validKey = 0 reply is exactly what this exists to establish. Either answer is worth having.

Refs #761, #103, #898.

[ryanbr]

The premise holds up and I checked it rather than taking it. isExhausted on main really is !validKey || index == 0xFF, isSkippable really was split out of it by #874, and the "Unverified on 5/MG" caveat really does sit above a layout that came off a 4.0 — so the terminator arrived with the layout and inherited its provenance. Removing an untested disjunction that can silently truncate a census is the right change, and 11 checks are green on both platforms.

One thing to fix before merge, and it is your own rule from #914.

"there is nothing past it" is not licensed by the observation

DECISIVE — validKey=0 is a TERMINATOR on this firmware: the next request returned the
same index with validKey=0 again, so the cursor is parked and there is nothing past it.

What the run observes is that the cursor does not advance past this point via this verb. That is solid. "There is nothing past it" is a step beyond it, and two firmwares produce the identical frame:

• end of list — the cursor parks at a sentinel, validKey=0 is a terminator, nothing follows;
• a cursor that only advances on a valid record — an empty or retired slot parks it, validKey=0 is a slot, the walk is stuck, and there may well be keys behind the hole.

The second is exactly the reading this PR exists to make testable. So on that branch the probe would print, as DECISIVE, the conclusion the PR was written to question — and a reader pasting that line into an issue would carry it forward as settled.

Worth saying what cuts the other way, because it is real: on your strap the firmware skipped indices 11 and 12 silently, never serving validKey = 0 for them. That is evidence this firmware advances past holes without a hole record, which makes the parked-on-a-hole reading less likely here. It does not eliminate it, and the string says "on this firmware" while being printed on whatever strap runs it.

Narrowing it costs nothing — the DECISIVE label can stay for what is actually decided:

the cursor does not advance past it, so this walk cannot see anything beyond. Whether the list truly ends here is not separable from a firmware whose cursor parks on an empty slot.

The EMPTY/RETIRED SLOT branch has no such problem: more names arriving after a validKey=0 is direct evidence, and "a walk that stopped here would have been truncated" is exactly what was observed.

What is right

• The fourth outcome is handled. "validKey=0 was never served, so it is untested here" with a test pinning it — and on your own 117/118 evidence that is the likeliest result, since validKey=0 alone has never been observed on either walk. A design that reports "the experiment did not get to run" as distinct from "the experiment came back negative" is the whole lesson of §5 in #891, applied before anyone asked.
• stopCode plus the INCOMPLETE suffix on the three client-side stops. The announced count ending the 117/118 walk is precisely how "16 keys" became a number people repeat, and a walk that stopped on our own bound can no longer read as the strap's answer.
• countOvershootAllowance = 4 is calibrated, not picked. Announced 16, served to 18, allowance twice the observed excess, still inside the 128 cap.
• Symmetric across both platforms with tests on each, and it merges clean over the new §2.4 — different section.

Fix the one string and this is good to go.

[vishk23]

Fixed, in your words — the replacement is yours verbatim, because it says exactly the right thing:

DECISIVE — validKey=0 is a TERMINATOR on this firmware: the next request returned the same index with validKey=0 again, so the cursor does not advance past it, so this walk cannot see anything beyond. Whether the list truly ends here is not separable from a firmware whose cursor parks on an empty slot.

You are right that this was my own #914 rule broken in my own PR, and right about the specific danger: the probe would have printed, as DECISIVE, the conclusion the PR was written to question. The DECISIVE label stays, because something is genuinely decided; only the claim narrows.

It was in four places, not one. The string you quoted, plus the StopCode.emptySlotCursorParked doc comment on both platforms ("behaves as a terminator rather than as an empty slot" — the same over-claim, quieter), plus both test doc comments ("A parked cursor has nothing past it, so validKey=0 really is a terminator"). A reader checking whether the verdict was justified would have found the doc comments agreeing with it. All four are fixed.

And it is pinned now, in both directions, so it cannot be widened back by an edit that looks like tightening prose:

XCTAssertTrue(finding.contains("the cursor does not advance past it"))
XCTAssertTrue(finding.contains("not separable from a firmware whose cursor parks on an empty slot"))
XCTAssertFalse(finding.contains("there is nothing past it"))

Kotlin twins of all three. The EMPTY/RETIRED SLOT branch is untouched — as you say, more names after a validKey=0 is direct evidence, and "would have been truncated here" is what was observed.

Your counter-evidence is worth keeping visible and I have not tried to argue it away: on my strap the firmware skipped indices 11 and 12 silently, never serving validKey = 0 for them, which is evidence this firmware advances past holes without a hole record. That makes the parked-on-a-hole reading less likely here. It does not eliminate it, and the string is printed on whatever strap runs it — which is the whole reason the narrower wording is the correct one.

---

The rebase was not clean, and the collision was the one you would care about

Rebasing onto 49830442 put this on top of #913, which landed after this branch was cut. #913 added, immediately above the line this PR rewrites:

if keys.isEmpty && steps == 0 { … "the key list was never read (inconclusive)" }

On Swift that survived the auto-merge. On Kotlin it was inside the conflict hunk, and this PR's side does not contain it — taking "theirs" would have silently deleted #913's fix from Android while Swift kept it. Resolved by keeping #913's guard and folding this PR's changes into it. Verified explicitly after the rebase, on both platforms: the guard is present in FeatureFlagProbe.swift:770 and FeatureFlagProbe.kt:768, and both of #913's tests still exist and still pass.

Two real interaction bugs fell out of that merge, neither of which either PR could have caught alone:

1. announcedFlags (added by Don't call an unread feature-flag key list "the strap named none" (#761) #913) hardcoded "flag(s)" — correct when 117/118 was the only walk, wrong now that the same code walks 115/116, where a device-config report would have announced "16 flag(s)".
2. Don't call an unread feature-flag key list "the strap named none" (#761) #913's inconclusive verdict named SEND_NEXT_FF(118) literally, so a 115/116 walk would have reported itself as a feature-flag walk.

Both now read from namespace (entryNoun, nextLabel/nextCmd). The feature-flag rendering is byte-identical to before, so #913's tests pass unchanged — which is what confirms the fix is a generalisation and not a behaviour change.

Verified

• swift test WhoopProtocol: 458 tests, 0 failures.
• Kotlin testFullDebugUnitTest: 3238 tests, 0 failures, 0 errors, 5 skipped across 393 classes, from the JUnit XML. FeatureFlagProbeTest is 46 of them.

Worth one note on that Kotlin number: my first resolution of the conflict left an unbalanced brace, and ./gradlew caught it as a compile error. Since upstream CI does not gate Kotlin unit tests, that would have merged green and broken the Android build — so the intermediate commit is amended rather than fixed in a follow-up, and neither commit on this branch is broken.