From 3a84edf4e17706c87d5bda00abbdc01a7d2fbd5a Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Fri, 28 Aug 2026 22:41:02 +0200 Subject: [PATCH 1/2] Tests for the guards whose removal fails open Mutation testing across thirteen suites (114 mutations) found guards that no test protects. These are the ones where broken code does something rather than nothing. RELAYS. patternFor() refuses mode "none", and that guard is load-bearing, not defensive: isValidRole("none") returns true, so without it "none" reaches the match loop. applyDrmMode() pads the roles list with "none" up to the relay count, and REST validates only that the mode string is 1..16 characters -- so POST /api/v1/drm/set ?mode=none would energise every unassigned relay on a 6CH wired to DRM inputs. The test that claimed this passed {"drm0"}, where the loop finds no match and returns false whether the guard is there or not. It now passes a roles list that contains "none". Also covers optionsFor()'s isValidRole filter, which no fixture reached. WRITES. writeSingleRegister had no tests at all -- the name did not appear anywhere under test/ -- while modbus_profile and sunspec both call it. Replacing its echo comparison with `accepted = true` left the suite green, so a device echoing a different address or value would have been recorded as Ok: a setpoint that never arrived, confirmed as delivered. Five cases now: a good echo, a wrong address, a wrong value, an exception with its code, and silence. Two more codec guards had the same shape. parseReadResponse's odd-byte-count check (a malformed frame would decode as one good register) and parseWriteResponse's function-code echo on the SUCCESS path -- covered only via the exception path, which is a different branch, as the mutation run confirmed by leaving that test green while the new one failed. RESPONSE SIZE. json_limits::finish() is the single choke point all 44 payload builders share. Deleting its `needed > maxBytes` check left three "still fits in the response" tests green, because their fixtures are naturally well under the cap; only test_oversized_response_is_refused, which forces maxBytes=50, actually covered it. Each of the three now also asserts the builder refuses at one byte under the measured size -- self-calibrating, so it cannot go vacuous as payloads grow. Four tests catch that mutation now instead of one. CAPABILITIES. test_a_writable_driver_flips_the_read_only_register asserted TEST_ASSERT_TRUE(anyWriteBit), an OR across four registers. Publishing the READ bitmap into the write registers kept the OR true, so the one test that says which channels are writable agreed with the wrong bitmap. It now compares the reconstructed 64-bit value against the driver's own declaration, and asserts the fixture's read and write sets differ so the comparison can tell them apart. Every assertion here was proven by breaking the code it guards and watching it fail, then restoring. 1030 native cases pass; check_layering.sh passes. --- test/test_config_backup/test_main.cpp | 11 ++++ test/test_drm/test_main.cpp | 20 ++++++- test/test_modbus/client.cpp | 78 +++++++++++++++++++++++++++ test/test_modbus/rtu.cpp | 50 +++++++++++++++++ test/test_register_map/test_main.cpp | 20 +++++-- test/test_rest/test_main.cpp | 22 ++++++++ 6 files changed, 196 insertions(+), 5 deletions(-) diff --git a/test/test_config_backup/test_main.cpp b/test/test_config_backup/test_main.cpp index ea7b182..a7207ee 100644 --- a/test/test_config_backup/test_main.cpp +++ b/test/test_config_backup/test_main.cpp @@ -418,6 +418,17 @@ static void test_the_worst_case_preview_still_fits_its_bound() { "the worst-case preview must not exceed kMaxRestorePreviewBytes"); TEST_ASSERT_TRUE_MESSAGE(out.size() < heliograph::rest::kMaxRestorePreviewBytes, "and must fit with room to spare, not exactly"); + + // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small + // enough; it says nothing about whether anything would stop a larger one. Mutation + // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single + // choke point all 44 payload builders share -- and this test stayed green. Refusing at + // one byte under the measured size is self-calibrating: it cannot go vacuous when the + // payload grows. + std::string refused = "untouched"; + TEST_ASSERT_FALSE(heliograph::rest::buildRestorePreviewPayload(bc, diff, true, true, refused, + out.size() - 1)); + TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str()); } static void test_diff_reports_a_changed_list_position_by_position() { diff --git a/test/test_drm/test_main.cpp b/test/test_drm/test_main.cpp index 164901b..66aa571 100644 --- a/test/test_drm/test_main.cpp +++ b/test/test_drm/test_main.cpp @@ -35,6 +35,15 @@ static void test_options_derive_from_roles() { // Duplicate roles (two relays on one line) appear once. TEST_ASSERT_EQUAL_UINT32(2, optionsFor({"drm0", "drm0"}).size()); + + // A role that is not "none" but is not valid either. Every fixture above feeds + // optionsFor roles it already accepts, so the isValidRole() filter was never + // exercised: garbage in the stored config would have been offered as a mode the + // user could pick, and patternFor() then refuses it -- a dead entry in the select. + const auto filtered = optionsFor({"drm0", "drm9", "DRM0", "", "drm12"}); + TEST_ASSERT_EQUAL_UINT32(2, filtered.size()); + TEST_ASSERT_EQUAL_STRING("normal", filtered[0].c_str()); + TEST_ASSERT_EQUAL_STRING("drm0", filtered[1].c_str()); } static void test_pattern_asserts_exactly_the_role() { @@ -62,8 +71,17 @@ static void test_invalid_modes_are_refused() { TEST_ASSERT_FALSE(patternFor({"drm0"}, "drm5", pattern)); // "custom" is a reported state, never a command. TEST_ASSERT_FALSE(patternFor({"drm0"}, "custom", pattern)); - // "none" is a role, never a mode. + // "none" is a role, never a mode. The roles list MUST contain a "none" for this to + // prove anything: with only {"drm0"} the match loop finds nothing and returns false + // whether the guard survives or not, so the assertion passed against code that had + // lost it. isValidRole("none") is true, so "none" reaches that loop -- and every + // unassigned relay carries exactly that role, because applyDrmMode() pads the list + // with it. Drop the guard and POST /api/v1/drm/set?mode=none energises all of them. TEST_ASSERT_FALSE(patternFor({"drm0"}, "none", pattern)); + TEST_ASSERT_FALSE(patternFor({"none", "drm0", "none"}, "none", pattern)); + for (const bool on : pattern) { + TEST_ASSERT_FALSE(on); + } // "normal" without any roles is meaningless. TEST_ASSERT_FALSE(patternFor({"none"}, "normal", pattern)); // Refusal always leaves a released pattern behind, so a caller that ignores the diff --git a/test/test_modbus/client.cpp b/test/test_modbus/client.cpp index dc1f916..e465576 100644 --- a/test/test_modbus/client.cpp +++ b/test/test_modbus/client.cpp @@ -41,6 +41,18 @@ std::vector exceptionReply(uint8_t unit, uint8_t fn, uint8_t code) { return f; } +/// The echo a device sends back for 0x06: the address and the value it accepted, verbatim. +std::vector writeEcho(uint8_t unit, uint16_t address, uint16_t value) { + std::vector f{unit, + kWriteSingleRegister, + static_cast(address >> 8), + static_cast(address & 0xFF), + static_cast(value >> 8), + static_cast(value & 0xFF)}; + test::appendModbusCrc(f); + return f; +} + } // namespace static void test_a_good_reply_decodes() { @@ -189,6 +201,67 @@ static void test_a_zero_length_read_is_refused() { TEST_ASSERT_TRUE(t.writes.empty()); } +// THE WRITE PATH HAD NO TESTS AT ALL. Every case below is new, and the reason they are worth +// having is the one the header states: a write whose echo is not verified is a request, not a +// setting. Mutation testing found this by replacing the echo comparison with `accepted = true` +// and watching the whole suite stay green -- nothing here called writeSingleRegister, while two +// production drivers do (modbus_profile and sunspec). +static void test_a_write_that_echoes_what_was_sent_is_a_setting() { + MockTransport t; + t.replyWith(writeEcho(kUnit, 0x0410, 750)); + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Ok, r.status); +} + +// The two that matter. A device that answers about a DIFFERENT register, or with a different +// value than the one asked for, has not done what was asked however well-formed the frame is. +// Reported as Protocol -- "intact, but not what we asked for" -- and never as Ok, because on a +// control register the difference is between an inverter that is limited and one everybody +// believes is limited. +static void test_a_write_echoing_another_address_is_not_a_setting() { + MockTransport t; + t.replyWith(writeEcho(kUnit, 0x0411, 750)); // neighbouring register + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Protocol, r.status); +} + +static void test_a_write_echoing_another_value_is_not_a_setting() { + MockTransport t; + t.replyWith(writeEcho(kUnit, 0x0410, 1000)); // clamped by the device, or stale + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Protocol, r.status); +} + +// A refusal is the ORDINARY answer to a control write -- read-only register, value out of +// range, a device wanting an unlock first -- so it carries its code rather than collapsing +// into a protocol error. +static void test_a_refused_write_carries_its_exception_code() { + MockTransport t; + t.replyWith(exceptionReply(kUnit, kWriteSingleRegister, 0x04)); // slave device failure + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Exception, r.status); + TEST_ASSERT_EQUAL_UINT8(0x04, r.exceptionCode); +} + +// Silence must never read as "written". This is the failure an unpowered or mis-addressed +// device produces, and it is the one most likely to be met in the field. +static void test_a_write_nobody_answers_is_a_timeout() { + MockTransport t; + t.replyWithSilence(); + + const auto r = writeSingleRegister(t, kUnit, 0x0410, 750); + + TEST_ASSERT_EQUAL(TransactionStatus::Timeout, r.status); +} + void run_modbus_client() { RUN_TEST(test_a_good_reply_decodes); RUN_TEST(test_an_exception_reports_its_code); @@ -200,4 +273,9 @@ void run_modbus_client() { RUN_TEST(test_a_reply_with_too_few_registers_is_refused); RUN_TEST(test_a_reply_with_exactly_the_requested_count_is_ok); RUN_TEST(test_a_zero_length_read_is_refused); + RUN_TEST(test_a_write_that_echoes_what_was_sent_is_a_setting); + RUN_TEST(test_a_write_echoing_another_address_is_not_a_setting); + RUN_TEST(test_a_write_echoing_another_value_is_not_a_setting); + RUN_TEST(test_a_refused_write_carries_its_exception_code); + RUN_TEST(test_a_write_nobody_answers_is_a_timeout); } diff --git a/test/test_modbus/rtu.cpp b/test/test_modbus/rtu.cpp index 4bbdc7d..9fb7d28 100644 --- a/test/test_modbus/rtu.cpp +++ b/test/test_modbus/rtu.cpp @@ -290,6 +290,54 @@ static void test_write_exception_with_a_bad_crc_indicts_the_cable() { parseWriteResponse(frame, 5, 0x01, kWriteSingleRegister, resp)); } +// Both cases below were found by mutation testing: deleting the guard each one exercises left +// the whole suite green, so the branch was validated by nothing. + +// A byte count that is not a whole number of 16-bit registers. Without the guard the count is +// silently halved, the odd trailing byte is dropped, and a malformed frame decodes as if it +// were one good register -- a wrong reading with no error anywhere to say so. +static void test_a_read_reply_with_an_odd_byte_count_is_malformed() { + uint8_t frame[8]; + frame[0] = 0x01; + frame[1] = kReadHoldingRegisters; + frame[2] = 0x03; // three bytes: one register and a half + frame[3] = 0x12; + frame[4] = 0x34; + frame[5] = 0x56; + const uint16_t crc = crc16(frame, 6); + frame[6] = static_cast(crc & 0xFF); + frame[7] = static_cast((crc >> 8) & 0xFF); + + uint16_t regs[4] = {}; + ReadResponse resp; + TEST_ASSERT_EQUAL( + ParseResult::Malformed, + parseReadResponse(frame, sizeof(frame), 0x01, kReadHoldingRegisters, regs, 4, resp)); +} + +// The function-code check on the write SUCCESS path. It was covered only through the exception +// path (test_write_exception_with_a_foreign_function_is_refused), which reaches a different +// branch -- parseExceptionFrame. A plain, well-formed echo for a function we never sent would +// otherwise be accepted as confirmation of our write: on a multidrop bus, that is somebody +// else's reply being read as our setpoint landing. +static void test_a_write_reply_echoing_a_foreign_function_is_refused() { + uint8_t frame[8]; + frame[0] = 0x01; + frame[1] = kWriteMultipleRegisters; // 0x10, and we sent 0x06 + frame[2] = 0x00; + frame[3] = 0x10; + frame[4] = 0x00; + frame[5] = 0x01; + const uint16_t crc = crc16(frame, 6); + frame[6] = static_cast(crc & 0xFF); + frame[7] = static_cast((crc >> 8) & 0xFF); + + WriteResponse resp; + TEST_ASSERT_EQUAL( + ParseResult::WrongFunction, + parseWriteResponse(frame, sizeof(frame), 0x01, kWriteSingleRegister, resp)); +} + void run_modbus_rtu() { RUN_TEST(test_crc_matches_the_canonical_vector); RUN_TEST(test_crc_second_vector); @@ -310,6 +358,8 @@ void run_modbus_rtu() { RUN_TEST(test_write_single_echo_is_validated); RUN_TEST(test_write_exception_is_reported); RUN_TEST(test_write_exception_with_a_foreign_function_is_refused); + RUN_TEST(test_a_read_reply_with_an_odd_byte_count_is_malformed); + RUN_TEST(test_a_write_reply_echoing_a_foreign_function_is_refused); RUN_TEST(test_write_exception_from_another_unit_is_refused); RUN_TEST(test_write_exception_with_a_bad_crc_indicts_the_cable); } diff --git a/test/test_register_map/test_main.cpp b/test/test_register_map/test_main.cpp index c612374..4a7875a 100644 --- a/test/test_register_map/test_main.cpp +++ b/test/test_register_map/test_main.cpp @@ -488,10 +488,22 @@ static void test_a_writable_driver_flips_the_read_only_register() { map.update(*store.snapshot(), BridgeInfo{}, diag.snapshot(), g_now); TEST_ASSERT_EQUAL_UINT16(0, map.at(reg::kDriverReadOnly)); - const bool anyWriteBit = map.at(reg::kCapabilitiesWrite) || map.at(reg::kCapabilitiesWrite + 1) || - map.at(reg::kCapabilitiesWrite + 2) || - map.at(reg::kCapabilitiesWrite + 3); - TEST_ASSERT_TRUE(anyWriteBit); + // The registers must carry the driver's WRITE set, not merely something non-empty. This + // was TEST_ASSERT_TRUE(anyWriteBit) -- an OR across the four registers -- and mutation + // testing published the READ bitmap into them instead: every working driver has a + // non-empty read set, so the OR stayed true and the one test that says which channels are + // writable agreed with the wrong bitmap. A client reading it would have been told it could + // write everything the device can report. + const auto& caps = driver.capabilities(); + const uint64_t wrote = (static_cast(map.at(reg::kCapabilitiesWrite)) << 48) | + (static_cast(map.at(reg::kCapabilitiesWrite + 1)) << 32) | + (static_cast(map.at(reg::kCapabilitiesWrite + 2)) << 16) | + static_cast(map.at(reg::kCapabilitiesWrite + 3)); + TEST_ASSERT_EQUAL_UINT64(caps.write.to_ullong(), wrote); + // Without this the assertion above could not tell the two bitmaps apart, and would go + // quietly vacuous again the day the mock's sets happened to coincide. + TEST_ASSERT_TRUE_MESSAGE(caps.read.to_ullong() != caps.write.to_ullong(), + "the fixture must declare different read and write sets"); } // The agreement below is only tested AFTER a poll. Before one -- at boot, or all night on an diff --git a/test/test_rest/test_main.cpp b/test/test_rest/test_main.cpp index 14a9768..028e49a 100644 --- a/test/test_rest/test_main.cpp +++ b/test/test_rest/test_main.cpp @@ -646,6 +646,18 @@ static void test_a_full_bus_of_summaries_still_fits() { // far over the cap a new field pushed it instead of only that it did. TEST_ASSERT_LESS_THAN_UINT32(rest::kMaxResponseBytes, json.size()); TEST_ASSERT_EQUAL_UINT32(kMaxDevices, parse(json)["devices"].size()); + + // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small + // enough; it says nothing about whether anything would stop a larger one. Mutation + // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single + // choke point all 44 payload builders share -- and this test stayed green. Refusing at + // one byte under the measured size is self-calibrating: it cannot go vacuous when the + // payload grows. + std::string refused = "untouched"; + TEST_ASSERT_FALSE(rest::buildStatusPayload(state, "modbus_profile-1", makeBridge(), + r.diagnostics.snapshot(), &eversolar::descriptor(), + g_now, fleet, refused, json.size() - 1)); + TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str()); } // A configured device that is not polling is the failure every mistake on the settings page @@ -1765,6 +1777,16 @@ static void test_a_capture_filled_to_its_byte_ceiling_fits_in_the_response() { std::string body; TEST_ASSERT_TRUE(rest::buildCapturePayload(report, 0, body)); TEST_ASSERT_TRUE(body.size() <= rest::kMaxCaptureResponseBytes); + + // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small + // enough; it says nothing about whether anything would stop a larger one. Mutation + // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single + // choke point all 44 payload builders share -- and this test stayed green. Refusing at + // one byte under the measured size is self-calibrating: it cannot go vacuous when the + // payload grows. + std::string refused = "untouched"; + TEST_ASSERT_FALSE(rest::buildCapturePayload(report, 0, refused, body.size() - 1)); + TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str()); } // --- restore preview ---------------------------------------------------------------------- From 74997a82eddea9a23292930fcf889789618e9b8a Mon Sep 17 00:00:00 2001 From: Tim de Bruijn Date: Sat, 29 Aug 2026 00:20:21 +0200 Subject: [PATCH 2/2] Twenty-four, not forty-four Review caught an overclaim in three of the comments this branch added: finish() is shared by 24 call sites, not 44. The 44 came from a grep that counted its definition, its own comments and unrelated `finish(` lines along with the calls -- the same shape of mistake these tests exist to catch, made while writing them. The argument is unchanged, and the number was never load-bearing. But a comment that overstates its evidence is the thing that makes the next reader trust the next number, and this file has been bitten by exactly that before. Worth recording separately, because it is a real gap rather than a wording fix: finish() is not the only serialiser either. home_assistant_discovery.cpp carries a private serialise() that is finish() minus the size check, used for every discovery entity. Addressed on the branch that changes production code, not here. --- test/test_config_backup/test_main.cpp | 2 +- test/test_rest/test_main.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_config_backup/test_main.cpp b/test/test_config_backup/test_main.cpp index a7207ee..934c19b 100644 --- a/test/test_config_backup/test_main.cpp +++ b/test/test_config_backup/test_main.cpp @@ -422,7 +422,7 @@ static void test_the_worst_case_preview_still_fits_its_bound() { // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small // enough; it says nothing about whether anything would stop a larger one. Mutation // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single - // choke point all 44 payload builders share -- and this test stayed green. Refusing at + // choke point twenty-four payload builders share -- and this test stayed green. Refusing at // one byte under the measured size is self-calibrating: it cannot go vacuous when the // payload grows. std::string refused = "untouched"; diff --git a/test/test_rest/test_main.cpp b/test/test_rest/test_main.cpp index 028e49a..fd65a14 100644 --- a/test/test_rest/test_main.cpp +++ b/test/test_rest/test_main.cpp @@ -650,7 +650,7 @@ static void test_a_full_bus_of_summaries_still_fits() { // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small // enough; it says nothing about whether anything would stop a larger one. Mutation // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single - // choke point all 44 payload builders share -- and this test stayed green. Refusing at + // choke point twenty-four payload builders share -- and this test stayed green. Refusing at // one byte under the measured size is self-calibrating: it cannot go vacuous when the // payload grows. std::string refused = "untouched"; @@ -1781,7 +1781,7 @@ static void test_a_capture_filled_to_its_byte_ceiling_fits_in_the_response() { // AND THE BOUND ACTUALLY BITES. The assertion above only says this payload is small // enough; it says nothing about whether anything would stop a larger one. Mutation // testing deleted the `needed > maxBytes` check in json_limits::finish() -- the single - // choke point all 44 payload builders share -- and this test stayed green. Refusing at + // choke point twenty-four payload builders share -- and this test stayed green. Refusing at // one byte under the measured size is self-calibrating: it cannot go vacuous when the // payload grows. std::string refused = "untouched";