Repository navigation
test: build four suites that existed but were never compiled, and guard it - #114
Conversation
|
Both PRs came from this account. #116 fixes the guard plus the Git merges them silently, because each adds its guard at a different point in the include block: Two opening guards, one closing. The default-configuration build fails exactly the way it does on master today, so merging both would put back the breakage both PRs exist to remove. Suggested resolution: take #116 for the Related: I closed #113 in favour of #116 for the same overlap. |
518eccf to
753cb10
Compare
|
Rebased onto #116. #113 is closed, and this branch carried its two commits; #116 repairs the same two things on master (the |
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#114 "test: build four suites that existed but were never compiled, and guard it"
head: 753cb10 author: Kartikey1306 ci: pass
Verdict: This is the right fix for a real and recurring failure mode, and it
verifies as claimed — 38/38 ctest, all five guard assertions pass, 43 suites fully
accounted for. One of the four newly registered suites, test_net_mock, links no
library and includes no EmbeddedOS header, so it adds a green ctest entry that cannot
detect any change to the code it is named after — and the guard the same commit
introduces has a policy that would have excluded it.
Reviewing commit 753cb10 only. d58bac4 is #116, reviewed separately in this run.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/CMakeLists.txt:113-115 |
add_executable(test_net_mock test_net_mock.c) has no target_link_libraries, and tests/test_net_mock.c:16-21 includes only <stdio.h> <stdlib.h> <string.h> <assert.h> <stdint.h> <stdbool.h> — no eos/ header at all. Its 174 checks exercise 24 static functions defined in the same file. nm -u on the linked binary reports no undefined eos_* symbol. Registering it makes ctest report 38 instead of 37 without adding one bit of coverage of net/, and a regression in net_posix.c or net.c cannot fail it. |
Either link it against eos_net and point it at the real API — the suite name promises a mock transport under the real socket layer, which is worth having — or move test_net_mock.c into NOT_BUILT with that as the reason, alongside test_emulation_simulation.c. |
| 2 | Low | tests/unit/test_cmake_test_registration.py:22-46 |
The guard checks that every suite is built and that every built target is registered with ctest. It does not check that a registered suite reaches any product code, which is why finding 1 passes it. The NOT_BUILT docstring already articulates the standard the guard does not enforce — test_emulation_simulation.c is excluded for asserting "a literal against itself … so it cannot fail", and test_profiles.c for reporting "0/0 and exits 0, which is a pass that tested nothing". |
Add a fourth assertion: a suite registered by add_executable must either appear in a target_link_libraries line or be listed in a new SELF_CONTAINED dict with a reason. That closes the gap the docstring already names. |
| 3 | Low | tests/CMakeLists.txt:104-107 |
The comment on test_mpu_validate — "Landed with the fix it guards (#109) and lost its registration to a merge" — is the one claim in this commit I could not confirm from the tree. It is also the claim test_mpu_validate_suite_is_registered is pinned to, so it is worth being exact. |
Quote the merge that dropped it, or drop the causal clause and keep the fact: the file is present and nothing built it. |
| 4 | Low | tests/test_net_mock.c:19 |
#include <assert.h> is unused — the file defines and uses its own CHECK() macro and has zero assert() calls. Same vestigial include #116 removes from test_net.c. |
Delete the line. |
Verified against this PR's head:
$ cmake -B build/host -DEOS_BUILD_TESTS=ON && cmake --build build/host -j6
# exit 0, no errors
$ cd build/host && ctest --output-on-failure
100% tests passed, 0 tests failed out of 38
The four newly registered suites are substantive, not placeholders:
$ ./tests/test_mpu_validate -> === ALL 6 MPU TESTS PASSED ===
$ ./tests/test_e2e -> ALL 88 CHECKS PASSED
$ ./tests/test_net_mock -> ALL 174 MOCK NETWORK CHECKS PASSED
$ ./tests/test_hal_stm32f4 -> === ALL 21 STM32F4 PERIPHERAL TESTS PASSED ===
The guard runs and is complete over the tree:
PASS test_every_built_suite_is_registered_with_ctest
PASS test_every_c_suite_is_built_or_listed
PASS test_mpu_validate_suite_is_registered
PASS test_not_built_list_has_no_stale_entries
PASS test_not_built_names_still_exist
suites: 43 registered: 38 NOT_BUILT: 5
neither registered nor excluded: []
It is also reachable from CI — .github/workflows/ci.yml:77 and
.github/workflows/python-tests.yml:32 both run python3 -m pytest tests/ -v, which
collects tests/unit/. That matters: a registration guard that CI does not run would be
decoration, and this one is not.
Finding 1, evidence:
$ grep -n '^#include' tests/test_net_mock.c
16:#include <stdio.h>
17:#include <stdlib.h>
18:#include <string.h>
19:#include <assert.h>
20:#include <stdint.h>
21:#include <stdbool.h>
$ nm -u build/host/tests/test_net_mock | grep -i eos
# no output
test_mpu_validate links eos_rtos_security, test_e2e links eos_kernel, and
test_hal_stm32f4 links eos_hal with the boards/stm32f407_discovery include
directory — all three do reach product code. test_net_mock is the only one that does
not.
Two things the brief asks about specifically, both clean here. No check was weakened:
the commit only adds registrations, and tests/CMakeLists.txt:18's add_compile_options(-UNDEBUG)
keeps assertions live in test targets, so none of the four suites can pass vacuously
under Release. And the PR's 38/38 claim is backed — it reproduces exactly.
Architecture conformance
Conforms. Master design §21 places eos in Tier 1 - Foundation, and the architect
target layout puts unit, functional, fuzz, performance and simulation suites under
tests/ — which is where all of this lands. No include, link or manifest edge points up
a tier; the new targets link eos_rtos_security, eos_kernel and eos_hal, all within
the same repo and all below the code under test. §5.1 holds.
test_hal_stm32f4 deserves an explicit note because it looks like a violation and is
not. tests/CMakeLists.txt:118-119 adds boards/stm32f407_discovery to the include
path of a suite that links eos_hal. The architect rule is that hal/ contains no board
specifics and "a board file is the leaf, never the trunk" — that constrains hal/, not a
test, and the include is on the test target only, so no board detail reaches eos_hal
itself. Registering it is what §22 asks for: a Reference-tier board is supposed to have
CI behind it, and six STM32F4 register suites that nothing compiled were not that.
The commit also serves §28's claims policy directly. A suite present in the tree but
built by nothing is the exact shape of an Implemented claim with no evidence behind
it; the guard converts that from invisible to a failing test. Finding 1 is the one place
the commit falls short of its own standard.
Proposed changes
Land this. The registration fix and the guard are worth having as they stand, and
finding 1 is a follow-up rather than a blocker — test_net_mock being green-but-hollow
is no worse than it was unbuilt, and the other three suites are real coverage that has
been dark.
Then, smallest sequence:
- Decide
test_net_mock's fate. If it is meant to driveeos_netthrough a mock
transport,target_link_libraries(test_net_mock PRIVATE eos_net)and rewrite it
againsteos/net.h; expect it to fail first, which is the point. If nobody will do
that now, move it toNOT_BUILTso the count stops overstating coverage. - Add the
SELF_CONTAINEDassertion from finding 2, so the next suite in this shape is
caught by the guard instead of by review. - Findings 3 and 4 are text and one include; fold them into either step.
Merge-order note, unchanged from the existing comments and re-verified: this branch is
now stacked on #116 rather than parallel to it, so the double-#if EOS_ENABLE_NET
hazard is gone — git show refs/pull/114/head:net/src/net_posix.c has exactly one
opening guard and one #endif. #115 sits on top of this commit and adds its block at
the same tests/CMakeLists.txt anchor as #118, so whichever of those two lands second
needs a one-hunk rebase there.
Not checked
- I did not verify that any of the four suites fails when the code it covers
regresses. I confirmed they run, are substantive, and pass; I did not revert #109 to
watchtest_mpu_validatego red. Without that, "these six cases guard the MPU fix" is
supported by their content but not demonstrated. - I did not read
test_net_mock.c's 174 checks individually. Finding 1 rests on the
include list and the emptynm -uoutput, which is sufficient to show it cannot reach
net/, but I did not assess whether the mock logic is internally correct. - I did not audit the five
NOT_BUILTexclusion reasons against their files. Each reads
as specific and checkable, and the guard proves the names still exist and are not
registered, but I took the stated reasons on trust. - I ran the guard's five test functions by importing the module directly;
pytestis
not installed on this host. That exercises the same assertions, not pytest's
collection of them. - CI is green at this head. I read the check list, not the job logs.
Automated architecture review of 753cb10a9668 — 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.
…removed `CI — eos` and `Python Unit Tests` are red on master (769191a), and both fail for one reason: these suites exist in tests/ but no add_executable() in tests/CMakeLists.txt builds them, so they never run: ['test_crypto_ed25519_loworder.c'] 3aa8644 (embeddedos-org#93, "remove duplicate test targets from tests/CMakeLists.txt") removed all four lines that build and register test_crypto_ed25519_loworder. It was not a duplicate — there was exactly one registration before that commit, and it took it. What stopped running is the suite that guards embeddedos-org#101: Ed25519 rejecting low-order public keys. A low-order key makes every term of the verification equation collapse to the identity regardless of the message, so a signature of all zeros verifies against any content at all. The fix is still in services/crypto/src/ed25519_verify.c and still correct; nothing has been checking it since embeddedos-org#93 merged, and nothing would have noticed if a later change had removed it too. Restored verbatim from 3aa8644^, in its original position before test_crypto_sha512. No other file changes. The guard that caught this is test_cmake_test_registration.py, added recently. It did exactly its job — this is the first thing it found. Verified: test_cmake_test_registration.py -> 5 passed (1 failed on master) pytest tests/ -> 15 passed (14 passed 1 failed on master) test_crypto_ed25519_loworder -> 5/5 tests passed ctest -> 39/39 passed (master builds 38)
… the guard checks Reduces this PR to what it still contributes, and answers the review. The four registrations it opened with have landed on master by other routes -- test_mpu_validate, test_e2e, test_hal_stm32f4 and the registration guard are all there. Merging the branch as it stood would have reverted 505 lines that landed since, including embeddedos-org#120's pkg trust anchor. Reset onto embeddedos-org#127 and kept only what is still true. Finding 1 (Medium) -- test_net_mock was registered with no target_link_libraries and includes no EmbeddedOS header. Its mock_net_* functions are called directly rather than standing in for eos_net_*, so no change to net/ can make it fail: 614 lines asserting a mock against itself. Registering it added a green ctest entry covering nothing, which is precisely the failure mode the guard in this same commit exists to surface -- and the guard's own NOT_BUILT policy is where it belongs. Listed there with the reason, including what would make it worth building: driving the real eos_net_* API against the mock transport, which is what its docblock already claims it does. Finding 2 (Low) -- documented the guard's limit where the policy is defined. It checks that every suite is built and registered with ctest. It does not check that a suite reaches the code it is named after, and a file that links nothing satisfies it completely. Worth stating next to NOT_BUILT, because that is the list someone reaches for when the guard fires. Findings 3 and 4 are moot at this head: the test_mpu_validate comment and test_net_mock's vestigial assert.h include both belonged to hunks that are either upstream or removed here. Verified: pytest tests/ 15 passed ctest 37/37 PASS (was 38 with test_net_mock, which tested nothing) Refs embeddedos-org#114
753cb10 to
b057e03
Compare
… the guard checks Reduces this PR to what it still contributes, and answers the review. The four registrations it opened with have landed on master by other routes -- test_mpu_validate, test_e2e, test_hal_stm32f4 and the registration guard are all there. Merging the branch as it stood would have reverted 505 lines that landed since, including embeddedos-org#120's pkg trust anchor. Reset onto embeddedos-org#127 and kept only what is still true. Finding 1 (Medium) -- test_net_mock was registered with no target_link_libraries and includes no EmbeddedOS header. Its mock_net_* functions are called directly rather than standing in for eos_net_*, so no change to net/ can make it fail: 614 lines asserting a mock against itself. Registering it added a green ctest entry covering nothing, which is precisely the failure mode the guard in this same commit exists to surface -- and the guard's own NOT_BUILT policy is where it belongs. Listed there with the reason, including what would make it worth building: driving the real eos_net_* API against the mock transport, which is what its docblock already claims it does. Finding 2 (Low) -- documented the guard's limit where the policy is defined. It checks that every suite is built and registered with ctest. It does not check that a suite reaches the code it is named after, and a file that links nothing satisfies it completely. Worth stating next to NOT_BUILT, because that is the list someone reaches for when the guard fires. Findings 3 and 4 are moot at this head: the test_mpu_validate comment and test_net_mock's vestigial assert.h include both belonged to hunks that are either upstream or removed here. Verified: pytest tests/ 15 passed ctest 38/38 PASS (test_net_mock is no longer among them; the count is unchanged from master because embeddedos-org#127, which this is stacked on, adds test_crypto_ed25519_loworder -- a suite that does exercise real code.) Refs embeddedos-org#114
b057e03 to
a706e55
Compare
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#114 "test: build four suites that existed but were never compiled, and guard it"
head: a706e55 author: Kartikey1306 ci: 2 required checks failing (pre-existing — see below)
Verdict: The judgement in the diff is right and I verified the hard part of it: test_net_mock genuinely cannot fail, and unregistering it removes a green tick that covered nothing. The guard is good — I mutated it three ways and it caught all three, including the allowlist-drift case that eBoot's version of this same file misses. What does not match is the PR body, which describes the pre-rebase revision and never mentions either of the two things this diff actually does.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/CMakeLists.txt:110-113 (removed) |
A registered, passing ctest entry is being removed. The brief treats a disabled test as a finding regardless of the reason given, so it is recorded here — and then the reason has to be weighed, which I did, and it holds. tests/test_net_mock.c includes only <stdio.h> <stdlib.h> <string.h> <assert.h> <stdint.h> <stdbool.h>; the string eos_net_ appears exactly twice in the file, both times inside a comment; master registers it with add_executable and no target_link_libraries at all; and nm on the binary master builds finds zero eos_net symbols, only its own mock_net_* statics. No change to net/ can make it fail. What is left over is the shape of the problem, not the tick: 614 lines stay in the tree with a docblock claiming "full-path testing of the net.c API without a real network stack", now provably untrue, unbuilt, and one NOT_BUILT entry away from looking like an intentional exclusion forever. |
Unregistering is the weaker of the two available fixes. The NOT_BUILT reason you wrote says the stronger one out loud — "rewriting it to drive the real eos_net_* API against the mock transport, which is what its own docblock claims it does, would make it worth building". Do that, or delete the file. A 614-line suite that asserts against its own mocks is the exact artefact this PR exists to stop producing, and leaving it in place preserves it. |
| 2 | Medium | PR body | The body describes a different and much larger change than the diff. It says nine suites are compiled by nothing and four are being registered — test_mpu_validate, test_hal_stm32f4, test_e2e, test_net_mock — with "ctest: 34 → 38". All four of those are already registered on origin/master, which is at 38 today; the diff touches none of them except to remove test_net_mock. It lists test_net_mock in the table of suites that "build and pass as they are", which is the direct opposite of what the diff does to it. Neither of the diff's two real changes — unregistering test_net_mock, registering test_crypto_ed25519_loworder — is mentioned anywhere. The "Verified: 38 of 38 (was 34)" block is therefore output from a tree that no longer exists. |
Rewrite the body around the two changes. The nm result and the "zero eos_net symbols" argument belong in it: that is the evidence that makes finding 1 defensible, and right now it is only in the NOT_BUILT string. 38 → 38 (one out, one in) is the honest headline. |
| 3 | Low | CI | Two required checks are red: Cross-compile ARM64 kernel and Full-stack integration summary. Not caused by this PR — the identical pair fails on all four open eos heads in this run, including #117, whose whole diff is .gitignore plus deletion of committed build artifacts. Named because the brief requires failing required checks to be named. |
Nothing here. It does mean this cannot merge on a green tree until master's ARM64 cross-compile is repaired. |
Not a finding, but worth stating
The tests/CMakeLists.txt hunk registering test_crypto_ed25519_loworder is identical to the one in #116, at the same anchor. I checked whether that reproduces the silent-bad-merge hazard your own 2026-09-01 comment warns about on these two PRs: it does not. git merge-tree of the two heads is clean, and the merged tests/CMakeLists.txt contains exactly one add_executable(test_crypto_ed25519_loworder ...) and zero references to test_net_mock. Either merge order is safe. That pinned comment is now stale for both PRs — neither head touches net/src/net_posix.c any more — and it is worth editing, because it currently reads as a live blocker.
Architecture conformance
Conforms. eos is Tier 1 Foundation (§21), and both files live under tests/, which .ai/architect.md names in the reference layout. No dependency direction is affected: the added registration links test_crypto_ed25519_loworder against eos_crypto, a core service in the same repository, and the removal takes an edge away rather than adding one. Nothing points up a tier.
The substance sits squarely on §28's evidence policy. "Implemented" requires "code and functional tests", and a suite that links no library and asserts against its own fixtures is a functional test of nothing — counting it makes the status matrix say something the code does not support. .ai/security.md makes the same point one notch harder for TCB code ("a security suite that silently collected nothing is a failed check reported as green"); test_net_mock is the non-security version of it, and the honest move is the one this diff makes.
The guard is the durable part. tests/unit/test_cmake_test_registration.py is notably stronger than the equivalent in eBoot#95: test_not_built_list_has_no_stale_entries and test_not_built_names_still_exist validate the allowlist in both directions, which is precisely the gap I found and reported in eBoot's copy, where a NO_TEST_MACRO entry naming a deleted file passes silently. If those two files are meant to stay siblings, eBoot's should take these two checks from here.
Proposed changes
- Rewrite the PR body (finding 2). Nothing else in the diff needs to change to merge.
- Follow-up, not this PR: rewrite
tests/test_net_mock.cagainsteos_net_*and linkeos_net, or delete it. Whichever you choose, theNOT_BUILTentry then leaves — andtest_not_built_list_has_no_stale_entrieswill enforce that it does. - Edit the 2026-09-01 cross-PR warning comment on #114/#116.
Optional, and I would not block on it: the one thing the guard cannot check is whether a NOT_BUILT reason is still true. test_net_mock.c's reason is a factual claim — "includes no EmbeddedOS header, links no library" — and it could be pinned as one, e.g. asserting the file contains no eos_net_ call outside a comment. That converts a prose justification into something that expires when it stops being true. It is also the kind of check that gets clever and brittle, so it is a suggestion rather than a recommendation.
Not checked
- Not run: the two failing required checks.
Cross-compile ARM64 kernelneeds a cross toolchain andFull-stack integration summaryneeds the integration harness; I reproduced neither, so I can say only that they fail on all foureosheads in this run. I did not determine the root cause on master. - Not verified: the body's claims about the other four excluded suites — that
test_net_integration.changs,test_profiles.creports0/0,test_performance_benchmarks.casserts a busy-loop threshold,test_emulation_simulation.casserts a literal against itself. Those entries predate this diff and I did not re-derive them. Only the newtest_net_mock.centry was checked, and it checks out. - Not verified: the
test_firmware.c/backends/analysis in the body's "Separate finding" section — the 30 compile errors, the missingadd_subdirectory(backends). Not in this diff and not examined. - Not verified: that
test_crypto_ed25519_loworderis correct, only that it builds, runs and passes its five cases. I did not check its low-order encodings against the curve. - Not checked: whether the
ADD_EXECUTABLE_RE/ADD_TEST_REpatterns handle multi-lineadd_executable(calls. Every call in the current file is single-line, so it does not bite today.
Evidence
Read-only git archive extractions of origin/master (feee272) and this head; the eos checkout was clean and untouched.
origin/master cmake -B build/test -DCMAKE_BUILD_TYPE=Release -DEOS_BUILD_TESTS=ON \
-DEOS_PRODUCT=vbox_test && cmake --build -> OK
ctest --no-tests=error -> 38/38 passed
this head same configuration -> OK
ctest --no-tests=error -> 38/38 passed
(one out: test_net_mock; one in: test_crypto_ed25519_loworder)
pytest tests/unit/test_cmake_test_registration.py -> 5 passed
finding 1, that test_net_mock cannot fail:
tests/test_net_mock.c #include lines -> libc only, no eos/ header
grep -n eos_net_ -> 2 hits, both in comments (lines 12, 92)
tests/CMakeLists.txt @ master -> add_executable(test_net_mock test_net_mock.c)
add_test(...) and NO target_link_libraries
nm build/test/tests/test_net_mock | grep -ci eos_net -> 0
defined text symbols matching "net": mock_net_recv, mock_net_send, mock_net_socket
guard, mutation-probed against this head (all three caught):
add a NOT_BUILT entry naming a file that does not exist
-> FAILED test_not_built_names_still_exist
register a suite that is still listed in NOT_BUILT
-> FAILED test_not_built_list_has_no_stale_entries
delete the test_mpu_validate add_executable()
-> FAILED test_mpu_validate_suite_is_registered (+ test_every_c_suite_is_built_or_listed)
overlap with #116:
git merge-tree --write-tree fafec150 a706e552 -> clean, no conflict
merged tests/CMakeLists.txt: add_executable(test_crypto_ed25519_loworder ...) -> 1
test_net_mock -> 0
Automated architecture review of a706e552f4a2 — 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.
Every open eos pull request is currently red on `EoS Full-Stack Simulation`, and none of them caused it. The kernel job checks out embeddedos-org/eBoot at `ref: master`, and eBoot master does not compile: include/eos_image.h:135 and :142 static-assert offsetof(eos_image_header_t, reserved) for a member that became tlv_len + tlv_hash, and core/ed25519_verify.c redefines point_is_identity -- two pairs of PRs that each merged clean and broke the build together. eBoot embeddedos-org#94 repairs it and waits on review; until it lands, no change to any eos PR can make this job pass, and after it lands the next broken eBoot master does the same thing again. A floating `ref: master` makes every eos PR's CI depend on the moment-to- moment state of another repository's default branch. Pinning the two compiled-against checkouts makes the job a function of this repository plus two recorded SHAs -- red means the PR broke something, green means it did not, and a cross-repo bump is a reviewable one-line diff with CI on it. EBOOT_COMMIT a172a6d6e1e6 the newest eBoot master ancestor that compiles; found by walking master back and building each candidate. Its child 8a015b2 already carries the ed25519 redefinition. EBUILD_COMMIT e5d8052f3e2c ebuild's current master, frozen as-is. Both of its checkouts feed steps that are commented out (eFab does not exist), so this pin only removes the float. The middleware matrix (10 repos) still floats; it runs those repos' own tests rather than compiling eos against them, and 10 more pins deserve their own decision. When eBoot embeddedos-org#94 merges, bump EBOOT_COMMIT to the merge SHA. Verified before pushing, master vs pin under the job's own configuration (-DEBLDR_BOARD=qemu_arm64 -DEBLDR_VERIFY_STAGE1=OFF -DCMAKE_BUILD_TYPE=Release, cross-compile mode): eBoot@master fails at the eos_image.h asserts, the exact error in the job log; eBoot@a172a6d builds every target, rc=0. Both pinned SHAs verified reachable from their repos' master. Workflow parses; the three `ref:` edits and the env block are the whole diff. This PR's own simulation run is the live test: it uses this branch's workflow and should go green while every master-based PR stays red. Carried onto this branch from embeddedos-org#135 (001128d) so this PR's own simulation job runs against a buildable eBoot: each pull request executes its own copy of the workflow, so the fix greens a PR only once its branch contains it. Byte-identical to embeddedos-org#135's commit -- git drops it as already-upstream the moment embeddedos-org#135 merges. The pin itself is embeddedos-org#135's to review.
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#114 "test: build four suites that existed but were never compiled, and guard it"
head: f43d860 author: Kartikey1306 ci: 24 pass, Full-stack integration summary pending, Create GitHub Release skipping; 0 failing
Verdict: The test-registration judgement is unchanged from the a706e552 review and still correct — verified at this head: build clean, ctest --no-tests=error 38/38, and pytest tests/unit/ 11 passed. What is new since that review is f43d860, which is a byte-identical copy of #135's cross-repo pin — the third open PR now carrying it. Two findings from the previous review are still open.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/eos-simulation.yml:11-20, :41, :48, :288 |
f43d860 ("ci: pin the eBoot and ebuild checkouts instead of floating on master") is the same change as #135, and also as #116's 2a32e3a. Verified: git diff of .github/workflows/eos-simulation.yml between this head, #116's head and #135's head is empty in both comparisons — same env: block, same comment, same three ref: substitutions, byte for byte. Three open PRs now carry one CI change, and none of the three bodies mentions it. Identical hunks merge without conflict so nothing breaks, but a cross-repository dependency pin is being reviewed in a PR titled "build four suites that existed but were never compiled", where nobody is looking for it. This is the same overlap hazard your own 2026-09-01 comment raises about #114/#116. |
Drop f43d860. #135 is scoped to the pin, has the staleness guard commit (f34edb3) that these copies lack, and has been reviewed twice. Its substance — the missing staleness signal, and the ten-repo middleware matrix still on ref: master at :248 — is reviewed there and is not re-raised here. |
| 2 | Medium | PR body | Unresolved from the a706e552 review and now further from the diff. The body says nine suites are compiled by nothing and that four are being registered — test_mpu_validate, test_hal_stm32f4, test_e2e, test_net_mock — with "ctest: 34 → 38". All four are already registered on origin/master (feee272), which runs 38 today, and this branch's own change to test_net_mock is to unregister it — the opposite of the table entry that lists it under "Four build and pass as they are". The body also does not mention f43d860 at all. The "Verified: 38 of 38 (was 34)" block is output from a tree that no longer exists, which per the brief is the finding rather than a cosmetic issue: it reads as evidence for the change in front of the reviewer. |
Rewrite the body around what this branch owns: unregistering test_net_mock because it covers nothing, and the NOT_BUILT documentation that says so. The nm evidence belongs in the body — right now the argument that makes the removal defensible exists only inside a CMake comment string. 38 → 38 is the honest headline once the stack parent is accounted for (see below). |
Stack attribution, which the body should state and does not. git log at this head is f43d860 → a706e55 → 13a8ffa → feee272. 13a8ffa is #127's commit ("fix(build): restore the low-order Ed25519 suite that #93 removed"), so the add_executable(test_crypto_ed25519_loworder ...) hunk at tests/CMakeLists.txt:93-97 is #127's, not this PR's — and the same is true of #116, whose history also contains 13a8ffa. A correction to my own earlier reviews of #116: I credited that registration to #116 twice, including in the report posted against #116 minutes ago. It belongs to #127. This PR's own content is a706e55 (unregister test_net_mock, document the NOT_BUILT limits) plus f43d860 (the duplicated pin). Saying so in the body would make all three PRs easier to sequence.
Still open from a706e552, not restated as findings:
tests/test_net_mock.cremains in the tree — 614 lines whose docblock claims "full-path testing of the net.c API without a real network stack", now unbuilt and provably not doing that. Unregistering is the weaker of the two available fixes; theNOT_BUILTreason you wrote names the stronger one ("rewriting it to drive the realeos_net_*API against the mock transport … would make it worth building"). Do that or delete the file;test_not_built_list_has_no_stale_entrieswill then enforce that the entry leaves.- The 2026-09-01T15:30:57Z cross-PR warning is stale for both #114 and #116 — neither head touches
net/src/net_posix.c, so theunterminated conditional directiveit demonstrates cannot occur. It currently reads as a live blocker on two PRs.
Architecture conformance
Conforms. eos is Tier 1 — Foundation (§21). tests/ and tests/unit/ are the directories .ai/architect.md names in the reference layout, and .github/workflows/ is Infrastructure per §21. No dependency direction is affected by the test change: unregistering test_net_mock removes an edge rather than adding one, and nothing new points up a tier (§5.1). The pin in f43d860 is a CI-time dependency from eos onto eBoot and ebuild, both Tier 1, so it points sideways; §5.1 governs runtime dependencies and is silent on this class, and that gap plus this exact SHA are already recorded in .ai/autoreview/proposals/2026-09.md (the 2026-09-03 entry on build- and CI-time dependencies and the 2026-09-04 addendum from #135). Nothing new to propose.
The substance sits on §28's evidence policy: "Implemented" requires "code and functional tests", and a suite that links no library and asserts against its own fixtures is a functional test of nothing — counting it makes the status matrix claim something the code does not support. .ai/security.md puts the same point harder for TCB code ("a security suite that silently collected nothing is a failed check reported as green"); test_net_mock is the non-security form of it and removing the tick is the honest move.
The guard remains the durable part, and a706e55 improves it in the one way it can be improved without becoming brittle: the new NOT_BUILT docblock states what the guard cannot check — that a suite reaches the code it is named after — instead of leaving a reader to assume registration implies coverage. It is still stronger than eBoot#95's sibling copy, which validates its allowlist in one direction only; if those two files are meant to stay siblings, eBoot's should take test_not_built_list_has_no_stale_entries and test_not_built_names_still_exist from here.
Proposed changes
- Drop
f43d860; let #135 carry the pin. - Rewrite the body around
a706e55, and state the#127 → #116 → thisstack. - Follow-up, not this PR: rewrite
tests/test_net_mock.cagainsteos_net_*and linkeos_net, or delete it. - Edit the 2026-09-01 cross-PR comment on #114/#116.
- Nothing in the CMake or the Python needs to change to merge.
Not checked
Full-stack integration summarywas still pending when this bundle was built, so I cannot report its outcome; nothing was failing. Worth recording thatCross-compile ARM64 kernel— red on this head's predecessor and on every openeoshead in the previous run — is green here, which is whatf43d860buys and why it keeps being copied.Cross-compile ARM Cortex-M4is declared by two workflows (.github/workflows/build.yml:74andci.yml:117), so it produces two check runs under one name on everyeosPR. Not this PR's, and not a finding against it, but it means that check's state is not answerable from a name — the defect ebuild#104 is fixing inebuild, live here.- The guard's mutation behaviour: NOT RE-PROBED at this head. The three mutations were verified at
a706e552andtests/unit/test_cmake_test_registration.pychanged only by a comment block since, so the behaviour is unchanged by inspection rather than by re-running. - Not verified: the body's claims about the four other excluded suites (
test_net_integration.changs,test_profiles.creports0/0,test_performance_benchmarks.casserts a busy-loop threshold,test_emulation_simulation.casserts a literal against itself). Those entries predate this branch and I did not re-derive them. - Not verified: the body's
test_firmware.c/backends/analysis — the missingadd_subdirectory(backends)and the 30 compile errors. Not in this diff. It is #118's subject and is reviewed there. - Not verified: that
test_crypto_ed25519_loworderis correct, only that it builds, runs and passes. That suite is #127's anyway. - Not verified: the pinned eBoot tree.
a172a6dexists and is an ancestor of eBoot'sorigin/master; I did not compile eos's kernel job against it, so "the pin makes the job green" rests onchecks.txt. - Not checked: whether
ADD_EXECUTABLE_RE/ADD_TEST_REhandle a multi-lineadd_executable(. Every call in the file is single-line, so it does not bite today.
Evidence
Read-only git archive extraction of f43d860e; the eos checkout was clean and was not modified.
this head cmake -B build/test -DCMAKE_BUILD_TYPE=Release -DEOS_BUILD_TESTS=ON \
-DEOS_PRODUCT=vbox_test && cmake --build -> OK
ctest --no-tests=error -> 38/38 passed
pytest tests/unit/ -> 11 passed
(pytest 9.1.1 from ~/.local/bin with the system site-packages on
PYTHONPATH; the system interpreter has no pytest of its own)
finding 1:
git diff f43d860e <#116 head 2a32e3a> -- .github/workflows/eos-simulation.yml -> empty
git diff f43d860e <#135 head f34edb3> -- .github/workflows/eos-simulation.yml -> empty
stack attribution:
git log --oneline f43d860e -> f43d860, a706e55, 13a8ffa, feee272
13a8ffa = "fix(build): restore the low-order Ed25519 suite that #93 removed" (#127)
tests/CMakeLists.txt @ origin/master: grep -c ed25519_loworder -> 0
Automated architecture review of f43d860eab49 — 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.
master's CI is red on the registration guard added in embeddedos-org#114: these suites exist in tests/ but no add_executable() in tests/CMakeLists.txt builds them, so they never run: ['test_crypto_ed25519_loworder.c'] The guard is doing its job. The suite landed with the subgroup fix it guards and was never registered, so the five cases proving an all-zero package key is refused -- the concrete bug its header describes, eos_pkg shipping eos_pkg_public_key[32] = {0} and accepting every file handed to it -- had never once run. It builds and passes as it stands, so it is registered rather than excluded: 5/5 on its own, ctest 38 -> 39.
Nine suites are compiled by nothing
Nothing reports this. The file stays in the tree, ctest counts one fewer test, and the run is green.
test_mpu_validate.cis the one that matters. It landed with the MPU fix it guards (#109); a later merge kept the file and dropped theadd_executable()that built it. Its six cases — regions the MPU cannot program, and the divide-before-check that preceded them — have not run since:Four build and pass as they are
test_mpu_validatetest_hal_stm32f4test_e2etest_net_mockctest: 34 → 38.
Five stay out, each with its reason
Recorded next to the name in the guard, so the exclusion is reviewable rather than silent:
test_net_integration.c— hangs.test_accept_no_client()assertseos_net_accept()returnsEOS_SOCKET_INVALID, which is stub semantics; the POSIX implementation blocks.test_firmware.c— cannot link. See below.test_profiles.c— reports0/0 tests passedand exits 0.main()accepts only the literal strings"products","./products","../products", andproducts/holds headers, not the*.yamlthe scan looks for. It is structurally incapable of running a case.test_performance_benchmarks.c— assertslatency_ns < 100.0from a busy loop: a property of the runner, not the code.test_emulation_simulation.c— assertsmock_val == 0x55AA55AAone line after assigning that literal, and never reads the register pointer it declares. It cannot fail.Three of those would have added a green tick for nothing, which is the thing #89 exists to prevent.
The guard
tests/unit/test_cmake_test_registration.pyparsestests/CMakeLists.txtstatically — no cmake, no compiler — and asserts:test_*.cis either built or listed with a reason;test_mpu_validate.cis built.Verified against the regression rather than assumed — dropping the registration again:
eBoot has carried this guard since a merge dropped
test_fw_transportthere; this is the same file adapted to this layout.Separate finding, not fixed here
test_firmware.ccannot link becauseeos_systems'sfirmware.ccallseos_backend_find()andbackends/is in noadd_subdirectory()— solibeos_backends.ais never produced.cmd/eos/CMakeLists.txtlinks it too, andcmd/is not in the build either. Addingadd_subdirectory(backends)does not fix it: all 12 sources fail to compile against the current headers, with 30 errors —unknown type name 'EosResult',use of undeclared identifier 'EOS_MAX_BACKENDS',EOS_OK. That subsystem has drifted out of the build and out of date with the API, and repairing it is its own piece of work.Verified