fix: repair merge artifacts that left master uncompilable

[KhajaRaheelAhmedMohiuddin]

fix: repair merge artifacts that left master uncompilable

Summary

The current tip of master (22d8f8b, merged 2026‑09‑03) does not build.
The documented native build from the README —

cmake -B build -DCMAKE_BUILD_TYPE=Release
cmake --build build --parallel

— fails on any C11 compiler, before any board‑specific code is reached. Three
independent duplicate/stale definitions landed together in the "restore host
build" merge. This PR repairs all three and restores a clean, warning‑free
build and a green test suite.

Root cause

Each defect is a merge artifact — a change that renamed/replaced a symbol on one
branch while another branch (or the same PR) still referenced the old form:

1. include/eos_image.h — static_assert on a removed struct member

The field‑pinning assertions added in #87 still reference a reserved member,
but the TLV‑authentication change (#93) had already replaced reserved[30] with
tlv_len (2 bytes) + tlv_hash[28]. Two assertions name a member that no longer
exists:

error: 'eos_image_header_t' has no member named 'reserved'

Because the header is included throughout eboot_core, every translation
unit that pulls it in fails (slot_manager.c, image_verify.c,
fw_transport_uart.c, …).

Fix: the offsets of tlv_len (62) and tlv_hash (64) are already pinned in
the first assertion block, so the redundant reserved‑offset assert is removed;
the field‑width block now pins sizeof(tlv_len) == 2 and
sizeof(tlv_hash) == 28 — exactly the 30 bytes the old reserved[30] occupied —
so the on‑wire layout stays fully asserted.

2. core/ed25519_verify.c — duplicate + dead function

point_is_identity() is defined twice (error: redefinition of 'point_is_identity'), and key_has_prime_order() is a never‑called duplicate of
the function actually used by the verifier, public_key_is_valid_subgroup().

Fix: remove the duplicate definition and the dead function. The richer
subgroup‑rejection rationale (and its #57 attribution) is preserved by moving
it onto the surviving public_key_is_valid_subgroup(). No verification logic
changes — the surviving path is byte‑for‑byte the one already in use.

3. tests/unit/test_ed25519.c — duplicated test, orphaned data, wrong count

The merge left this test file broken in four ways:

• test_ed25519_identity_key_forgery_rejected is defined twice (redefinition).
• The file‑scope messages[] and k_low_order[8][32] arrays that
  test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery depends on were
  dropped (error: 'k_low_order' undeclared, 'messages' undeclared).
• That low‑order‑R test was never wired into main().
• The SHA‑512 section banner was misplaced above an Ed25519 test, and
  tests_run was hardcoded to 11 against 13 run_*() calls — so the
  binary would report failure (13/11) even once it compiled.

Fix: restore the two arrays, de‑duplicate the test, wire the low‑order‑R test
into main(), move the banner back above the SHA‑512 tests, and set
tests_run = 13.

Verification

All commands run on a clean checkout with GCC 13 (C11):

Build	Result
cmake -B build -DCMAKE_BUILD_TYPE=Release (README basic)	✅ builds, 0 errors
cmake -B build -DEBLDR_BUILD_TESTS=ON	✅ builds, 0 warnings
cmake -B build -DEBLDR_BUILD_FUZZ=ON -DEBLDR_BUILD_TESTS=ON	✅ builds, 0 errors

$ ctest --test-dir build -E valgrind
100% tests passed, 0 tests failed out of 21

$ ./build/tests/eboot_test_ed25519
13/13 tests passed

valgrind_test_image_verify and valgrind_test_tlv_auth pass under Valgrind
with no errors. (The valgrind_test_ed25519 variant is CPU‑bound under Valgrind
and exceeds the default ctest timeout; its logic is unchanged by this PR.)

Scope / notes

• No functional or on‑wire format change: the image header layout is identical,
  and the Ed25519 verifier keeps the exact code path it already executed.
• Pure repair of the broken merge plus restoration of the test coverage the
  merge silently dropped. Net: +58 / −59 lines across 3 files.

[srpatcha]

Review — eBoot#98 "fix: repair merge artifacts that left master uncompilable"

head: abc255b author: KhajaRaheelAhmedMohiuddin ci: none reported

Verdict: The diagnosis is right and I reproduced every number in the body. The problem is that this is the fourth open PR repairing the same two files, and it arrived last.

Reproduced from a gh api tarball snapshot at abc255b1, GCC on this host:

cmake -B build -DEBLDR_BUILD_TESTS=ON && cmake --build build   -> 0 errors
ctest --test-dir build -E valgrind --no-tests=error            -> 21/21 passed
./build/tests/eboot_test_ed25519                               -> 13/13 tests passed

And the premise, independently: git archive origin/master (22d8f8b) + cmake --build gives 12 errors, all include/eos_image.h:135 / :142 — 'eos_image_header_t' has no member named 'reserved'. core/ed25519_verify.c defines point_is_identity at both 281 and 338. Master is broken exactly as described.

I also checked the part that matters most, because this is TCB crypto and .ai/security.md says not to round it down: the change is behaviour-preserving. The two point_is_identity bodies on master are character-for-character identical, so keeping the first loses nothing. key_has_prime_order and public_key_is_valid_subgroup implement the same algorithm with only declaration style differing, key_has_prime_order had zero call sites, and the verifier calls public_key_is_valid_subgroup — core/ed25519_verify.c:496 on master, :467 at this head. The #57 attribution to @muhammadburhandevv-hub is carried onto the survivor. Nothing in the verification path moved.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	whole PR	Duplicate. #94 ("fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken", opened 2026-09-03T09:37Z) changes exactly the same three files — core/ed25519_verify.c, include/eos_image.h, tests/unit/test_ed25519.c — and fixes the same three defects. This PR opened 2026-09-03T17:41Z, eight hours later. #55 ("restore eboot_core buildability broken by unresolved merge conflicts") also touches core/ed25519_verify.c and include/eos_image.h and is already mergeStateStatus: DIRTY. #95 touches both again. Four PRs, one broken master; three of them will conflict on whichever lands first, and #84/#85 are stacked on #94 specifically so they can build.	Close this in favour of #94, or reduce it to the delta over #94 and say what the delta is. If it is kept instead of #94, that decision has to be made explicitly — #84 and #85 are stacked on #94 and will need restacking.
2	Medium	— (CI)	No CI evidence at all. gh pr checks 98 → "no checks reported on the 'fix/repair-uncompilable-master' branch"; statusCheckRollup has length 0, an hour after the PR opened, on a repo where the sibling PRs in this batch each report 24 checks. This looks like the first-time-contributor workflow-approval gate rather than anything wrong with the branch. A PR whose entire purpose is "restore the build" cannot be merged on its author's build log alone.	A maintainer needs to approve the workflow run so Build & Test, the cross-compiles and Host Build & Tests actually report. Until then the only independent evidence is a reviewer's local build.
3	Low	tests/unit/test_ed25519.c:399	tests_run = 13; is still a hand-maintained constant — the same construct that produced the 13/11 this PR is fixing. It fails closed (a mismatch exits 1), so this is maintenance rather than risk, but the next test added re-creates the report. #95 ("test: derive the suite totals and the Valgrind list instead of restating them") removes the class across every suite and adds tests/unit/test_suite_bookkeeping.py to keep it removed; the #94 line of work derives it inside the TEST macro with tests_run++. Either way this line conflicts with #95.	Derive it: tests_run++ in the TEST macro, and drop the constant. Or take #95.
4	Low	include/eos_image.h:147	sizeof(...->tlv_hash) == EOS_IMG_TLV_HASH_LEN with the message "tlv_hash[] is 28 bytes on the wire" cannot fail, because the field is declared uint8_t tlv_hash[EOS_IMG_TLV_HASH_LEN] — the assert restates the declaration. The file's own argument three lines up is that a wire format needs every field pinned against the wire, not against itself; the literal 28 (which #94 uses here) is what does that. The layout is still caught indirectly by offsetof(signature) == 92, so this is a weakened assert, not an absent check.	sizeof(((eos_image_header_t *)0)->tlv_hash) == 28. Same for tlv_len == 2, which already uses the literal.
5	Low	PR body, "Verification" table	"cmake -B build -DEBLDR_BUILD_TESTS=ON ✅ builds, 0 warnings" — the build emits one: core/keystore.c:29: warning: #warning "eBoot: building with the RFC 8032 test-vector public key as the secure-boot trust anchor; define EBLDR_PRODUCTION_KEY for any real device" [-Wcpp]. It is deliberate, pre-existing and not this PR's doing, but "0 warnings" is not what the build prints, and this is a PR arguing from its build output.	Say "one pre-existing #warning from core/keystore.c:29, no new diagnostics".

The include/eos_image.h coverage question — does dropping the two reserved asserts lose wire-format coverage? No. offsetof(tlv_len) == 62, offsetof(tlv_hash) == 64, offsetof(signature) == 92 and offsetof(tlv_hash) + EOS_IMG_TLV_HASH_LEN == offsetof(signature) are all still asserted in the first block, so bytes 62–91 stay pinned by offset, and finding 4 is the only softening.

Architecture conformance

Conforms. Tier 1 Foundation (§21), no dependency direction touched (§5.1) — the diff is three files inside eBoot with no new include, link or manifest entry. §8's "eBoot keeps the trusted computing base minimal and auditable" is served by deleting a dead duplicate of a subgroup check rather than leaving two copies where a future edit lands on the unused one. No .docx-level gap: master shipping uncompilable is a process failure (§28 evidence policy, §23 release model), not a design gap, and there is no design text that permits it.

Proposed changes

1. Decide between #94 and #98 before either lands; #84 and #85 are stacked on #94, so the cheaper decision is #94 plus a delta from #98 if #98 has one.
2. Approve the workflow run so this branch reports checks.
3. tests_run derived, not restated (or defer to #95).
4. sizeof(...->tlv_hash) == 28 with the literal.
5. Correct the "0 warnings" line in the body.

Not checked

• Valgrind: NOT RUN. find_program(VALGRIND) finds nothing on this host, so ctest -N lists 21 tests with no valgrind_* targets and -E valgrind was a no-op. The body's claims about valgrind_test_image_verify and valgrind_test_tlv_auth are unverified here.
• -DEBLDR_BUILD_FUZZ=ON configure/build: NOT RUN on this head. On the #84 head the same tree compiles all six harnesses but links none — this host has no libFuzzer/ASan runtime (libclang_rt.fuzzer.a absent) — so the body's "✅ builds, 0 errors" for the fuzz configure is not reproducible here either way.
• Cross builds: NOT RUN. No arm-none-eabi run locally, and no CI to fall back on for this PR (finding 2). The #warning in core/keystore.c is the only signal I have that the trust-anchor path is unchanged.
• -DEBLDR_SANITIZE=ON: NOT RUN on this head.
• I did not diff #94 against #98 line by line to establish whether #98 contains anything #94 lacks; finding 1 is based on the file sets, the three defects both describe, and the fact that both reduce to the same repair. That comparison is the maintainer's decision input and I have not made it.
• The local clone does not have abc255b1; all of the above was read from a gh api tarball snapshot at that sha.

---

Automated architecture review of abc255b1d2b7 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[KhajaRaheelAhmedMohiuddin]

Thanks @srpatcha - genuinely thorough review, and I appreciate you reproducing the numbers and specifically checking that the crypto change is behaviour-preserving.

You're right on the duplication. I ran the line-by-line #94 ↔ #98 comparison you left open, so it's not left for the maintainers:

• fix: repair merge artifacts that left master uncompilable #98 has no delta over fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken #94. The eos_image.h and ed25519_verify.c changes are equivalent, and on the two points you flagged fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken #94 is the stronger version: it uses the literal sizeof(tlv_hash) == 28 (finding 4) where I used EOS_IMG_TLV_HASH_LEN, and it derives the suite total with tests_run++ in the TEST macro (finding 3) where I left the hand-maintained tests_run = 13.
• fix: repair master — the ABI asserts and the Ed25519 verifier both merged broken #94's test file is also strictly more thorough, it expands k_low_order to the full eight canonical encodings, adds the non-canonical cases as a separate test, and runs the full key×R×message sweep. Mine just restored the pre-merge arrays.

So there's nothing here worth carrying over as a delta. #94 opened ~8h earlier and has #84/#85 stacked on it, merging #94 is clearly the right path, so I'm closing this in its favour.

On finding 5: you're correct, the build prints one pre-existing #warning from core/keystore.c:29 (the test trust-anchor notice). "0 warnings" in my description should have read "one pre-existing #warning, no new diagnostics" - my error.

Thanks again for the careful look.