Conversation
…y jobs Opened by the scheduled autoreview pipeline after review of open PRs. Reviewed against the EmbeddedOS Master Design v2.0. Files: .github/workflows/eosim-sanity.yml
srpatcha
left a comment
There was a problem hiding this comment.
Review — ebuild#121 "fix: install EoSim from the pinned tag in the Windows and macOS sanity jobs"
head: 5aa5e79 author: srpatcha ci: fail (CI Gate + 9 × Test (Python …); unrelated — see finding 4) draft
Independence, first. This PR was opened by this same autoreview pipeline (the body says so). .ai/reviewer.md — "if you implemented it, you do not approve it" — applies. This comment is a comment and never an approval, so the structural boundary holds, but nothing below should be read as independent sign-off. A human decides this one. I have deliberately not softened anything.
Verdict: Every factual claim in the body checks out, including the release-asset survey, and the diagnosis is exactly right — no EoSim release has ever carried a wheel, so that URL could never resolve. But the fix makes two jobs pass that, once fixed, do literally nothing the install-validate matrix does not already do on the same runner images with the same commands; and the workflow has no pull_request trigger, so the change cannot be exercised before it merges. The honest scope-out of the fail-open gate is the right instinct applied to the wrong boundary: this fix has no observable effect until that gate is fixed.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | .github/workflows/eosim-sanity.yml:3-6 |
The change cannot be validated before merge. The on: block is schedule (0 4 * * *) and workflow_dispatch only — no pull_request, no push. Verified on both this head and origin/master. The 33 checks reported on this PR all come from ci.yml, codeql.yml, simulation-test.yml and auto-assign.yml; Windows Sanity and macOS Sanity do not appear among them, because the workflow this PR edits never ran. A CI-repair PR whose repair cannot be observed until the next 04:00 UTC nightly on master is landing blind — and the body's "Expected impact" section is, by construction, a prediction. |
Before marking ready: trigger the workflow on this branch with workflow_dispatch and link the run. That is available today and settles it. The durable fix is a path-filtered trigger — pull_request: paths: ['.github/workflows/eosim-sanity.yml'] — which is the pattern the eos repository already uses for the same workflow and which its test_ci_gate.py already classifies as NOT_REQUIRED for exactly this reason. |
| 2 | Medium | .github/workflows/eosim-sanity.yml:105-140 vs :23-44 |
After this fix the two jobs are redundant. install-validate runs the matrix [ubuntu-latest, windows-latest, macos-latest] × [3.10, 3.11, 3.12]. windows-sanity is windows-latest + Python 3.12; macos-sanity is macos-latest + Python 3.12 — each identical to a leg that already exists. The install step this PR adds is now byte-identical to install-validate's (:35-38). And the commands are the same set: install-validate runs eosim --version, eosim list (:40-42) then eosim list && eosim doctor (:44); the sanity jobs run eosim --version, eosim list && eosim doctor, eosim list. Same three assertions, different order. Brief item 10 — logic the diff adds that already exists elsewhere, where one call site would do. |
Delete windows-sanity and macos-sanity instead of repairing them, and remove them from sanity-gate's needs. That is a smaller diff than this one, removes two runner-minutes from every nightly, and loses no coverage — install-validate already asserts all of it on both platforms. If there is a reason these must be separate jobs, the PR should say what it is, because I could not find one in the file. |
| 3 | Medium | .github/workflows/eosim-sanity.yml:154-163 |
The PR scopes out the fail-open sanity-gate as "a separate defect … belongs in its own change". The disclosure is right and the reasoning is not: the gate ignores windows-sanity and macos-sanity results entirely (if [ "${{ needs.install-validate.result }}" != "success" ] is the only condition, then it unconditionally prints ✅ All EoSim sanity checks passed), so this fix produces no gating value whatsoever until the gate changes. Worse, once these jobs install successfully, a future break in them — a moved tag, a GitHub rate limit, an EoSim packaging change — will still report green. The two changes are not independent; one is inert without the other. This is precisely the fail-open shape .ai/security.md names ("A verification step that cannot run must fail, not pass") and that the eos repo eliminated with .github/scripts/ci-gate-check.sh, which refuses any result other than success. |
Do the gate in this PR or in the very next one, and say which in the body. eos's ci-gate-check.sh is a working, tested implementation of the rule — printf '%s' "$RESULTS" | .github/scripts/ci-gate-check.sh over toJSON(needs). Note that taking finding 2 does not dissolve this: nested-simulation and nested-guest-install results are ignored too. |
| 4 | Low | CI state on this head | The 10 red checks are not this PR's doing, recorded so the state is not misread. I pulled the log for Test (Python 3.10, ubuntu-22.04): it fails at ruff check . with Found 4 errors. [*] 2 fixable with the --fix option. — the same four pre-existing findings in tests/unit/test_build_dir_resolution.py, test_package_recipe.py and test_ci_gate.py that I measured independently on origin/master while reviewing #124. ci.yml runs ruff before the test suite, so every matrix leg dies there and CI Gate follows. Open PR #122 ("ci: fix the lint findings that stop CI before any test runs") is the fix. |
Nothing to do here — but this PR cannot go green until #122 lands, which is worth saying in the body so a reader does not spend time on it. |
| 5 | Low | .github/workflows/eosim-sanity.yml:13 |
EOSIM_VERSION: "1.5.0" is pinned to the release GitHub marks Latest, and the comment explains why. Worth recording what that survey turned up: v3.0.1 was released 2026-05-16, before v1.0.0 on 2026-05-27. The tag history runs v3.0.1 → v1.0.0 → v1.1.0 → … → v1.5.0, so "Latest" is a GitHub flag here, not a version ordering. .github/STANDARDS.md requires SemVer for canonical product repos, and a repo whose highest tag predates its 1.0.0 does not satisfy it. The consequence for this workflow: the sanity suite validates a version whose relationship to what a consumer would resolve as newest is undefined. |
Out of scope for this PR — do not grow it. Worth an issue against embeddedos-org/EoSim, and worth one sentence in the EOSIM_VERSION comment saying the pin is to the Latest-flagged release, not the highest tag, since that is the surprising part. |
Claims verified
All four of the body's factual claims hold. Checked rather than taken on trust, because the pipeline wrote them:
| Claim | Result |
|---|---|
"embeddedos-org/EoSim does not publish wheels … No release in the repository has ever attached a .whl" |
Confirmed across all eight releases. v1.5.0: EoSim_v1.5.0_promo.mp4 · v1.4.0: EoSim_v1.4.0_promo.mp4 · v1.3.1: EoSim_v1.3.1_promo.mp4, manifest.json · v1.3.0: same · v1.2.0, v1.1.0, v1.0.0, v3.0.1: promo mp4 only. Not one .whl. The URL was unresolvable for every value of EOSIM_VERSION, exactly as claimed. |
"The other three jobs … already install by cloning the tag (:38, :69, :97) and all pass" |
Confirmed at those line numbers; the clone-and-pip install -e step is identical in all of them. |
"sanity-gate (:154) exits non-zero only when install-validate fails" |
Confirmed — :160-163. See finding 3. |
"/tmp/EoSim … install-validate already clones to exactly that path on windows-latest" |
Confirmed (:36). Using the POSIX-style path verbatim on Windows is correct here precisely because a green job already proves Git Bash resolves it. |
Architecture conformance
Conforms; nothing structural is engaged.
- §21 / Tier 1.
ebuildis Tier 1 — Foundation. §17 makes EoSim an adoption primitive and §21 places it in Tier 1 alongsideeos,eBootandebuild, so eBuild's CI validating EoSim is peer-to-peer within a tier, not an upward dependency. - §5.1. Not engaged. The diff is a workflow file: no
#include, no import, no link line, notarget_link_libraries, no manifest entry. "eBuild understands the complete graph but is not a runtime dependency" is undisturbed — this is CI-time, and nothing here ships. - §9.2, "Reproducible lockfiles/manifests for production builds": cloning
--branch v1.5.0 --depth 1pins to a tag, which is reproducible only insofar as the tag is immutable..github/STANDARDS.mdstates tags are immutable, so the pin is sound. Installing from a git tag rather than a published artifact does mean there is no checksum and no provenance on what gets installed — a §11 concern, pre-existing in the nine passing jobs, and not made worse by this PR. - No architecture proposal appended. Nothing here shows the master design wrong, stale or silent. Finding 5 is a repo failing an existing standard (
STANDARDS.mdSemVer), not a gap in the design — the standard is clear, EoSim's tags do not meet it.
Proposed changes
Smallest sequence that keeps things working, in order:
- Decide finding 2 first, because it changes everything else: if
windows-sanityandmacos-sanityare deleted, this PR's diff disappears entirely and the change becomes a deletion. I think that is the right answer and I could not find an argument against it in the file. - If they are kept: fix
sanity-gatein the same PR (finding 3), otherwise the install fix gates nothing. - Either way, prove it before merge —
workflow_dispatchon this branch, link the run (finding 1). - Note in the body that the red checks are #122's, not this PR's (finding 4).
Not checked
- Nothing in this workflow was executed. No
git cloneof EoSim, nopip install -e, noeosim --version,eosim listoreosim doctoron any platform. The entire "Expected impact" section is unverified by me and, per finding 1, by CI. I checked that the other jobs using this install method are green in GitHub's reported status; I did not run them. - The body's own verification table is unverified.
eosim-install"pass, 3s,/tmp/eosim_install_check.sh" and theyamllintrun were recorded by the fix pipeline in a worktree that no longer exists. I did not re-run either. I did not runyamllintat all — it is not installed on this host. eosim listreportingAvailable platforms (0)— NOT VERIFIED, and it is the most important thing in the PR. The body reports thateosim listprints zero platforms and exits 0 whileeosim doctorreports 149 in the same session, which would make every step named "Validate all platform configs" incapable of failing — actest-without---no-tests=errorin a different costume, across nineinstall-validatelegs plus these two. I could not confirm it: the localEoSimclone has a dirty working tree (179 files) and was left untouched per the rules of engagement, and I did not install EoSim. If that observation is right it is a higher-severity finding than anything in this PR, and it should be filed as its own issue with evidence rather than left in a PR body.- Windows and macOS — NOT RUN. No such host.
- I did not check whether
simulation-test.ymloverlaps these jobs as well; finding 2 compareseosim-sanity.ymlagainst itself only. mergeStateStatusandmergeableare bothUNKNOWNon this head, so I could not determine whether the branch conflicts with currentmaster. It is a draft, last updated 2026-09-10.
Automated architecture review of 5aa5e7944d25 — 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.
CI -- ebuild has been red on master since the 09-08 batch merge, and the first failing step (ruff) has hidden the ones behind it. Lint (ruff, all nine Test legs): - test_build_dir_resolution.py imported shutil twice (F811). - test_package_recipe.py lost its trailing newline (W292). - test_ci_gate.py had `import itertools` / `import re` two hundred lines down (E402) -- my own embeddedos-org#103, replayed onto a file that had moved. These three hunks are byte-identical to embeddedos-org#122's, so either PR merging first leaves the other clean. Type check and tests (never reached on master since 09-08): - ebuild/packages/index_sync.py calls PackageRecipe.to_dict(), which embeddedos-org#111 defined and embeddedos-org#112 -- merged five minutes later from a base without it -- deleted in its replay. mypy names it once; pytest fails nine test_index_sync cases with AttributeError. The method is restored verbatim from embeddedos-org#111 (cc90078): it emits the `package:`/`build:` keys parse_recipe() reads back, which an asdict() replacement would not. Vendored core drift: - embeddedos-org#109 (dba3d83) edited core/eos/docs/three-way-alignment.md, a vendored copy pinned to eos 5544c98, so drift went 44 -> 45 and the guard failed as designed. Reverted to the pinned content (blob 7f9c8c1, the same bytes as eos:docs/three-way-alignment.md at the pin). The alignment note belongs in ebuild's own docs or upstream in eos, not in the snapshot. OSSF Scorecard: - ossf/scorecard-action@v2.4.0 pulls gcr.io/openssf/scorecard-action, and gcr.io now refuses the pull ("requires billing to be enabled"). v2.4.3 pulls from ghcr.io; eos already pins it and its Scorecard job is green. Not in this PR: EoSim Sanity's Windows/macOS legs install a wheel that has never been published; embeddedos-org#121 (srpatcha) already replaces that with the clone the other legs use. Verified locally: ruff clean, yamllint clean, mypy clean over 107 files, 680 passed / 1 skipped, scripts/check_vendor_drift.py 44/44 and 46/46.
|
All ten red checks here are inherited from Each
Verified by building the merge result locally rather than inferring it:
Nothing is needed here. Once #132 lands, a rebase or merge of |
CI -- ebuild has been red on master since the 09-08 batch merge, and the first failing step (ruff) has hidden the ones behind it. Lint (ruff, all nine Test legs): - test_build_dir_resolution.py imported shutil twice (F811). - test_package_recipe.py lost its trailing newline (W292). - test_ci_gate.py had `import itertools` / `import re` two hundred lines down (E402) -- my own embeddedos-org#103, replayed onto a file that had moved. These three hunks are byte-identical to embeddedos-org#122's, so either PR merging first leaves the other clean. Type check and tests (never reached on master since 09-08): - ebuild/packages/index_sync.py calls PackageRecipe.to_dict(), which embeddedos-org#111 defined and embeddedos-org#112 -- merged five minutes later from a base without it -- deleted in its replay. mypy names it once; pytest fails nine test_index_sync cases with AttributeError. The method is restored verbatim from embeddedos-org#111 (cc90078): it emits the `package:`/`build:` keys parse_recipe() reads back, which an asdict() replacement would not. Vendored core drift: - embeddedos-org#109 (dba3d83) edited core/eos/docs/three-way-alignment.md, a vendored copy pinned to eos 5544c98, so drift went 44 -> 45 and the guard failed as designed. Reverted to the pinned content (blob 7f9c8c1, the same bytes as eos:docs/three-way-alignment.md at the pin). The alignment note belongs in ebuild's own docs or upstream in eos, not in the snapshot. OSSF Scorecard: - ossf/scorecard-action@v2.4.0 pulls gcr.io/openssf/scorecard-action, and gcr.io now refuses the pull ("requires billing to be enabled"). v2.4.3 pulls from ghcr.io; eos already pins it and its Scorecard job is green. Not in this PR: EoSim Sanity's Windows/macOS legs install a wheel that has never been published; embeddedos-org#121 (srpatcha) already replaces that with the clone the other legs use. Verified locally: ruff clean, yamllint clean, mypy clean over 107 files, 680 passed / 1 skipped, scripts/check_vendor_drift.py 44/44 and 46/46.
|
Update: the lint fix landed on |
Problem
EoSim Sanityhas two red jobs on every nightly run. In the most recent run(34436907943)
20 of 22 jobs passed and exactly two failed: Windows Sanity and macOS Sanity.
Both die on their first command:
Root cause
.github/workflows/eosim-sanity.yml:116and:132install EoSim from a releasewheel asset:
pip install "eosim @ https://github.com/embeddedos-org/EoSim/releases/download/v${{ env.EOSIM_VERSION }}/eosim-${{ env.EOSIM_VERSION }}-py3-none-any.whl"embeddedos-org/EoSimdoes not publish wheels. Thev1.5.0release carries oneasset,
EoSim_v1.5.0_promo.mp4;v3.0.1likewise carries onlyEoSim_v3.0.1_promo.mp4. No release in the repository has ever attached a.whl, so this URL cannot resolve for any value ofEOSIM_VERSION— bumpingthe version would not help.
The other three jobs in this same workflow —
install-validate(9 legs acrossUbuntu, Windows and macOS),
nested-simulationandnested-guest-install—already install by cloning the tag (
:38,:69,:97) and all pass, includingthe three Windows legs and the three macOS legs. Only these two jobs were left
on the wheel URL.
The fix
Give
windows-sanityandmacos-sanitythe sameInstall EoSim from sourcestep the passing jobs use, and leave their existing assertions untouched:
The
/tmp/EoSimpath is used verbatim rather than a Windows-native path becauseinstall-validatealready clones to exactly that path onwindows-latestandthose legs are green.
Files changed
.github/workflows/eosim-sanity.yml— two jobs, install step only.Expected impact
The two failing jobs get a working EoSim, so their
eosim --version,eosim listandeosim doctorassertions actually run instead of aborting atinstall. Nothing else in the workflow changes.
Risks and compatibility
Low. Workflow-only; no product code, no public interface, no dependency change.
The change makes two jobs behave like nine jobs that already pass on the same
runner images.
Two things this PR deliberately does not do, recorded so they are not
mistaken for fixed:
sanity-gate(:154) exitsnon-zero only when
install-validatefails;windows-sanity,macos-sanity,nested-simulationandnested-guest-installresults areprinted and ignored. Run 34436907943 therefore reported
✅ All EoSim sanity checks passedwith two jobs red. That is a separatedefect in the same file and belongs in its own change rather than mixed into
an install fix.
eosim listasserts nothing. Observed while verifying this change:eosim listprintsAvailable platforms (0)and exits 0, even thougheosim doctorreportsTotal platforms 149in the same session. The stepsnamed "Validate all platform configs" cannot fail on an empty list. Also out
of scope here.
Evidence
.ai/autoreview/state/maint/20260910T000629/ebuild.md§"CI — latest run on master" (
failure EoSim Sanity).gh release view v1.5.0 --repo embeddedos-org/EoSim --json assets.Verification
Executed in an isolated worktree branched from
origin/master:eosim-install/tmp/eosim_install_check.shyamllint/tmp/eosimv/bin/yamllint -c .yamllint.yml .github/workflows/eosim-sanity.ymlOpened by the scheduled autoreview pipeline (model
claude-opus-5), branched fromorigin/master. No human has reviewed this yet. Close it freely if the fix is wrong - a bad automated PR is a bug worth reporting.Fixes #128