Don't call an unread feature-flag key list "the strap named none" (#761)

[vishk23]

The defect

FeatureFlagProbeReport.verdict (Packages/WhoopProtocol/.../FeatureFlagProbe.swift) returned

strap announced N flag(s) but named none

whenever keys was empty and the strap had answered 117 — with no check that a single SEND_NEXT_FF(118) reply was ever decoded.

The path is reachable, and it is plausibly the first-contact result on a 5/MG:

1. BLEManager.probeFeatureFlags() sends START_FF_KEY_EXCHANGE(117)
2. the reply lands → noteStart(r) records reportedCount
3. sendFeatureFlagStep(.sendNextFeatureFlag) puts 118 on the wire
4. the 8s timer fires → noteTimeout(command: 118, seconds: 8) appends a trace line and sets stopReason
5. finishFeatureFlagProbe() renders

Zero 118 replies were received — yet the headline asserts a fact about the strap's key list. It blames the strap for our timeout, and it's the sentence someone would paste into #103 as evidence that the enumerate verb answers but serves nothing. The run established no such thing.

It also contradicts the principle the file states two branches above and applies correctly to skipped > 0:

"named none" would blame the strap for OUR decode. […] never report our limitation as the strap's behaviour.

Same defect class as the ECG probe verdict fixed in #896: a verdict asserting a conclusion the run's own inputs could not support.

The fix

steps (the decoded-118-reply counter) already existed, so the gate is one condition:

if keys.isEmpty && steps == 0 {
    return "strap announced \(announcedFlags); no SEND_NEXT_FF(118) reply was decoded — "
        + "the key list was never read (inconclusive)"
}

Worded "no reply was decoded" rather than "no reply arrived" because steps == 0 also covers a 118 reply that failed to parse (noteFailure), where the strap did answer. stopReason renders directly beneath the verdict and names which of the two it was, so one branch covers both without over-claiming either.

A 118 reply that genuinely walked to the end marker carrying no name is the opposite case — a real fact about the strap — and still says "named none" plainly, unchanged.

Secondary (judgement call, flagging it explicitly): the verdict also restated the announced count as bare fact even when countIsPlausible was false. noteStart marks an implausible count in the trace (:277), but the verdict is the line that gets quoted, so it now carries the same doubt (an implausible 9999 flag(s)). A plausible count renders exactly as before — real reports are byte-for-byte unchanged. Happy to drop this half if you'd rather keep the change to the single gate.

Test change — the old test pinned the bug

testAnnouncedCountWithNoNamesIsSaidPlainly did noteStart + noteTimeout(118) and asserted the "named none" string: it pinned the buggy path as intended behaviour. Split into the two cases it was conflating:

test	case	verdict
…WithNo118ReplyIsInconclusiveNotBlamedOnTheStrap	117 answered, 118 timed out	inconclusive; asserts !contains("named none")
…WithARealEmptyWalkIsSaidPlainly	118 replied with the 0xFF end marker	"named none" — unchanged
…ImplausibleAnnouncedCountIsNotRestatedAsFactInTheVerdict	count outside 1…128	count carries the doubt

Kotlin twins added for all three; rendered strings stay byte-identical across platforms per the parity contract.

Verification

Both new tests were confirmed to fail against the unfixed source before the fix was restored — Swift 4 assertions, Kotlin 2 tests — so they demonstrably catch the defect rather than merely describing it. The unchanged-behaviour test passed on both sides, as intended.

suite	result
cd Packages/WhoopProtocol && swift test	427 tests, 0 failures
cd android && ./gradlew testFullDebugUnitTest (JDK 17)	3198 tests, 0 failures, 5 skipped

Android counts are read from app/build/test-results/testFullDebugUnitTest/*.xml (391 files) on a forced --rerun, not from the Gradle exit code — a plain BUILD SUCCESSFUL can hide a cached or skipped test task.

Pure Packages/ + Kotlin protocol change: no app-target Swift touched, no BLE behaviour changed, no new storage. The probe stays read-only — this only changes the sentence it prints.

Refs #761.

[ryanbr]

The defect is real and I checked it on main rather than from the description. The keys.isEmpty branch is at :387 with no steps guard, and the comment stating the principle it breaks —

Same class as isSkippable: never report our limitation as the strap's behaviour.

— is at :377. Eight lines apart. The file states the rule and then breaks it in the next branch, which is the most convincing form this kind of report can take. steps already exists at :255, so the gate really is one condition.

Wording the guard as "no reply was decoded" rather than "no reply arrived" is the right call, and the reason given is the correct one: steps == 0 also covers a 118 reply that failed to parse, where the strap did answer. One branch, neither over-claim, with stopReason underneath to say which.

The thing that actually needs deciding: this collides with #917

Both rewrite FeatureFlagProbe on both platforms, and they conflict — I merge-tested the two heads against each other, not just against main.

Worse than a textual conflict: #917 touches this exact line and does not fix it.

-            return "strap announced \(reportedCount ?? 0) flag(s) but named none"
+            return "strap announced \(reportedCount ?? 0) \(namespace.entryNoun)(s) but named none"

It parameterises the noun and carries the defect forward, reworded. So the guard has to survive whichever order these land in, and the rebase is where it would quietly not:

• #913 first (my preference — small, focused, one condition) — then #917 rebases onto it and must keep the steps == 0 gate. #917 rewrites both test files too (+405/-30 Swift, +446/-27 Kotlin), so the test that pins the gate can disappear in the same rebase as the gate. Worth checking explicitly after the rebase that the guard's test still exists and still fails without the guard.
• #917 first — then this reapplies on top of entryNoun, which is a smaller edit but leaves the defect on main in the meantime, and #917 is the larger review.

Either way it is worth saying in #917 that this line is spoken for.

The secondary change

Keep it. An implausible count that the trace flags but the verdict restates as bare fact is the same defect in miniature — the verdict is the line that gets quoted, and :277 already knows the count is doubtful. Real reports are unchanged because a plausible count renders as before, which is the property that makes it safe.

Test change

Splitting testAnnouncedCountWithNoNamesIsSaidPlainly is right, and worth being explicit about what it was: the old test asserted the buggy string, so it pinned the defect as intended behaviour. A test can do that, and when it does it is the reason a defect survives review — the suite goes green and the wrong sentence looks deliberate.

The failing check is not yours

check is red on AppChangelog.kt:42, a hardcoded What's New title on a line this PR does not touch. Cutting 9.2.1 added it — appchangelog-gen.py still writes that title as a raw literal (#878, reopened). Fixed on main in #916; the audit is a whole-tree gate, so it fails every PR branched before that. #914 hit the same thing and cleared on a branch update.