fix: run the signing suite in CI and keep the boot log head consistent on write failure

.github/workflows/ci.yml

@@ -36,7 +36,14 @@ jobs:
             cmake ninja-build gcc-12 g++-12 \
             gcc-arm-none-eabi binutils-arm-none-eabi \
             lcov gcovr python3-pip
-          pip3 install pytest pytest-cov
+          # Install from requirements.txt rather than a hand-maintained list.
+          # tests/unit/test_sign_image.py guards itself with
+          # pytest.importorskip("cryptography"), so a dependency that is
+          # declared nowhere does not fail the job -- it skips the 14 cases
+          # that pin the signed header format and the run still goes green.
+          # pytest-cov stays separate: it is a CI-only coverage plugin, not a
+          # dependency of anything in the repository.
+          pip3 install -r requirements.txt pytest-cov
 
       - name: Configure (host)
         run: |

core/boot_log.c

@@ -41,7 +41,15 @@ void eos_boot_log_append(uint32_t event, uint32_t slot, uint32_t detail)
     uint32_t offset = log_head * sizeof(eos_boot_log_entry_t);
     uint32_t addr   = ops->log_addr + offset;
 
-    eos_hal_flash_write(addr, &entry, sizeof(entry));
+    /* Advance the head only once the entry is actually on flash. Advancing
+     * unconditionally skips a slot that was never written, and the head is
+     * persisted in the boot control block, so the gap survives the reset:
+     * eos_boot_log_read() then hands the reader erased flash as if it were an
+     * entry, and the slot is never reused. eos_boot_log_clear() guards its own
+     * state change against a failed erase for the same reason. */
+    int rc = eos_hal_flash_write(addr, &entry, sizeof(entry));
+    if (rc != EOS_OK)
+        return;
 
     log_head = (log_head + 1) % EOS_BOOT_LOG_MAX;
 }

core/boot_menu.c

@@ -12,13 +12,21 @@
 #include "eos_bootctl.h"
 #include <string.h>
 
+/* eos_hal_uart_write()/eos_hal_uart_read() are compatibility macros over the
+ * HAL's single-UART API (eos_hal_uart_send()/eos_hal_uart_recv()), so they
+ * expand without their port argument and it reads as unused under -Wextra.
+ * The parameter is kept because eos_boot_menu_config_t carries a uart_port and
+ * the board ports address one; it starts being honoured the moment the HAL
+ * grows a per-port call. */
 static void uart_puts(uint8_t port, const char *str)
 {
+    (void)port;
     eos_hal_uart_write(port, (const uint8_t *)str, strlen(str));
 }
 
 static int uart_getc(uint8_t port, uint32_t timeout_ms)
 {
+    (void)port;
     uint8_t ch;
     int rc = eos_hal_uart_read(port, &ch, 1, timeout_ms);
     return (rc == EOS_OK) ? (int)ch : -1;

requirements.txt

@@ -1,3 +1,4 @@
 pyyaml>=6.0
 pytest>=9.0
 pyserial>=3.5
+cryptography>=41.0

tests/CMakeLists.txt

@@ -27,8 +27,12 @@ target_link_libraries(eboot_test_secure_boot PRIVATE eboot_core)
 add_test(NAME test_secure_boot COMMAND eboot_test_secure_boot)
 
 # --- test_recovery: UART recovery write range ---
+# eboot_stage1 links eboot_core PUBLIC, so naming eboot_core here too put it on
+# the link line twice -- ahead of the target that needs it, which is the wrong
+# order for a left-to-right archive scan, so CMake appended it again anyway and
+# the linker reported the duplicate. The transitive dependency covers it.
 add_executable(eboot_test_recovery unit/test_recovery.c)
-target_link_libraries(eboot_test_recovery PRIVATE eboot_core eboot_stage1)
+target_link_libraries(eboot_test_recovery PRIVATE eboot_stage1)
 add_test(NAME test_recovery COMMAND eboot_test_recovery)
 
 # --- test_fw_transport: UART raw/XMODEM/YMODEM firmware transport ---

tests/unit/test_boot_log.c

@@ -239,6 +239,43 @@ static void test_clear_reports_erase_failure(void)
     ASSERT(eos_boot_log_get_head() == 1);
 }
 
+/* The same reasoning on the write path. append() cannot report a failure --
+ * it returns void -- but it can decline to advance past a slot it never wrote.
+ * The head is persisted in the boot control block, so advancing anyway leaves
+ * a permanent hole: the slot is skipped for good and reads back as erased
+ * flash that a reader cannot tell apart from a real entry. */
+static void test_append_does_not_advance_head_when_write_fails(void)
+{
+    eos_boot_log_init(0);
+    eos_boot_log_append(EOS_LOG_BOOT_START, EOS_SLOT_A, 111);
+    ASSERT(eos_boot_log_get_head() == 1);
+
+    write_result = EOS_ERR_FLASH;
+    eos_boot_log_append(EOS_LOG_BOOT_FAIL, EOS_SLOT_A, 222);
+
+    /* Head stays put and slot 1 is still erased flash. */
+    ASSERT(eos_boot_log_get_head() == 1);
+
+    eos_boot_log_entry_t entry;
+    ASSERT(eos_boot_log_read(1, &entry) == EOS_OK);
+    ASSERT(entry.event == 0xFFFFFFFFu);
+
+    /* The next successful append reuses that slot instead of skipping it. */
+    write_result = EOS_OK;
+    eos_boot_log_append(EOS_LOG_CONFIRM, EOS_SLOT_B, 333);
+    ASSERT(eos_boot_log_get_head() == 2);
+
+    ASSERT(eos_boot_log_read(1, &entry) == EOS_OK);
+    ASSERT(entry.event == EOS_LOG_CONFIRM);
+    ASSERT(entry.slot == EOS_SLOT_B);
+    ASSERT(entry.detail == 333);
+
+    /* The entry written before the failure is untouched. */
+    ASSERT(eos_boot_log_read(0, &entry) == EOS_OK);
+    ASSERT(entry.event == EOS_LOG_BOOT_START);
+    ASSERT(entry.detail == 111);
+}
+
 static void test_entry_layout_is_stable(void)
 {
     /* The log is parsed by host tooling and by application firmware through
@@ -261,7 +298,8 @@ int main(void)
     RUN(test_read_rejects_bad_arguments);
     RUN(test_clear_erases_the_sector_and_resets_head);
     RUN(test_clear_reports_erase_failure);
+    RUN(test_append_does_not_advance_head_when_write_fails);
     RUN(test_entry_layout_is_stable);
-    printf("\n%d/10 tests passed\n", tests_passed);
+    printf("\n%d/11 tests passed\n", tests_passed);
     return 0;
 }

tests/unit/test_requirements.py

@@ -0,0 +1,152 @@
+"""Regression tests for the Python dependencies the test suite needs to run.
+
+tests/unit/test_sign_image.py guards itself with
+pytest.importorskip("cryptography"). That is the right behaviour for a
+contributor who has not installed the signing tools, but it also means an
+undeclared dependency fails nothing: the 14 cases that pin the signed .efw
+header wire format are skipped and the run still reports success.
+
+cryptography is imported by tools/sign_image.py and tools/eos_sign.py, but it
+was named neither in requirements.txt nor by the CI job that runs pytest --
+that job installed a hand-written list instead of the repository requirements
+-- so those 14 tests had never executed in CI.
+
+A hand-maintained dependency list beside requirements.txt is the thing that
+goes stale, so these check that the list is the requirements file, rather than
+checking any particular package name twice over.
+
+These parse requirements.txt and the workflow files as text, in the style of
+test_cmake_test_registration.py, so no cmake, compiler, network or GitHub API
+is needed.
+"""
+
+import re
+from pathlib import Path
+
+REPO_ROOT = Path(__file__).resolve().parents[2]
+REQUIREMENTS = REPO_ROOT / "requirements.txt"
+WORKFLOWS_DIR = REPO_ROOT / ".github" / "workflows"
+SIGN_IMAGE_SUITE = Path(__file__).resolve().parent / "test_sign_image.py"
+
+# A requirement line: the distribution name, up to any extras, version
+# specifier or environment marker.
+REQUIREMENT_NAME_RE = re.compile(r"^\s*([A-Za-z0-9][A-Za-z0-9._-]*)")
+
+# Job ids are the only keys at two-space indent inside a workflow's jobs: block.
+JOB_RE = re.compile(r"^  ([A-Za-z0-9_-]+):$", re.M)
+
+# `pytest tests/` or `python3 -m pytest tests/ -v ...`
+RUNS_PYTEST_RE = re.compile(r"\bpytest\b[^\n]*\btests/")
+
+# `pip install -r requirements.txt`, with or without the pip3 spelling.
+INSTALLS_REQUIREMENTS_RE = re.compile(r"\bpip3?\s+install\b[^\n]*-r\s+requirements\.txt")
+
+
+def _strip_comments(text):
+    """Drop whole-line YAML comments so prose about pytest is not mistaken
+    for a step that runs it."""
+    return "\n".join(
+        line for line in text.splitlines() if not line.lstrip().startswith("#")
+    )
+
+
+def _declared_requirements():
+    """Distribution names declared in requirements.txt, lowercased."""
+    names = set()
+    for line in REQUIREMENTS.read_text(encoding="utf-8").splitlines():
+        line = line.split("#", 1)[0].strip()
+        if not line or line.startswith("-"):
+            continue
+        match = REQUIREMENT_NAME_RE.match(line)
+        if match:
+            names.add(match.group(1).lower())
+    return names
+
+
+def _workflow_jobs():
+    """(workflow name, job id, job text) for every job in .github/workflows."""
+    jobs = []
+    for workflow in sorted(WORKFLOWS_DIR.glob("*.yml")):
+        text = _strip_comments(workflow.read_text(encoding="utf-8"))
+
+        start = text.find("\njobs:")
+        if start == -1:
+            continue
+        body = text[start:]
+
+        headers = list(JOB_RE.finditer(body))
+        for i, header in enumerate(headers):
+            end = headers[i + 1].start() if i + 1 < len(headers) else len(body)
+            jobs.append((workflow.name, header.group(1), body[header.start():end]))
+    return jobs
+
+
+def _importorskip_modules():
+    """Module names test_sign_image.py refuses to run without."""
+    text = SIGN_IMAGE_SUITE.read_text(encoding="utf-8")
+    return set(re.findall(r"importorskip\(\s*[\"']([A-Za-z0-9._-]+)[\"']", text))
+
+
+def test_cryptography_is_declared_in_requirements():
+    """Pin the specific dependency that was missing, by name."""
+    assert "cryptography" in _declared_requirements(), (
+        "cryptography is not declared in requirements.txt -- it is imported by "
+        "tools/sign_image.py and tools/eos_sign.py, and without it "
+        "tests/unit/test_sign_image.py skips all 14 of its cases instead of "
+        "running them"
+    )
+
+
+def test_every_importorskip_dependency_is_declared():
+    """A suite may only opt out of running for a dependency the repo declares.
+
+    Guards the general case: adding a new importorskip for some package that
+    requirements.txt never names would silently park that suite in the skipped
+    column, exactly as happened to the signing tests.
+    """
+    modules = _importorskip_modules()
+    assert modules, (
+        "expected test_sign_image.py to guard itself with "
+        "pytest.importorskip(...); if that guard is gone this test no longer "
+        "checks anything and should be updated"
+    )
+
+    declared = _declared_requirements()
+    undeclared = sorted(name for name in modules if name.lower() not in declared)
+
+    assert not undeclared, (
+        "test_sign_image.py skips itself when these modules are missing, but "
+        f"requirements.txt does not declare them, so they will never be "
+        f"installed and the suite will never run: {undeclared}"
+    )
+
+
+def test_jobs_that_run_pytest_install_the_repository_requirements():
+    """The environment the tests run in has to come from requirements.txt.
+
+    Installing a hand-written package list beside requirements.txt is what let
+    cryptography go missing: the list was complete enough for the suite to
+    collect and import, so nothing failed -- the signing cases just skipped.
+    """
+    pytest_jobs = [
+        (workflow, job, text)
+        for workflow, job, text in _workflow_jobs()
+        if RUNS_PYTEST_RE.search(text)
+    ]
+
+    assert pytest_jobs, (
+        "no workflow job appears to run pytest over tests/; if the Python "
+        "suite moved, this guard needs to move with it"
+    )
+
+    missing = [
+        f"{workflow}:{job}"
+        for workflow, job, text in pytest_jobs
+        if not INSTALLS_REQUIREMENTS_RE.search(text)
+    ]
+
+    assert not missing, (
+        "these jobs run pytest over tests/ but never install from "
+        "requirements.txt, so a declared dependency is absent at run time and "
+        f"the suites that need it skip while the job still passes: {missing}"
+    )