🔥 FIX - a linear group reports the level its members were set from, not their raw brightness 🎚️ (#10)

[vibechoom]

Refs #10

Implements the light_group_linear.rs TODO, "Inverse map member brightness to group brightness".

The bug

LightGroupLinear::calculate_group_state pushed each lit member's raw brightness into re_derive, so the group averaged member levels as if they were group levels. Every member goes through its own [min, max] range, so after any re-derive the group drifted off the level it had been set to.

Worked example, using the shipped virtual_devices/bedroom_lights.toml (top 80-100, main 40-90, bed 0-50):

step	members (top / main / bed)	group before	group now
set group to 50	90 / 65 / 25	50	50
bed turned off at the wall	90 / 65 / off	(90+65)/2 = 77	50
bed dimmed to 10 at the wall	90 / 65 / 10	(90+65+10)/3 = 55	(50+50+20)/3 = 40
plain re-derive of the set state	90 / 65 / 25	(90+65+25)/3 = 60	50

(The accounts_for guard from #22 already hid the last row for pure echoes. Any real member change exposed it.)

Note: #10 describes breakpoint curves, but the linear group maps each member through a two-point [min, max] range (map_brightness). The breakpoint curves (BrightnessCurve) belong to the other group type, LightGroup. That type has the same raw push, but it's outside this change (see follow-ups).

The inversion rule

The fix changes only what goes into re_derive. The aggregation itself (average of the lit members, off when none are lit, level kept) is unchanged.

• Candidates. A lit member at level m has as candidates the group levels 1..=100 whose fan-out lights it at exactly m. If no level does, the candidates are the levels that light it nearest to m. So a member dimmed below its range, or turned up past it, clamps to the bottom or top of that range. The candidates come from scanning the existing forward map (map_brightness), so the forward logic isn't duplicated and the rounding and clamping match exactly. The candidate set is a u128 bitset.
• Flat segments. Several group levels can light a member at the same level: rounding when a range is narrower than 100 (top moves 1 per 5 group levels), the clamp at 100 when max > 100, or min == max. The choice among them is deterministic:
  1. If some level is a candidate of every lit member, the members are exactly where that one level puts them. Each inverts to the level among those nearest the group's set level, and that level becomes the set level. The set level is the level the group last took from a write, or found every member at. It starts at DEFAULT_GROUP_LEVEL.
  2. Otherwise each member inverts to its own candidate nearest the set level. A member still where the group put it inverts to the set level itself. A member moved at the wall inverts to the nearest level that puts it where it is.
  3. Ties go to the lower level.
• Non-monotonic. A linear range can't be non-monotonic. It rises, falls (the loader accepts min > max), or is flat, and the loader has no validation to rely on. The rule doesn't assume monotonicity anyway, because it scans the fan-out. A unit test covers a V-shaped fan-out with candidates on both sides of the dip, plus the tie rule.
• Unchanged cases. A member that's off, or on at level 0, is left out, as before. A member with no range falls back to identity through map_brightness (new rejects a member without a range anyway).

Properties:

• A write at any G reads G back exactly: G is a candidate of every member, and it's the set level.
• Re-deriving is idempotent: the result depends only on the members and the set level, and rule 1 moves the set level to a level that is still a candidate for every member.
• A group seeded after a restart (set level 100) reads the highest level that lights its members exactly as they are. That level accounts for all of them.

Tests

• light_group_linear.rs (in-file):
  • a_group_set_to_a_level_reads_it_back_from_its_members: property round trip for G in 0..=100 (plan_write, then members into the store, then take_state, then re-derive; the group reports G). Runs for the shipped bedroom_lights.toml, read from the file, and for hand-made identity (0-100), steep (0-250, flat from 40), flat (70-70) and falling (90-40) ranges, plus all four in one group.
  • a_group_started_on_its_members_reads_the_highest_level_that_puts_them_there: the documented representative for the same groups. A brute-force oracle checks the level, and the group must account for every lit member.
  • a_member_dimmed_at_the_wall_counts_at_the_level_that_puts_it_there: exactly 40, re-deriving again stays 40, and bed back at 25 gives 50.
  • a_member_turned_off_leaves_the_group_at_its_level: 50, not 77.
  • a_group_inverts_towards_the_level_it_found_its_members_at (fix round): pins rule 1 moving set_level. The group starts on members where 50 put them and reads 50. With bed then dimmed to 11 it reads 40. Without the update it reads 41, because top inverts to 52, nearest the stale 100.
  • a_member_level_inverts_to_the_group_levels_that_light_it_there: exact candidate sets for top: 90 → {48..52}, 81 → {3..7}, 30 → {1,2}, 100 → {98,99,100}.
  • a_fan_out_that_falls_and_rises_inverts_to_the_nearest_candidate: non-monotonic fan-out, ties, and both rules of invert.
• tests/linear_group_levels.rs (new, end to end through VirtualDeviceManager and the bus, shipped config):
  • Group set to 50, then bed turned off at the wall: the manager re-derives the group, it reports 50, and nothing is echoed. bed back on leaves it at 50.
  • bed dimmed to 10: the group re-derives to exactly 40, echoed once. Back to 25 gives 50.
• Existing expectations updated, because they pinned the raw average:
  • The dummy's kitchen light (bed, 0-50) starts on at 75, which is past its range. Bedroom Lights therefore now seeds at 100 (the level that gets it nearest) instead of 75. The two controller-gesture tests (tests/dummy_scenario.rs and the axum_server.rs test) now assert the seed of 100 and set the group to 75 first, so inc 10 still shows 85.
  • axum_server.rs outside_member_change_after_a_group_write_re_derives_the_group: 65 becomes 59, i.e. (50 + 50 + 79) / 3.
• Echo-skip guards (fix round, 🔥 FIX - a linear group reports the level its members were set from, not their raw brightness 🎚️ (#10) #65 review finding 1). Re-deriving a linear group from members at its own fan-out is now exact, so for that type the accounts_for skip only saves work, and the tests that guarded it over Bedroom Lights lost their teeth. The skip still matters for a curved LightGroup, which re-derives from raw levels (set to 50, it reads 57 or 60). The guards therefore run over a curved group again:
  • manager.rs: lossy_group (k in input_tracking_catches_the_groups_up_after_falling_behind) is now a curved LightGroup with the same 80-100 and 0-50 shapes. It has two new guards: a_curved_group_is_not_re_derived_from_its_own_echoes (own echoes, and two back-to-back writes' stale echoes) and a_member_colour_the_hub_normalised_does_not_re_derive_a_curved_group.
  • axum_server.rs: home_with(kind) registers either Bedroom Lights or a curved group that lights the same members exactly the same way (MEMBERS_AT_50 / MEMBERS_AT_80). virtual_group_write_echoes_group_and_members, group_off_then_on_restores_members, back_to_back_level_changes_end_at_the_last_level and hub_normalised_member_colour_does_not_re_derive_the_group each run over both.

Mutants

mutant	result
restore the raw-brightness push (the old TODO line)	red, 9 tests: 4 in-file, both e2e tests (77 ≠ 50, 55 ≠ 40), dummy_scenario, 2 API tests
take_state stops tracking the set level	red: round trip and API test
rule 1 doesn't move the set level (light_group_linear.rs:218)	red: a_group_inverts_towards_the_level_it_found_its_members_at (41 ≠ 40). It survived before the fix round.
drop rule 1 (no every-member level)	red: seeded representative test and invert unit test
no accounts_for skip in track_input (manager.rs:638), -p v1bectl_virtual -p v1bectl_api	base 4d40cbb: 9 red (2 in v1bectl_virtual); 1e1fcdf: 1 red (0 in v1bectl_virtual); now: 8 red (3 in v1bectl_virtual: the lag catch-up test and both new manager guards; the 4 axum guards; api_round_trip's curved group)

At the base, a_virtual_group_write_comes_back_over_the_socket, press_button_runs_the_controller… and shipped_button_controller_follows_reported_gestures… also went red without the skip. That was only because Bedroom Lights re-derived lossily. They now re-derive exactly, and each has a curved-group guard covering the same path.

Verification

Re-run on the fix-round head 3562f78:

• cargo fmt --all --check: 0
• cargo clippy --workspace --exclude v1bectl_web --all-targets -- -D warnings: 0 with clippy 0.1.96 (nix develop) and 0.1.98 (nixpkgs)
• cargo test --workspace --exclude v1bectl_web: 0. -p v1bectl_virtual looped 5×: all green.
• RUSTDOCFLAGS='-D warnings' cargo doc --workspace --exclude v1bectl_web --no-deps: 0
• Cargo.lock unchanged

Follow-ups (not in this PR)

Tracked on #66:

• LightGroup::calculate_group_state still averages raw member levels over BrightnessCurve. Levels::inverting and invert take any forward map, so reusing them there is mechanical.
• Stale "lossy re-derive" comments for linear groups: virtual_device.rs (the accounts_for docs), manager.rs:641 (the track_input comment), and docs/VIRTUAL_DEVICES.md.
• bedroom_lights.toml's range comment ("x < min => 0 and x > max = 100") doesn't describe what map_brightness does.

(The weakened echo-skip guards listed here before are fixed in this PR, in the fix round above.)

🤖 Generated with Claude Code

[vibechoom]

Review

Head reviewed: 1e1fcdf (base 4d40cbb)

Verdict: FIX-FIRST. Only the tests need work. The inversion and the set_level rule are sound: I re-derived every changed number by hand and with a probe, and they're right. The blocker is that this PR removes more regression coverage than it says. 8 tests, not 2, lost the ability to catch a missing accounts_for skip, and the v1bectl_virtual crate now has no test that catches it at all. The fix is 8 lines, and I checked it (finding 1).

Findings

1. Medium: the accounts_for guard lost 8 of its 9 red tests, including every one in v1bectl_virtual. lib/v1bectl_virtual/src/manager.rs:3609 (lossy_group), manager.rs:638-641

Mutant: skip removed in track_input (if false && virtual_device.accounts_for(..) at manager.rs:638), run with cargo test --no-fail-fast -p v1bectl_virtual -p v1bectl_api.

• Base 4d40cbb: 9 red. a_light_group_write_comes_back_over_the_socket, a_virtual_group_write_comes_back_over_the_socket, back_to_back_level_changes_end_at_the_last_level, group_off_then_on_restores_members, hub_normalised_member_colour_does_not_re_derive_the_group, press_button_runs_the_controller_bound_to_the_dummy_switch, virtual_group_write_echoes_group_and_members, input_tracking_catches_the_groups_up_after_falling_behind, shipped_button_controller_follows_reported_gestures_against_dummy.
• Head 1e1fcdf: 1 red. Only a_light_group_write_comes_back_over_the_socket (lib/v1bectl_api/tests/api_round_trip.rs:477), because it's the one test over a curved LightGroup.

That's expected: re-deriving a linear group from members at its own fan-out is now exact, so for that type the skip is only a saving. But LightGroup (curves) still pushes raw brightness and is still lossy, and the skip is what keeps it at its level. Failure scenario: a change that breaks the skip, or LightGroup::accounts_for, passes the whole v1bectl_virtual suite. A curved group set to 50 then reads 57 after its own echo, and only the API integration test notices. The PR's follow-up note says "the k / hub-colour tests", which undersells this.

Fix, checked here: point lossy_group at a curved LightGroup. The test is green as is and red without the skip (k was re-derived: 57 ≠ 50):

    ) -> LightGroup {
        let curves = serde_json::json!({
            e: { "breakpoints": [[0, 80], [100, 100]] },
            f: { "breakpoints": [[0, 0], [100, 50]] },
        });
        let config = VirtualDeviceConfig {
            device_id: device_id.to_string(),
            device_type: VirtualDeviceType::LightGroup,
            name: device_id.to_string(),
            description: None,
            enabled: true,
            config: serde_json::json!({ "lights": [e, f], "brightness_curves": curves }),
        };
        LightGroup::new(config, store.clone()).expect("lossy group")

(Its doc at :3606-3608 and the test doc at :3643-3646 then need "curves" in place of "linear ranges".) Doing the same for the API hub-colour test is optional, since api_round_trip already covers the API side.

2. Low: rule 1 moving set_level isn't pinned. lib/v1bectl_virtual/src/light_group_linear.rs:217-219

Mutant: self.set_level = level; becomes let _ = level;. Every test stays green. The change is visible, though. Start on members where 50 put them (top 90, main 65, bed 25), so the seed reads 50. Then dim bed to 11 at the wall: the PR reads 40 (top inverts to 50) and the mutant reads 41 (top inverts to 52, the end of {48..52} nearest the stale 100). With bed at 12 it's 41 against 42. Please add this as a test. The PR's own mutant table covers take_state but not this line.

3. Low: design note, no change needed. A stale set_level only biases a member within its own plateau. light_group_linear.rs:116,121

Rule 2 leaves set_level where it was, so each moved member inverts to the edge of its plateau nearest the old level. With the shipped ranges the plateaus are at most 5 wide (top), so the bias is at most about 1 level on a 3-member average. Measured:

• Your example: set 50, top → 85, bed → 10 at the wall (no common candidate) reads 32 = (27 + 50 + 20) / 3. top's plateau is 23..27, so a centre pick would give about 31.7. Then main → 50 reads 22. Sensible.
• Same members (top 85, main 65, bed 10) after different last writes: 30 if the group was last set to 10, and 32 if it was set to 30, 50, 70 or 100. The history dependence stays small.
• Set 90, then dim members one at a time toward 20's fan-out: 90 → 67 → 44 → 20. Once they all agree, rule 1 snaps to 20 and set_level follows, so the staleness heals.
• A wide plateau is where it would matter: a member with max > 100 (flat above the clamp) or min == max counts at set_level itself under rule 2. That fits the doc ("still where the group put it"), but it's the one case where history really moves the number.
• All members off, then one comes on at the wall: rule 1 with one lit member, so the group becomes that member's level and set_level follows. bed at 25 → 50, top at 81 → 7, bed at 1 → 2. One cosmetic effect: top at 100 reads 98, and main or bed at 100 reads 99, because the stale 50 picks the low end of {98,99,100} or {99,100}. That's acceptable.

4. Nit: the dummy now starts at 100. lib/v1bectl_virtual/src/dummy.rs:163-169

This is correct under the model. bed at 75 is past its 0-50 range, so its candidates are {99, 100} (map(bed, 99) = round(49.5) = 50). Nearest to the seed's 100, that's 100. The alternatives are worse:

• Excluding the member would make the group read off while a member is lit, which breaks re_derive's contract.
• Keeping the raw 75 mixes member and group units, which is the bug being fixed.
• "Clamp to its max" is the same thing as 100.

On a real hub, a member turned up past its max counts as 100: set 50, then bed → 75 at the wall reads 66 = (50 + 50 + 100) / 3 (it was 76 raw). The one user-visible change: on a fresh dummy, toggling off and then on now lights all three at full (top 100, main 90, bed 50) instead of at 75's fan-out. It's worth a line in the docs, and a dummy kitchen light starting within range would also avoid it. The two gesture tests that now "set 75 first" keep their teeth: my rule-1-lowest mutant turns both red.

5. Nit: performance is fine. A 20-member group costs 78 µs per re-derive in release (464 µs in debug), about 90% of it the inversion. That's 20 × 100 map_brightness calls, and each one hashes the member name (light_group_linear.rs:154,207-209). A re-derive runs once for each outside member change the group doesn't account for (its own echoes are skipped), under the virtual_devices write lock, so 20 members moved at once cost about 1.6 ms. That's far inside the 100 ms target. Optionally, look up (min, max) once per member before the scan.

6. Info: not a regression. A scene, or an outer group, that writes this group as one of its devices goes through commit_member_write(.., member_is_virtual = true) (manager.rs:230-262). That writes the store only and never calls take_state. So neither current_state nor set_level moves, and the members aren't fanned out either. This predates the PR. The stale "lossy linear" comments are already in the PR's follow-ups (manager.rs:641, virtual_device.rs:168-169, axum_server.rs:1485-1486, docs/VIRTUAL_DEVICES.md:48).

Changed expectations, recomputed

• 65 → 59 (axum_server.rs). The candidates are top 90 → {48..52}, main 65 → {49, 50} and kitchen 40 → {79, 80}. No level is common to all three, so rule 2 applies: nearest 50 gives 50, 50 and 79, and (50 + 50 + 79) / 3 = 59.67, which re_derive truncates to 59. Correct.
• 40 (in the file and end to end). bed 10 → {19, 20} → 20, and (50 + 50 + 20) / 3 = 40. Correct.
• Seed 100, then set 75 (the gesture tests). See finding 4. Correct, and the tests aren't bent to fit.
• The probe reproduces the in-file candidate sets for top (90 → 48..52, 81 → 3..7, 30 → {1, 2}, 100 → 98..100).

Oracle

The oracle is independent and not circular. It uses only map_brightness, the forward map, which is the spec, plus a brute-force scan for the highest level with the same fan-out. It touches neither Levels nor invert. It only covers members where some level put them exactly (rule 1 on a seed), though. Rule 2 and the out-of-reach "nearest" branch rest on three hand-computed numbers (40, 59, and the V-shape unit test), which is why finding 2 slipped through.

My mutants (--no-fail-fast, v1bectl_virtual + v1bectl_api)

mutant	result
ties go to the higher level (Reverse(level) in nearest)	red: V-shape unit test only. A linear range can't tie, because its candidate sets are contiguous, so the tie rule is only reachable in that synthetic test. Fine.
rule 1 ignores set_level, picks the highest common level	red: round trip
rule 1 ignores set_level, picks the lowest common level	red: 9 tests (in-file, end to end, both gesture tests)
rule 2 ignores set_level, picks each member's highest candidate	red: 1 (the axum 59 test)
rule 1 doesn't move set_level	survives (finding 2)
no accounts_for skip in track_input	red: 1 at head, 9 at base (finding 1)

What I ran, and what was fine

• Detached worktree at 1e1fcdf, nix develop, cargo test -p v1bectl_virtual -p v1bectl_api: all green.
• A throwaway probe test (not pushed) for the scenarios above, the mutants above, the same skip mutant at base 4d40cbb, and the lossy_group re-point with and without the skip.
• Fine:
  • Every write that goes through the group itself updates set_level: the API and button actions both reach write_virtual → commit_write → take_state (manager.rs:960).
  • A failed partial write keeps the old set_level, and tracking then re-derives sensibly. For example, set 50, then a write of 80 fails after top reaches 96: the group reads 59 (by hand: top 96 → {78..82} → 78, so (78 + 50 + 50) / 3).
  • resolve_write always yields a level > 0, so the > 0 filter in take_state never drops a write's level.
  • Re-deriving is idempotent, and the round trip holds for 0..=100 on all six groups.
  • Levels::ALL and the bit operations are correct, and inverting is never empty.
  • The seed and resync paths behave as documented.

This was an independent review by a separate agent (Opus).

[vibechoom]

Switched to draft for the review's FIX-FIRST, which is a test-coverage finding only; the inversion logic checks out. With the manager's accounts_for skip removed as a mutant, only 1 test goes red at this head (9 at the base). The fix round points the lossy_group fixture at a curved LightGroup, which still re-derives lossily, and adds a test for the "found every member at" set_level update (light_group_linear.rs:217-219).

[vibechoom]

Fix round

New head 3562f78, one commit on 1e1fcdf (no rebase, no force-push). It changes tests only: the inversion logic is untouched. Main moved to e8b4520; GitHub reports no conflict, so I didn't merge it.

Finding 1 (Medium): the echo-skip guards have teeth again

The skip still matters for a curved LightGroup, which re-derives from raw member levels, so the guards now run over one:

• manager.rs, test module only:
  • lossy_group is now a curved LightGroup with the same shapes ([[0,80],[100,100]], [[0,0],[100,50]]). This is the reviewer's version, with its docs changed from "ranges" to "curves".
  • Two new guards over it: a_curved_group_is_not_re_derived_from_its_own_echoes (its own echoes at 50, then two back-to-back writes, 80 and 30, whose stale echoes are judged by the store) and a_member_colour_the_hub_normalised_does_not_re_derive_a_curved_group (57 without the skip).
• axum_server.rs, test module only:
  • home_with(kind) registers either Bedroom Lights or a curved group that lights the same members exactly the same way (MEMBERS_AT_50 / MEMBERS_AT_80 hold for both).
  • The four skip guards loop over both kinds: virtual_group_write_echoes_group_and_members, group_off_then_on_restores_members, back_to_back_level_changes_end_at_the_last_level and hub_normalised_member_colour_does_not_re_derive_the_group.

Mutant: remove the skip (if false && virtual_device.accounts_for(..), manager.rs:638), then run cargo test --no-fail-fast -p v1bectl_virtual -p v1bectl_api. I measured all three commits in this worktree:

commit	red	in v1bectl_virtual
base 4d40cbb	9	2
1e1fcdf	1	0
3562f78	8	3

At 3562f78 the 8 are:

• manager: input_tracking_catches_the_groups_up_after_falling_behind, a_curved_group_is_not_re_derived_from_its_own_echoes, a_member_colour_the_hub_normalised_does_not_re_derive_a_curved_group
• axum_server: virtual_group_write_echoes_group_and_members, group_off_then_on_restores_members, back_to_back_level_changes_end_at_the_last_level, hub_normalised_member_colour_does_not_re_derive_the_group
• api_round_trip: a_light_group_write_comes_back_over_the_socket

Three tests that went red at the base don't come back: a_virtual_group_write_comes_back_over_the_socket, press_button_runs_the_controller… and shipped_button_controller_follows_reported_gestures…. They only caught the skip because Bedroom Lights used to re-derive lossily. Each path they cover now has a curved-group guard.

Finding 2 (Low): rule 1 moving set_level is pinned

light_group_linear.rs has a new test, a_group_inverts_towards_the_level_it_found_its_members_at. The group starts on members where 50 put them and reads 50. With bed then dimmed to 11, it reads 40.

Mutant: self.set_level = level; becomes let _ = level; (:218). That turns this test red (41 ≠ 40), and every other test stays green.

PR body

• Tests and mutant table updated with the counts above.
• Follow-ups: the weakened-guard item is gone (fixed here). The rest points at virtual: group level follow-ups after #65 — LightGroup has the same raw-average drift, stale 'lossy re-derive' comments, two guard tests weakened, bedroom_lights.toml range comment #66: LightGroup's raw-average drift, the stale "lossy re-derive" comments, and the bedroom_lights.toml comment.

Gates (on 3562f78)

• cargo fmt --all --check: 0
• cargo clippy --workspace --exclude v1bectl_web --all-targets -- -D warnings (RUSTC_WRAPPER unset): 0 on 0.1.96 (nix develop) and 0 on 0.1.98 (nixpkgs; target-198 deleted)
• cargo test --workspace --exclude v1bectl_web: 0. -p v1bectl_virtual looped 5×: 5/5 green (71 lib tests).
• RUSTDOCFLAGS='-D warnings' cargo doc --workspace --exclude v1bectl_web --no-deps: 0
• Cargo.lock byte-identical