diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fcf98f..104d4e1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -90,6 +90,13 @@ jobs: - name: Layering invariants run: bash tools/check_layering.sh + # The release gate had no gate of its own. It recognised a release tag as /^v[0-9]/ while + # release.yml triggers on "v*", so vtest, v-wip and a bare v all built a release with no + # version check at all. Run here, on every push, rather than only at tag time -- a broken + # gate should surface before the release that needs it. + - name: Release version gate self-test + run: bash tools/check_version.sh --self-test + - name: Embedded JS syntax run: python3 tools/check_web_js.py diff --git a/platformio.ini b/platformio.ini index 165f255..ae28a4f 100644 --- a/platformio.ini +++ b/platformio.ini @@ -122,7 +122,7 @@ lib_deps = ; the Home Assistant discovery generator are tested on the host -- which is exactly where ; a wrong value_template or device_class would otherwise stay invisible until Home ; Assistant quietly shows nothing. - bblanchon/ArduinoJson@^7.4.3 + bblanchon/ArduinoJson@7.4.3 ; --------------------------------------------------------------------------------------- ; Target firmware. One environment per supported board (the HELIOGRAPH_BOARD_* flag picks @@ -301,8 +301,17 @@ lib_deps = esp32async/AsyncTCP@3.5.0 https://github.com/ESP32Async/ESPAsyncWebServer#v3.12.0 miq19/eModbus@1.7.4 - bblanchon/ArduinoJson@^7.4.3 - bertmelis/espMqttClient@^1.7.3 + ; EXACT, like everything above it. These two carried carets (^7.4.3, ^1.7.3), which is not + ; a pin: it accepts any later minor. There is no lockfile -- .pio/ is gitignored -- so the + ; resolved set lives only in whatever machine built last, and a CI runner with a cold cache + ; could produce a different firmware from the same commit without anything saying so. That + ; matters more here than in most projects: this builds an OTA image, and "the version that + ; shipped" has to be reconstructible from the tag. + ; + ; Both were already resolving to the versions named here, and both are the latest release + ; upstream (checked 2026-08-28), so this pins the current behaviour rather than changing it. + bblanchon/ArduinoJson@7.4.3 + bertmelis/espMqttClient@1.7.3 ; The board Tim runs in production (16 MB flash, PCF85063 RTC, no relays). [env:waveshare-rs485-can] diff --git a/src/device/bridge_info.h b/src/device/bridge_info.h index d2487eb..1b5495f 100644 --- a/src/device/bridge_info.h +++ b/src/device/bridge_info.h @@ -113,7 +113,7 @@ struct BridgeInfo { /// reason is not here: it is this boot's resetReason, already above. uint32_t bootCount = 0; bool breadcrumbsCold = true; - uint32_t previousUptimeMs = 0; + uint64_t previousUptimeMs = 0; uint32_t previousFirmware = 0; /// The board this firmware is running on. Reported to Home Assistant as the bridge diff --git a/src/diagnostics/breadcrumbs.cpp b/src/diagnostics/breadcrumbs.cpp index e119f6d..010349c 100644 --- a/src/diagnostics/breadcrumbs.cpp +++ b/src/diagnostics/breadcrumbs.cpp @@ -15,21 +15,41 @@ namespace heliograph::breadcrumbs { namespace { +/// What the twelve checksummed bytes MEAN. Not stored -- it is folded into the CRC, so it +/// costs no RTC bytes and a record written under a different schema simply fails validation. +/// +/// This exists because renaming heartbeatUptimeMs to heartbeatUptimeSeconds changed a field's +/// UNIT without changing the layout, so an old record still CRC'd correctly and was read back +/// as warm: a bridge that had been up three days reported three thousand. That is not a +/// plausible-looking error, it is a thousandfold one, and it would have happened once on every +/// device in the fleet at its first update into that firmware. +/// +/// Bump this whenever a field's meaning, unit or width changes. The cost of bumping is one +/// cold start -- the boot count restarts and the previous life is reported as unknown, which +/// is the honest answer, because the bytes genuinely cannot be interpreted. +inline constexpr uint32_t kStorageSchema = 2; + /// Plain CRC32 (reflected, poly 0xEDB88320), byte at a time, no table. Twelve bytes once a /// second does not justify 1 KB of lookup table in a build where RAM is the scarce resource. -uint32_t crc32(const uint8_t* data, size_t len) { - uint32_t crc = 0xFFFFFFFFu; +/// +/// Takes and returns the RUNNING value, without the final inversion, so a checksum can span +/// more than one buffer -- the schema tag and the struct are two. +uint32_t crc32Update(uint32_t crc, const uint8_t* data, size_t len) { for (size_t i = 0; i < len; ++i) { crc ^= data[i]; for (int b = 0; b < 8; ++b) { crc = (crc >> 1) ^ (0xEDB88320u & (0u - (crc & 1u))); } } - return crc ^ 0xFFFFFFFFu; + return crc; } uint32_t storageCrc(const Storage& s) { - return crc32(reinterpret_cast(&s), offsetof(Storage, crc)); + uint32_t crc = 0xFFFFFFFFu; + crc = crc32Update(crc, reinterpret_cast(&kStorageSchema), + sizeof kStorageSchema); + crc = crc32Update(crc, reinterpret_cast(&s), offsetof(Storage, crc)); + return crc ^ 0xFFFFFFFFu; } } // namespace @@ -42,26 +62,32 @@ BootRecord begin(Storage& storage, uint32_t runningFirmware) { // field says what it was running. Its death reason is this boot's // esp_reset_reason(), which the caller already exposes -- nothing to store. record.coldStart = false; - record.previousUptimeMs = storage.heartbeatUptimeMs; + record.previousUptimeMs = static_cast(storage.heartbeatUptimeSeconds) * 1000ULL; record.previousFirmware = storage.runningFirmware; record.bootCount = storage.bootCount + 1; } // Cold path and warm path converge: write this life's record. On cold, bootCount in // the record defaulted to 1. storage.bootCount = record.bootCount; - storage.heartbeatUptimeMs = 0; + storage.heartbeatUptimeSeconds = 0; storage.runningFirmware = runningFirmware; storage.crc = storageCrc(storage); return record; } -void tick(Storage& storage, uint32_t uptimeMs) { - // One write per second of uptime. The comparison also handles the first call (heartbeat - // starts at 0) and is immune to loop() pace. - if (uptimeMs < storage.heartbeatUptimeMs + 1000) { +void tick(Storage& storage, uint64_t uptimeMs) { + // One write per second of uptime, which is also why storing seconds loses nothing. Takes + // the full 64-bit clock: the caller used to narrow it to uint32 here, and casting a + // wrapped value to a wider type does not un-wrap it. + const uint32_t seconds = static_cast(uptimeMs / 1000); + // <=, not !=. The old millisecond comparison was implicitly monotonic -- it could never + // write a value smaller than the one stored -- and an equality check quietly gave that up. + // esp_timer_get_time() does not run backwards, so nothing reaches this today; the property + // is worth keeping anyway, because the field's whole meaning is "how far this life got". + if (seconds <= storage.heartbeatUptimeSeconds) { return; } - storage.heartbeatUptimeMs = uptimeMs; + storage.heartbeatUptimeSeconds = seconds; storage.crc = storageCrc(storage); } diff --git a/src/diagnostics/breadcrumbs.h b/src/diagnostics/breadcrumbs.h index b0bea0a..3ac00e0 100644 --- a/src/diagnostics/breadcrumbs.h +++ b/src/diagnostics/breadcrumbs.h @@ -50,7 +50,13 @@ namespace heliograph::breadcrumbs { /// once-per-four-billion event that still only costs one fabricated boot count. struct Storage { uint32_t bootCount; - uint32_t heartbeatUptimeMs; + /// SECONDS, not milliseconds. As milliseconds this wrapped at 49.7 days and the record + /// exists precisely to describe a bridge that had been up a long time: a 60-day life + /// reported ~10 days, which reads as a plausible number rather than as an error. Seconds + /// in the same uint32 reach 136 years and keep this struct at sixteen bytes, so the CRC + /// window and the RTC layout are untouched. Nothing is lost -- tick() only writes once + /// per second, so the millisecond digits were never meaningful. + uint32_t heartbeatUptimeSeconds; /// (major<<16)|(minor<<8)|patch of the image that is running. "The previous life died /// right after an OTA" is the single most valuable thing this record can show. uint32_t runningFirmware; @@ -86,7 +92,11 @@ struct BootRecord { uint32_t bootCount = 1; /// The previous life's last heartbeat: "it had been up this long when it died". /// Meaningless on a cold start; the payload reports it absent then. - uint32_t previousUptimeMs = 0; + /// + /// Still milliseconds, and still what /api/v1/diagnostics publishes as + /// previous_uptime_ms -- the storage changed, the interface did not. 64-bit because a + /// 32-bit millisecond count is the wrap this moved away from. + uint64_t previousUptimeMs = 0; /// The image the previous life was running, same encoding as Storage::runningFirmware. uint32_t previousFirmware = 0; }; @@ -98,6 +108,6 @@ BootRecord begin(Storage& storage, uint32_t runningFirmware); /// Heartbeat: remembers how far this life got. Throttled internally to one write per second /// of uptime -- calling it every loop() pass is fine and expected. -void tick(Storage& storage, uint32_t uptimeMs); +void tick(Storage& storage, uint64_t uptimeMs); } // namespace heliograph::breadcrumbs diff --git a/src/main.cpp b/src/main.cpp index f90206c..481d200 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1980,7 +1980,7 @@ void loop() { // Breadcrumb heartbeat: "this life reached this uptime". Throttled inside to one RTC-RAM // write per second. - breadcrumbs::tick(g_breadcrumbStore, static_cast(nowMs())); + breadcrumbs::tick(g_breadcrumbStore, nowMs()); g_wifi.loop(nowMs()); startOutputs(); // no-op until there is a network, and only ever runs once diff --git a/src/outputs/mqtt/home_assistant_discovery.cpp b/src/outputs/mqtt/home_assistant_discovery.cpp index bb55d22..75b65ff 100644 --- a/src/outputs/mqtt/home_assistant_discovery.cpp +++ b/src/outputs/mqtt/home_assistant_discovery.cpp @@ -8,6 +8,8 @@ #include #include "device/command.h" +#include "json_limits.h" +#include "mqtt_payloads.h" #include "relays/drm.h" namespace heliograph::mqtt { @@ -185,16 +187,16 @@ void addDeviceBlock(JsonObject entity, const BridgeInfo& bridge, const DeviceIde device["via_device"] = bridge.bridgeId; } +// Was a private copy of json_limits::finish() with the size check taken out: it caught an +// overflowed document but let a well-formed one grow to whatever the heap allowed. Every other +// payload on this device is bounded, and discovery payloads are the LARGEST ones -- each +// carries the full device block -- so this was the one path without the guard, on the output +// that publishes the most bytes. +// +// Now the shared one, at the same ceiling the state payloads use. One point of truth: a change +// to how a payload is bounded should not need finding in two places. bool serialise(const JsonDocument& doc, std::string& out) { - if (doc.overflowed()) { - return false; - } - const size_t needed = measureJson(doc); - std::string buffer; - buffer.resize(needed + 1); - buffer.resize(serializeJson(doc, buffer.data(), buffer.size())); - out = std::move(buffer); - return true; + return json_limits::finish(doc, out, kMaxPayloadBytes); } } // namespace diff --git a/src/outputs/mqtt/publish_policy.cpp b/src/outputs/mqtt/publish_policy.cpp index 7abcd6e..4403630 100644 --- a/src/outputs/mqtt/publish_policy.cpp +++ b/src/outputs/mqtt/publish_policy.cpp @@ -4,6 +4,7 @@ #include #include +#include namespace heliograph::mqtt { @@ -45,7 +46,7 @@ bool PublishThrottle::shouldPublish(const DeviceState& state, uint64_t nowMs) { continue; } const auto it = std::find_if(lastPublished_.begin(), lastPublished_.end(), - [&m](const Sample& s) { return s.id == m.id; }); + [&m](const Sample& s) { return std::strcmp(s.id, m.id) == 0; }); const Sample* previous = it == lastPublished_.end() ? nullptr : &*it; if (previous == nullptr) { return true; // a channel appeared, e.g. a second MPPT was detected diff --git a/src/outputs/mqtt/publish_policy.h b/src/outputs/mqtt/publish_policy.h index e8c3f8a..fb309a3 100644 --- a/src/outputs/mqtt/publish_policy.h +++ b/src/outputs/mqtt/publish_policy.h @@ -50,7 +50,19 @@ class PublishThrottle { double deadbandFor(MeasurementType type) const; struct Sample { - std::string id; + /// Borrowed, not owned -- and safe to borrow: an id is either one of the + /// `inline constexpr const char*` constants in measurement.h or, in a few tests, a + /// bare string literal. Both have static storage duration, so both outlive every + /// throttle that points at them. What would NOT be safe is an id built at runtime, + /// which nothing does and measurement.h's own contract forbids. + /// + /// This was a std::string, which rebuilt the whole vector on every publish: one + /// allocation per channel, freed again on the next, at least once a minute per device + /// for the life of the bridge. measurement.h keeps ids as const char* for exactly that + /// reason -- it records 80-120 short-lived allocations per poll as "a classic + /// fragmentation hazard" on a device with no defragmenting allocator -- and this was + /// the one place downstream that put them back on the heap. + const char* id; double value; bool valid; bool stale; diff --git a/test/test_mqtt/test_main.cpp b/test/test_mqtt/test_main.cpp index fb525e6..b1fa898 100644 --- a/test/test_mqtt/test_main.cpp +++ b/test/test_mqtt/test_main.cpp @@ -435,6 +435,47 @@ static void test_writable_driver_lists_its_bounds() { // --- Home Assistant discovery -------------------------------------------------------------- +// Discovery payloads are the largest this device publishes -- every entity repeats the whole +// device block -- and they were the one path that went out unbounded, through a private copy of +// json_limits::finish() with the size check removed. They share the state payloads' ceiling now. +// +// Asserted BOTH ways, because either alone is worthless: real payloads must still be published +// (a ceiling that refuses them breaks Home Assistant discovery outright and silently), and the +// ceiling must actually refuse something (or it is decoration). +static void test_every_discovery_payload_fits_the_ceiling_it_now_shares() { + Rig r; + const auto state = r.poll(); + const auto bridge = makeBridge(); + const MqttTopics topics(kDefaultBaseTopic, bridge.bridgeId); + const auto entities = buildDiscoveryEntities(state, bridge, topics, topics.availability(), + kDefaultDiscoveryPrefix, bridge.bridgeId); + + TEST_ASSERT_TRUE_MESSAGE(entities.size() > 3, "the fixture must produce real entities"); + size_t largest = 0; + for (const auto& e : entities) { + TEST_ASSERT_FALSE_MESSAGE(e.payload.empty(), "an entity was serialised to nothing"); + largest = std::max(largest, e.payload.size()); + } + // Room to spare, not "just fits": the device block grows when a field is added, and a + // ceiling this sits under by a hair would start dropping entities on the next one. + TEST_ASSERT_LESS_THAN_UINT32(kMaxPayloadBytes / 2, largest); + + // AND THE BOUND BITES, through the real builder rather than beside it. Identity strings + // are unbounded std::strings filled from whatever a device reports, and every entity + // repeats the device block -- so a device with an absurd model name is the reachable way + // over the ceiling. Asserting this through buildDiscoveryEntities is what makes the guard + // testable at all: removing an upper bound is strictly MORE permissive, so a test that + // only checks real payloads fit cannot detect its absence. + // + // Over the ceiling the entity is dropped rather than published oversized. That is the + // convention every other payload on this device already follows. + DeviceState huge = r.poll(); + huge.identity.model = std::string(kMaxPayloadBytes, 'M'); + const auto dropped = buildDiscoveryEntities(huge, bridge, topics, topics.availability(), + kDefaultDiscoveryPrefix, bridge.bridgeId); + TEST_ASSERT_EQUAL_UINT32(0, dropped.size()); +} + static void test_discovery_creates_an_entity_per_supported_measurement() { Rig r; const auto state = r.poll(); @@ -1626,6 +1667,7 @@ int main(int, char**) { RUN_TEST(test_capabilities_payload_reports_read_only); RUN_TEST(test_writable_driver_lists_its_bounds); RUN_TEST(test_discovery_creates_an_entity_per_supported_measurement); + RUN_TEST(test_every_discovery_payload_fits_the_ceiling_it_now_shares); RUN_TEST(test_discovery_metadata_matches_the_measurement_type); RUN_TEST(test_value_template_reads_the_right_key); RUN_TEST(test_availability_tracks_the_bridge_not_the_inverter); diff --git a/test/test_rest/test_main.cpp b/test/test_rest/test_main.cpp index 14a9768..09de128 100644 --- a/test/test_rest/test_main.cpp +++ b/test/test_rest/test_main.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -2091,14 +2092,15 @@ static void test_breadcrumbs_cold_then_warm() { TEST_ASSERT_EQUAL_UINT32(1, rec.bootCount); // This life runs 65 s; the heartbeat throttles to 1 Hz, so a second tick inside the - // same second must not move it. + // same second must not move it. Reported to the whole second, because that is the rate + // it was ever written at -- the millisecond digits were noise, never accuracy. breadcrumbs::tick(st, 65400); breadcrumbs::tick(st, 65900); auto rec2 = breadcrumbs::begin(st, 0x00001802); // the next boot runs 0.24.2 TEST_ASSERT_FALSE(rec2.coldStart); TEST_ASSERT_EQUAL_UINT32(2, rec2.bootCount); - TEST_ASSERT_EQUAL_UINT32(65400, rec2.previousUptimeMs); + TEST_ASSERT_EQUAL_UINT64(65000, rec2.previousUptimeMs); TEST_ASSERT_EQUAL_UINT32(0x00001801, rec2.previousFirmware); // And the record begin() left behind must itself survive a third boot: count keeps @@ -2106,10 +2108,83 @@ static void test_breadcrumbs_cold_then_warm() { auto rec3 = breadcrumbs::begin(st, 0x00001802); TEST_ASSERT_FALSE(rec3.coldStart); TEST_ASSERT_EQUAL_UINT32(3, rec3.bootCount); - TEST_ASSERT_EQUAL_UINT32(0, rec3.previousUptimeMs); // that life never ticked + TEST_ASSERT_EQUAL_UINT64(0, rec3.previousUptimeMs); // that life never ticked TEST_ASSERT_EQUAL_UINT32(0x00001802, rec3.previousFirmware); } +/// The record exists to describe a bridge that had been up a long time, and as milliseconds in +/// a uint32 it wrapped at 49.7 days -- so the lives it was built for were exactly the ones it +/// misreported, and it misreported them as a plausible smaller number rather than as an error. +static void test_a_life_past_the_32_bit_millisecond_wrap_is_recorded_whole() { + breadcrumbs::Storage st; + std::memset(&st, 0, sizeof st); + breadcrumbs::begin(st, 1); + + constexpr uint64_t kSixtyDaysMs = 60ULL * 86400ULL * 1000ULL; // 5184000000, over 2^32 + breadcrumbs::tick(st, kSixtyDaysMs); + + const auto rec = breadcrumbs::begin(st, 1); + TEST_ASSERT_FALSE(rec.coldStart); + TEST_ASSERT_EQUAL_UINT64(kSixtyDaysMs, rec.previousUptimeMs); +} + +/// "How far this life got" can only go up. The millisecond comparison this replaced was +/// implicitly monotonic -- it could never store a value smaller than the one already there -- +/// and rewriting it as an equality check quietly gave that up. esp_timer_get_time() does not +/// run backwards, so nothing in the firmware reaches this; tick() takes the clock as an +/// argument, so the property can be asserted rather than merely asserted ABOUT. +static void test_the_heartbeat_never_moves_backwards() { + breadcrumbs::Storage st; + std::memset(&st, 0, sizeof st); + breadcrumbs::begin(st, 1); + + breadcrumbs::tick(st, 9000); + breadcrumbs::tick(st, 3000); // a clock that went back must not rewrite the high-water mark + + const auto rec = breadcrumbs::begin(st, 1); + TEST_ASSERT_FALSE(rec.coldStart); + TEST_ASSERT_EQUAL_UINT64(9000, rec.previousUptimeMs); +} + +/// The checksum firmware BEFORE the seconds change would have written: the same twelve bytes, +/// with no schema tag folded in. Spelled out rather than calling the current helper, because it +/// is the OTHER system's checksum -- if ours changes again, this must NOT follow it. +static uint32_t legacyStorageCrc(const breadcrumbs::Storage& st) { + const auto* p = reinterpret_cast(&st); + uint32_t crc = 0xFFFFFFFFu; + for (size_t i = 0; i < offsetof(breadcrumbs::Storage, crc); ++i) { + crc ^= p[i]; + for (int b = 0; b < 8; ++b) { + crc = (crc >> 1) ^ (0xEDB88320u & (0u - (crc & 1u))); + } + } + return crc ^ 0xFFFFFFFFu; +} + +/// Renaming heartbeatUptimeMs to heartbeatUptimeSeconds changed a field's UNIT without changing +/// the layout, so a record left in RTC RAM by the previous firmware still checksummed correctly +/// and was read back as warm -- multiplied by a thousand. RTC_NOINIT survives exactly the reset +/// an OTA performs, so this would have happened once on every device in the fleet, and three +/// days of uptime would have been published as three thousand. +/// +/// Folding the schema tag into the CRC makes such a record fail validation instead. One cold +/// start is the price, and it is the honest answer: those bytes cannot be interpreted. +static void test_a_record_written_under_the_millisecond_schema_reads_as_cold() { + breadcrumbs::Storage st; + std::memset(&st, 0, sizeof st); + st.bootCount = 7; + st.runningFirmware = 0x00001801; + st.heartbeatUptimeSeconds = 259200000; // three days, written by firmware that meant ms + st.crc = legacyStorageCrc(st); + + const auto rec = breadcrumbs::begin(st, 0x00001802); + + TEST_ASSERT_TRUE_MESSAGE(rec.coldStart, + "a record whose fields mean something else is not a past"); + TEST_ASSERT_EQUAL_UINT64(0, rec.previousUptimeMs); // not 259200000000 + TEST_ASSERT_EQUAL_UINT32(1, rec.bootCount); +} + /// One flipped bit must read as cold: a torn RTC write may never become an invented past. /// This is the mutation-facing test -- remove the CRC comparison and it fails. static void test_breadcrumbs_corruption_reads_as_cold() { @@ -2117,7 +2192,7 @@ static void test_breadcrumbs_corruption_reads_as_cold() { std::memset(&st, 0, sizeof st); breadcrumbs::begin(st, 1); breadcrumbs::tick(st, 5000); - st.heartbeatUptimeMs ^= 0x4; // torn write, CRC now stale + st.heartbeatUptimeSeconds ^= 0x4; // torn write, CRC now stale auto rec = breadcrumbs::begin(st, 1); TEST_ASSERT_TRUE(rec.coldStart); @@ -2146,13 +2221,21 @@ static void test_breadcrumbs_payload_shapes() { bridge.bootCount = 4; bridge.breadcrumbsCold = false; - bridge.previousUptimeMs = 65400; + bridge.previousUptimeMs = 65000; bridge.previousFirmware = 0x00001801; TEST_ASSERT_TRUE(rest::buildDiagnosticsPayload(d.snapshot(), bridge, json)); doc = parse(json); - TEST_ASSERT_EQUAL_UINT32(65400, doc["previous_uptime_ms"].as()); + TEST_ASSERT_EQUAL_UINT64(65000, doc["previous_uptime_ms"].as()); TEST_ASSERT_EQUAL_STRING("0.24.1", doc["previous_firmware"]); + // And the payload must carry a value past 2^32 without narrowing it on the way out. The + // field is milliseconds and the storage behind it now reaches 136 years, so the JSON is + // the last place left that could still wrap it at 49.7 days. + bridge.previousUptimeMs = 60ULL * 86400ULL * 1000ULL; // 5184000000 + TEST_ASSERT_TRUE(rest::buildDiagnosticsPayload(d.snapshot(), bridge, json)); + TEST_ASSERT_EQUAL_UINT64(5184000000ULL, parse(json)["previous_uptime_ms"].as()); + bridge.previousUptimeMs = 65000; + // A version whose three components are all DIFFERENT and all nonzero. Every firmware this // project has shipped is 0.x, so every stored breadcrumb has a zero major -- which meant the // assertion above held just as well if the major were decoded from the wrong byte. Proved: @@ -3002,6 +3085,9 @@ int main(int, char**) { RUN_TEST(test_poll_duration_payload_absent_until_sampled); RUN_TEST(test_breadcrumbs_cold_then_warm); RUN_TEST(test_breadcrumbs_corruption_reads_as_cold); + RUN_TEST(test_a_life_past_the_32_bit_millisecond_wrap_is_recorded_whole); + RUN_TEST(test_a_record_written_under_the_millisecond_schema_reads_as_cold); + RUN_TEST(test_the_heartbeat_never_moves_backwards); RUN_TEST(test_breadcrumbs_payload_shapes); RUN_TEST(test_diagnostics_payload_has_no_secrets); RUN_TEST(test_diagnostics_report_stack_marks_and_fragmentation); diff --git a/tools/check_version.sh b/tools/check_version.sh index c9e3602..bfd784a 100755 --- a/tools/check_version.sh +++ b/tools/check_version.sh @@ -41,9 +41,60 @@ declared="$(read_define HELIOGRAPH_VERSION_MAJOR).$(read_define HELIOGRAPH_VERSI # tried to read "main" as a version would fail every pull request for a reason that has nothing # to do with the change. Being wired into the wrong workflow should do nothing, not break it. tag="${1:-}" + +if [ "$tag" = "--self-test" ]; then + self_fail=0 + # Driven through the ENVIRONMENT and with no argument, because that is how release.yml + # invokes this (`run: bash tools/check_version.sh`). An earlier version of this self-test + # passed each tag as an argument instead -- and every case still passed with the bug + # reintroduced, because the argument path was never the broken one. The bug was that a ref + # the old guard did not recognise never reached the comparison at all. + expect() { # expect + local want="$1" rt="$2" rn="$3" got=0 + GITHUB_REF_TYPE="$rt" GITHUB_REF_NAME="$rn" "$0" >/dev/null 2>&1 || got=$? + if [ "$got" -ne "$want" ]; then + echo "check_version self-test: ${rt} '${rn}' exited ${got}, expected ${want}" >&2 + self_fail=1 + fi + } + expect 0 tag "v${declared}" + expect 0 tag "v${declared}-rc1" + # Every one of these used to exit 0 WITHOUT CHECKING ANYTHING: the old guard recognised a + # release tag as /^v[0-9]/, so these refs left $tag empty and the script printed the + # declared version and returned success -- while release.yml triggers on the far wider + # "v*" and would have built and published each of them. + expect 1 tag "vtest" + expect 1 tag "v-wip" + expect 1 tag "v" + expect 1 tag "v1.2" + expect 1 tag "v0.0.0" + # A branch must never be read as a version, whatever it is called. This is the false + # positive the old name-shape guard existed to avoid, and it still must not happen. + expect 0 branch "v-something" + expect 0 branch "main" + # And with no ref type at all -- not Actions, or a runner that stopped setting it -- a + # v-shaped name is still checked rather than waved through. + expect 1 "" "vtest" + expect 0 "" "" + if [ "$self_fail" -eq 0 ]; then + echo "check_version self-test: OK" + fi + exit "$self_fail" +fi + +# A release is identified by the REF TYPE, not by the shape of the name. GITHUB_REF_NAME holds +# a branch name in ordinary CI, and the old heuristic told the two apart by reading the text -- +# which let through every tag that was not v, unchecked, while release.yml triggers on +# "v*". A tag of vtest, v-wip or a bare v built and published a release with no version check +# at all, and release.yml names the release ${GITHUB_REF_NAME#v}, so vtest shipped as "test". if [ -z "$tag" ]; then - case "${GITHUB_REF_NAME:-}" in - v[0-9]*) tag="$GITHUB_REF_NAME" ;; + case "${GITHUB_REF_TYPE:-}" in + tag) tag="${GITHUB_REF_NAME:-}" ;; + # Unset means this is not a GitHub Actions run -- or a runner that stopped providing + # the variable. Fall back to the old name-shape heuristic rather than to NO CHECK: a + # branch wrongly read as a version is a loud failure on a pull request, and a release + # that skips its version check is a silent one. Given the choice, be loud. + "") case "${GITHUB_REF_NAME:-}" in v*) tag="$GITHUB_REF_NAME" ;; esac ;; esac fi if [ -z "$tag" ]; then @@ -51,6 +102,20 @@ if [ -z "$tag" ]; then exit 0 fi +# The tag must BE a version, not merely begin with a v. +if ! printf '%s' "$tag" | grep -qE '^v[0-9]+\.[0-9]+\.[0-9]+(-[A-Za-z0-9.]+)?$'; then + cat >&2 <