Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBrowser#26 "ci: run the native CMake test suite"
head: 8194c5a author: srpatcha ci: pass (7 checks green) draft
Verdict: The core move is right and overdue — the old matrix died at pip install -r requirements.txt on a repository that has no such file, so nothing it claimed to run ever ran, and replacing it with a real CMake/CTest job that uses --no-tests=error is the correct fix. But the diff does not only replace the broken Python job; it deletes Python testing from this repository entirely. There are 26 Python tests that pass today in 0.07 seconds with nothing but pytest installed, README.md:99 documents how to run them, and after this PR no workflow in the repo runs pytest at all. That is a weakened check, and the brief is explicit that it is a finding regardless of the reason given.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | .github/workflows/ci.yml, removed test job |
The Python suite loses all CI coverage. Verified: pytest tests/ -q on this head gives 26 passed in 0.07s in a bare venv with only pytest installed — tests/unit/test_unit_core.py, tests/functional/test_functional_e2e.py, tests/performance/test_performance_benchmarks.py, tests/simulation/test_emulation_simulation.py. These are live and green, not vestigial. Verified: grep -l pytest .github/workflows/*.yml returns nothing after this change — video-build.yml, release-smoke-test.yml and release.yml mention python but none of them runs pytest. And README.md:99 documents python run_all_tests.py # runs: pytest tests/unit tests/functional tests/performance tests/simulation -v as a supported entry point, so this is a documented surface going dark. The old job's actual defect was one line — pip install -r requirements.txt against a file that does not exist. Deleting the job deletes 26 working tests' coverage to fix a missing requirements file. |
Keep the native job exactly as written and add a small second job beside it, not in place of it: actions/setup-python@v5 → pip install pytest → python -m pytest tests/ -q. No requirements.txt, no -r, no continue-on-error. That is four lines and it restores everything this diff drops. Drop mypy/ruff if you like — they genuinely had nothing to check and were continue-on-error: true anyway — but the tests are a different case. |
| 2 | Low | codecov.yml |
The diff removes the only codecov/codecov-action upload, and codecov.yml is left in place declaring status.project.default.target: 100% and status.patch.default.target: 100% with require_ci_to_pass: yes. Nothing will ever produce a report for those gates to evaluate again. Brief item 11 — a change that makes existing configuration wrong is not finished. |
Either restore a coverage upload as part of finding 1's Python job, or delete codecov.yml in this PR and say so in the body. Leaving a 100% coverage gate wired to no producer is the worst of the three. |
| 3 | Low | .github/workflows/ci.yml, build job |
The three-OS matrix runs --config Release with BUILD_TESTING=OFF, so ctest never executes on macOS or Windows. The -C Debug / --config Debug handling that the body credits the independent review with adding for multi-config generators therefore runs only on ubuntu-22.04, where the generator is single-config and -C is ignored. The one thing that change exists for is the one platform it is never exercised on. |
Cheapest: add -DBUILD_TESTING=ON and a ctest --test-dir build -C Release --output-on-failure --no-tests=error step to the matrix job. That also gets the C suite onto Windows and macOS, which is the coverage the body says it is retaining but is currently compile-only. |
| 4 | Low | .github/workflows/ci.yml:5-7 |
on.push.tags: ["v*"] is retained but the release job it existed to trigger is gone. A tag push now runs test and build for nothing — release.yml already builds Linux, .deb and AppImage on the same tag, and it runs its own tests (release.yml:56). |
Drop tags: ["v*"] from this workflow's push trigger. The removal of the release job itself is correct and verified — release.yml does own packaging. |
What is right, and verified
Stated because most of this diff is an improvement and should not get lost in the findings:
- The premise holds. No
requirements.txt, nopyproject.toml, nosetup.pyanywhere in the tree. The oldpip install -r requirements.txtcould only fail, which meansRun unit testsandRun functional testsnever executed, andBuild Python Package(python -m build) could never have produced a wheel either. Four jobs' worth of green that verified nothing. --no-tests=erroris present on the new ctest step..ai/security.mdnames the exact failure this prevents: "ctestexits 0 when it finds no tests at all. A security suite that silently collected nothing is a failed check reported as green." Getting this right unprompted is the single best line in the diff.- Three
continue-on-error: truesteps are gone (ruff,mypy, the benchmark run). Those are fail-open by construction and the diff is right to remove them rather than carry them forward. permissions: contents: readadded at workflow level, consistent with the release job's removal — nothing left in this workflow needs write.- Removing the release job is justified, not just asserted.
release.ymlexists, triggers on tag push, declarescontents: write/packages: write/id-token: write, and hasbuild-linuxproducing.deband AppImage artifacts with its ownRun testsstep. The body's claim that "the dedicated release workflow remains responsible for packaging" is true. - mbedTLS is not lost on the matrix legs. I checked, because the test job passes
-DeBrowser_USE_MBEDTLS=ONand the build job does not:CMakeLists.txt:34declaresoption(eBrowser_USE_MBEDTLS "Use mbedTLS for real TLS support" ON), so the default is ON and all three OS legs still compile the production TLS backend. The explicit flag on the test job is redundant but harmless, and being explicit about the configuration under test is the right instinct.
Architecture conformance
Conforms, with one placement observation that is not this PR's to resolve.
- §21 / Tier 5.
eBrowseris Tier 5 — Applications ("Reference/advanced applications"), and §20.1 is explicit that "eBrowser and eOffice should be treated as reference or advanced applications proving platform capability, not as peer pillars of the kernel." A CI workflow change in a Tier-5 repository engages no tier rule. - §5.1. Not engaged. The diff is one workflow file: no
#include, no import, no link line, notarget_link_libraries, no manifest entry. Nothing points up or down a tier. The mbedTLS dependency is pre-existing and is a third-party library, not an EmbeddedOS tier. - §14.1, "Use reviewed cryptographic libraries; do not invent cryptographic primitives":
CMakeLists.txt:110-115FetchContents mbedTLS from upstream, which is the right shape. Out of scope here, but worth recording that the fetch is byGIT_REPOSITORY— provenance of that dependency is a §11 question this PR does not touch and I did not audit. .github/STANDARDS.md, "A claim without (2) and (3) is aspirational, not asserted": finding 2 is the local instance — a 100% coverage target with no verifying workflow is aspirational, andcodecov.ymldoes not label it as such.- No architecture proposal appended. Nothing here shows the master design wrong, stale or silent; this is a repository-local CI gap.
Proposed changes
Smallest sequence that keeps everything working:
- Add a
python-testsjob besidetest—setup-python→pip install pytest→python -m pytest tests/ -q. Norequirements.txtreference, nocontinue-on-error(finding 1). - Decide
codecov.yml's fate in the same PR: coverage upload restored, or the file deleted (finding 2). - Turn
BUILD_TESTING=ONand add acteststep to the three-OS matrix, so the multi-config path is actually exercised (finding 3). - Drop
tags: ["v*"]from thepushtrigger (finding 4).
1 and 2 are what I would hold the draft for. 3 and 4 are improvements.
Not checked
- No build or test of the C suite was run. I did not configure or build eBrowser. The body's "Debug CMake build … PASS", "CTest: 20/20 passed" and "Release CMake build: PASS" are unverified here — building would
FetchContentmbedTLS from GitHub, and I did not do that. The PR's own greenNative C testsand threeNative buildchecks are the evidence for those claims, not anything I ran. Note the count discrepancy worth confirming: the body says 20 registered tests, and I did not countadd_testentries to check it. - Windows and macOS — NOT RUN. Finding 3 is reasoning about which steps execute where, from the YAML. I did not run either leg.
- The 26 Python tests were run, but only on Linux with the venv's CPython, not across the 3.10/3.11/3.12 range the old matrix nominally covered. They pass; I did not check what they actually assert or whether they are meaningful tests, only that they exist, run, and are green — which is what finding 1 turns on.
- Independence. The PR author is the same account that operates this autoreview pipeline.
.ai/reviewer.mdsays an implementer does not approve their own work; this pipeline never approves anything and this comment is not an approval, but a reader should not count it as independent human review either. - Draft. This PR is marked draft and was last updated 2026-09-10. Findings 1 and 2 may already be intended before it is marked ready; they are recorded rather than assumed.
- I did not audit the other 15 workflows for overlap with the new jobs beyond grepping them for
pytestand readingrelease.yml's job list.
Automated architecture review of 8194c5a40edb — 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.
Summary
The previous workflow failed during
actions/setup-pythonbecause this C repository has neitherrequirements.txtnorpyproject.toml, so no project test executed.Validation
BUILD_TESTING=ON, mbedTLS enabled, standalone disabled: PASSgit diff --check: PASSFixes #29