Skip to content

test: build four suites that existed but were never compiled, and guard it - #114

Merged
srpatcha merged 3 commits into
embeddedos-org:masterfrom
Kartikey1306:test/register-orphaned-suites
Sep 8, 2026
Merged

srpatcha merged 3 commits into
embeddedos-org:masterfrom
Kartikey1306:test/register-orphaned-suites

Conversation

@Kartikey1306

@Kartikey1306 Kartikey1306 commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #113, which master needs to build at all. Review the last commit.

Nine suites are compiled by nothing

$ for f in tests/test_*.c; do grep -rqF "$(basename $f)" --include=CMakeLists.txt . || echo "$f"; done
tests/test_e2e.c                     tests/test_net_integration.c
tests/test_emulation_simulation.c    tests/test_net_mock.c
tests/test_firmware.c                tests/test_performance_benchmarks.c
tests/test_hal_stm32f4.c             tests/test_profiles.c
tests/test_mpu_validate.c

Nothing reports this. The file stays in the tree, ctest counts one fewer test, and the run is green.

test_mpu_validate.c is the one that matters. It landed with the MPU fix it guards (#109); a later merge kept the file and dropped the add_executable() that built it. Its six cases — regions the MPU cannot program, and the divide-before-check that preceded them — have not run since:

$ git show 495dbef:tests/CMakeLists.txt | grep -c mpu_validate   # the merged branch
4
$ grep -c mpu_validate tests/CMakeLists.txt                      # master
0

Four build and pass as they are

suite
test_mpu_validate 6 cases — registration restored
test_hal_stm32f4 21 cases — needed only the board header directory on the include path, which is the whole reason it never ran
test_e2e kernel boot-to-task path
test_net_mock eNet socket API against a mock transport

ctest: 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() asserts eos_net_accept() returns EOS_SOCKET_INVALID, which is stub semantics; the POSIX implementation blocks.
  • test_firmware.c — cannot link. See below.
  • test_profiles.c — reports 0/0 tests passed and exits 0. main() accepts only the literal strings "products", "./products", "../products", and products/ holds headers, not the *.yaml the scan looks for. It is structurally incapable of running a case.
  • test_performance_benchmarks.c — asserts latency_ns < 100.0 from a busy loop: a property of the runner, not the code.
  • test_emulation_simulation.c — asserts mock_val == 0x55AA55AA one 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.py parses tests/CMakeLists.txt statically — no cmake, no compiler — and asserts:

  1. every test_*.c is either built or listed with a reason;
  2. every built target is registered with ctest (otherwise a failure in it cannot fail the build);
  3. the exclusion list has no stale entries (a suite that gets registered must leave it);
  4. the exclusion list names no file that has been deleted;
  5. by name, that test_mpu_validate.c is built.

Verified against the regression rather than assumed — dropping the registration again:

E  AssertionError: these suites exist in tests/ but no add_executable() ... builds them,
   so they never run: ['test_mpu_validate.c']
E  AssertionError: test_mpu_validate.c is not built ... would silently stop running
2 failed, 3 passed

eBoot has carried this guard since a merge dropped test_fw_transport there; this is the same file adapted to this layout.

Separate finding, not fixed here

test_firmware.c cannot link because eos_systems's firmware.c calls eos_backend_find() and backends/ is in no add_subdirectory() — so libeos_backends.a is never produced. cmd/eos/CMakeLists.txt links it too, and cmd/ is not in the build either. Adding add_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

ctest                            100% passed, 38 of 38   (was 34)
pytest tests/                    15 passed
cmake -S . -B build              default (non-test) build OK

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

⚠️ #114 and #116 both add the #if EOS_ENABLE_NET guard to net/src/net_posix.c, and merging both breaks master — with no merge conflict to warn you.

Both PRs came from this account. #116 fixes the guard plus the test_net link error; #114 fixes the same guard plus adds a CMake test-registration check. The overlap is net/src/net_posix.c and tests/test_net.c.

Git merges them silently, because each adds its guard at a different point in the include block:

$ git merge fork/test/register-orphaned-suites   # onto #116
Auto-merging net/src/net_posix.c
Auto-merging tests/test_net.c
   (no conflict)

$ grep -c '^#if EOS_ENABLE_NET'            net/src/net_posix.c   ->  2
$ grep -c '^#endif /\* EOS_ENABLE_NET \*/' net/src/net_posix.c   ->  1

$ cmake -B build/host -DCMAKE_BUILD_TYPE=Release && cmake --build build/host
net/src/net_posix.c:35:2: error: unterminated conditional directive

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 net_posix.c guard and the test_net.c fix, and drop those two files from #114, leaving it the tests/CMakeLists.txt registration guard and tests/unit/test_cmake_test_registration.py — which is the part of #114 that is genuinely its own and worth having. In either order, the second one merged then needs a rebase rather than a straight merge.

Related: I closed #113 in favour of #116 for the same overlap.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Rebased onto #116. #113 is closed, and this branch carried its two commits; #116 repairs the same two things on master (the EOS_ENABLE_NET guard, and the test link — via CHECK() rather than including assert.h, which is the better call). Those commits are dropped here, so the stack is now #116 → this. 38/38 locally.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Decide test_net_mock's fate. If it is meant to drive eos_net through a mock
    transport, target_link_libraries(test_net_mock PRIVATE eos_net) and rewrite it
    against eos/net.h; expect it to fail first, which is the point. If nobody will do
    that now, move it to NOT_BUILT so the count stops overstating coverage.
  2. Add the SELF_CONTAINED assertion from finding 2, so the next suite in this shape is
    caught by the guard instead of by review.
  3. 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
    watch test_mpu_validate go 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 empty nm -u output, 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_BUILT exclusion 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; pytest is
    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)
Kartikey1306 added a commit to Kartikey1306/eos that referenced this pull request Sep 3, 2026
… 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
@Kartikey1306
Kartikey1306 force-pushed the test/register-orphaned-suites branch from 753cb10 to b057e03 Compare September 3, 2026 10:21
… 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
@Kartikey1306
Kartikey1306 force-pushed the test/register-orphaned-suites branch from b057e03 to a706e55 Compare September 3, 2026 10:21

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Rewrite the PR body (finding 2). Nothing else in the diff needs to change to merge.
  2. Follow-up, not this PR: rewrite tests/test_net_mock.c against eos_net_* and link eos_net, or delete it. Whichever you choose, the NOT_BUILT entry then leaves — and test_not_built_list_has_no_stale_entries will enforce that it does.
  3. 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 kernel needs a cross toolchain and Full-stack integration summary needs the integration harness; I reproduced neither, so I can say only that they fail on all four eos heads 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.c hangs, test_profiles.c reports 0/0, test_performance_benchmarks.c asserts a busy-loop threshold, test_emulation_simulation.c asserts a literal against itself. Those entries predate this diff and I did not re-derive them. Only the new test_net_mock.c entry 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 missing add_subdirectory(backends). Not in this diff and not examined.
  • Not verified: that test_crypto_ed25519_loworder is 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_RE patterns handle multi-line add_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 srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.c remains 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; the NOT_BUILT reason you wrote names the stronger one ("rewriting it to drive the real eos_net_* API against the mock transport … would make it worth building"). Do that or delete the file; test_not_built_list_has_no_stale_entries will 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 the unterminated conditional directive it 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

  1. Drop f43d860; let #135 carry the pin.
  2. Rewrite the body around a706e55, and state the #127 → #116 → this stack.
  3. Follow-up, not this PR: rewrite tests/test_net_mock.c against eos_net_* and link eos_net, or delete it.
  4. Edit the 2026-09-01 cross-PR comment on #114/#116.
  5. Nothing in the CMake or the Python needs to change to merge.

Not checked

  • Full-stack integration summary was still pending when this bundle was built, so I cannot report its outcome; nothing was failing. Worth recording that Cross-compile ARM64 kernel — red on this head's predecessor and on every open eos head in the previous run — is green here, which is what f43d860 buys and why it keeps being copied.
  • Cross-compile ARM Cortex-M4 is declared by two workflows (.github/workflows/build.yml:74 and ci.yml:117), so it produces two check runs under one name on every eos PR. 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 in ebuild, live here.
  • The guard's mutation behaviour: NOT RE-PROBED at this head. The three mutations were verified at a706e552 and tests/unit/test_cmake_test_registration.py changed 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.c hangs, test_profiles.c reports 0/0, test_performance_benchmarks.c asserts a busy-loop threshold, test_emulation_simulation.c asserts 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 missing add_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_loworder is correct, only that it builds, runs and passes. That suite is #127's anyway.
  • Not verified: the pinned eBoot tree. a172a6d exists and is an ancestor of eBoot's origin/master; I did not compile eos's kernel job against it, so "the pin makes the job green" rests on checks.txt.
  • Not checked: whether ADD_EXECUTABLE_RE / ADD_TEST_RE handle a multi-line add_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.

@srpatcha
srpatcha merged commit 2812e8a into embeddedos-org:master Sep 8, 2026
28 checks passed
Kartikey1306 added a commit to Kartikey1306/eos that referenced this pull request Sep 14, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants