Tests for the guards whose removal fails open

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 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";
+    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() {

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

test/test_modbus/client.cpp

@@ -41,6 +41,18 @@ std::vector<uint8_t> 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<uint8_t> writeEcho(uint8_t unit, uint16_t address, uint16_t value) {
+    std::vector<uint8_t> f{unit,
+                           kWriteSingleRegister,
+                           static_cast<uint8_t>(address >> 8),
+                           static_cast<uint8_t>(address & 0xFF),
+                           static_cast<uint8_t>(value >> 8),
+                           static_cast<uint8_t>(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);
 }

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<uint8_t>(crc & 0xFF);
+    frame[7]           = static_cast<uint8_t>((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<uint8_t>(crc & 0xFF);
+    frame[7]           = static_cast<uint8_t>((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);
 }

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<uint64_t>(map.at(reg::kCapabilitiesWrite)) << 48) |
+                           (static_cast<uint64_t>(map.at(reg::kCapabilitiesWrite + 1)) << 32) |
+                           (static_cast<uint64_t>(map.at(reg::kCapabilitiesWrite + 2)) << 16) |
+                           static_cast<uint64_t>(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

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 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";
+    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 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";
+    TEST_ASSERT_FALSE(rest::buildCapturePayload(report, 0, refused, body.size() - 1));
+    TEST_ASSERT_EQUAL_STRING("untouched", refused.c_str());
 }
 
 // --- restore preview ----------------------------------------------------------------------