Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The updated contributor setup references a missing requirements-dev.txt, and the trust-anchor test fixture creation weakens POSIX file-permission hardening without an equivalent portable alternative.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR targets Windows/MSVC portability for the contributor build and test workflow by adjusting platform selection in CMake, adding MSVC-friendly struct packing/alignment, and replacing/guarding POSIX-only code paths so Windows builds don’t pull in unavailable headers/APIs.
Changes:
- Make HAL and networking backend selection explicitly UNIX-only in CMake so Windows/MSVC builds don’t compile POSIX implementations.
- Add MSVC-compatible packing/alignment for
eapp_header_tand test heap buffers. - Update contributor documentation to describe a repo-local Python virtual environment for running pytest tooling.
File summaries
| File | Description |
|---|---|
tests/test_stress.c |
Adds MSVC-compatible 8-byte alignment for the stress-test heap buffer. |
tests/test_pkg_trust_anchor.c |
Removes POSIX-only includes and switches fixture creation to fopen() for portability. |
tests/test_e2e.c |
Adds MSVC-compatible 8-byte alignment for the e2e heap buffer. |
tests/test_config.c |
Removes a POSIX-only header include to improve Windows compatibility. |
tests/CMakeLists.txt |
Gates POSIX networking test configuration behind UNIX so Windows doesn’t try to use POSIX settings/libs. |
services/pkg/eos_pkg.h |
Introduces MSVC-compatible packed layout for the .eapp header while preserving GCC/Clang packed behavior. |
hal/src/hal_rtos.c |
Adds Windows host timing (Sleep, GetTickCount) to avoid hangs in timing-related tests when using the RTOS fallback backend. |
CONTRIBUTING.md |
Documents .venv + requirements-dev.txt setup for pytest tooling (but the file appears missing in-repo). |
CMakeLists.txt |
Restricts Linux HAL and POSIX networking backend compilation to UNIX hosts. |
Review details
Suppressed comments (1)
tests/test_pkg_trust_anchor.c:132
- After switching to
fopen(), the test no longer tightens permissions on POSIX hosts. Adding a POSIX-onlychmod()here preserves the original intent (fixture not world-writable) without reintroducing Windows-only headers/APIs.
f = fopen(path, "wb");
ASSERT(f != NULL);
ASSERT(fwrite(&h, sizeof(h), 1, f) == 1);
ASSERT(fwrite(payload, 1, payload_len, f) == payload_len);
fclose(f);
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
A few portability-related details (MSVC-only pragmas guarded by _WIN32, MSVC assert/NDEBUG handling across configs, and a POSIX file-permission regression in a security-relevant test fixture) should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/test_pkg_trust_anchor.c:132
- Switching from
open(..., 0600)tofopen()reintroduces the original test’s world-writable-fixture risk on POSIX (mode becomes 0666 & ~umask). If Windows portability is the goal, consider keeping theopen+fdopenpath under#ifndef _WIN32(orchmod/fchmodafterfopen) and usingfopen()only on Windows.
f = fopen(path, "wb");
ASSERT(f != NULL);
ASSERT(fwrite(&h, sizeof(h), 1, f) == 1);
ASSERT(fwrite(payload, 1, payload_len, f) == payload_len);
fclose(f);
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Lite
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#132 "fix(windows): make contributor build portable across MSVC and POSIX hosts"
head: 6229c2f author: abhishekkapoorx ci: not run — every check is action_required
Verdict: Real portability work, and the diagnosis behind most of it is right — the AND UNIX guards fix a condition that was already wrong for any non-Linux host, and the test_hal hang has the cause the body gives. Two things need to change before this is safe to land. The .eapp on-disk header gains a second, compiler-specific packing mechanism with nothing verifying the two produce the same layout, and the MSVC replacement for -UNDEBUG fails silently — if it ever misses, the test suite passes with every assert() compiled out. Neither has been exercised anywhere but the author's machine, because no CI has run on this PR at all.
The crypto restructure looks alarming in a diff and is not: it is behaviour-preserving and stays fail-closed. I checked that specifically — see below.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | services/pkg/eos_pkg.h:90-121 |
A second packing mechanism for an on-disk wire header, with nothing checking the two agree. eapp_header_t is written to disk verbatim — fwrite(&h, sizeof(h), 1, f) at tests/test_pkg_trust_anchor.c:145 — and the comment calls it "at offset 0 of the .eapp". Until now one compiler and one attribute produced that layout. Now GCC/Clang use __attribute__((packed)) and MSVC uses #pragma pack(push,1), and no assertion pins the result: grep -n "_Static_assert" services/pkg/eos_pkg.h finds nothing. Measured here with GCC on x86-64: sizeof(eapp_header_t) == 303, alignment 1, offsets magic=0 name=5 package_id=69 capabilities=136 binary_offset=189 signature=205 hash=269 has_gui=301 has_cli=302. If the MSVC layout ever diverges — a header pulled in between the push and the pop that changes packing, a /Zp on the command line, someone adding a field after the pop — a package written on Windows is unreadable on Linux, and because signature and hash sit at 205 and 269 the symptom is a signature verification failure, not a format error. This repository's sibling has already paid for exactly this: eBoot#87, "pin the whole .efw header, not four fields of it", where two transposed same-width fields compiled clean and would have loaded an image at its entry address. §23.2 lists "Package format — versioned .epkg/.eapp metadata schema" as a compatibility contract. |
Add the asserts next to the struct, in the style of eBoot/include/eos_image.h:108-140: _Static_assert(sizeof(eapp_header_t) == 303, …) plus offsetof for magic, name, package_id, capabilities, binary_offset, signature, hash, has_gui. Two more hazards this change makes live and the asserts would pin: supported_archs[EAPP_ARCH_COUNT] is sized by an enum count (48 today), so adding one architecture silently shifts every field from offset 189 onward; and the last two members are bool, whose width is not fixed by the standard, in a struct that goes to disk — make them uint8_t. |
| 2 | High | whole PR | No check has run. gh api …/commits/6229c2f8/check-runs returns nothing, and gh run list --branch fix/windows-msvc-build-issue shows all five workflows as action_required — the first-time-contributor approval gate, which is not the author's doing. But it means a 15-file change that edits two conditions in the top-level CMakeLists.txt, a public header, the stub-crypto guard, and the test-assertion policy has been verified on exactly one platform with one compiler. Build & Test (Linux x86_64), Cross-compile ARM Cortex-M4, Cross-compile ARM64 kernel, Static Analysis and Analyze (C/C++) have said nothing. The body's checklist ticks "Unit tests pass", "Existing Windows regression tests pass" and "Code compiles without warnings" — all honestly labelled as the Windows build, and none of them is evidence about the platforms this diff also changes. |
A maintainer approves the workflow runs. Nothing here should be judged ready before at least the Linux host build, the two cross builds and static analysis are green — the AND UNIX guards and the removed #include <unistd.h> are precisely the kind of change whose Linux effect is invisible from Windows. |
| 3 | Medium | tests/CMakeLists.txt:18-24 |
The MSVC assertion-enabling path can fail silently; the GCC one cannot. add_compile_options(-UNDEBUG) undefines NDEBUG unconditionally, after any -DNDEBUG on the line. The replacement string-matches the literal /DNDEBUG inside CMAKE_C_FLAGS_{RELEASE,RELWITHDEBINFO,MINSIZEREL}. It works against today's CMake defaults for MSVC. It silently does nothing if the flag arrives spelled -DNDEBUG (MSVC accepts both), from a toolchain file, from add_definitions, from a user-supplied -DCMAKE_C_FLAGS_RELEASE=…, or from a future CMake default — and the consequence is that the 684 assert() call sites the comment above it describes become no-ops while ctest reports 38/38. That is a suite that passes having checked nothing, which .ai/reviewer.md names as a finding in its own right. CMAKE_CXX_FLAGS_<CONFIG> is not touched at all. The author's reason for avoiding /UNDEBUG is sound (D9025, "overriding '/DNDEBUG' with '/UNDEBUG'", and the body's warning scan requires no D diagnostics) — the mechanism is defensible; what is missing is a check that it worked. |
Make it fail closed rather than fail silent. A two-line guard in a header included by every test, or a tiny tests/assert_enabled.c compiled into each: #ifdef NDEBUG / #error "tests must be built with assertions enabled" / #endif. Then a missed strip is a build failure instead of a green run. Add CMAKE_CXX_FLAGS_${config} to the same loop while you are there. |
| 4 | Medium | hal/src/hal_rtos.c:15-24,33-35,433-455; CMakeLists.txt:84 |
Windows host support is added as #ifdef _WIN32 branches inside the bare-metal backend. hal.h:53-62 defines EOS_HAL_HOSTED as "compiled against a hosted POSIX environment", so Windows falls to EOS_HAL_BACKEND_RTOS and compiles hal_rtos.c — the file that also carries STM32F4 register-level GPIO at physical addresses 0x40020000+. The immediate routing is right: hal_linux.c needs <sys/ioctl.h> and sysfs GPIO, so it genuinely cannot serve Windows. But the fix puts host-OS code in the target backend, and .ai/architect.md is explicit that dependencies point downward and inward and that the target adaptation layer is not where host specifics live. Two concrete consequences: eos_hal_systick_handler() still increments systick_ms, which nothing on Windows reads, so the two halves of this file's timing now disagree about what a tick is; and the bare-metal spin-wait in rtos_delay_ms becomes unexercised on the only host that compiles this file. What actually changed is that there are now three host classes — POSIX-hosted, Windows-hosted, bare metal — and a two-way macro is being made to express them with #ifdef. |
hal/src/hal_win32.c with EOS_HAL_BACKEND_WIN32, selected by an elseif(WIN32) in the same block at CMakeLists.txt:84-90, and revert hal_rtos.c to what it was. Extend the EOS_HAL_HOSTED comment at hal.h:53 to name the third case, and give hal_common.c:47 the matching third branch. That is the same shape the existing comment at CMakeLists.txt:75-83 argues for — the build system decides, and the source does not decide again. |
| 5 | Low | hal/src/hal_rtos.c:441-447 |
rtos_get_tick_ms() returns GetTickCount() - host_start_tick, and host_start_tick is set only in rtos_hal_init(). Called before init, the Windows branch returns the machine's raw uptime in milliseconds where the POSIX branch returns 0 — two implementations of one function with different pre-init behaviour, and neither consults hal_initialized. Separately, GetTickCount() resolution is the system timer period, typically 15.6 ms, which is coarse for the eos_hal_init() timing test this change exists to unblock; a "delay 10 ms and observe the tick advance" assertion can land on either side of one tick. |
Return 0 unless hal_initialized, matching the other branch. If the timing test needs finer than 15 ms, use QueryPerformanceCounter scaled to milliseconds. |
| 6 | Low | tests/test_pkg_trust_anchor.c:130-142 |
The POSIX 0600 creation is correctly preserved, but the comment explaining why it exists was deleted: "Create with 0600 rather than fopen()'s 0666 & ~umask. These fixtures are the signed packages the verifier is about to trust, so a world-writable one lets anything on the box rewrite the payload between the write here and the read under test." Nothing now records either that reasoning or the fact that the Windows branch does not have the protection. The next reader has no reason not to simplify the POSIX branch to fopen for symmetry — which is how the original defect returns. |
Restore the comment above the #ifdef, and add one sentence: on Windows the fixture inherits the containing directory's ACL, which is weaker, and is accepted because the fixtures are created in the test's own build tree. |
| 7 | Low | core/src/log.c:12-19, hal/src/hal_rtos.c:15-24, tests/test_board_configs.c:16-23; requirements-dev.txt:2 |
The #pragma warning(push) / disable: 5105 / pop wrapper around <windows.h> is copy-pasted in three files and will be needed in the fourth. One include/eos/eos_windows.h that does the wrapping and is included instead would remove the repetition and give the suppression a single place to be explained — C5105 is a Windows SDK header issue under /Zc:preprocessor, which is worth writing down once rather than three times. Also requirements-dev.txt has no trailing newline. |
A one-file wrapper header, and a newline. |
What I checked that looks worse in the diff than it is
- The stub-crypto restructure is behaviour-preserving and stays fail-closed.
EOS_STUB_CRYPTO_REFUSEis defined atservices/crypto/src/rsa_ecc_sha512.c:28-41as((void)0)whenEOS_ALLOW_STUB_CRYPTOis defined and asfprintf(stderr, …); return -1;otherwise. Wrapping each stub body in#elsetherefore removes only code that was already unreachable. The condition is now spelled in five places instead of one, which normally worries me — but both spellings test the same symbol, and the failure direction of a typo is safe: a mistyped#ifndefselects the stub body and leaves the refusing macro in front of it, so the function still returns −1.tests/test_crypto_failclosed.cexercises all four entry points (:55, :70, :72, :87, :89, :110, :112) and runs inctest. No finding. - Removing
#include <unistd.h>fromtests/test_config.cis safe. Grepped the file forclose(,unlink(,access(,read(,write(,ftruncate,getpid,usleep— the only matches arefclose, which is<stdio.h>. It was an unused include. AND UNIXon bothCMakeLists.txtconditions is the right predicate. CMake'sUNIXreflects the target system, so a Linux cross build still sets it and a bare-metalGenericbuild does not — which preserves the intent recorded in the comments at:75-83and:194-197while fixing the Windows case. Thetests/CMakeLists.txt:204change to thetest_netguard matches the library's own condition, which is what the comment there requires.- The
test_halhang has the cause the body gives.rtos_delay_msspins onsystick_ms, which onlyeos_hal_systick_handler()advances, and nothing calls that on a host — so a Windows host build routed tohal_rtos.cwaits forever. Finding 4 is about where the fix belongs, not whether the diagnosis is right.
Architecture conformance
§21 / §5.1 — no tier is crossed. services/pkg is Tier 4 (Registry/eApps, §21), hal/ and services/crypto are Tier 1/2, tests/ and CONTRIBUTING.md are not runtime. No new #include, link line or target_link_libraries entry points up a tier; the diff adds only system headers and platform guards.
§23.2 Compatibility contract — finding 1 sits directly on it. "Package format — Versioned .epkg/.eapp metadata schema" is one of the seven contracts §23.2 enumerates. A struct written to disk with fwrite(&h, sizeof(h), …) is that schema's binary form, and this PR is the moment it acquires a second producer toolchain. The design versions the schema and says nothing about pinning its layout, which is a gap I have raised as a proposal — see .ai/autoreview/proposals/2026-09.md.
§11 / .ai/platform.md, "artifact provenance is verifiable end to end" — the same finding, from the consuming side. signature[64] and hash[32] live at offsets 205 and 269 of a layout no check pins. A cross-toolchain layout drift presents to the verifier as a bad signature, which is the least diagnosable failure this format can produce.
.ai/architect.md, "do not restructure and change behaviour in the same commit" — finding 4 is the instance. Adding Windows host timing is a behaviour change; deciding that it lives in the RTOS backend is a structural one.
Weakened checks — one, and it is finding 3. Nothing is disabled, no assertion is removed and no lint is loosened; the risk is that the MSVC assertion-enabling path can quietly not work. Finding 6 removes a comment rather than a check. The Windows fixture permission is a genuine widening, but it is a test fixture in the build tree and there is no portable alternative in standard C, so it is Low rather than a §4 finding — the omission worth fixing is that it is now unrecorded.
§9.2, "actionable diagnostics" and CONTRIBUTING.md — the .venv and requirements-dev.txt addition is a straightforward improvement and matches §25.2's "one-command environment diagnosis" direction. The winget line is fine as an example; it should not become the only documented route, and it is not.
Proposed changes
- Finding 1 first. It is four lines of
_Static_assertand it is the only thing in this PR that can corrupt an artifact rather than a build. Do it before the packing change lands, not after. - Finding 2 — ask a maintainer to approve the workflow runs, then read them. If
Cross-compile ARM Cortex-M4andBuild & Test (Linux x86_64)are green, findings 3–7 are all small. - Finding 3 — the
#errorguard. It should be added even if the string replace is kept, because it is what makes the replace's success observable. - Finding 4 —
hal_win32.c. This is the largest of the changes suggested here and it is the one I would be most willing to see deferred to a follow-up, provided the deferral is written down inhal_rtos.cbeside the_WIN32branches. What should not happen is that the branches stay and the reason is forgotten. - Findings 5–7 — small, independent, any order.
- Nothing here needs to wait on another PR. The
test_crypto_ed25519_loworderregistration failure the body mentions is #127's and is correctly excluded.
I opened no fix PR. Finding 1 is High and the fix is small, but the header is inside this PR's own diff, so a branch cut from master would conflict with it — and with no CI having run I have no way to verify a fix here in the way the brief requires.
Not checked
- Nothing in this PR was built or run. No CI has executed on this head and I did not build the branch locally; the eos checkout is on another branch and I did not want a second worktree for a change this wide. Every finding above is from reading the diff against the files it modifies, except finding 1's numbers.
- Finding 1's layout figures are GCC 15.2.0 on x86-64 only.
sizeof(eapp_header_t) == 303and the offsets were measured by compiling a probe againstservices/pkg/eos_pkg.hatorigin/master;package_id=69isnameplusEAPP_MAX_NAME, derived rather than printed. I did not measure the MSVC layout — I have no MSVC here, so I cannot say whether it currently agrees. The finding is that nothing checks; it is not a claim that it currently differs. - The Windows build was not reproduced. MSVC 19.29, the
wingetDoxygen route, the 38/38ctestfigure and the warning scan are the author's; I have no way to confirm or contradict them, and they are stated clearly enough to be checkable by someone who does. - The
boolwidth concern is general, not measured.sizeof(bool) == 1on this host; MSVC also uses 1 for_Bool/boolin C. The point is that neither is guaranteed by the standard for a struct that goes to disk, not that a divergence exists today. services/linux/src/linux_security.c:529— the added(void)bb;is in an#elsebranch I did not read the surrounding condition for. It suppresses an unused-parameter warning and cannot change behaviour, but I did not confirm which platforms take that branch.- No fuzzing or negative testing of the
.eappparser.services/pkgreads an attacker-supplied header;.ai/security.mdrequires fuzz coverage for externally reachable parsers and there is no harness for this one. Pre-existing, not this PR's to fix, and worth recording next to finding 1 because a layout change is exactly the kind of thing a parser fuzz corpus would catch on the reading side.
Automated architecture review of 6229c2f879ce — 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 I’ve updated the PR based on the review feedback:
Could you please approve the CI workflow so the test suite can run? |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#132 "fix(windows): make contributor build portable across MSVC and POSIX hosts"
head: 2afc9c2 author: abhishekkapoorx ci: none reported
Verdict: A real portability fix that also repairs a latent ARM link failure on
master; conforms to §5.1 and §21. Three things to address: an unrelated behaviour
change to the RTOS tick source, a new Windows code path that no merge-gating check
compiles, and a security rationale in a test fixture rewritten from a requirement into
an assertion.
What was verified locally
Built from 2afc9c2a (confirmed based on current origin/master feee2726, 0 commits
behind):
| Check | Command | Result |
|---|---|---|
| Host build + tests | cmake -B build -G Ninja -DEOS_BUILD_TESTS=ON -DEOS_PRODUCT=vbox_test && cmake --build build && ctest --test-dir build --no-tests=error |
PASS — 38/38 |
| Host Release (assertion guard) | same with -DCMAKE_BUILD_TYPE=Release |
PASS — 38/38, tests/assertions_enabled.h does not fire, so assert() really stays live |
| ARM cross build | -DCMAKE_TOOLCHAIN_FILE=toolchains/arm-cortex-m4.cmake -DEOS_BUILD_TESTS=OFF |
PASS — 142 warnings, identical count to origin/master; hal_rtos.c adds none |
eapp_header_t wire asserts |
compiled the header standalone, GCC 14, x86-64, C11 | PASS — sizeof == 303 and all nine offsets match the values asserted |
The HAL selection fix is not cosmetic. On origin/master the ARM Cortex-M4 build
compiles hal_linux.c (EOS_PLATFORM defaults to linux and
toolchains/arm-cortex-m4.cmake does not override it), which is an empty translation
unit for arm-none-eabi, while hal_common.c falls through to eos_hal_rtos_register().
Linking a Cortex-M4 executable against that library:
master: hal_common.c:(.text.eos_hal_init+0x1e): undefined reference to `eos_hal_rtos_register'
pr132 : LINK OK
CI does not catch this today because ci.yml's ARM job builds static libraries only and
nothing links them. This PR fixes it by making the EOS_HAL_BACKEND_* definition the
single source of truth for the fallback in hal_common.c:47 — which is exactly the fix
the comment at CMakeLists.txt:74-82 asks for.
The crypto change in services/crypto/src/rsa_ecc_sha512.c is behaviour-preserving:
EOS_STUB_CRYPTO_REFUSE already expanded to ((void)0) under EOS_ALLOW_STUB_CRYPTO
and to return -1 otherwise, so the #ifndef/#else split reaches the same code in
both configurations. test_crypto_failclosed still passes. No fail-closed regression.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | hal/src/hal_rtos.c:424 |
rtos_get_tick_ms() gains a hal_initialized ? systick_ms : 0 guard. This is a behaviour change on the bare-metal target, not a portability fix — hal_rtos.c is never compiled for Windows (CMakeLists.txt:86 routes WIN32 to hal_win32.c). It is not in the PR's "Changes" list, and the checklist states "no new behavior was added". It also has no upside here: systick_ms is a file-static already zero before init and re-zeroed at hal_rtos.c:429, so the guard only changes the post-eos_hal_deinit() path, where the getter now reports a frozen clock while eos_hal_systick_handler() keeps incrementing. The internal timeout loops at hal_rtos.c:192-195 and :420-421 read systick_ms directly, so the guard does not protect them either. |
Revert hal/src/hal_rtos.c to static uint32_t rtos_get_tick_ms(void) { return systick_ms; } and keep the guard only in hal_win32.c:194, where it is genuinely required because host_start_tick is indeterminate before win32_hal_init(). If the deinit semantics are wanted, land them separately with a test that states which behaviour is correct. |
| 2 | Medium | .github/workflows/ci.yml:29,118 |
The 95 new lines of hal/src/hal_win32.c, the MSVC branches in tests/CMakeLists.txt:18-24 and the #pragma pack path in services/pkg/eos_pkg.h:92-97 are compiled by no check that gates a merge. ci.yml runs Linux x86-64 and an ARM cross build only; checks.txt for this head is empty. The one windows-latest build is nightly.yml:23, which is schedule-driven, configures without -DEOS_PRODUCT=vbox_test (CONTRIBUTING.md states that is required alongside EOS_BUILD_TESTS=ON), and runs ctest --test-dir build --output-on-failure -C Release with no --no-tests=error — so it reports green on a run that collected nothing. The Windows evidence in the PR body is therefore unreproducible and will not stay true. Both nightly defects predate this PR. |
Add windows-latest to the test job in ci.yml so the MSVC path is built and ctest-ed on every PR, and add --no-tests=error plus -DEOS_PRODUCT=vbox_test to nightly.yml:32-38. |
| 3 | Low | tests/test_pkg_trust_anchor.c:135-148 |
The fixture's 0600 creation is dropped on Windows in favour of fopen(path,"wb"), and the rationale is rewritten from a stated requirement ("a world-writable one lets anything on the box rewrite the payload between the write here and the read under test") to an assertion ("Windows inherits the test build directory's ACL; that is acceptable here"). Nothing checks that ACL, and the build directory's location is the contributor's choice. These fixtures are the signed packages the verifier under test is about to trust. |
Either restrict the handle on Windows — `_sopen_s(&fd, path, _O_CREAT |
| 4 | Low | .github/workflows/python-tests.yml:20, CONTRIBUTING.md:29-32 |
Adding requirements-dev.txt makes the CI comment "this repository has neither requirements.txt nor …" false; the install step at line 28 already picks the file up, so only the comment is wrong. Separately, the quick start now installs pytest/pytest-cov but the command sequence that follows never runs pytest, and both pins are open-ended (pytest>=7) in a repo whose SDK rules call for reproducible lockfiles (§9.2). |
Delete the stale comment in python-tests.yml; add the python -m pytest invocation to CONTRIBUTING.md so the install has a purpose; pin upper bounds or state that these are intentionally floating. |
Architecture conformance
Conforms.
- §5.1 architectural law. No dependency points up a tier.
hal/src/hal_win32.c
includes only<eos/hal.h>and the new<eos/eos_windows.h>, which wraps
<windows.h>— a host runtime abstraction, which is what §5.1 permits EoS to depend
on.hal/include/eos/hal.h:66declareseos_hal_win32_register()unconditionally,
matching the existing convention documented athal.h:45-52. Nothing in the diff
references eAI, eNI, eBrowser, eOffice, health or aerospace. - §21 tier placement.
eosis Tier 1 — Foundation. A host HAL backend sits beside
the existinghal_linux.cinhal/, which is where.ai/architect.md's target
layout puts hardware abstraction; there are no board specifics in it, soboards/is
correctly untouched. §21.1 is respected: no new repository was created. - §14.1 / fail-closed. The crypto refactor keeps both configurations identical, as
shown above. Nothing widens the stub-crypto opt-in. - §9.2 / STANDARDS.md. Commit messages follow Conventional Commits. The one
standards gap is the unpinned dev requirements (finding 4).
The direction of hal_common.c's change — deriving the runtime fallback from the macro
CMake set rather than from a host predefine — is the correct fix and removes a
disagreement the file's own comment had already flagged.
Proposed changes
- Revert
hal/src/hal_rtos.cto itsmasterform (finding 1). One line; the rest of
the PR is unaffected. - In
ci.yml, extend thetestjob to a matrix over
[ubuntu-22.04, windows-latest], keeping-DEOS_BUILD_TESTS=ON -DEOS_PRODUCT=vbox_test
andctest ... --no-tests=error. Without this,hal_win32.chas no guard against
bit-rot from the day it lands. - In
nightly.yml:32-38, add-DEOS_PRODUCT=vbox_testto the configure step and
--no-tests=errorto thecteststep. - Fix the Windows fixture permission comment or the permission itself (finding 3).
- Drop the stale
python-tests.ymlcomment and add thepytestinvocation to
CONTRIBUTING.md.
Items 2 and 3 are CI-only and can land separately from this PR; items 1, 3 and 5 belong
in it.
Not checked
- Everything Windows-specific. No Windows host was available. The MSVC
/FI
forced include, the/DNDEBUGstrip intests/CMakeLists.txt:19-23, the
#pragma pack(push,1)layout,__declspec(align(8)),Sleep()/GetTickCount64(),
the C5105 suppression, and the "38/38 tests passed" and "nowarning Cdiagnostics"
claims in the PR body are all unverified here. Theeapp_header_toffsets were
verified only under GCC on x86-64 — the static asserts are precisely what would catch
an MSVC divergence, but nobody has run them under MSVC in CI. eapp_header_ton a big-endian or 32-bit target. Only x86-64 was checked.
_Static_assert(sizeof(bool) == 1)was verified for GCC only.- Whether any
.eappfile already in circulation matches the pinned 303-byte
layout. The asserts pin the layout to whatmastercompiles today; no existing
artifact was decoded to confirm that is also what was shipped. - The Python suite. The PR body reports "14 passed, 1 known registration failure"
and attributes the failure to PR #127; neither the suite nor #127 was run here. services/linux/src/linux_security.c:529. The(void)bb;addition is trivially
correct, but the surroundingsystem(cmd)call it sits next to was not reviewed —
it is outside this diff.
Automated architecture review of 2afc9c2a7471 — 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.
- add ` -DEOS_PRODUCT=vbox_test` during config - add `--no-tests=error` to test step
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#132 "fix(windows): make contributor build portable across MSVC and POSIX hosts"
head: 34812f2 author: abhishekkapoorx ci: not run — every workflow on this branch is action_required
Verdict: Follow-up review. All four findings from the previous report are answered:
three are fully resolved and one — "no merge-gating check compiles the Windows path" —
is half resolved. The nightly half is fixed correctly; the new windows-test job is the
right shape and, contrary to what I would have guessed, the Visual Studio 18 2026
generator in 34812f2 is the correct one for windows-latest today. What is not true
is that it gates anything, and it has still never executed. Host and ARM builds at this
head are green here.
Status of the previous findings (eos-132-2afc9c2a.md)
| Prev # | Status | Evidence |
|---|---|---|
1 — hal_rtos.c:424 tick guard is an unrelated behaviour change |
Resolved in 1f6fe04 |
git show 34812f2c:hal/src/hal_rtos.c line 424 is static uint32_t rtos_get_tick_ms(void) { return systick_ms; }, byte-identical to origin/master (feee272) line 424. The hal_initialized guard survives only in hal/src/hal_win32.c:64, where it is required. |
2 — no merge-gating check compiles the Windows path; nightly.yml reports green on a run that collected nothing |
Partially addressed | nightly.yml fully fixed in 27b5423: :27 now configures -DEOS_PRODUCT=vbox_test and :33 runs ctest … --no-tests=error. The ci.yml half is not: see findings 1 and 2 below. |
| 3 — Windows fixture comment asserts an ACL nothing checks | Resolved in a16d015 |
tests/test_pkg_trust_anchor.c:135-139 no longer claims the inherited ACL "is acceptable"; it states that Windows has no portable CRT equivalent and scopes the test to package verification. That is the second of the two options offered. |
4 — stale python-tests.yml comment, pytest installed but never invoked, open-ended pins |
Resolved in c351e8a + 0896052 |
Comment gone from python-tests.yml:20; CONTRIBUTING.md:36 now runs python -m pytest tests/ -v; requirements-dev.txt is pytest>=7,<9 / pytest-cov>=4,<7. |
What was verified locally at this head
Built from a detached worktree at 34812f2c (0 commits behind origin/master feee272,
20 ahead; mergeable: MERGEABLE, no conflict):
| Check | Command | Result |
|---|---|---|
| Host Release build | cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DEOS_BUILD_TESTS=ON -DEOS_PRODUCT=vbox_test && cmake --build build --parallel 4 |
PASS — 95 warnings |
| Host tests | ctest --test-dir build --output-on-failure --no-tests=error --timeout 120 |
PASS — 38/38 |
| ARM Cortex-M4 cross build | -DCMAKE_TOOLCHAIN_FILE=toolchains/arm-cortex-m4.cmake -DEOS_BUILD_TESTS=OFF |
PASS — 81 warnings |
ARM warning parity vs origin/master |
same command on feee272 |
PASS — 81 on both; this PR adds none |
ci.yml YAML validity |
yaml.safe_load |
PASS — jobs test, windows-test, default-config, build-arm, static-analysis, release |
cmake_minimum_required vs the runner's CMake 4.4.2 |
git grep cmake_minimum_required |
PASS — lowest is layers/ai/CMakeLists.txt:1 at 3.15; CMake 4 only rejects <3.5 |
The generator fix in 34812f2 is correct, and worth saying so because it could not be
tested. windows-latest now resolves to the Windows Server 2025 VS 2026 image
(actions/runner-images README, label windows-2025-vs2026), which carries Visual
Studio Enterprise 2026 18.9.12112.369, CMake 4.4.2 and Ninja 1.13.2 — VS 2022 (v17) is
not on it. The Visual Studio 18 2026 generator was added in CMake 4.2 (CMake 4.4.3
docs), so 4.4.2 accepts it. The earlier Visual Studio 17 2022 string would have failed.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/ci.yml:82; PR body "Changes" |
"Required merge gate" is asserted, and no check is required in this repository. The comment above the job reads "MSVC is a required merge gate for the hosted Windows backend", and the PR body says "Add a required Windows MSVC build and CTest job to merge-gating CI". GET /repos/embeddedos-org/eos/branches/master/protection returns required_pull_request_reviews, required_linear_history, required_conversation_resolution and no required_status_checks key at all; the one active ruleset (19425690) is Copilot code review. Adding a job to ci.yml makes it visible, not required — a red Build & Test (Windows MSVC) does not block the merge button today. This matters because the sentence is precisely the reassurance a future reader will rely on when deciding the Windows path is protected. |
Either add Build & Test (Windows MSVC) to required_status_checks.contexts on master (a maintainer action, not a PR change), or reword the comment and the PR body to what is true: "runs on every PR; not currently in required_status_checks". Do not leave the claim standing on a repo that has no required checks. |
| 2 | Medium | .github/workflows/ci.yml:84-110; PR body "Testing" |
The only Windows evidence in this PR comes from a compiler two generations older than the one the new job will use, and the job has never run. The body records MSVC 19.29.30159 — that is Visual Studio 2019 (v16.11). The new gate targets windows-latest, i.e. MSVC v18.9 / VS 2026. Every MSVC-specific construct this PR adds meets v18 for the first time when the job first executes: #pragma pack(push,1) on eapp_header_t in services/pkg/eos_pkg.h:92-97 — the on-disk .eapp header whose signature and hash sit at offsets 205 and 269, and whose _Static_assert block exists exactly to catch a cross-toolchain divergence — plus __declspec(align(8)) in tests/test_e2e.c / tests/test_stress.c, the /FI forced include and the /DNDEBUG string strip at tests/CMakeLists.txt:18-27, and the C5105 suppression in include/eos/eos_windows.h:13. The body's "38/38 tests passed" and "no warning C or warning D diagnostics" are honest about being the author's Windows run, but they are not evidence about v18. The body is also now stale: it still says the job "is configured for windows-latest with Visual Studio 17 2022", which 34812f2 changed. Meanwhile gh api …/commits/34812f2c/check-runs returns total_count: 0 and every run on fix/windows-msvc-build-issue is action_required — so the 33 new CI lines are themselves untested. |
A maintainer approves the workflow runs; nothing here should be judged ready before Build & Test (Windows MSVC) has gone green once. Update the body's "Visual Studio 17 2022" line, and say which MSVC version produced the 38/38 rather than leaving "Windows" to imply the CI compiler. The _Static_assert block is the right guard for the packing risk — it just has not been compiled by MSVC yet, by anyone. |
| 3 | Low | .github/workflows/ci.yml:93-94 |
ilammy/msvc-dev-cmd@v1 is the repo's first GitHub Action from a personal account, on a mutable tag, and it is redundant here. Every other third-party action in .github/workflows/ comes from an organisation — actions/*, github/codeql-action, softprops/action-gh-release, anchore/sbom-action, sigstore/cosign-installer, ossf/scorecard-action@v2.4.3, anthropics/claude-code-action. A mutable @v1 on a single-maintainer account means a compromise of that account runs arbitrary code in this repo's CI; scorecard.yml runs OpenSSF Scorecard, whose Pinned-Dependencies check counts every unpinned action. It is also not needed: -G "Visual Studio 18 2026" makes CMake locate the toolset through vswhere itself, so the only consumer of the PATH this action sets is the cl smoke step on the next line. |
Drop both the Set up MSVC and Check MSVC steps and let the generator find the toolset, or — if the cl smoke check is wanted — pin the action to a full 40-char commit SHA with the tag in a trailing comment, as ADR-017 already requires of vendored source. |
| 4 | Low | .github/workflows/ci.yml:41, requirements-dev.txt |
The new upper bounds do not reach the job that runs the Python suite in CI — eos. The test job installs pip3 install pytest pytest-cov at ci.yml:41 — unpinned, and it never reads requirements-dev.txt — then runs python3 -m pytest tests/ at :73. Only python-tests.yml:25 installs the file. So pytest>=7,<9 constrains the contributor's machine and the Python Unit Tests workflow, but not CI — eos, which is the one whose name suggests it is authoritative. (python-tests.yml:23 also carries a pre-existing pip install pytest pytest-cov 2>/dev/null || true, a swallowed install failure that .ai/reviewer.md names as a finding in its own right — outside this diff, recorded because this PR is what gave the file a real dependency list.) |
Replace ci.yml:41's pip3 install pytest pytest-cov with pip3 install -r requirements-dev.txt so one file governs both jobs, and drop the || true in python-tests.yml:23 while the file is open. |
Architecture conformance
Conforms; nothing in the new commits touches the dependency graph. 1f6fe04 removes a
line, a16d015 and c351e8a change comments, 0896052 edits docs and a requirements
file, and 27b5423 / 9d13aff / 34812f2 are workflow YAML. No #include, link line,
target_link_libraries entry or manifest dependency was added or moved.
The conformance findings in the previous report stand unchanged and are not restated:
§5.1 holds (hal_win32.c includes only <eos/hal.h> and <eos/eos_windows.h>), §21
tier placement is correct for Tier 1 eos, §21.1 is respected, and §14.1 fail-closed
behaviour is preserved. The .eapp layout question under §23.2 is now pinned by the
_Static_assert block added earlier in this PR; finding 2 above is about that block not
yet having been compiled by the toolchain it exists to police, not about its correctness.
.ai/architect.md's "do not restructure and change behaviour in the same commit" is now
satisfied — that was the substance of previous finding 1, and 1f6fe04 settles it.
Proposed changes
- Finding 1 — one sentence in
ci.yml:82and one bullet in the PR body, or a
maintainer adding the context torequired_status_checks. Do this either way; a
false guarantee is worse than an absent one. - Finding 2 — a maintainer approves the workflow runs. This is the whole blocker on
this PR and has been since the first review; nothing the author can do moves it.
While waiting, correct the "Visual Studio 17 2022" line in the body and name the
MSVC version behind the 38/38. - Finding 3 — delete two steps, or pin to a SHA. Independent of everything else.
- Finding 4 — one line in
ci.yml:41. Independent.
None of these blocks the portability work itself, which is sound and now builds clean on
both platforms I can reach.
Blocked / stale
Blocked, not stale. Seven commits landed since the last review and they answer it
directly, so the author is not sitting still. The PR is MERGEABLE with no conflict and
0 commits behind master. mergeStateStatus: BLOCKED is reviewDecision: REVIEW_REQUIRED (protection requires one approving review), not a red check.
What unblocks it: a maintainer clicking "Approve and run workflows" on the
first-time-contributor gate. Until then Build & Test (Linux x86_64),
Cross-compile ARM Cortex-M4, Build & Test (Windows MSVC), Static Analysis, CodeQL,
Third-party policy and the Python suite have all said nothing about any of the twenty
commits on this branch. I have covered the first two locally; the Windows job, static
analysis and CodeQL remain uncovered by anybody.
Not checked
- Everything MSVC. No Windows host here. The new
windows-testjob, the
Visual Studio 18 2026configure,ilammy/msvc-dev-cmd@v1's behaviour on the VS 2026
image,#pragma pack,__declspec(align(8)), the/FIinclude, the/DNDEBUGstrip
and the C5105 suppression are all unexercised. My generator/CMake/VS-version statements
above come from theactions/runner-imagesand CMake documentation, not from a run. - The
_Static_assertlayout under MSVC. Verified under GCC on x86-64 only, in the
previous review. The point of finding 2 is that nobody has compiled it with v18. - The Python suite.
python3 -m pytest— NOT RUN,No module named pyteston this
host. The commandCONTRIBUTING.md:36now documents was not executed; I confirmed only
that it matches whatci.yml:73andpython-tests.yml:28run. - Static analysis and CodeQL. Not run locally and not run in CI on any head of this
branch. - Warning counts are this host's toolchain, not CI's. 95 host / 81 ARM here; the
previous review measured 142 on ARM with a different compiler. Only the
head-vs-masterparity on the same machine (81 == 81) is meaningful, and that is what
I claim. - Whether a maintainer intends
Build & Test (Windows MSVC)to become required.
Finding 1 reports the configuration as it is now; it does not assume the intent.
Automated architecture review of 34812f2ce025 — 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
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eos#132 "fix(windows): make contributor build portable across MSVC and POSIX hosts"
head: 9595bdb author: abhishekkapoorx ci: not run — 0 check-runs on this head; every workflow on the branch is action_required
Verdict: Follow-up review. One commit since the last report (9595bdb), touching
.github/workflows/ci.yml only. It closes both Low findings from eos-132-34812f2c.md
exactly as recommended, and introduces nothing. The two Medium findings are still open,
and both are outside the author's reach: the repository has no required status checks at
all, and no CI has executed on any of the twenty-one commits on this branch. Host and ARM
builds are green at this head here.
Status of the previous findings (eos-132-34812f2c.md)
| Prev # | Status | Evidence |
|---|---|---|
1 — Medium — ci.yml:82 and the PR body assert a "required merge gate" that does not exist |
Untouched | git show 9595bdbc:.github/workflows/ci.yml line 82 still reads "MSVC is a required merge gate for the hosted Windows backend"; the body still lists "Add a required Windows MSVC build and CTest job to merge-gating CI". Re-checked today: GET /repos/embeddedos-org/eos/branches/master/protection returns keys allow_deletions, allow_force_pushes, allow_fork_syncing, block_creations, enforce_admins, lock_branch, required_conversation_resolution, required_linear_history, required_pull_request_reviews, required_signatures, url — no required_status_checks key. The one active ruleset (19425690) is still Copilot code review. See finding 1 below. |
| 2 — Medium — the Windows evidence predates the compiler the job will use, and the job has never run | Partially addressed | The body-staleness half is fixed: it now says "configured for windows-latest with Visual Studio 18 2026", and it already named MSVC 19.29.30159 (VS 2019 v16.11) as the compiler behind the 38/38, so the version is no longer implicit. The substantive half is untouched and not the author's to fix: gh api …/commits/9595bdbc/check-runs returns total_count: 0, combined status pending with 0 statuses, and gh run list --branch fix/windows-msvc-build-issue shows all six workflows for this head (CI — eos, CodeQL, Python Unit Tests, Third-party policy, EoS Build & Test, EoS Full-Stack Simulation) completed / action_required at 2026-09-06T07:37:38Z. See finding 2 below. |
3 — Low — ilammy/msvc-dev-cmd@v1, first personal-account action, mutable tag, redundant |
Resolved in 9595bdb |
Both the Set up MSVC and Check MSVC steps are gone; windows-test is now checkout / Configure (MSVC) / Build / Run C unit tests. git grep -i msvc-dev-cmd 9595bdbc finds nothing. Every action in .github/workflows/ at this head is org-owned again: actions/*, github/codeql-action/*, anchore/sbom-action, sigstore/cosign-installer, softprops/action-gh-release, ossf/scorecard-action@v2.4.3, anthropics/claude-code-action. That is the first of the two options offered. |
4 — Low — requirements-dev.txt pins do not reach CI — eos; || true in python-tests.yml |
Partially addressed | The first half is resolved in 9595bdb: ci.yml:42 is now pip3 install -r requirements-dev.txt, so pytest>=7,<9 / pytest-cov>=4,<7 govern the test job as well as Python Unit Tests. The second half is untouched: python-tests.yml:23 still carries pip install pytest pytest-cov 2>/dev/null || true. It was flagged as pre-existing and outside the diff, so it is recorded here rather than restated as a new finding — but it is not fixed, and .ai/reviewer.md names a swallowed install failure as a finding in its own right. |
The new commit on its own merits
9595bdb is 1 insertion, 7 deletions, one file. I checked it for the ways a fix can
introduce a defect and found none:
- YAML still parses and the job graph is unchanged.
yaml.safe_loadon the head's
ci.ymlyieldstest, windows-test, default-config, build-arm, static-analysis, release— the same six as at34812f2c. - Dropping
msvc-dev-cmddoes not orphan a consumer. Its only consumer was the
clsmoke step removed in the same commit;-G "Visual Studio 18 2026"makes CMake
locate the toolset throughvswhere, so theConfigure (MSVC)andBuildsteps do
not read the PATH that action set. Not executable here — see Not checked. requirements-dev.txtis reachable from thetestjob. It is at the repo root,
actions/checkout@v4puts the job's cwd there, and the file content
(pytest>=7,<9,pytest-cov>=4,<7) covers everythingci.yml:77-79invokes,
including--cov-report=xml, which needspytest-cov.ubuntu-22.04ships no
EXTERNALLY-MANAGEDmarker, so the mechanism is exactly the one the removed
pip3 install pytest pytest-covalready relied on — no new PEP 668 exposure.pytest tests/still collects. Notests/test_*.pyat the top level, but seven
modules undertests/unit,tests/functional,tests/simulation,
tests/performance, so the job cannot hit pytest's exit-5 "no tests collected".
No source file changed. git diff --name-only 34812f2c 9595bdbc is .github/workflows/ci.yml
alone, so the C/CMake review carried out at 34812f2c and 2afc9c2a stands unchanged and
is not restated.
What was verified locally at this head
Detached worktree at 9595bdbc (0 commits behind origin/master feee272, 21 ahead;
mergeable: MERGEABLE):
| Check | Command | Result |
|---|---|---|
| Host Release build | cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DEOS_BUILD_TESTS=ON -DEOS_PRODUCT=vbox_test && cmake --build build --parallel 4 |
PASS — 95 warnings, identical count to 34812f2c |
| Host tests | ctest --test-dir build --output-on-failure --no-tests=error --timeout 120 --parallel 4 |
PASS — 38/38, 0.51 s |
| ARM Cortex-M4 cross build | cmake -B build-arm -G Ninja -DCMAKE_TOOLCHAIN_FILE=toolchains/arm-cortex-m4.cmake -DEOS_BUILD_TESTS=OFF && cmake --build build-arm --parallel 4 |
PASS — 81 warnings, identical count to 34812f2c |
ci.yml YAML validity |
yaml.safe_load |
PASS |
| Branch protection | gh api …/branches/master/protection, gh api …/rulesets |
no required_status_checks; one Copilot-review ruleset |
| CI state at this head | gh api …/commits/9595bdbc/check-runs, …/status, gh run list |
total_count: 0; status pending, 0 statuses; six workflows action_required |
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/ci.yml:82; PR body "Changes" |
Carried from the previous review, unchanged: "required merge gate" is asserted and nothing in this repository is required. Verified again at this head — master's protection has no required_status_checks key at all, so a red Build & Test (Windows MSVC) does not block the merge button. 9595bdb edited this very file and left the sentence standing. It is the reassurance a future reader will rely on when deciding the Windows path is protected, and it is false. |
One sentence. Either a maintainer adds Build & Test (Windows MSVC) to required_status_checks.contexts on master — a repo-settings action, not a PR change — or ci.yml:82 and the body bullet are reworded to "runs on every PR; not currently in required_status_checks". Do not leave the claim standing either way. |
| 2 | Medium | whole PR | Carried from the previous review, unchanged and still the blocker: nothing in this PR has been compiled by CI, by anybody, at any head. Twenty-one commits, six workflows, total_count: 0 check-runs. Everything MSVC-specific this PR adds — #pragma pack(push,1) on the on-disk eapp_header_t (services/pkg/eos_pkg.h:92-97, whose signature and hash sit at offsets 205 and 269), __declspec(align(8)) in tests/test_e2e.c and tests/test_stress.c, the /FI forced include and /DNDEBUG strip at tests/CMakeLists.txt:18-27, the C5105 suppression in include/eos/eos_windows.h:13, and now the generator-only toolset discovery left behind by 9595bdb — meets MSVC v18 for the first time when the job first executes. The only Windows evidence in the body is the author's MSVC 19.29.30159, two major versions older than the windows-latest image the job targets. Static Analysis, CodeQL, Third-party policy and the Python suite have said nothing about any commit on this branch either; I have covered the Linux host and ARM cross builds locally, and nobody has covered the rest. |
A maintainer clicks "Approve and run workflows" on the first-time-contributor gate. Nothing here should be judged ready before Build & Test (Windows MSVC) has gone green once — the _Static_assert block in eos_pkg.h is the right guard for the packing risk, and its whole purpose is to be compiled by the toolchain it polices. |
Both were Medium in the previous report and stay Medium; neither is rounded down and
neither is escalated for age. Findings 3 and 4(a) of the previous report are resolved and
are not restated. Finding 4(b) — the \|\| true at python-tests.yml:23 — is recorded in
the status table as untouched rather than reopened as its own row, because it is
pre-existing and outside this diff; it is still worth one line while the file is open.
No new findings. 9595bdb is a clean, correctly-scoped commit that does what the last
review asked and nothing else.
Architecture conformance
Conforms. 9595bdb is workflow YAML: no #include, link line, target_link_libraries
entry or manifest dependency was added, moved or removed, and no source file changed at
all. §5.1 dependency direction, §21 Tier 1 placement for eos, §21.1, §14.1 fail-closed
behaviour and the §23.2 .eapp package-format contract are all exactly as assessed at
34812f2c and 2afc9c2a, and are not re-argued here.
The §23.2 gap that finding 1 of the first review sat on — the design versions the package
schema but says nothing about pinning its binary layout — is already recorded as a
proposal in .ai/autoreview/proposals/2026-09.md. This head adds nothing new to it, so no
proposal is appended for this review.
Proposed changes
- Finding 1 — one sentence in
ci.yml:82and one bullet in the PR body, or a maintainer
adding the context torequired_status_checks. This is the only thing on the list the
author can act on today, and it has been open across three reviews. - Finding 2 — a maintainer approves the workflow runs. This is the whole blocker and has
been since the first review; nothing the author does moves it. - Optional, one line while
python-tests.ymlis open — drop the2>/dev/null || true
at:23. Lines 24-25 already installrequirements-dev.txtconditionally, so the
swallowed install adds nothing but a way to reach the test step with no pytest.
The portability work itself remains sound and builds clean on both platforms I can reach.
Blocked / stale
Blocked, and now close to stale. One commit in the two days since the last review, and
it addresses only the two Low findings; the Medium ones are untouched. That is not the
author stalling — finding 2 is a maintainer action, and finding 1 is arguably one too. The
PR is MERGEABLE, 0 commits behind master, no conflict. mergeStateStatus: BLOCKED
resolves to reviewDecision: REVIEW_REQUIRED (protection requires one approving review),
not to a red check — there are no checks to be red.
What unblocks it: a maintainer approving the workflow runs, and then a decision on
whether Build & Test (Windows MSVC) is meant to be required. Until the first happens,
every claim about the Windows path in this PR rests on one contributor's VS 2019 machine.
Not checked
- Everything MSVC. No Windows host here. The
Visual Studio 18 2026configure, the
now-unassisted toolset discovery after9595bdbremovedmsvc-dev-cmd,#pragma pack,
__declspec(align(8)), the/FIinclude, the/DNDEBUGstrip and the C5105
suppression are all unexercised. That CMake's VS generator locates the toolset without
the action is from the CMake documentation, not from a run. - The
_Static_assertlayout under MSVC. Verified under GCC on x86-64 in the
2afc9c2areview only. Nobody has compiled it with v18. That is the substance of
finding 2, not a claim that the layouts differ. - The Python suite.
python3 -m pytest— NOT RUN,No module named pyteston
this host. I confirmed by inspection only thatrequirements-dev.txtcovers what
ci.yml:77-79invokes and thattests/contains collectable modules; I did not execute
the install or the suite. The body's "14 passed, 1 known registration failure" is
unconfirmed here. - Static analysis and CodeQL. Not run locally and not run in CI on any head of this
branch. pip3 install -r requirements-dev.txton the runner. Not executed. The PEP 668
statement is from theubuntu-22.04image's Python packaging, not from a run.- Warning counts are this host's toolchain, not CI's. 95 host / 81 ARM. Only the
parity against the previously measured34812f2con the same machine is meaningful,
and that is what I claim. - Whether a maintainer intends
Build & Test (Windows MSVC)to become required.
Finding 1 reports the configuration as it is; it does not assume intent.
Automated architecture review of 9595bdbcca13 — 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.
Re-registering it put it in front of the MSVC leg (embeddedos-org#132) for the first time, where <unistd.h> does not exist. The suite probes with access(), open(O_CREAT) and rmdir to prove refused inputs never reach a shell; on Windows the library's own #else arm refuses every install/build call before is_path_safe() matters, so there is nothing for it to test there.
Nine PRs were merged into master within minutes on 09-08, each on the base it was written against. Master has not compiled since, and the Python guards that would have named the rest never ran because the C build failed first. Compile (every C job, CodeQL, and the ARM64 kernel in the simulation): - services/linux/src/linux_security.c: embeddedos-org#119 and embeddedos-org#132 each added an #else to the same #ifndef _WIN32 in eos_busybox_install_to_rootfs(), so master has "#else after #else". The embeddedos-org#132 arm (`(void)bb;`) is the one removed -- the embeddedos-org#119 arm already uses bb and reports the unsupported platform. Guards from embeddedos-org#121 / embeddedos-org#93 that later merges walked back: - ci.yml: embeddedos-org#132 added windows-test after embeddedos-org#121's gate; the gate did not wait for it, so "CI Gate" could be green with the MSVC leg red. - test_ci_gate.py: embeddedos-org#129 gave eosim-sanity.yml and simulation-test.yml a path-filtered pull_request trigger (they test their own edits); a path-filtered check cannot be required, so both are recorded in NOT_REQUIRED with the book-build.yml reason. - tests/CMakeLists.txt: test_linux_security_paths (embeddedos-org#119) and test_pkg_fetch (embeddedos-org#115) had no add_executable(). embeddedos-org#115's replay replaced embeddedos-org#119's block with its own, and embeddedos-org#118's replay replaced that; two suites compiled against nothing. Both registered again. 41 -> 43 suites. - tests/test_kernel.c: four tests from embeddedos-org#130 and embeddedos-org#131 were defined and never called -- their RUN() lines did not survive the replay of main(). And the one that was not bookkeeping: - kernel/src/task.c: embeddedos-org#130 was merged after embeddedos-org#131 from a base that predates it, and its copy of task.c replaced embeddedos-org#131's. embeddedos-org#131 had also flattened 574 CRLF line endings, so its 1166-line diff hid a 39/21 change and the replay took embeddedos-org#130's side wholesale. Master kept embeddedos-org#131's tests and lost its kernel: the idle task could be deleted and suspended, a half-initialised TCB was published to the scheduler before its stack existed, and eos_schedule() pointed g_current_sp at the outgoing task. test_idle_task_is_permanent fails on master the moment it is called. embeddedos-org#131's task.c diff re-applied on top of embeddedos-org#130's; the result is embeddedos-org#131's file plus embeddedos-org#130's wake_armed hunk and nothing else (verified by diff against 3a00bd9). Verified locally (macOS, clang): Release build clean, 43/43 ctest; the README default configuration builds; 47/47 pytest. Not in this PR: the nightly "Upstream drift" job builds eBoot at master and eBoot master is broken separately (its own repair PR); bump EBOOT_COMMIT in eos-simulation.yml once that lands.
Re-registering it put it in front of the MSVC leg (embeddedos-org#132) for the first time, where <unistd.h> does not exist. The suite probes with access(), open(O_CREAT) and rmdir to prove refused inputs never reach a shell; on Windows the library's own #else arm refuses every install/build call before is_path_safe() matters, so there is nothing for it to test there.
Summary
This PR fixes Windows/MSVC portability problems found when following the
contributor setup in
CONTRIBUTING.md. It also documents a repository-localPython test environment.
Type of Change
Changes
eapp_header_twhile preserving GCC/Clangpacking.
runtime on Windows.
Sleep()andGetTickCount()so the fallbackHAL does not hang in
eos_hal_init()timing tests.0600; use the standard-Cfopen()path on Windows and document that inherited ACLs are not tested.test_e2eandtest_stress.requirements-dev.txtand document.venvsetup forpytestandpytest-cov.EOS_PRODUCT=vbox_testand make CTestfail when no tests are discovered.
isolated to the Windows backend.
to the contributor quick start, and bound the development dependency versions.
headers, and Windows-only unused parameters.
configurations without conflicting
/DNDEBUGand/UNDEBUGoptions.Testing
ctest --test-dir build/windows-assessment --build-config Release --output-on-failure --no-tests=error --timeout 120)host test execution, not an integration workflow.
contributor setup with MSVC 19.29.30159.
change remains in the RTOS backend.
Results:
warning Corwarning Ddiagnostics in the clean WindowsRelease build.
test_halbuild: passed without diagnostics.Follow-up validation for the review fixes:
git diff --check: passed.is configured for
windows-latestwith Visual Studio 18 2026.its tests were not rerun locally.
Pre-Submission Checklist
build (MSVC equivalent of the
-Wall -Wextra -Werrorrequirement); theRelWithDebInfoHAL target was also verified without diagnostics.new functionality and updates existing portability coverage.
type(scope): descriptionconventionorigin/masterRelated Issues
No issue number identified. The separate missing Ed25519 test-registration
problem is covered by upstream PR #127 and is intentionally excluded from this
PR.
Screenshots / Logs
Relevant original Windows build failures included:
After the changes:
Additional Notes
Windows uses the generic fallback HAL because the existing Linux HAL relies on
POSIX APIs. This PR does not add Windows hardware-peripheral support.
The Python registration test still reports that
tests/test_crypto_ed25519_loworder.cis not registered intests/CMakeLists.txt; that independent change is already addressed byupstream PR #127.