Add manifest-driven automatic firmware updates#47
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/AutoUpdateModule.h`:
- Line 7: Remove the direct platform/platform.h dependency from AutoUpdateModule
and replace its platform-specific usage with an injected platform-neutral
OTA/HTTP interface. Define or consume the interface in core, provide its
implementation within src/platform/, and wire the runtime layer to inject it
without adding platform includes under src/core/**.
- Around line 150-152: The OTA validation in AutoUpdateModule must reject c.size
== 0 before invoking the checked OTA API, while preserving the existing
oversized-image rejection. In test/unit/core/unit_AutoUpdateModule.cpp, add
manifest cases for an omitted size and an explicit zero size, asserting both are
rejected.
- Around line 183-185: Update AutoUpdateModule::validateUrl to accept only
https:// URLs, while preserving null or empty values as the disabled-checks
case. Also validate the baked-in default manifest URL before any fetch so all
configured and default manifest requests require HTTPS.
- Line 95: Update the OTA lifecycle guard around otaInFlight() to use a separate
atomic or mutex-protected in-flight flag rather than reading g_otaStatus. Set
and clear this flag across the OTA task lifecycle, while keeping g_otaStatus
limited to display/status reporting.
In `@test/unit/core/unit_AutoUpdateModule.cpp`:
- Around line 15-74: Add a test alongside the existing AutoUpdateModule
selection tests for an otherwise valid newer OTA manifest whose installable
object omits size. Assert that selectCandidate returns the appropriate rejection
decision and does not request installation, covering both the standard ota
fixture and the documented size-required behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d32e49ed-2be2-42dd-a3b1-8f945e148127
⛔ Files ignored due to path filters (3)
moondeck/build/build_esp32.pyis excluded by!**/build/**moondeck/build/generate_build_info.pyis excluded by!**/build/**moondeck/build/generate_manifest.pyis excluded by!**/build/**
📒 Files selected for processing (12)
docs/moonmodules/core/services.mdesp32/main/CMakeLists.txtsrc/core/AutoUpdateModule.hsrc/core/FirmwareUpdateModule.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/esp32/platform_esp32_ota.cppsrc/platform/platform.htest/CMakeLists.txttest/unit/core/unit_AutoUpdateModule.cpptest/unit/core/unit_FirmwareUpdateModule.cpp
| #include "core/FirmwareUpdateModule.h" | ||
| #include "core/JsonUtil.h" | ||
| #include "core/build_info.h" | ||
| #include "platform/platform.h" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the platform dependency from core.
AutoUpdateModule directly includes platform/platform.h, violating the core/platform boundary. Inject a platform-neutral OTA/HTTP interface from the runtime layer, with its implementation confined to src/platform/.
As per coding guidelines, "src/core/**: Core code ... must be platform-independent — no platform includes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/AutoUpdateModule.h` at line 7, Remove the direct platform/platform.h
dependency from AutoUpdateModule and replace its platform-specific usage with an
injected platform-neutral OTA/HTTP interface. Define or consume the interface in
core, provide its implementation within src/platform/, and wire the runtime
layer to inject it without adding platform includes under src/core/**.
Source: Coding guidelines
| return; | ||
| } | ||
| if (!autoInstall_) { clearStatus(); return; } | ||
| if (otaInFlight()) return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\bg_otaStatus\b|\botaInFlight\s*\(|http_fetch_to_ota_checked' \
src/core/FirmwareUpdateModule.h \
src/core/AutoUpdateModule.h \
src/platform/esp32/platform_esp32_ota.cppRepository: MoonModules/projectMM
Length of output: 7232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/core/FirmwareUpdateModule.h
printf '\n====\n'
sed -n '220,360p' src/core/AutoUpdateModule.h
printf '\n====\n'
sed -n '220,340p' src/platform/esp32/platform_esp32_ota.cppRepository: MoonModules/projectMM
Length of output: 23549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [
Path('src/core/FirmwareUpdateModule.h'),
Path('src/core/AutoUpdateModule.h'),
Path('src/platform/esp32/platform_esp32_ota.cpp'),
]:
print(f'### {p}')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'otaInFlight' in line or 'g_otaStatus' in line or 'http_fetch_to_ota' in line or 'start' in line.lower() or 'task' in line.lower():
print(f'{i}: {line}')
print()
PYRepository: MoonModules/projectMM
Length of output: 4619
Synchronize OTA in-flight gating. otaInFlight() reads the same mutable g_otaStatus buffer that the OTA task writes, so this guard can race and miss an active update. Move lifecycle gating to a separate atomic or mutex-protected flag; keep g_otaStatus display-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/AutoUpdateModule.h` at line 95, Update the OTA lifecycle guard
around otaInFlight() to use a separate atomic or mutex-protected in-flight flag
rather than reading g_otaStatus. Set and clear this flag across the OTA task
lifecycle, while keeping g_otaStatus limited to display/status reporting.
| const size_t part = platform::firmwarePartition(); | ||
| if (part > 0 && c.size > 0 && c.size > part) { | ||
| return fail(DecisionCode::TooLarge, "image too large"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make OTA image size mandatory and test it. A missing manifest size remains zero, and the platform contract interprets zero as “skip size enforcement.”
src/core/AutoUpdateModule.h#L150-L152: rejectc.size == 0before invoking the checked OTA API.test/unit/core/unit_AutoUpdateModule.cpp#L15-L74: add omitted- and zero-size manifest cases asserting rejection.
📍 Affects 2 files
src/core/AutoUpdateModule.h#L150-L152(this comment)test/unit/core/unit_AutoUpdateModule.cpp#L15-L74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/AutoUpdateModule.h` around lines 150 - 152, The OTA validation in
AutoUpdateModule must reject c.size == 0 before invoking the checked OTA API,
while preserving the existing oversized-image rejection. In
test/unit/core/unit_AutoUpdateModule.cpp, add manifest cases for an omitted size
and an explicit zero size, asserting both are rejected.
Source: Path instructions
| static bool validateUrl(const char* s) { | ||
| return !s || !*s || std::strncmp(s, "https://", 8) == 0 || std::strncmp(s, "http://", 7) == 0; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require HTTPS for manifest URLs.
Allowing http:// makes the manifest attacker-controlled in transit; its supplied SHA-256 then provides no authenticity guarantee for the selected firmware. Restrict configured URLs to HTTPS and validate the baked-in default before fetching it.
Proposed fix
static bool validateUrl(const char* s) {
- return !s || !*s || std::strncmp(s, "https://", 8) == 0 || std::strncmp(s, "http://", 7) == 0;
+ return !s || !*s || std::strncmp(s, "https://", 8) == 0;
}As per path instructions, "manifestUrl (HTTPS URL; empty disables checks)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static bool validateUrl(const char* s) { | |
| return !s || !*s || std::strncmp(s, "https://", 8) == 0 || std::strncmp(s, "http://", 7) == 0; | |
| } | |
| static bool validateUrl(const char* s) { | |
| return !s || !*s || std::strncmp(s, "https://", 8) == 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/AutoUpdateModule.h` around lines 183 - 185, Update
AutoUpdateModule::validateUrl to accept only https:// URLs, while preserving
null or empty values as the disabled-checks case. Also validate the baked-in
default manifest URL before any fetch so all configured and default manifest
requests require HTTPS.
Source: Path instructions
| TEST_CASE("AutoUpdateModule selects a newer ota object and resolves relative URL") { | ||
| char manifest[900]; | ||
| std::snprintf(manifest, sizeof(manifest), | ||
| "{\"version\":\"9.0.0\",\"ota\":{\"path\":\"firmware-esp32s3.bin\"," | ||
| "\"offset\":65536,\"chipFamily\":\"ESP32-S3\",\"size\":1234,\"sha256\":\"%s\"}}", kHash); | ||
|
|
||
| auto d = AutoUpdateModule::selectCandidate( | ||
| manifest, "https://updates.example.test/devices/manifest.json", | ||
| "1.0.0", "ESP32-S3"); | ||
| CHECK(d.shouldInstall()); | ||
| CHECK(std::strcmp(d.candidate.url, "https://updates.example.test/devices/firmware-esp32s3.bin") == 0); | ||
| CHECK(std::strcmp(d.candidate.version, "9.0.0") == 0); | ||
| } | ||
|
|
||
| // A manifest with the running version is recognised as already installed. | ||
| TEST_CASE("AutoUpdateModule refuses a same-version manifest") { | ||
| char manifest[900]; | ||
| std::snprintf(manifest, sizeof(manifest), | ||
| "{\"version\":\"1.0.0\",\"ota\":{\"path\":\"firmware.bin\"," | ||
| "\"offset\":65536,\"chipFamily\":\"ESP32-S3\",\"sha256\":\"%s\"}}", kHash); | ||
| auto d = AutoUpdateModule::selectCandidate(manifest, "https://x.test/manifest.json", | ||
| "1.0.0", "ESP32-S3"); | ||
| CHECK(d.code == AutoUpdateModule::DecisionCode::UpToDate); | ||
| CHECK_FALSE(d.shouldInstall()); | ||
| } | ||
|
|
||
| // A full flash bundle at offset 0 is rejected because OTA can only write the app partition. | ||
| TEST_CASE("AutoUpdateModule rejects full-bundle offset zero") { | ||
| char manifest[900]; | ||
| std::snprintf(manifest, sizeof(manifest), | ||
| "{\"version\":\"9.0.0\",\"ota\":{\"path\":\"merged.bin\"," | ||
| "\"offset\":0,\"chipFamily\":\"ESP32-S3\",\"sha256\":\"%s\"}}", kHash); | ||
| auto d = AutoUpdateModule::selectCandidate(manifest, "https://x.test/manifest.json", | ||
| "1.0.0", "ESP32-S3"); | ||
| CHECK(d.code == AutoUpdateModule::DecisionCode::BadOffset); | ||
| } | ||
|
|
||
| // ESP Web Tools manifests pick the app part for the matching chip family. | ||
| TEST_CASE("AutoUpdateModule selects app part from ESP Web Tools builds") { | ||
| char manifest[1200]; | ||
| std::snprintf(manifest, sizeof(manifest), | ||
| "{\"version\":\"2.0.0\",\"builds\":[{\"chipFamily\":\"ESP32\"," | ||
| "\"parts\":[{\"path\":\"wrong.bin\",\"offset\":65536,\"sha256\":\"%s\"}]}," | ||
| "{\"chipFamily\":\"ESP32-S3\",\"parts\":[" | ||
| "{\"path\":\"boot.bin\",\"offset\":0}," | ||
| "{\"path\":\"app.bin\",\"offset\":65536,\"size\":456,\"sha256\":\"%s\"}]}]}", | ||
| kHash, kHash); | ||
| auto d = AutoUpdateModule::selectCandidate(manifest, "https://host/fw/manifest.json", | ||
| "1.0.0", "ESP32-S3"); | ||
| CHECK(d.shouldInstall()); | ||
| CHECK(std::strcmp(d.candidate.url, "https://host/fw/app.bin") == 0); | ||
| CHECK(d.candidate.size == 456); | ||
| } | ||
|
|
||
| // Development and stable semantic versions compare in release order. | ||
| TEST_CASE("AutoUpdateModule compares dev versions monotonically") { | ||
| CHECK(AutoUpdateModule::versionIsNewer("2.1.0-dev.7", "2.1.0-dev.6")); | ||
| CHECK_FALSE(AutoUpdateModule::versionIsNewer("2.1.0-dev.6", "2.1.0-dev.7")); | ||
| CHECK(AutoUpdateModule::versionIsNewer("2.1.0", "2.1.0-dev.9")); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a missing-size rejection test.
All installable fixtures provide size, so the suite cannot catch a regression that passes zero to the OTA API and disables size enforcement.
As per path instructions, tests "should cover edge cases and match the specifications in docs/moonmodules/."
🧰 Tools
🪛 Clang (14.0.6)
[warning] 15-15: variable 'TEST_CASE' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
🪛 Cppcheck (2.21.0)
[style] 26-26: The function 'parseDottedQuad' is never used.
(unusedFunction)
[style] 49-49: The function 'formatDottedQuad' is never used.
(unusedFunction)
[style] 62-62: The function 'sanitizeHostname' is never used.
(unusedFunction)
[style] 27-27: The function 'parseString' is never used.
(unusedFunction)
[style] 22-22: The function 'roleName' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/core/unit_AutoUpdateModule.cpp` around lines 15 - 74, Add a test
alongside the existing AutoUpdateModule selection tests for an otherwise valid
newer OTA manifest whose installable object omits size. Assert that
selectCandidate returns the appropriate rejection decision and does not request
installation, covering both the standard ota fixture and the documented
size-required behavior.
Source: Path instructions
Moves shared OTA state into a core-neutral header, injects the auto-update runtime from main, gates all OTA triggers with a real atomic in-flight flag, and rejects HTTP or size-less manifest candidates. Checks: check_specs.py; git diff --check; build_esp32.py --firmware esp32s3-n16r8 (projectMM.bin 0x180dd0, total image 1576280 bytes).
1 participant
Summary
Validation
git diff --check origin/main..HEADmoondeck/check/check_specs.pymoondeck/check/check_platform_boundary.pymoondeck/build/build_esp32.py --firmware esp32s3-n16r8esp32s3-n16r8image:0x180c50, 62% app partition freeNot Run
Summary by CodeRabbit
New Features
Bug Fixes
Tests