ci: run the build-and-test workflow on master - #31
Conversation
ci.yml watched `main` and `develop`. Neither exists — this repository's default branch is `master`, so every push to it and every pull request against it fell outside the trigger. Last run of ci.yml: 2026-05-31. A repository-wide rename from main to master in late May left the workflow pointing at a branch that had gone, and nothing has built or tested a change here since. master is added rather than substituted, on both push and pull_request, so a rename in either direction does not break this again. Expect the first run to be red. Three months of changes have landed unverified; finding that out is the point. Same fix as embeddedos-org/eAI#39, where it is verified to work: the PR went from a single skipped `assign` job to `C/C++ Tests` and `Python Tests` actually running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI is running on this PR, and it immediately surfaced failures that have been Same result across all six repositories where the trigger was orphaned:
Nine failing checks across six repositories, none of which anyone could see eAI's has been diagnosed (embeddedos-org/eAI#40): the test suite imports pip install -r requirements.txt 2>/dev/null || truewhere On merging this while it is redThe red reflects reality; the green before it did not. My preference is to merge If you would rather land a green |
srpatcha
left a comment
There was a problem hiding this comment.
Review — eIPC#31 "ci: run the build-and-test workflow on master"
head: f77dd17 author: srpatcha ci: fail — Build & Test (Linux x86_64) fails; Cross-compile ARM Cortex-M4 and Create GitHub Release skipped; 6 others pass
Verdict: The two-line change is correct and I would merge it. The framing around it is not: this is not a repository whose gate went red from three months of unwatched drift. ci.yml's test job runs cmake -B build/host at the repository root, and this repository has never had a root CMakeLists.txt — git log --all --diff-filter=AD -- CMakeLists.txt is empty across the whole history. eIPC is a Go module with a C SDK in a subdirectory. So restoring the trigger restores a workflow that cannot pass as written, and Build & Test (Linux x86_64) will stay red after every fix to the Go code, because the failure is in Configure (host) before any test runs. Separately, the sweep that found six repositories missed three, and missed two non-ci.yml workflows with the identical defect — including one in eApps, the repository the body holds up as the counter-example.
I am not restating the existing comment's points (the red-vs-honest-gate argument, the required-status-checks gap, the || true pattern in eAI). Finding 3 answers the open question it left for a reviewer.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | .github/workflows/ci.yml:32-40 |
Build & Test (Linux x86_64) cannot pass, and not for a reason any code change will fix. The Configure (host) step runs cmake -B build/host -G Ninja … with the working directory at the repository root. There is no CMakeLists.txt there, and there never has been: git log --oneline --all --diff-filter=AD -- CMakeLists.txt returns nothing, and find . -name CMakeLists.txt -not -path './.git/*' returns exactly two, both under sdk/c/ (sdk/c/CMakeLists.txt, sdk/c/tests/CMakeLists.txt). go.mod declares module github.com/embeddedos-org/eipc, go 1.22; the top-level source directories are cmd/, core/, protocol/, transport/, services/, security/, sdk/. This is a Go repository with a C SDK subproject, and ci.yml was written for a C/CMake project rooted at the top. The body says "finding out what broke is the point of turning it back on" — but nothing broke here; the workflow never fitted. That distinction decides what the next PR has to do. |
Point the C build at the subproject that has one, and add the Go gate that is missing (finding 2). Minimum viable change to ci.yml: cmake -B build/host -S sdk/c … and ctest --test-dir build/host. Keep this PR as the two-line trigger fix and do the workflow rewrite separately — they are different changes and mixing them makes the trigger fix un-revertable. |
| 2 | High | .github/workflows/ci.yml:15-58 |
No job in ci.yml runs go test, so the majority of the repository has no CI gate even once the trigger is fixed. The test job's steps are apt install, cmake configure, cmake build, ctest, pytest tests/, codecov. There is no actions/setup-go, no go build, no go vet, no go test — grep -n "setup-go|go test|go build|go vet" .github/workflows/ci.yml returns nothing. Meanwhile Makefile:37 defines a test target and is never invoked, and tests/ contains integration_test.go, security_test.go and stress_test.go alongside the Python suites. Git history shows this was not always so: de37d10 (2026-04-28, "fix: ensure continue-on-error on all CI jobs") shows the then-current ci.yml had setup-go, a fuzz job, and a build-c-sdk job that did cmake -B build inside sdk/c or fell back to make. 4d99ce0 (2026-05-27, "feat: production-ready v1.4.0 — real source code, real tests, CI pipelines", +147/-17 on this file) replaced it with the current CMake-at-root template. The Go gate was removed four days before the branch rename hid the result. |
Add a Go job: actions/setup-go@v5 with go-version from go.mod, then go build ./..., go vet ./..., go test ./... -race. Makefile already has test, vet and lint targets — calling make test vet keeps one definition of the commands instead of two. This is the check that would actually have caught three months of drift. |
| 3 | High | .github/workflows/ci.yml:104-113 |
Static Analysis (cppcheck + clang-tidy) reports pass without analysing anything, and half of what its name promises is never invoked. Answering the question the existing comment left open — eIPC's ci.yml does not contain eAI's pip install -r requirements.txt 2>/dev/null || true; line 29 installs pytest pytest-cov directly, so that specific failure mode is absent here. The equivalent is worse. The cppcheck step is cppcheck --enable=all --error-exitcode=1 … -I include/ src/ with continue-on-error: true on the step. Two independent defects: (a) neither include/ nor src/ exists in this repository (ls -d include src → both "No such file or directory"; they have never existed, and the real C sources are sdk/c/src/), so cppcheck is given no input and cannot report on any file; (b) --error-exitcode=1 is explicitly asking cppcheck to fail the step, and continue-on-error: true then discards that exit code — the two lines cancel out. checks.txt shows this check green in 13s, which is about what an apt install plus a no-op costs. .ai/reviewer.md names "a verification whose result is discarded" as a finding regardless of the reason, and this is one twice over. clang-tidy is apt-installed at line 105 and never run; the job name asserts a tool that does not execute. |
Point cppcheck at sdk/c/src with -I sdk/c/include (confirm those paths first), and delete continue-on-error: true so --error-exitcode=1 means something. Either add a clang-tidy step or rename the job to Static Analysis (cppcheck). If cppcheck's output is not yet clean, gate it to changed files or record a baseline — do not keep a green check that inspects nothing. |
| 4 | Medium | .github/workflows/ci.yml:76-84 |
Cross-compile ARM Cortex-M4 references a toolchain file that has never existed. Configure (ARM) passes -DCMAKE_TOOLCHAIN_FILE=cmake/arm-cortex-m4.cmake. There is no cmake/ directory in the repository and git log --all -- cmake/ is empty. The job currently reports skipping only because needs: test and test fails; once finding 1 is fixed it will run and fail here instead. Worth knowing now, because "fix the failing check" will otherwise look like whack-a-mole. |
Add the toolchain file, or drop the job until there is an ARM target to build. A cross-compile job that has never once compiled is not evidence of portability, and §22 requires "Regular build/simulation" before a target may be called Validated. |
| 5 | Medium | PR body, the six-repo table | The sweep is incomplete: three more repositories have the same orphaned ci.yml, and two workflows other than ci.yml have it too — one of them in eApps, which the body names as the repository that "got it right". I read .github/workflows/* from origin/<default-branch> in all 20 local clones and reported every pre-jobs: branches: filter that omits master. Still orphaned on the default branch right now: eAI/.github/workflows/ci.yml [main, develop]/[main] (PR #39 open); eAI/.github/workflows/cross-platform-hal.yml [main, develop] (no PR); eApps/.github/workflows/ci-native.yml [main]/[main] (no PR — eApps/ci.yml is indeed correct, so the body's claim holds for that one file and not for the repository); eCAD-Hardware-Products/.github/workflows/ci.yml [main, develop]/[main] (no PR); embeddedos-org.github.io/.github/workflows/ci.yml [main, develop]/[main] (no PR); eosllm/.github/workflows/ci.yml [main, develop]/[main] (no PR); and this repository, which this PR fixes. eDB, eBrowser and eOffice are already clean on origin/master, so their fixes landed. Every clone's default branch is master and none has an origin/main (git show-ref refs/remotes/origin/main fails in all 20). eos-aero has no workflows on origin/master; eos-health has none on origin/main. The eCAD gap matters most: eCAD#20 and eCAD#22 in this same review batch are both about a pytest failure in a repository whose test workflow has never run, and neither PR fixes the trigger. |
Extend the sweep rather than the table: for each repo, for f in .github/workflows/*; do check every pre-jobs: branches: list, not just ci.yml. Then open the four remaining PRs (eAI ×1 more, eApps, eCAD-Hardware-Products, embeddedos-org.github.io, eosllm). A repeatable check belongs in the org .github repo so the next rename cannot reopen this — that is the durable version of the "mark it required" suggestion already in the body. |
Verified clean, and these are the parts of a trigger fix that most often go wrong:
- The change is additive in both places, exactly as claimed.
push: branches: [master, main, develop],pull_request: branches: [master, main]—masterprepended, nothing removed,tags: ["v*"]untouched. A rename in either direction now leaves a working trigger, which is the property the body says it wanted. - The trigger fix demonstrably worked.
checks.txton this head carriesBuild & Test (Linux x86_64),Static Analysis (cppcheck + clang-tidy),Cross-compile ARM Cortex-M4andCreate GitHub Release— jobs fromci.yml, which by definition could not have appeared on a PR before this change. The mechanism is proven; findings 1–4 are about what it now exposes. - It is a two-line diff in one file, with no behaviour change smuggled in.
files.txtis2+ 2- .github/workflows/ci.yml;pr.jsonagrees (changedFiles: 1)..ai/architect.mdforbids restructuring and changing behaviour in one commit, and this PR keeps to that — which is why I would merge it rather than fold findings 1–4 into it. - The
pytest tests/step is correctly targeted.tests/unit/,tests/functional/,tests/performance/andtests/simulation/all exist with__init__.pyandtest_*.pyfiles, so onceConfigure (host)stops failing, that step has real suites to run.
Architecture conformance
Conforms. §21 places eIPC in Tier 2 — Core Platform ("Communication, security, connectivity and lifecycle"), and .github/workflows/ is §21's Infrastructure row ("Governance, release automation and documentation"). §5.1 is not engaged: no #include, import, link line, target_link_libraries entry or manifest dependency changes, and CI is host-side, never a runtime dependency. §21.1 is not engaged; nothing moves.
The design bears on the findings in two places, and in both the design is right and the repository is not, so no proposal is appended:
- §12 eIPC Redesign splits the subsystem into "EoS IPC Core" (§12.1 — queues, mailboxes, shared memory, events, local RPC) and "eIPC Fabric" (§12.2 — UART/CAN/TCP/shared transport), and closes with "Small MCUs must not be forced to carry a gateway-class communication runtime." That split is precisely what findings 1–2 leave unverified: the C SDK under
sdk/c/is the small-target side and its CMake project is never built by CI, while the Go side is the fabric/gateway side and has no gate at all. §12's central constraint has no check behind it. - §28 Status, Evidence and Claims Policy requires
Implementedto carry "Code and functional tests" andValidatedto carry "Hardware/CI/test reports/benchmarks". Finding 3's green-but-vacuous static-analysis check is the case the already-appended §28.2 proposal inproposals/2026-09.md("The evidence policy is silent on checks that verify nothing", triggers eAI#39/#41/eBoot#81) was written for. This is another instance of it, not a new gap, so I have added no duplicate proposal. Finding 4's never-run cross-compile job is §22's Validated tier asserted without the "Regular build/simulation" it requires.
Proposed changes
- Merge this PR as-is. An honest red gate beats an invisible green one, and the existing comment already argues that; findings 1–4 are reasons the follow-up is bigger than expected, not reasons to hold this.
- Rewrite
ci.yml'stestjob for what this repository is: a Go job (setup-go,go build/vet/test ./... -race, ormake test vet) plus a C job rooted atsdk/c(findings 1–2). This is the change that turns the gate from unpassable to meaningful. - Fix the static-analysis job: real paths, drop
continue-on-error: true, and either run clang-tidy or stop naming it (finding 3). - Add
cmake/arm-cortex-m4.cmakeor remove the ARM job (finding 4). - Open the five remaining orphaned-trigger PRs and add a repeatable org-level check over all workflows, not just
ci.yml(finding 5). Independent of 1–4 and the cheapest item on this list.
Order: 1 now, then 5 (it is mechanical and unblocks eCAD#20/#22's test claims), then 2–4 as one workflow rewrite. Marking the check required — already suggested in the body — must wait until after 2, or it will block every PR on a Configure (host) failure.
Not checked
- I did not read the failing run's log. The
Build & Test (Linux x86_64)attribution toConfigure (host)is derived from the workflow file plus the absence of a rootCMakeLists.txtin the working tree and in the full history, not from the job output atactions/runs/33359520394/job/99388047193, which I did not fetch. It is the first step that must fail, but I have not excluded an earlier failure inInstall dependencies. - Nothing was built, compiled or tested. No
cmake, noctest, nogo test, nopytest. I do not know whethersdk/cbuilds, whether the Go suites pass, or what cppcheck would say if aimed at real files. Findings 1–4 are about what the workflow can and cannot do, not about the state of the code. - cppcheck is not installed on this host (
command -v cppcheck→ nothing), so finding 3(a) rests oninclude/andsrc/being absent rather than on an observed cppcheck error message. The conclusion that the step analyses nothing follows from the missing input path; the exact exit code and message are inferred. - Run history is unverified. "the build-and-test workflow has not run on a change since 2026-05-31" and the per-repo "last run" column are the body's claims; I did not query the Actions API. What I did verify is the mechanism that would cause it (orphaned filters, no
origin/mainin any clone) and the 2026-05-27 workflow replacement in4d99ce0four days before that date. Whether the four runs in that window passed or failed, I do not know. - Finding 5 covers the 20 local clones only, and reads
origin/<default>as of this run's fetch. The organisation has 26 repositories by the sibling PR bodies' count, so up to six were not examined. I also matched onlybranches:lists appearing beforejobs:; a filter inside a reusable-workflow call or apaths-only trigger would not be caught, and I did not checkworkflow_run,scheduleorworkflow_dispatcharms. required_status_checks: nulleverywhere is the body's claim about branch protection; that is org-admin state I did not query.mergeStateStatus: BLOCKED,mergeable: MERGEABLE,reviewDecision: REVIEW_REQUIRED. No merge attempted, nothing pushed. The clone sits onfix/ci-runs-on-master; the sync step reported it clean, I read history and file layout throughgit log/git show/git archiveinto a temp directory, and the working tree is unchanged.
Automated architecture review of f77dd17666d3 — 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.ymlwatchedmainanddevelop. Neither exists — this repository's defaultbranch is
master:So every push to
masterand every pull request against it falls outside thetrigger, and the build-and-test workflow has not run on a change since
2026-05-31.
A repository-wide rename from
maintomasterin late May left the workflowpointing at a branch that had gone. The same thing happened in six repositories
at once:
[main, develop][main, develop][main, develop][main, develop][main, develop][main, develop]eApps is the one that got it right:
[main, master, develop].The change
masteris added rather than substituted, on bothpushandpull_request, so a rename in either direction does not break this again. YAMLvalidated.
Verified on eAI first
embeddedos-org/eAI#39 is the same change, and it demonstrably works — that PR
went from a single skipped
assignjob toC/C++ TestsandPython Testsactually running.
Expect the first run to be red
Three months of changes have landed here with no build or test gate. Finding out
what broke is the point of turning it back on; it is not a regression introduced
by this PR.
Worth doing next
No repository in the organisation has a required status check
(
required_status_checks: nulleverywhere). That gap let non-compiling codereach
masterin eos and an unparseable file reachmasterin ebuild. Once thisworkflow is green again, it is the obvious candidate to mark required.
This is the third hardcoded-name failure found this week, after
embeddedos-org/ebuild#81 (
"branch": "main"for repositories whose default ismaster) and embeddedos-org/EoSim#16 (a lowercase repo list that found 2 of 19on a case-sensitive filesystem).