rob/CampervanControlSystems

Watch

Fork 0

Fetch a per-node-type manifest instead of the shared file #64

Merged

Claude merged 2 commits from feat/per-type-manifest into main

2026-09-21 06:46:21 +00:00

Claude commented

2026-09-21 06:37:51 +00:00

Collaborator

CCS-UF-20. A node fetched /firmware/manifest.json, which held every node type's entry, and read only its own — 13 updatable node types already breach the 2048-byte cap. Fetches /firmware/manifest/0x<type>.json instead, holding just that node's entry with no images wrapper and no key.

This is mostly a deletion: firmware_manifest.cpp loses the key-matching walk (the source of CCS-UF-5's out-of-bounds read and CCS-UF-19's version-overflow findings), and manifest_ shrinks from 2048 to 512 bytes. Net -37 lines.

A 404 is now distinguished from any other transport failure (transportNotFound vs transportFailed in update_transport.h), so a node with nothing published fails at once instead of burning three retries a second apart. Spec section 11.1 updated to match.

No fallback to the old shared path — CCS-UHA-24 keeps Home Assistant serving both, so this lands independently.

Tested: pio test -e native (434 cases), pio run (both nodes) and pio run -e host_sim all pass. Vectors regenerated and diffed clean — no frame bytes changed.

CCS-UF-20. A node fetched `/firmware/manifest.json`, which held every node type's entry, and read only its own — 13 updatable node types already breach the 2048-byte cap. Fetches `/firmware/manifest/0x<type>.json` instead, holding just that node's entry with no `images` wrapper and no key. This is mostly a deletion: `firmware_manifest.cpp` loses the key-matching walk (the source of CCS-UF-5's out-of-bounds read and CCS-UF-19's version-overflow findings), and `manifest_` shrinks from 2048 to 512 bytes. Net -37 lines. A 404 is now distinguished from any other transport failure (`transportNotFound` vs `transportFailed` in `update_transport.h`), so a node with nothing published fails at once instead of burning three retries a second apart. Spec section 11.1 updated to match. No fallback to the old shared path — CCS-UHA-24 keeps Home Assistant serving both, so this lands independently. Tested: `pio test -e native` (434 cases), `pio run` (both nodes) and `pio run -e host_sim` all pass. Vectors regenerated and diffed clean — no frame bytes changed.

Claude added 1 commit

2026-09-21 06:37:51 +00:00

Fetch a per-node-type manifest instead of the shared file

Build and test / build (pull_request) Successful in 55s

Details

Build and test / release (pull_request) Has been skipped

Details

7d4e3f671c

Drops the images wrapper and key-matching walk from the manifest
parser, shrinking the buffer and removing the code CCS-UF-5 and
CCS-UF-19 flagged as fiddliest. A 404 is now distinguished from a
transport failure so a node with nothing published fails at once
instead of burning three retries.

Claude reviewed

2026-09-21 06:41:16 +00:00

Claude left a comment

Reviewed the diff, ran pio test -e native (full suite, 434 cases, ASan/UBSan) and clang-format --dry-run on the touched files — all clean. Parser simplification (descendTo/writeKey removal, matchString/nextMember losing foldCase) checks out against the old CCS-UF-5/CCS-UF-19 bug sites; no case-folding path remains anywhere it matters. maxManifestPathLength = 28 and maxManifestLength = 512 are both correctly sized (28 chars + NUL fits the +1 buffer exactly for a %02x-formatted byte; 512 comfortably covers a worst-case single entry, ~194 bytes). No dead code or half-migrated constants left behind.

One thing worth a look, nothing else blocking:

lib/node-runtime/src/firmware_updater.cpp, open() — the transportNotFound branch is shared by both the manifest fetch and the image fetch (fetchingManifest ? "no firmware published for this node type" : "image missing from the server"). That means a 404 while fetching the image .bin is now also terminal (skips the retry/attempt-limit path it used to go through), which is a real behaviour change beyond the ticket's manifest-path swap. It looks like the right call, but there's no test exercising that branch (both new tests, test_a_404_for_the_manifest_fails_at_once_without_retrying and the test_update_failed_state equivalent, only set notFound during the manifest fetch), and docs/can-protocol.md §11.1 only documents the 404 case for the manifest, not the image. Suggest either a small test for the image-404 case, or a one-line spec mention, so this isn't silently relying on the shared code path.

Nit: char manifestPath[maxManifestPathLength + 1]; in open() is left uninitialised, unlike every other stack char buffer in this file/library which use = {}. It's not a bug today (only read on the branch that writes it via snprintf), but it's inconsistent with the surrounding style and a future edit that reads it unconditionally (e.g. for logging) would pick up garbage.

Everything else — the manifest parser's edge cases (escapes, unknown-member skip, truncation, nested skip), the transport 404-vs-failure plumbing, the snprintf/static_cast<unsigned> path formatting, the spec wording, and the rewritten tests (heap exact-length buffers preserved for the ASan redzone, no lost coverage) — checks out.

Reviewed the diff, ran `pio test -e native` (full suite, 434 cases, ASan/UBSan) and `clang-format --dry-run` on the touched files — all clean. Parser simplification (`descendTo`/`writeKey` removal, `matchString`/`nextMember` losing `foldCase`) checks out against the old CCS-UF-5/CCS-UF-19 bug sites; no case-folding path remains anywhere it matters. `maxManifestPathLength = 28` and `maxManifestLength = 512` are both correctly sized (28 chars + NUL fits the `+1` buffer exactly for a `%02x`-formatted byte; 512 comfortably covers a worst-case single entry, ~194 bytes). No dead code or half-migrated constants left behind. One thing worth a look, nothing else blocking: **`lib/node-runtime/src/firmware_updater.cpp`, `open()`** — the `transportNotFound` branch is shared by both the manifest fetch and the image fetch (`fetchingManifest ? "no firmware published for this node type" : "image missing from the server"`). That means a 404 while fetching the *image* .bin is now also terminal (skips the retry/attempt-limit path it used to go through), which is a real behaviour change beyond the ticket's manifest-path swap. It looks like the right call, but there's no test exercising that branch (both new tests, `test_a_404_for_the_manifest_fails_at_once_without_retrying` and the `test_update_failed_state` equivalent, only set `notFound` during the manifest fetch), and `docs/can-protocol.md` §11.1 only documents the 404 case for the manifest, not the image. Suggest either a small test for the image-404 case, or a one-line spec mention, so this isn't silently relying on the shared code path. Nit: `char manifestPath[maxManifestPathLength + 1];` in `open()` is left uninitialised, unlike every other stack char buffer in this file/library which use `= {}`. It's not a bug today (only read on the branch that writes it via `snprintf`), but it's inconsistent with the surrounding style and a future edit that reads it unconditionally (e.g. for logging) would pick up garbage. Everything else — the manifest parser's edge cases (escapes, unknown-member skip, truncation, nested skip), the transport 404-vs-failure plumbing, the `snprintf`/`static_cast<unsigned>` path formatting, the spec wording, and the rewritten tests (heap exact-length buffers preserved for the ASan redzone, no lost coverage) — checks out.

rob added 1 commit

2026-09-21 06:43:12 +00:00

Cover the image-fetch 404 path and tidy the review's nits

Build and test / build (pull_request) Successful in 55s

Details

Build and test / release (pull_request) Has been skipped

Details

8eb162ee2e

Zero-init the manifest path buffer and spell out in the spec that a
404 is terminal for either fetch, not just the manifest.

Claude reviewed

2026-09-21 06:45:06 +00:00

Claude left a comment

Follow-up on 8eb162e — both notes from the previous review are closed out.

manifestPath is now zero-initialised (= {}), matching the file's convention.
The new test test_a_404_for_the_image_fails_at_once_without_retrying genuinely exercises the image-fetch path: it runs two loop() calls to get through the manifest fetch and parse (stage becomes Image), then sets notFound and calls loop() a third time. beginCalls == 2 and sink.beginCalls == 0 confirm the 404 hits on the image request before the slot is ever opened, not a repeat of the manifest case.
Spec wording ("A 404 on either fetch is terminal...") is accurate and stays minimal, no added rationale.

pio test -e native passes all 435 cases (up from 434). pio run (both node envs) and pio run -e host_sim build clean. No new issues found. (Can't set an approving state — Forgejo blocks self-approval on your own PR — posting as a comment instead.)

Follow-up on 8eb162e — both notes from the previous review are closed out. - `manifestPath` is now zero-initialised (`= {}`), matching the file's convention. - The new test `test_a_404_for_the_image_fails_at_once_without_retrying` genuinely exercises the image-fetch path: it runs two `loop()` calls to get through the manifest fetch and parse (stage becomes `Image`), *then* sets `notFound` and calls `loop()` a third time. `beginCalls == 2` and `sink.beginCalls == 0` confirm the 404 hits on the image request before the slot is ever opened, not a repeat of the manifest case. - Spec wording ("A 404 on either fetch is terminal...") is accurate and stays minimal, no added rationale. `pio test -e native` passes all 435 cases (up from 434). `pio run` (both node envs) and `pio run -e host_sim` build clean. No new issues found. (Can't set an approving state — Forgejo blocks self-approval on your own PR — posting as a comment instead.)

Claude merged commit 7e95529552 into main

2026-09-21 06:46:21 +00:00

Claude deleted branch feat/per-type-manifest

2026-09-21 06:46:21 +00:00

Claude referenced this pull request from a commit

2026-09-21 06:46:21 +00:00

Merge pull request 'Fetch a per-node-type manifest instead of the shared file' (#64) from feat/per-type-manifest into main