ci: install EoSim from a tag that exists, not a wheel that never shipped - #81
ci: install EoSim from a tag that exists, not a wheel that never shipped#81Kartikey1306 wants to merge 2 commits into
Conversation
d84718c to
990d713
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
e7485fb to
3340494
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#81 "ci: install EoSim from a tag that exists, not a wheel that never shipped"
head: 3340494 author: Kartikey1306 ci: 24 checks pass — none of which run the changed file
Verdict: The 404 diagnosis is right and the install now works. But the workflow it
revives cannot fail for the reasons it claims to check: three of its five jobs validate
zero platforms, one still clones unpinned, and the gate ignores four of its five inputs.
Fixing the install without fixing those turns "red every morning" into "green every
morning, having checked nothing."
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/eosim-sanity.yml:42-43 |
The step named "Validate all platform configs" runs eosim list && eosim doctor with no platforms/ copy in that job. By the PR's own caveat, eosim list prints Available platforms (0) without the copy — and exits 0. So the step passes having validated nothing, and install-validate is the only job the gate can fail on (finding 4). Same shape in windows-sanity:122-125 and macos-sanity:138-141. |
Copy platforms/ from the same checkout in these jobs, as nested-simulation now does, and assert a non-zero count rather than trusting the exit code — e.g. python -c "from eosim... import discover_platforms; assert len(discover_platforms(...)) > 0". |
| 2 | Medium | .github/workflows/eosim-sanity.yml:103 |
nested-guest-install still does git clone --depth 1 https://github.com/embeddedos-org/EoSim.git /tmp/eosim-data — no --branch. That job installs the package from v1.5.0 and then copies platforms/ from EoSim's default branch. The PR body says "the platforms/ copy … now sources from the same checkout rather than a second unpinned clone"; that is true for nested-simulation only, not for this job. |
Reuse /tmp/EoSim here as the other job does, and delete the second clone. |
| 3 | Medium | .github/workflows/eosim-sanity.yml:12 |
EOSIM_VERSION: "1.5.0", but the PR's own verification block shows eosim --version -> eosim, version 2.0.0. The tag and the package's declared version disagree, so pinning to v1.5.0 does not pin what the name suggests, and the Verify installation step (:38-41) prints the version without asserting it. This is the kind of drift master design §9.2 ("Reproducible lockfiles/manifests for production builds") and §23.2 exist to prevent. |
Assert it: eosim --version | grep -q "$EXPECTED". Then raise the mismatch against EoSim — a v1.5.0 tag shipping version 2.0.0 is that repo's bug, not this workflow's. |
| 4 | Medium | .github/workflows/eosim-sanity.yml:160-163 |
sanity-gate fails only when needs.install-validate.result != 'success'. It then prints ✅ All EoSim sanity checks passed — which it will do with nested-simulation, nested-guest-install, windows-sanity and macos-sanity all red. Pre-existing, but this PR is what makes the workflow able to run at all, so it is the moment the gate starts mattering. |
Fail on any non-success in needs.*, or drop the "All … passed" line so the gate does not assert more than it checked. |
| 5 | Low | (whole PR) | 24 checks are green on this head and none of them execute eosim-sanity.yml — it is on: schedule + workflow_dispatch only. The green list is other workflows passing. The evidence for this change is the author's local run, which is thorough but is not CI. |
The workflow has workflow_dispatch; trigger it on the branch and link the run. That is the one piece of evidence this PR can actually produce and currently does not. |
On the analysis
The 404 is real and the cause is stated precisely: embeddedos-org/EoSim has no v0.1.0
tag and no release publishing a wheel. Installing from a tagged checkout is what eos and
ebuild already do, so this converges rather than diverges. The platforms/ explanation
holds up — PLATFORMS_DIR resolving to <site-packages>/platforms while the package
ships only eosim/platforms/__init__.py is a packaging bug in EoSim, and the copy is the
right workaround until it is fixed there.
Flagging the eosim list display bug rather than leaving it to surprise the next person
was the right instinct. Findings 1 and 4 are what follows from it: that bug is not
cosmetic once eosim list is load-bearing in three jobs.
Architecture conformance
Conforms, and §17 is the reason this matters more than a nightly-red badge. Master design
§17 makes EoSim "essential to developer onboarding and CI" and asks that "CI tests share
the same application artifacts used on real hardware"; §21 Tier 1 places EoSim in
Foundation alongside eBoot. A Tier-1 simulation gate that has never had a green run is a
gap against §17, and this PR closes the install half of it. §5.1 untouched — a CI
workflow is not a runtime dependency, and eBoot gains no edge toward EoSim in any
built artifact.
Proposed changes
:12 assert the version rather than printing it (finding 3)
:42 copy platforms/ before "Validate all platform configs"; assert count > 0
:103 drop the unpinned clone; copy from /tmp/EoSim
:122 \
:138 / same platforms/ copy + assertion for windows-sanity and macos-sanity
:160 gate fails on any non-success in needs.*
then: workflow_dispatch run on the branch, linked in the PR
Findings 1-4 are all inside the file this PR already owns, so they belong here rather
than in a follow-up — otherwise the workflow's first green run will not mean anything.
Not checked
- Nothing was run. The install sequence,
eosim doctor, and the platform counts are the
author's measurements, not mine. - I did not verify EoSim's tag list, release assets, or the
v1.5.0/2.0.0mismatch
against the EoSim repository — the localEoSimclone was skipped by the sync step
as dirty (179 files) and the brief forbids touching it. Finding 3 rests on the PR
body's own output. /tmp/EoSimas a clone target onwindows-sanity— I did not confirm how Git for
Windows and pip resolve that path under pwsh. It is plausibly fine; it is untested here
and untested in CI (finding 5).
Automated architecture review of 33404944b866 — 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.
…rged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — embeddedos-org#93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. embeddedos-org#87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) embeddedos-org#93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — embeddedos-org#86 and embeddedos-org#57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only embeddedos-org#57's public_key_is_valid_subgroup() is wired to the call site, so embeddedos-org#86's key_has_prime_order() was dead. Kept the live function, folded embeddedos-org#86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on embeddedos-org#86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight.
… check Answers the review on embeddedos-org#81. The install fix was right; the workflow it revives could not have failed for most of what it says it verifies. Finding 1 (Medium) -- the step named "Validate all platform configs" ran `eosim list && eosim doctor` with no platforms/ copy in that job. `eosim list` prints "Available platforms (0)" without it and exits 0, so the step passed having validated nothing -- and the same shape was in windows-sanity and macos-sanity. All three now copy platforms/ from the pinned checkout and assert a non-zero count via discover_platforms(), because the exit code is exactly what cannot be trusted here. Finding 2 (Medium) -- nested-guest-install cloned EoSim a second time with no --branch for its platforms/ copy, so the package came from v${EOSIM_VERSION} while the platform data came from whatever the default branch pointed at that morning. Now copies from the same pinned checkout; the second clone is gone. Finding 3 (Medium) -- `eosim --version` was printed and never asserted, while the tag and the package's declared version disagree upstream (v1.5.0 ships "eosim, version 2.0.0"). Pinning to a tag therefore does not pin what the name suggests, and nothing would have noticed if the tag moved. Now asserted. The mismatch itself is EoSim's bug and is raised there rather than worked around here. Finding 4 (Medium) -- sanity-gate failed only on install-validate and then printed "All EoSim sanity checks passed", which it would do with the other four jobs red. Replaced with the toJSON(needs) + jq body from ci.yml, which cannot fall out of step with `needs:`. embeddedos-org#90 adds a test that enforces this across every gate in the repository; this gate passes it. Verified: yaml.safe_load of eosim-sanity.yml parses, 6 jobs the gate now iterates toJSON(needs), no longer branches on install-validate alone, and no longer prints an "all passed" claim embeddedos-org#90's test_no_aggregating_gate_ignores_part_of_its_needs, run against this workflow: eosim-sanity.yml is not among its offenders pytest tests/ 47 passed ctest 21/21 PASS NOT RUN: the workflow itself. It is `on: schedule` + `workflow_dispatch` only, so none of this PR's checks execute it -- which is finding 5, and it is the one piece of evidence this PR cannot produce from a fork branch without a maintainer dispatching it. The install sequence was verified locally end to end (clone -> pip install -> eosim --version 2.0.0 -> doctor -> run am62x --headless PASSED); the assertions added here are not covered by that and remain unexecuted. Refs embeddedos-org#81
3340494 to
a28e7c5
Compare
… check Answers the review on embeddedos-org#81. The install fix was right; the workflow it revives could not have failed for most of what it says it verifies. Finding 1 (Medium) -- the step named "Validate all platform configs" ran `eosim list && eosim doctor` with no platforms/ copy in that job. `eosim list` prints "Available platforms (0)" without it and exits 0, so the step passed having validated nothing -- and the same shape was in windows-sanity and macos-sanity. All three now copy platforms/ from the pinned checkout and assert a non-zero count via discover_platforms(), because the exit code is exactly what cannot be trusted here. Finding 2 (Medium) -- nested-guest-install cloned EoSim a second time with no --branch for its platforms/ copy, so the package came from v${EOSIM_VERSION} while the platform data came from whatever the default branch pointed at that morning. Now copies from the same pinned checkout; the second clone is gone. Finding 3 (Medium) -- `eosim --version` was printed and never asserted, while the tag and the package's declared version disagree upstream (v1.5.0 ships "eosim, version 2.0.0"). Pinning to a tag therefore does not pin what the name suggests, and nothing would have noticed if the tag moved. Now asserted. The mismatch itself is EoSim's bug and is raised there rather than worked around here. Finding 4 (Medium) -- sanity-gate failed only on install-validate and then printed "All EoSim sanity checks passed", which it would do with the other four jobs red. Replaced with the toJSON(needs) + jq body from ci.yml, which cannot fall out of step with `needs:`. embeddedos-org#90 adds a test that enforces this across every gate in the repository; this gate passes it. Verified: yaml.safe_load of eosim-sanity.yml parses, 6 jobs the gate now iterates toJSON(needs), no longer branches on install-validate alone, and no longer prints an "all passed" claim embeddedos-org#90's test_no_aggregating_gate_ignores_part_of_its_needs, run against this workflow: eosim-sanity.yml is not among its offenders pytest tests/ 38 passed ctest 21/21 PASS NOT RUN: the workflow itself. It is `on: schedule` + `workflow_dispatch` only, so none of this PR's checks execute it -- which is finding 5, and it is the one piece of evidence this PR cannot produce from a fork branch without a maintainer dispatching it. The install sequence was verified locally end to end (clone -> pip install -> eosim --version 2.0.0 -> doctor -> run am62x --headless PASSED); the assertions added here are not covered by that and remain unexecuted. Refs embeddedos-org#81
a28e7c5 to
d712a65
Compare
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#81 "ci: install EoSim from a tag that exists, not a wheel that never shipped"
head: d712a65 author: Kartikey1306 ci: pass
Verdict: The EoSim install fix is right and the assertions replace two checks that
passed while validating nothing. But the PR is not the workflow-only change its body and
your comment describe — it also carries the repair for two compile errors currently on
master, in TCB crypto and in the signed-header asserts. And the new bash-only steps land
in a matrix that includes windows-latest without shell: bash, so EoSim Sanity still
cannot go green.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | .github/workflows/eosim-sanity.yml:23-30, 37-75 |
install-validate runs on os: [ubuntu-latest, windows-latest, macos-latest], and the three steps this PR rewrites are bash-only: grep -qE plus a || { …; } brace group (:45-49), SITE_PACKAGES=$(python -c …) (:57), and a python - <<'PY' heredoc (:66-75). The documented default shell on Windows runners is pwsh; VAR=$(…) and <<'PY' are parse errors there and grep is not on PATH. Before this PR the job's steps were pwsh-compatible (pip install "eosim @ …", eosim list && eosim doctor), so this is a regression on 3 of the 9 matrix cells. Combined with finding 2's stricter gate, the workflow the PR sets out to make green will still be red every morning — just for a new reason. |
Add shell: bash to the three rewritten steps in install-validate, exactly as you already did for windows-sanity:161 and macos-sanity:187. On the Windows leg also resolve site-packages as a POSIX path (python -c "import sysconfig;print(sysconfig.get_paths()['purelib'])" piped through cygpath -u) before cp -r, or the copy gets a backslash path. |
| 2 | Medium | pr body; core/ed25519_verify.c, include/eos_image.h, tests/unit/test_ed25519.c |
The body describes a CI-only change and your 2026-09-01 comment says "This PR only touches .github/workflows/eosim-sanity.yml". GitHub reports changedFiles: 4, +220/-112. Three of those files are not CI: a signature-verification source file, the public image header, and its unit test. A reviewer who reads the body will not know this PR modifies TCB crypto. |
Say in the body what the other three files do and why they are here — they are the fix for a broken master (see Architecture conformance), which is the most consequential thing in the PR and currently the least visible. If they arrived from stacking on #77, say which commits are inherited. |
| 3 | Medium | .github/workflows/eosim-sanity.yml:13, 45 |
EOSIM_VERSION: "1.5.0" and the assertion grep -qE "2\.0\.0" are two independent literals. Bumping EOSIM_VERSION leaves the assertion demanding 2.0.0 and the job fails with a message that blames the wrong thing. It also pins an upstream defect as the expected state: when EoSim makes tag v1.5.0 report 1.5.0, this workflow turns red on a fix. |
Add EOSIM_EXPECTED_VERSION: "2.0.0" next to EOSIM_VERSION with the upstream issue link in the comment, and grep for ${{ env.EOSIM_EXPECTED_VERSION }}. One place to change, and the failure message names the real cause. |
| 4 | Low | core/ed25519_verify.c:314-330 |
The surviving comment drops the line "Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached this before I did and states both conditions in one expression." The deleted key_has_prime_order carried it; public_key_is_valid_subgroup is the copy that survives and it does not. |
Keep the attribution line in the surviving comment. The code it credits is the code that stayed. |
| 5 | Low | .github/workflows/eosim-sanity.yml:37-49 |
eosim --version is run three times in the step: once bare, once in the grep pipeline, once again in the failure branch. Harmless, but the bare first call is the "print, don't assert" habit the comment above it argues against. |
Drop the bare eosim --version on :44; the grep pipeline and the failure branch already show the value. |
Architecture conformance
Master design §8.1 (signed manifests and images), §14.1 (key management reviewed as one
system across eBoot/eSec/eOTA), §5.1 (eBoot keeps the TCB minimal and auditable).
Conforms — and repairs a violation currently on master.
origin/master at 22d8f8b does not compile. Verified locally, two independent hard
errors from two merges that collided:
include/eos_image.h:135:23: error: 'eos_image_header_t' has no member named 'reserved'
include/eos_image.h:142:55: error: 'eos_image_header_t' has no member named 'reserved'
core/ed25519_verify.c:338:12: error: redefinition of 'point_is_identity'
core/ed25519_verify.c:281:12: note: previous definition ...
#93 replaced reserved[30] with tlv_len/tlv_hash while #87's assert block still pins
reserved; #57 and #76 each landed a point_is_identity and a subgroup check. gh run list --branch master confirms it: CI — eBoot, eBoot Build & Test and CodeQL are all
failure on 22d8f8b. This PR's head fixes both — the header asserts now pin tlv_len
(2 bytes) and tlv_hash (28 bytes) over the same 30-byte span, and the duplicate
point_is_identity and the uncalled key_has_prime_order are removed.
The deletion is behaviour-preserving: key_has_prime_order had no callers
(ed25519_verify.c:496 calls public_key_is_valid_subgroup), and the two bodies are the
same computation — derive the scalar from ORDER_L, scalarmult, then
point_is_identity(multiple) && !point_is_identity(A). Removing uncalled crypto from the
TCB is the §5.1 direction.
Verified by running:
gcc -Iinclude -c core/ed25519_verify.c # master: redefinition error; PR head: clean
gcc -Iinclude -c <TU including eos_image.h> # master: 4 errors; PR head: clean
gcc -Iinclude -I. tests/unit/test_ed25519.c core/ed25519_verify.c core/sha512.c
./a.out -> 14/14 tests passed, rc=0
The test-harness change is a real fix, not bookkeeping: master hardcodes tests_run = 11
next to 12 run_* calls, so the tally and the return (tests_passed == tests_run) gate
disagreed with what actually ran. tests_run++ inside TEST() ties the denominator to
execution. The low-order sweep is 8 keys × 8 R values × 8 messages and the four previously
missing encodings (y=0 sign-set, both sign-flipped order-8 points) are now present; the
non-decodable D9FF…FF vector is moved to k_non_canonical, where it exercises
unpackneg() instead of masquerading as a low-order point. That matches
.ai/security.md: reject low-order keys explicitly, and give malformed encodings their own
test rather than assuming a later check covers them.
Nothing in the diff points up a tier. eBoot depends on nothing above it here.
Proposed changes
- Add
shell: bashto the three rewritteninstall-validatesteps (finding 1). Smallest
possible diff, and without it the PR's stated goal is not reached. - On the Windows leg, emit site-packages as a POSIX path before
cp -r. - Hoist the expected version into
EOSIM_EXPECTED_VERSION(finding 3). - Rewrite the body to disclose the three non-CI files, and restore the #57 attribution
(findings 2, 4). - Consider landing the
masterrepair as its own small PR ahead of this one. It is
currently gated behind a nightly-CI change, and it is what unblocks every other open
eBoot PR.
Not checked
- The workflow was not executed.
EoSim Sanityisschedule+workflow_dispatchonly,
so it does not run on this PR andchecks.txtcontains no result for it. Finding 1 rests
on the matrix in the file plus GitHub's documented default shell for Windows runners
(pwsh), read from docs, not observed on a runner. - Whether MSYS
cpaccepts the backslash site-packages path inwindows-sanity:161-163—
it may work by path conversion. Flagged as risk, not asserted. - The upstream EoSim claims (no wheel on any release, tags
v1.0.0–v3.0.1,v1.5.0
reporting2.0.0,eosim listprinting 0 whilediscover_platforms()returns 149) were
not independently confirmed againstembeddedos-org/EoSim. - No cross-compile or on-hardware verification. Only the host
gcccompile and the
test_ed25519binary above were run;ctest, the full suite and the ARM/STM32 legs were
not. mergeStateStatus: BLOCKED,reviewDecision: REVIEW_REQUIRED— the branch protection
rule behind that was not inspected.
Automated architecture review of d712a65db7ca — 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.
3 participants
EoSim Sanityhas never had a green run here. It is a nightly workflow, so it does not gate pull requests — it is simply red every morning.The cause
Every job installs from a release asset that does not exist:
Two things are wrong with that URL.
embeddedos-org/EoSimhas nov0.1.0tag — its tags runv1.0.0throughv3.0.1— and no release of it publishes a wheel at all; the newest release's only asset is a promo video.EoSim is a normal Python project, so this now installs it the way eos and ebuild already do in their copies of this workflow: clone the tag,
pip installthe checkout.EOSIM_VERSIONnames v1.5.0, the release marked Latest.The platforms copy is load-bearing
The
platforms/copy in the nested-simulation job is kept, and now sources from the same checkout rather than a second unpinned clone. It is required, not incidental:eosim/cli/main.pyresolvesPLATFORMS_DIRto<site-packages>/platformseosim/platforms/__init__.pyso without the copy, no platform resolves.
Verified, not assumed
I ran exactly what the workflow runs:
One caveat worth recording
eosim listprintsAvailable platforms (0)even with the copy in place, whilediscover_platforms()on that same directory returns 149. That is a display bug inside EoSim, not something this workflow can fix — it exits 0, so the sanity steps pass, andinfo/runresolve platforms correctly. Flagging it rather than leaving it to surprise the next person.Relationship to #77 / #80
Independent — different workflow file, and this one is
on: scheduleonly. My earliersimulation-test.ymlchange (in eos #102) disabled its EoSim steps because the install could not work; with the install fixed, re-enabling those is a sensible follow-up.🤖 Generated with Claude Code